Re: [PATCH 1/3] eeprom: at25: Add DT support for EEPROMs with odd address bits

[Geert Uytterhoeven]

Hi Rob,

On Tue, Dec 5, 2017 at 2:56 PM, Rob Herring  wrote:
> On Tue, Dec 5, 2017 at 3:09 AM, Geert Uytterhoeven  
> wrote:
>> On Tue, Dec 5, 2017 at 9:57 AM, Geert Uytterhoeven  
>> wrote:
>>> On Mon, Dec 4, 2017 at 10:17 PM, Rob Herring  wrote:
 On Mon, Dec 04, 2017 at 10:17:47AM +0100, Geert Uytterhoeven wrote:
> On Thu, Nov 30, 2017 at 2:29 PM, Geert Uytterhoeven
>  wrote:
> > Certain EEPROMS have a size that is larger than the number of address
> > bytes would allow, and store the MSB of the address in bit 3 of the
> > instruction byte.
> >
> > This can be described in platform data using EE_INSTR_BIT3_IS_ADDR, or
> > in DT using the obsolete legacy "at25,addr-mode" property.
> > But currently there exists no non-deprecated way to describe this in DT.
> >
> > Hence extend the existing "address-width" DT property to allow
> > specifying 9, 17, or 25 address bits, and enable support for that in the
> > driver.
> >
> > Signed-off-by: Geert Uytterhoeven 
> > ---
> > EEPROMs using 9 address bits are common (e.g. M95040, 25AA040/25LC040).
> > Do EEPROMs using 17 or 25 address bits, as mentioned in
> > include/linux/spi/eeprom.h, really exist?
> > Or should we just limit it to a single odd value (9 bits)?
>
> At least for the real Atmel parts, only the AT25040 part uses odd (8 +
> 1 bit) addressing.

 Seems like we should have a specific compatible for it.
>>>
>>> Possibly. But currently all configuration is done through DT properties, not
>>> through matching on compatible values.
>>
>> Adding compatible values for all known/used parts could quickly become a
>> large table.
>> E.g. Atmel/Microchip has 3 variants of 512-byte EEPROMs: AT25040B,
>> 25LC040A, and 25AA040A. The former uses an 8-byte pagesize, while the
>> latter parts use 16-byte pagesizes.
>> Not to mention "compatible" parts from other manufacturers, and all other
>> supported size.
>>
>> Currently all of this is configured through the "pagesize", "size", and
>> "address-width" DT properties, with matching on generic "atmel,at25".
>
> I wasn't suggesting throwing out all these. Just add a compatible for
> the one oddball 9-bit part.
>
> But I'm fine adding address-width=9 too.

OK. Then I'll go for the least intrusive solution (address-width=9).
These EEPROMs are fairly small and simple, and I can imagine them being
used on small systems too, so driver code/data size matters.

Stay tuned for v2.

Thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 1/3] eeprom: at25: Add DT support for EEPROMs with odd address bits

[Geert Uytterhoeven]

Hi Rob,

On Tue, Dec 5, 2017 at 2:56 PM, Rob Herring  wrote:
> On Tue, Dec 5, 2017 at 3:09 AM, Geert Uytterhoeven  
> wrote:
>> On Tue, Dec 5, 2017 at 9:57 AM, Geert Uytterhoeven  
>> wrote:
>>> On Mon, Dec 4, 2017 at 10:17 PM, Rob Herring  wrote:
 On Mon, Dec 04, 2017 at 10:17:47AM +0100, Geert Uytterhoeven wrote:
> On Thu, Nov 30, 2017 at 2:29 PM, Geert Uytterhoeven
>  wrote:
> > Certain EEPROMS have a size that is larger than the number of address
> > bytes would allow, and store the MSB of the address in bit 3 of the
> > instruction byte.
> >
> > This can be described in platform data using EE_INSTR_BIT3_IS_ADDR, or
> > in DT using the obsolete legacy "at25,addr-mode" property.
> > But currently there exists no non-deprecated way to describe this in DT.
> >
> > Hence extend the existing "address-width" DT property to allow
> > specifying 9, 17, or 25 address bits, and enable support for that in the
> > driver.
> >
> > Signed-off-by: Geert Uytterhoeven 
> > ---
> > EEPROMs using 9 address bits are common (e.g. M95040, 25AA040/25LC040).
> > Do EEPROMs using 17 or 25 address bits, as mentioned in
> > include/linux/spi/eeprom.h, really exist?
> > Or should we just limit it to a single odd value (9 bits)?
>
> At least for the real Atmel parts, only the AT25040 part uses odd (8 +
> 1 bit) addressing.

 Seems like we should have a specific compatible for it.
>>>
>>> Possibly. But currently all configuration is done through DT properties, not
>>> through matching on compatible values.
>>
>> Adding compatible values for all known/used parts could quickly become a
>> large table.
>> E.g. Atmel/Microchip has 3 variants of 512-byte EEPROMs: AT25040B,
>> 25LC040A, and 25AA040A. The former uses an 8-byte pagesize, while the
>> latter parts use 16-byte pagesizes.
>> Not to mention "compatible" parts from other manufacturers, and all other
>> supported size.
>>
>> Currently all of this is configured through the "pagesize", "size", and
>> "address-width" DT properties, with matching on generic "atmel,at25".
>
> I wasn't suggesting throwing out all these. Just add a compatible for
> the one oddball 9-bit part.
>
> But I'm fine adding address-width=9 too.

OK. Then I'll go for the least intrusive solution (address-width=9).
These EEPROMs are fairly small and simple, and I can imagine them being
used on small systems too, so driver code/data size matters.

Stay tuned for v2.

Thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 1/3] eeprom: at25: Add DT support for EEPROMs with odd address bits

[Rob Herring]

On Tue, Dec 5, 2017 at 3:09 AM, Geert Uytterhoeven  wrote:
> Hi Rob,
>
> On Tue, Dec 5, 2017 at 9:57 AM, Geert Uytterhoeven  
> wrote:
>> On Mon, Dec 4, 2017 at 10:17 PM, Rob Herring  wrote:
>>> On Mon, Dec 04, 2017 at 10:17:47AM +0100, Geert Uytterhoeven wrote:
 On Thu, Nov 30, 2017 at 2:29 PM, Geert Uytterhoeven
  wrote:
 > Certain EEPROMS have a size that is larger than the number of address
 > bytes would allow, and store the MSB of the address in bit 3 of the
 > instruction byte.
 >
 > This can be described in platform data using EE_INSTR_BIT3_IS_ADDR, or
 > in DT using the obsolete legacy "at25,addr-mode" property.
 > But currently there exists no non-deprecated way to describe this in DT.
 >
 > Hence extend the existing "address-width" DT property to allow
 > specifying 9, 17, or 25 address bits, and enable support for that in the
 > driver.
 >
 > Signed-off-by: Geert Uytterhoeven 
 > ---
 > EEPROMs using 9 address bits are common (e.g. M95040, 25AA040/25LC040).
 > Do EEPROMs using 17 or 25 address bits, as mentioned in
 > include/linux/spi/eeprom.h, really exist?
 > Or should we just limit it to a single odd value (9 bits)?

 At least for the real Atmel parts, only the AT25040 part uses odd (8 +
 1 bit) addressing.
>>>
>>> Seems like we should have a specific compatible for it.
>>
>> Possibly. But currently all configuration is done through DT properties, not
>> through matching on compatible values.
>
> Adding compatible values for all known/used parts could quickly become a
> large table.
> E.g. Atmel/Microchip has 3 variants of 512-byte EEPROMs: AT25040B,
> 25LC040A, and 25AA040A. The former uses an 8-byte pagesize, while the
> latter parts use 16-byte pagesizes.
> Not to mention "compatible" parts from other manufacturers, and all other
> supported size.
>
> Currently all of this is configured through the "pagesize", "size", and
> "address-width" DT properties, with matching on generic "atmel,at25".

I wasn't suggesting throwing out all these. Just add a compatible for
the one oddball 9-bit part.

But I'm fine adding address-width=9 too.

Rob

Re: [PATCH 1/3] eeprom: at25: Add DT support for EEPROMs with odd address bits

[Rob Herring]

On Tue, Dec 5, 2017 at 3:09 AM, Geert Uytterhoeven  wrote:
> Hi Rob,
>
> On Tue, Dec 5, 2017 at 9:57 AM, Geert Uytterhoeven  
> wrote:
>> On Mon, Dec 4, 2017 at 10:17 PM, Rob Herring  wrote:
>>> On Mon, Dec 04, 2017 at 10:17:47AM +0100, Geert Uytterhoeven wrote:
 On Thu, Nov 30, 2017 at 2:29 PM, Geert Uytterhoeven
  wrote:
 > Certain EEPROMS have a size that is larger than the number of address
 > bytes would allow, and store the MSB of the address in bit 3 of the
 > instruction byte.
 >
 > This can be described in platform data using EE_INSTR_BIT3_IS_ADDR, or
 > in DT using the obsolete legacy "at25,addr-mode" property.
 > But currently there exists no non-deprecated way to describe this in DT.
 >
 > Hence extend the existing "address-width" DT property to allow
 > specifying 9, 17, or 25 address bits, and enable support for that in the
 > driver.
 >
 > Signed-off-by: Geert Uytterhoeven 
 > ---
 > EEPROMs using 9 address bits are common (e.g. M95040, 25AA040/25LC040).
 > Do EEPROMs using 17 or 25 address bits, as mentioned in
 > include/linux/spi/eeprom.h, really exist?
 > Or should we just limit it to a single odd value (9 bits)?

 At least for the real Atmel parts, only the AT25040 part uses odd (8 +
 1 bit) addressing.
>>>
>>> Seems like we should have a specific compatible for it.
>>
>> Possibly. But currently all configuration is done through DT properties, not
>> through matching on compatible values.
>
> Adding compatible values for all known/used parts could quickly become a
> large table.
> E.g. Atmel/Microchip has 3 variants of 512-byte EEPROMs: AT25040B,
> 25LC040A, and 25AA040A. The former uses an 8-byte pagesize, while the
> latter parts use 16-byte pagesizes.
> Not to mention "compatible" parts from other manufacturers, and all other
> supported size.
>
> Currently all of this is configured through the "pagesize", "size", and
> "address-width" DT properties, with matching on generic "atmel,at25".

I wasn't suggesting throwing out all these. Just add a compatible for
the one oddball 9-bit part.

But I'm fine adding address-width=9 too.

Rob

Re: [PATCH 1/3] eeprom: at25: Add DT support for EEPROMs with odd address bits

[Geert Uytterhoeven]

Hi Rob,

On Tue, Dec 5, 2017 at 9:57 AM, Geert Uytterhoeven  wrote:
> On Mon, Dec 4, 2017 at 10:17 PM, Rob Herring  wrote:
>> On Mon, Dec 04, 2017 at 10:17:47AM +0100, Geert Uytterhoeven wrote:
>>> On Thu, Nov 30, 2017 at 2:29 PM, Geert Uytterhoeven
>>>  wrote:
>>> > Certain EEPROMS have a size that is larger than the number of address
>>> > bytes would allow, and store the MSB of the address in bit 3 of the
>>> > instruction byte.
>>> >
>>> > This can be described in platform data using EE_INSTR_BIT3_IS_ADDR, or
>>> > in DT using the obsolete legacy "at25,addr-mode" property.
>>> > But currently there exists no non-deprecated way to describe this in DT.
>>> >
>>> > Hence extend the existing "address-width" DT property to allow
>>> > specifying 9, 17, or 25 address bits, and enable support for that in the
>>> > driver.
>>> >
>>> > Signed-off-by: Geert Uytterhoeven 
>>> > ---
>>> > EEPROMs using 9 address bits are common (e.g. M95040, 25AA040/25LC040).
>>> > Do EEPROMs using 17 or 25 address bits, as mentioned in
>>> > include/linux/spi/eeprom.h, really exist?
>>> > Or should we just limit it to a single odd value (9 bits)?
>>>
>>> At least for the real Atmel parts, only the AT25040 part uses odd (8 +
>>> 1 bit) addressing.
>>
>> Seems like we should have a specific compatible for it.
>
> Possibly. But currently all configuration is done through DT properties, not
> through matching on compatible values.

Adding compatible values for all known/used parts could quickly become a
large table.
E.g. Atmel/Microchip has 3 variants of 512-byte EEPROMs: AT25040B,
25LC040A, and 25AA040A. The former uses an 8-byte pagesize, while the
latter parts use 16-byte pagesizes.
Not to mention "compatible" parts from other manufacturers, and all other
supported size.

Currently all of this is configured through the "pagesize", "size", and
"address-width" DT properties, with matching on generic "atmel,at25".

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 1/3] eeprom: at25: Add DT support for EEPROMs with odd address bits

[Geert Uytterhoeven]

Hi Rob,

On Tue, Dec 5, 2017 at 9:57 AM, Geert Uytterhoeven  wrote:
> On Mon, Dec 4, 2017 at 10:17 PM, Rob Herring  wrote:
>> On Mon, Dec 04, 2017 at 10:17:47AM +0100, Geert Uytterhoeven wrote:
>>> On Thu, Nov 30, 2017 at 2:29 PM, Geert Uytterhoeven
>>>  wrote:
>>> > Certain EEPROMS have a size that is larger than the number of address
>>> > bytes would allow, and store the MSB of the address in bit 3 of the
>>> > instruction byte.
>>> >
>>> > This can be described in platform data using EE_INSTR_BIT3_IS_ADDR, or
>>> > in DT using the obsolete legacy "at25,addr-mode" property.
>>> > But currently there exists no non-deprecated way to describe this in DT.
>>> >
>>> > Hence extend the existing "address-width" DT property to allow
>>> > specifying 9, 17, or 25 address bits, and enable support for that in the
>>> > driver.
>>> >
>>> > Signed-off-by: Geert Uytterhoeven 
>>> > ---
>>> > EEPROMs using 9 address bits are common (e.g. M95040, 25AA040/25LC040).
>>> > Do EEPROMs using 17 or 25 address bits, as mentioned in
>>> > include/linux/spi/eeprom.h, really exist?
>>> > Or should we just limit it to a single odd value (9 bits)?
>>>
>>> At least for the real Atmel parts, only the AT25040 part uses odd (8 +
>>> 1 bit) addressing.
>>
>> Seems like we should have a specific compatible for it.
>
> Possibly. But currently all configuration is done through DT properties, not
> through matching on compatible values.

Adding compatible values for all known/used parts could quickly become a
large table.
E.g. Atmel/Microchip has 3 variants of 512-byte EEPROMs: AT25040B,
25LC040A, and 25AA040A. The former uses an 8-byte pagesize, while the
latter parts use 16-byte pagesizes.
Not to mention "compatible" parts from other manufacturers, and all other
supported size.

Currently all of this is configured through the "pagesize", "size", and
"address-width" DT properties, with matching on generic "atmel,at25".

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 1/3] eeprom: at25: Add DT support for EEPROMs with odd address bits

[Geert Uytterhoeven]

Hi Ivo,

On Mon, Dec 4, 2017 at 11:00 PM, Ivo Sieben  wrote:
> 2017-12-04 10:17 GMT+01:00 Geert Uytterhoeven :
>>> EEPROMs using 9 address bits are common (e.g. M95040, 25AA040/25LC040).
>>> Do EEPROMs using 17 or 25 address bits, as mentioned in
>>> include/linux/spi/eeprom.h, really exist?
>>> Or should we just limit it to a single odd value (9 bits)?
>>
>> At least for the real Atmel parts, only the AT25040 part uses odd (8 +
>> 1 bit) addressing.
>> AT25M01 uses 3-byte addressing (it needs 17 bits).
>>
>> So I tend to believe EEPROMs using 16 + 1  or 24 + 1 address bits (with the
>> extra bit in the instruction byte) do not exist?
>>
>
> I think you are right. Most likely this extra address bit option is
> only used for 9 bit addressable chips.
> I'm not an expert, but I know only the M95040 chip for which I
> originally wrote the patch.
> By then I decided to make it a bit broader (so also to be used as
> address 17 & 25 bit addressing) but that might
> not make any sense indeed.
>
>>> @@ -6,7 +6,9 @@ Required properties:
>>>  - spi-max-frequency : max spi frequency to use
>>>  - pagesize : size of the eeprom page
>>>  - size : total eeprom size in bytes
>>> -- address-width : number of address bits (one of 8, 16, or 24)
>>> +- address-width : number of address bits (one of 8, 9, 16, 17, 24, or 25).
>>> +  For odd values, the MSB of the address is sent as bit 3 of the 
>>> instruction
>>> +  byte, before the address byte(s).
>>
>> Alternatively, we can drop the binding change, i.e. keep on using
>> address-width = <8> for 512-byte '040...
>>
>
> As you also stated before: maybe it is more clear to leave only the
> "9" value option documented
> here, that looks to me the only valid use case for it.

OK.

>
>>> +   if (val & 1) {
>>> +   chip->flags |= EE_INSTR_BIT3_IS_ADDR;
>>> +   val -= 1;
>>> +   }
>>
>> ... and handle it here like:
>>
>> if (chip->byte_len == 2U << val)
>> chip->flags |= EE_INSTR_BIT3_IS_ADDR;
>>
>> However, that would IMHO be a bit confusing, as the "address-width"
>> property is no longer the real address width, but indicates how many bits
>> are specified in address bytes sent after the read/write command.
>> So "address-bytes" = 1, 2, or 3 would be more correct ;-)
>>
>> Or deprecate this whole "specify parameters using DT properties" business,
>> and derive them from the compatible value. But that would mean adding a
>> large and ever growing table to an old driver...
>>
>> Thoughts?
>
> I'm not a DT expert but to me your first proposal makes the most sense
> to me and feels the most intuitive:
> I would go for the address-with value 9 option here.

OK.

> Since we only expect value 9 to be a valid option, maybe you could
> rewrite it a bit to explicitly check for value 9:
>
> if (val == 9) {
> chip->flags |= EE_INSTR_BIT3_IS_ADDR;
> val -= 1;
> }
>
> I think this is slightly more readable.

Sure.

> Hope this helps,

Thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 1/3] eeprom: at25: Add DT support for EEPROMs with odd address bits

[Geert Uytterhoeven]

Hi Ivo,

On Mon, Dec 4, 2017 at 11:00 PM, Ivo Sieben  wrote:
> 2017-12-04 10:17 GMT+01:00 Geert Uytterhoeven :
>>> EEPROMs using 9 address bits are common (e.g. M95040, 25AA040/25LC040).
>>> Do EEPROMs using 17 or 25 address bits, as mentioned in
>>> include/linux/spi/eeprom.h, really exist?
>>> Or should we just limit it to a single odd value (9 bits)?
>>
>> At least for the real Atmel parts, only the AT25040 part uses odd (8 +
>> 1 bit) addressing.
>> AT25M01 uses 3-byte addressing (it needs 17 bits).
>>
>> So I tend to believe EEPROMs using 16 + 1  or 24 + 1 address bits (with the
>> extra bit in the instruction byte) do not exist?
>>
>
> I think you are right. Most likely this extra address bit option is
> only used for 9 bit addressable chips.
> I'm not an expert, but I know only the M95040 chip for which I
> originally wrote the patch.
> By then I decided to make it a bit broader (so also to be used as
> address 17 & 25 bit addressing) but that might
> not make any sense indeed.
>
>>> @@ -6,7 +6,9 @@ Required properties:
>>>  - spi-max-frequency : max spi frequency to use
>>>  - pagesize : size of the eeprom page
>>>  - size : total eeprom size in bytes
>>> -- address-width : number of address bits (one of 8, 16, or 24)
>>> +- address-width : number of address bits (one of 8, 9, 16, 17, 24, or 25).
>>> +  For odd values, the MSB of the address is sent as bit 3 of the 
>>> instruction
>>> +  byte, before the address byte(s).
>>
>> Alternatively, we can drop the binding change, i.e. keep on using
>> address-width = <8> for 512-byte '040...
>>
>
> As you also stated before: maybe it is more clear to leave only the
> "9" value option documented
> here, that looks to me the only valid use case for it.

OK.

>
>>> +   if (val & 1) {
>>> +   chip->flags |= EE_INSTR_BIT3_IS_ADDR;
>>> +   val -= 1;
>>> +   }
>>
>> ... and handle it here like:
>>
>> if (chip->byte_len == 2U << val)
>> chip->flags |= EE_INSTR_BIT3_IS_ADDR;
>>
>> However, that would IMHO be a bit confusing, as the "address-width"
>> property is no longer the real address width, but indicates how many bits
>> are specified in address bytes sent after the read/write command.
>> So "address-bytes" = 1, 2, or 3 would be more correct ;-)
>>
>> Or deprecate this whole "specify parameters using DT properties" business,
>> and derive them from the compatible value. But that would mean adding a
>> large and ever growing table to an old driver...
>>
>> Thoughts?
>
> I'm not a DT expert but to me your first proposal makes the most sense
> to me and feels the most intuitive:
> I would go for the address-with value 9 option here.

OK.

> Since we only expect value 9 to be a valid option, maybe you could
> rewrite it a bit to explicitly check for value 9:
>
> if (val == 9) {
> chip->flags |= EE_INSTR_BIT3_IS_ADDR;
> val -= 1;
> }
>
> I think this is slightly more readable.

Sure.

> Hope this helps,

Thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 1/3] eeprom: at25: Add DT support for EEPROMs with odd address bits

[Geert Uytterhoeven]

Hi Rob,

On Mon, Dec 4, 2017 at 10:17 PM, Rob Herring  wrote:
> On Mon, Dec 04, 2017 at 10:17:47AM +0100, Geert Uytterhoeven wrote:
>> On Thu, Nov 30, 2017 at 2:29 PM, Geert Uytterhoeven
>>  wrote:
>> > Certain EEPROMS have a size that is larger than the number of address
>> > bytes would allow, and store the MSB of the address in bit 3 of the
>> > instruction byte.
>> >
>> > This can be described in platform data using EE_INSTR_BIT3_IS_ADDR, or
>> > in DT using the obsolete legacy "at25,addr-mode" property.
>> > But currently there exists no non-deprecated way to describe this in DT.
>> >
>> > Hence extend the existing "address-width" DT property to allow
>> > specifying 9, 17, or 25 address bits, and enable support for that in the
>> > driver.
>> >
>> > Signed-off-by: Geert Uytterhoeven 
>> > ---
>> > EEPROMs using 9 address bits are common (e.g. M95040, 25AA040/25LC040).
>> > Do EEPROMs using 17 or 25 address bits, as mentioned in
>> > include/linux/spi/eeprom.h, really exist?
>> > Or should we just limit it to a single odd value (9 bits)?
>>
>> At least for the real Atmel parts, only the AT25040 part uses odd (8 +
>> 1 bit) addressing.
>
> Seems like we should have a specific compatible for it.

Possibly. But currently all configuration is done through DT properties, not
through matching on compatible values.

>> AT25M01 uses 3-byte addressing (it needs 17 bits).
>
> Do you need to know it is 17-bit vs. 24-bits? I'm guessing not as the
> unused bits are probably don't care.

The 17 bits can be derived from the EEPROM size in bytes (1 Mb = 128 KiB).
What is important to know is how to pass addresses to the device:
  1. 3 address bytes, OR
  2. 2 address bytes, and the odd MSB bit in the command byte.

But apparently the second scheme is not used for 17-bit addressing.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 1/3] eeprom: at25: Add DT support for EEPROMs with odd address bits

[Geert Uytterhoeven]

Hi Rob,

On Mon, Dec 4, 2017 at 10:17 PM, Rob Herring  wrote:
> On Mon, Dec 04, 2017 at 10:17:47AM +0100, Geert Uytterhoeven wrote:
>> On Thu, Nov 30, 2017 at 2:29 PM, Geert Uytterhoeven
>>  wrote:
>> > Certain EEPROMS have a size that is larger than the number of address
>> > bytes would allow, and store the MSB of the address in bit 3 of the
>> > instruction byte.
>> >
>> > This can be described in platform data using EE_INSTR_BIT3_IS_ADDR, or
>> > in DT using the obsolete legacy "at25,addr-mode" property.
>> > But currently there exists no non-deprecated way to describe this in DT.
>> >
>> > Hence extend the existing "address-width" DT property to allow
>> > specifying 9, 17, or 25 address bits, and enable support for that in the
>> > driver.
>> >
>> > Signed-off-by: Geert Uytterhoeven 
>> > ---
>> > EEPROMs using 9 address bits are common (e.g. M95040, 25AA040/25LC040).
>> > Do EEPROMs using 17 or 25 address bits, as mentioned in
>> > include/linux/spi/eeprom.h, really exist?
>> > Or should we just limit it to a single odd value (9 bits)?
>>
>> At least for the real Atmel parts, only the AT25040 part uses odd (8 +
>> 1 bit) addressing.
>
> Seems like we should have a specific compatible for it.

Possibly. But currently all configuration is done through DT properties, not
through matching on compatible values.

>> AT25M01 uses 3-byte addressing (it needs 17 bits).
>
> Do you need to know it is 17-bit vs. 24-bits? I'm guessing not as the
> unused bits are probably don't care.

The 17 bits can be derived from the EEPROM size in bytes (1 Mb = 128 KiB).
What is important to know is how to pass addresses to the device:
  1. 3 address bytes, OR
  2. 2 address bytes, and the odd MSB bit in the command byte.

But apparently the second scheme is not used for 17-bit addressing.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 1/3] eeprom: at25: Add DT support for EEPROMs with odd address bits

[Ivo Sieben]

Hi Geert,

My 2 cents:

2017-12-04 10:17 GMT+01:00 Geert Uytterhoeven :
>> EEPROMs using 9 address bits are common (e.g. M95040, 25AA040/25LC040).
>> Do EEPROMs using 17 or 25 address bits, as mentioned in
>> include/linux/spi/eeprom.h, really exist?
>> Or should we just limit it to a single odd value (9 bits)?
>
> At least for the real Atmel parts, only the AT25040 part uses odd (8 +
> 1 bit) addressing.
> AT25M01 uses 3-byte addressing (it needs 17 bits).
>
> So I tend to believe EEPROMs using 16 + 1  or 24 + 1 address bits (with the
> extra bit in the instruction byte) do not exist?
>

I think you are right. Most likely this extra address bit option is
only used for 9 bit addressable chips.
I'm not an expert, but I know only the M95040 chip for which I
originally wrote the patch.
By then I decided to make it a bit broader (so also to be used as
address 17 & 25 bit addressing) but that might
not make any sense indeed.

>> @@ -6,7 +6,9 @@ Required properties:
>>  - spi-max-frequency : max spi frequency to use
>>  - pagesize : size of the eeprom page
>>  - size : total eeprom size in bytes
>> -- address-width : number of address bits (one of 8, 16, or 24)
>> +- address-width : number of address bits (one of 8, 9, 16, 17, 24, or 25).
>> +  For odd values, the MSB of the address is sent as bit 3 of the instruction
>> +  byte, before the address byte(s).
>
> Alternatively, we can drop the binding change, i.e. keep on using
> address-width = <8> for 512-byte '040...
>

As you also stated before: maybe it is more clear to leave only the
"9" value option documented
here, that looks to me the only valid use case for it.

>> +   if (val & 1) {
>> +   chip->flags |= EE_INSTR_BIT3_IS_ADDR;
>> +   val -= 1;
>> +   }
>
> ... and handle it here like:
>
> if (chip->byte_len == 2U << val)
> chip->flags |= EE_INSTR_BIT3_IS_ADDR;
>
> However, that would IMHO be a bit confusing, as the "address-width"
> property is no longer the real address width, but indicates how many bits
> are specified in address bytes sent after the read/write command.
> So "address-bytes" = 1, 2, or 3 would be more correct ;-)
>
> Or deprecate this whole "specify parameters using DT properties" business,
> and derive them from the compatible value. But that would mean adding a
> large and ever growing table to an old driver...
>
> Thoughts?

I'm not a DT expert but to me your first proposal makes the most sense
to me and feels the most intuitive:
I would go for the address-with value 9 option here.

Since we only expect value 9 to be a valid option, maybe you could
rewrite it a bit to explicitly check for value 9:

if (val == 9) {
chip->flags |= EE_INSTR_BIT3_IS_ADDR;
val -= 1;
}

I think this is slightly more readable.

Hope this helps,

Regards,
Ivo Sieben

Re: [PATCH 1/3] eeprom: at25: Add DT support for EEPROMs with odd address bits

[Ivo Sieben]

Hi Geert,

My 2 cents:

2017-12-04 10:17 GMT+01:00 Geert Uytterhoeven :
>> EEPROMs using 9 address bits are common (e.g. M95040, 25AA040/25LC040).
>> Do EEPROMs using 17 or 25 address bits, as mentioned in
>> include/linux/spi/eeprom.h, really exist?
>> Or should we just limit it to a single odd value (9 bits)?
>
> At least for the real Atmel parts, only the AT25040 part uses odd (8 +
> 1 bit) addressing.
> AT25M01 uses 3-byte addressing (it needs 17 bits).
>
> So I tend to believe EEPROMs using 16 + 1  or 24 + 1 address bits (with the
> extra bit in the instruction byte) do not exist?
>

I think you are right. Most likely this extra address bit option is
only used for 9 bit addressable chips.
I'm not an expert, but I know only the M95040 chip for which I
originally wrote the patch.
By then I decided to make it a bit broader (so also to be used as
address 17 & 25 bit addressing) but that might
not make any sense indeed.

>> @@ -6,7 +6,9 @@ Required properties:
>>  - spi-max-frequency : max spi frequency to use
>>  - pagesize : size of the eeprom page
>>  - size : total eeprom size in bytes
>> -- address-width : number of address bits (one of 8, 16, or 24)
>> +- address-width : number of address bits (one of 8, 9, 16, 17, 24, or 25).
>> +  For odd values, the MSB of the address is sent as bit 3 of the instruction
>> +  byte, before the address byte(s).
>
> Alternatively, we can drop the binding change, i.e. keep on using
> address-width = <8> for 512-byte '040...
>

As you also stated before: maybe it is more clear to leave only the
"9" value option documented
here, that looks to me the only valid use case for it.

>> +   if (val & 1) {
>> +   chip->flags |= EE_INSTR_BIT3_IS_ADDR;
>> +   val -= 1;
>> +   }
>
> ... and handle it here like:
>
> if (chip->byte_len == 2U << val)
> chip->flags |= EE_INSTR_BIT3_IS_ADDR;
>
> However, that would IMHO be a bit confusing, as the "address-width"
> property is no longer the real address width, but indicates how many bits
> are specified in address bytes sent after the read/write command.
> So "address-bytes" = 1, 2, or 3 would be more correct ;-)
>
> Or deprecate this whole "specify parameters using DT properties" business,
> and derive them from the compatible value. But that would mean adding a
> large and ever growing table to an old driver...
>
> Thoughts?

I'm not a DT expert but to me your first proposal makes the most sense
to me and feels the most intuitive:
I would go for the address-with value 9 option here.

Since we only expect value 9 to be a valid option, maybe you could
rewrite it a bit to explicitly check for value 9:

if (val == 9) {
chip->flags |= EE_INSTR_BIT3_IS_ADDR;
val -= 1;
}

I think this is slightly more readable.

Hope this helps,

Regards,
Ivo Sieben

Re: [PATCH 1/3] eeprom: at25: Add DT support for EEPROMs with odd address bits

[Rob Herring]

On Mon, Dec 04, 2017 at 10:17:47AM +0100, Geert Uytterhoeven wrote:
> On Thu, Nov 30, 2017 at 2:29 PM, Geert Uytterhoeven
>  wrote:
> > Certain EEPROMS have a size that is larger than the number of address
> > bytes would allow, and store the MSB of the address in bit 3 of the
> > instruction byte.
> >
> > This can be described in platform data using EE_INSTR_BIT3_IS_ADDR, or
> > in DT using the obsolete legacy "at25,addr-mode" property.
> > But currently there exists no non-deprecated way to describe this in DT.
> >
> > Hence extend the existing "address-width" DT property to allow
> > specifying 9, 17, or 25 address bits, and enable support for that in the
> > driver.
> >
> > Signed-off-by: Geert Uytterhoeven 
> > ---
> > EEPROMs using 9 address bits are common (e.g. M95040, 25AA040/25LC040).
> > Do EEPROMs using 17 or 25 address bits, as mentioned in
> > include/linux/spi/eeprom.h, really exist?
> > Or should we just limit it to a single odd value (9 bits)?
> 
> At least for the real Atmel parts, only the AT25040 part uses odd (8 +
> 1 bit) addressing.

Seems like we should have a specific compatible for it.

> AT25M01 uses 3-byte addressing (it needs 17 bits).

Do you need to know it is 17-bit vs. 24-bits? I'm guessing not as the 
unused bits are probably don't care.

Rob

Re: [PATCH 1/3] eeprom: at25: Add DT support for EEPROMs with odd address bits

[Rob Herring]

On Mon, Dec 04, 2017 at 10:17:47AM +0100, Geert Uytterhoeven wrote:
> On Thu, Nov 30, 2017 at 2:29 PM, Geert Uytterhoeven
>  wrote:
> > Certain EEPROMS have a size that is larger than the number of address
> > bytes would allow, and store the MSB of the address in bit 3 of the
> > instruction byte.
> >
> > This can be described in platform data using EE_INSTR_BIT3_IS_ADDR, or
> > in DT using the obsolete legacy "at25,addr-mode" property.
> > But currently there exists no non-deprecated way to describe this in DT.
> >
> > Hence extend the existing "address-width" DT property to allow
> > specifying 9, 17, or 25 address bits, and enable support for that in the
> > driver.
> >
> > Signed-off-by: Geert Uytterhoeven 
> > ---
> > EEPROMs using 9 address bits are common (e.g. M95040, 25AA040/25LC040).
> > Do EEPROMs using 17 or 25 address bits, as mentioned in
> > include/linux/spi/eeprom.h, really exist?
> > Or should we just limit it to a single odd value (9 bits)?
> 
> At least for the real Atmel parts, only the AT25040 part uses odd (8 +
> 1 bit) addressing.

Seems like we should have a specific compatible for it.

> AT25M01 uses 3-byte addressing (it needs 17 bits).

Do you need to know it is 17-bit vs. 24-bits? I'm guessing not as the 
unused bits are probably don't care.

Rob

Re: [PATCH 1/3] eeprom: at25: Add DT support for EEPROMs with odd address bits

[Geert Uytterhoeven]

On Thu, Nov 30, 2017 at 2:29 PM, Geert Uytterhoeven
 wrote:
> Certain EEPROMS have a size that is larger than the number of address
> bytes would allow, and store the MSB of the address in bit 3 of the
> instruction byte.
>
> This can be described in platform data using EE_INSTR_BIT3_IS_ADDR, or
> in DT using the obsolete legacy "at25,addr-mode" property.
> But currently there exists no non-deprecated way to describe this in DT.
>
> Hence extend the existing "address-width" DT property to allow
> specifying 9, 17, or 25 address bits, and enable support for that in the
> driver.
>
> Signed-off-by: Geert Uytterhoeven 
> ---
> EEPROMs using 9 address bits are common (e.g. M95040, 25AA040/25LC040).
> Do EEPROMs using 17 or 25 address bits, as mentioned in
> include/linux/spi/eeprom.h, really exist?
> Or should we just limit it to a single odd value (9 bits)?

At least for the real Atmel parts, only the AT25040 part uses odd (8 +
1 bit) addressing.
AT25M01 uses 3-byte addressing (it needs 17 bits).

So I tend to believe EEPROMs using 16 + 1  or 24 + 1 address bits (with the
extra bit in the instruction byte) do not exist?

> ---
>  Documentation/devicetree/bindings/eeprom/at25.txt | 4 +++-
>  drivers/misc/eeprom/at25.c| 4 
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/eeprom/at25.txt 
> b/Documentation/devicetree/bindings/eeprom/at25.txt
> index 1d3447165c374f67..d00779e4ab4377b9 100644
> --- a/Documentation/devicetree/bindings/eeprom/at25.txt
> +++ b/Documentation/devicetree/bindings/eeprom/at25.txt
> @@ -6,7 +6,9 @@ Required properties:
>  - spi-max-frequency : max spi frequency to use
>  - pagesize : size of the eeprom page
>  - size : total eeprom size in bytes
> -- address-width : number of address bits (one of 8, 16, or 24)
> +- address-width : number of address bits (one of 8, 9, 16, 17, 24, or 25).
> +  For odd values, the MSB of the address is sent as bit 3 of the instruction
> +  byte, before the address byte(s).

Alternatively, we can drop the binding change, i.e. keep on using
address-width = <8> for 512-byte '040...

>  Optional properties:
>  - spi-cpha : SPI shifted clock phase, as per spi-bus bindings.
> diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
> index 5afe4cd165699060..a50a0f16fa0e1d1d 100644
> --- a/drivers/misc/eeprom/at25.c
> +++ b/drivers/misc/eeprom/at25.c
> @@ -275,6 +275,10 @@ static int at25_fw_to_chip(struct device *dev, struct 
> spi_eeprom *chip)
> "Error: missing \"address-width\" 
> property\n");
> return -ENODEV;
> }
> +   if (val & 1) {
> +   chip->flags |= EE_INSTR_BIT3_IS_ADDR;
> +   val -= 1;
> +   }

... and handle it here like:

if (chip->byte_len == 2U << val)
chip->flags |= EE_INSTR_BIT3_IS_ADDR;

However, that would IMHO be a bit confusing, as the "address-width"
property is no longer the real address width, but indicates how many bits
are specified in address bytes sent after the read/write command.
So "address-bytes" = 1, 2, or 3 would be more correct ;-)

Or deprecate this whole "specify parameters using DT properties" business,
and derive them from the compatible value. But that would mean adding a
large and ever growing table to an old driver...

Thoughts?

Thanks again!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 1/3] eeprom: at25: Add DT support for EEPROMs with odd address bits

[Geert Uytterhoeven]

On Thu, Nov 30, 2017 at 2:29 PM, Geert Uytterhoeven
 wrote:
> Certain EEPROMS have a size that is larger than the number of address
> bytes would allow, and store the MSB of the address in bit 3 of the
> instruction byte.
>
> This can be described in platform data using EE_INSTR_BIT3_IS_ADDR, or
> in DT using the obsolete legacy "at25,addr-mode" property.
> But currently there exists no non-deprecated way to describe this in DT.
>
> Hence extend the existing "address-width" DT property to allow
> specifying 9, 17, or 25 address bits, and enable support for that in the
> driver.
>
> Signed-off-by: Geert Uytterhoeven 
> ---
> EEPROMs using 9 address bits are common (e.g. M95040, 25AA040/25LC040).
> Do EEPROMs using 17 or 25 address bits, as mentioned in
> include/linux/spi/eeprom.h, really exist?
> Or should we just limit it to a single odd value (9 bits)?

At least for the real Atmel parts, only the AT25040 part uses odd (8 +
1 bit) addressing.
AT25M01 uses 3-byte addressing (it needs 17 bits).

So I tend to believe EEPROMs using 16 + 1  or 24 + 1 address bits (with the
extra bit in the instruction byte) do not exist?

> ---
>  Documentation/devicetree/bindings/eeprom/at25.txt | 4 +++-
>  drivers/misc/eeprom/at25.c| 4 
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/eeprom/at25.txt 
> b/Documentation/devicetree/bindings/eeprom/at25.txt
> index 1d3447165c374f67..d00779e4ab4377b9 100644
> --- a/Documentation/devicetree/bindings/eeprom/at25.txt
> +++ b/Documentation/devicetree/bindings/eeprom/at25.txt
> @@ -6,7 +6,9 @@ Required properties:
>  - spi-max-frequency : max spi frequency to use
>  - pagesize : size of the eeprom page
>  - size : total eeprom size in bytes
> -- address-width : number of address bits (one of 8, 16, or 24)
> +- address-width : number of address bits (one of 8, 9, 16, 17, 24, or 25).
> +  For odd values, the MSB of the address is sent as bit 3 of the instruction
> +  byte, before the address byte(s).

Alternatively, we can drop the binding change, i.e. keep on using
address-width = <8> for 512-byte '040...

>  Optional properties:
>  - spi-cpha : SPI shifted clock phase, as per spi-bus bindings.
> diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
> index 5afe4cd165699060..a50a0f16fa0e1d1d 100644
> --- a/drivers/misc/eeprom/at25.c
> +++ b/drivers/misc/eeprom/at25.c
> @@ -275,6 +275,10 @@ static int at25_fw_to_chip(struct device *dev, struct 
> spi_eeprom *chip)
> "Error: missing \"address-width\" 
> property\n");
> return -ENODEV;
> }
> +   if (val & 1) {
> +   chip->flags |= EE_INSTR_BIT3_IS_ADDR;
> +   val -= 1;
> +   }

... and handle it here like:

if (chip->byte_len == 2U << val)
chip->flags |= EE_INSTR_BIT3_IS_ADDR;

However, that would IMHO be a bit confusing, as the "address-width"
property is no longer the real address width, but indicates how many bits
are specified in address bytes sent after the read/write command.
So "address-bytes" = 1, 2, or 3 would be more correct ;-)

Or deprecate this whole "specify parameters using DT properties" business,
and derive them from the compatible value. But that would mean adding a
large and ever growing table to an old driver...

Thoughts?

Thanks again!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds