| Message ID | 20230515063200.301026-3-jiawenwu@trustnetic.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | TXGBE PHYLINK support | expand |
Mon, May 15, 2023 at 02:31:53PM +0800, Jiawen Wu kirjoitti: > 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. ... > dev->flags = (uintptr_t)device_get_match_data(&pdev->dev); > + if (device_property_present(&pdev->dev, "snps,i2c-platform")) > + dev->flags |= MODEL_WANGXUN_SP; What I meant here is to use device_property_present() _iff_ you have decided to go with the _vendor-specific_ property name. Otherwise it should be handled differently, i.e. with reading the actual value of that property. Hence it should correspond the model enum, which you need to declare in the Device Tree bindings before use. So, either if (device_property_present(&pdev->dev, "wx,...")) dev->flags |= MODEL_WANGXUN_SP; or if ((dev->flags & MODEL_MASK) == MODEL_NONE) { // you now have to distinguish that there is no model set in driver data u32 model; ret = device_property_read_u32(dev, "snps,i2c-platform"); if (ret) { ...handle error... } dev->flags |= model
On 5/15/23 12:24, andy.shevchenko@gmail.com wrote: > Mon, May 15, 2023 at 02:31:53PM +0800, Jiawen Wu kirjoitti: >> 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. > > ... > >> dev->flags = (uintptr_t)device_get_match_data(&pdev->dev); >> + if (device_property_present(&pdev->dev, "snps,i2c-platform")) >> + dev->flags |= MODEL_WANGXUN_SP; > > What I meant here is to use device_property_present() _iff_ you have decided to > go with the _vendor-specific_ property name. > > Otherwise it should be handled differently, i.e. with reading the actual value > of that property. Hence it should correspond the model enum, which you need to > declare in the Device Tree bindings before use. > > So, either > > if (device_property_present(&pdev->dev, "wx,...")) > dev->flags |= MODEL_WANGXUN_SP; > > or > > if ((dev->flags & MODEL_MASK) == MODEL_NONE) { > // you now have to distinguish that there is no model set in driver data > u32 model; > > ret = device_property_read_u32(dev, "snps,i2c-platform"); > if (ret) { > ...handle error... > } > dev->flags |= model > I'm not a device tree expert but I wonder would it be possible somehow combine this and compatible properties in dw_i2c_of_match[]? They set model flag for MODEL_MSCC_OCELOT and MODEL_BAIKAL_BT1. Then I'm thinking is "snps,i2c-platform" descriptive enough name for a model and does it confuse with "snps,designware-i2c" compatible property?
On Wednesday, May 17, 2023 4:49 PM, Jarkko Nikula wrote: > On 5/15/23 12:24, andy.shevchenko@gmail.com wrote: > > Mon, May 15, 2023 at 02:31:53PM +0800, Jiawen Wu kirjoitti: > >> 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. > > > > ... > > > >> dev->flags = (uintptr_t)device_get_match_data(&pdev->dev); > >> + if (device_property_present(&pdev->dev, "snps,i2c-platform")) > >> + dev->flags |= MODEL_WANGXUN_SP; > > > > What I meant here is to use device_property_present() _iff_ you have decided to > > go with the _vendor-specific_ property name. > > > > Otherwise it should be handled differently, i.e. with reading the actual value > > of that property. Hence it should correspond the model enum, which you need to > > declare in the Device Tree bindings before use. > > > > So, either > > > > if (device_property_present(&pdev->dev, "wx,...")) > > dev->flags |= MODEL_WANGXUN_SP; > > > > or > > > > if ((dev->flags & MODEL_MASK) == MODEL_NONE) { > > // you now have to distinguish that there is no model set in driver data > > u32 model; > > > > ret = device_property_read_u32(dev, "snps,i2c-platform"); > > if (ret) { > > ...handle error... > > } > > dev->flags |= model > > > I'm not a device tree expert but I wonder would it be possible somehow > combine this and compatible properties in dw_i2c_of_match[]? They set > model flag for MODEL_MSCC_OCELOT and MODEL_BAIKAL_BT1. Maybe the table could be changed to match device property, instead of relying on DT only. Or device_get_match_data() could be also implemented in software node case? > > Then I'm thinking is "snps,i2c-platform" descriptive enough name for a > model and does it confuse with "snps,designware-i2c" compatible property? I'd like to change the name back to "wx,i2c-snps-model" for the specific one.
On Wed, May 17, 2023 at 11:49 AM Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote: > On 5/15/23 12:24, andy.shevchenko@gmail.com wrote: > > Mon, May 15, 2023 at 02:31:53PM +0800, Jiawen Wu kirjoitti: > >> 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. > > > > ... > > > >> dev->flags = (uintptr_t)device_get_match_data(&pdev->dev); > >> + if (device_property_present(&pdev->dev, "snps,i2c-platform")) > >> + dev->flags |= MODEL_WANGXUN_SP; > > > > What I meant here is to use device_property_present() _iff_ you have decided to > > go with the _vendor-specific_ property name. > > > > Otherwise it should be handled differently, i.e. with reading the actual value > > of that property. Hence it should correspond the model enum, which you need to > > declare in the Device Tree bindings before use. > > > > So, either > > > > if (device_property_present(&pdev->dev, "wx,...")) > > dev->flags |= MODEL_WANGXUN_SP; > > > > or > > > > if ((dev->flags & MODEL_MASK) == MODEL_NONE) { > > // you now have to distinguish that there is no model set in driver data > > u32 model; > > > > ret = device_property_read_u32(dev, "snps,i2c-platform"); > > if (ret) { > > ...handle error... > > } > > dev->flags |= model > > > I'm not a device tree expert Me neither, that's why I replied earlier that this needs to be reviewed by DT people. > but I wonder would it be possible somehow > combine this and compatible properties in dw_i2c_of_match[]? They set > model flag for MODEL_MSCC_OCELOT and MODEL_BAIKAL_BT1. > > Then I'm thinking is "snps,i2c-platform" descriptive enough name for a > model and does it confuse with "snps,designware-i2c" compatible property?
On Wed, May 17, 2023 at 12:26 PM Jiawen Wu <jiawenwu@trustnetic.com> wrote: > On Wednesday, May 17, 2023 4:49 PM, Jarkko Nikula wrote: > > On 5/15/23 12:24, andy.shevchenko@gmail.com wrote: > > > Mon, May 15, 2023 at 02:31:53PM +0800, Jiawen Wu kirjoitti: ... > > >> dev->flags = (uintptr_t)device_get_match_data(&pdev->dev); > > >> + if (device_property_present(&pdev->dev, "snps,i2c-platform")) > > >> + dev->flags |= MODEL_WANGXUN_SP; > > > > > > What I meant here is to use device_property_present() _iff_ you have decided to > > > go with the _vendor-specific_ property name. > > > > > > Otherwise it should be handled differently, i.e. with reading the actual value > > > of that property. Hence it should correspond the model enum, which you need to > > > declare in the Device Tree bindings before use. > > > > > > So, either > > > > > > if (device_property_present(&pdev->dev, "wx,...")) > > > dev->flags |= MODEL_WANGXUN_SP; > > > > > > or > > > > > > if ((dev->flags & MODEL_MASK) == MODEL_NONE) { > > > // you now have to distinguish that there is no model set in driver data > > > u32 model; > > > > > > ret = device_property_read_u32(dev, "snps,i2c-platform"); > > > if (ret) { > > > ...handle error... > > > } > > > dev->flags |= model > > > > > I'm not a device tree expert but I wonder would it be possible somehow > > combine this and compatible properties in dw_i2c_of_match[]? They set > > model flag for MODEL_MSCC_OCELOT and MODEL_BAIKAL_BT1. > > Maybe the table could be changed to match device property, instead of relying > on DT only. Or device_get_match_data() could be also implemented in > software node case? This has been discussed [1] and still no visible prototype. Perhaps you can collaborate with Vladimir on the matter. [1]: https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/
On 5/17/23 12:44, Andy Shevchenko wrote: > On Wed, May 17, 2023 at 12:26 PM Jiawen Wu <jiawenwu@trustnetic.com> wrote: >> On Wednesday, May 17, 2023 4:49 PM, Jarkko Nikula wrote: >>> On 5/15/23 12:24, andy.shevchenko@gmail.com wrote: >>>> Mon, May 15, 2023 at 02:31:53PM +0800, Jiawen Wu kirjoitti: >>>>> dev->flags = (uintptr_t)device_get_match_data(&pdev->dev); >>>>> + if (device_property_present(&pdev->dev, "snps,i2c-platform")) >>>>> + dev->flags |= MODEL_WANGXUN_SP; >>>> >>>> What I meant here is to use device_property_present() _iff_ you have decided to >>>> go with the _vendor-specific_ property name. >>>> >>>> Otherwise it should be handled differently, i.e. with reading the actual value >>>> of that property. Hence it should correspond the model enum, which you need to >>>> declare in the Device Tree bindings before use. >>>> >>>> So, either >>>> >>>> if (device_property_present(&pdev->dev, "wx,...")) >>>> dev->flags |= MODEL_WANGXUN_SP; >>>> >>>> or >>>> >>>> if ((dev->flags & MODEL_MASK) == MODEL_NONE) { >>>> // you now have to distinguish that there is no model set in driver data >>>> u32 model; >>>> >>>> ret = device_property_read_u32(dev, "snps,i2c-platform"); >>>> if (ret) { >>>> ...handle error... >>>> } >>>> dev->flags |= model >>>> >>> I'm not a device tree expert but I wonder would it be possible somehow >>> combine this and compatible properties in dw_i2c_of_match[]? They set >>> model flag for MODEL_MSCC_OCELOT and MODEL_BAIKAL_BT1. >> >> Maybe the table could be changed to match device property, instead of relying >> on DT only. Or device_get_match_data() could be also implemented in >> software node case? > > This has been discussed [1] and still no visible prototype. Perhaps > you can collaborate with Vladimir on the matter. > > [1]: https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/ > Ok, not possible at the moment. Perhaps for now setting model using device_property_read_u32() is good enough? I asked above out of curiosity rather than demanding perfection. They say "Perfect is the enemy of good" :-)
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c index 0dc6b1ce663f..cdd8c67d9129 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 = TXGBE_TX_FIFO_DEPTH; + dev->rx_fifo_depth = TXGBE_RX_FIFO_DEPTH; + + 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..782532c20bd1 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) /* @@ -312,6 +313,9 @@ struct dw_i2c_dev { #define AMD_UCSI_INTR_REG 0x474 #define AMD_UCSI_INTR_EN 0xd +#define TXGBE_TX_FIFO_DEPTH 4 +#define TXGBE_RX_FIFO_DEPTH 0 + struct i2c_dw_semaphore_callbacks { int (*probe)(struct dw_i2c_dev *dev); void (*remove)(struct dw_i2c_dev *dev); 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);