| Message ID | 20210714033833.11640-2-quan@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add SSIF BMC driver | expand |
Hi all, On Wed, Jul 14, 2021 at 10:38:31AM +0700, Quan Nguyen wrote: > Slave needs time to prepare the response data before Master could > enquiry via read transaction. However, there is no mechanism for > i2c-aspeed Slave to notify Master that it needs more time to process > and this make Master side to time out when trying to get the response. > > This commit introduces the slave_enable() callback in struct > i2c_algorithm for Slave to temporary stop the Slave mode while working > on the response and re-enable the Slave when response data ready. Sorry that I couldn't chime in earlier, but NAK! > include/linux/i2c.h | 2 ++ @Corey: Please do not change this file without my ACK. It is not a trivial change but an API extenstion and that should really be acked by the subsystem maintainer, in this case me. I was really surprised to see this in linux-next already. @all: Plus, I neither like the API (because it doesn't look generic to me but mostly handling one issue needed here) nor do I fully understand the use case. Normally, when a read is requested and the backend needs time to deliver the data, the hardware should stretch the SCL clock until some data register is finally written to. If it doesn't do it for whatever reason, this is a quirky hardware in my book and needs handling in the driver only. So, what is special with this HW? Can't we solve it differently? All the best, Wolfram
On Thu, Aug 12, 2021 at 09:39:43AM +0200, Wolfram Sang wrote: > Hi all, > > On Wed, Jul 14, 2021 at 10:38:31AM +0700, Quan Nguyen wrote: > > Slave needs time to prepare the response data before Master could > > enquiry via read transaction. However, there is no mechanism for > > i2c-aspeed Slave to notify Master that it needs more time to process > > and this make Master side to time out when trying to get the response. > > > > This commit introduces the slave_enable() callback in struct > > i2c_algorithm for Slave to temporary stop the Slave mode while working > > on the response and re-enable the Slave when response data ready. > > Sorry that I couldn't chime in earlier, but NAK! > > > include/linux/i2c.h | 2 ++ > > @Corey: Please do not change this file without my ACK. It is not a > trivial change but an API extenstion and that should really be acked by > the subsystem maintainer, in this case me. I was really surprised to see > this in linux-next already. I am sorry, I'll pull it out. -corey > > @all: Plus, I neither like the API (because it doesn't look generic to > me but mostly handling one issue needed here) nor do I fully understand > the use case. Normally, when a read is requested and the backend needs > time to deliver the data, the hardware should stretch the SCL clock > until some data register is finally written to. If it doesn't do it for > whatever reason, this is a quirky hardware in my book and needs handling > in the driver only. So, what is special with this HW? Can't we solve it > differently? > > All the best, > > Wolfram > > _______________________________________________ > Openipmi-developer mailing list > Openipmi-developer@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Hi, I still wonder if we can't get the SSIF BMC driver upstream... > @all: Plus, I neither like the API (because it doesn't look generic to > me but mostly handling one issue needed here) nor do I fully understand > the use case. Normally, when a read is requested and the backend needs > time to deliver the data, the hardware should stretch the SCL clock > until some data register is finally written to. If it doesn't do it for > whatever reason, this is a quirky hardware in my book and needs handling > in the driver only. So, what is special with this HW? Can't we solve it > differently? ... for that, it would be great if somebody could answer my questions here :) Happy hacking, Wolfram
On 30/11/2021 02:22, Wolfram Sang wrote: > Hi, > > I still wonder if we can't get the SSIF BMC driver upstream... > Thanks Wolfram to help bring this up, This driver was tested with Aspeed ast2500 and we have tried many way to avoid using slave_enable() to toggle slave mode but there is no progress. Our expectation is still to have this driver upstream'ed and I'm thinking about testing this driver on other HW and re-post the driver. >> @all: Plus, I neither like the API (because it doesn't look generic to >> me but mostly handling one issue needed here) nor do I fully understand >> the use case. Normally, when a read is requested and the backend needs >> time to deliver the data, the hardware should stretch the SCL clock >> until some data register is finally written to. If it doesn't do it for >> whatever reason, this is a quirky hardware in my book and needs handling >> in the driver only. So, what is special with this HW? Can't we solve it >> differently? > > ... for that, it would be great if somebody could answer my questions > here :) > I have to admit that you are all right with the above comments. The fact is we still not be able to find any way to solve this differently. We don't own this HW and dont know what happen on this particular issue. The SCL clock stretching on this HW does not work as expected and the slave_enable() is the only solution for now. I hope if someone could help with the issue as well. Best regards, - Quan
Hi, > Thanks Wolfram to help bring this up, Sure thing! It would be sad to see this work bitrot. > This driver was tested with Aspeed ast2500 and we have tried many way to > avoid using slave_enable() to toggle slave mode but there is no progress. I see. I also can't help you there. I have neither experience with nor access to this HW. > Our expectation is still to have this driver upstream'ed and I'm thinking > about testing this driver on other HW and re-post the driver. That sounds like a good plan. > I have to admit that you are all right with the above comments. The fact is > we still not be able to find any way to solve this differently. We don't own > this HW and dont know what happen on this particular issue. The SCL clock > stretching on this HW does not work as expected and the slave_enable() is > the only solution for now. I hope if someone could help with the issue as > well. From this distance, it looks like HW access and a logic analyzer might be helpful in understanding the behaviour. Pity that you don't own the HW. Good luck nonetheless! Wolfram
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index 67e8b97c0c95..a6a19dc8a501 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -785,6 +785,25 @@ static int aspeed_i2c_unreg_slave(struct i2c_client *client) return 0; } + +static int aspeed_i2c_slave_enable(struct i2c_client *client, bool enable) +{ + struct aspeed_i2c_bus *bus = i2c_get_adapdata(client->adapter); + u32 func_ctrl_reg_val; + + if (!bus->slave) + return -EINVAL; + + /* Toggle slave mode. */ + func_ctrl_reg_val = readl(bus->base + ASPEED_I2C_FUN_CTRL_REG); + if (enable) + func_ctrl_reg_val |= ASPEED_I2CD_SLAVE_EN; + else + func_ctrl_reg_val &= ~ASPEED_I2CD_SLAVE_EN; + writel(func_ctrl_reg_val, bus->base + ASPEED_I2C_FUN_CTRL_REG); + + return 0; +} #endif /* CONFIG_I2C_SLAVE */ static const struct i2c_algorithm aspeed_i2c_algo = { @@ -793,6 +812,7 @@ static const struct i2c_algorithm aspeed_i2c_algo = { #if IS_ENABLED(CONFIG_I2C_SLAVE) .reg_slave = aspeed_i2c_reg_slave, .unreg_slave = aspeed_i2c_unreg_slave, + .slave_enable = aspeed_i2c_slave_enable, #endif /* CONFIG_I2C_SLAVE */ }; diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 3eb60a2e9e61..8c1765aa7e3f 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -520,6 +520,7 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info, * from the ``I2C_FUNC_*`` flags. * @reg_slave: Register given client to I2C slave mode of this adapter * @unreg_slave: Unregister given client from I2C slave mode of this adapter + * @slave_enable: Toggle enable slave mode for given client of this adapter * * The following structs are for those who like to implement new bus drivers: * i2c_algorithm is the interface to a class of hardware solutions which can @@ -557,6 +558,7 @@ struct i2c_algorithm { #if IS_ENABLED(CONFIG_I2C_SLAVE) int (*reg_slave)(struct i2c_client *client); int (*unreg_slave)(struct i2c_client *client); + int (*slave_enable)(struct i2c_client *client, bool enable); #endif };
Slave needs time to prepare the response data before Master could enquiry via read transaction. However, there is no mechanism for i2c-aspeed Slave to notify Master that it needs more time to process and this make Master side to time out when trying to get the response. This commit introduces the slave_enable() callback in struct i2c_algorithm for Slave to temporary stop the Slave mode while working on the response and re-enable the Slave when response data ready. Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com> --- v5: + None v4: + First introduced follow Ryan's suggestion [Ryan] + Fix recursive spinlock issue in v3 (aspeed_set_slave_busy()) and apply in this patch [Graeme] drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++ include/linux/i2c.h | 2 ++ 2 files changed, 22 insertions(+)