| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Zgus_bFm7Ls7ApTx@matsya>
Date: Tue, 2 Apr 2024 12:30:13 +0530
From: Vinod Koul <vkoul@...nel.org>
To: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
Cc: konrad.dybcio@...aro.org, andersson@...nel.org, andi.shyti@...nel.org,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-i2c@...r.kernel.org, dmaengine@...r.kernel.org,
quic_vdadhani@...cinc.com
Subject: Re: [PATCH v2] i2c: i2c-qcom-geni: Add support to share an I2C SE
from two subsystem
On 02-04-24, 11:51, Mukesh Kumar Savaliya wrote:
> Add feature to share an I2C serial engine between two subsystems(SS) so
> that individual clients from different subsystems can access the same bus.
> For example single i2c slave device can be accessed by Client driver from
> APPS OR modem subsystem image. Same way we can have slave being accessed
> between APPS and TZ subsystems.
>
> This is possible in GSI mode where driver queues the TREs with required
> descriptors and ensures to execute TREs in an mutually exclusive way.
> Issue a "Lock TRE" command at the start of the transfer and an "Unlock TRE"
> command at the end of the transfer. This prevents other subsystems from
> concurrently performing DMA transfers and avoids disturbance to data path.
> Change MAX_TRE macro to 5 from 3 because of these two additional TREs.
>
> 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.
>
> This feature needs to be controlled by DTSI flag to make it flexible
> based on the usecase, hence during probe check the same from i2c driver.
>
> Export function geni_se_clks_off() to call explicitly instead of
> geni_se_resources_off() to not modify TLMM configuration as other SS might
> perform the transfer while APPS SS can go to sleep.
>
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
> ---
> v1 -> v2:
> - Addressed review comments.
> - Removed unwanted comments from the gpi_create_i2c_tre().
> - Enhanced logic by removing ternary assignment in i2c-qcom-geni.c.
> - Confirmed dt-bindings change is required too in separate patch.
> - Formed LOCK_TRE and UNLOCK_TRE by using BIT fields similar to other TREs.
> ---
> drivers/dma/qcom/gpi.c | 37 +++++++++++++++++++++++++++++-
> 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, 66 insertions(+), 8 deletions(-)
why are all changes mashed into one commit, pls use separate one for
dmaengine patches!
>
> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index 1c93864e0e4d..0997210df6b1 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,14 @@
> /* DMA TRE */
> #define TRE_DMA_LEN GENMASK(23, 0)
>
> +/* Lock TRE */
> +#define TRE_I2C_LOCK BIT(0)
> +#define TRE_MINOR_TYPE GENMASK(19, 16)
> +#define TRE_MAJOR_TYPE GENMASK(23, 20)
> +
> +/* Unlock TRE */
> +#define TRE_I2C_UNLOCK 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 +531,7 @@ struct gpii {
> bool ieob_set;
> };
>
> -#define MAX_TRE 3
> +#define MAX_TRE 5
>
> struct gpi_desc {
> struct virt_dma_desc vd;
> @@ -1644,6 +1653,19 @@ 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++;
> +
> + tre->dword[0] = 0;
> + tre->dword[1] = 0;
> + tre->dword[2] = 0;
> + tre->dword[3] = u32_encode_bits(1, TRE_I2C_LOCK);
> + tre->dword[3] |= u32_encode_bits(0, TRE_MINOR_TYPE);
> + tre->dword[3] |= u32_encode_bits(3, TRE_MAJOR_TYPE);
This is not optimal, you are always going to do this. Pls define
LOCK_TRE as lock and minor/major bits and then assign it here
> + }
> +
> /* first create config tre if applicable */
> if (i2c->set_config) {
> tre = &desc->tre[tre_idx];
> @@ -1702,6 +1724,19 @@ 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++;
> +
> + tre->dword[0] = 0;
> + tre->dword[1] = 0;
> + tre->dword[2] = 0;
> + tre->dword[3] = u32_encode_bits(1, TRE_I2C_UNLOCK);
> + tre->dword[3] |= u32_encode_bits(1, TRE_MINOR_TYPE);
> + tre->dword[3] |= u32_encode_bits(3, TRE_MAJOR_TYPE);
Here as well
> + }
> +
> 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..fbfcd375c06f 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);
> + peripheral.last_msg = (i == num - 1);
> 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")) {
what is the actual use case of this, when would this be shared...?
> + 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
--
~Vinod
Powered by blists - more mailing lists