| 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: <46715939-9aa6-8c83-2433-7bebf85a49c3@broadcom.com>
Date: Tue, 1 Jun 2021 14:51:45 -0700
From: Ray Jui <ray.jui@...adcom.com>
To: Chris Packham <Chris.Packham@...iedtelesis.co.nz>,
"wsa@...nel.org" <wsa@...nel.org>,
"rjui@...adcom.com" <rjui@...adcom.com>,
"sbranden@...adcom.com" <sbranden@...adcom.com>,
"bcm-kernel-feedback-list@...adcom.com"
<bcm-kernel-feedback-list@...adcom.com>
Cc: "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Richard Laing <Richard.Laing@...iedtelesis.co.nz>
Subject: Re: [PATCH] i2c: bcm-iproc: Add i2c recovery support
On 6/1/2021 2:44 PM, Chris Packham wrote:
>
> On 2/06/21 9:03 am, Ray Jui wrote:
>>
>> On 5/30/2021 3:56 PM, Chris Packham wrote:
>>> From: Richard Laing <richard.laing@...iedtelesis.co.nz>
>>>
>>> The bcm-iproc controller can put the SDA/SCL lines into bit-bang mode,
>>> make use of this to support i2c bus recovery.
>>>
>>> Signed-off-by: Richard Laing <richard.laing@...iedtelesis.co.nz>
>>> Signed-off-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
>>> ---
>>>
>>> Notes:
>>> Richard did most of the work on this. I'm just cleaning it up to get it
>>> upstream.
>>>
>>> drivers/i2c/busses/i2c-bcm-iproc.c | 115 +++++++++++++++++++++++++++++
>>> 1 file changed, 115 insertions(+)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
>>> index cceaf69279a9..d63a286c1660 100644
>>> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
>>> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
>>> @@ -26,6 +26,7 @@
>>> #define CFG_RESET_SHIFT 31
>>> #define CFG_EN_SHIFT 30
>>> #define CFG_SLAVE_ADDR_0_SHIFT 28
>>> +#define CFG_BIT_BANG_SHIFT 29
>> move this up one line (to be consistent with existing bit order)
>
> Ack.
>
>>> #define CFG_M_RETRY_CNT_SHIFT 16
>>> #define CFG_M_RETRY_CNT_MASK 0x0f
>>>
>>> @@ -66,6 +67,12 @@
>>> #define S_FIFO_RX_THLD_SHIFT 8
>>> #define S_FIFO_RX_THLD_MASK 0x3f
>>>
>>> +#define M_BB_CTRL_OFFSET 0x14
>>> +#define M_BB_SMBCLK_IN 31
>> M_BB_CTRL_CLK_IN_SHIFT, ket is to have '_SHIFT' to be consistent with
>> existing code
>
> Ack.
>
>>> +#define M_BB_SMBCLK_OUT_EN 30
>> M_BB_CTRL_CLK_OUT_SHIFT
>>
>>> +#define M_BB_SMBDAT_IN 29
>> M_BB_CTRL_DATA_IN_SHIFT
>>
>>> +#define M_BB_SMBDAT_OUT_EN 28
>> M_BB_CTRL_DATA_OUT_SHIFT
>>
>>> +
>>> #define M_CMD_OFFSET 0x30
>>> #define M_CMD_START_BUSY_SHIFT 31
>>> #define M_CMD_STATUS_SHIFT 25
>>> @@ -713,6 +720,112 @@ static void bcm_iproc_i2c_enable_disable(struct bcm_iproc_i2c_dev *iproc_i2c,
>>> iproc_i2c_wr_reg(iproc_i2c, CFG_OFFSET, val);
>>> }
>>>
>>> +static void bcm_iproc_i2c_reset(struct bcm_iproc_i2c_dev *iproc_i2c)
>>> +{
>>> + u32 tmp;
>>> +
>>> + tmp = readl(iproc_i2c->base + CFG_OFFSET);
>>> + tmp |= BIT(CFG_RESET_SHIFT);
>>> + writel(tmp, iproc_i2c->base + CFG_OFFSET);
>>> + udelay(100);
>> This puts the controller in reset and hold it there, but never brings
>> the controller out of reset (bcm_iproc_i2c_init called in unprepare
>> brings the controller out of reset)
>>
>> Calling it a "reset" function is a bit misleading to me. My expectation
>> of a reset function is that you generate a reset pulse, ie.g., reset ->
>> delay -> out of reset.
>>
>> Why don't you simply put this seuquence of code in the prepare_recovery
>> function below, instead of calling this a reset function?
>
> Will do. Should I also change the udelay(100) to a usleep_range(100,
> 200)? Same question for the other uses of udelay().
>
Sure I think that makes sense.
>>
>>> +
>>> +}
>>> +
>>> +static void bcm_iproc_i2c_prepare_recovery(struct i2c_adapter *adap)
>>> +{
>>> + struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
>>> + u32 tmp;
>>> +
>>> + dev_dbg(iproc_i2c->device, "Prepare recovery\n");
>>> +
>>> + /* Disable interrupts */
>>> + writel(0, iproc_i2c->base + IE_OFFSET);
>>> + readl(iproc_i2c->base + IE_OFFSET);
>>> + synchronize_irq(iproc_i2c->irq);
>>> +
>>> + bcm_iproc_i2c_reset(iproc_i2c);
>>> +
>>> + /* Switch to bit-bang mode */
>>> + tmp = readl(iproc_i2c->base + CFG_OFFSET);
>>> + tmp |= BIT(CFG_BIT_BANG_SHIFT);
>>> + writel(tmp, iproc_i2c->base + CFG_OFFSET);
>> add usleep_range(100, 200) here, required delay after switching to bit
>> bang based on spec.
>
> Ack.
>
>>
>>> +}
>>> +
>>> +static void bcm_iproc_i2c_unprepare_recovery(struct i2c_adapter *adap)
>>> +{
>>> + struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
>>> + u32 tmp;
>>> +
>>> + /* Switch to normal mode */
>>> + tmp = readl(iproc_i2c->base + CFG_OFFSET);
>>> + tmp &= ~BIT(CFG_BIT_BANG_SHIFT);
>>> + writel(tmp, iproc_i2c->base + CFG_OFFSET);
>>> + udelay(100);
>>> +
>>> + bcm_iproc_i2c_init(iproc_i2c);
>> Add sequence to re-configure to desired bus speed here after the reset
>> sequence (someone else in our team tested this is required to resume to
>> proper bus speed).
> bcm_iproc_i2c_resume() seems to have the exact sequence I need. If I
> move it outside of CONFIG_PM_SLEEP I could just call it here.
Yes agree with you that 'bcm_iproc_i2c_resume' can be resued.
>>> + bcm_iproc_i2c_enable_disable(iproc_i2c, true);
>>> +
>>> + dev_dbg(iproc_i2c->device, "Recovery complete\n");
>>> +}
>>> +
>>> +static int bcm_iproc_i2c_get_scl(struct i2c_adapter *adap)
>>> +{
>>> + struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
>>> + u32 tmp;
>>> +
>>> + tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);
>>> +
>>> + return !!(tmp & BIT(M_BB_SMBCLK_IN));
>>> +}
>>> +
>>> +static void bcm_iproc_i2c_set_scl(struct i2c_adapter *adap, int val)
>>> +{
>>> + struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
>>> + u32 tmp;
>>> +
>>> + tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);
>>> + if (val)
>>> + tmp |= BIT(M_BB_SMBCLK_OUT_EN);
>>> + else
>>> + tmp &= ~BIT(M_BB_SMBCLK_OUT_EN);
>>> +
>>> + writel(tmp, iproc_i2c->base + M_BB_CTRL_OFFSET);
>>> +}
>>> +
>>> +static void bcm_iproc_i2c_set_sda(struct i2c_adapter *adap, int val)
>>> +{
>>> + struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
>>> + u32 tmp;
>>> +
>>> + tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);
>>> + if (val)
>>> + tmp |= BIT(M_BB_SMBDAT_OUT_EN);
>>> + else
>>> + tmp &= ~BIT(M_BB_SMBDAT_OUT_EN);
>>> +
>>> + writel(tmp, iproc_i2c->base + M_BB_CTRL_OFFSET);
>>> +}
>>> +
>>> +static int bcm_iproc_i2c_get_sda(struct i2c_adapter *adap)
>>> +{
>>> + struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
>>> + u32 tmp;
>>> +
>>> + tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);
>>> +
>>> + return !!(tmp & BIT(M_BB_SMBDAT_IN));
>>> +}
>>> +
>>> +static struct i2c_bus_recovery_info bcm_iproc_recovery_info = {
>> static const struct ...
> Ack.
>>> + .recover_bus = i2c_generic_scl_recovery,
>>> + .prepare_recovery = bcm_iproc_i2c_prepare_recovery,
>>> + .unprepare_recovery = bcm_iproc_i2c_unprepare_recovery,
>>> + .set_scl = bcm_iproc_i2c_set_scl,
>>> + .get_scl = bcm_iproc_i2c_get_scl,
>>> + .set_sda = bcm_iproc_i2c_set_sda,
>>> + .get_sda = bcm_iproc_i2c_get_sda,
>>> +};
>>> +
>>> static int bcm_iproc_i2c_check_status(struct bcm_iproc_i2c_dev *iproc_i2c,
>>> struct i2c_msg *msg)
>>> {
>>> @@ -839,6 +952,7 @@ static int bcm_iproc_i2c_xfer_internal(struct bcm_iproc_i2c_dev *iproc_i2c,
>>> if (!!(iproc_i2c_rd_reg(iproc_i2c,
>>> M_CMD_OFFSET) & BIT(M_CMD_START_BUSY_SHIFT))) {
>>> dev_warn(iproc_i2c->device, "bus is busy\n");
>>> + i2c_recover_bus(&iproc_i2c->adapter);
>> 'i2c_recover_bus' should not be ALWAYS called here. You don't know if
>> bus is actually locked up or it's other issues that caused this.
>>
>> We need a logic to detect and confirm the lock up condition before
>> committing to recover operation:
>>
>> /* Check if bus lockup occurred, and invoke recovery if so. */
>> static void iproc_i2c_lockup_recover(struct bcm_iproc_i2c_dev *iproc_i2c)
>> {
>> /*
>> * assume bus lockup if SDA line is low;
>> * note that there is no need to switch to
>> * bit-bang mode for this check.
>> */
>> if (!bcm_iproc_i2c_get_sda(&iproc_i2c->adapter)) {
>> /* locked up - invoke i2c bus recovery. */
>> int ret = i2c_recover_bus(&iproc_i2c->adapter);
>> if (ret)
>> dev_err(iproc_i2c->device,
>> "bus recovery: error %d\n",
>> ret);
>> }
>> }
>>
>> 'iproc_i2c_lockup_recover' should be called in two locations in the driver:
>>
>> 1. After 'transaction timed out' (and after flush both TX/RX FIFOS)
>> 2. After 'bcm_iproc_i2c_check_status' failures (and after flush both
>> TX/RX FIFOs).
>
> I see bcm_iproc_i2c_check_status() has a "recovery" case on an unknown
> error. Should I remove that if I'm going to call
> iproc_i2c_lockup_recover() or check for ret != -EIO?
>
You may leave that (as first level recovery due to some potential
internal ASIC issues).
Let iproc_i2c_lockup_recover handles the bus lockup type of recovery
(after confirming the bus is indeed locked up).
>>
>>> return -EBUSY;
>>> }
>>>
>>> @@ -1111,6 +1225,7 @@ static int bcm_iproc_i2c_probe(struct platform_device *pdev)
>>> of_node_full_name(iproc_i2c->device->of_node));
>>> adap->algo = &bcm_iproc_algo;
>>> adap->quirks = &bcm_iproc_i2c_quirks;
>>> + adap->bus_recovery_info = &bcm_iproc_recovery_info;
>>> adap->dev.parent = &pdev->dev;
>>> adap->dev.of_node = pdev->dev.of_node;
>>>
>>>
>> Thanks,
>>
>> Ray
Download attachment "smime.p7s" of type "application/pkcs7-signature" (4194 bytes)
Powered by blists - more mailing lists