diff mbox series

[net-next,v7,2/9] i2c: designware: Add driver support for Wangxun 10Gb NIC

Message ID 20230509022734.148970-3-jiawenwu@trustnetic.com
State Superseded
Headers show
Series TXGBE PHYLINK support | expand

Commit Message

Jiawen Wu May 9, 2023, 2:27 a.m. UTC
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(-)

Comments

Jiawen Wu May 10, 2023, 6:43 a.m. UTC | #1
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.
Andy Shevchenko May 10, 2023, 7:47 a.m. UTC | #2
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.
Jiawen Wu May 10, 2023, 8 a.m. UTC | #3
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 mbox series

Patch

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);