diff mbox series

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

Message ID 20230419082739.295180-3-jiawenwu@trustnetic.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series TXGBE PHYLINK support | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 18 this patch: 18
netdev/cc_maintainers warning 3 maintainers not CCed: jsd@semihalf.com mika.westerberg@linux.intel.com andriy.shevchenko@linux.intel.com
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 18 this patch: 18
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? WARNING: line length of 82 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jiawen Wu April 19, 2023, 8:27 a.m. UTC
Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate
with SFP.

Add platform data to pass IOMEM base address, board flag and other
parameters, since resource address was mapped on ethernet driver.

The exists IP limitations are dealt as workarounds:
- IP does not support interrupt mode, it works on polling mode.
- I2C cannot read continuously, only one byte can at a time.
- Additionally set FIFO depth address the chip issue.

Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/i2c/busses/i2c-designware-common.c  |  4 +
 drivers/i2c/busses/i2c-designware-core.h    |  1 +
 drivers/i2c/busses/i2c-designware-master.c  | 91 ++++++++++++++++++++-
 drivers/i2c/busses/i2c-designware-platdrv.c | 36 +++++++-
 include/linux/platform_data/i2c-dw.h        | 15 ++++
 5 files changed, 143 insertions(+), 4 deletions(-)
 create mode 100644 include/linux/platform_data/i2c-dw.h

Comments

Jarkko Nikula April 19, 2023, 2:36 p.m. UTC | #1
Hi

On 4/19/23 11:27, Jiawen Wu wrote:
> Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate
> with SFP.
> 
> Add platform data to pass IOMEM base address, board flag and other
> parameters, since resource address was mapped on ethernet driver.
> 
> The exists IP limitations are dealt as workarounds:
> - IP does not support interrupt mode, it works on polling mode.
> - I2C cannot read continuously, only one byte can at a time.
> - Additionally set FIFO depth address the chip issue.
> 
> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
>   drivers/i2c/busses/i2c-designware-common.c  |  4 +
>   drivers/i2c/busses/i2c-designware-core.h    |  1 +
>   drivers/i2c/busses/i2c-designware-master.c  | 91 ++++++++++++++++++++-
>   drivers/i2c/busses/i2c-designware-platdrv.c | 36 +++++++-
>   include/linux/platform_data/i2c-dw.h        | 15 ++++
>   5 files changed, 143 insertions(+), 4 deletions(-)
>   create mode 100644 include/linux/platform_data/i2c-dw.h
> 
> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index 0dc6b1ce663f..e4faab4655cb 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -588,6 +588,10 @@ int i2c_dw_set_fifo_size(struct dw_i2c_dev *dev)
>   	if (ret)
>   		return ret;
>   
> +	/* workaround for IP hardware issue */
> +	if ((dev->flags & MODEL_MASK) == MODEL_WANGXUN_SP)
> +		param |= 0x80800;
> +
What is the issue here? Is register DW_IC_COMP_PARAM_1 implemented? If 
not then I think safer is not to even read it lines before this added test.

> +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;
> +	u8 dev_addr;
> +	u8 *buf;
> +
> +	dev->msgs = msgs;
> +	dev->msgs_num = num_msgs;
> +	i2c_dw_xfer_init(dev);
> +	regmap_write(dev->map, DW_IC_INTR_MASK, 0);
> +
> +	dev_addr = msgs[0].buf[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 (msgs[msg_idx].flags & I2C_M_RD) {
> +				ret = i2c_dw_poll_tx_empty(dev);
> +				if (ret)
> +					return ret;
> +
> +				regmap_write(dev->map, DW_IC_DATA_CMD,
> +					     (dev_addr + data_idx) | BIT(9));
> +				regmap_write(dev->map, DW_IC_DATA_CMD, 0x100 | BIT(9));
> +

Am I wrong but this looks tailored to the use-case rather than generic 
implementation? I don't understand what is this write command with data 
(dev_addr + data_idx) + STOP followed by read command with STOP.

DW_IC_DATA_CMD upper bits have following meaning:

BIT(8) == 0x100: 1 = read, 0 = write
BIT(9): Stop issued after this byte
BIT(10): RESTART is issued before the byte is sent or received

> +			} else {
> +				ret = i2c_dw_poll_tx_empty(dev);
> +				if (ret)
> +					return ret;
> +
> +				regmap_write(dev->map, DW_IC_DATA_CMD, buf[data_idx]);
> +				if (data_idx == (buf_len - 1))
> +					regmap_write(dev->map, DW_IC_DATA_CMD, BIT(9));

I think these separate writes must be combined if I'm not wrong? I 
believe this cause needless extra zero byte + STOP transferred on the 
bus instead of last buffer byte + STOP?

> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c

> +static void dw_i2c_get_plat_data(struct dw_i2c_dev *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev->dev);
> +	struct dw_i2c_platform_data *pdata;
> +
> +	pdata = dev_get_platdata(&pdev->dev);
> +	if (!pdata)
> +		return;
> +
> +	dev->flags |= pdata->flags;
> +	dev->base = pdata->base;
> +
> +	if (pdata->ss_hcnt && pdata->ss_lcnt) {
> +		dev->ss_hcnt = pdata->ss_hcnt;
> +		dev->ss_lcnt = pdata->ss_lcnt;
> +	} else {
> +		dev->ss_hcnt = 6;
> +		dev->ss_lcnt = 8;
> +	}
> +
> +	if (pdata->fs_hcnt && pdata->fs_lcnt) {
> +		dev->fs_hcnt = pdata->fs_hcnt;
> +		dev->fs_lcnt = pdata->fs_lcnt;
> +	} else {
> +		dev->fs_hcnt = 6;
> +		dev->fs_lcnt = 8;
> +	}
> +}
> +
>   static const struct dmi_system_id dw_i2c_hwmon_class_dmi[] = {
>   	{
>   		.ident = "Qtechnology QT5222",
> @@ -282,6 +314,8 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>   	dev->irq = irq;
>   	platform_set_drvdata(pdev, dev);
>   
> +	dw_i2c_get_plat_data(dev);
> +
Instead of this added code would it be possible to use generic timing 
parameters which can come either from firmware or code? Those are 
handled already here by the call to i2c_parse_fw_timings().

Then drivers/i2c/busses/i2c-designware-master.c: 
i2c_dw_set_timings_master() takes care of calculating Designware 
specific hcnt/lcnt timing parameters from those generic values.
Andrew Lunn April 19, 2023, 8:58 p.m. UTC | #2
On Wed, Apr 19, 2023 at 04:27:33PM +0800, Jiawen Wu wrote:
> Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate
> with SFP.
> 
> Add platform data to pass IOMEM base address, board flag and other
> parameters, since resource address was mapped on ethernet driver.
> 
> The exists IP limitations are dealt as workarounds:
> - IP does not support interrupt mode, it works on polling mode.
> - I2C cannot read continuously, only one byte can at a time.

Are you really sure about that?

It is a major limitation for SFP devices. It means you cannot access
the diagnostics, since you need to perform an atomic 2 byte read.

Or maybe i'm understanding you wrong.

   Andrew
Jiawen Wu April 20, 2023, 10:29 a.m. UTC | #3
On Thursday, April 20, 2023 4:58 AM, Andrew Lunn wrote:
> On Wed, Apr 19, 2023 at 04:27:33PM +0800, Jiawen Wu wrote:
> > Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate
> > with SFP.
> >
> > Add platform data to pass IOMEM base address, board flag and other
> > parameters, since resource address was mapped on ethernet driver.
> >
> > The exists IP limitations are dealt as workarounds:
> > - IP does not support interrupt mode, it works on polling mode.
> > - I2C cannot read continuously, only one byte can at a time.
> 
> Are you really sure about that?
> 
> It is a major limitation for SFP devices. It means you cannot access
> the diagnostics, since you need to perform an atomic 2 byte read.
> 
> Or maybe i'm understanding you wrong.
> 
>    Andrew
> 

Maybe I'm a little confused about this. Every time I read a byte info, I have to
write a 'read command'. It can normally get the information for SFP devices.
But I'm not sure if this is regular I2C behavior.
Jiawen Wu April 20, 2023, 10:49 a.m. UTC | #4
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> 
> > +static void dw_i2c_get_plat_data(struct dw_i2c_dev *dev)
> > +{
> > +	struct platform_device *pdev = to_platform_device(dev->dev);
> > +	struct dw_i2c_platform_data *pdata;
> > +
> > +	pdata = dev_get_platdata(&pdev->dev);
> > +	if (!pdata)
> > +		return;
> > +
> > +	dev->flags |= pdata->flags;
> > +	dev->base = pdata->base;
> > +
> > +	if (pdata->ss_hcnt && pdata->ss_lcnt) {
> > +		dev->ss_hcnt = pdata->ss_hcnt;
> > +		dev->ss_lcnt = pdata->ss_lcnt;
> > +	} else {
> > +		dev->ss_hcnt = 6;
> > +		dev->ss_lcnt = 8;
> > +	}
> > +
> > +	if (pdata->fs_hcnt && pdata->fs_lcnt) {
> > +		dev->fs_hcnt = pdata->fs_hcnt;
> > +		dev->fs_lcnt = pdata->fs_lcnt;
> > +	} else {
> > +		dev->fs_hcnt = 6;
> > +		dev->fs_lcnt = 8;
> > +	}
> > +}
> > +
> >   static const struct dmi_system_id dw_i2c_hwmon_class_dmi[] = {
> >   	{
> >   		.ident = "Qtechnology QT5222",
> > @@ -282,6 +314,8 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> >   	dev->irq = irq;
> >   	platform_set_drvdata(pdev, dev);
> >
> > +	dw_i2c_get_plat_data(dev);
> > +
> Instead of this added code would it be possible to use generic timing
> parameters which can come either from firmware or code? Those are
> handled already here by the call to i2c_parse_fw_timings().
> 
> Then drivers/i2c/busses/i2c-designware-master.c:
> i2c_dw_set_timings_master() takes care of calculating Designware
> specific hcnt/lcnt timing parameters from those generic values.
> 

I am confused about why fs_hcnt/fs_lcnt must be set when I use the
standard mode?
Andrew Lunn April 20, 2023, 1:22 p.m. UTC | #5
On Thu, Apr 20, 2023 at 06:29:11PM +0800, Jiawen Wu wrote:
> On Thursday, April 20, 2023 4:58 AM, Andrew Lunn wrote:
> > On Wed, Apr 19, 2023 at 04:27:33PM +0800, Jiawen Wu wrote:
> > > Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate
> > > with SFP.
> > >
> > > Add platform data to pass IOMEM base address, board flag and other
> > > parameters, since resource address was mapped on ethernet driver.
> > >
> > > The exists IP limitations are dealt as workarounds:
> > > - IP does not support interrupt mode, it works on polling mode.
> > > - I2C cannot read continuously, only one byte can at a time.
> > 
> > Are you really sure about that?
> > 
> > It is a major limitation for SFP devices. It means you cannot access
> > the diagnostics, since you need to perform an atomic 2 byte read.
> > 
> > Or maybe i'm understanding you wrong.
> > 
> >    Andrew
> > 
> 
> Maybe I'm a little confused about this. Every time I read a byte info, I have to
> write a 'read command'. It can normally get the information for SFP devices.
> But I'm not sure if this is regular I2C behavior.
 
I don't know this hardware, so i cannot say what a 'read command'
actually does. Can you put a bus pirate or similar sort of device on
the bus and look at the actual I2C signals. Is it performing one I2C
transaction per byte? If so, that is not good.

The diagnostic values, things like temperature sensor, voltage sensor,
received signal power are all 16 bits. You cannot read them using two
time one byte reads. Say the first read sees a 16bit value of 0x00FF,
but only reads the first byte. The second read sees a 16bit value of
0x0100 but only reads the second byte. You end up with 0x0000. When
you do a multi byte read, the SFP should do an atomic read of the
sensor, so you would see either 0x00FF, or 0x0100.

If your hardware can only do single byte reads, please make sure the
I2C framework knows this. The SFP driver should then refuse to access
the diagnostic parts of the SFP, because your I2C bus master hardware
is too broken. The rest of the SFP should still work.

	Andrew.
Jiawen Wu April 21, 2023, 2:20 a.m. UTC | #6
On Thursday, April 20, 2023 9:23 PM, Andrew Lunn wrote:
> On Thu, Apr 20, 2023 at 06:29:11PM +0800, Jiawen Wu wrote:
> > On Thursday, April 20, 2023 4:58 AM, Andrew Lunn wrote:
> > > On Wed, Apr 19, 2023 at 04:27:33PM +0800, Jiawen Wu wrote:
> > > > Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate
> > > > with SFP.
> > > >
> > > > Add platform data to pass IOMEM base address, board flag and other
> > > > parameters, since resource address was mapped on ethernet driver.
> > > >
> > > > The exists IP limitations are dealt as workarounds:
> > > > - IP does not support interrupt mode, it works on polling mode.
> > > > - I2C cannot read continuously, only one byte can at a time.
> > >
> > > Are you really sure about that?
> > >
> > > It is a major limitation for SFP devices. It means you cannot access
> > > the diagnostics, since you need to perform an atomic 2 byte read.
> > >
> > > Or maybe i'm understanding you wrong.
> > >
> > >    Andrew
> > >
> >
> > Maybe I'm a little confused about this. Every time I read a byte info, I have to
> > write a 'read command'. It can normally get the information for SFP devices.
> > But I'm not sure if this is regular I2C behavior.
> 
> I don't know this hardware, so i cannot say what a 'read command'
> actually does. Can you put a bus pirate or similar sort of device on
> the bus and look at the actual I2C signals. Is it performing one I2C
> transaction per byte? If so, that is not good.
> 
> The diagnostic values, things like temperature sensor, voltage sensor,
> received signal power are all 16 bits. You cannot read them using two
> time one byte reads. Say the first read sees a 16bit value of 0x00FF,
> but only reads the first byte. The second read sees a 16bit value of
> 0x0100 but only reads the second byte. You end up with 0x0000. When
> you do a multi byte read, the SFP should do an atomic read of the
> sensor, so you would see either 0x00FF, or 0x0100.
> 
> If your hardware can only do single byte reads, please make sure the
> I2C framework knows this. The SFP driver should then refuse to access
> the diagnostic parts of the SFP, because your I2C bus master hardware
> is too broken. The rest of the SFP should still work.
> 
> 	Andrew.
> 

You may have misunderstood. If you want to read a 16-bit message, the
size of 'i2c_msg.len' is set to 2 in the array that 'flags = I2C_M_RD'.

For example in sfp_i2c_read(), block_size limits the message length of every
time call i2c_transfer() to read, usually it is 16. The one-byte read limit I
mentioned means that during the 16-byte read, I2C device needs to write a
read command and then read the message 16 times to fill the 16-byte buffer,
instead of reading 16 bytes at once after stop command writes.

Before I thought this behavior might be strange, so I mentioned in the commit.
Jarkko Nikula April 21, 2023, 6:52 a.m. UTC | #7
On 4/20/23 13:29, Jiawen Wu wrote:
> On Thursday, April 20, 2023 4:58 AM, Andrew Lunn wrote:
>> On Wed, Apr 19, 2023 at 04:27:33PM +0800, Jiawen Wu wrote:
>>> Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate
>>> with SFP.
>>>
>>> Add platform data to pass IOMEM base address, board flag and other
>>> parameters, since resource address was mapped on ethernet driver.
>>>
>>> The exists IP limitations are dealt as workarounds:
>>> - IP does not support interrupt mode, it works on polling mode.
>>> - I2C cannot read continuously, only one byte can at a time.
>>
>> Are you really sure about that?
>>
>> It is a major limitation for SFP devices. It means you cannot access
>> the diagnostics, since you need to perform an atomic 2 byte read.
>>
>> Or maybe i'm understanding you wrong.
>>
>>     Andrew
>>
> 
> Maybe I'm a little confused about this. Every time I read a byte info, I have to
> write a 'read command'. It can normally get the information for SFP devices.
> But I'm not sure if this is regular I2C behavior.
> 
I agree, IC_DATA_CMD operation is obscure. In order to read from the 
bus, writes with BIT(8) set is required into IC_DATA_CMD, wait 
(irq/poll) DW_IC_INTR_RX_FULL is set in DW_IC_RAW_INTR_STAT and then 
read back received data from IC_DATA_CMD while taking into count FIFO sizes.

Full documentation of this IP is in DesignWare DW_apb_i2c Databook. You 
may try to get access to Synopsys or try to find if older version of it 
is available somewhere.

https://www.synopsys.com/dw/ipdir.php?c=DW_apb_i2c

Plain register specifications can be found from some of the Intel 
datasheets. For example chapter 13.2 I2C Memory Mapped Registers 
Summary, page 667 in a datasheet below:

https://cdrdv2-public.intel.com/743845/743845_001.pdf
Andrew Lunn April 21, 2023, 12:15 p.m. UTC | #8
On Fri, Apr 21, 2023 at 10:20:17AM +0800, Jiawen Wu wrote:
> On Thursday, April 20, 2023 9:23 PM, Andrew Lunn wrote:
> > On Thu, Apr 20, 2023 at 06:29:11PM +0800, Jiawen Wu wrote:
> > > On Thursday, April 20, 2023 4:58 AM, Andrew Lunn wrote:
> > > > On Wed, Apr 19, 2023 at 04:27:33PM +0800, Jiawen Wu wrote:
> > > > > Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate
> > > > > with SFP.
> > > > >
> > > > > Add platform data to pass IOMEM base address, board flag and other
> > > > > parameters, since resource address was mapped on ethernet driver.
> > > > >
> > > > > The exists IP limitations are dealt as workarounds:
> > > > > - IP does not support interrupt mode, it works on polling mode.
> > > > > - I2C cannot read continuously, only one byte can at a time.
> > > >
> > > > Are you really sure about that?
> > > >
> > > > It is a major limitation for SFP devices. It means you cannot access
> > > > the diagnostics, since you need to perform an atomic 2 byte read.
> > > >
> > > > Or maybe i'm understanding you wrong.
> > > >
> > > >    Andrew
> > > >
> > >
> > > Maybe I'm a little confused about this. Every time I read a byte info, I have to
> > > write a 'read command'. It can normally get the information for SFP devices.
> > > But I'm not sure if this is regular I2C behavior.
> > 
> > I don't know this hardware, so i cannot say what a 'read command'
> > actually does. Can you put a bus pirate or similar sort of device on
> > the bus and look at the actual I2C signals. Is it performing one I2C
> > transaction per byte? If so, that is not good.

....

> You may have misunderstood. If you want to read a 16-bit message, the
> size of 'i2c_msg.len' is set to 2 in the array that 'flags = I2C_M_RD'.

The SFP driver uses a mixture of message lengths, due to SFP bugs. But
in general it will do 16 byte block reads, except for when it needs
smaller quantity of bytes.

However, your wording was:

> > > Every time I read a byte info, I have to write a 'read command'.

This suggests you are reading one byte at a time with each read
command. I just want to make sure that is not one I2C transaction per
byte.

     Andrew.
Andrew Lunn April 21, 2023, 12:22 p.m. UTC | #9
On Fri, Apr 21, 2023 at 09:52:02AM +0300, Jarkko Nikula wrote:
> On 4/20/23 13:29, Jiawen Wu wrote:
> > On Thursday, April 20, 2023 4:58 AM, Andrew Lunn wrote:
> > > On Wed, Apr 19, 2023 at 04:27:33PM +0800, Jiawen Wu wrote:
> > > > Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate
> > > > with SFP.
> > > > 
> > > > Add platform data to pass IOMEM base address, board flag and other
> > > > parameters, since resource address was mapped on ethernet driver.
> > > > 
> > > > The exists IP limitations are dealt as workarounds:
> > > > - IP does not support interrupt mode, it works on polling mode.
> > > > - I2C cannot read continuously, only one byte can at a time.
> > > 
> > > Are you really sure about that?
> > > 
> > > It is a major limitation for SFP devices. It means you cannot access
> > > the diagnostics, since you need to perform an atomic 2 byte read.
> > > 
> > > Or maybe i'm understanding you wrong.
> > > 
> > >     Andrew
> > > 
> > 
> > Maybe I'm a little confused about this. Every time I read a byte info, I have to
> > write a 'read command'. It can normally get the information for SFP devices.
> > But I'm not sure if this is regular I2C behavior.
> > 
> I agree, IC_DATA_CMD operation is obscure. In order to read from the bus,
> writes with BIT(8) set is required into IC_DATA_CMD, wait (irq/poll)
> DW_IC_INTR_RX_FULL is set in DW_IC_RAW_INTR_STAT and then read back received
> data from IC_DATA_CMD while taking into count FIFO sizes.

Just for my understanding, this read command just allows access to the
data in the FIFO. It has nothing to do with I2C bus transactions.

You also mention FIFO depth. So you should not need to do this per
byte, you can read upto the full depth of the FIFO before having to do
the read command, poll/irq cycle again?

	Andrew
Jarkko Nikula April 21, 2023, 1 p.m. UTC | #10
On 4/21/23 15:22, Andrew Lunn wrote:
> On Fri, Apr 21, 2023 at 09:52:02AM +0300, Jarkko Nikula wrote:
>> I agree, IC_DATA_CMD operation is obscure. In order to read from the bus,
>> writes with BIT(8) set is required into IC_DATA_CMD, wait (irq/poll)
>> DW_IC_INTR_RX_FULL is set in DW_IC_RAW_INTR_STAT and then read back received
>> data from IC_DATA_CMD while taking into count FIFO sizes.
> 
> Just for my understanding, this read command just allows access to the
> data in the FIFO. It has nothing to do with I2C bus transactions.
> 
Not only but it controls both the bus transactions and data to/from FIFO.

> You also mention FIFO depth. So you should not need to do this per
> byte, you can read upto the full depth of the FIFO before having to do
> the read command, poll/irq cycle again?
> 
Commands need to be written to IC_DATA_CMD for each byte and no more 
than is the FIFO depth. Like writing n read commands to it, wait for 
RX_FULL and read as many bytes as available, continue waiting if not done.

It perhaps best explained by looking at 
drivers/i2c/busses/i2c-designware-master.c: i2c_dw_xfer_msg() and 
i2c_dw_read().
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 0dc6b1ce663f..e4faab4655cb 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -588,6 +588,10 @@  int i2c_dw_set_fifo_size(struct dw_i2c_dev *dev)
 	if (ret)
 		return ret;
 
+	/* workaround for IP hardware issue */
+	if ((dev->flags & MODEL_MASK) == MODEL_WANGXUN_SP)
+		param |= 0x80800;
+
 	tx_fifo_depth = ((param >> 16) & 0xff) + 1;
 	rx_fifo_depth = ((param >> 8)  & 0xff) + 1;
 	if (!dev->tx_fifo_depth) {
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 050d8c63ad3c..c686198e12d2 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..21e350203247 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -354,6 +354,75 @@  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;
+	u8 dev_addr;
+	u8 *buf;
+
+	dev->msgs = msgs;
+	dev->msgs_num = num_msgs;
+	i2c_dw_xfer_init(dev);
+	regmap_write(dev->map, DW_IC_INTR_MASK, 0);
+
+	dev_addr = msgs[0].buf[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 (msgs[msg_idx].flags & I2C_M_RD) {
+				ret = i2c_dw_poll_tx_empty(dev);
+				if (ret)
+					return ret;
+
+				regmap_write(dev->map, DW_IC_DATA_CMD,
+					     (dev_addr + data_idx) | BIT(9));
+				regmap_write(dev->map, DW_IC_DATA_CMD, 0x100 | BIT(9));
+
+				ret = i2c_dw_poll_rx_full(dev);
+				if (ret)
+					return ret;
+
+				regmap_read(dev->map, DW_IC_DATA_CMD, &val);
+				buf[data_idx] = 0xFF & val;
+			} else {
+				ret = i2c_dw_poll_tx_empty(dev);
+				if (ret)
+					return ret;
+
+				regmap_write(dev->map, DW_IC_DATA_CMD, buf[data_idx]);
+				if (data_idx == (buf_len - 1))
+					regmap_write(dev->map, DW_IC_DATA_CMD, BIT(9));
+			}
+		}
+	}
+
+	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
@@ -568,6 +637,11 @@  i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 		goto done_nolock;
 	}
 
+	if ((dev->flags & MODEL_MASK) == MODEL_WANGXUN_SP) {
+		ret = txgbe_i2c_dw_xfer_quirk(adap, msgs, num);
+		goto done_nolock;
+	}
+
 	reinit_completion(&dev->cmd_complete);
 	dev->msgs = msgs;
 	dev->msgs_num = num;
@@ -848,7 +922,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 poll_i2c_adap_quirk(struct dw_i2c_dev *dev)
 {
 	struct i2c_adapter *adap = &dev->adapter;
 	int ret;
@@ -862,6 +936,17 @@  static int amd_i2c_adap_quirk(struct dw_i2c_dev *dev)
 	return ret;
 }
 
+static bool i2c_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 +1002,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_is_model_poll(dev))
+		return poll_i2c_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 74182db03a88..10b2c61b279f 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -23,6 +23,7 @@ 
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/platform_data/i2c-dw.h>
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
@@ -179,12 +180,14 @@  static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)
 static int dw_i2c_plat_request_regs(struct dw_i2c_dev *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev->dev);
-	int ret;
+	int ret = 0;
 
 	switch (dev->flags & MODEL_MASK) {
 	case MODEL_BAIKAL_BT1:
 		ret = bt1_i2c_request_regs(dev);
 		break;
+	case MODEL_WANGXUN_SP:
+		break;
 	default:
 		dev->base = devm_platform_ioremap_resource(pdev, 0);
 		ret = PTR_ERR_OR_ZERO(dev->base);
@@ -194,6 +197,35 @@  static int dw_i2c_plat_request_regs(struct dw_i2c_dev *dev)
 	return ret;
 }
 
+static void dw_i2c_get_plat_data(struct dw_i2c_dev *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev->dev);
+	struct dw_i2c_platform_data *pdata;
+
+	pdata = dev_get_platdata(&pdev->dev);
+	if (!pdata)
+		return;
+
+	dev->flags |= pdata->flags;
+	dev->base = pdata->base;
+
+	if (pdata->ss_hcnt && pdata->ss_lcnt) {
+		dev->ss_hcnt = pdata->ss_hcnt;
+		dev->ss_lcnt = pdata->ss_lcnt;
+	} else {
+		dev->ss_hcnt = 6;
+		dev->ss_lcnt = 8;
+	}
+
+	if (pdata->fs_hcnt && pdata->fs_lcnt) {
+		dev->fs_hcnt = pdata->fs_hcnt;
+		dev->fs_lcnt = pdata->fs_lcnt;
+	} else {
+		dev->fs_hcnt = 6;
+		dev->fs_lcnt = 8;
+	}
+}
+
 static const struct dmi_system_id dw_i2c_hwmon_class_dmi[] = {
 	{
 		.ident = "Qtechnology QT5222",
@@ -282,6 +314,8 @@  static int dw_i2c_plat_probe(struct platform_device *pdev)
 	dev->irq = irq;
 	platform_set_drvdata(pdev, dev);
 
+	dw_i2c_get_plat_data(dev);
+
 	ret = dw_i2c_plat_request_regs(dev);
 	if (ret)
 		return ret;
diff --git a/include/linux/platform_data/i2c-dw.h b/include/linux/platform_data/i2c-dw.h
new file mode 100644
index 000000000000..f4552df08084
--- /dev/null
+++ b/include/linux/platform_data/i2c-dw.h
@@ -0,0 +1,15 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_I2C_DW_H
+#define _LINUX_I2C_DW_H
+
+struct dw_i2c_platform_data {
+	void __iomem *base;
+	unsigned int flags;
+	unsigned int ss_hcnt;
+	unsigned int ss_lcnt;
+	unsigned int fs_hcnt;
+	unsigned int fs_lcnt;
+};
+
+#endif /* _LINUX_I2C_DW_H */