| Message ID | 20230509022734.148970-3-jiawenwu@trustnetic.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | TXGBE PHYLINK support | expand |
On Tuesday, May 9, 2023 9:52 PM, Piotr Raczynski wrote: > On Tue, May 09, 2023 at 10:27:27AM +0800, Jiawen Wu wrote: > > Wangxun 10Gb ethernet chip is connected to Designware I2C, to > > communicate with SFP. > > > > Introduce the property "snps,i2c-platform" to match device data for > > Wangxun in software node case. Since IO resource was mapped on the > > ethernet driver, add a model quirk to get regmap from parent device. > > > > The exists IP limitations are dealt as workarounds: > > - IP does not support interrupt mode, it works on polling mode. > > - Additionally set FIFO depth address the chip issue. > > > > Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com> > > I'm definitely not an i2c expert, a couple of nit picks below, thanks. > Reviewed-by: Piotr Raczynski <piotr.raczynski@intel.com> > > > --- > > drivers/i2c/busses/i2c-designware-common.c | 8 ++ > > drivers/i2c/busses/i2c-designware-core.h | 1 + > > drivers/i2c/busses/i2c-designware-master.c | 89 > > +++++++++++++++++++-- drivers/i2c/busses/i2c-designware-platdrv.c | > > 15 ++++ > > 4 files changed, 108 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-designware-common.c > > b/drivers/i2c/busses/i2c-designware-common.c > > index 0dc6b1ce663f..a7c2e67ccbf6 100644 > > --- a/drivers/i2c/busses/i2c-designware-common.c > > +++ b/drivers/i2c/busses/i2c-designware-common.c > > @@ -575,6 +575,14 @@ int i2c_dw_set_fifo_size(struct dw_i2c_dev *dev) > > unsigned int param; > > int ret; > > > > + /* DW_IC_COMP_PARAM_1 not implement for IP issue */ > > + if ((dev->flags & MODEL_MASK) == MODEL_WANGXUN_SP) { > > + dev->tx_fifo_depth = 4; > I understand this is some kind of workaround but is the number chosen > empirically? Maybe a defined value would be clearer instead of magic > number. Yes, this value setting worked and passed test. > > @@ -559,13 +621,19 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > > pm_runtime_get_sync(dev->dev); > > > > /* > > - * Initiate I2C message transfer when AMD NAVI GPU card is enabled, > > + * Initiate I2C message transfer when polling mode is enabled, > > * As it is polling based transfer mechanism, which does not support > > * interrupt based functionalities of existing DesignWare driver. > > */ > > - if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU) { > > + switch (dev->flags & MODEL_MASK) { > > + case MODEL_AMD_NAVI_GPU: > > ret = amd_i2c_dw_xfer_quirk(adap, msgs, num); > > goto done_nolock; > > + case MODEL_WANGXUN_SP: > > + ret = txgbe_i2c_dw_xfer_quirk(adap, msgs, num); > > + goto done_nolock; > > + default: > > + break; > > } > Nit pick, when I first saw above code it looked a little weird, > maybe it would be a little clearer with: > > if (i2c_dw_is_model_poll(dev)) { > switch (dev->flags & MODEL_MASK) { > case MODEL_AMD_NAVI_GPU: > ret = amd_i2c_dw_xfer_quirk(adap, msgs, num); > break; > case MODEL_WANGXUN_SP: > ret = txgbe_i2c_dw_xfer_quirk(adap, msgs, num); > break; > default: > break; > } > goto done_nolock; > } > > I do not insist though. Sure, it looks more obvious as polling mode quirk.
Wed, May 10, 2023 at 02:43:50PM +0800, Jiawen Wu kirjoitti: > On Tuesday, May 9, 2023 9:52 PM, Piotr Raczynski wrote: > > On Tue, May 09, 2023 at 10:27:27AM +0800, Jiawen Wu wrote: ... > > > /* > > > - * Initiate I2C message transfer when AMD NAVI GPU card is enabled, > > > + * Initiate I2C message transfer when polling mode is enabled, > > > * As it is polling based transfer mechanism, which does not support > > > * interrupt based functionalities of existing DesignWare driver. > > > */ > > > - if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU) { > > > + switch (dev->flags & MODEL_MASK) { > > > + case MODEL_AMD_NAVI_GPU: > > > ret = amd_i2c_dw_xfer_quirk(adap, msgs, num); > > > goto done_nolock; > > > + case MODEL_WANGXUN_SP: > > > + ret = txgbe_i2c_dw_xfer_quirk(adap, msgs, num); > > > + goto done_nolock; > > > + default: > > > + break; > > > } > > Nit pick, when I first saw above code it looked a little weird, > > maybe it would be a little clearer with: > > > > if (i2c_dw_is_model_poll(dev)) { > > switch (dev->flags & MODEL_MASK) { > > case MODEL_AMD_NAVI_GPU: > > ret = amd_i2c_dw_xfer_quirk(adap, msgs, num); > > break; > > case MODEL_WANGXUN_SP: > > ret = txgbe_i2c_dw_xfer_quirk(adap, msgs, num); > > break; > > default: > > break; > > } > > goto done_nolock; > > } > > > > I do not insist though. > > Sure, it looks more obvious as polling mode quirk. I don't think we need a double checks. The i2c_dw_is_model_poll() will repeat the switch. Please, leave it as is in your current version.
On Wednesday, May 10, 2023 3:48 PM, andy.shevchenko@gmail.com wrote: > Wed, May 10, 2023 at 02:43:50PM +0800, Jiawen Wu kirjoitti: > > On Tuesday, May 9, 2023 9:52 PM, Piotr Raczynski wrote: > > > On Tue, May 09, 2023 at 10:27:27AM +0800, Jiawen Wu wrote: > > ... > > > > > /* > > > > - * Initiate I2C message transfer when AMD NAVI GPU card is enabled, > > > > + * Initiate I2C message transfer when polling mode is enabled, > > > > * As it is polling based transfer mechanism, which does not support > > > > * interrupt based functionalities of existing DesignWare driver. > > > > */ > > > > - if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU) { > > > > + switch (dev->flags & MODEL_MASK) { > > > > + case MODEL_AMD_NAVI_GPU: > > > > ret = amd_i2c_dw_xfer_quirk(adap, msgs, num); > > > > goto done_nolock; > > > > + case MODEL_WANGXUN_SP: > > > > + ret = txgbe_i2c_dw_xfer_quirk(adap, msgs, num); > > > > + goto done_nolock; > > > > + default: > > > > + break; > > > > } > > > Nit pick, when I first saw above code it looked a little weird, > > > maybe it would be a little clearer with: > > > > > > if (i2c_dw_is_model_poll(dev)) { > > > switch (dev->flags & MODEL_MASK) { > > > case MODEL_AMD_NAVI_GPU: > > > ret = amd_i2c_dw_xfer_quirk(adap, msgs, num); > > > break; > > > case MODEL_WANGXUN_SP: > > > ret = txgbe_i2c_dw_xfer_quirk(adap, msgs, num); > > > break; > > > default: > > > break; > > > } > > > goto done_nolock; > > > } > > > > > > I do not insist though. > > > > Sure, it looks more obvious as polling mode quirk. > > I don't think we need a double checks. The i2c_dw_is_model_poll() will repeat > the switch. Please, leave it as is in your current version. Okay, thanks for all your comments.
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c index 0dc6b1ce663f..a7c2e67ccbf6 100644 --- a/drivers/i2c/busses/i2c-designware-common.c +++ b/drivers/i2c/busses/i2c-designware-common.c @@ -575,6 +575,14 @@ int i2c_dw_set_fifo_size(struct dw_i2c_dev *dev) unsigned int param; int ret; + /* DW_IC_COMP_PARAM_1 not implement for IP issue */ + if ((dev->flags & MODEL_MASK) == MODEL_WANGXUN_SP) { + dev->tx_fifo_depth = 4; + dev->rx_fifo_depth = 0; + + return 0; + } + /* * Try to detect the FIFO depth if not set by interface driver, * the depth could be from 2 to 256 from HW spec. diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index c5d87aae39c6..e2213b08d724 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -303,6 +303,7 @@ struct dw_i2c_dev { #define MODEL_MSCC_OCELOT BIT(8) #define MODEL_BAIKAL_BT1 BIT(9) #define MODEL_AMD_NAVI_GPU BIT(10) +#define MODEL_WANGXUN_SP BIT(11) #define MODEL_MASK GENMASK(11, 8) /* diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index 55ea91a63382..3bfd7a2232db 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -354,6 +354,68 @@ static int amd_i2c_dw_xfer_quirk(struct i2c_adapter *adap, struct i2c_msg *msgs, return 0; } +static int i2c_dw_poll_tx_empty(struct dw_i2c_dev *dev) +{ + u32 val; + + return regmap_read_poll_timeout(dev->map, DW_IC_RAW_INTR_STAT, val, + val & DW_IC_INTR_TX_EMPTY, + 100, 1000); +} + +static int i2c_dw_poll_rx_full(struct dw_i2c_dev *dev) +{ + u32 val; + + return regmap_read_poll_timeout(dev->map, DW_IC_RAW_INTR_STAT, val, + val & DW_IC_INTR_RX_FULL, + 100, 1000); +} + +static int txgbe_i2c_dw_xfer_quirk(struct i2c_adapter *adap, struct i2c_msg *msgs, + int num_msgs) +{ + struct dw_i2c_dev *dev = i2c_get_adapdata(adap); + int msg_idx, buf_len, data_idx, ret; + unsigned int val, stop = 0; + u8 *buf; + + dev->msgs = msgs; + dev->msgs_num = num_msgs; + i2c_dw_xfer_init(dev); + regmap_write(dev->map, DW_IC_INTR_MASK, 0); + + for (msg_idx = 0; msg_idx < num_msgs; msg_idx++) { + buf = msgs[msg_idx].buf; + buf_len = msgs[msg_idx].len; + + for (data_idx = 0; data_idx < buf_len; data_idx++) { + if (msg_idx == num_msgs - 1 && data_idx == buf_len - 1) + stop |= BIT(9); + + if (msgs[msg_idx].flags & I2C_M_RD) { + regmap_write(dev->map, DW_IC_DATA_CMD, 0x100 | stop); + + ret = i2c_dw_poll_rx_full(dev); + if (ret) + return ret; + + regmap_read(dev->map, DW_IC_DATA_CMD, &val); + buf[data_idx] = val; + } else { + ret = i2c_dw_poll_tx_empty(dev); + if (ret) + return ret; + + regmap_write(dev->map, DW_IC_DATA_CMD, + buf[data_idx] | stop); + } + } + } + + return num_msgs; +} + /* * Initiate (and continue) low level master read/write transaction. * This function is only called from i2c_dw_isr, and pumping i2c_msg @@ -559,13 +621,19 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) pm_runtime_get_sync(dev->dev); /* - * Initiate I2C message transfer when AMD NAVI GPU card is enabled, + * Initiate I2C message transfer when polling mode is enabled, * As it is polling based transfer mechanism, which does not support * interrupt based functionalities of existing DesignWare driver. */ - if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU) { + switch (dev->flags & MODEL_MASK) { + case MODEL_AMD_NAVI_GPU: ret = amd_i2c_dw_xfer_quirk(adap, msgs, num); goto done_nolock; + case MODEL_WANGXUN_SP: + ret = txgbe_i2c_dw_xfer_quirk(adap, msgs, num); + goto done_nolock; + default: + break; } reinit_completion(&dev->cmd_complete); @@ -848,7 +916,7 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev) return 0; } -static int amd_i2c_adap_quirk(struct dw_i2c_dev *dev) +static int i2c_dw_poll_adap_quirk(struct dw_i2c_dev *dev) { struct i2c_adapter *adap = &dev->adapter; int ret; @@ -862,6 +930,17 @@ static int amd_i2c_adap_quirk(struct dw_i2c_dev *dev) return ret; } +static bool i2c_dw_is_model_poll(struct dw_i2c_dev *dev) +{ + switch (dev->flags & MODEL_MASK) { + case MODEL_AMD_NAVI_GPU: + case MODEL_WANGXUN_SP: + return true; + default: + return false; + } +} + int i2c_dw_probe_master(struct dw_i2c_dev *dev) { struct i2c_adapter *adap = &dev->adapter; @@ -917,8 +996,8 @@ int i2c_dw_probe_master(struct dw_i2c_dev *dev) adap->dev.parent = dev->dev; i2c_set_adapdata(adap, dev); - if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU) - return amd_i2c_adap_quirk(dev); + if (i2c_dw_is_model_poll(dev)) + return i2c_dw_poll_adap_quirk(dev); if (dev->flags & ACCESS_NO_IRQ_SUSPEND) { irq_flags = IRQF_NO_SUSPEND; diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 89ad88c54754..1bf50150386d 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -168,6 +168,15 @@ static inline int dw_i2c_of_configure(struct platform_device *pdev) } #endif +static int txgbe_i2c_request_regs(struct dw_i2c_dev *dev) +{ + dev->map = dev_get_regmap(dev->dev->parent, NULL); + if (!dev->map) + return -ENODEV; + + return 0; +} + static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev) { pm_runtime_disable(dev->dev); @@ -185,6 +194,9 @@ static int dw_i2c_plat_request_regs(struct dw_i2c_dev *dev) case MODEL_BAIKAL_BT1: ret = bt1_i2c_request_regs(dev); break; + case MODEL_WANGXUN_SP: + ret = txgbe_i2c_request_regs(dev); + break; default: dev->base = devm_platform_ioremap_resource(pdev, 0); ret = PTR_ERR_OR_ZERO(dev->base); @@ -277,6 +289,9 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) return -ENOMEM; dev->flags = (uintptr_t)device_get_match_data(&pdev->dev); + if (device_property_present(&pdev->dev, "snps,i2c-platform")) + dev->flags |= MODEL_WANGXUN_SP; + dev->dev = &pdev->dev; dev->irq = irq; platform_set_drvdata(pdev, dev);
Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate with SFP. Introduce the property "snps,i2c-platform" to match device data for Wangxun in software node case. Since IO resource was mapped on the ethernet driver, add a model quirk to get regmap from parent device. The exists IP limitations are dealt as workarounds: - IP does not support interrupt mode, it works on polling mode. - Additionally set FIFO depth address the chip issue. Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com> --- drivers/i2c/busses/i2c-designware-common.c | 8 ++ drivers/i2c/busses/i2c-designware-core.h | 1 + drivers/i2c/busses/i2c-designware-master.c | 89 +++++++++++++++++++-- drivers/i2c/busses/i2c-designware-platdrv.c | 15 ++++ 4 files changed, 108 insertions(+), 5 deletions(-)