| Message ID | 20200323234303.526748-12-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:43:00AM +0100, Marek Vasut wrote: > The register and FIFO accessors are bus specific. Implement callbacks so > that each variant of the KS8851 can implement matching accessors and use > the rest of the common code. [...] > + unsigned int (*rdreg16)(struct ks8851_net *ks, > + unsigned int reg); > + void (*wrreg16)(struct ks8851_net *ks, > + unsigned int reg, unsigned int val); > + void (*rdfifo)(struct ks8851_net *ks, u8 *buff, > + unsigned int len); > + void (*wrfifo)(struct ks8851_net *ks, > + struct sk_buff *txp, bool irq); Using callbacks entails a dereference for each invocation. A cheaper approach is to just declare the function signatures in ks8851.h and provide non-static implementations in ks8851_spi.c and ks8851_mll.c, so I'd strongly prefer that. Even better, since this only concerns the register accessors (which are inlined anyway by the compiler), it would be best to have them in header files (e.g. ks8851_spi.h / ks8851_par.h) which are included by the common ks8851.c based on the target which is being compiled. That involves a bit of kbuild magic though to generate two different .o from the same .c file, each with specific "-include ..." CFLAGS. Thanks, Lukas
On 3/24/20 2:45 PM, Lukas Wunner wrote: > On Tue, Mar 24, 2020 at 12:43:00AM +0100, Marek Vasut wrote: >> The register and FIFO accessors are bus specific. Implement callbacks so >> that each variant of the KS8851 can implement matching accessors and use >> the rest of the common code. > [...] >> + unsigned int (*rdreg16)(struct ks8851_net *ks, >> + unsigned int reg); >> + void (*wrreg16)(struct ks8851_net *ks, >> + unsigned int reg, unsigned int val); >> + void (*rdfifo)(struct ks8851_net *ks, u8 *buff, >> + unsigned int len); >> + void (*wrfifo)(struct ks8851_net *ks, >> + struct sk_buff *txp, bool irq); > > Using callbacks entails a dereference for each invocation. Yes indeed, the SPI stack which you use to talk to the KS8851 SPI is also full of those. > A cheaper approach is to just declare the function signatures > in ks8851.h and provide non-static implementations in > ks8851_spi.c and ks8851_mll.c, so I'd strongly prefer that. > > Even better, since this only concerns the register accessors > (which are inlined anyway by the compiler), it would be best > to have them in header files (e.g. ks8851_spi.h / ks8851_par.h) > which are included by the common ks8851.c based on the target > which is being compiled. That involves a bit of kbuild magic > though to generate two different .o from the same .c file, > each with specific "-include ..." CFLAGS. Before we go down the complex and ugly path, can you check whether this actually has performance impact ? I would expect that since this is an SPI-connected device, this here shouldn't have that much impact. But I might be wrong, I don't have the hardware. Also note that having this dereference in place, it permits me to easily implement accessors for both LE and BE variant of the parallel bus device.
On Tue, Mar 24, 2020 at 03:10:59PM +0100, Marek Vasut wrote: > On 3/24/20 2:45 PM, Lukas Wunner wrote: > > On Tue, Mar 24, 2020 at 12:43:00AM +0100, Marek Vasut wrote: > >> The register and FIFO accessors are bus specific. Implement callbacks so > >> that each variant of the KS8851 can implement matching accessors and use > >> the rest of the common code. > > [...] > >> + unsigned int (*rdreg16)(struct ks8851_net *ks, > >> + unsigned int reg); > >> + void (*wrreg16)(struct ks8851_net *ks, > >> + unsigned int reg, unsigned int val); > >> + void (*rdfifo)(struct ks8851_net *ks, u8 *buff, > >> + unsigned int len); > >> + void (*wrfifo)(struct ks8851_net *ks, > >> + struct sk_buff *txp, bool irq); > > > > Using callbacks entails a dereference for each invocation. > > Yes indeed, the SPI stack which you use to talk to the KS8851 SPI is > also full of those. Apples and oranges. Low-level SPI drivers provide callbacks to the SPI core because it would be too expensive (space-wise) to link the SPI core into every low-level driver. Whereas in this case, you're generating two separate modules anyway, so there's no need at all to use callbacks. > > A cheaper approach is to just declare the function signatures > > in ks8851.h and provide non-static implementations in > > ks8851_spi.c and ks8851_mll.c, so I'd strongly prefer that. > > > > Even better, since this only concerns the register accessors > > (which are inlined anyway by the compiler), it would be best > > to have them in header files (e.g. ks8851_spi.h / ks8851_par.h) > > which are included by the common ks8851.c based on the target > > which is being compiled. That involves a bit of kbuild magic > > though to generate two different .o from the same .c file, > > each with specific "-include ..." CFLAGS. > > Before we go down the complex and ugly path, can you check whether this > actually has performance impact ? I would expect that since this is an > SPI-connected device, this here shouldn't have that much impact. But I > might be wrong, I don't have the hardware. I can test it, but the devices are in the office, I won't return there before Thursday. That said, I don't think it's a proper approach to make the code more expensive even though it's perfectly possible to avoid any performance impact, and shrug off concerns with the argument that the impact should be measured first. > Also note that having this dereference in place, it permits me to easily > implement accessors for both LE and BE variant of the parallel bus device. My understanding is that you're supposed to configure the chip to use the native endianness of your architecture on ->probe() such that conversions become unnecessary and the same accessor can be used for LE and BE. So why do you need two accessors? Thanks, Lukas
On 3/24/20 3:29 PM, Lukas Wunner wrote: > On Tue, Mar 24, 2020 at 03:10:59PM +0100, Marek Vasut wrote: >> On 3/24/20 2:45 PM, Lukas Wunner wrote: >>> On Tue, Mar 24, 2020 at 12:43:00AM +0100, Marek Vasut wrote: >>>> The register and FIFO accessors are bus specific. Implement callbacks so >>>> that each variant of the KS8851 can implement matching accessors and use >>>> the rest of the common code. >>> [...] >>>> + unsigned int (*rdreg16)(struct ks8851_net *ks, >>>> + unsigned int reg); >>>> + void (*wrreg16)(struct ks8851_net *ks, >>>> + unsigned int reg, unsigned int val); >>>> + void (*rdfifo)(struct ks8851_net *ks, u8 *buff, >>>> + unsigned int len); >>>> + void (*wrfifo)(struct ks8851_net *ks, >>>> + struct sk_buff *txp, bool irq); >>> >>> Using callbacks entails a dereference for each invocation. >> >> Yes indeed, the SPI stack which you use to talk to the KS8851 SPI is >> also full of those. > > Apples and oranges. Low-level SPI drivers provide callbacks to the > SPI core because it would be too expensive (space-wise) to link the > SPI core into every low-level driver. Whereas in this case, you're > generating two separate modules anyway, so there's no need at all > to use callbacks. > > >>> A cheaper approach is to just declare the function signatures >>> in ks8851.h and provide non-static implementations in >>> ks8851_spi.c and ks8851_mll.c, so I'd strongly prefer that. >>> >>> Even better, since this only concerns the register accessors >>> (which are inlined anyway by the compiler), it would be best >>> to have them in header files (e.g. ks8851_spi.h / ks8851_par.h) >>> which are included by the common ks8851.c based on the target >>> which is being compiled. That involves a bit of kbuild magic >>> though to generate two different .o from the same .c file, >>> each with specific "-include ..." CFLAGS. >> >> Before we go down the complex and ugly path, can you check whether this >> actually has performance impact ? I would expect that since this is an >> SPI-connected device, this here shouldn't have that much impact. But I >> might be wrong, I don't have the hardware. > > I can test it, but the devices are in the office, I won't return there > before Thursday. That said, I don't think it's a proper approach to > make the code more expensive even though it's perfectly possible to > avoid any performance impact, and shrug off concerns with the argument > that the impact should be measured first. I cannot measure the impact on the SPI device, but I would like to know the numbers to see whether it's worth it all, before I start creating a more complex solution. Since the SPI bus is limited to 40 MHz per datasheet, I don't think the pointer dereference is gonna introduce any performance problem. I can at least try skipping the dereference on the parallel bus option and see if it makes a difference. >> Also note that having this dereference in place, it permits me to easily >> implement accessors for both LE and BE variant of the parallel bus device. > > My understanding is that you're supposed to configure the chip to use > the native endianness of your architecture on ->probe() such that > conversions become unnecessary and the same accessor can be used for > LE and BE. So why do you need two accessors? Because I have a device here which is configured the "wrong" way thus far.
On 3/24/20 2:45 PM, Lukas Wunner wrote: > On Tue, Mar 24, 2020 at 12:43:00AM +0100, Marek Vasut wrote: >> The register and FIFO accessors are bus specific. Implement callbacks so >> that each variant of the KS8851 can implement matching accessors and use >> the rest of the common code. > [...] >> + unsigned int (*rdreg16)(struct ks8851_net *ks, >> + unsigned int reg); >> + void (*wrreg16)(struct ks8851_net *ks, >> + unsigned int reg, unsigned int val); >> + void (*rdfifo)(struct ks8851_net *ks, u8 *buff, >> + unsigned int len); >> + void (*wrfifo)(struct ks8851_net *ks, >> + struct sk_buff *txp, bool irq); > > Using callbacks entails a dereference for each invocation. > > A cheaper approach is to just declare the function signatures > in ks8851.h and provide non-static implementations in > ks8851_spi.c and ks8851_mll.c, so I'd strongly prefer that. > > Even better, since this only concerns the register accessors > (which are inlined anyway by the compiler), it would be best > to have them in header files (e.g. ks8851_spi.h / ks8851_par.h) > which are included by the common ks8851.c based on the target > which is being compiled. That involves a bit of kbuild magic > though to generate two different .o from the same .c file, > each with specific "-include ..." CFLAGS. Seems this also fails when both options are compiled in, then there is a symbol name collision. Thoughts ?
diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c index 608c8fe4b5e8..d5bdbd9122bf 100644 --- a/drivers/net/ethernet/micrel/ks8851.c +++ b/drivers/net/ethernet/micrel/ks8851.c @@ -129,6 +129,15 @@ struct ks8851_net { struct regulator *vdd_reg; struct regulator *vdd_io; int gpio; + + unsigned int (*rdreg16)(struct ks8851_net *ks, + unsigned int reg); + void (*wrreg16)(struct ks8851_net *ks, + unsigned int reg, unsigned int val); + void (*rdfifo)(struct ks8851_net *ks, u8 *buff, + unsigned int len); + void (*wrfifo)(struct ks8851_net *ks, + struct sk_buff *txp, bool irq); }; /** @@ -171,14 +180,15 @@ static int msg_enable; */ /** - * ks8851_wrreg16 - write 16bit register value to chip + * ks8851_wrreg16_spi - write 16bit register value to chip via SPI * @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_wrreg16(struct ks8851_net *ks, unsigned reg, unsigned val) +static void ks8851_wrreg16_spi(struct ks8851_net *ks, unsigned int reg, + unsigned int val) { struct ks8851_net_spi *kss = to_ks8851_spi(ks); struct spi_transfer *xfer = &kss->spi_xfer1; @@ -198,6 +208,20 @@ static void ks8851_wrreg16(struct ks8851_net *ks, unsigned reg, unsigned val) netdev_err(ks->netdev, "spi_sync() failed\n"); } +/** + * ks8851_wrreg16 - write 16bit 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_wrreg16(struct ks8851_net *ks, unsigned int reg, + unsigned int val) +{ + ks->wrreg16(ks, reg, val); +} + /** * ks8851_rdreg - issue read register command and return the data * @ks: The device state @@ -251,13 +275,14 @@ static void ks8851_rdreg(struct ks8851_net *ks, unsigned op, } /** - * ks8851_rdreg16 - read 16 bit register from device + * ks8851_rdreg16_spi - read 16 bit register from device via SPI * @ks: The chip information * @reg: The register address * * Read a 16bit register from the chip, returning the result */ -static unsigned ks8851_rdreg16(struct ks8851_net *ks, unsigned reg) +static unsigned int ks8851_rdreg16_spi(struct ks8851_net *ks, + unsigned int reg) { __le16 rx = 0; @@ -265,6 +290,19 @@ static unsigned ks8851_rdreg16(struct ks8851_net *ks, unsigned reg) return le16_to_cpu(rx); } +/** + * ks8851_rdreg16 - read 16 bit register from device + * @ks: The chip information + * @reg: The register address + * + * Read a 16bit register from the chip, returning the result + */ +static unsigned int ks8851_rdreg16(struct ks8851_net *ks, + unsigned int reg) +{ + return ks->rdreg16(ks, reg); +} + /** * ks8851_soft_reset - issue one of the soft reset to the device * @ks: The device state. @@ -402,7 +440,7 @@ static void ks8851_init_mac(struct ks8851_net *ks, struct device *ddev) } /** - * ks8851_rdfifo - read data from the receive fifo + * ks8851_rdfifo_spi - read data from the receive fifo via SPI * @ks: The device state. * @buff: The buffer address * @len: The length of the data to read @@ -410,7 +448,8 @@ static void ks8851_init_mac(struct ks8851_net *ks, struct device *ddev) * Issue an RXQ FIFO read command and read the @len amount of data from * the FIFO into the buffer specified by @buff. */ -static void ks8851_rdfifo(struct ks8851_net *ks, u8 *buff, unsigned len) +static void ks8851_rdfifo_spi(struct ks8851_net *ks, u8 *buff, + unsigned int len) { struct ks8851_net_spi *kss = to_ks8851_spi(ks); struct spi_transfer *xfer = kss->spi_xfer2; @@ -516,7 +555,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks) rxpkt = skb_put(skb, rxlen) - 8; - ks8851_rdfifo(ks, rxpkt, rxalign + 8); + ks->rdfifo(ks, rxpkt, rxalign + 8); if (netif_msg_pktdata(ks)) ks8851_dbg_dumpkkt(ks, rxpkt); @@ -645,7 +684,7 @@ static inline unsigned calc_txlen(unsigned len) } /** - * ks8851_wrpkt - write packet to TX FIFO + * ks8851_wrpkt_spi - write packet to TX FIFO via SPI * @ks: The device state. * @txp: The sk_buff to transmit. * @irq: IRQ on completion of the packet. @@ -655,7 +694,8 @@ static inline unsigned calc_txlen(unsigned len) * needs, such as IRQ on completion. Send the header and the packet data to * the device. */ -static void ks8851_wrpkt(struct ks8851_net *ks, struct sk_buff *txp, bool irq) +static void ks8851_wrpkt_spi(struct ks8851_net *ks, struct sk_buff *txp, + bool irq) { struct ks8851_net_spi *kss = to_ks8851_spi(ks); struct spi_transfer *xfer = kss->spi_xfer2; @@ -727,7 +767,7 @@ static void ks8851_tx_work(struct work_struct *work) if (txb != NULL) { ks8851_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_SDA); - ks8851_wrpkt(ks, txb, last); + ks->wrfifo(ks, txb, last); ks8851_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr); ks8851_wrreg16(ks, KS_TXQCR, TXQCR_METFE); @@ -1535,6 +1575,11 @@ static int ks8851_probe(struct spi_device *spi) spi->bits_per_word = 8; ks = netdev_priv(ndev); + ks->rdreg16 = ks8851_rdreg16_spi; + ks->wrreg16 = ks8851_wrreg16_spi; + ks->rdfifo = ks8851_rdfifo_spi; + ks->wrfifo = ks8851_wrpkt_spi; + kss = to_ks8851_spi(ks); kss->spidev = spi;
The register and FIFO accessors are bus specific. Implement callbacks so that each variant of the KS8851 can implement matching accessors and use the rest of the common code. 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 | 65 +++++++++++++++++++++++----- 1 file changed, 55 insertions(+), 10 deletions(-)