[PATCH v1 mfd] mfd: ocelot-spi: fix bulk read

[PATCH v1 mfd] mfd: ocelot-spi: fix bulk read

[Colin Foster]

Ocelot chips (VSC7511, VSC7512, VSC7513, VSC7514) don't support bulk read
operations over SPI.

Many SPI buses have hardware that can optimize consecutive reads.
Essentially an address is written to the chip, and if the SPI controller
continues to toggle the clock, subsequent register values are reported.
This can lead to significant optimizations, because the time between
"address is written to the chip" and "chip starts to report data" can often
take a fixed amount of time.

When support for Ocelot chips were added in commit f3e893626abe ("mfd:
ocelot: Add support for the vsc7512 chip via spi") it was believed that
this optimization was supported. However it is not.

Most register transactions with the Ocelot chips are not done in bulk, so
this bug could go unnoticed. The one scenario where bulk register
operations _are_ performed is when polling port statistics counters, which
was added in commit d87b1c08f38a ("net: mscc: ocelot: use bulk reads for
stats").

Things get slightly more complicated here...

A bug was introduced in commit d4c367650704 ("net: mscc: ocelot: keep
ocelot_stat_layout by reg address, not offset") that broke the optimization
of bulk reads. This means that when Ethernet support for the VSC7512 chip
was added in commit 3d7316ac81ac ("net: dsa: ocelot: add external ocelot
switch control") things were actually working "as expected".

The bulk read opmtimization was discovered, and fixed in commit
6acc72a43eac ("net: mscc: ocelot: fix stats region batching") and the
timing optimizations for SPI were noticed. A bulk read went from ~14ms to
~2ms. But this timing improvement came at the cost of every register
reading zero due the fact that bulk reads don't work.

The read timings increase back to 13-14ms, but that's a price worth paying
in order to receive valid data. This is verified in a DSA setup (cpsw-new
switch tied to port 0 on the VSC7512, after having been running overnight)

     Rx Octets: 16222055 # Counters from CPSW switch
     Tx Octets: 12034702
     Net Octets: 28256757
     p00_rx_octets: 12034702 # Counters from Ocelot switch
     p00_rx_frames_below_65_octets: 0
     p00_rx_frames_65_to_127_octets: 88188
     p00_rx_frames_128_to_255_octets: 13
     p00_rx_frames_256_to_511_octets: 0
     p00_rx_frames_512_to_1023_octets: 0
     p00_rx_frames_over_1526_octets: 3306
     p00_tx_octets: 16222055

Fixes: f3e893626abe ("mfd: ocelot: Add support for the vsc7512 chip via spi")
Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/mfd/ocelot-spi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
index 2ecd271de2fb..85021f94e587 100644
--- a/drivers/mfd/ocelot-spi.c
+++ b/drivers/mfd/ocelot-spi.c
@@ -130,6 +130,7 @@ static const struct regmap_config ocelot_spi_regmap_config = {
 
 	.write_flag_mask = 0x80,
 
+	.use_single_read = true,
 	.use_single_write = true,
 	.can_multi_write = false,
 
-- 
2.25.1

Re: [PATCH v1 mfd] mfd: ocelot-spi: fix bulk read

[Vladimir Oltean]

On Wed, Mar 22, 2023 at 07:11:30AM -0700, Colin Foster wrote:
> Ocelot chips (VSC7511, VSC7512, VSC7513, VSC7514) don't support bulk read
> operations over SPI.
> 
> Many SPI buses have hardware that can optimize consecutive reads.
> Essentially an address is written to the chip, and if the SPI controller
> continues to toggle the clock, subsequent register values are reported.
> This can lead to significant optimizations, because the time between
> "address is written to the chip" and "chip starts to report data" can often
> take a fixed amount of time.
> 
> When support for Ocelot chips were added in commit f3e893626abe ("mfd:
> ocelot: Add support for the vsc7512 chip via spi") it was believed that
> this optimization was supported. However it is not.

Details? What about bulk reads is "not supported", and not supported by whom?

Re: [PATCH v1 mfd] mfd: ocelot-spi: fix bulk read

[Colin Foster]

On Wed, Mar 22, 2023 at 05:25:51PM +0200, Vladimir Oltean wrote:
> On Wed, Mar 22, 2023 at 07:11:30AM -0700, Colin Foster wrote:
> > Ocelot chips (VSC7511, VSC7512, VSC7513, VSC7514) don't support bulk read
> > operations over SPI.
> > 
> > Many SPI buses have hardware that can optimize consecutive reads.
> > Essentially an address is written to the chip, and if the SPI controller
> > continues to toggle the clock, subsequent register values are reported.
> > This can lead to significant optimizations, because the time between
> > "address is written to the chip" and "chip starts to report data" can often
> > take a fixed amount of time.
> > 
> > When support for Ocelot chips were added in commit f3e893626abe ("mfd:
> > ocelot: Add support for the vsc7512 chip via spi") it was believed that
> > this optimization was supported. However it is not.
> 
> Details? What about bulk reads is "not supported", and not supported by whom?

The chip itself doesn't support bulk reads. Every register read must be
"Chip Select \" > "Read+Address Command" > "Padding" > "One Register Value" >
"Chip Select /"

Figure 74 of [1] shows "SI Read Timing in Fast Mode", but that is when
the VSC751X is the SPI controller, not a SPI endpoint. i.e. when the
VSC751X is reading _from_ an external flash chip. It also has a blurb
about "After reading address n, the SI boot controller automatically
continues reading address n+1".

Figure 63 shows "Read Sequence for One-Byte Padding" which is actually
done when an external CPU is reading _from_ the VSC751X devices. There
is no suggestion that address n+1 will be returned in this scenario.

[1] https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10489.pdf

I can update the commit message as needed.

Re: [PATCH v1 mfd] mfd: ocelot-spi: fix bulk read

[Vladimir Oltean]

On Wed, Mar 22, 2023 at 08:45:32AM -0700, Colin Foster wrote:
> The chip itself doesn't support bulk reads. Every register read must be
> "Chip Select \" > "Read+Address Command" > "Padding" > "One Register Value" >
> "Chip Select /"
> 
> Figure 74 of [1] shows "SI Read Timing in Fast Mode", but that is when
> the VSC751X is the SPI controller, not a SPI endpoint. i.e. when the
> VSC751X is reading _from_ an external flash chip. It also has a blurb
> about "After reading address n, the SI boot controller automatically
> continues reading address n+1".
> 
> Figure 63 shows "Read Sequence for One-Byte Padding" which is actually
> done when an external CPU is reading _from_ the VSC751X devices. There
> is no suggestion that address n+1 will be returned in this scenario.
> 
> [1] https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10489.pdf
> 
> I can update the commit message as needed.

I have no further comment. From my perspective, resending the patch is
not needed.

Re: [PATCH v1 mfd] mfd: ocelot-spi: fix bulk read

[Lee Jones]

On Wed, 22 Mar 2023, Colin Foster wrote:

> Ocelot chips (VSC7511, VSC7512, VSC7513, VSC7514) don't support bulk read
> operations over SPI.
>
> Many SPI buses have hardware that can optimize consecutive reads.
> Essentially an address is written to the chip, and if the SPI controller
> continues to toggle the clock, subsequent register values are reported.
> This can lead to significant optimizations, because the time between
> "address is written to the chip" and "chip starts to report data" can often
> take a fixed amount of time.
>
> When support for Ocelot chips were added in commit f3e893626abe ("mfd:
> ocelot: Add support for the vsc7512 chip via spi") it was believed that
> this optimization was supported. However it is not.
>
> Most register transactions with the Ocelot chips are not done in bulk, so
> this bug could go unnoticed. The one scenario where bulk register
> operations _are_ performed is when polling port statistics counters, which
> was added in commit d87b1c08f38a ("net: mscc: ocelot: use bulk reads for
> stats").
>
> Things get slightly more complicated here...
>
> A bug was introduced in commit d4c367650704 ("net: mscc: ocelot: keep
> ocelot_stat_layout by reg address, not offset") that broke the optimization
> of bulk reads. This means that when Ethernet support for the VSC7512 chip
> was added in commit 3d7316ac81ac ("net: dsa: ocelot: add external ocelot
> switch control") things were actually working "as expected".
>
> The bulk read opmtimization was discovered, and fixed in commit
> 6acc72a43eac ("net: mscc: ocelot: fix stats region batching") and the
> timing optimizations for SPI were noticed. A bulk read went from ~14ms to
> ~2ms. But this timing improvement came at the cost of every register
> reading zero due the fact that bulk reads don't work.
>
> The read timings increase back to 13-14ms, but that's a price worth paying
> in order to receive valid data. This is verified in a DSA setup (cpsw-new
> switch tied to port 0 on the VSC7512, after having been running overnight)
>
>      Rx Octets: 16222055 # Counters from CPSW switch
>      Tx Octets: 12034702
>      Net Octets: 28256757
>      p00_rx_octets: 12034702 # Counters from Ocelot switch
>      p00_rx_frames_below_65_octets: 0
>      p00_rx_frames_65_to_127_octets: 88188
>      p00_rx_frames_128_to_255_octets: 13
>      p00_rx_frames_256_to_511_octets: 0
>      p00_rx_frames_512_to_1023_octets: 0
>      p00_rx_frames_over_1526_octets: 3306
>      p00_tx_octets: 16222055
>
> Fixes: f3e893626abe ("mfd: ocelot: Add support for the vsc7512 chip via spi")
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
>  drivers/mfd/ocelot-spi.c | 1 +
>  1 file changed, 1 insertion(+)

Applied, thanks

--
Lee Jones [李琼斯]