| Message ID | 1656914060-24445-1-git-send-email-shubhrajyoti.datta@xilinx.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] i2c: cadence: Add standard bus recovery support | expand |
On 7/4/22 07:54, Shubhrajyoti Datta wrote: > From: Robert Hancock <robert.hancock@calian.com> > > Hook up the standard GPIO/pinctrl-based recovery support.. > In the discurssion typo > https://patchwork.ozlabs.org/project/linux-i2c/patch/20211129090116.16628-1-shubhrajyoti.datta@xilinx.com/ > > recovery should be done at the beginning of the transaction. > Here we are doing the recovery at the beginning on a timeout. > > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com> > --- > v2: > Updated the busbusy check on a timeout > > drivers/i2c/busses/i2c-cadence.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c > index b4c1ad19cdae..cf15eca1f9e4 100644 > --- a/drivers/i2c/busses/i2c-cadence.c > +++ b/drivers/i2c/busses/i2c-cadence.c > @@ -10,6 +10,7 @@ > #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> > @@ -125,6 +126,8 @@ > #define CDNS_I2C_DIVB_MAX 64 > > #define CDNS_I2C_TIMEOUT_MAX 0xFF > +#define CDNS_I2C_POLL_US 100000 > +#define CDNS_I2C_TIMEOUT_US 500000 > > #define CDNS_I2C_BROKEN_HOLD_BIT BIT(0) > > @@ -179,6 +182,7 @@ enum cdns_i2c_slave_state { > * @clk_rate_change_nb: Notifier block for clock rate changes > * @quirks: flag for broken hold bit usage in r1p10 > * @ctrl_reg: Cached value of the control register. > + * @rinfo: Structure holding recovery information. > * @ctrl_reg_diva_divb: value of fields DIV_A and DIV_B from CR register > * @slave: Registered slave instance. > * @dev_mode: I2C operating role(master/slave). > @@ -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; > @@ -796,6 +801,7 @@ static int cdns_i2c_process_msg(struct cdns_i2c *id, struct i2c_msg *msg, > /* Wait for the signal of completion */ > time_left = wait_for_completion_timeout(&id->xfer_done, msg_timeout); > if (time_left == 0) { > + i2c_recover_bus(adap); > cdns_i2c_master_reset(adap); > dev_err(id->adap.dev.parent, > "timeout waiting on completion\n"); > @@ -852,8 +858,12 @@ 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_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; > } > > @@ -1278,6 +1288,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); I have no problem with it. I expect you have tested it on the real HW. Acked-by: Michal Simek <michal.simek@amd.com> Thanks, Michal
Hi, On Mon, Jul 04, 2022 at 11:24:20AM +0530, Shubhrajyoti Datta wrote: > From: Robert Hancock <robert.hancock@calian.com> > > Hook up the standard GPIO/pinctrl-based recovery support.. > In the discurssion > https://patchwork.ozlabs.org/project/linux-i2c/patch/20211129090116.16628-1-shubhrajyoti.datta@xilinx.com/ > > recovery should be done at the beginning of the transaction. > Here we are doing the recovery at the beginning on a timeout. Which is still wrong. > > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com> This is an AMD address, but the one you sent from is from Xilinx? > if (time_left == 0) { > + i2c_recover_bus(adap); This is the wrong part. > cdns_i2c_master_reset(adap); > dev_err(id->adap.dev.parent, > "timeout waiting on completion\n"); > @@ -852,8 +858,12 @@ 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_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; This is proper. > } > > @@ -1278,6 +1288,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; Since 'rinfo' is never populated with actual data, I am quite sure this patch has never been tested. Regards, Wolfram
On Thu, 2022-07-07 at 23:03 +0200, Wolfram Sang wrote: > Hi, > > On Mon, Jul 04, 2022 at 11:24:20AM +0530, Shubhrajyoti Datta wrote: > > From: Robert Hancock <robert.hancock@calian.com> > > > > Hook up the standard GPIO/pinctrl-based recovery support.. > > In the discurssion > > https://patchwork.ozlabs.org/project/linux-i2c/patch/20211129090116.16628-1-shubhrajyoti.datta@xilinx.com/ > > > > recovery should be done at the beginning of the transaction. > > Here we are doing the recovery at the beginning on a timeout. > > Which is still wrong. > > > > > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com> > > This is an AMD address, but the one you sent from is from Xilinx? > > > if (time_left == 0) { > > + i2c_recover_bus(adap); > > This is the wrong part. > > > cdns_i2c_master_reset(adap); > > dev_err(id->adap.dev.parent, > > "timeout waiting on completion\n"); > > @@ -852,8 +858,12 @@ 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_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; > > This is proper. > > > } > > > > @@ -1278,6 +1288,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; > > Since 'rinfo' is never populated with actual data, I am quite sure > this > patch has never been tested. I think this (setting bus_recovery_info to point to a zeroed structure) is sufficient to allow the generic recovery initialization in i2c-core- base.c to work. i2c_gpio_init_recovery should fill in the required info based on the available pinctrl and gpio configuration in this case. > > Regards, > > Wolfram >
[AMD Official Use Only - General] > -----Original Message----- > From: Robert Hancock <robert.hancock@calian.com> > Sent: Friday, July 8, 2022 3:45 AM > To: shubhrajyoti.datta@xilinx.com; wsa@kernel.org > Cc: michal.simek@xilinx.com; linux-i2c@vger.kernel.org; Datta, Shubhrajyoti > <shubhrajyoti.datta@amd.com> > Subject: Re: [PATCH v2] i2c: cadence: Add standard bus recovery support > > [CAUTION: External Email] > > On Thu, 2022-07-07 at 23:03 +0200, Wolfram Sang wrote: > > Hi, > > > > On Mon, Jul 04, 2022 at 11:24:20AM +0530, Shubhrajyoti Datta wrote: > > > From: Robert Hancock <robert.hancock@calian.com> > > > > > > Hook up the standard GPIO/pinctrl-based recovery support.. > > > In the discurssion > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa > > > tchwork.ozlabs.org%2Fproject%2Flinux- > i2c%2Fpatch%2F20211129090116.16 > > > 628-1- > shubhrajyoti.datta%40xilinx.com%2F&data=05%7C01%7Cshubhraj > > > > yoti.datta%40amd.com%7C4e1a91e059a8495756a208da60661551%7C3dd89 > 61fe4 > > > > 884e608e11a82d994e183d%7C0%7C0%7C637928288864536085%7CUnknow > n%7CTWFp > > > > bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI > 6 > > > > Mn0%3D%7C3000%7C%7C%7C&sdata=xhjvSNSf1%2FO5ihd%2B92O%2B > LCOci265Q > > > R8HiNh%2B8TBWXw8%3D&reserved=0 > > > > > > recovery should be done at the beginning of the transaction. > > > Here we are doing the recovery at the beginning on a timeout. > > > > Which is still wrong. Will fix this. > > > > > > > > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com> > > > > This is an AMD address, but the one you sent from is from Xilinx? > > > > > if (time_left == 0) { > > > + i2c_recover_bus(adap); > > > > This is the wrong part. Will fix > > > > > cdns_i2c_master_reset(adap); > > > dev_err(id->adap.dev.parent, > > > "timeout waiting on completion\n"); > > > @@ -852,8 +858,12 @@ 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_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; > > > > This is proper. > > > > > } > > > > > > @@ -1278,6 +1288,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; > > > > Since 'rinfo' is never populated with actual data, I am quite sure > > this patch has never been tested. > > I think this (setting bus_recovery_info to point to a zeroed structure) is > sufficient to allow the generic recovery initialization in i2c-core- base.c to > work. i2c_gpio_init_recovery should fill in the required info based on the > available pinctrl and gpio configuration in this case. I checked the handler calls however I will try to check with a setup where I can probe and Verify the clock cycles. > > > > > Regards, > > > > Wolfram > >
diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c index b4c1ad19cdae..cf15eca1f9e4 100644 --- a/drivers/i2c/busses/i2c-cadence.c +++ b/drivers/i2c/busses/i2c-cadence.c @@ -10,6 +10,7 @@ #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> @@ -125,6 +126,8 @@ #define CDNS_I2C_DIVB_MAX 64 #define CDNS_I2C_TIMEOUT_MAX 0xFF +#define CDNS_I2C_POLL_US 100000 +#define CDNS_I2C_TIMEOUT_US 500000 #define CDNS_I2C_BROKEN_HOLD_BIT BIT(0) @@ -179,6 +182,7 @@ enum cdns_i2c_slave_state { * @clk_rate_change_nb: Notifier block for clock rate changes * @quirks: flag for broken hold bit usage in r1p10 * @ctrl_reg: Cached value of the control register. + * @rinfo: Structure holding recovery information. * @ctrl_reg_diva_divb: value of fields DIV_A and DIV_B from CR register * @slave: Registered slave instance. * @dev_mode: I2C operating role(master/slave). @@ -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; @@ -796,6 +801,7 @@ static int cdns_i2c_process_msg(struct cdns_i2c *id, struct i2c_msg *msg, /* Wait for the signal of completion */ time_left = wait_for_completion_timeout(&id->xfer_done, msg_timeout); if (time_left == 0) { + i2c_recover_bus(adap); cdns_i2c_master_reset(adap); dev_err(id->adap.dev.parent, "timeout waiting on completion\n"); @@ -852,8 +858,12 @@ 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_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; } @@ -1278,6 +1288,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);