Add feature to share an I2C serial engine from two subsystem(SS) so that
individual client from different subsystem can work on the same bus.
This is possible in GSI mode where driver queues the TRE with required
descriptors and ensures it executes them in a mutual exclusive way.
Add Lock TRE at the start of the transfer and Unlock TRE at the end of
the transfer protecting the DMA channel. This way not allowing other SS
to queue anything in between and disturb the data path.
Since the GPIOs are also shared for the i2c bus, do not touch GPIO
configuration while going to runtime suspend and only turn off the
clocks. This will allow other SS to continue to transfer the data.
To realize this, add below change:
1) Check if the Particular I2C requires to be shared during probe() time.
2) If shared SE add LOCK TRE inside gpi_create_i2c_tre() before first
message.
3) If shared SE add UNLOCK TRE inside gpi_create_i2c_tre() before
last transfer message.
4) Export function geni_se_clks_off() to call explicitly instead of
geni_se_resources_off().
Signed-off-by: Mukesh Kumar Savaliya <[email protected]>
---
drivers/dma/qcom/gpi.c | 33 +++++++++++++++++++++++++++++-
drivers/i2c/busses/i2c-qcom-geni.c | 24 ++++++++++++++++------
drivers/soc/qcom/qcom-geni-se.c | 4 +++-
include/linux/dma/qcom-gpi-dma.h | 6 ++++++
include/linux/soc/qcom/geni-se.h | 3 +++
5 files changed, 62 insertions(+), 8 deletions(-)
diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
index 1c93864e0e4d..df276ccf9cbb 100644
--- a/drivers/dma/qcom/gpi.c
+++ b/drivers/dma/qcom/gpi.c
@@ -2,6 +2,7 @@
/*
* Copyright (c) 2017-2020, The Linux Foundation. All rights reserved.
* Copyright (c) 2020, Linaro Limited
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
*/
#include <dt-bindings/dma/qcom-gpi.h>
@@ -65,6 +66,12 @@
/* DMA TRE */
#define TRE_DMA_LEN GENMASK(23, 0)
+/* Lock TRE */
+#define TRE_I2C_LOCK_WORD_3 (3 << 20 | 0 << 16 | BIT(0))
+
+/* Unlock TRE */
+#define TRE_I2C_UNLOCK_WORD_3 (3 << 20 | 1 << 16 | BIT(8))
+
/* Register offsets from gpi-top */
#define GPII_n_CH_k_CNTXT_0_OFFS(n, k) (0x20000 + (0x4000 * (n)) + (0x80 * (k)))
#define GPII_n_CH_k_CNTXT_0_EL_SIZE GENMASK(31, 24)
@@ -522,7 +529,7 @@ struct gpii {
bool ieob_set;
};
-#define MAX_TRE 3
+#define MAX_TRE 5
struct gpi_desc {
struct virt_dma_desc vd;
@@ -1644,6 +1651,18 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
struct gpi_tre *tre;
unsigned int i;
+ /* create lock tre for first tranfser */
+ if (i2c->shared_se && i2c->first_msg) {
+ tre = &desc->tre[tre_idx];
+ tre_idx++;
+
+ /* lock: chain bit set */
+ tre->dword[0] = 0;
+ tre->dword[1] = 0;
+ tre->dword[2] = 0;
+ tre->dword[3] = TRE_I2C_LOCK_WORD_3;
+ }
+
/* first create config tre if applicable */
if (i2c->set_config) {
tre = &desc->tre[tre_idx];
@@ -1702,6 +1721,18 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_IEOT);
}
+ /* Unlock tre for last transfer */
+ if (i2c->shared_se && i2c->last_msg && i2c->op != I2C_READ) {
+ tre = &desc->tre[tre_idx];
+ tre_idx++;
+
+ /* unlock tre: ieob set */
+ tre->dword[0] = 0;
+ tre->dword[1] = 0;
+ tre->dword[2] = 0;
+ tre->dword[3] = TRE_I2C_UNLOCK_WORD_3;
+ }
+
for (i = 0; i < tre_idx; i++)
dev_dbg(dev, "TRE:%d %x:%x:%x:%x\n", i, desc->tre[i].dword[0],
desc->tre[i].dword[1], desc->tre[i].dword[2], desc->tre[i].dword[3]);
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index da94df466e83..c5935c5f46e8 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
// Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+// Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
#include <linux/acpi.h>
#include <linux/clk.h>
@@ -99,6 +100,7 @@ struct geni_i2c_dev {
struct dma_chan *rx_c;
bool gpi_mode;
bool abort_done;
+ bool is_shared;
};
struct geni_i2c_desc {
@@ -601,6 +603,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
peripheral.clk_div = itr->clk_div;
peripheral.set_config = 1;
peripheral.multi_msg = false;
+ peripheral.shared_se = gi2c->is_shared;
for (i = 0; i < num; i++) {
gi2c->cur = &msgs[i];
@@ -611,6 +614,8 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
if (i < num - 1)
peripheral.stretch = 1;
+ peripheral.first_msg = (i == 0) ? true : false;
+ peripheral.last_msg = (i == num - 1) ? true : false;
peripheral.addr = msgs[i].addr;
ret = geni_i2c_gpi(gi2c, &msgs[i], &config,
@@ -802,6 +807,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
gi2c->clk_freq_out = KHZ(100);
}
+ if (of_property_read_bool(pdev->dev.of_node, "qcom,shared-se")) {
+ gi2c->is_shared = true;
+ dev_info(&pdev->dev, "Multi-EE usecase with shared SE\n");
+ }
+
if (has_acpi_companion(dev))
ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
@@ -964,14 +974,16 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
disable_irq(gi2c->irq);
- ret = geni_se_resources_off(&gi2c->se);
- if (ret) {
- enable_irq(gi2c->irq);
- return ret;
-
+ if (gi2c->is_shared) {
+ geni_se_clks_off(&gi2c->se);
} else {
- gi2c->suspended = 1;
+ ret = geni_se_resources_off(&gi2c->se);
+ if (ret) {
+ enable_irq(gi2c->irq);
+ return ret;
+ }
}
+ gi2c->suspended = 1;
clk_disable_unprepare(gi2c->core_clk);
diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 2e8f24d5da80..20166c8fc919 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
// Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+// Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
/* Disable MMIO tracing to prevent excessive logging of unwanted MMIO traces */
#define __DISABLE_TRACE_MMIO__
@@ -482,13 +483,14 @@ void geni_se_config_packing(struct geni_se *se, int bpw, int pack_words,
}
EXPORT_SYMBOL_GPL(geni_se_config_packing);
-static void geni_se_clks_off(struct geni_se *se)
+void geni_se_clks_off(struct geni_se *se)
{
struct geni_wrapper *wrapper = se->wrapper;
clk_disable_unprepare(se->clk);
clk_bulk_disable_unprepare(wrapper->num_clks, wrapper->clks);
}
+EXPORT_SYMBOL_GPL(geni_se_clks_off);
/**
* geni_se_resources_off() - Turn off resources associated with the serial
diff --git a/include/linux/dma/qcom-gpi-dma.h b/include/linux/dma/qcom-gpi-dma.h
index 6680dd1a43c6..8589c711afae 100644
--- a/include/linux/dma/qcom-gpi-dma.h
+++ b/include/linux/dma/qcom-gpi-dma.h
@@ -65,6 +65,9 @@ enum i2c_op {
* @rx_len: receive length for buffer
* @op: i2c cmd
* @muli-msg: is part of multi i2c r-w msgs
+ * @shared_se: bus is shared between subsystems
+ * @bool first_msg: use it for tracking multimessage xfer
+ * @bool last_msg: use it for tracking multimessage xfer
*/
struct gpi_i2c_config {
u8 set_config;
@@ -78,6 +81,9 @@ struct gpi_i2c_config {
u32 rx_len;
enum i2c_op op;
bool multi_msg;
+ bool shared_se;
+ bool first_msg;
+ bool last_msg;
};
#endif /* QCOM_GPI_DMA_H */
diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
index 0f038a1a0330..caf2c0c4505b 100644
--- a/include/linux/soc/qcom/geni-se.h
+++ b/include/linux/soc/qcom/geni-se.h
@@ -1,6 +1,7 @@
/* SPDX-License-Identifier: GPL-2.0 */
/*
* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
*/
#ifndef _LINUX_QCOM_GENI_SE
@@ -494,6 +495,8 @@ int geni_se_resources_off(struct geni_se *se);
int geni_se_resources_on(struct geni_se *se);
+void geni_se_clks_off(struct geni_se *se);
+
int geni_se_clk_tbl_get(struct geni_se *se, unsigned long **tbl);
int geni_se_clk_freq_match(struct geni_se *se, unsigned long req_freq,
--
2.25.1
On 27.03.2024 11:18 AM, Mukesh Kumar Savaliya wrote:
> Add feature to share an I2C serial engine from two subsystem(SS) so that
from -> between
subystem -> subsystems
> individual client from different subsystem can work on the same bus.
client -> clients
subystem -> subsystems
"work on the same bus" - access the same bus? Does that concern both
read and write to the same slave device?
Please give a more specific example. Could that be for example APSS and
TZ?
>
> This is possible in GSI mode where driver queues the TRE with required
> descriptors and ensures it executes them in a mutual exclusive way.
mutually
the "it" and "them" are confusing, could you reword this?
> Add Lock TRE at the start of the transfer and Unlock TRE at the end of
> the transfer protecting the DMA channel. This way not allowing other SS
> to queue anything in between and disturb the data path.
'Issue a "Lock TRE" command at the start of the transfer and an "Unlock TRE"
at the end of it. This prevents other subsystems from concurrently
performing DMA transfers through the same GPI channel, so as to avoid
disturbing the data path.'
Would that be a fair representation of what this is trying to achieve?
>
> Since the GPIOs are also shared for the i2c bus, do not touch GPIO
> configuration while going to runtime suspend and only turn off the
> clocks. This will allow other SS to continue to transfer the data.
>
> To realize this, add below change:
> 1) Check if the Particular I2C requires to be shared during probe() time.
This requires a dt-bindings change. Had you run something like:
/scripts/checkpatch.pl -g $(git describe --abbrev=0)..HEAD
you'dve noticed there's new warnings.
> 2) If shared SE add LOCK TRE inside gpi_create_i2c_tre() before first
> message.
> 3) If shared SE add UNLOCK TRE inside gpi_create_i2c_tre() before
> last transfer message.
You already described this above.
> 4) Export function geni_se_clks_off() to call explicitly instead of
> geni_se_resources_off().
Do we expect other SEs (UART, I3C, SPI) to also support this "shared"
configuration? Would it be beneficial to make the "shared" cases common
and bail out of geni_se_resources_off() early if a SE is marked as such?
>
> Signed-off-by: Mukesh Kumar Savaliya <[email protected]>
> ---
> drivers/dma/qcom/gpi.c | 33 +++++++++++++++++++++++++++++-
> drivers/i2c/busses/i2c-qcom-geni.c | 24 ++++++++++++++++------
> drivers/soc/qcom/qcom-geni-se.c | 4 +++-
> include/linux/dma/qcom-gpi-dma.h | 6 ++++++
> include/linux/soc/qcom/geni-se.h | 3 +++
This is a big no-go, you're changing files across 3 different subsystems,
you must split this patch up. In this case, you want one for dma, one for
soc and one for i2c. Check ./scripts/get_maintainer.pl <filename>
> 5 files changed, 62 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index 1c93864e0e4d..df276ccf9cbb 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
> @@ -2,6 +2,7 @@
> /*
> * Copyright (c) 2017-2020, The Linux Foundation. All rights reserved.
> * Copyright (c) 2020, Linaro Limited
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> */
>
> #include <dt-bindings/dma/qcom-gpi.h>
> @@ -65,6 +66,12 @@
> /* DMA TRE */
> #define TRE_DMA_LEN GENMASK(23, 0)
>
> +/* Lock TRE */
> +#define TRE_I2C_LOCK_WORD_3 (3 << 20 | 0 << 16 | BIT(0))
> +
> +/* Unlock TRE */
> +#define TRE_I2C_UNLOCK_WORD_3 (3 << 20 | 1 << 16 | BIT(8))
The comments you're introducing don't seem particularly helpful, looking
at the define names. You should also avoid adding random shifted values
and #define the bitfields used in your messages with BIT() and GENMASK()
> +
> /* Register offsets from gpi-top */
> #define GPII_n_CH_k_CNTXT_0_OFFS(n, k) (0x20000 + (0x4000 * (n)) + (0x80 * (k)))
> #define GPII_n_CH_k_CNTXT_0_EL_SIZE GENMASK(31, 24)
> @@ -522,7 +529,7 @@ struct gpii {
> bool ieob_set;
> };
>
> -#define MAX_TRE 3
> +#define MAX_TRE 5
This almost looks like a separate commit itself? Definitely needs
an explanation.
>
> struct gpi_desc {
> struct virt_dma_desc vd;
> @@ -1644,6 +1651,18 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
> struct gpi_tre *tre;
> unsigned int i;
>
> + /* create lock tre for first tranfser */
> + if (i2c->shared_se && i2c->first_msg) {
> + tre = &desc->tre[tre_idx];
> + tre_idx++;
> +
> + /* lock: chain bit set */
What does this mean?
> + tre->dword[0] = 0;
> + tre->dword[1] = 0;
> + tre->dword[2] = 0;
> + tre->dword[3] = TRE_I2C_LOCK_WORD_3;
> + }
> +
> /* first create config tre if applicable */
> if (i2c->set_config) {
> tre = &desc->tre[tre_idx];
> @@ -1702,6 +1721,18 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
> tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_IEOT);
> }
>
> + /* Unlock tre for last transfer */
> + if (i2c->shared_se && i2c->last_msg && i2c->op != I2C_READ) {
> + tre = &desc->tre[tre_idx];
> + tre_idx++;
> +
> + /* unlock tre: ieob set */
What does this mean?
> + tre->dword[0] = 0;
> + tre->dword[1] = 0;
> + tre->dword[2] = 0;
> + tre->dword[3] = TRE_I2C_UNLOCK_WORD_3;
> + }
> +
> for (i = 0; i < tre_idx; i++)
> dev_dbg(dev, "TRE:%d %x:%x:%x:%x\n", i, desc->tre[i].dword[0],
> desc->tre[i].dword[1], desc->tre[i].dword[2], desc->tre[i].dword[3]);
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index da94df466e83..c5935c5f46e8 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -1,5 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
> // Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> +// Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>
> #include <linux/acpi.h>
> #include <linux/clk.h>
> @@ -99,6 +100,7 @@ struct geni_i2c_dev {
> struct dma_chan *rx_c;
> bool gpi_mode;
> bool abort_done;
> + bool is_shared;
> };
>
> struct geni_i2c_desc {
> @@ -601,6 +603,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
> peripheral.clk_div = itr->clk_div;
> peripheral.set_config = 1;
> peripheral.multi_msg = false;
> + peripheral.shared_se = gi2c->is_shared;
>
> for (i = 0; i < num; i++) {
> gi2c->cur = &msgs[i];
> @@ -611,6 +614,8 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
> if (i < num - 1)
> peripheral.stretch = 1;
>
> + peripheral.first_msg = (i == 0) ? true : false;
> + peripheral.last_msg = (i == num - 1) ? true : false;
Why the ternary operator? == already returns a boolean value
> peripheral.addr = msgs[i].addr;
>
> ret = geni_i2c_gpi(gi2c, &msgs[i], &config,
> @@ -802,6 +807,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
> gi2c->clk_freq_out = KHZ(100);
> }
>
> + if (of_property_read_bool(pdev->dev.of_node, "qcom,shared-se")) {
> + gi2c->is_shared = true;
> + dev_info(&pdev->dev, "Multi-EE usecase with shared SE\n");
How would this line be useful in my kernel log?
Konrad
Thanks Konrad for detailed review. For dt-bindings sending a separate
patch soon, rest comments tried to address and updated patch V2.
On 3/28/2024 2:35 AM, Konrad Dybcio wrote:
> On 27.03.2024 11:18 AM, Mukesh Kumar Savaliya wrote:
>> Add feature to share an I2C serial engine from two subsystem(SS) so that
>
> from -> between
> subystem -> subsystems
>
Done
>> individual client from different subsystem can work on the same bus.
>
> client -> clients
> subystem -> subsystems
>
Done
> "work on the same bus" - access the same bus? Does that concern both
> read and write to the same slave device?
>
yes, access the same bus suits better.
Yes, they can read/write from/to same slave device.
>
> Please give a more specific example. Could that be for example APSS and
> TZ?
>
sure. Example: APSS and TZ, APPS and Modem, Modem and DSP etc.
Changed Enhanced commit log accordingly.
>>
>> This is possible in GSI mode where driver queues the TRE with required
>> descriptors and ensures it executes them in a mutual exclusive way.
>
> mutually
>
Done
> the "it" and "them" are confusing, could you reword this?
>
Sure, i have modified.
>> Add Lock TRE at the start of the transfer and Unlock TRE at the end of
>> the transfer protecting the DMA channel. This way not allowing other SS
>> to queue anything in between and disturb the data path.
>
> 'Issue a "Lock TRE" command at the start of the transfer and an "Unlock TRE"
> at the end of it. This prevents other subsystems from concurrently
> performing DMA transfers through the same GPI channel, so as to avoid
> disturbing the data path.'
>
> Would that be a fair representation of what this is trying to achieve?
>
Yes, that's what i meant to say. I have changed into similar way.
>>
>> Since the GPIOs are also shared for the i2c bus, do not touch GPIO
>> configuration while going to runtime suspend and only turn off the
>> clocks. This will allow other SS to continue to transfer the data.
>>
>> To realize this, add below change:
>> 1) Check if the Particular I2C requires to be shared during probe() time.
>
> This requires a dt-bindings change. Had you run something like:
>
> ./scripts/checkpatch.pl -g $(git describe --abbrev=0)..HEAD
>
> you'dve noticed there's new warnings.
>
yes, I understand this needs dt-bindings update for new flag.
Though Ran this script now but could not find any warning, i shall
update dt-bindings.
>> 2) If shared SE add LOCK TRE inside gpi_create_i2c_tre() before first
>> message.
>> 3) If shared SE add UNLOCK TRE inside gpi_create_i2c_tre() before
>> last transfer message.
>
> You already described this above.
>
sure, i can remove it.
>> 4) Export function geni_se_clks_off() to call explicitly instead of
>> geni_se_resources_off().
>
> Do we expect other SEs (UART, I3C, SPI) to also support this "shared"
> configuration? Would it be beneficial to make the "shared" cases common
> and bail out of geni_se_resources_off() early if a SE is marked as such?
>
As such this is generic GSI based supported feature, But I3C and UART
doesn't have GSI mode support Added. Right now, we are enabling this
feature for i2 only, in future we may enable for SPI, I3C as and when
required. Even Considering commonality in future, shared feature is
known to individual SE protocol driver and would be little complex to
add into common driver.
>>
>> Signed-off-by: Mukesh Kumar Savaliya <[email protected]>
>> ---
>> drivers/dma/qcom/gpi.c | 33 +++++++++++++++++++++++++++++-
>> drivers/i2c/busses/i2c-qcom-geni.c | 24 ++++++++++++++++------
>> drivers/soc/qcom/qcom-geni-se.c | 4 +++-
>> include/linux/dma/qcom-gpi-dma.h | 6 ++++++
>> include/linux/soc/qcom/geni-se.h | 3 +++
>
> This is a big no-go, you're changing files across 3 different subsystems,
> you must split this patch up. In this case, you want one for dma, one for
> soc and one for i2c. Check ./scripts/get_maintainer.pl <filename>
>
Please correct me if this is wrong. The overall change is for i2c in GSI
DMA mode. This also requires changes in resource control like TLMM
changes. But it's more like integrated feature.
Are you suggesting to make 3 sub-patches under same change ?
>> 5 files changed, 62 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
>> index 1c93864e0e4d..df276ccf9cbb 100644
>> --- a/drivers/dma/qcom/gpi.c
>> +++ b/drivers/dma/qcom/gpi.c
>> @@ -2,6 +2,7 @@
>> /*
>> * Copyright (c) 2017-2020, The Linux Foundation. All rights reserved.
>> * Copyright (c) 2020, Linaro Limited
>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> */
>>
>> #include <dt-bindings/dma/qcom-gpi.h>
>> @@ -65,6 +66,12 @@
>> /* DMA TRE */
>> #define TRE_DMA_LEN GENMASK(23, 0)
>>
>> +/* Lock TRE */
>> +#define TRE_I2C_LOCK_WORD_3 (3 << 20 | 0 << 16 | BIT(0))
>> +
>> +/* Unlock TRE */
>> +#define TRE_I2C_UNLOCK_WORD_3 (3 << 20 | 1 << 16 | BIT(8))
>
> The comments you're introducing don't seem particularly helpful, looking
> at the define names. You should also avoid adding random shifted values
> and #define the bitfields used in your messages with BIT() and GENMASK()
>
Sure, Can remove the comment as macro name explains it.
Will define macros with BIT() and GENMASK(). Made changes similar to the
existing macros.
>> +
>> /* Register offsets from gpi-top */
>> #define GPII_n_CH_k_CNTXT_0_OFFS(n, k) (0x20000 + (0x4000 * (n)) + (0x80 * (k)))
>> #define GPII_n_CH_k_CNTXT_0_EL_SIZE GENMASK(31, 24)
>> @@ -522,7 +529,7 @@ struct gpii {
>> bool ieob_set;
>> };
>>
>> -#define MAX_TRE 3
>> +#define MAX_TRE 5
>
> This almost looks like a separate commit itself? Definitely needs
> an explanation.
>
Agree, Added into commit log explanation.
This is changed because we have added LOCK_TRE and UNLOCK_TRE, hence 3
became 5.
>>
>> struct gpi_desc {
>> struct virt_dma_desc vd;
>> @@ -1644,6 +1651,18 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
>> struct gpi_tre *tre;
>> unsigned int i;
>>
>> + /* create lock tre for first tranfser */
>> + if (i2c->shared_se && i2c->first_msg) {
>> + tre = &desc->tre[tre_idx];
>> + tre_idx++;
>> +
>> + /* lock: chain bit set */
>
> What does this mean?
It guides GSI engine saying there is next TRE queued, Basically not the
last TRE. I am removing this comment, as it's code specific.
>
>> + tre->dword[0] = 0;
>> + tre->dword[1] = 0;
>> + tre->dword[2] = 0;
>> + tre->dword[3] = TRE_I2C_LOCK_WORD_3;
>> + }
>> +
>> /* first create config tre if applicable */
>> if (i2c->set_config) {
>> tre = &desc->tre[tre_idx];
>> @@ -1702,6 +1721,18 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
>> tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_IEOT);
>> }
>>
>> + /* Unlock tre for last transfer */
>> + if (i2c->shared_se && i2c->last_msg && i2c->op != I2C_READ) {
>> + tre = &desc->tre[tre_idx];
>> + tre_idx++;
>> +
>> + /* unlock tre: ieob set */
>
> What does this mean?
>
It says, end of block interrupt. Engine will generate an interrupt.
But comment is not required as it's code specific.
>> + tre->dword[0] = 0;
>> + tre->dword[1] = 0;
>> + tre->dword[2] = 0;
>> + tre->dword[3] = TRE_I2C_UNLOCK_WORD_3;
>> + }
>> +
>> for (i = 0; i < tre_idx; i++)
>> dev_dbg(dev, "TRE:%d %x:%x:%x:%x\n", i, desc->tre[i].dword[0],
>> desc->tre[i].dword[1], desc->tre[i].dword[2], desc->tre[i].dword[3]);
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>> index da94df466e83..c5935c5f46e8 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -1,5 +1,6 @@
>> // SPDX-License-Identifier: GPL-2.0
>> // Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>> +// Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>
>> #include <linux/acpi.h>
>> #include <linux/clk.h>
>> @@ -99,6 +100,7 @@ struct geni_i2c_dev {
>> struct dma_chan *rx_c;
>> bool gpi_mode;
>> bool abort_done;
>> + bool is_shared;
>> };
>>
>> struct geni_i2c_desc {
>> @@ -601,6 +603,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>> peripheral.clk_div = itr->clk_div;
>> peripheral.set_config = 1;
>> peripheral.multi_msg = false;
>> + peripheral.shared_se = gi2c->is_shared;
>>
>> for (i = 0; i < num; i++) {
>> gi2c->cur = &msgs[i];
>> @@ -611,6 +614,8 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>> if (i < num - 1)
>> peripheral.stretch = 1;
>>
>> + peripheral.first_msg = (i == 0) ? true : false;
>> + peripheral.last_msg = (i == num - 1) ? true : false;
>
> Why the ternary operator? == already returns a boolean value
>
Sure, this was more easier to understand. I have removed ternary logic.
>> peripheral.addr = msgs[i].addr;
>>
>> ret = geni_i2c_gpi(gi2c, &msgs[i], &config,
>> @@ -802,6 +807,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
>> gi2c->clk_freq_out = KHZ(100);
>> }
>>
>> + if (of_property_read_bool(pdev->dev.of_node, "qcom,shared-se")) {
>> + gi2c->is_shared = true;
>> + dev_info(&pdev->dev, "Multi-EE usecase with shared SE\n");
>
> How would this line be useful in my kernel log?
>
It informs that particular SE is shared between SEs from two subsystems,
hence respective debug can happen accordingly in case of the issue.
> Konrad
On 4/2/24 08:21, Mukesh Kumar Savaliya wrote:
> Thanks Konrad for detailed review. For dt-bindings sending a separate patch soon, rest comments tried to address and updated patch V2.
[...]
>>> + if (of_property_read_bool(pdev->dev.of_node, "qcom,shared-se")) {
>>> + gi2c->is_shared = true;
>>> + dev_info(&pdev->dev, "Multi-EE usecase with shared SE\n");
>>
>> How would this line be useful in my kernel log?
>>
> It informs that particular SE is shared between SEs from two subsystems, hence respective debug can happen accordingly in case of the issue.
This amounts to "not very useful". As an end user, I couldn't care less
about the nitty-gritty of firmware-hardware interactions, so long as the
thing works. You must not spam the kernel log with debug messages, as it
slows things down and makes actually useful messages harder to spot. If
you want to keep it, use dev_dbg.
Konrad
Hi Konrad, Thanks for confirming your preferences.
On 4/16/2024 5:14 AM, Konrad Dybcio wrote:
>
>
> On 4/2/24 08:21, Mukesh Kumar Savaliya wrote:
>> Thanks Konrad for detailed review. For dt-bindings sending a separate
>> patch soon, rest comments tried to address and updated patch V2.
>
> [...]
>
>>>> + if (of_property_read_bool(pdev->dev.of_node, "qcom,shared-se")) {
>>>> + gi2c->is_shared = true;
>>>> + dev_info(&pdev->dev, "Multi-EE usecase with shared SE\n");
>>>
>>> How would this line be useful in my kernel log?
>>>
>> It informs that particular SE is shared between SEs from two
>> subsystems, hence respective debug can happen accordingly in case of
>> the issue.
>
> This amounts to "not very useful". As an end user, I couldn't care less
> about the nitty-gritty of firmware-hardware interactions, so long as the
> thing works. You must not spam the kernel log with debug messages, as it
> slows things down and makes actually useful messages harder to spot. If
> you want to keep it, use dev_dbg.
>
Sure, will go with the dev_dbg. I am uploading a separate patches for
i2c and GSI DMA sub system.
> Konrad