| Message ID | 20230506232616.1792109-3-bigunclemax@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Allwinner R329/D1/R528/T113s SPI support | expand |
From: Maksim Kiselev > Sent: 07 May 2023 00:26 > > As we're adding more properties to the OF match data, convert it to a > struct now. > ... > - sspi->fifo_depth = (unsigned long)of_device_get_match_data(&pdev->dev); > + sspi->cfg = of_device_get_match_data(&pdev->dev); Is it worth doing a structure copy at this point? The 'cfg' data is short and constant and it would make the code that uses it smaller and faster. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi, David > Is it worth doing a structure copy at this point? > The 'cfg' data is short and constant and it would make > the code that uses it smaller and faster. I'm sorry but I don't fully understand what you are suggesting. In patch 3\5 we extend the sun6i_spi_cfg structure with the has_clk_ctl field. пн, 8 мая 2023 г. в 12:47, David Laight <David.Laight@aculab.com>: > > From: Maksim Kiselev > > Sent: 07 May 2023 00:26 > > > > As we're adding more properties to the OF match data, convert it to a > > struct now. > > > ... > > - sspi->fifo_depth = (unsigned long)of_device_get_match_data(&pdev->dev); > > + sspi->cfg = of_device_get_match_data(&pdev->dev); > > Is it worth doing a structure copy at this point? > The 'cfg' data is short and constant and it would make > the code that uses it smaller and faster. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) >
From: Maxim Kiselev > Sent: 10 May 2023 09:34 > > Hi, David > > > Is it worth doing a structure copy at this point? > > The 'cfg' data is short and constant and it would make > > the code that uses it smaller and faster. > > I'm sorry but I don't fully understand what you are suggesting. > In patch 3\5 we extend the sun6i_spi_cfg structure with the has_clk_ctl field. You are still only adding a second integer. I'm suggesting that instead of sspi->cfg being a pointer to the config data it is a copy of the config data. So the assignment below becomes (ignoring error checks) memcpy(&sspi->cfg, of_device_get_match_data(&pdev->dev), sizeof sspi->cfg); and the code that needs the values is: sspi->cfg.fifo_depth (etc) David > > пн, 8 мая 2023 г. в 12:47, David Laight <David.Laight@aculab.com>: > > > > From: Maksim Kiselev > > > Sent: 07 May 2023 00:26 > > > > > > As we're adding more properties to the OF match data, convert it to a > > > struct now. > > > > > ... > > > - sspi->fifo_depth = (unsigned long)of_device_get_match_data(&pdev->dev); > > > + sspi->cfg = of_device_get_match_data(&pdev->dev); > > > > Is it worth doing a structure copy at this point? > > The 'cfg' data is short and constant and it would make > > the code that uses it smaller and faster. > > > > David > > > > - > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > > Registration No: 1397386 (Wales) > > - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, 10 May 2023 08:55:27 +0000 David Laight <David.Laight@ACULAB.COM> wrote: Hi David, > From: Maxim Kiselev > > Sent: 10 May 2023 09:34 > > > > Hi, David > > > > > Is it worth doing a structure copy at this point? > > > The 'cfg' data is short and constant and it would make Sorry, I don't really get the reason for this. Since the data is constant, wouldn't it make much more sense to keep it there in the const section, which we need anyway? What would a second copy bring us? > > > the code that uses it smaller and faster. Smaller: Do you mean the generated code? Not sure that really matters, but your sketch below hints that the C code would get larger, more error prone (you mention yourself that you skipped over the error checking) and most importantly harder to read. Faster: Do you have numbers that back that up, or does that solve a particular problem of yours? This is programming a SPI controller transfer, which runs in the vicinity of a few Mbits/s. I doubt that saving a few memory accesses (once per transfer, not per word) matters even the slightest? The actual MMIO access to program the controller registers already takes a few dozen to a few hundred cycles, so I doubt that doing a struct copy saves us anything here, in practice. Besides: Copying the pointer is the most common pattern in the kernel, I believe. I just sampled 21 SPI drivers in the tree, 17 out of them do this. The other either copy the members of the struct into the driver data (which would be an option for us, too), or immediately consume the data in the probe() routine. If you have some good reason to optimise this, please send a patch (on top). Cheers, Andre. > > > > I'm sorry but I don't fully understand what you are suggesting. > > In patch 3\5 we extend the sun6i_spi_cfg structure with the has_clk_ctl field. > > You are still only adding a second integer. > > I'm suggesting that instead of sspi->cfg being a pointer to the > config data it is a copy of the config data. > So the assignment below becomes (ignoring error checks) > memcpy(&sspi->cfg, of_device_get_match_data(&pdev->dev), sizeof sspi->cfg); > and the code that needs the values is: > sspi->cfg.fifo_depth > (etc) > > David > > > > > пн, 8 мая 2023 г. в 12:47, David Laight <David.Laight@aculab.com>: > > > > > > From: Maksim Kiselev > > > > Sent: 07 May 2023 00:26 > > > > > > > > As we're adding more properties to the OF match data, convert it to a > > > > struct now. > > > > > > > ... > > > > - sspi->fifo_depth = (unsigned long)of_device_get_match_data(&pdev->dev); > > > > + sspi->cfg = of_device_get_match_data(&pdev->dev); > > > > > > Is it worth doing a structure copy at this point? > > > The 'cfg' data is short and constant and it would make > > > the code that uses it smaller and faster. > > > > > > David > > > > > > - > > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > > > Registration No: 1397386 (Wales) > > > > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c index 7532c85a352c..01a01cd86db5 100644 --- a/drivers/spi/spi-sun6i.c +++ b/drivers/spi/spi-sun6i.c @@ -85,6 +85,10 @@ #define SUN6I_TXDATA_REG 0x200 #define SUN6I_RXDATA_REG 0x300 +struct sun6i_spi_cfg { + unsigned long fifo_depth; +}; + struct sun6i_spi { struct spi_master *master; void __iomem *base_addr; @@ -99,7 +103,7 @@ struct sun6i_spi { const u8 *tx_buf; u8 *rx_buf; int len; - unsigned long fifo_depth; + const struct sun6i_spi_cfg *cfg; }; static inline u32 sun6i_spi_read(struct sun6i_spi *sspi, u32 reg) @@ -156,7 +160,7 @@ static inline void sun6i_spi_fill_fifo(struct sun6i_spi *sspi) u8 byte; /* See how much data we can fit */ - cnt = sspi->fifo_depth - sun6i_spi_get_tx_fifo_count(sspi); + cnt = sspi->cfg->fifo_depth - sun6i_spi_get_tx_fifo_count(sspi); len = min((int)cnt, sspi->len); @@ -289,14 +293,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master, * the hardcoded value used in old generation of Allwinner * SPI controller. (See spi-sun4i.c) */ - trig_level = sspi->fifo_depth / 4 * 3; + trig_level = sspi->cfg->fifo_depth / 4 * 3; } else { /* * Setup FIFO DMA request trigger level * We choose 1/2 of the full fifo depth, that value will * be used as DMA burst length. */ - trig_level = sspi->fifo_depth / 2; + trig_level = sspi->cfg->fifo_depth / 2; if (tfr->tx_buf) reg |= SUN6I_FIFO_CTL_TF_DRQ_EN; @@ -410,9 +414,9 @@ static int sun6i_spi_transfer_one(struct spi_master *master, reg = SUN6I_INT_CTL_TC; if (!use_dma) { - if (rx_len > sspi->fifo_depth) + if (rx_len > sspi->cfg->fifo_depth) reg |= SUN6I_INT_CTL_RF_RDY; - if (tx_len > sspi->fifo_depth) + if (tx_len > sspi->cfg->fifo_depth) reg |= SUN6I_INT_CTL_TF_ERQ; } @@ -543,7 +547,7 @@ static bool sun6i_spi_can_dma(struct spi_master *master, * the fifo length we can just fill the fifo and wait for a single * irq, so don't bother setting up dma */ - return xfer->len > sspi->fifo_depth; + return xfer->len > sspi->cfg->fifo_depth; } static int sun6i_spi_probe(struct platform_device *pdev) @@ -582,7 +586,7 @@ static int sun6i_spi_probe(struct platform_device *pdev) } sspi->master = master; - sspi->fifo_depth = (unsigned long)of_device_get_match_data(&pdev->dev); + sspi->cfg = of_device_get_match_data(&pdev->dev); master->max_speed_hz = 100 * 1000 * 1000; master->min_speed_hz = 3 * 1000; @@ -695,9 +699,17 @@ static void sun6i_spi_remove(struct platform_device *pdev) dma_release_channel(master->dma_rx); } +static const struct sun6i_spi_cfg sun6i_a31_spi_cfg = { + .fifo_depth = SUN6I_FIFO_DEPTH, +}; + +static const struct sun6i_spi_cfg sun8i_h3_spi_cfg = { + .fifo_depth = SUN8I_FIFO_DEPTH, +}; + static const struct of_device_id sun6i_spi_match[] = { - { .compatible = "allwinner,sun6i-a31-spi", .data = (void *)SUN6I_FIFO_DEPTH }, - { .compatible = "allwinner,sun8i-h3-spi", .data = (void *)SUN8I_FIFO_DEPTH }, + { .compatible = "allwinner,sun6i-a31-spi", .data = &sun6i_a31_spi_cfg }, + { .compatible = "allwinner,sun8i-h3-spi", .data = &sun8i_h3_spi_cfg }, {} }; MODULE_DEVICE_TABLE(of, sun6i_spi_match);