| Message ID | 20181220010647.4059-5-marex@denx.de |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: microchip: Convert to regmap | expand |
On 12/19/18 5:06 PM, Marek Vasut wrote: > Previous patches unconver that ksz_spi_write() is always ever called > with len = 1, 2 or 4. We can thus drop the if (len > SPI_TX_BUF_LEN) > check and we can also drop the allocation of the txbuf which is part > of the driver data. This wastes 256 bytes for no reason and can be > replaced with 8-byte stack allocated buffer, which is what this patch > does. This is an intermediate step though, which will go away after > regmap conversion. dev is a kmalloc'd buffer, so dev->txbuf is a DMA-able buffer, that could be presumably why it was used in the first place, DMA from the stack is not something safe, but I did not check the core SPI layer or the SPI bus master driver to see whether they do take care of that already.
On Wed, Dec 19, 2018 at 05:20:33PM -0800, Florian Fainelli wrote: > On 12/19/18 5:06 PM, Marek Vasut wrote: > > Previous patches unconver that ksz_spi_write() is always ever called > > with len = 1, 2 or 4. We can thus drop the if (len > SPI_TX_BUF_LEN) > > check and we can also drop the allocation of the txbuf which is part > > of the driver data. This wastes 256 bytes for no reason and can be > > replaced with 8-byte stack allocated buffer, which is what this patch > > does. This is an intermediate step though, which will go away after > > regmap conversion. > > dev is a kmalloc'd buffer, so dev->txbuf is a DMA-able buffer, that > could be presumably why it was used in the first place, DMA from the > stack is not something safe, but I did not check the core SPI layer or > the SPI bus master driver to see whether they do take care of that already. Hi Marek This is also something i requested for the i2c layering. An i2c bus master might use DMA. So you cannot use stack space, or the i2c core needs to copy the data to somewhere which is DMA'able. Please could you check what regmap offers. If regmap does the copies, than this buffer can be removed, but only after the change to regmap is performed. You should not remove it before swapping to regmap, otherwise you can introducing a regression, which somebody might hit during a git bisect. Thanks Andrew
On 12/20/2018 10:41 AM, Andrew Lunn wrote: > On Wed, Dec 19, 2018 at 05:20:33PM -0800, Florian Fainelli wrote: >> On 12/19/18 5:06 PM, Marek Vasut wrote: >>> Previous patches unconver that ksz_spi_write() is always ever called >>> with len = 1, 2 or 4. We can thus drop the if (len > SPI_TX_BUF_LEN) >>> check and we can also drop the allocation of the txbuf which is part >>> of the driver data. This wastes 256 bytes for no reason and can be >>> replaced with 8-byte stack allocated buffer, which is what this patch >>> does. This is an intermediate step though, which will go away after >>> regmap conversion. >> >> dev is a kmalloc'd buffer, so dev->txbuf is a DMA-able buffer, that >> could be presumably why it was used in the first place, DMA from the >> stack is not something safe, but I did not check the core SPI layer or >> the SPI bus master driver to see whether they do take care of that already. > > Hi Marek > > This is also something i requested for the i2c layering. An i2c bus > master might use DMA. So you cannot use stack space, or the i2c core > needs to copy the data to somewhere which is DMA'able. > > Please could you check what regmap offers. If regmap does the copies, > than this buffer can be removed, but only after the change to regmap > is performed. You should not remove it before swapping to regmap, > otherwise you can introducing a regression, which somebody might hit > during a git bisect. This code goes away with the regmap completely, so I'll just reorder the patches and remove it in 6/7 . The 6/7 will need rework anyway, I talked to Mark Brown about this crazy register layout of this switch and it seems we'll need a regmap per register width (sigh).
On 12/20/2018 02:20 AM, Florian Fainelli wrote: > On 12/19/18 5:06 PM, Marek Vasut wrote: >> Previous patches unconver that ksz_spi_write() is always ever called >> with len = 1, 2 or 4. We can thus drop the if (len > SPI_TX_BUF_LEN) >> check and we can also drop the allocation of the txbuf which is part >> of the driver data. This wastes 256 bytes for no reason and can be >> replaced with 8-byte stack allocated buffer, which is what this patch >> does. This is an intermediate step though, which will go away after >> regmap conversion. > > dev is a kmalloc'd buffer, so dev->txbuf is a DMA-able buffer, that > could be presumably why it was used in the first place, DMA from the > stack is not something safe, but I did not check the core SPI layer or > the SPI bus master driver to see whether they do take care of that already. Well, I can just squash this and 6/7 and let regmap handle these details.
> On 12/20/2018 10:41 AM, Andrew Lunn wrote: > > On Wed, Dec 19, 2018 at 05:20:33PM -0800, Florian Fainelli wrote: > >> On 12/19/18 5:06 PM, Marek Vasut wrote: > >>> Previous patches unconver that ksz_spi_write() is always ever called > >>> with len = 1, 2 or 4. We can thus drop the if (len > SPI_TX_BUF_LEN) > >>> check and we can also drop the allocation of the txbuf which is part > >>> of the driver data. This wastes 256 bytes for no reason and can be > >>> replaced with 8-byte stack allocated buffer, which is what this patch > >>> does. This is an intermediate step though, which will go away after > >>> regmap conversion. The switch uses auto address increment and can return the whole register space in 1 transfer. Most switches have only 256 registers, so I just stopped at that number for KSZ9477. This read operation is mostly done by users to dump the registers for debugging. The write operation can be used to setup registers from a file. I mentioned before the driver can use the standard Linux register access API for users to read the chip. Maybe you also want to try that for debugging.
On 12/21/2018 02:02 AM, Tristram.Ha@microchip.com wrote: >> On 12/20/2018 10:41 AM, Andrew Lunn wrote: >>> On Wed, Dec 19, 2018 at 05:20:33PM -0800, Florian Fainelli wrote: >>>> On 12/19/18 5:06 PM, Marek Vasut wrote: >>>>> Previous patches unconver that ksz_spi_write() is always ever called >>>>> with len = 1, 2 or 4. We can thus drop the if (len > SPI_TX_BUF_LEN) >>>>> check and we can also drop the allocation of the txbuf which is part >>>>> of the driver data. This wastes 256 bytes for no reason and can be >>>>> replaced with 8-byte stack allocated buffer, which is what this patch >>>>> does. This is an intermediate step though, which will go away after >>>>> regmap conversion. > > The switch uses auto address increment and can return the whole register > space in 1 transfer. Most switches have only 256 registers, so I just stopped > at that number for KSZ9477. > > This read operation is mostly done by users to dump the registers for debugging. > The write operation can be used to setup registers from a file. > > I mentioned before the driver can use the standard Linux register access API for > users to read the chip. Maybe you also want to try that for debugging. The bulk IO is never used in the driver, ever, so that's not really a concern. Although, you can use the regmap to read (and write, if you enable it) registers via debugfs.
On Fri, Dec 21, 2018 at 01:02:23AM +0000, Tristram.Ha@microchip.com wrote: > > On 12/20/2018 10:41 AM, Andrew Lunn wrote: > > > On Wed, Dec 19, 2018 at 05:20:33PM -0800, Florian Fainelli wrote: > > >> On 12/19/18 5:06 PM, Marek Vasut wrote: > > >>> Previous patches unconver that ksz_spi_write() is always ever called > > >>> with len = 1, 2 or 4. We can thus drop the if (len > SPI_TX_BUF_LEN) > > >>> check and we can also drop the allocation of the txbuf which is part > > >>> of the driver data. This wastes 256 bytes for no reason and can be > > >>> replaced with 8-byte stack allocated buffer, which is what this patch > > >>> does. This is an intermediate step though, which will go away after > > >>> regmap conversion. > > The switch uses auto address increment and can return the whole register > space in 1 transfer. Most switches have only 256 registers, so I just stopped > at that number for KSZ9477. > > This read operation is mostly done by users to dump the registers for debugging. > The write operation can be used to setup registers from a file. > > I mentioned before the driver can use the standard Linux register access API for > users to read the chip. Maybe you also want to try that for debugging. Hi Tristram You might want to implement the register dump per port, and connect it to ethtool --register-dump. The core DSA code has what is needed, you just need driver specific code. Vivien also sent patches for ethtool to pritty print the values for the mv88e6xxx. You can do the same for your chipsets. Andrew
diff --git a/drivers/net/dsa/microchip/ksz9477_spi.c b/drivers/net/dsa/microchip/ksz9477_spi.c index 9ca150a472ea..69baf9677def 100644 --- a/drivers/net/dsa/microchip/ksz9477_spi.c +++ b/drivers/net/dsa/microchip/ksz9477_spi.c @@ -65,11 +65,11 @@ static int ksz_spi_write(struct ksz_device *dev, u32 reg, void *data, unsigned int len) { struct spi_device *spi = dev->priv; + u8 txbuf[8]; - if (len > SPI_TX_BUF_LEN) - len = SPI_TX_BUF_LEN; - memcpy(&dev->txbuf[4], data, len); - return ksz9477_spi_write_reg(spi, reg, dev->txbuf, len); + memcpy(txbuf + 4, data, len); + + return ksz9477_spi_write_reg(spi, reg, txbuf, len); } static int ksz_spi_read8(struct ksz_device *dev, u32 reg, u8 *val) @@ -135,8 +135,6 @@ static int ksz9477_spi_probe(struct spi_device *spi) if (spi->dev.platform_data) dev->pdata = spi->dev.platform_data; - dev->txbuf = devm_kzalloc(dev->dev, 4 + SPI_TX_BUF_LEN, GFP_KERNEL); - ret = ksz9477_switch_register(dev); /* Main DSA driver may not be started yet. */ diff --git a/drivers/net/dsa/microchip/ksz_priv.h b/drivers/net/dsa/microchip/ksz_priv.h index c3a272505af1..3ab14ee0e36b 100644 --- a/drivers/net/dsa/microchip/ksz_priv.h +++ b/drivers/net/dsa/microchip/ksz_priv.h @@ -81,8 +81,6 @@ struct ksz_device { u64 mib_value[TOTAL_SWITCH_COUNTER_NUM]; - u8 *txbuf; - struct ksz_port *ports; struct timer_list mib_read_timer; struct work_struct mib_read;
Previous patches unconver that ksz_spi_write() is always ever called with len = 1, 2 or 4. We can thus drop the if (len > SPI_TX_BUF_LEN) check and we can also drop the allocation of the txbuf which is part of the driver data. This wastes 256 bytes for no reason and can be replaced with 8-byte stack allocated buffer, which is what this patch does. This is an intermediate step though, which will go away after regmap conversion. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: Tristram Ha <Tristram.Ha@microchip.com> Cc: Woojung Huh <Woojung.Huh@microchip.com> --- drivers/net/dsa/microchip/ksz9477_spi.c | 10 ++++------ drivers/net/dsa/microchip/ksz_priv.h | 2 -- 2 files changed, 4 insertions(+), 8 deletions(-)