| Message ID | 20200325150543.78569-9-marex@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: ks8851: Unify KS8851 SPI and MLL drivers | expand |
On Wed, Mar 25, 2020 at 04:05:37PM +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> > --- > V2: Get rid of the KS_MAR(i + 1) by adjusting KS_MAR(x) macro > --- [...] > @@ -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), 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)); > + dev->dev_addr[i] = reg & 0xff; > + dev->dev_addr[i + 1] = reg >> 8; > + } > > mutex_unlock(&ks->lock); > } It seems my question from v1 went unnoticed and the inconsistency still seems to be there so let me ask again: when writing, you put addr[i] into upper part of the 16-bit value and addr[i+1] into lower but when reading, you do the opposite. Is it correct? Michal Kubecek
On 3/25/20 5:56 PM, Michal Kubecek wrote: > On Wed, Mar 25, 2020 at 04:05:37PM +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> >> --- >> V2: Get rid of the KS_MAR(i + 1) by adjusting KS_MAR(x) macro >> --- > [...] >> @@ -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), 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)); >> + dev->dev_addr[i] = reg & 0xff; >> + dev->dev_addr[i + 1] = reg >> 8; >> + } >> >> mutex_unlock(&ks->lock); >> } > > It seems my question from v1 went unnoticed and the inconsistency still > seems to be there so let me ask again: when writing, you put addr[i] > into upper part of the 16-bit value and addr[i+1] into lower but when > reading, you do the opposite. Is it correct? I believe so, and it works at least on the hardware I have here. I need to wait for Lukas to verify that on KS8851 SPI edition tomorrow (that's also why I sent out the V2, so he can test it out)
On Wed, Mar 25, 2020 at 06:05:30PM +0100, Marek Vasut wrote: > On 3/25/20 5:56 PM, Michal Kubecek wrote: > > On Wed, Mar 25, 2020 at 04:05:37PM +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> > >> --- > >> V2: Get rid of the KS_MAR(i + 1) by adjusting KS_MAR(x) macro > >> --- > > [...] > >> @@ -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), 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)); > >> + dev->dev_addr[i] = reg & 0xff; > >> + dev->dev_addr[i + 1] = reg >> 8; > >> + } > >> > >> mutex_unlock(&ks->lock); > >> } > > > > It seems my question from v1 went unnoticed and the inconsistency still > > seems to be there so let me ask again: when writing, you put addr[i] > > into upper part of the 16-bit value and addr[i+1] into lower but when > > reading, you do the opposite. Is it correct? > > I believe so, and it works at least on the hardware I have here. > I need to wait for Lukas to verify that on KS8851 SPI edition tomorrow > (that's also why I sent out the V2, so he can test it out) That's a bit surprising (and counterintuitive) as it means that if you do ks8851_wrreg16(ks, a, val); val = ks8851_rdreg16(ks, a); you read a different value than you wrote. But I know nothing about the hardware (I only noticed the strange inconsistency) so I can't say where does it come from. Michal Kubecek
On 3/25/20 6:30 PM, Michal Kubecek wrote: > On Wed, Mar 25, 2020 at 06:05:30PM +0100, Marek Vasut wrote: >> On 3/25/20 5:56 PM, Michal Kubecek wrote: >>> On Wed, Mar 25, 2020 at 04:05:37PM +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> >>>> --- >>>> V2: Get rid of the KS_MAR(i + 1) by adjusting KS_MAR(x) macro >>>> --- >>> [...] >>>> @@ -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), 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)); >>>> + dev->dev_addr[i] = reg & 0xff; >>>> + dev->dev_addr[i + 1] = reg >> 8; >>>> + } >>>> >>>> mutex_unlock(&ks->lock); >>>> } >>> >>> It seems my question from v1 went unnoticed and the inconsistency still >>> seems to be there so let me ask again: when writing, you put addr[i] >>> into upper part of the 16-bit value and addr[i+1] into lower but when >>> reading, you do the opposite. Is it correct? >> >> I believe so, and it works at least on the hardware I have here. >> I need to wait for Lukas to verify that on KS8851 SPI edition tomorrow >> (that's also why I sent out the V2, so he can test it out) > > That's a bit surprising (and counterintuitive) as it means that if you do > > ks8851_wrreg16(ks, a, val); > val = ks8851_rdreg16(ks, a); > > you read a different value than you wrote. But I know nothing about the > hardware (I only noticed the strange inconsistency) so I can't say where > does it come from. So this really does need fixing.
diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c index 8f1cc05dc3c8..6f3ecf980514 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), 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)); + dev->dev_addr[i] = reg & 0xff; + dev->dev_addr[i + 1] = reg >> 8; + } mutex_unlock(&ks->lock); } diff --git a/drivers/net/ethernet/micrel/ks8851.h b/drivers/net/ethernet/micrel/ks8851.h index 8f834aef8e32..f210d18a10b5 100644 --- a/drivers/net/ethernet/micrel/ks8851.h +++ b/drivers/net/ethernet/micrel/ks8851.h @@ -19,7 +19,7 @@ #define CCR_32PIN (1 << 0) /* KSZ8851SNL */ /* MAC address registers */ -#define KS_MAR(_m) (0x15 - (_m)) +#define KS_MAR(_m) (0x14 - (_m)) #define KS_MARL 0x10 #define KS_MARM 0x12 #define KS_MARH 0x14
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> --- V2: Get rid of the KS_MAR(i + 1) by adjusting KS_MAR(x) macro --- drivers/net/ethernet/micrel/ks8851.c | 47 ++++++++-------------------- drivers/net/ethernet/micrel/ks8851.h | 2 +- 2 files changed, 14 insertions(+), 35 deletions(-)