| Message ID | 20220222134632.18598-1-shubhrajyoti.datta@xilinx.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: cadence: Recover bus after controller reset | expand |
On 2022/2/22 21:46, Shubhrajyoti Datta wrote: > From: Chirag Parekh <chiragp@xilinx.com> > > This will save from potential lock-up caused when I2c master controller > resets in the middle of transfer and the slave is holding SDA line to > transmit more data. > > Signed-off-by: Chirag Parekh <chiragp@xilinx.com> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > --- > drivers/i2c/busses/i2c-cadence.c | 109 +++++++++++++++++++++++++++++++ > 1 file changed, 109 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c > index 805c77143a0f..682821481b67 100644 > --- a/drivers/i2c/busses/i2c-cadence.c > +++ b/drivers/i2c/busses/i2c-cadence.c > @@ -7,13 +7,16 @@ > > #include <linux/clk.h> > #include <linux/delay.h> > +#include <linux/gpio/consumer.h> > #include <linux/i2c.h> > #include <linux/interrupt.h> > #include <linux/io.h> > #include <linux/module.h> > #include <linux/platform_device.h> > #include <linux/of.h> > +#include <linux/of_gpio.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 */ > @@ -179,6 +182,10 @@ 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. > + * @pinctrl: Pin control state holder. > + * @pinctrl_pins_default: Default pin control state. > + * @pinctrl_pins_gpio: GPIO pin control state. > * @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 +211,10 @@ struct cdns_i2c { > struct notifier_block clk_rate_change_nb; > u32 quirks; > u32 ctrl_reg; > + struct i2c_bus_recovery_info rinfo; > + struct pinctrl *pinctrl; > + struct pinctrl_state *pinctrl_pins_default; > + struct pinctrl_state *pinctrl_pins_gpio; Some reason for managing the pin info in the cdns_i2c struct? Since you're using generic GPIO recovery, if you store these in struct i2c_bus_recovery_info the i2c core framework will help handling the pin mux on recovery and I think you don't need the prepare/unprepare() method anymore. > #if IS_ENABLED(CONFIG_I2C_SLAVE) > u16 ctrl_reg_diva_divb; > struct i2c_client *slave; > @@ -788,6 +799,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, adap->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"); > @@ -1208,6 +1220,96 @@ static int __maybe_unused cdns_i2c_runtime_resume(struct device *dev) > return 0; > } > > +/** > + * cdns_i2c_prepare_recovery - Withhold recovery state > + * @adapter: Pointer to i2c adapter > + * > + * This function is called to prepare for recovery. > + * It changes the state of pins from SCL/SDA to GPIO. > + */ > +static void cdns_i2c_prepare_recovery(struct i2c_adapter *adapter) > +{ > + struct cdns_i2c *p_cdns_i2c; > + int ret; > + > + p_cdns_i2c = container_of(adapter, struct cdns_i2c, adap); > + > + /* Setting pin state as gpio */ > + ret = pinctrl_select_state(p_cdns_i2c->pinctrl, > + p_cdns_i2c->pinctrl_pins_gpio); > + if (ret < 0) > + dev_err(p_cdns_i2c->adap.dev.parent, > + "pinctrl_select_state failed\n"); > +} > + > +/** > + * cdns_i2c_unprepare_recovery - Release recovery state > + * @adapter: Pointer to i2c adapter > + * > + * This function is called on exiting recovery. It reverts > + * the state of pins from GPIO to SCL/SDA. > + */ > +static void cdns_i2c_unprepare_recovery(struct i2c_adapter *adapter) > +{ > + struct cdns_i2c *p_cdns_i2c; > + int ret; > + > + p_cdns_i2c = container_of(adapter, struct cdns_i2c, adap); > + > + /* Setting pin state to default(i2c) */ > + ret = pinctrl_select_state(p_cdns_i2c->pinctrl, > + p_cdns_i2c->pinctrl_pins_default); > + if (ret < 0) > + dev_err(p_cdns_i2c->adap.dev.parent, > + "pinctrl_select_state failed\n"); > +} > + > +/** > + * cdns_i2c_init_recovery_info - Initialize I2C bus recovery > + * @pid: Pointer to cdns i2c structure > + * @pdev: Handle to the platform device structure > + * > + * This function does required initialization for i2c bus > + * recovery. It registers three functions for prepare, > + * recover and unprepare > + * > + * Return: 0 on Success, negative error otherwise. > + */ > +static int cdns_i2c_init_recovery_info(struct cdns_i2c *pid, > + struct platform_device *pdev) > +{ > + struct i2c_bus_recovery_info *rinfo = &pid->rinfo; > + > + pid->pinctrl_pins_default = pinctrl_lookup_state(pid->pinctrl, > + PINCTRL_STATE_DEFAULT); > + pid->pinctrl_pins_gpio = pinctrl_lookup_state(pid->pinctrl, "gpio"); > + > + /* Fetches GPIO pins */ > + rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda-gpios", GPIOD_ASIS); > + rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl-gpios", GPIOD_ASIS); > + > + /* if GPIO driver isn't ready yet, deffer probe */ > + if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER || > + PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + > + /* Validates fetched information */ > + if (IS_ERR(rinfo->sda_gpiod) || > + IS_ERR(rinfo->scl_gpiod) || > + IS_ERR(pid->pinctrl_pins_default) || > + IS_ERR(pid->pinctrl_pins_gpio)) { > + dev_dbg(&pdev->dev, "recovery information incomplete\n"); > + return 0; > + } > + > + rinfo->prepare_recovery = cdns_i2c_prepare_recovery; > + rinfo->unprepare_recovery = cdns_i2c_unprepare_recovery; > + rinfo->recover_bus = i2c_generic_scl_recovery; > + pid->adap.bus_recovery_info = rinfo; > + > + return 0; > +} > + > static const struct dev_pm_ops cdns_i2c_dev_pm_ops = { > SET_RUNTIME_PM_OPS(cdns_i2c_runtime_suspend, > cdns_i2c_runtime_resume, NULL) > @@ -1254,6 +1356,13 @@ static int cdns_i2c_probe(struct platform_device *pdev) > id->quirks = data->quirks; > } > > + id->pinctrl = devm_pinctrl_get(&pdev->dev); > + if (!IS_ERR(id->pinctrl)) { > + ret = cdns_i2c_init_recovery_info(id, pdev); > + if (ret) > + return ret; > + } > + > id->membase = devm_platform_get_and_ioremap_resource(pdev, 0, &r_mem); > if (IS_ERR(id->membase)) > return PTR_ERR(id->membase); >
On Tue, Feb 22, 2022 at 07:16:32PM +0530, Shubhrajyoti Datta wrote: > From: Chirag Parekh <chiragp@xilinx.com> > > This will save from potential lock-up caused when I2c master controller > resets in the middle of transfer and the slave is holding SDA line to > transmit more data. > > Signed-off-by: Chirag Parekh <chiragp@xilinx.com> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> The comment from last version has not been clarified or addressed. http://patchwork.ozlabs.org/project/linux-i2c/patch/20211129090116.16628-1-shubhrajyoti.datta@xilinx.com/
> -----Original Message----- > From: Wolfram Sang <wsa@kernel.org> > Sent: Wednesday, February 23, 2022 7:30 PM > To: Shubhrajyoti Datta <shubhraj@xilinx.com> > Cc: linux-i2c@vger.kernel.org; git <git@xilinx.com>; Michal Simek > <michals@xilinx.com>; linux-kernel@vger.kernel.org; Chirag Parekh > <chiragp@xilinx.com> > Subject: Re: [PATCH] i2c: cadence: Recover bus after controller reset > > On Tue, Feb 22, 2022 at 07:16:32PM +0530, Shubhrajyoti Datta wrote: > > From: Chirag Parekh <chiragp@xilinx.com> > > > > This will save from potential lock-up caused when I2c master > > controller resets in the middle of transfer and the slave is holding > > SDA line to transmit more data. > > > > Signed-off-by: Chirag Parekh <chiragp@xilinx.com> > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > The comment from last version has not been clarified or addressed. > > http://patchwork.ozlabs.org/project/linux- > i2c/patch/20211129090116.16628-1-shubhrajyoti.datta@xilinx.com/ I missed that apologies.
diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c index 805c77143a0f..682821481b67 100644 --- a/drivers/i2c/busses/i2c-cadence.c +++ b/drivers/i2c/busses/i2c-cadence.c @@ -7,13 +7,16 @@ #include <linux/clk.h> #include <linux/delay.h> +#include <linux/gpio/consumer.h> #include <linux/i2c.h> #include <linux/interrupt.h> #include <linux/io.h> #include <linux/module.h> #include <linux/platform_device.h> #include <linux/of.h> +#include <linux/of_gpio.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 */ @@ -179,6 +182,10 @@ 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. + * @pinctrl: Pin control state holder. + * @pinctrl_pins_default: Default pin control state. + * @pinctrl_pins_gpio: GPIO pin control state. * @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 +211,10 @@ struct cdns_i2c { struct notifier_block clk_rate_change_nb; u32 quirks; u32 ctrl_reg; + struct i2c_bus_recovery_info rinfo; + struct pinctrl *pinctrl; + struct pinctrl_state *pinctrl_pins_default; + struct pinctrl_state *pinctrl_pins_gpio; #if IS_ENABLED(CONFIG_I2C_SLAVE) u16 ctrl_reg_diva_divb; struct i2c_client *slave; @@ -788,6 +799,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, adap->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"); @@ -1208,6 +1220,96 @@ static int __maybe_unused cdns_i2c_runtime_resume(struct device *dev) return 0; } +/** + * cdns_i2c_prepare_recovery - Withhold recovery state + * @adapter: Pointer to i2c adapter + * + * This function is called to prepare for recovery. + * It changes the state of pins from SCL/SDA to GPIO. + */ +static void cdns_i2c_prepare_recovery(struct i2c_adapter *adapter) +{ + struct cdns_i2c *p_cdns_i2c; + int ret; + + p_cdns_i2c = container_of(adapter, struct cdns_i2c, adap); + + /* Setting pin state as gpio */ + ret = pinctrl_select_state(p_cdns_i2c->pinctrl, + p_cdns_i2c->pinctrl_pins_gpio); + if (ret < 0) + dev_err(p_cdns_i2c->adap.dev.parent, + "pinctrl_select_state failed\n"); +} + +/** + * cdns_i2c_unprepare_recovery - Release recovery state + * @adapter: Pointer to i2c adapter + * + * This function is called on exiting recovery. It reverts + * the state of pins from GPIO to SCL/SDA. + */ +static void cdns_i2c_unprepare_recovery(struct i2c_adapter *adapter) +{ + struct cdns_i2c *p_cdns_i2c; + int ret; + + p_cdns_i2c = container_of(adapter, struct cdns_i2c, adap); + + /* Setting pin state to default(i2c) */ + ret = pinctrl_select_state(p_cdns_i2c->pinctrl, + p_cdns_i2c->pinctrl_pins_default); + if (ret < 0) + dev_err(p_cdns_i2c->adap.dev.parent, + "pinctrl_select_state failed\n"); +} + +/** + * cdns_i2c_init_recovery_info - Initialize I2C bus recovery + * @pid: Pointer to cdns i2c structure + * @pdev: Handle to the platform device structure + * + * This function does required initialization for i2c bus + * recovery. It registers three functions for prepare, + * recover and unprepare + * + * Return: 0 on Success, negative error otherwise. + */ +static int cdns_i2c_init_recovery_info(struct cdns_i2c *pid, + struct platform_device *pdev) +{ + struct i2c_bus_recovery_info *rinfo = &pid->rinfo; + + pid->pinctrl_pins_default = pinctrl_lookup_state(pid->pinctrl, + PINCTRL_STATE_DEFAULT); + pid->pinctrl_pins_gpio = pinctrl_lookup_state(pid->pinctrl, "gpio"); + + /* Fetches GPIO pins */ + rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda-gpios", GPIOD_ASIS); + rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl-gpios", GPIOD_ASIS); + + /* if GPIO driver isn't ready yet, deffer probe */ + if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER || + PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER) + return -EPROBE_DEFER; + + /* Validates fetched information */ + if (IS_ERR(rinfo->sda_gpiod) || + IS_ERR(rinfo->scl_gpiod) || + IS_ERR(pid->pinctrl_pins_default) || + IS_ERR(pid->pinctrl_pins_gpio)) { + dev_dbg(&pdev->dev, "recovery information incomplete\n"); + return 0; + } + + rinfo->prepare_recovery = cdns_i2c_prepare_recovery; + rinfo->unprepare_recovery = cdns_i2c_unprepare_recovery; + rinfo->recover_bus = i2c_generic_scl_recovery; + pid->adap.bus_recovery_info = rinfo; + + return 0; +} + static const struct dev_pm_ops cdns_i2c_dev_pm_ops = { SET_RUNTIME_PM_OPS(cdns_i2c_runtime_suspend, cdns_i2c_runtime_resume, NULL) @@ -1254,6 +1356,13 @@ static int cdns_i2c_probe(struct platform_device *pdev) id->quirks = data->quirks; } + id->pinctrl = devm_pinctrl_get(&pdev->dev); + if (!IS_ERR(id->pinctrl)) { + ret = cdns_i2c_init_recovery_info(id, pdev); + if (ret) + return ret; + } + id->membase = devm_platform_get_and_ioremap_resource(pdev, 0, &r_mem); if (IS_ERR(id->membase)) return PTR_ERR(id->membase);