| 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: <687db538-1a41-4353-89fd-d1869d960a12@quicinc.com>
Date: Tue, 10 Sep 2024 14:45:13 +0530
From: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
To: <neil.armstrong@...aro.org>, <konrad.dybcio@...aro.org>,
<andersson@...nel.org>, <andi.shyti@...nel.org>,
<linux-arm-msm@...r.kernel.org>, <dmaengine@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-i2c@...r.kernel.org>,
<conor+dt@...nel.org>, <agross@...nel.org>,
<devicetree@...r.kernel.org>, <vkoul@...nel.org>, <linux@...blig.org>,
<dan.carpenter@...aro.org>, <Frank.Li@....com>,
<konradybcio@...nel.org>
CC: <quic_vdadhani@...cinc.com>
Subject: Re: [PATCH v2 4/4] i2c: i2c-qcom-geni: Enable i2c controller sharing
between two subsystems
Hi Neil,
On 9/9/2024 6:34 PM, neil.armstrong@...aro.org wrote:
> On 09/09/2024 11:18, Mukesh Kumar Savaliya wrote:
>> Hi Neil,
>>
>> On 9/9/2024 2:24 PM, neil.armstrong@...aro.org wrote:
>>> Hi,
>>>
>>> On 06/09/2024 21:14, Mukesh Kumar Savaliya wrote:
>>>> Add support to share I2C SE by two Subsystems in a mutually
>>>> exclusive way.
>>>> Use "qcom,shared-se" flag in a particular i2c instance node if the
>>>> usecase requires i2c controller to be shared.
>>>>
>>>> I2C driver just need to mark first_msg and last_msg flag to help
>>>> indicate
>>>> GPI driver to take lock and unlock TRE there by protecting from
>>>> concurrent
>>>> access from other EE or Subsystem.
>>>>
>>>> gpi_create_i2c_tre() function at gpi.c will take care of adding Lock
>>>> and
>>>> Unlock TRE for the respective transfer operations.
>>>>
>>>> Since the GPIOs are also shared for the i2c bus between two SS, do not
>>>> touch GPIO configuration during runtime suspend and only turn off the
>>>> clocks. This will allow other SS to continue to transfer the data
>>>> without any disturbance over the IO lines.
>>>
>>> This doesn't answer my question about what would be the behavior if one
>>> use uses, for example, GPI DMA, and the Linux kernel FIFO mode or SE
>>> DMA ?
>>>
>> Shared usecase is not supported for non GSI mode (FIFO and DMA), it
>> should be static usecase. Dynamic sharing from two clients of two
>> subsystems is only for GSI mode. Hope this helps ?
>
> Sure, this is why I proposed on v1 cover letter reply to add:
Sure, i will add in cover letter and code check combining with
fifo_disable check.
> ==============><=====================================================================
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
> b/drivers/i2c/busses/i2c-qcom-geni.c
> index ee2e431601a6..a15825ea56de 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -885,7 +885,7 @@ static int geni_i2c_probe(struct platform_device *pdev)
> else
> fifo_disable = readl_relaxed(gi2c->se.base +
> GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
>
> - if (fifo_disable) {
> + if (gi2c->is_shared || fifo_disable) {
Should be ANDING logically, as we need to combine both check. Shared
usecase possible only for fifo_disable.
if(gi2c->is_shared && fifo_disable) {
> /* FIFO is disabled, so we can only use GPI DMA */
> gi2c->gpi_mode = true;
> ret = setup_gpi_dma(gi2c);
> ==============><=====================================================================
>
> Thanks,
> Neil
>
>>> Because it seems to "fix" only the GPI DMA shared case.
>>>
>>> Neil
>>>
>>>>
>>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
>>>> ---
>>>> drivers/i2c/busses/i2c-qcom-geni.c | 29 ++++++++++++++++++++++-------
>>>> 1 file changed, 22 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
>>>> b/drivers/i2c/busses/i2c-qcom-geni.c
>>>> index eebb0cbb6ca4..ee2e431601a6 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 {
>>>> @@ -602,6 +604,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];
>>>> @@ -612,6 +615,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,
>>>> @@ -631,8 +636,11 @@ static int geni_i2c_gpi_xfer(struct
>>>> geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>>>> dma_async_issue_pending(gi2c->tx_c);
>>>> time_left = wait_for_completion_timeout(&gi2c->done,
>>>> XFER_TIMEOUT);
>>>> - if (!time_left)
>>>> + if (!time_left) {
>>>> + dev_err(gi2c->se.dev, "I2C timeout gpi flags:%d
>>>> addr:0x%x\n",
>>>> + gi2c->cur->flags, gi2c->cur->addr);
>>>> gi2c->err = -ETIMEDOUT;
>>>> + }
>>>> if (gi2c->err) {
>>>> ret = gi2c->err;
>>>> @@ -800,6 +808,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_dbg(&pdev->dev, "Shared SE Usecase\n");
>>>> + }
>>>> +
>>>> if (has_acpi_companion(dev))
>>>> ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
>>>> @@ -962,14 +975,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);
>>>
>>>
>
>
Powered by blists - more mailing lists