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 <[email protected]>
---
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
> -----Original Message-----
> From: openbmc
> <[email protected]> On Behalf
> Of Quan Nguyen
> Sent: Wednesday, May 19, 2021 3:50 PM
> To: Corey Minyard <[email protected]>; Rob Herring <[email protected]>;
> Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Brendan
> Higgins <[email protected]>; Benjamin Herrenschmidt
> <[email protected]>; Wolfram Sang <[email protected]>; Philipp Zabel
> <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> Cc: Open Source Submission <[email protected]>; Thang Q .
> Nguyen <[email protected]>; Phong Vo
> <[email protected]>; [email protected]
> 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 <[email protected]>
> ---
> 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
>> <[email protected]> On Behalf
>> Of Quan Nguyen
>> Sent: Wednesday, May 19, 2021 3:50 PM
>> To: Corey Minyard <[email protected]>; Rob Herring <[email protected]>;
>> Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Brendan
>> Higgins <[email protected]>; Benjamin Herrenschmidt
>> <[email protected]>; Wolfram Sang <[email protected]>; Philipp Zabel
>> <[email protected]>; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]
>> Cc: Open Source Submission <[email protected]>; Thang Q .
>> Nguyen <[email protected]>; Phong Vo
>> <[email protected]>; [email protected]
>> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
> Sent: Thursday, May 20, 2021 10:10 PM
> To: Ryan Chen <[email protected]>; Corey Minyard
> <[email protected]>; Rob Herring <[email protected]>; Joel Stanley
> <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins
> <[email protected]>; Benjamin Herrenschmidt
> <[email protected]>; Wolfram Sang <[email protected]>; Philipp Zabel
> <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> Cc: Open Source Submission <[email protected]>; Thang Q .
> Nguyen <[email protected]>; Phong Vo
> <[email protected]>; [email protected]
> 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
> >> <[email protected]> On
> Behalf
> >> Of Quan Nguyen
> >> Sent: Wednesday, May 19, 2021 3:50 PM
> >> To: Corey Minyard <[email protected]>; Rob Herring
> >> <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery
> >> <[email protected]>; Brendan Higgins <[email protected]>;
> >> Benjamin Herrenschmidt <[email protected]>; Wolfram Sang
> >> <[email protected]>; Philipp Zabel <[email protected]>;
> >> [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]
> >> Cc: Open Source Submission <[email protected]>; Thang Q .
> >> Nguyen <[email protected]>; Phong Vo
> >> <[email protected]>; [email protected]
> >> 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 <[email protected]>
> >> ---
> >> 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
> <[email protected]> On Behalf
> Of Quan Nguyen
> Sent: Wednesday, May 19, 2021 3:50 PM
> To: Corey Minyard <[email protected]>; Rob Herring <[email protected]>;
> Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Brendan
> Higgins <[email protected]>; Benjamin Herrenschmidt
> <[email protected]>; Wolfram Sang <[email protected]>; Philipp Zabel
> <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> Cc: Open Source Submission <[email protected]>; Thang Q .
> Nguyen <[email protected]>; Phong Vo
> <[email protected]>; [email protected]
> 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 <[email protected]>
> ---
> 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
>> <[email protected]> On Behalf
>> Of Quan Nguyen
>> Sent: Wednesday, May 19, 2021 3:50 PM
>> To: Corey Minyard <[email protected]>; Rob Herring <[email protected]>;
>> Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Brendan
>> Higgins <[email protected]>; Benjamin Herrenschmidt
>> <[email protected]>; Wolfram Sang <[email protected]>; Philipp Zabel
>> <[email protected]>; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]
>> Cc: Open Source Submission <[email protected]>; Thang Q .
>> Nguyen <[email protected]>; Phong Vo
>> <[email protected]>; [email protected]
>> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
> Sent: Monday, May 24, 2021 6:20 PM
> To: Ryan Chen <[email protected]>; Corey Minyard
> <[email protected]>; Rob Herring <[email protected]>; Joel Stanley
> <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins
> <[email protected]>; Benjamin Herrenschmidt
> <[email protected]>; Wolfram Sang <[email protected]>; Philipp Zabel
> <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> Cc: Open Source Submission <[email protected]>; Thang Q .
> Nguyen <[email protected]>; Phong Vo
> <[email protected]>; [email protected]
> 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
> >> <[email protected]> On
> Behalf
> >> Of Quan Nguyen
> >> Sent: Wednesday, May 19, 2021 3:50 PM
> >> To: Corey Minyard <[email protected]>; Rob Herring
> >> <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery
> >> <[email protected]>; Brendan Higgins <[email protected]>;
> >> Benjamin Herrenschmidt <[email protected]>; Wolfram Sang
> >> <[email protected]>; Philipp Zabel <[email protected]>;
> >> [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]
> >> Cc: Open Source Submission <[email protected]>; Thang Q .
> >> Nguyen <[email protected]>; Phong Vo
> >> <[email protected]>; [email protected]
> >> 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 <[email protected]>
> >> ---
> >> 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 <[email protected]>
>> Sent: Monday, May 24, 2021 6:20 PM
>> To: Ryan Chen <[email protected]>; Corey Minyard
>> <[email protected]>; Rob Herring <[email protected]>; Joel Stanley
>> <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins
>> <[email protected]>; Benjamin Herrenschmidt
>> <[email protected]>; Wolfram Sang <[email protected]>; Philipp Zabel
>> <[email protected]>; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]
>> Cc: Open Source Submission <[email protected]>; Thang Q .
>> Nguyen <[email protected]>; Phong Vo
>> <[email protected]>; [email protected]
>> 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
>>>> <[email protected]> On
>> Behalf
>>>> Of Quan Nguyen
>>>> Sent: Wednesday, May 19, 2021 3:50 PM
>>>> To: Corey Minyard <[email protected]>; Rob Herring
>>>> <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery
>>>> <[email protected]>; Brendan Higgins <[email protected]>;
>>>> Benjamin Herrenschmidt <[email protected]>; Wolfram Sang
>>>> <[email protected]>; Philipp Zabel <[email protected]>;
>>>> [email protected];
>>>> [email protected]; [email protected];
>>>> [email protected]; [email protected];
>>>> [email protected]
>>>> Cc: Open Source Submission <[email protected]>; Thang Q .
>>>> Nguyen <[email protected]>; Phong Vo
>>>> <[email protected]>; [email protected]
>>>> 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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
> Sent: Monday, May 24, 2021 6:49 PM
> To: Ryan Chen <[email protected]>; Corey Minyard
> <[email protected]>; Rob Herring <[email protected]>; Joel Stanley
> <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins
> <[email protected]>; Benjamin Herrenschmidt
> <[email protected]>; Wolfram Sang <[email protected]>; Philipp Zabel
> <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> Cc: Open Source Submission <[email protected]>; Thang Q .
> Nguyen <[email protected]>; Phong Vo
> <[email protected]>; [email protected]
> 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 <[email protected]>
> >> Sent: Monday, May 24, 2021 6:20 PM
> >> To: Ryan Chen <[email protected]>; Corey Minyard
> >> <[email protected]>; Rob Herring <[email protected]>; Joel Stanley
> >> <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins
> >> <[email protected]>; Benjamin Herrenschmidt
> >> <[email protected]>; Wolfram Sang <[email protected]>; Philipp
> >> Zabel <[email protected]>;
> >> [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]
> >> Cc: Open Source Submission <[email protected]>; Thang Q .
> >> Nguyen <[email protected]>; Phong Vo
> >> <[email protected]>; [email protected]
> >> 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
> >>>> <[email protected]> On
> >> Behalf
> >>>> Of Quan Nguyen
> >>>> Sent: Wednesday, May 19, 2021 3:50 PM
> >>>> To: Corey Minyard <[email protected]>; Rob Herring
> >>>> <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery
> >>>> <[email protected]>; Brendan Higgins <[email protected]>;
> >>>> Benjamin Herrenschmidt <[email protected]>; Wolfram Sang
> >>>> <[email protected]>; Philipp Zabel <[email protected]>;
> >>>> [email protected];
> >>>> [email protected]; [email protected];
> >>>> [email protected]; [email protected];
> >>>> [email protected]
> >>>> Cc: Open Source Submission <[email protected]>; Thang
> Q .
> >>>> Nguyen <[email protected]>; Phong Vo
> >>>> <[email protected]>; [email protected]
> >>>> 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 <[email protected]>
> >>>> ---
> >>>> 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 <[email protected]>
>> Sent: Monday, May 24, 2021 6:49 PM
>> To: Ryan Chen <[email protected]>; Corey Minyard
>> <[email protected]>; Rob Herring <[email protected]>; Joel Stanley
>> <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins
>> <[email protected]>; Benjamin Herrenschmidt
>> <[email protected]>; Wolfram Sang <[email protected]>; Philipp Zabel
>> <[email protected]>; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]
>> Cc: Open Source Submission <[email protected]>; Thang Q .
>> Nguyen <[email protected]>; Phong Vo
>> <[email protected]>; [email protected]
>> 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 <[email protected]>
>>>> Sent: Monday, May 24, 2021 6:20 PM
>>>> To: Ryan Chen <[email protected]>; Corey Minyard
>>>> <[email protected]>; Rob Herring <[email protected]>; Joel Stanley
>>>> <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins
>>>> <[email protected]>; Benjamin Herrenschmidt
>>>> <[email protected]>; Wolfram Sang <[email protected]>; Philipp
>>>> Zabel <[email protected]>;
>>>> [email protected];
>>>> [email protected]; [email protected];
>>>> [email protected]; [email protected];
>>>> [email protected]
>>>> Cc: Open Source Submission <[email protected]>; Thang Q .
>>>> Nguyen <[email protected]>; Phong Vo
>>>> <[email protected]>; [email protected]
>>>> 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
>>>>>> <[email protected]> On
>>>> Behalf
>>>>>> Of Quan Nguyen
>>>>>> Sent: Wednesday, May 19, 2021 3:50 PM
>>>>>> To: Corey Minyard <[email protected]>; Rob Herring
>>>>>> <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery
>>>>>> <[email protected]>; Brendan Higgins <[email protected]>;
>>>>>> Benjamin Herrenschmidt <[email protected]>; Wolfram Sang
>>>>>> <[email protected]>; Philipp Zabel <[email protected]>;
>>>>>> [email protected];
>>>>>> [email protected]; [email protected];
>>>>>> [email protected]; [email protected];
>>>>>> [email protected]
>>>>>> Cc: Open Source Submission <[email protected]>; Thang
>> Q .
>>>>>> Nguyen <[email protected]>; Phong Vo
>>>>>> <[email protected]>; [email protected]
>>>>>> 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 <[email protected]>
>>>>>> ---
>>>>>> 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 <[email protected]>
>> Sent: Thursday, May 20, 2021 10:10 PM
>> To: Ryan Chen <[email protected]>; Corey Minyard
>> <[email protected]>; Rob Herring <[email protected]>; Joel Stanley
>> <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins
>> <[email protected]>; Benjamin Herrenschmidt
>> <[email protected]>; Wolfram Sang <[email protected]>; Philipp Zabel
>> <[email protected]>; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]
>> Cc: Open Source Submission <[email protected]>; Thang Q .
>> Nguyen <[email protected]>; Phong Vo
>> <[email protected]>; [email protected]
>> 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
>>>> <[email protected]> On
>> Behalf
>>>> Of Quan Nguyen
>>>> Sent: Wednesday, May 19, 2021 3:50 PM
>>>> To: Corey Minyard <[email protected]>; Rob Herring
>>>> <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery
>>>> <[email protected]>; Brendan Higgins <[email protected]>;
>>>> Benjamin Herrenschmidt <[email protected]>; Wolfram Sang
>>>> <[email protected]>; Philipp Zabel <[email protected]>;
>>>> [email protected];
>>>> [email protected]; [email protected];
>>>> [email protected]; [email protected];
>>>> [email protected]
>>>> Cc: Open Source Submission <[email protected]>; Thang Q .
>>>> Nguyen <[email protected]>; Phong Vo
>>>> <[email protected]>; [email protected]
>>>> 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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
> Sent: Friday, May 28, 2021 8:53 AM
> To: Ryan Chen <[email protected]>; Corey Minyard
> <[email protected]>; Rob Herring <[email protected]>; Joel Stanley
> <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins
> <[email protected]>; Benjamin Herrenschmidt
> <[email protected]>; Wolfram Sang <[email protected]>; Philipp Zabel
> <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> Cc: Open Source Submission <[email protected]>; Thang Q .
> Nguyen <[email protected]>; Phong Vo
> <[email protected]>; [email protected]
> 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 <[email protected]>
> >> Sent: Monday, May 24, 2021 6:49 PM
> >> To: Ryan Chen <[email protected]>; Corey Minyard
> >> <[email protected]>; Rob Herring <[email protected]>; Joel Stanley
> >> <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins
> >> <[email protected]>; Benjamin Herrenschmidt
> >> <[email protected]>; Wolfram Sang <[email protected]>; Philipp
> >> Zabel <[email protected]>;
> >> [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]
> >> Cc: Open Source Submission <[email protected]>; Thang Q .
> >> Nguyen <[email protected]>; Phong Vo
> >> <[email protected]>; [email protected]
> >> 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 <[email protected]>
> >>>> Sent: Monday, May 24, 2021 6:20 PM
> >>>> To: Ryan Chen <[email protected]>; Corey Minyard
> >>>> <[email protected]>; Rob Herring <[email protected]>; Joel Stanley
> >>>> <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins
> >>>> <[email protected]>; Benjamin Herrenschmidt
> >>>> <[email protected]>; Wolfram Sang <[email protected]>; Philipp
> >>>> Zabel <[email protected]>;
> >>>> [email protected];
> >>>> [email protected]; [email protected];
> >>>> [email protected]; [email protected];
> >>>> [email protected]
> >>>> Cc: Open Source Submission <[email protected]>; Thang
> Q .
> >>>> Nguyen <[email protected]>; Phong Vo
> >>>> <[email protected]>; [email protected]
> >>>> 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
> >>>>>> <[email protected]> On
> >>>> Behalf
> >>>>>> Of Quan Nguyen
> >>>>>> Sent: Wednesday, May 19, 2021 3:50 PM
> >>>>>> To: Corey Minyard <[email protected]>; Rob Herring
> >>>>>> <[email protected]>; Joel Stanley <[email protected]>; Andrew
> >>>>>> Jeffery <[email protected]>; Brendan Higgins
> >>>>>> <[email protected]>; Benjamin Herrenschmidt
> >>>>>> <[email protected]>; Wolfram Sang <[email protected]>;
> >>>>>> Philipp Zabel <[email protected]>;
> >>>>>> [email protected];
> >>>>>> [email protected]; [email protected];
> >>>>>> [email protected]; [email protected];
> >>>>>> [email protected]
> >>>>>> Cc: Open Source Submission <[email protected]>;
> Thang
> >> Q .
> >>>>>> Nguyen <[email protected]>; Phong Vo
> >>>>>> <[email protected]>; [email protected]
> >>>>>> 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 <[email protected]>
> >>>>>> ---
> >>>>>> 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 <[email protected]>
> ---
> 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
>