| Message ID | 20220728055150.18368-1-shubhrajyoti.datta@xilinx.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v3] i2c: cadence: Add standard bus recovery support | expand |
On 7/28/22 07:51, Shubhrajyoti Datta wrote: > Hook up the standard GPIO/pinctrl-based recovery support. > We are doing the recovery at the beginning on a timeout. > > Multiple people have contributed to the series. > Original patch from Cirag and another one from Robert. > > Cc: Chirag Parekh <chiragp@xilinx.com> > Cc: Robert Hancock <robert.hancock@calian.com> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > --- > v2: > Updated the busbusy check on a timeout > v3: > Added pinctrl_get > > Did unit testing and probed the scl to see the clock pulses. Can you please describe testing procedure? What board did you use? What was the hardware configuration? Thanks, Michal
[AMD Official Use Only - General] > -----Original Message----- > From: Simek, Michal <michal.simek@amd.com> > Sent: Friday, July 29, 2022 4:26 PM > To: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>; linux- > i2c@vger.kernel.org > Cc: michal.simek@xilinx.com; git-dev (AMD-Xilinx) <git-dev@amd.com>; > Chirag Parekh <chiragp@xilinx.com>; Robert Hancock > <robert.hancock@calian.com> > Subject: Re: [PATCH v3] i2c: cadence: Add standard bus recovery support > > > > On 7/28/22 07:51, Shubhrajyoti Datta wrote: > > Hook up the standard GPIO/pinctrl-based recovery support. > > We are doing the recovery at the beginning on a timeout. > > > > Multiple people have contributed to the series. > > Original patch from Cirag and another one from Robert. > > > > Cc: Chirag Parekh <chiragp@xilinx.com> > > Cc: Robert Hancock <robert.hancock@calian.com> > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > --- > > v2: > > Updated the busbusy check on a timeout > > v3: > > Added pinctrl_get > > > > Did unit testing and probed the scl to see the clock pulses. > > Can you please describe testing procedure? What board did you use? > What was the hardware configuration? Tested on zcu102 . and then I called the recovery in send setup . Also I had added the prints in the To confirm that the core was calling the zynqmp pinctrl to set pin mux. The scl line was Probed. I could see the clock cycles in the scl line. [ 84.286340] zynqmp-pinctrl firmware:zynqmp-firmware:pinctrl: set mux for pin 14 [ 84.293779] zynqmp-pinctrl firmware:zynqmp-firmware:pinctrl: set mux for pin 15 [ 84.301266] i2c-core: set_scl_gpio_value 164 1 [ 84.305807] i2c i2c-0: SCL is set_scl 1 i ia 1 [ 84.310331] i2c-core: get_scl_gpio_value 158 [ 84.314681] i2c-core: set_scl_gpio_value 164 0 [ 84.322519] i2c i2c-0: SCL is set_scl 0 [ 84.326439] i2c i2c-0: SCL is set_scl 0 i ia 2 [ 84.330965] i2c-core: set_scl_gpio_value 164 1 [ 84.335496] i2c i2c-0: SCL is set_scl 1 [ 84.339418] i2c i2c-0: SCL is set_scl 1 i ia 3 [ 84.343947] i2c-core: get_scl_gpio_value 158 [ 84.348302] i2c-core: set_scl_gpio_value 164 0 [ 84.356135] i2c i2c-0: SCL is set_scl 0 [ 84.360059] i2c i2c-0: SCL is set_scl 0 i ia 4 [ 84.364586] i2c-core: set_scl_gpio_value 164 1 [ 84.369117] i2c i2c-0: SCL is set_scl 1 [ 84.373037] i2c i2c-0: SCL is set_scl 1 i ia 5 [ 84.377564] i2c-core: get_scl_gpio_value 158 [ 84.381914] i2c-core: set_scl_gpio_value 164 0 > > Thanks, > Michal
On 7/29/22 13:05, Datta, Shubhrajyoti wrote: > [AMD Official Use Only - General] > > > >> -----Original Message----- >> From: Simek, Michal <michal.simek@amd.com> >> Sent: Friday, July 29, 2022 4:26 PM >> To: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>; linux- >> i2c@vger.kernel.org >> Cc: michal.simek@xilinx.com; git-dev (AMD-Xilinx) <git-dev@amd.com>; >> Chirag Parekh <chiragp@xilinx.com>; Robert Hancock >> <robert.hancock@calian.com> >> Subject: Re: [PATCH v3] i2c: cadence: Add standard bus recovery support >> >> >> >> On 7/28/22 07:51, Shubhrajyoti Datta wrote: >>> Hook up the standard GPIO/pinctrl-based recovery support. >>> We are doing the recovery at the beginning on a timeout. >>> >>> Multiple people have contributed to the series. >>> Original patch from Cirag and another one from Robert. >>> >>> Cc: Chirag Parekh <chiragp@xilinx.com> >>> Cc: Robert Hancock <robert.hancock@calian.com> >>> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> >>> --- >>> v2: >>> Updated the busbusy check on a timeout >>> v3: >>> Added pinctrl_get >>> >>> Did unit testing and probed the scl to see the clock pulses. >> >> Can you please describe testing procedure? What board did you use? >> What was the hardware configuration? > > > Tested on zcu102 . and then I called the recovery in send setup . > Also I had added the prints in the > To confirm that the core was calling the zynqmp pinctrl to set pin mux. > > The scl line was Probed. I could see the clock cycles in the scl line. > > [ 84.286340] zynqmp-pinctrl firmware:zynqmp-firmware:pinctrl: set mux for pin 14 > [ 84.293779] zynqmp-pinctrl firmware:zynqmp-firmware:pinctrl: set mux for pin 15 > [ 84.301266] i2c-core: set_scl_gpio_value 164 1 > [ 84.305807] i2c i2c-0: SCL is set_scl 1 i ia 1 > [ 84.310331] i2c-core: get_scl_gpio_value 158 > [ 84.314681] i2c-core: set_scl_gpio_value 164 0 > [ 84.322519] i2c i2c-0: SCL is set_scl 0 > [ 84.326439] i2c i2c-0: SCL is set_scl 0 i ia 2 > [ 84.330965] i2c-core: set_scl_gpio_value 164 1 > [ 84.335496] i2c i2c-0: SCL is set_scl 1 > [ 84.339418] i2c i2c-0: SCL is set_scl 1 i ia 3 > [ 84.343947] i2c-core: get_scl_gpio_value 158 > [ 84.348302] i2c-core: set_scl_gpio_value 164 0 > [ 84.356135] i2c i2c-0: SCL is set_scl 0 > [ 84.360059] i2c i2c-0: SCL is set_scl 0 i ia 4 > [ 84.364586] i2c-core: set_scl_gpio_value 164 1 > [ 84.369117] i2c i2c-0: SCL is set_scl 1 > [ 84.373037] i2c i2c-0: SCL is set_scl 1 i ia 5 > [ 84.377564] i2c-core: get_scl_gpio_value 158 > [ 84.381914] i2c-core: set_scl_gpio_value 164 0 ok. Thanks. Acked-by: Michal Simek <michal.simek@amd.com> Thanks, Michal
Hi Wolfram, On 7/29/22 13:17, Michal Simek wrote: > > > On 7/29/22 13:05, Datta, Shubhrajyoti wrote: >> [AMD Official Use Only - General] >> >> >> >>> -----Original Message----- >>> From: Simek, Michal <michal.simek@amd.com> >>> Sent: Friday, July 29, 2022 4:26 PM >>> To: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>; linux- >>> i2c@vger.kernel.org >>> Cc: michal.simek@xilinx.com; git-dev (AMD-Xilinx) <git-dev@amd.com>; >>> Chirag Parekh <chiragp@xilinx.com>; Robert Hancock >>> <robert.hancock@calian.com> >>> Subject: Re: [PATCH v3] i2c: cadence: Add standard bus recovery support >>> >>> >>> >>> On 7/28/22 07:51, Shubhrajyoti Datta wrote: >>>> Hook up the standard GPIO/pinctrl-based recovery support. >>>> We are doing the recovery at the beginning on a timeout. >>>> >>>> Multiple people have contributed to the series. >>>> Original patch from Cirag and another one from Robert. >>>> >>>> Cc: Chirag Parekh <chiragp@xilinx.com> >>>> Cc: Robert Hancock <robert.hancock@calian.com> >>>> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> >>>> --- >>>> v2: >>>> Updated the busbusy check on a timeout >>>> v3: >>>> Added pinctrl_get >>>> >>>> Did unit testing and probed the scl to see the clock pulses. >>> >>> Can you please describe testing procedure? What board did you use? >>> What was the hardware configuration? >> >> >> Tested on zcu102 . and then I called the recovery in send setup . >> Also I had added the prints in the >> To confirm that the core was calling the zynqmp pinctrl to set pin mux. >> >> The scl line was Probed. I could see the clock cycles in the scl line. >> >> [ 84.286340] zynqmp-pinctrl firmware:zynqmp-firmware:pinctrl: set mux for >> pin 14 >> [ 84.293779] zynqmp-pinctrl firmware:zynqmp-firmware:pinctrl: set mux for >> pin 15 >> [ 84.301266] i2c-core: set_scl_gpio_value 164 1 >> [ 84.305807] i2c i2c-0: SCL is set_scl 1 i ia 1 >> [ 84.310331] i2c-core: get_scl_gpio_value 158 >> [ 84.314681] i2c-core: set_scl_gpio_value 164 0 >> [ 84.322519] i2c i2c-0: SCL is set_scl 0 >> [ 84.326439] i2c i2c-0: SCL is set_scl 0 i ia 2 >> [ 84.330965] i2c-core: set_scl_gpio_value 164 1 >> [ 84.335496] i2c i2c-0: SCL is set_scl 1 >> [ 84.339418] i2c i2c-0: SCL is set_scl 1 i ia 3 >> [ 84.343947] i2c-core: get_scl_gpio_value 158 >> [ 84.348302] i2c-core: set_scl_gpio_value 164 0 >> [ 84.356135] i2c i2c-0: SCL is set_scl 0 >> [ 84.360059] i2c i2c-0: SCL is set_scl 0 i ia 4 >> [ 84.364586] i2c-core: set_scl_gpio_value 164 1 >> [ 84.369117] i2c i2c-0: SCL is set_scl 1 >> [ 84.373037] i2c i2c-0: SCL is set_scl 1 i ia 5 >> [ 84.377564] i2c-core: get_scl_gpio_value 158 >> [ 84.381914] i2c-core: set_scl_gpio_value 164 0 > > ok. Thanks. > > Acked-by: Michal Simek <michal.simek@amd.com> > Do you see any issue with this version? Thanks, Michal
On Thu, Jul 28, 2022 at 11:21:50AM +0530, Shubhrajyoti Datta wrote: > Hook up the standard GPIO/pinctrl-based recovery support. > We are doing the recovery at the beginning on a timeout. > > Multiple people have contributed to the series. > Original patch from Cirag and another one from Robert. > > Cc: Chirag Parekh <chiragp@xilinx.com> > Cc: Robert Hancock <robert.hancock@calian.com> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> Applied to for-next, thanks!
> Applied to for-next, thanks!
Unrelated to this patch, but cppcheck found this issue:
drivers/i2c/busses/i2c-cadence.c:1038:33: warning: Condition 'actual_fscl>fscl' is always false [knownConditionTrueFalse]
current_error = ((actual_fscl > fscl) ? (actual_fscl - fscl) :
^
drivers/i2c/busses/i2c-cadence.c:1035:19: note: Assuming that condition 'actual_fscl>fscl' is not redundant
if (actual_fscl > fscl)
^
drivers/i2c/busses/i2c-cadence.c:1038:33: note: Condition 'actual_fscl>fscl' is always false
current_error = ((actual_fscl > fscl) ? (actual_fscl - fscl) :
I had a glimpse and I think the checker is correct...
Hi Wolfram, On 9/27/22 22:35, Wolfram Sang wrote: > >> Applied to for-next, thanks! > > Unrelated to this patch, but cppcheck found this issue: > > drivers/i2c/busses/i2c-cadence.c:1038:33: warning: Condition 'actual_fscl>fscl' is always false [knownConditionTrueFalse] > current_error = ((actual_fscl > fscl) ? (actual_fscl - fscl) : > ^ > drivers/i2c/busses/i2c-cadence.c:1035:19: note: Assuming that condition 'actual_fscl>fscl' is not redundant > if (actual_fscl > fscl) > ^ > drivers/i2c/busses/i2c-cadence.c:1038:33: note: Condition 'actual_fscl>fscl' is always false > current_error = ((actual_fscl > fscl) ? (actual_fscl - fscl) : > > I had a glimpse and I think the checker is correct... Are you still using your ninja-check script? Can you please share your latest version? Thanks, Michal
diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c index 630cfa4ddd46..c9a10d4297a9 100644 --- a/drivers/i2c/busses/i2c-cadence.c +++ b/drivers/i2c/busses/i2c-cadence.c @@ -10,10 +10,12 @@ #include <linux/i2c.h> #include <linux/interrupt.h> #include <linux/io.h> +#include <linux/iopoll.h> #include <linux/module.h> #include <linux/platform_device.h> #include <linux/of.h> #include <linux/pm_runtime.h> +#include <linux/pinctrl/consumer.h> /* Register offsets for the I2C device. */ #define CDNS_I2C_CR_OFFSET 0x00 /* Control Register, RW */ @@ -127,6 +129,8 @@ #define CDNS_I2C_TIMEOUT_MAX 0xFF #define CDNS_I2C_BROKEN_HOLD_BIT BIT(0) +#define CDNS_I2C_POLL_US 100000 +#define CDNS_I2C_TIMEOUT_US 500000 #define cdns_i2c_readreg(offset) readl_relaxed(id->membase + offset) #define cdns_i2c_writereg(val, offset) writel_relaxed(val, id->membase + offset) @@ -204,6 +208,7 @@ struct cdns_i2c { struct notifier_block clk_rate_change_nb; u32 quirks; u32 ctrl_reg; + struct i2c_bus_recovery_info rinfo; #if IS_ENABLED(CONFIG_I2C_SLAVE) u16 ctrl_reg_diva_divb; struct i2c_client *slave; @@ -832,8 +837,14 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, #endif /* Check if the bus is free */ - if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA) { + + ret = readl_relaxed_poll_timeout(id->membase + CDNS_I2C_SR_OFFSET, + reg, + !(reg & CDNS_I2C_SR_BA), + CDNS_I2C_POLL_US, CDNS_I2C_TIMEOUT_US); + if (ret) { ret = -EAGAIN; + i2c_recover_bus(adap); goto out; } @@ -1242,6 +1253,12 @@ static int cdns_i2c_probe(struct platform_device *pdev) id->quirks = data->quirks; } + id->rinfo.pinctrl = devm_pinctrl_get(&pdev->dev); + if (IS_ERR(id->rinfo.pinctrl)) { + dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n"); + return PTR_ERR(id->rinfo.pinctrl); + } + id->membase = devm_platform_get_and_ioremap_resource(pdev, 0, &r_mem); if (IS_ERR(id->membase)) return PTR_ERR(id->membase); @@ -1258,6 +1275,7 @@ static int cdns_i2c_probe(struct platform_device *pdev) id->adap.retries = 3; /* Default retry value. */ id->adap.algo_data = id; id->adap.dev.parent = &pdev->dev; + id->adap.bus_recovery_info = &id->rinfo; init_completion(&id->xfer_done); snprintf(id->adap.name, sizeof(id->adap.name), "Cadence I2C at %08lx", (unsigned long)r_mem->start);
Hook up the standard GPIO/pinctrl-based recovery support. We are doing the recovery at the beginning on a timeout. Multiple people have contributed to the series. Original patch from Cirag and another one from Robert. Cc: Chirag Parekh <chiragp@xilinx.com> Cc: Robert Hancock <robert.hancock@calian.com> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> --- v2: Updated the busbusy check on a timeout v3: Added pinctrl_get Did unit testing and probed the scl to see the clock pulses. drivers/i2c/busses/i2c-cadence.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)