| Message ID | 20210519074934.20712-6-quan@os.amperecomputing.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add Aspeed SSIF BMC driver | expand |
> -----Original Message----- > From: openbmc > <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On Behalf > Of Quan Nguyen > Sent: Wednesday, May 19, 2021 3:50 PM > To: Corey Minyard <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; > Joel Stanley <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan > Higgins <brendanhiggins@google.com>; Benjamin Herrenschmidt > <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel > <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net; > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org > Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q . > Nguyen <thang@os.amperecomputing.com>; Phong Vo > <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org > Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() > > Slave i2c device on AST2500 received a lot of slave irq while it is busy > processing the response. To handle this case, adds and exports > aspeed_set_slave_busy() for controller to temporary stop slave irq while slave > is handling the response, and re-enable them again when the response is ready. > > Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> > --- > v3: > + First introduce in v3 [Quan] > > drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > index b2e9c8f0ddf7..9926d04831a2 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus > *bus, > return 0; > } > > +#if IS_ENABLED(CONFIG_I2C_SLAVE) > +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) { > + struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap); > + unsigned long current_mask, flags; > + > + spin_lock_irqsave(&bus->lock, flags); > + > + current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG); > + if (busy) > + current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE | > ASPEED_I2CD_INTR_SLAVE_MATCH); > + else > + current_mask |= ASPEED_I2CD_INTR_RX_DONE | > ASPEED_I2CD_INTR_SLAVE_MATCH; > + writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG); > + > + spin_unlock_irqrestore(&bus->lock, flags); } > +EXPORT_SYMBOL_GPL(aspeed_set_slave_busy); > +#endif > + > static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus) { > struct platform_device *pdev = to_platform_device(bus->dev); > -- > 2.28.0 Hello, The better idea is use disable i2c slave mode. Due to if i2c controller running in slave will get slave match, and latch the SCL. Until cpu clear interrupt status. Ryan
On 20/05/2021 18:06, Ryan Chen wrote: >> -----Original Message----- >> From: openbmc >> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On Behalf >> Of Quan Nguyen >> Sent: Wednesday, May 19, 2021 3:50 PM >> To: Corey Minyard <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; >> Joel Stanley <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan >> Higgins <brendanhiggins@google.com>; Benjamin Herrenschmidt >> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel >> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net; >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; >> linux-i2c@vger.kernel.org >> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q . >> Nguyen <thang@os.amperecomputing.com>; Phong Vo >> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org >> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() >> >> Slave i2c device on AST2500 received a lot of slave irq while it is busy >> processing the response. To handle this case, adds and exports >> aspeed_set_slave_busy() for controller to temporary stop slave irq while slave >> is handling the response, and re-enable them again when the response is ready. >> >> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> >> --- >> v3: >> + First introduce in v3 [Quan] >> >> drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c >> index b2e9c8f0ddf7..9926d04831a2 100644 >> --- a/drivers/i2c/busses/i2c-aspeed.c >> +++ b/drivers/i2c/busses/i2c-aspeed.c >> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus >> *bus, >> return 0; >> } >> >> +#if IS_ENABLED(CONFIG_I2C_SLAVE) >> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) { >> + struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap); >> + unsigned long current_mask, flags; >> + >> + spin_lock_irqsave(&bus->lock, flags); >> + >> + current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG); >> + if (busy) >> + current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE | >> ASPEED_I2CD_INTR_SLAVE_MATCH); >> + else >> + current_mask |= ASPEED_I2CD_INTR_RX_DONE | >> ASPEED_I2CD_INTR_SLAVE_MATCH; >> + writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG); >> + >> + spin_unlock_irqrestore(&bus->lock, flags); } >> +EXPORT_SYMBOL_GPL(aspeed_set_slave_busy); >> +#endif >> + >> static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus) { >> struct platform_device *pdev = to_platform_device(bus->dev); >> -- >> 2.28.0 > > Hello, > The better idea is use disable i2c slave mode. > Due to if i2c controller running in slave will get slave match, and latch the SCL. > Until cpu clear interrupt status. > Ryan > Thanks Ryan, Do you mean to enable/disable slave function as per example code below ? /* Turn on slave mode. */ func_ctrl_reg_val = readl(bus->base + ASPEED_I2C_FUN_CTRL_REG); func_ctrl_reg_val |= ASPEED_I2CD_SLAVE_EN; writel(func_ctrl_reg_val, bus->base + ASPEED_I2C_FUN_CTRL_REG); Will try this idea. - Quan
> -----Original Message----- > From: Quan Nguyen <quan@os.amperecomputing.com> > Sent: Thursday, May 20, 2021 10:10 PM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard > <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley > <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins > <brendanhiggins@google.com>; Benjamin Herrenschmidt > <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel > <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net; > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org > Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q . > Nguyen <thang@os.amperecomputing.com>; Phong Vo > <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org > Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() > > On 20/05/2021 18:06, Ryan Chen wrote: > >> -----Original Message----- > >> From: openbmc > >> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On > Behalf > >> Of Quan Nguyen > >> Sent: Wednesday, May 19, 2021 3:50 PM > >> To: Corey Minyard <minyard@acm.org>; Rob Herring > >> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery > >> <andrew@aj.id.au>; Brendan Higgins <brendanhiggins@google.com>; > >> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Wolfram Sang > >> <wsa@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>; > >> openipmi-developer@lists.sourceforge.net; > >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; > >> linux-i2c@vger.kernel.org > >> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q . > >> Nguyen <thang@os.amperecomputing.com>; Phong Vo > >> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org > >> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() > >> > >> Slave i2c device on AST2500 received a lot of slave irq while it is > >> busy processing the response. To handle this case, adds and exports > >> aspeed_set_slave_busy() for controller to temporary stop slave irq > >> while slave is handling the response, and re-enable them again when the > response is ready. > >> > >> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> > >> --- > >> v3: > >> + First introduce in v3 [Quan] > >> > >> drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++ > >> 1 file changed, 20 insertions(+) > >> > >> diff --git a/drivers/i2c/busses/i2c-aspeed.c > >> b/drivers/i2c/busses/i2c-aspeed.c index b2e9c8f0ddf7..9926d04831a2 > >> 100644 > >> --- a/drivers/i2c/busses/i2c-aspeed.c > >> +++ b/drivers/i2c/busses/i2c-aspeed.c > >> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus > >> *bus, > >> return 0; > >> } > >> > >> +#if IS_ENABLED(CONFIG_I2C_SLAVE) > >> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) { > >> + struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap); > >> + unsigned long current_mask, flags; > >> + > >> + spin_lock_irqsave(&bus->lock, flags); > >> + > >> + current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG); > >> + if (busy) > >> + current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE | > >> ASPEED_I2CD_INTR_SLAVE_MATCH); > >> + else > >> + current_mask |= ASPEED_I2CD_INTR_RX_DONE | > >> ASPEED_I2CD_INTR_SLAVE_MATCH; > >> + writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG); > >> + > >> + spin_unlock_irqrestore(&bus->lock, flags); } > >> +EXPORT_SYMBOL_GPL(aspeed_set_slave_busy); > >> +#endif > >> + > >> static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus) { > >> struct platform_device *pdev = to_platform_device(bus->dev); > >> -- > >> 2.28.0 > > > > Hello, > > The better idea is use disable i2c slave mode. > > Due to if i2c controller running in slave will get slave match, and latch the > SCL. > > Until cpu clear interrupt status. > > Ryan > > > Thanks Ryan, > > Do you mean to enable/disable slave function as per example code below ? Yes. it is. > > /* Turn on slave mode. */ > func_ctrl_reg_val = readl(bus->base + > ASPEED_I2C_FUN_CTRL_REG); > func_ctrl_reg_val |= ASPEED_I2CD_SLAVE_EN; > writel(func_ctrl_reg_val, bus->base + > ASPEED_I2C_FUN_CTRL_REG); > > Will try this idea. > - Quan
> -----Original Message----- > From: openbmc > <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On Behalf > Of Quan Nguyen > Sent: Wednesday, May 19, 2021 3:50 PM > To: Corey Minyard <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; > Joel Stanley <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan > Higgins <brendanhiggins@google.com>; Benjamin Herrenschmidt > <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel > <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net; > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org > Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q . > Nguyen <thang@os.amperecomputing.com>; Phong Vo > <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org > Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() > > Slave i2c device on AST2500 received a lot of slave irq while it is busy > processing the response. To handle this case, adds and exports > aspeed_set_slave_busy() for controller to temporary stop slave irq while slave > is handling the response, and re-enable them again when the response is ready. > > Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> > --- > v3: > + First introduce in v3 [Quan] > > drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > index b2e9c8f0ddf7..9926d04831a2 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus > *bus, > return 0; > } > > +#if IS_ENABLED(CONFIG_I2C_SLAVE) > +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) { > + struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap); > + unsigned long current_mask, flags; > + > + spin_lock_irqsave(&bus->lock, flags); > + > + current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG); Hello Where the bus->base to be remap? > + if (busy) > + current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE | > ASPEED_I2CD_INTR_SLAVE_MATCH); > + else > + current_mask |= ASPEED_I2CD_INTR_RX_DONE | > ASPEED_I2CD_INTR_SLAVE_MATCH; > + writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG); > + > + spin_unlock_irqrestore(&bus->lock, flags); } > +EXPORT_SYMBOL_GPL(aspeed_set_slave_busy); > +#endif > + > static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus) { > struct platform_device *pdev = to_platform_device(bus->dev); > -- > 2.28.0
On 24/05/2021 17:06, Ryan Chen wrote: >> -----Original Message----- >> From: openbmc >> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On Behalf >> Of Quan Nguyen >> Sent: Wednesday, May 19, 2021 3:50 PM >> To: Corey Minyard <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; >> Joel Stanley <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan >> Higgins <brendanhiggins@google.com>; Benjamin Herrenschmidt >> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel >> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net; >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; >> linux-i2c@vger.kernel.org >> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q . >> Nguyen <thang@os.amperecomputing.com>; Phong Vo >> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org >> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() >> >> Slave i2c device on AST2500 received a lot of slave irq while it is busy >> processing the response. To handle this case, adds and exports >> aspeed_set_slave_busy() for controller to temporary stop slave irq while slave >> is handling the response, and re-enable them again when the response is ready. >> >> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> >> --- >> v3: >> + First introduce in v3 [Quan] >> >> drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c >> index b2e9c8f0ddf7..9926d04831a2 100644 >> --- a/drivers/i2c/busses/i2c-aspeed.c >> +++ b/drivers/i2c/busses/i2c-aspeed.c >> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus >> *bus, >> return 0; >> } >> >> +#if IS_ENABLED(CONFIG_I2C_SLAVE) >> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) { >> + struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap); >> + unsigned long current_mask, flags; >> + >> + spin_lock_irqsave(&bus->lock, flags); >> + >> + current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG); > Hello > Where the bus->base to be remap? > Hi Ryan, In "[PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver", the ->priv is retrieved by calling i2c_get_adapdata(client->adapter). And in aspeed_set_ssif_bmc_status(), call the exported aspeed_set_slave_busy() using ->priv pointer as code below. +extern void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy); +static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx *ssif_bmc, unsigned int status) +{ + if (status & SSIF_BMC_BUSY) + aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, true); + else if (status & SSIF_BMC_READY) + aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, false); +} + +static int ssif_bmc_probe(struct i2c_client *client, const struct i2c_device_id *id) +{ + struct ssif_bmc_ctx *ssif_bmc; + + ssif_bmc = ssif_bmc_alloc(client, 0); + if (IS_ERR(ssif_bmc)) + return PTR_ERR(ssif_bmc); + + ssif_bmc->priv = i2c_get_adapdata(client->adapter); + ssif_bmc->set_ssif_bmc_status = aspeed_set_ssif_bmc_status; + + return 0; +} - Quan
> -----Original Message----- > From: Quan Nguyen <quan@os.amperecomputing.com> > Sent: Monday, May 24, 2021 6:20 PM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard > <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley > <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins > <brendanhiggins@google.com>; Benjamin Herrenschmidt > <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel > <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net; > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org > Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q . > Nguyen <thang@os.amperecomputing.com>; Phong Vo > <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org > Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() > > On 24/05/2021 17:06, Ryan Chen wrote: > >> -----Original Message----- > >> From: openbmc > >> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On > Behalf > >> Of Quan Nguyen > >> Sent: Wednesday, May 19, 2021 3:50 PM > >> To: Corey Minyard <minyard@acm.org>; Rob Herring > >> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery > >> <andrew@aj.id.au>; Brendan Higgins <brendanhiggins@google.com>; > >> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Wolfram Sang > >> <wsa@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>; > >> openipmi-developer@lists.sourceforge.net; > >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; > >> linux-i2c@vger.kernel.org > >> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q . > >> Nguyen <thang@os.amperecomputing.com>; Phong Vo > >> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org > >> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() > >> > >> Slave i2c device on AST2500 received a lot of slave irq while it is > >> busy processing the response. To handle this case, adds and exports > >> aspeed_set_slave_busy() for controller to temporary stop slave irq > >> while slave is handling the response, and re-enable them again when the > response is ready. > >> > >> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> > >> --- > >> v3: > >> + First introduce in v3 [Quan] > >> > >> drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++ > >> 1 file changed, 20 insertions(+) > >> > >> diff --git a/drivers/i2c/busses/i2c-aspeed.c > >> b/drivers/i2c/busses/i2c-aspeed.c index b2e9c8f0ddf7..9926d04831a2 > >> 100644 > >> --- a/drivers/i2c/busses/i2c-aspeed.c > >> +++ b/drivers/i2c/busses/i2c-aspeed.c > >> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus > >> *bus, > >> return 0; > >> } > >> > >> +#if IS_ENABLED(CONFIG_I2C_SLAVE) > >> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) { > >> + struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap); > >> + unsigned long current_mask, flags; > >> + > >> + spin_lock_irqsave(&bus->lock, flags); > >> + > >> + current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG); > > Hello > > Where the bus->base to be remap? > > > > Hi Ryan, > > In "[PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver", the > ->priv is retrieved by calling i2c_get_adapdata(client->adapter). And in > aspeed_set_ssif_bmc_status(), call the exported aspeed_set_slave_busy() > using ->priv pointer as code below. > Yes, I see the probe function " ssif_bmc->priv = i2c_get_adapdata(client->adapter);" to get priv. But my question I don’t see the bus->base address be assigned. > +extern void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy); > +static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx *ssif_bmc, > unsigned int status) > +{ > + if (status & SSIF_BMC_BUSY) > + aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, true); > + else if (status & SSIF_BMC_READY) > + aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, false); } > + > +static int ssif_bmc_probe(struct i2c_client *client, const struct > i2c_device_id *id) > +{ > + struct ssif_bmc_ctx *ssif_bmc; > + > + ssif_bmc = ssif_bmc_alloc(client, 0); > + if (IS_ERR(ssif_bmc)) > + return PTR_ERR(ssif_bmc); > + > + ssif_bmc->priv = i2c_get_adapdata(client->adapter); > + ssif_bmc->set_ssif_bmc_status = aspeed_set_ssif_bmc_status; > + > + return 0; > +} > > - Quan > >
On 24/05/2021 17:36, Ryan Chen wrote: >> -----Original Message----- >> From: Quan Nguyen <quan@os.amperecomputing.com> >> Sent: Monday, May 24, 2021 6:20 PM >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard >> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley >> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins >> <brendanhiggins@google.com>; Benjamin Herrenschmidt >> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel >> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net; >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; >> linux-i2c@vger.kernel.org >> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q . >> Nguyen <thang@os.amperecomputing.com>; Phong Vo >> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org >> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() >> >> On 24/05/2021 17:06, Ryan Chen wrote: >>>> -----Original Message----- >>>> From: openbmc >>>> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On >> Behalf >>>> Of Quan Nguyen >>>> Sent: Wednesday, May 19, 2021 3:50 PM >>>> To: Corey Minyard <minyard@acm.org>; Rob Herring >>>> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery >>>> <andrew@aj.id.au>; Brendan Higgins <brendanhiggins@google.com>; >>>> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Wolfram Sang >>>> <wsa@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>; >>>> openipmi-developer@lists.sourceforge.net; >>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; >>>> linux-i2c@vger.kernel.org >>>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q . >>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo >>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org >>>> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() >>>> >>>> Slave i2c device on AST2500 received a lot of slave irq while it is >>>> busy processing the response. To handle this case, adds and exports >>>> aspeed_set_slave_busy() for controller to temporary stop slave irq >>>> while slave is handling the response, and re-enable them again when the >> response is ready. >>>> >>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> >>>> --- >>>> v3: >>>> + First introduce in v3 [Quan] >>>> >>>> drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++ >>>> 1 file changed, 20 insertions(+) >>>> >>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c >>>> b/drivers/i2c/busses/i2c-aspeed.c index b2e9c8f0ddf7..9926d04831a2 >>>> 100644 >>>> --- a/drivers/i2c/busses/i2c-aspeed.c >>>> +++ b/drivers/i2c/busses/i2c-aspeed.c >>>> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus >>>> *bus, >>>> return 0; >>>> } >>>> >>>> +#if IS_ENABLED(CONFIG_I2C_SLAVE) >>>> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) { >>>> + struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap); >>>> + unsigned long current_mask, flags; >>>> + >>>> + spin_lock_irqsave(&bus->lock, flags); >>>> + >>>> + current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG); >>> Hello >>> Where the bus->base to be remap? >>> >> >> Hi Ryan, >> >> In "[PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver", the >> ->priv is retrieved by calling i2c_get_adapdata(client->adapter). And in >> aspeed_set_ssif_bmc_status(), call the exported aspeed_set_slave_busy() >> using ->priv pointer as code below. >> > Yes, I see the probe function " ssif_bmc->priv = i2c_get_adapdata(client->adapter);" to get priv. > But my question I don’t see the bus->base address be assigned. > Hi Ryan, In drivers/i2c/busses/i2c-aspeed.c: struct aspeed_i2c_bus { struct i2c_adapter adap; struct device *dev; void __iomem *base; struct reset_control *rst; /* Synchronizes I/O mem access to base. */ spinlock_t lock; So when "struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);", the bus->base should point to the base of the aspeed_i2c_bus, which is already initialized by the aspeed i2c bus driver. Do I miss something? - Quan >> +extern void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy); >> +static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx *ssif_bmc, >> unsigned int status) >> +{ >> + if (status & SSIF_BMC_BUSY) >> + aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, true); >> + else if (status & SSIF_BMC_READY) >> + aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, false); } >> + >> +static int ssif_bmc_probe(struct i2c_client *client, const struct >> i2c_device_id *id) >> +{ >> + struct ssif_bmc_ctx *ssif_bmc; >> + >> + ssif_bmc = ssif_bmc_alloc(client, 0); >> + if (IS_ERR(ssif_bmc)) >> + return PTR_ERR(ssif_bmc); >> + >> + ssif_bmc->priv = i2c_get_adapdata(client->adapter); >> + ssif_bmc->set_ssif_bmc_status = aspeed_set_ssif_bmc_status; >> + >> + return 0; >> +} >> >> - Quan >> >> >
> -----Original Message----- > From: Quan Nguyen <quan@os.amperecomputing.com> > Sent: Monday, May 24, 2021 6:49 PM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard > <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley > <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins > <brendanhiggins@google.com>; Benjamin Herrenschmidt > <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel > <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net; > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org > Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q . > Nguyen <thang@os.amperecomputing.com>; Phong Vo > <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org > Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() > > On 24/05/2021 17:36, Ryan Chen wrote: > >> -----Original Message----- > >> From: Quan Nguyen <quan@os.amperecomputing.com> > >> Sent: Monday, May 24, 2021 6:20 PM > >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard > >> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley > >> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins > >> <brendanhiggins@google.com>; Benjamin Herrenschmidt > >> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp > >> Zabel <p.zabel@pengutronix.de>; > >> openipmi-developer@lists.sourceforge.net; > >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; > >> linux-i2c@vger.kernel.org > >> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q . > >> Nguyen <thang@os.amperecomputing.com>; Phong Vo > >> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org > >> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() > >> > >> On 24/05/2021 17:06, Ryan Chen wrote: > >>>> -----Original Message----- > >>>> From: openbmc > >>>> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On > >> Behalf > >>>> Of Quan Nguyen > >>>> Sent: Wednesday, May 19, 2021 3:50 PM > >>>> To: Corey Minyard <minyard@acm.org>; Rob Herring > >>>> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery > >>>> <andrew@aj.id.au>; Brendan Higgins <brendanhiggins@google.com>; > >>>> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Wolfram Sang > >>>> <wsa@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>; > >>>> openipmi-developer@lists.sourceforge.net; > >>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > >>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; > >>>> linux-i2c@vger.kernel.org > >>>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang > Q . > >>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo > >>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org > >>>> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() > >>>> > >>>> Slave i2c device on AST2500 received a lot of slave irq while it is > >>>> busy processing the response. To handle this case, adds and exports > >>>> aspeed_set_slave_busy() for controller to temporary stop slave irq > >>>> while slave is handling the response, and re-enable them again when > >>>> the > >> response is ready. > >>>> > >>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> > >>>> --- > >>>> v3: > >>>> + First introduce in v3 [Quan] > >>>> > >>>> drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++ > >>>> 1 file changed, 20 insertions(+) > >>>> > >>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c > >>>> b/drivers/i2c/busses/i2c-aspeed.c index b2e9c8f0ddf7..9926d04831a2 > >>>> 100644 > >>>> --- a/drivers/i2c/busses/i2c-aspeed.c > >>>> +++ b/drivers/i2c/busses/i2c-aspeed.c > >>>> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct > >>>> aspeed_i2c_bus *bus, > >>>> return 0; > >>>> } > >>>> > >>>> +#if IS_ENABLED(CONFIG_I2C_SLAVE) > >>>> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) { > >>>> + struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap); > >>>> + unsigned long current_mask, flags; > >>>> + > >>>> + spin_lock_irqsave(&bus->lock, flags); > >>>> + > >>>> + current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG); > >>> Hello > >>> Where the bus->base to be remap? > >>> > >> > >> Hi Ryan, > >> > >> In "[PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver", the > >> ->priv is retrieved by calling i2c_get_adapdata(client->adapter). And > >> ->in > >> aspeed_set_ssif_bmc_status(), call the exported > >> aspeed_set_slave_busy() using ->priv pointer as code below. > >> > > Yes, I see the probe function " ssif_bmc->priv = > i2c_get_adapdata(client->adapter);" to get priv. > > But my question I don’t see the bus->base address be assigned. > > > Hi Ryan, > > In drivers/i2c/busses/i2c-aspeed.c: > struct aspeed_i2c_bus { > struct i2c_adapter adap; > struct device *dev; > void __iomem *base; > struct reset_control *rst; > /* Synchronizes I/O mem access to base. */ > spinlock_t lock; > > So when "struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);", the > bus->base should point to the base of the aspeed_i2c_bus, which is > already initialized by the aspeed i2c bus driver. > > Do I miss something? Hello Quan, After study. I think the ssif_bmc_aspeed.c assume the priv data is the same struct. That is trick. Do we have a better for slave enable/disable call back to implement this? If add call back in struct i2c_algorithm; is it workable? > > >> +extern void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy); > >> +static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx *ssif_bmc, > >> unsigned int status) > >> +{ > >> + if (status & SSIF_BMC_BUSY) > >> + aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, true); > >> + else if (status & SSIF_BMC_READY) > >> + aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, false); } > >> + > >> +static int ssif_bmc_probe(struct i2c_client *client, const struct > >> i2c_device_id *id) > >> +{ > >> + struct ssif_bmc_ctx *ssif_bmc; > >> + > >> + ssif_bmc = ssif_bmc_alloc(client, 0); > >> + if (IS_ERR(ssif_bmc)) > >> + return PTR_ERR(ssif_bmc); > >> + > >> + ssif_bmc->priv = i2c_get_adapdata(client->adapter); > >> + ssif_bmc->set_ssif_bmc_status = aspeed_set_ssif_bmc_status; > >> + > >> + return 0; > >> +} > >> > >> - Quan > >> > >> > >
On 25/05/2021 17:30, Ryan Chen wrote: >> -----Original Message----- >> From: Quan Nguyen <quan@os.amperecomputing.com> >> Sent: Monday, May 24, 2021 6:49 PM >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard >> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley >> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins >> <brendanhiggins@google.com>; Benjamin Herrenschmidt >> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel >> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net; >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; >> linux-i2c@vger.kernel.org >> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q . >> Nguyen <thang@os.amperecomputing.com>; Phong Vo >> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org >> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() >> >> On 24/05/2021 17:36, Ryan Chen wrote: >>>> -----Original Message----- >>>> From: Quan Nguyen <quan@os.amperecomputing.com> >>>> Sent: Monday, May 24, 2021 6:20 PM >>>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard >>>> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley >>>> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins >>>> <brendanhiggins@google.com>; Benjamin Herrenschmidt >>>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp >>>> Zabel <p.zabel@pengutronix.de>; >>>> openipmi-developer@lists.sourceforge.net; >>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; >>>> linux-i2c@vger.kernel.org >>>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q . >>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo >>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org >>>> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() >>>> >>>> On 24/05/2021 17:06, Ryan Chen wrote: >>>>>> -----Original Message----- >>>>>> From: openbmc >>>>>> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On >>>> Behalf >>>>>> Of Quan Nguyen >>>>>> Sent: Wednesday, May 19, 2021 3:50 PM >>>>>> To: Corey Minyard <minyard@acm.org>; Rob Herring >>>>>> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery >>>>>> <andrew@aj.id.au>; Brendan Higgins <brendanhiggins@google.com>; >>>>>> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Wolfram Sang >>>>>> <wsa@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>; >>>>>> openipmi-developer@lists.sourceforge.net; >>>>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >>>>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; >>>>>> linux-i2c@vger.kernel.org >>>>>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang >> Q . >>>>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo >>>>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org >>>>>> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() >>>>>> >>>>>> Slave i2c device on AST2500 received a lot of slave irq while it is >>>>>> busy processing the response. To handle this case, adds and exports >>>>>> aspeed_set_slave_busy() for controller to temporary stop slave irq >>>>>> while slave is handling the response, and re-enable them again when >>>>>> the >>>> response is ready. >>>>>> >>>>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> >>>>>> --- >>>>>> v3: >>>>>> + First introduce in v3 [Quan] >>>>>> >>>>>> drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++ >>>>>> 1 file changed, 20 insertions(+) >>>>>> >>>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c >>>>>> b/drivers/i2c/busses/i2c-aspeed.c index b2e9c8f0ddf7..9926d04831a2 >>>>>> 100644 >>>>>> --- a/drivers/i2c/busses/i2c-aspeed.c >>>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c >>>>>> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct >>>>>> aspeed_i2c_bus *bus, >>>>>> return 0; >>>>>> } >>>>>> >>>>>> +#if IS_ENABLED(CONFIG_I2C_SLAVE) >>>>>> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) { >>>>>> + struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap); >>>>>> + unsigned long current_mask, flags; >>>>>> + >>>>>> + spin_lock_irqsave(&bus->lock, flags); >>>>>> + >>>>>> + current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG); >>>>> Hello >>>>> Where the bus->base to be remap? >>>>> >>>> >>>> Hi Ryan, >>>> >>>> In "[PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver", the >>>> ->priv is retrieved by calling i2c_get_adapdata(client->adapter). And >>>> ->in >>>> aspeed_set_ssif_bmc_status(), call the exported >>>> aspeed_set_slave_busy() using ->priv pointer as code below. >>>> >>> Yes, I see the probe function " ssif_bmc->priv = >> i2c_get_adapdata(client->adapter);" to get priv. >>> But my question I don’t see the bus->base address be assigned. >>> >> Hi Ryan, >> >> In drivers/i2c/busses/i2c-aspeed.c: >> struct aspeed_i2c_bus { >> struct i2c_adapter adap; >> struct device *dev; >> void __iomem *base; >> struct reset_control *rst; >> /* Synchronizes I/O mem access to base. */ >> spinlock_t lock; >> >> So when "struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);", the >> bus->base should point to the base of the aspeed_i2c_bus, which is >> already initialized by the aspeed i2c bus driver. >> >> Do I miss something? > Hello Quan, > After study. I think the ssif_bmc_aspeed.c assume the priv data is the same struct. > That is trick. > Do we have a better for slave enable/disable call back to implement this? > If add call back in struct i2c_algorithm; is it workable? >> Hi Ryan, I dont know which is better, ie: adding callback to struct i2c_algorithm or to struct i2c_adapter. I have tried to add generic callback to struct i2c_adapter as below and it works. diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 4e7714c88f95..6e9abf2d6abb 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -713,6 +713,10 @@ struct i2c_adapter { const struct i2c_adapter_quirks *quirks; struct irq_domain *host_notify_domain; +#if IS_ENABLED(CONFIG_I2C_SLAVE) + int (*slave_enable)(struct i2c_adapter *adap); + int (*slave_disable)(struct i2c_adapter *adap); +#endif }; #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev) >>>> +extern void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy); >>>> +static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx *ssif_bmc, >>>> unsigned int status) >>>> +{ >>>> + if (status & SSIF_BMC_BUSY) >>>> + aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, true); >>>> + else if (status & SSIF_BMC_READY) >>>> + aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, false); } >>>> + >>>> +static int ssif_bmc_probe(struct i2c_client *client, const struct >>>> i2c_device_id *id) >>>> +{ >>>> + struct ssif_bmc_ctx *ssif_bmc; >>>> + >>>> + ssif_bmc = ssif_bmc_alloc(client, 0); >>>> + if (IS_ERR(ssif_bmc)) >>>> + return PTR_ERR(ssif_bmc); >>>> + >>>> + ssif_bmc->priv = i2c_get_adapdata(client->adapter); >>>> + ssif_bmc->set_ssif_bmc_status = aspeed_set_ssif_bmc_status; >>>> + >>>> + return 0; >>>> +} >>>> >>>> - Quan >>>> >>>> >>> >
On 21/05/2021 13:09, Ryan Chen wrote: >> -----Original Message----- >> From: Quan Nguyen <quan@os.amperecomputing.com> >> Sent: Thursday, May 20, 2021 10:10 PM >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard >> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley >> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins >> <brendanhiggins@google.com>; Benjamin Herrenschmidt >> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel >> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net; >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; >> linux-i2c@vger.kernel.org >> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q . >> Nguyen <thang@os.amperecomputing.com>; Phong Vo >> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org >> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() >> >> On 20/05/2021 18:06, Ryan Chen wrote: >>>> -----Original Message----- >>>> From: openbmc >>>> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On >> Behalf >>>> Of Quan Nguyen >>>> Sent: Wednesday, May 19, 2021 3:50 PM >>>> To: Corey Minyard <minyard@acm.org>; Rob Herring >>>> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery >>>> <andrew@aj.id.au>; Brendan Higgins <brendanhiggins@google.com>; >>>> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Wolfram Sang >>>> <wsa@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>; >>>> openipmi-developer@lists.sourceforge.net; >>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; >>>> linux-i2c@vger.kernel.org >>>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q . >>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo >>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org >>>> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() >>>> >>>> Slave i2c device on AST2500 received a lot of slave irq while it is >>>> busy processing the response. To handle this case, adds and exports >>>> aspeed_set_slave_busy() for controller to temporary stop slave irq >>>> while slave is handling the response, and re-enable them again when the >> response is ready. >>>> >>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> >>>> --- >>>> v3: >>>> + First introduce in v3 [Quan] >>>> >>>> drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++ >>>> 1 file changed, 20 insertions(+) >>>> >>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c >>>> b/drivers/i2c/busses/i2c-aspeed.c index b2e9c8f0ddf7..9926d04831a2 >>>> 100644 >>>> --- a/drivers/i2c/busses/i2c-aspeed.c >>>> +++ b/drivers/i2c/busses/i2c-aspeed.c >>>> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus >>>> *bus, >>>> return 0; >>>> } >>>> >>>> +#if IS_ENABLED(CONFIG_I2C_SLAVE) >>>> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) { >>>> + struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap); >>>> + unsigned long current_mask, flags; >>>> + >>>> + spin_lock_irqsave(&bus->lock, flags); >>>> + >>>> + current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG); >>>> + if (busy) >>>> + current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE | >>>> ASPEED_I2CD_INTR_SLAVE_MATCH); >>>> + else >>>> + current_mask |= ASPEED_I2CD_INTR_RX_DONE | >>>> ASPEED_I2CD_INTR_SLAVE_MATCH; >>>> + writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG); >>>> + >>>> + spin_unlock_irqrestore(&bus->lock, flags); } >>>> +EXPORT_SYMBOL_GPL(aspeed_set_slave_busy); >>>> +#endif >>>> + >>>> static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus) { >>>> struct platform_device *pdev = to_platform_device(bus->dev); >>>> -- >>>> 2.28.0 >>> >>> Hello, >>> The better idea is use disable i2c slave mode. >>> Due to if i2c controller running in slave will get slave match, and latch the >> SCL. >>> Until cpu clear interrupt status. >>> Ryan >>> >> Thanks Ryan, >> >> Do you mean to enable/disable slave function as per example code below ? > Yes. it is. Dear Ryan, This solution looks good. I'll switch to use this way in next version. Thanks for the suggestion. - Quan >> >> /* Turn on slave mode. */ >> func_ctrl_reg_val = readl(bus->base + >> ASPEED_I2C_FUN_CTRL_REG); >> func_ctrl_reg_val |= ASPEED_I2CD_SLAVE_EN; >> writel(func_ctrl_reg_val, bus->base + >> ASPEED_I2C_FUN_CTRL_REG); >> >> Will try this idea. >> - Quan
> -----Original Message----- > From: Quan Nguyen <quan@os.amperecomputing.com> > Sent: Friday, May 28, 2021 8:53 AM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard > <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley > <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins > <brendanhiggins@google.com>; Benjamin Herrenschmidt > <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel > <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net; > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org > Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q . > Nguyen <thang@os.amperecomputing.com>; Phong Vo > <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org > Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() > > On 25/05/2021 17:30, Ryan Chen wrote: > >> -----Original Message----- > >> From: Quan Nguyen <quan@os.amperecomputing.com> > >> Sent: Monday, May 24, 2021 6:49 PM > >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard > >> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley > >> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins > >> <brendanhiggins@google.com>; Benjamin Herrenschmidt > >> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp > >> Zabel <p.zabel@pengutronix.de>; > >> openipmi-developer@lists.sourceforge.net; > >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; > >> linux-i2c@vger.kernel.org > >> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q . > >> Nguyen <thang@os.amperecomputing.com>; Phong Vo > >> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org > >> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() > >> > >> On 24/05/2021 17:36, Ryan Chen wrote: > >>>> -----Original Message----- > >>>> From: Quan Nguyen <quan@os.amperecomputing.com> > >>>> Sent: Monday, May 24, 2021 6:20 PM > >>>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard > >>>> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley > >>>> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins > >>>> <brendanhiggins@google.com>; Benjamin Herrenschmidt > >>>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp > >>>> Zabel <p.zabel@pengutronix.de>; > >>>> openipmi-developer@lists.sourceforge.net; > >>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > >>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; > >>>> linux-i2c@vger.kernel.org > >>>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang > Q . > >>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo > >>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org > >>>> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add > >>>> aspeed_set_slave_busy() > >>>> > >>>> On 24/05/2021 17:06, Ryan Chen wrote: > >>>>>> -----Original Message----- > >>>>>> From: openbmc > >>>>>> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On > >>>> Behalf > >>>>>> Of Quan Nguyen > >>>>>> Sent: Wednesday, May 19, 2021 3:50 PM > >>>>>> To: Corey Minyard <minyard@acm.org>; Rob Herring > >>>>>> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew > >>>>>> Jeffery <andrew@aj.id.au>; Brendan Higgins > >>>>>> <brendanhiggins@google.com>; Benjamin Herrenschmidt > >>>>>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; > >>>>>> Philipp Zabel <p.zabel@pengutronix.de>; > >>>>>> openipmi-developer@lists.sourceforge.net; > >>>>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > >>>>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; > >>>>>> linux-i2c@vger.kernel.org > >>>>>> Cc: Open Source Submission <patches@amperecomputing.com>; > Thang > >> Q . > >>>>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo > >>>>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org > >>>>>> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy() > >>>>>> > >>>>>> Slave i2c device on AST2500 received a lot of slave irq while it > >>>>>> is busy processing the response. To handle this case, adds and > >>>>>> exports > >>>>>> aspeed_set_slave_busy() for controller to temporary stop slave > >>>>>> irq while slave is handling the response, and re-enable them > >>>>>> again when the > >>>> response is ready. > >>>>>> > >>>>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> > >>>>>> --- > >>>>>> v3: > >>>>>> + First introduce in v3 [Quan] > >>>>>> > >>>>>> drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++ > >>>>>> 1 file changed, 20 insertions(+) > >>>>>> > >>>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c > >>>>>> b/drivers/i2c/busses/i2c-aspeed.c index > >>>>>> b2e9c8f0ddf7..9926d04831a2 > >>>>>> 100644 > >>>>>> --- a/drivers/i2c/busses/i2c-aspeed.c > >>>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c > >>>>>> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct > >>>>>> aspeed_i2c_bus *bus, > >>>>>> return 0; > >>>>>> } > >>>>>> > >>>>>> +#if IS_ENABLED(CONFIG_I2C_SLAVE) void > >>>>>> +aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) { > >>>>>> + struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap); > >>>>>> + unsigned long current_mask, flags; > >>>>>> + > >>>>>> + spin_lock_irqsave(&bus->lock, flags); > >>>>>> + > >>>>>> + current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG); > >>>>> Hello > >>>>> Where the bus->base to be remap? > >>>>> > >>>> > >>>> Hi Ryan, > >>>> > >>>> In "[PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver", the > >>>> ->priv is retrieved by calling i2c_get_adapdata(client->adapter). > >>>> ->And in > >>>> aspeed_set_ssif_bmc_status(), call the exported > >>>> aspeed_set_slave_busy() using ->priv pointer as code below. > >>>> > >>> Yes, I see the probe function " ssif_bmc->priv = > >> i2c_get_adapdata(client->adapter);" to get priv. > >>> But my question I don’t see the bus->base address be assigned. > >>> > >> Hi Ryan, > >> > >> In drivers/i2c/busses/i2c-aspeed.c: > >> struct aspeed_i2c_bus { > >> struct i2c_adapter adap; > >> struct device *dev; > >> void __iomem *base; > >> struct reset_control *rst; > >> /* Synchronizes I/O mem access to base. */ > >> spinlock_t lock; > >> > >> So when "struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);", the > >> bus->base should point to the base of the aspeed_i2c_bus, which is > >> already initialized by the aspeed i2c bus driver. > >> > >> Do I miss something? > > Hello Quan, > > After study. I think the ssif_bmc_aspeed.c assume the priv data is the > same struct. > > That is trick. > > Do we have a better for slave enable/disable call back to implement this? > > If add call back in struct i2c_algorithm; is it workable? > >> > > Hi Ryan, > > I dont know which is better, ie: adding callback to struct i2c_algorithm or to > struct i2c_adapter. > I have tried to add generic callback to struct i2c_adapter as below and it > works. > Hello Quan, Thanks your feedback. Because, if we apply it in callback. It can unify it in ssif_bmc.c to do callback. Not have ssif_bmc_aspeed.c, ssif_bmc_xxx.c ..... Because the priv struct is different in different i2c driver implement. Ryan > diff --git a/include/linux/i2c.h b/include/linux/i2c.h index > 4e7714c88f95..6e9abf2d6abb 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -713,6 +713,10 @@ struct i2c_adapter { > const struct i2c_adapter_quirks *quirks; > > struct irq_domain *host_notify_domain; > +#if IS_ENABLED(CONFIG_I2C_SLAVE) > + int (*slave_enable)(struct i2c_adapter *adap); > + int (*slave_disable)(struct i2c_adapter *adap); #endif > }; > #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev) > > >>>> +extern void aspeed_set_slave_busy(struct i2c_adapter *adap, bool > >>>> +busy); static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx > >>>> +*ssif_bmc, > >>>> unsigned int status) > >>>> +{ > >>>> + if (status & SSIF_BMC_BUSY) > >>>> + aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, > true); > >>>> + else if (status & SSIF_BMC_READY) > >>>> + aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, > >>>> +false); } > >>>> + > >>>> +static int ssif_bmc_probe(struct i2c_client *client, const struct > >>>> i2c_device_id *id) > >>>> +{ > >>>> + struct ssif_bmc_ctx *ssif_bmc; > >>>> + > >>>> + ssif_bmc = ssif_bmc_alloc(client, 0); > >>>> + if (IS_ERR(ssif_bmc)) > >>>> + return PTR_ERR(ssif_bmc); > >>>> + > >>>> + ssif_bmc->priv = i2c_get_adapdata(client->adapter); > >>>> + ssif_bmc->set_ssif_bmc_status = aspeed_set_ssif_bmc_status; > >>>> + > >>>> + return 0; > >>>> +} > >>>> > >>>> - Quan > >>>> > >>>> > >>> > >
On Wed, May 19, 2021 at 02:49:32PM +0700, Quan Nguyen wrote: > Slave i2c device on AST2500 received a lot of slave irq while it is > busy processing the response. To handle this case, adds and exports > aspeed_set_slave_busy() for controller to temporary stop slave irq > while slave is handling the response, and re-enable them again when > the response is ready. > > Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> > --- > v3: > + First introduce in v3 [Quan] > > drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > index b2e9c8f0ddf7..9926d04831a2 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus *bus, > return 0; > } > > +#if IS_ENABLED(CONFIG_I2C_SLAVE) > +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) > +{ > + struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap); > + unsigned long current_mask, flags; > + > + spin_lock_irqsave(&bus->lock, flags); This as far as I can see is still a recursive spinlock, and the spinlock debugger seems to agree with me. Graeme > + > + current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG); > + if (busy) > + current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_SLAVE_MATCH); > + else > + current_mask |= ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_SLAVE_MATCH; > + writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG); > + > + spin_unlock_irqrestore(&bus->lock, flags); > +} > +EXPORT_SYMBOL_GPL(aspeed_set_slave_busy); > +#endif > + > static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus) > { > struct platform_device *pdev = to_platform_device(bus->dev); > -- > 2.28.0 >
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index b2e9c8f0ddf7..9926d04831a2 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus *bus, return 0; } +#if IS_ENABLED(CONFIG_I2C_SLAVE) +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) +{ + struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap); + unsigned long current_mask, flags; + + spin_lock_irqsave(&bus->lock, flags); + + current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG); + if (busy) + current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_SLAVE_MATCH); + else + current_mask |= ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_SLAVE_MATCH; + writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG); + + spin_unlock_irqrestore(&bus->lock, flags); +} +EXPORT_SYMBOL_GPL(aspeed_set_slave_busy); +#endif + static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus) { struct platform_device *pdev = to_platform_device(bus->dev);
Slave i2c device on AST2500 received a lot of slave irq while it is busy processing the response. To handle this case, adds and exports aspeed_set_slave_busy() for controller to temporary stop slave irq while slave is handling the response, and re-enable them again when the response is ready. Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> --- v3: + First introduce in v3 [Quan] drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)