| Message ID | 20200323234303.526748-9-marex@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: ks8851: Unify KS8851 SPI and MLL drivers | expand |
> @@ -470,7 +455,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks) > unsigned rxstat; > u8 *rxpkt; > > - rxfc = ks8851_rdreg8(ks, KS_RXFC); > + rxfc = (ks8851_rdreg16(ks, KS_RXFCTR) >> 8) & 0xff; The datasheet says: 2. When software driver reads back Receive Frame Count (RXFCTR) Register; the KSZ8851 will update both Receive Frame Header Status and Byte Count Registers (RXFHSR/RXFHBCR) Are you sure there is no side affect here? Andrew
On Tue, Mar 24, 2020 at 12:42:57AM +0100, Marek Vasut wrote: > The RXFC register is the only one being read using 8-bit accessors. > To make it easier to support the 16-bit accesses used by the parallel > bus variant of KS8851, use 16-bit accessor to read RXFC register as > well as neighboring RXFCTR register. This means that an additional 8 bits need to be transferred over the SPI bus whenever a set of packets is read from the RX queue. This should be avoided. I'd suggest adding a separate hook to read RXFC and thus keep the 8-bit read function for the SPI variant. Thanks, Lukas
On 3/24/20 11:41 AM, Lukas Wunner wrote: > On Tue, Mar 24, 2020 at 12:42:57AM +0100, Marek Vasut wrote: >> The RXFC register is the only one being read using 8-bit accessors. >> To make it easier to support the 16-bit accesses used by the parallel >> bus variant of KS8851, use 16-bit accessor to read RXFC register as >> well as neighboring RXFCTR register. > > This means that an additional 8 bits need to be transferred over the > SPI bus whenever a set of packets is read from the RX queue. This > should be avoided. I'd suggest adding a separate hook to read RXFC > and thus keep the 8-bit read function for the SPI variant. See my comment about the 32bit read and regmap. It is slightly less efficient, but it also makes the conversion much easier. Can you check on the real hardware whether the is measurable performance impact ?
On 3/24/20 2:50 AM, Andrew Lunn wrote: >> @@ -470,7 +455,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks) >> unsigned rxstat; >> u8 *rxpkt; >> >> - rxfc = ks8851_rdreg8(ks, KS_RXFC); >> + rxfc = (ks8851_rdreg16(ks, KS_RXFCTR) >> 8) & 0xff; > > The datasheet says: > > 2. When software driver reads back Receive Frame Count (RXFCTR) > Register; the KSZ8851 will update both Receive Frame Header Status and > Byte Count Registers (RXFHSR/RXFHBCR) > > Are you sure there is no side affect here? Yes, look at the RXFC register 0x9c itself. It's a 16bit register, 0x9c is the LSByte and 0x9d is the MSByte. What happened here before was readout of register 0x9d, MSByte of RXFC, which triggers the update of RXFHSR/RXFHBCR. What happens now is the readout of the whole RXFC as 16bit value, which also triggers the update.
On Tue, Mar 24, 2020 at 01:50:53PM +0100, Marek Vasut wrote: > On 3/24/20 2:50 AM, Andrew Lunn wrote: > >> @@ -470,7 +455,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks) > >> unsigned rxstat; > >> u8 *rxpkt; > >> > >> - rxfc = ks8851_rdreg8(ks, KS_RXFC); > >> + rxfc = (ks8851_rdreg16(ks, KS_RXFCTR) >> 8) & 0xff; > > > > The datasheet says: > > > > 2. When software driver reads back Receive Frame Count (RXFCTR) > > Register; the KSZ8851 will update both Receive Frame Header Status and > > Byte Count Registers (RXFHSR/RXFHBCR) > > > > Are you sure there is no side affect here? > > Yes, look at the RXFC register 0x9c itself. It's a 16bit register, 0x9c > is the LSByte and 0x9d is the MSByte. > > What happened here before was readout of register 0x9d, MSByte of RXFC, > which triggers the update of RXFHSR/RXFHBCR. What happens now is the > readout of the whole RXFC as 16bit value, which also triggers the update. Hi Marek It would be nice to indicate in the commit message that things like this have been considered. As a reviewer, these are the sort of questions which goes through my mind. If there is a comment it has been considered, i get the answer to my questions without having to ask. Andrew
diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c index 3150f1b928c0..03d349208794 100644 --- a/drivers/net/ethernet/micrel/ks8851.c +++ b/drivers/net/ethernet/micrel/ks8851.c @@ -236,21 +236,6 @@ static void ks8851_rdreg(struct ks8851_net *ks, unsigned op, memcpy(rxb, trx + 2, rxl); } -/** - * ks8851_rdreg8 - read 8 bit register from device - * @ks: The chip information - * @reg: The register address - * - * Read a 8bit register from the chip, returning the result -*/ -static unsigned ks8851_rdreg8(struct ks8851_net *ks, unsigned reg) -{ - u8 rxb[1]; - - ks8851_rdreg(ks, MK_OP(1 << (reg & 3), reg), rxb, 1); - return rxb[0]; -} - /** * ks8851_rdreg16 - read 16 bit register from device * @ks: The chip information @@ -470,7 +455,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks) unsigned rxstat; u8 *rxpkt; - rxfc = ks8851_rdreg8(ks, KS_RXFC); + rxfc = (ks8851_rdreg16(ks, KS_RXFCTR) >> 8) & 0xff; netif_dbg(ks, rx_status, ks->netdev, "%s: %d packets\n", __func__, rxfc);
The RXFC register is the only one being read using 8-bit accessors. To make it easier to support the 16-bit accesses used by the parallel bus variant of KS8851, use 16-bit accessor to read RXFC register as well as neighboring RXFCTR register. Remove ks8851_rdreg8() 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 | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-)