[PATCH] eeprom: at25: set minimum read/write access stride to 1

[PATCH] eeprom: at25: set minimum read/write access stride to 1

[Christian Eggers]

SPI eeproms are addressed by byte.

Signed-off-by: Christian Eggers <ceggers@arri.de>
Cc: stable@vger.kernel.org
---
 drivers/misc/eeprom/at25.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
index 0e7c8dc01195..4e57eb145fcc 100644
--- a/drivers/misc/eeprom/at25.c
+++ b/drivers/misc/eeprom/at25.c
@@ -358,7 +358,7 @@ static int at25_probe(struct spi_device *spi)
 	at25->nvmem_config.reg_read = at25_ee_read;
 	at25->nvmem_config.reg_write = at25_ee_write;
 	at25->nvmem_config.priv = at25;
-	at25->nvmem_config.stride = 4;
+	at25->nvmem_config.stride = 1;
 	at25->nvmem_config.word_size = 1;
 	at25->nvmem_config.size = chip.byte_len;
 
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler

RE: [PATCH] eeprom: at25: set minimum read/write access stride to 1

[David Laight]

From: Christian Eggers
> Sent: 28 July 2020 10:30
> 
> SPI eeproms are addressed by byte.

They also support multi-byte writes - possibly with alignment
restrictions.
So forcing 4-byte writes (at aligned addresses) would typically
speed up writes by a factor of 4 over byte writes.

So does this fix a problem?
If so what.

So setting the 'stride' to 4 may be a compromise.
Looking at some code that writes the EPCQ for Altera FPGA
(which I think is just SPI) it does aligned 256 byte writes.
The long writes (and the 4-bit physical interface) are needed
to get the write times down to a sensible value.

	David

> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> Cc: stable@vger.kernel.org
> ---
>  drivers/misc/eeprom/at25.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
> index 0e7c8dc01195..4e57eb145fcc 100644
> --- a/drivers/misc/eeprom/at25.c
> +++ b/drivers/misc/eeprom/at25.c
> @@ -358,7 +358,7 @@ static int at25_probe(struct spi_device *spi)
>  	at25->nvmem_config.reg_read = at25_ee_read;
>  	at25->nvmem_config.reg_write = at25_ee_write;
>  	at25->nvmem_config.priv = at25;
> -	at25->nvmem_config.stride = 4;
> +	at25->nvmem_config.stride = 1;
>  	at25->nvmem_config.word_size = 1;
>  	at25->nvmem_config.size = chip.byte_len;
> 
> --
> Christian Eggers
> Embedded software developer
> 
> Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
> Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
> Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
> Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
> Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] eeprom: at25: set minimum read/write access stride to 1

[Christian Eggers]

On Tuesday, 28 July 2020, 11:52:05 CEST, David Laight wrote:
> From: Christian Eggers
> 
> > Sent: 28 July 2020 10:30
> > 
> > SPI eeproms are addressed by byte.
> 
> They also support multi-byte writes - possibly with alignment
> restrictions.
> So forcing 4-byte writes (at aligned addresses) would typically
> speed up writes by a factor of 4 over byte writes.
> 
> So does this fix a problem?
> If so what.
I use the nvmem-cells property for getting the MAC-Address out of the eeprom 
(actually an FRAM in my case).

&spi {
    ....
    fram: fram@0 {
    ...
        mac_address_fec2: mac-address@126 {
            reg = <0x126 6>;
        };
    ...
    };
};

&fec2 {
    ...
    nvmem-cells = <&mac_address_fec2>;
    nvmem-cell-names = "mac-address";
    ...
};

As the address of the MAC is not aligned to 4 bytes,
reading the MAC from FRAM fails.

> So setting the 'stride' to 4 may be a compromise.
> Looking at some code that writes the EPCQ for Altera FPGA
> (which I think is just SPI) it does aligned 256 byte writes.
> The long writes (and the 4-bit physical interface) are needed
> to get the write times down to a sensible value.
I do not understand why a minimum read/write stride of 1 would affect 
performance. It is fully up to the user of the eeprom, how much data is read/
written at once. Of course it would not be economical only to read/write one 
byte at a time.

For reading data, there should be no difference whether the access is aligned 
to a particular address. For writing, data is usually organized in "write 
pages". For (I2C) eeproms I've used, the write page size is typically 32 
bytes. But his is handled separately by the "pagesize" property.

Is there any benefit at all if the stride size is set > 1?


> 
> 	David
> 
regards
Christian

RE: [PATCH] eeprom: at25: set minimum read/write access stride to 1

[David Laight]

From: Christian Eggers
> Sent: 28 July 2020 11:30
> 
> On Tuesday, 28 July 2020, 11:52:05 CEST, David Laight wrote:
> > From: Christian Eggers
> >
> > > Sent: 28 July 2020 10:30
> > >
> > > SPI eeproms are addressed by byte.
> >
> > They also support multi-byte writes - possibly with alignment
> > restrictions.
> > So forcing 4-byte writes (at aligned addresses) would typically
> > speed up writes by a factor of 4 over byte writes.
> >
> > So does this fix a problem?
> > If so what.
> I use the nvmem-cells property for getting the MAC-Address out of the eeprom
> (actually an FRAM in my case).
> 
> &spi {
>     ....
>     fram: fram@0 {
>     ...
>         mac_address_fec2: mac-address@126 {
>             reg = <0x126 6>;
>         };
>     ...
>     };
> };

Hmmmm.... the 'stride' only constrains the alignment of 'cells'.
(ie address ranges from the device tree.)

It looks as though you can open the entire NVMEM device and
then do reads from byte offsets.
The 'stride' and 'word_size' are then not checked!

Actually it might be that before 01973a01f9ec3 byte aligned
'cells' were allowed.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] eeprom: at25: set minimum read/write access stride to 1

[Christian Eggers]

On Tuesday, 28 July 2020, 13:20:15 CEST, David Laight wrote:
> From: Christian Eggers
> 
> > &spi {
> >     ....
> >     fram: fram@0 {
> >     ...
> >         mac_address_fec2: mac-address@126 {
> >             reg = <0x126 6>;
> >         };
> >     ...
> >     };
> > };
> 
> 
> Hmmmm.... the 'stride' only constrains the alignment of 'cells'.
> (ie address ranges from the device tree.)

My mac-address is not aligned to 4 bytes...

> It looks as though you can open the entire NVMEM device and
> then do reads from byte offsets.
> The 'stride' and 'word_size' are then not checked!

When I set back the stride to 4, I get the following errors:
[    6.998788] 000: nvmem spi0.00: cell mac-address unaligned to nvmem stride 4
[    6.998902] 000: at25: probe of spi0.0 failed with error -22
...
[    7.146454] 000: fec 20b4000.ethernet: Invalid MAC address: 00:00:00:00:00:00
[    7.146480] 000: fec 20b4000.ethernet: Using random MAC address: 6e:9d:37:49:6d:15

> Actually it might be that before 01973a01f9ec3 byte aligned
> 'cells' were allowed.

I use linux-5.4.x (latest), the mentioned patch has been included long time ago.

regards
Christian