| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZFpQF4hi0FciwQsj@nimitz>
Date: Tue, 9 May 2023 15:52:23 +0200
From: Piotr Raczynski <piotr.raczynski@...el.com>
To: Jiawen Wu <jiawenwu@...stnetic.com>
CC: <netdev@...r.kernel.org>, <jarkko.nikula@...ux.intel.com>,
<andriy.shevchenko@...ux.intel.com>, <mika.westerberg@...ux.intel.com>,
<jsd@...ihalf.com>, <Jose.Abreu@...opsys.com>, <andrew@...n.ch>,
<hkallweit1@...il.com>, <linux@...linux.org.uk>, <linux-i2c@...r.kernel.org>,
<linux-gpio@...r.kernel.org>, <mengyuanlou@...-swift.com>
Subject: Re: [PATCH net-next v7 2/9] i2c: designware: Add driver support for
Wangxun 10Gb NIC
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@...stnetic.com>
I'm definitely not an i2c expert, a couple of nit picks below, thanks.
Reviewed-by: Piotr Raczynski <piotr.raczynski@...el.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.
> + 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;
> }
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.
>
> 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);
> --
> 2.27.0
>
>
Powered by blists - more mailing lists