Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/3] net: ks8851-ml: Remove 8-bit bus accessors
@ 2020-02-10 18:41 Marek Vasut
  2020-02-10 18:41 ` [PATCH 2/3] net: ks8851-ml: Fix 16-bit data access Marek Vasut
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Marek Vasut @ 2020-02-10 18:41 UTC (permalink / raw)
  To: netdev
  Cc: Marek Vasut, David S . Miller, Lukas Wunner, Petr Stetiar, YueHaibing

This driver is mixing 8-bit and 16-bit bus accessors for reasons unknown,
however the speculation is that this was some sort of attempt to support
the 8-bit bus mode.

As per the KS8851-16MLL documentation, all two registers accessed via the
8-bit accessors are internally 16-bit registers, so reading them using
16-bit accessors is fine. The KS_CCR read can be converted to 16-bit read
outright, as it is already a concatenation of two 8-bit reads of that
register. The KS_RXQCR accesses are 8-bit only, however writing the top
8 bits of the register is OK as well, since the driver caches the entire
16-bit register value anyway.

Finally, the driver is not used by any hardware in the kernel right now.
The only hardware available to me is one with 16-bit bus, so I have no
way to test the 8-bit bus mode, however it is unlikely this ever really
worked anyway. If the 8-bit bus mode is ever required, it can be easily
added by adjusting the 16-bit accessors to do 2 consecutive accesses,
which is how this should have been done from the beginning.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/micrel/ks8851_mll.c | 45 +++---------------------
 1 file changed, 5 insertions(+), 40 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851_mll.c b/drivers/net/ethernet/micrel/ks8851_mll.c
index a41a90c589db..e2fb20154511 100644
--- a/drivers/net/ethernet/micrel/ks8851_mll.c
+++ b/drivers/net/ethernet/micrel/ks8851_mll.c
@@ -156,24 +156,6 @@ static int msg_enable;
  * chip is busy transferring packet data (RX/TX FIFO accesses).
  */
 
-/**
- * ks_rdreg8 - read 8 bit register from device
- * @ks	  : The chip information
- * @offset: The register address
- *
- * Read a 8bit register from the chip, returning the result
- */
-static u8 ks_rdreg8(struct ks_net *ks, int offset)
-{
-	u16 data;
-	u8 shift_bit = offset & 0x03;
-	u8 shift_data = (offset & 1) << 3;
-	ks->cmd_reg_cache = (u16) offset | (u16)(BE0 << shift_bit);
-	iowrite16(ks->cmd_reg_cache, ks->hw_addr_cmd);
-	data  = ioread16(ks->hw_addr);
-	return (u8)(data >> shift_data);
-}
-
 /**
  * ks_rdreg16 - read 16 bit register from device
  * @ks	  : The chip information
@@ -189,22 +171,6 @@ static u16 ks_rdreg16(struct ks_net *ks, int offset)
 	return ioread16(ks->hw_addr);
 }
 
-/**
- * ks_wrreg8 - write 8bit register value to chip
- * @ks: The chip information
- * @offset: The register address
- * @value: The value to write
- *
- */
-static void ks_wrreg8(struct ks_net *ks, int offset, u8 value)
-{
-	u8  shift_bit = (offset & 0x03);
-	u16 value_write = (u16)(value << ((offset & 1) << 3));
-	ks->cmd_reg_cache = (u16)offset | (BE0 << shift_bit);
-	iowrite16(ks->cmd_reg_cache, ks->hw_addr_cmd);
-	iowrite16(value_write, ks->hw_addr);
-}
-
 /**
  * ks_wrreg16 - write 16bit register value to chip
  * @ks: The chip information
@@ -324,8 +290,7 @@ static void ks_read_config(struct ks_net *ks)
 	u16 reg_data = 0;
 
 	/* Regardless of bus width, 8 bit read should always work.*/
-	reg_data = ks_rdreg8(ks, KS_CCR) & 0x00FF;
-	reg_data |= ks_rdreg8(ks, KS_CCR+1) << 8;
+	reg_data = ks_rdreg16(ks, KS_CCR);
 
 	/* addr/data bus are multiplexed */
 	ks->sharedbus = (reg_data & CCR_SHARED) == CCR_SHARED;
@@ -429,7 +394,7 @@ static inline void ks_read_qmu(struct ks_net *ks, u16 *buf, u32 len)
 
 	/* 1. set sudo DMA mode */
 	ks_wrreg16(ks, KS_RXFDPR, RXFDPR_RXFPAI);
-	ks_wrreg8(ks, KS_RXQCR, (ks->rc_rxqcr | RXQCR_SDA) & 0xff);
+	ks_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_SDA);
 
 	/* 2. read prepend data */
 	/**
@@ -446,7 +411,7 @@ static inline void ks_read_qmu(struct ks_net *ks, u16 *buf, u32 len)
 	ks_inblk(ks, buf, ALIGN(len, 4));
 
 	/* 4. reset sudo DMA Mode */
-	ks_wrreg8(ks, KS_RXQCR, ks->rc_rxqcr);
+	ks_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr);
 }
 
 /**
@@ -679,13 +644,13 @@ static void ks_write_qmu(struct ks_net *ks, u8 *pdata, u16 len)
 	ks->txh.txw[1] = cpu_to_le16(len);
 
 	/* 1. set sudo-DMA mode */
-	ks_wrreg8(ks, KS_RXQCR, (ks->rc_rxqcr | RXQCR_SDA) & 0xff);
+	ks_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_SDA);
 	/* 2. write status/lenth info */
 	ks_outblk(ks, ks->txh.txw, 4);
 	/* 3. write pkt data */
 	ks_outblk(ks, (u16 *)pdata, ALIGN(len, 4));
 	/* 4. reset sudo-DMA mode */
-	ks_wrreg8(ks, KS_RXQCR, ks->rc_rxqcr);
+	ks_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr);
 	/* 5. Enqueue Tx(move the pkt from TX buffer into TXQ) */
 	ks_wrreg16(ks, KS_TXQCR, TXQCR_METFE);
 	/* 6. wait until TXQCR_METFE is auto-cleared */
-- 
2.24.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/3] net: ks8851-ml: Fix 16-bit data access
  2020-02-10 18:41 [PATCH 1/3] net: ks8851-ml: Remove 8-bit bus accessors Marek Vasut
@ 2020-02-10 18:41 ` Marek Vasut
  2020-02-10 19:35   ` Lukas Wunner
  2020-02-15  9:24   ` David Miller
  2020-02-10 18:41 ` [PATCH 3/3] net: ks8851-ml: Fix 16-bit IO operation Marek Vasut
  2020-02-10 19:31 ` [PATCH 1/3] net: ks8851-ml: Remove 8-bit bus accessors Lukas Wunner
  2 siblings, 2 replies; 10+ messages in thread
From: Marek Vasut @ 2020-02-10 18:41 UTC (permalink / raw)
  To: netdev
  Cc: Marek Vasut, David S . Miller, Lukas Wunner, Petr Stetiar, YueHaibing

The packet data written to and read from Micrel KSZ8851-16MLLI must be
byte-swapped in 16-bit mode, add this byte-swapping.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/micrel/ks8851_mll.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851_mll.c b/drivers/net/ethernet/micrel/ks8851_mll.c
index e2fb20154511..e9ca198a67a4 100644
--- a/drivers/net/ethernet/micrel/ks8851_mll.c
+++ b/drivers/net/ethernet/micrel/ks8851_mll.c
@@ -197,7 +197,7 @@ static inline void ks_inblk(struct ks_net *ks, u16 *wptr, u32 len)
 {
 	len >>= 1;
 	while (len--)
-		*wptr++ = (u16)ioread16(ks->hw_addr);
+		*wptr++ = swab16(ioread16(ks->hw_addr));
 }
 
 /**
@@ -211,7 +211,7 @@ static inline void ks_outblk(struct ks_net *ks, u16 *wptr, u32 len)
 {
 	len >>= 1;
 	while (len--)
-		iowrite16(*wptr++, ks->hw_addr);
+		iowrite16(swab16(*wptr++), ks->hw_addr);
 }
 
 static void ks_disable_int(struct ks_net *ks)
-- 
2.24.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 3/3] net: ks8851-ml: Fix 16-bit IO operation
  2020-02-10 18:41 [PATCH 1/3] net: ks8851-ml: Remove 8-bit bus accessors Marek Vasut
  2020-02-10 18:41 ` [PATCH 2/3] net: ks8851-ml: Fix 16-bit data access Marek Vasut
@ 2020-02-10 18:41 ` Marek Vasut
  2020-02-10 19:31 ` [PATCH 1/3] net: ks8851-ml: Remove 8-bit bus accessors Lukas Wunner
  2 siblings, 0 replies; 10+ messages in thread
From: Marek Vasut @ 2020-02-10 18:41 UTC (permalink / raw)
  To: netdev
  Cc: Marek Vasut, David S . Miller, Lukas Wunner, Petr Stetiar, YueHaibing

The Micrel KSZ8851-16MLLI datasheet DS00002357B page 12 states that
BE[3:0] signals are active high. This contradicts the measurements
of the behavior of the actual chip, where these signals behave as
active low. For example, to read the CIDER register, the bus must
expose 0xc0c0 during the address phase, which means BE[3:0]=4'b1100.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/micrel/ks8851_mll.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851_mll.c b/drivers/net/ethernet/micrel/ks8851_mll.c
index e9ca198a67a4..0729b95d555c 100644
--- a/drivers/net/ethernet/micrel/ks8851_mll.c
+++ b/drivers/net/ethernet/micrel/ks8851_mll.c
@@ -166,7 +166,7 @@ static int msg_enable;
 
 static u16 ks_rdreg16(struct ks_net *ks, int offset)
 {
-	ks->cmd_reg_cache = (u16)offset | ((BE1 | BE0) << (offset & 0x02));
+	ks->cmd_reg_cache = (u16)offset | ((BE3 | BE2) >> (offset & 0x02));
 	iowrite16(ks->cmd_reg_cache, ks->hw_addr_cmd);
 	return ioread16(ks->hw_addr);
 }
@@ -181,7 +181,7 @@ static u16 ks_rdreg16(struct ks_net *ks, int offset)
 
 static void ks_wrreg16(struct ks_net *ks, int offset, u16 value)
 {
-	ks->cmd_reg_cache = (u16)offset | ((BE1 | BE0) << (offset & 0x02));
+	ks->cmd_reg_cache = (u16)offset | ((BE3 | BE2) >> (offset & 0x02));
 	iowrite16(ks->cmd_reg_cache, ks->hw_addr_cmd);
 	iowrite16(value, ks->hw_addr);
 }
-- 
2.24.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] net: ks8851-ml: Remove 8-bit bus accessors
  2020-02-10 18:41 [PATCH 1/3] net: ks8851-ml: Remove 8-bit bus accessors Marek Vasut
  2020-02-10 18:41 ` [PATCH 2/3] net: ks8851-ml: Fix 16-bit data access Marek Vasut
  2020-02-10 18:41 ` [PATCH 3/3] net: ks8851-ml: Fix 16-bit IO operation Marek Vasut
@ 2020-02-10 19:31 ` Lukas Wunner
  2020-02-10 19:44   ` Marek Vasut
  2 siblings, 1 reply; 10+ messages in thread
From: Lukas Wunner @ 2020-02-10 19:31 UTC (permalink / raw)
  To: Marek Vasut; +Cc: netdev, David S . Miller, Petr Stetiar, YueHaibing

On Mon, Feb 10, 2020 at 07:41:37PM +0100, Marek Vasut wrote:
> This driver is mixing 8-bit and 16-bit bus accessors for reasons unknown,
> however the speculation is that this was some sort of attempt to support
> the 8-bit bus mode.

ks8851.c was introduced in July 2009 with commit 3ba81f3ece3c.
ks8851_mll.c was introduced two months later with a55c0a0ed415.

Perhaps the 8-bit accesses are remnants of the SPI-version ks8851.c?

Both chips are very similar.  Unfortunately ks8851_mll.c duplicated
much of ks8851.c, instead of separating it into a common portion and
an SPI-specific portion.  I've deduplicated at least the register
macros with commit aae079aa76d0.  It would be great if you could
continue this effort and increase the amount of shared code between
the two drivers.  Right now ks8851_mll.c supports features that
ks8851.c does not, e.g. multicast filtering.  On the other hand
I've fixed bugs in ks8851.c which I believe still exist in ks8851_mll.c,
see 536d3680fd2d for an example.  I didn't apply the fixes to
ks8851_mll.c simply because I don't have hardware with that chip.
I do have access to hardware using ks8851.c.

HTH,

Lukas

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] net: ks8851-ml: Fix 16-bit data access
  2020-02-10 18:41 ` [PATCH 2/3] net: ks8851-ml: Fix 16-bit data access Marek Vasut
@ 2020-02-10 19:35   ` Lukas Wunner
  2020-02-10 19:40     ` Marek Vasut
  2020-02-15  9:24   ` David Miller
  1 sibling, 1 reply; 10+ messages in thread
From: Lukas Wunner @ 2020-02-10 19:35 UTC (permalink / raw)
  To: Marek Vasut; +Cc: netdev, David S . Miller, Petr Stetiar, YueHaibing

On Mon, Feb 10, 2020 at 07:41:38PM +0100, Marek Vasut wrote:
> The packet data written to and read from Micrel KSZ8851-16MLLI must be
> byte-swapped in 16-bit mode, add this byte-swapping.
[...]
> -		*wptr++ = (u16)ioread16(ks->hw_addr);
> +		*wptr++ = swab16(ioread16(ks->hw_addr));

Um, doesn't this depend on the endianness of the CPU architecture?
I'd expect be16_to_cpu() or le16_to_cpu() here instead of swab16().

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] net: ks8851-ml: Fix 16-bit data access
  2020-02-10 19:35   ` Lukas Wunner
@ 2020-02-10 19:40     ` Marek Vasut
  0 siblings, 0 replies; 10+ messages in thread
From: Marek Vasut @ 2020-02-10 19:40 UTC (permalink / raw)
  To: Lukas Wunner; +Cc: netdev, David S . Miller, Petr Stetiar, YueHaibing

On 2/10/20 8:35 PM, Lukas Wunner wrote:
> On Mon, Feb 10, 2020 at 07:41:38PM +0100, Marek Vasut wrote:
>> The packet data written to and read from Micrel KSZ8851-16MLLI must be
>> byte-swapped in 16-bit mode, add this byte-swapping.
> [...]
>> -		*wptr++ = (u16)ioread16(ks->hw_addr);
>> +		*wptr++ = swab16(ioread16(ks->hw_addr));
> 
> Um, doesn't this depend on the endianness of the CPU architecture?
> I'd expect be16_to_cpu() or le16_to_cpu() here instead of swab16().

I don't really know in this case, this is a bus IO accessor, so I would
expect the answer is no. But I can only test this on ARMv7a.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] net: ks8851-ml: Remove 8-bit bus accessors
  2020-02-10 19:31 ` [PATCH 1/3] net: ks8851-ml: Remove 8-bit bus accessors Lukas Wunner
@ 2020-02-10 19:44   ` Marek Vasut
  0 siblings, 0 replies; 10+ messages in thread
From: Marek Vasut @ 2020-02-10 19:44 UTC (permalink / raw)
  To: Lukas Wunner; +Cc: netdev, David S . Miller, Petr Stetiar, YueHaibing

On 2/10/20 8:31 PM, Lukas Wunner wrote:
> On Mon, Feb 10, 2020 at 07:41:37PM +0100, Marek Vasut wrote:
>> This driver is mixing 8-bit and 16-bit bus accessors for reasons unknown,
>> however the speculation is that this was some sort of attempt to support
>> the 8-bit bus mode.
> 
> ks8851.c was introduced in July 2009 with commit 3ba81f3ece3c.
> ks8851_mll.c was introduced two months later with a55c0a0ed415.
> 
> Perhaps the 8-bit accesses are remnants of the SPI-version ks8851.c?
> 
> Both chips are very similar.  Unfortunately ks8851_mll.c duplicated
> much of ks8851.c, instead of separating it into a common portion and
> an SPI-specific portion.  I've deduplicated at least the register
> macros with commit aae079aa76d0.  It would be great if you could
> continue this effort and increase the amount of shared code between
> the two drivers.  Right now ks8851_mll.c supports features that
> ks8851.c does not, e.g. multicast filtering.  On the other hand
> I've fixed bugs in ks8851.c which I believe still exist in ks8851_mll.c,
> see 536d3680fd2d for an example.  I didn't apply the fixes to
> ks8851_mll.c simply because I don't have hardware with that chip.
> I do have access to hardware using ks8851.c.

Right now I cannot promise that I'll be able to work on this driver
beyond these basic fixes. I'll see what I can do.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] net: ks8851-ml: Fix 16-bit data access
  2020-02-10 18:41 ` [PATCH 2/3] net: ks8851-ml: Fix 16-bit data access Marek Vasut
  2020-02-10 19:35   ` Lukas Wunner
@ 2020-02-15  9:24   ` David Miller
  2020-02-15  9:47     ` Marek Vasut
  1 sibling, 1 reply; 10+ messages in thread
From: David Miller @ 2020-02-15  9:24 UTC (permalink / raw)
  To: marex; +Cc: netdev, lukas, ynezz, yuehaibing

From: Marek Vasut <marex@denx.de>
Date: Mon, 10 Feb 2020 19:41:38 +0100

> @@ -197,7 +197,7 @@ static inline void ks_inblk(struct ks_net *ks, u16 *wptr, u32 len)
>  {
>  	len >>= 1;
>  	while (len--)
> -		*wptr++ = (u16)ioread16(ks->hw_addr);
> +		*wptr++ = swab16(ioread16(ks->hw_addr));

I agree with the other feedback, the endianness looks wrong here.

The ioread16() translates whatever is behind ks->hw_addr into cpu
endianness.

This is either always little endian (for example), which means that
unconditionally swapping this doesn't seem to make sense.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] net: ks8851-ml: Fix 16-bit data access
  2020-02-15  9:24   ` David Miller
@ 2020-02-15  9:47     ` Marek Vasut
  2020-02-15  9:56       ` David Miller
  0 siblings, 1 reply; 10+ messages in thread
From: Marek Vasut @ 2020-02-15  9:47 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, lukas, ynezz, yuehaibing

On 2/15/20 10:24 AM, David Miller wrote:
> From: Marek Vasut <marex@denx.de>
> Date: Mon, 10 Feb 2020 19:41:38 +0100
> 
>> @@ -197,7 +197,7 @@ static inline void ks_inblk(struct ks_net *ks, u16 *wptr, u32 len)
>>  {
>>  	len >>= 1;
>>  	while (len--)
>> -		*wptr++ = (u16)ioread16(ks->hw_addr);
>> +		*wptr++ = swab16(ioread16(ks->hw_addr));
> 
> I agree with the other feedback, the endianness looks wrong here.
> 
> The ioread16() translates whatever is behind ks->hw_addr into cpu
> endianness.
> 
> This is either always little endian (for example), which means that
> unconditionally swapping this doesn't seem to make sense.

So what is the suggestion to fix this properly ?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] net: ks8851-ml: Fix 16-bit data access
  2020-02-15  9:47     ` Marek Vasut
@ 2020-02-15  9:56       ` David Miller
  0 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2020-02-15  9:56 UTC (permalink / raw)
  To: marex; +Cc: netdev, lukas, ynezz, yuehaibing

From: Marek Vasut <marex@denx.de>
Date: Sat, 15 Feb 2020 10:47:56 +0100

> On 2/15/20 10:24 AM, David Miller wrote:
>> From: Marek Vasut <marex@denx.de>
>> Date: Mon, 10 Feb 2020 19:41:38 +0100
>> 
>>> @@ -197,7 +197,7 @@ static inline void ks_inblk(struct ks_net *ks, u16 *wptr, u32 len)
>>>  {
>>>  	len >>= 1;
>>>  	while (len--)
>>> -		*wptr++ = (u16)ioread16(ks->hw_addr);
>>> +		*wptr++ = swab16(ioread16(ks->hw_addr));
>> 
>> I agree with the other feedback, the endianness looks wrong here.
>> 
>> The ioread16() translates whatever is behind ks->hw_addr into cpu
>> endianness.
>> 
>> This is either always little endian (for example), which means that
>> unconditionally swapping this doesn't seem to make sense.
> 
> So what is the suggestion to fix this properly ?

once you know the endianness coming out of ioread16() you can then
use le16_to_cpu() or be16_to_cpu() as appropriate.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-10 18:41 [PATCH 1/3] net: ks8851-ml: Remove 8-bit bus accessors Marek Vasut
2020-02-10 18:41 ` [PATCH 2/3] net: ks8851-ml: Fix 16-bit data access Marek Vasut
2020-02-10 19:35   ` Lukas Wunner
2020-02-10 19:40     ` Marek Vasut
2020-02-15  9:24   ` David Miller
2020-02-15  9:47     ` Marek Vasut
2020-02-15  9:56       ` David Miller
2020-02-10 18:41 ` [PATCH 3/3] net: ks8851-ml: Fix 16-bit IO operation Marek Vasut
2020-02-10 19:31 ` [PATCH 1/3] net: ks8851-ml: Remove 8-bit bus accessors Lukas Wunner
2020-02-10 19:44   ` Marek Vasut

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git