| Message ID | 20200323234303.526748-7-marex@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: ks8851: Unify KS8851 SPI and MLL drivers | expand |
> @@ -527,9 +507,8 @@ static void ks8851_rx_pkts(struct ks8851_net *ks) > */ > > for (; rxfc != 0; rxfc--) { > - rxh = ks8851_rdreg32(ks, KS_RXFHSR); > - rxstat = rxh & 0xffff; > - rxlen = (rxh >> 16) & 0xfff; > + rxstat = ks8851_rdreg16(ks, KS_RXFHSR); > + rxlen = ks8851_rdreg16(ks, KS_RXFHBCR) & RXFHBCR_CNT_MASK; Hi Marek Is there anything in the datasheet about these registers? Does reading them clear an interrupt etc? A 32bit read is i assume one SPI transaction, where as this is now two transactions, so no longer atomic. Andrew
On Tue, Mar 24, 2020 at 12:42:55AM +0100, Marek Vasut wrote: > The ks8851_rdreg32() is used only in one place, to read two registers > using a single read. To make it easier to support 16-bit accesses via > parallel bus later on, replace this single read with two 16-bit reads > from each of the registers and drop the ks8851_rdreg32() altogether. This doubles the SPI transactions necessary to read the RX queue status, which happens for each received packet, so I expect the performance impact to be noticeable. Can you keep the 32-bit variant for SPI and instead just introduce a 32-bit read for the MLL chip which performs two 16-bit reads internally? Thanks, Lukas
On 3/24/20 2:30 AM, Andrew Lunn wrote: >> @@ -527,9 +507,8 @@ static void ks8851_rx_pkts(struct ks8851_net *ks) >> */ >> >> for (; rxfc != 0; rxfc--) { >> - rxh = ks8851_rdreg32(ks, KS_RXFHSR); >> - rxstat = rxh & 0xffff; >> - rxlen = (rxh >> 16) & 0xfff; >> + rxstat = ks8851_rdreg16(ks, KS_RXFHSR); >> + rxlen = ks8851_rdreg16(ks, KS_RXFHBCR) & RXFHBCR_CNT_MASK; > > Hi Marek > > Is there anything in the datasheet about these registers? Does reading > them clear an interrupt etc? A 32bit read is i assume one SPI > transaction, where as this is now two transactions, so no longer > atomic. Nope, they're just two registers holding packet metadata. There are separate interrupt registers and separate register to clear the packet from the RX FIFO, so reading these two registers does not have to be atomic.
On 3/24/20 8:22 AM, Lukas Wunner wrote: > On Tue, Mar 24, 2020 at 12:42:55AM +0100, Marek Vasut wrote: >> The ks8851_rdreg32() is used only in one place, to read two registers >> using a single read. To make it easier to support 16-bit accesses via >> parallel bus later on, replace this single read with two 16-bit reads >> from each of the registers and drop the ks8851_rdreg32() altogether. > > This doubles the SPI transactions necessary to read the RX queue status, > which happens for each received packet, so I expect the performance > impact to be noticeable. Can you keep the 32-bit variant for SPI > and instead just introduce a 32-bit read for the MLL chip which performs > two 16-bit reads internally? Please test it before I'll be forced to rework the harder half of this patchset. I don't have the SPI variant of the chip to collect those statistics. But the real fix here would be to convert the driver to regmap in the end and permit regmap to merge neighboring register accesses over SPI.
diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c index 1c0a0364b047..be9478f39009 100644 --- a/drivers/net/ethernet/micrel/ks8851.c +++ b/drivers/net/ethernet/micrel/ks8851.c @@ -296,25 +296,6 @@ static unsigned ks8851_rdreg16(struct ks8851_net *ks, unsigned reg) return le16_to_cpu(rx); } -/** - * ks8851_rdreg32 - read 32 bit register from device - * @ks: The chip information - * @reg: The register address - * - * Read a 32bit register from the chip. - * - * Note, this read requires the address be aligned to 4 bytes. -*/ -static unsigned ks8851_rdreg32(struct ks8851_net *ks, unsigned reg) -{ - __le32 rx = 0; - - WARN_ON(reg & 3); - - ks8851_rdreg(ks, MK_OP(0xf, reg), (u8 *)&rx, 4); - return le32_to_cpu(rx); -} - /** * ks8851_soft_reset - issue one of the soft reset to the device * @ks: The device state. @@ -508,7 +489,6 @@ static void ks8851_rx_pkts(struct ks8851_net *ks) unsigned rxfc; unsigned rxlen; unsigned rxstat; - u32 rxh; u8 *rxpkt; rxfc = ks8851_rdreg8(ks, KS_RXFC); @@ -527,9 +507,8 @@ static void ks8851_rx_pkts(struct ks8851_net *ks) */ for (; rxfc != 0; rxfc--) { - rxh = ks8851_rdreg32(ks, KS_RXFHSR); - rxstat = rxh & 0xffff; - rxlen = (rxh >> 16) & 0xfff; + rxstat = ks8851_rdreg16(ks, KS_RXFHSR); + rxlen = ks8851_rdreg16(ks, KS_RXFHBCR) & RXFHBCR_CNT_MASK; netif_dbg(ks, rx_status, ks->netdev, "rx: stat 0x%04x, len 0x%04x\n", rxstat, rxlen);
The ks8851_rdreg32() is used only in one place, to read two registers using a single read. To make it easier to support 16-bit accesses via parallel bus later on, replace this single read with two 16-bit reads from each of the registers and drop the ks8851_rdreg32() altogether. If this has noticeable performance impact on the SPI variant of KS8851, then we should consider using regmap to abstract the SPI and parallel bus options and in case of SPI, permit regmap to merge register reads of neighboring registers into single, longer, read. 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 | 25 ++----------------------- 1 file changed, 2 insertions(+), 23 deletions(-)