| Message ID | 20240823030242.3083528-4-jiawenwu@trustnetic.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Andi Shyti |
| Headers | show |
| Series | Add I2C bus lock for Wangxun | expand |
On Fri, Aug 23, 2024 at 11:02:42AM +0800, Jiawen Wu wrote: > Support acquire_lock() and release_lock() for Wangxun 10Gb NIC. Since the > firmware needs to access I2C all the time for some features, the semaphore > is used between software and firmware. The driver should set software > semaphore before accessing I2C bus and release it when it is finished. > Otherwise, there is probability that the correct information on I2C bus > will not be obtained. ... > i2c-designware-core-$(CONFIG_I2C_DESIGNWARE_SLAVE) += i2c-designware-slave.o > i2c-designware-platform-y := i2c-designware-platdrv.o > +i2c-designware-platform-y += i2c-designware-wx.o These lines have TABs/spaces mixture. Please fix at least your entry to avoid this from happening. ... > int i2c_dw_amdpsp_probe_lock_support(struct dw_i2c_dev *dev); > #endif ^^^ > +int i2c_dw_txgbe_probe_lock_support(struct dw_i2c_dev *dev); See below. ... > .probe = i2c_dw_amdpsp_probe_lock_support, > }, > #endif ^^^ > + { > + .probe = i2c_dw_txgbe_probe_lock_support, > + }, Do we all need this support? Even if the driver is not compiled? Why? ... > +#include <linux/platform_data/i2c-wx.h> > +#include <linux/platform_device.h> > +#include <linux/i2c.h> > +#include <linux/pci.h> This is a semi-random list. Please, take your time to understand the core you wrote. Follow IWYU principle. ... > +static int i2c_dw_txgbe_acquire_lock(struct dw_i2c_dev *dev) > +{ > + void __iomem *req_addr; > + u32 swsm; > + int i; > + > + req_addr = dev->ext + I2C_DW_TXGBE_MNG_SW; > + > + for (i = 0; i < I2C_DW_TXGBE_REQ_RETRY_CNT; i++) { Retry loops much better in a form of unsigned int retries = ...; ... do { ... } while (--retries); BUT... see below. > + writel(I2C_DW_TXGBE_MNG_SW_SM, req_addr); > + > + /* If we set the bit successfully then we got semaphore. */ > + swsm = readl(req_addr); > + if (swsm & I2C_DW_TXGBE_MNG_SW_SM) > + break; > + > + udelay(50); So, can a macro from iopoll.h be utilised here? Why not? > + } > + > + if (i == I2C_DW_TXGBE_REQ_RETRY_CNT) > + return -ETIMEDOUT; > + > + return 0; > +} > +int i2c_dw_txgbe_probe_lock_support(struct dw_i2c_dev *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev->dev); Why do you need this dance? I.o.w. how pdev is being used here? > + struct txgbe_i2c_platform_data *pdata; > + > + pdata = dev_get_platdata(&pdev->dev); > + if (!pdata) > + return -ENXIO; > + > + dev->ext = pdata->hw_addr; > + if (!dev->ext) > + return -ENXIO; > + > + dev->acquire_lock = i2c_dw_txgbe_acquire_lock; > + dev->release_lock = i2c_dw_txgbe_release_lock; > + > + return 0; > +}
On Fri, Aug 23, 2024 10:13 PM, Andy Shevchenko wrote: > On Fri, Aug 23, 2024 at 11:02:42AM +0800, Jiawen Wu wrote: > > Support acquire_lock() and release_lock() for Wangxun 10Gb NIC. Since the > > firmware needs to access I2C all the time for some features, the semaphore > > is used between software and firmware. The driver should set software > > semaphore before accessing I2C bus and release it when it is finished. > > Otherwise, there is probability that the correct information on I2C bus > > will not be obtained. > > ... > > > i2c-designware-core-$(CONFIG_I2C_DESIGNWARE_SLAVE) += i2c-designware-slave.o > > > i2c-designware-platform-y := i2c-designware-platdrv.o > > +i2c-designware-platform-y += i2c-designware-wx.o > > These lines have TABs/spaces mixture. Please fix at least your entry to avoid > this from happening. > > > ... > > > int i2c_dw_amdpsp_probe_lock_support(struct dw_i2c_dev *dev); > > #endif > > ^^^ > > > +int i2c_dw_txgbe_probe_lock_support(struct dw_i2c_dev *dev); > > See below. > > ... > > > .probe = i2c_dw_amdpsp_probe_lock_support, > > }, > > #endif > > ^^^ > > > + { > > + .probe = i2c_dw_txgbe_probe_lock_support, > > + }, > > Do we all need this support? Even if the driver is not compiled? Why? I'll add the macro CONFIG_I2C_DESIGNWARE_WX to control it. > ... > > > +#include <linux/platform_data/i2c-wx.h> > > +#include <linux/platform_device.h> > > +#include <linux/i2c.h> > > +#include <linux/pci.h> > > This is a semi-random list. Please, take your time to understand the core you > wrote. Follow IWYU principle. > > ... > > > +static int i2c_dw_txgbe_acquire_lock(struct dw_i2c_dev *dev) > > +{ > > + void __iomem *req_addr; > > + u32 swsm; > > + int i; > > + > > + req_addr = dev->ext + I2C_DW_TXGBE_MNG_SW; > > + > > + for (i = 0; i < I2C_DW_TXGBE_REQ_RETRY_CNT; i++) { > > Retry loops much better in a form of > > unsigned int retries = ...; > ... > do { > ... > } while (--retries); > > BUT... see below. > > > + writel(I2C_DW_TXGBE_MNG_SW_SM, req_addr); > > + > > + /* If we set the bit successfully then we got semaphore. */ > > + swsm = readl(req_addr); > > + if (swsm & I2C_DW_TXGBE_MNG_SW_SM) > > + break; > > + > > + udelay(50); > > So, can a macro from iopoll.h be utilised here? Why not? I need to write the register first and then read it in this loop. It does not seem to apply to the macros in iopoll.h. > > + } > > + > > + if (i == I2C_DW_TXGBE_REQ_RETRY_CNT) > > + return -ETIMEDOUT; > > + > > + return 0; > > +} > > > +int i2c_dw_txgbe_probe_lock_support(struct dw_i2c_dev *dev) > > +{ > > + struct platform_device *pdev = to_platform_device(dev->dev); > > Why do you need this dance? I.o.w. how pdev is being used here? I'll change to add the data in node property.
On Thu, Aug 29, 2024 at 05:15:42PM +0800, Jiawen Wu wrote: > On Fri, Aug 23, 2024 10:13 PM, Andy Shevchenko wrote: > > On Fri, Aug 23, 2024 at 11:02:42AM +0800, Jiawen Wu wrote: ... > > > +static int i2c_dw_txgbe_acquire_lock(struct dw_i2c_dev *dev) > > > +{ > > > + void __iomem *req_addr; > > > + u32 swsm; > > > + int i; > > > + > > > + req_addr = dev->ext + I2C_DW_TXGBE_MNG_SW; > > > + > > > + for (i = 0; i < I2C_DW_TXGBE_REQ_RETRY_CNT; i++) { > > > > Retry loops much better in a form of > > > > unsigned int retries = ...; > > ... > > do { > > ... > > } while (--retries); > > > > BUT... see below. > > > > > + writel(I2C_DW_TXGBE_MNG_SW_SM, req_addr); > > > + > > > + /* If we set the bit successfully then we got semaphore. */ > > > + swsm = readl(req_addr); > > > + if (swsm & I2C_DW_TXGBE_MNG_SW_SM) > > > + break; > > > + > > > + udelay(50); > > > > So, can a macro from iopoll.h be utilised here? Why not? > > I need to write the register first and then read it in this loop. > It does not seem to apply to the macros in iopoll.h. I don't see how does it prevent from using read_poll_timeout() macro. You need to implement a body of the loop as a helper function that you supply into macro as a parameter. > > > + } > > > + > > > + if (i == I2C_DW_TXGBE_REQ_RETRY_CNT) > > > + return -ETIMEDOUT; > > > + > > > + return 0; > > > +}
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 78d0561339e5..f0ad9ebaacaa 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -58,6 +58,7 @@ i2c-designware-core-y += i2c-designware-master.o i2c-designware-core-$(CONFIG_I2C_DESIGNWARE_SLAVE) += i2c-designware-slave.o obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM) += i2c-designware-platform.o i2c-designware-platform-y := i2c-designware-platdrv.o +i2c-designware-platform-y += i2c-designware-wx.o i2c-designware-platform-$(CONFIG_I2C_DESIGNWARE_AMDPSP) += i2c-designware-amdpsp.o i2c-designware-platform-$(CONFIG_I2C_DESIGNWARE_BAYTRAIL) += i2c-designware-baytrail.o obj-$(CONFIG_I2C_DESIGNWARE_PCI) += i2c-designware-pci.o diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 12b77f464fb5..eae2c4cdc851 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -414,6 +414,8 @@ int i2c_dw_baytrail_probe_lock_support(struct dw_i2c_dev *dev); int i2c_dw_amdpsp_probe_lock_support(struct dw_i2c_dev *dev); #endif +int i2c_dw_txgbe_probe_lock_support(struct dw_i2c_dev *dev); + int i2c_dw_validate_speed(struct dw_i2c_dev *dev); void i2c_dw_adjust_bus_speed(struct dw_i2c_dev *dev); diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index df3dc1e8093e..49c71ae5b6c1 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -229,6 +229,9 @@ static const struct i2c_dw_semaphore_callbacks i2c_dw_semaphore_cb_table[] = { .probe = i2c_dw_amdpsp_probe_lock_support, }, #endif + { + .probe = i2c_dw_txgbe_probe_lock_support, + }, {} }; diff --git a/drivers/i2c/busses/i2c-designware-wx.c b/drivers/i2c/busses/i2c-designware-wx.c new file mode 100644 index 000000000000..0f98160b956d --- /dev/null +++ b/drivers/i2c/busses/i2c-designware-wx.c @@ -0,0 +1,65 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2015 - 2024 Beijing WangXun Technology Co., Ltd. */ + +#include <linux/platform_data/i2c-wx.h> +#include <linux/platform_device.h> +#include <linux/i2c.h> +#include <linux/pci.h> + +#include "i2c-designware-core.h" + +#define I2C_DW_TXGBE_REQ_RETRY_CNT 4000 +#define I2C_DW_TXGBE_MNG_SW 0x1E004 +#define I2C_DW_TXGBE_MNG_SW_SM BIT(0) +#define I2C_DW_TXGBE_FLUSH 0x10000 + +static int i2c_dw_txgbe_acquire_lock(struct dw_i2c_dev *dev) +{ + void __iomem *req_addr; + u32 swsm; + int i; + + req_addr = dev->ext + I2C_DW_TXGBE_MNG_SW; + + for (i = 0; i < I2C_DW_TXGBE_REQ_RETRY_CNT; i++) { + writel(I2C_DW_TXGBE_MNG_SW_SM, req_addr); + + /* If we set the bit successfully then we got semaphore. */ + swsm = readl(req_addr); + if (swsm & I2C_DW_TXGBE_MNG_SW_SM) + break; + + udelay(50); + } + + if (i == I2C_DW_TXGBE_REQ_RETRY_CNT) + return -ETIMEDOUT; + + return 0; +} + +static void i2c_dw_txgbe_release_lock(struct dw_i2c_dev *dev) +{ + writel(0, dev->ext + I2C_DW_TXGBE_MNG_SW); + /* flush register status */ + readl(dev->ext + I2C_DW_TXGBE_FLUSH); +} + +int i2c_dw_txgbe_probe_lock_support(struct dw_i2c_dev *dev) +{ + struct platform_device *pdev = to_platform_device(dev->dev); + struct txgbe_i2c_platform_data *pdata; + + pdata = dev_get_platdata(&pdev->dev); + if (!pdata) + return -ENXIO; + + dev->ext = pdata->hw_addr; + if (!dev->ext) + return -ENXIO; + + dev->acquire_lock = i2c_dw_txgbe_acquire_lock; + dev->release_lock = i2c_dw_txgbe_release_lock; + + return 0; +}
Support acquire_lock() and release_lock() for Wangxun 10Gb NIC. Since the firmware needs to access I2C all the time for some features, the semaphore is used between software and firmware. The driver should set software semaphore before accessing I2C bus and release it when it is finished. Otherwise, there is probability that the correct information on I2C bus will not be obtained. Cc: stable@vger.kernel.org Fixes: 2f8d1ed79345 ("i2c: designware: Add driver support for Wangxun 10Gb NIC") Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com> --- drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-designware-core.h | 2 + drivers/i2c/busses/i2c-designware-platdrv.c | 3 + drivers/i2c/busses/i2c-designware-wx.c | 65 +++++++++++++++++++++ 4 files changed, 71 insertions(+) create mode 100644 drivers/i2c/busses/i2c-designware-wx.c