| Message ID | 20200323234303.526748-8-marex@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: ks8851: Unify KS8851 SPI and MLL drivers | expand |
On Tue, Mar 24, 2020 at 12:42:56AM +0100, Marek Vasut wrote: > On the SPI variant of KS8851, the MAC address can be programmed with > either 8/16/32-bit writes. To make it easier to support the 16-bit > parallel option of KS8851 too, switch both the MAC address programming > and readout to 16-bit operations. > > Remove ks8851_wrreg8() as it is not used anywhere anymore. > > There should be no functional change. > > 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> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Tue, Mar 24, 2020 at 12:42:56AM +0100, Marek Vasut wrote: > On the SPI variant of KS8851, the MAC address can be programmed with > either 8/16/32-bit writes. To make it easier to support the 16-bit > parallel option of KS8851 too, switch both the MAC address programming > and readout to 16-bit operations. > > Remove ks8851_wrreg8() as it is not used anywhere anymore. > > There should be no functional change. > > 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> > --- [...] > + > + for (i = 0; i < ETH_ALEN; i += 2) { > + val = (dev->dev_addr[i] << 8) | dev->dev_addr[i + 1]; > + ks8851_wrreg16(ks, KS_MAR(i + 1), val); > + } [...] > + for (i = 0; i < ETH_ALEN; i += 2) { > + reg = ks8851_rdreg16(ks, KS_MAR(i + 1)); > + dev->dev_addr[i] = reg & 0xff; > + dev->dev_addr[i + 1] = reg >> 8; > + } I know nothing about the hardware but this seems inconsistent: while writing, you put addr[i] into upper part of the 16-bit value and addr[i+1] into lower but for read you do the opposite. Is it correct? Michal Kubecek
On Tue, Mar 24, 2020 at 12:42:56AM +0100, Marek Vasut wrote: > On the SPI variant of KS8851, the MAC address can be programmed with > either 8/16/32-bit writes. To make it easier to support the 16-bit > parallel option of KS8851 too, switch both the MAC address programming > and readout to 16-bit operations. [...] > static int ks8851_write_mac_addr(struct net_device *dev) > { > struct ks8851_net *ks = netdev_priv(dev); > + u16 val; > int i; > > mutex_lock(&ks->lock); > @@ -358,8 +329,12 @@ static int ks8851_write_mac_addr(struct net_device *dev) > * the first write to the MAC address does not take effect. > */ > ks8851_set_powermode(ks, PMECR_PM_NORMAL); > - for (i = 0; i < ETH_ALEN; i++) > - ks8851_wrreg8(ks, KS_MAR(i), dev->dev_addr[i]); > + > + for (i = 0; i < ETH_ALEN; i += 2) { > + val = (dev->dev_addr[i] << 8) | dev->dev_addr[i + 1]; > + ks8851_wrreg16(ks, KS_MAR(i + 1), val); > + } > + This looks like it won't work on little-endian machines: The MAC bytes are stored in dev->dev_addr as 012345, but in the EEPROM they're stored as 543210. The first 16-bit value that you write is 10 on big-endian and 01 on little-endian if I'm not mistaken. By only writing 8-bit values, the original author elegantly sidestepped this issue. Maybe the simplest and most readable solution is something like: u8 val[2]; ... val[0] = dev->dev_addr[i+1]; val[1] = dev->dev_addr; Then cast val to a u16 when passing it to ks8851_wrreg16(). Alternatively, use cpu_to_be16(). > static void ks8851_read_mac_addr(struct net_device *dev) > { > struct ks8851_net *ks = netdev_priv(dev); > + u16 reg; > int i; > > mutex_lock(&ks->lock); > > - for (i = 0; i < ETH_ALEN; i++) > - dev->dev_addr[i] = ks8851_rdreg8(ks, KS_MAR(i)); > + for (i = 0; i < ETH_ALEN; i += 2) { > + reg = ks8851_rdreg16(ks, KS_MAR(i + 1)); > + dev->dev_addr[i] = reg & 0xff; > + dev->dev_addr[i + 1] = reg >> 8; > + } Same here. These seem to be the only two places where KS_MAR() is used. You may want to adjust that macro so that you don't have to add 1 in each of the two places. Thanks, Lukas
On Tue, Mar 24, 2020 at 09:13:11AM +0100, Lukas Wunner wrote: > On Tue, Mar 24, 2020 at 12:42:56AM +0100, Marek Vasut wrote: > > On the SPI variant of KS8851, the MAC address can be programmed with > > either 8/16/32-bit writes. To make it easier to support the 16-bit > > parallel option of KS8851 too, switch both the MAC address programming > > and readout to 16-bit operations. > [...] > > static int ks8851_write_mac_addr(struct net_device *dev) > > { > > struct ks8851_net *ks = netdev_priv(dev); > > + u16 val; > > int i; > > > > mutex_lock(&ks->lock); > > @@ -358,8 +329,12 @@ static int ks8851_write_mac_addr(struct net_device *dev) > > * the first write to the MAC address does not take effect. > > */ > > ks8851_set_powermode(ks, PMECR_PM_NORMAL); > > - for (i = 0; i < ETH_ALEN; i++) > > - ks8851_wrreg8(ks, KS_MAR(i), dev->dev_addr[i]); > > + > > + for (i = 0; i < ETH_ALEN; i += 2) { > > + val = (dev->dev_addr[i] << 8) | dev->dev_addr[i + 1]; > > + ks8851_wrreg16(ks, KS_MAR(i + 1), val); > > + } > > + > > This looks like it won't work on little-endian machines: The MAC bytes > are stored in dev->dev_addr as 012345, but in the EEPROM they're stored > as 543210. The first 16-bit value that you write is 10 on big-endian > and 01 on little-endian if I'm not mistaken. > > By only writing 8-bit values, the original author elegantly sidestepped > this issue. > > Maybe the simplest and most readable solution is something like: > > u8 val[2]; > ... > val[0] = dev->dev_addr[i+1]; > val[1] = dev->dev_addr; > > Then cast val to a u16 when passing it to ks8851_wrreg16(). > > Alternatively, use cpu_to_be16(). Hi Lukas There is a cpu_to_be16() inside ks8851_wrreg16(). Something i already checked, because i wondered about endianess issues as well. Andrew
On Tue, Mar 24, 2020 at 01:25:53PM +0100, Andrew Lunn wrote: > On Tue, Mar 24, 2020 at 09:13:11AM +0100, Lukas Wunner wrote: > > On Tue, Mar 24, 2020 at 12:42:56AM +0100, Marek Vasut wrote: > > > On the SPI variant of KS8851, the MAC address can be programmed with > > > either 8/16/32-bit writes. To make it easier to support the 16-bit > > > parallel option of KS8851 too, switch both the MAC address programming > > > and readout to 16-bit operations. > > [...] > > > static int ks8851_write_mac_addr(struct net_device *dev) > > > { > > > struct ks8851_net *ks = netdev_priv(dev); > > > + u16 val; > > > int i; > > > > > > mutex_lock(&ks->lock); > > > @@ -358,8 +329,12 @@ static int ks8851_write_mac_addr(struct net_device *dev) > > > * the first write to the MAC address does not take effect. > > > */ > > > ks8851_set_powermode(ks, PMECR_PM_NORMAL); > > > - for (i = 0; i < ETH_ALEN; i++) > > > - ks8851_wrreg8(ks, KS_MAR(i), dev->dev_addr[i]); > > > + > > > + for (i = 0; i < ETH_ALEN; i += 2) { > > > + val = (dev->dev_addr[i] << 8) | dev->dev_addr[i + 1]; > > > + ks8851_wrreg16(ks, KS_MAR(i + 1), val); > > > + } > > > + > > > > This looks like it won't work on little-endian machines: The MAC bytes > > are stored in dev->dev_addr as 012345, but in the EEPROM they're stored > > as 543210. The first 16-bit value that you write is 10 on big-endian > > and 01 on little-endian if I'm not mistaken. > > > > By only writing 8-bit values, the original author elegantly sidestepped > > this issue. > > > > Maybe the simplest and most readable solution is something like: > > > > u8 val[2]; > > ... > > val[0] = dev->dev_addr[i+1]; > > val[1] = dev->dev_addr; > > > > Then cast val to a u16 when passing it to ks8851_wrreg16(). > > > > Alternatively, use cpu_to_be16(). > > There is a cpu_to_be16() inside ks8851_wrreg16(). Something i already > checked, because i wondered about endianess issues as well. There's a cpu_to_le16() in ks8851_wrreg16(), not a cpu_to_be16(). Thanks, Lukas
On 3/24/20 1:36 PM, Lukas Wunner wrote: > On Tue, Mar 24, 2020 at 01:25:53PM +0100, Andrew Lunn wrote: >> On Tue, Mar 24, 2020 at 09:13:11AM +0100, Lukas Wunner wrote: >>> On Tue, Mar 24, 2020 at 12:42:56AM +0100, Marek Vasut wrote: >>>> On the SPI variant of KS8851, the MAC address can be programmed with >>>> either 8/16/32-bit writes. To make it easier to support the 16-bit >>>> parallel option of KS8851 too, switch both the MAC address programming >>>> and readout to 16-bit operations. >>> [...] >>>> static int ks8851_write_mac_addr(struct net_device *dev) >>>> { >>>> struct ks8851_net *ks = netdev_priv(dev); >>>> + u16 val; >>>> int i; >>>> >>>> mutex_lock(&ks->lock); >>>> @@ -358,8 +329,12 @@ static int ks8851_write_mac_addr(struct net_device *dev) >>>> * the first write to the MAC address does not take effect. >>>> */ >>>> ks8851_set_powermode(ks, PMECR_PM_NORMAL); >>>> - for (i = 0; i < ETH_ALEN; i++) >>>> - ks8851_wrreg8(ks, KS_MAR(i), dev->dev_addr[i]); >>>> + >>>> + for (i = 0; i < ETH_ALEN; i += 2) { >>>> + val = (dev->dev_addr[i] << 8) | dev->dev_addr[i + 1]; >>>> + ks8851_wrreg16(ks, KS_MAR(i + 1), val); >>>> + } >>>> + >>> >>> This looks like it won't work on little-endian machines: The MAC bytes >>> are stored in dev->dev_addr as 012345, but in the EEPROM they're stored >>> as 543210. The first 16-bit value that you write is 10 on big-endian >>> and 01 on little-endian if I'm not mistaken. >>> >>> By only writing 8-bit values, the original author elegantly sidestepped >>> this issue. >>> >>> Maybe the simplest and most readable solution is something like: >>> >>> u8 val[2]; >>> ... >>> val[0] = dev->dev_addr[i+1]; >>> val[1] = dev->dev_addr; >>> >>> Then cast val to a u16 when passing it to ks8851_wrreg16(). >>> >>> Alternatively, use cpu_to_be16(). >> >> There is a cpu_to_be16() inside ks8851_wrreg16(). Something i already >> checked, because i wondered about endianess issues as well. > > There's a cpu_to_le16() in ks8851_wrreg16(), not a cpu_to_be16(). I have a feeling this whole thing might be more messed up then we thought. At least the KS8851-16MLL has an "endian mode" bit in the CCR register, the SPI variant does not. So what I think you need to do here is write exactly the registers 0x14/0x12/0x10 and let the accessors swap the endianness as needed.
On 3/24/20 2:09 PM, Marek Vasut wrote: > On 3/24/20 1:36 PM, Lukas Wunner wrote: >> On Tue, Mar 24, 2020 at 01:25:53PM +0100, Andrew Lunn wrote: >>> On Tue, Mar 24, 2020 at 09:13:11AM +0100, Lukas Wunner wrote: >>>> On Tue, Mar 24, 2020 at 12:42:56AM +0100, Marek Vasut wrote: >>>>> On the SPI variant of KS8851, the MAC address can be programmed with >>>>> either 8/16/32-bit writes. To make it easier to support the 16-bit >>>>> parallel option of KS8851 too, switch both the MAC address programming >>>>> and readout to 16-bit operations. >>>> [...] >>>>> static int ks8851_write_mac_addr(struct net_device *dev) >>>>> { >>>>> struct ks8851_net *ks = netdev_priv(dev); >>>>> + u16 val; >>>>> int i; >>>>> >>>>> mutex_lock(&ks->lock); >>>>> @@ -358,8 +329,12 @@ static int ks8851_write_mac_addr(struct net_device *dev) >>>>> * the first write to the MAC address does not take effect. >>>>> */ >>>>> ks8851_set_powermode(ks, PMECR_PM_NORMAL); >>>>> - for (i = 0; i < ETH_ALEN; i++) >>>>> - ks8851_wrreg8(ks, KS_MAR(i), dev->dev_addr[i]); >>>>> + >>>>> + for (i = 0; i < ETH_ALEN; i += 2) { >>>>> + val = (dev->dev_addr[i] << 8) | dev->dev_addr[i + 1]; >>>>> + ks8851_wrreg16(ks, KS_MAR(i + 1), val); >>>>> + } >>>>> + >>>> >>>> This looks like it won't work on little-endian machines: The MAC bytes >>>> are stored in dev->dev_addr as 012345, but in the EEPROM they're stored >>>> as 543210. The first 16-bit value that you write is 10 on big-endian >>>> and 01 on little-endian if I'm not mistaken. >>>> >>>> By only writing 8-bit values, the original author elegantly sidestepped >>>> this issue. >>>> >>>> Maybe the simplest and most readable solution is something like: >>>> >>>> u8 val[2]; >>>> ... >>>> val[0] = dev->dev_addr[i+1]; >>>> val[1] = dev->dev_addr; >>>> >>>> Then cast val to a u16 when passing it to ks8851_wrreg16(). >>>> >>>> Alternatively, use cpu_to_be16(). >>> >>> There is a cpu_to_be16() inside ks8851_wrreg16(). Something i already >>> checked, because i wondered about endianess issues as well. >> >> There's a cpu_to_le16() in ks8851_wrreg16(), not a cpu_to_be16(). > > I have a feeling this whole thing might be more messed up then we > thought. At least the KS8851-16MLL has an "endian mode" bit in the CCR > register, the SPI variant does not. > > So what I think you need to do here is write exactly the registers > 0x14/0x12/0x10 and let the accessors swap the endianness as needed. I'll try to get the parallel version with switched endianness, but that might take a few days. However, the SPI variant should be able to write the MAC with the code above just fine, no matter what the host endianness is, right ?
On Tue, Mar 24, 2020 at 02:09:18PM +0100, Marek Vasut wrote: > I have a feeling this whole thing might be more messed up then we > thought. At least the KS8851-16MLL has an "endian mode" bit in the CCR > register, the SPI variant does not. On the MLL variant of this chip, pin 10 can be pulled up to force it into big endian mode, otherwise it's in little-endian mode. Obviously this should be configured by the board designer such that it matches the CPU's endianness. Of course we *could* support inverted endianness in case the hardware engineer botched the board layout. Not sure if we have to. In the CCR register that you mention, you can determine whether the pin is pulled up or not. If it is in big-endian mode and you're on a little-endian CPU, you're hosed and the only option that you've got is to invert endianness in software, i.e. in the accessors. If the pin is pulled to ground or not connected (again, can be determined from CCR) then you're able to switch the endianness by setting bit 11 in the RXFDPR register. No need to convert it in the accessors in this case. Thanks, Lukas
On 3/24/20 3:47 PM, Lukas Wunner wrote: > On Tue, Mar 24, 2020 at 02:09:18PM +0100, Marek Vasut wrote: >> I have a feeling this whole thing might be more messed up then we >> thought. At least the KS8851-16MLL has an "endian mode" bit in the CCR >> register, the SPI variant does not. > > On the MLL variant of this chip, pin 10 can be pulled up to force it > into big endian mode, otherwise it's in little-endian mode. Obviously > this should be configured by the board designer such that it matches > the CPU's endianness. Sadly, that's not the case on the device I have here right now. So I'm suffering the performance impact of having to endian-swap on every 16bit access. > Of course we *could* support inverted endianness in case the hardware > engineer botched the board layout. Not sure if we have to. > > In the CCR register that you mention, you can determine whether the > pin is pulled up or not. If it is in big-endian mode and you're > on a little-endian CPU, you're hosed and the only option that you've > got is to invert endianness in software, i.e. in the accessors. Yes > If the pin is pulled to ground or not connected (again, can be > determined from CCR) then you're able to switch the endianness by > setting bit 11 in the RXFDPR register. No need to convert it in > the accessors in this case. That's not the setup I have right now, sadly.
diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c index be9478f39009..3150f1b928c0 100644 --- a/drivers/net/ethernet/micrel/ks8851.c +++ b/drivers/net/ethernet/micrel/ks8851.c @@ -185,36 +185,6 @@ static void ks8851_wrreg16(struct ks8851_net *ks, unsigned reg, unsigned val) netdev_err(ks->netdev, "spi_sync() failed\n"); } -/** - * ks8851_wrreg8 - write 8bit register value to chip - * @ks: The chip state - * @reg: The register address - * @val: The value to write - * - * Issue a write to put the value @val into the register specified in @reg. - */ -static void ks8851_wrreg8(struct ks8851_net *ks, unsigned reg, unsigned val) -{ - struct spi_transfer *xfer = &ks->spi_xfer1; - struct spi_message *msg = &ks->spi_msg1; - __le16 txb[2]; - int ret; - int bit; - - bit = 1 << (reg & 3); - - txb[0] = cpu_to_le16(MK_OP(bit, reg) | KS_SPIOP_WR); - txb[1] = val; - - xfer->tx_buf = txb; - xfer->rx_buf = NULL; - xfer->len = 3; - - ret = spi_sync(ks->spidev, msg); - if (ret < 0) - netdev_err(ks->netdev, "spi_sync() failed\n"); -} - /** * ks8851_rdreg - issue read register command and return the data * @ks: The device state @@ -349,6 +319,7 @@ static void ks8851_set_powermode(struct ks8851_net *ks, unsigned pwrmode) static int ks8851_write_mac_addr(struct net_device *dev) { struct ks8851_net *ks = netdev_priv(dev); + u16 val; int i; mutex_lock(&ks->lock); @@ -358,8 +329,12 @@ static int ks8851_write_mac_addr(struct net_device *dev) * the first write to the MAC address does not take effect. */ ks8851_set_powermode(ks, PMECR_PM_NORMAL); - for (i = 0; i < ETH_ALEN; i++) - ks8851_wrreg8(ks, KS_MAR(i), dev->dev_addr[i]); + + for (i = 0; i < ETH_ALEN; i += 2) { + val = (dev->dev_addr[i] << 8) | dev->dev_addr[i + 1]; + ks8851_wrreg16(ks, KS_MAR(i + 1), val); + } + if (!netif_running(dev)) ks8851_set_powermode(ks, PMECR_PM_SOFTDOWN); @@ -377,12 +352,16 @@ static int ks8851_write_mac_addr(struct net_device *dev) static void ks8851_read_mac_addr(struct net_device *dev) { struct ks8851_net *ks = netdev_priv(dev); + u16 reg; int i; mutex_lock(&ks->lock); - for (i = 0; i < ETH_ALEN; i++) - dev->dev_addr[i] = ks8851_rdreg8(ks, KS_MAR(i)); + for (i = 0; i < ETH_ALEN; i += 2) { + reg = ks8851_rdreg16(ks, KS_MAR(i + 1)); + dev->dev_addr[i] = reg & 0xff; + dev->dev_addr[i + 1] = reg >> 8; + } mutex_unlock(&ks->lock); }
On the SPI variant of KS8851, the MAC address can be programmed with either 8/16/32-bit writes. To make it easier to support the 16-bit parallel option of KS8851 too, switch both the MAC address programming and readout to 16-bit operations. Remove ks8851_wrreg8() as it is not used anywhere anymore. There should be no functional change. 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.c | 47 ++++++++-------------------- 1 file changed, 13 insertions(+), 34 deletions(-)