On Thu, Nov 30, 2017 at 2:29 PM, Geert Uytterhoeven
<[email protected]> 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 <[email protected]>
> ---
> 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 -- [email protected]
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
On Mon, Dec 04, 2017 at 10:17:47AM +0100, Geert Uytterhoeven wrote:
> On Thu, Nov 30, 2017 at 2:29 PM, Geert Uytterhoeven
> <[email protected]> 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 <[email protected]>
> > ---
> > 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
Hi Geert,
My 2 cents:
2017-12-04 10:17 GMT+01:00 Geert Uytterhoeven <[email protected]>:
>> 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
Hi Rob,
On Mon, Dec 4, 2017 at 10:17 PM, Rob Herring <[email protected]> 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
>> <[email protected]> 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 <[email protected]>
>> > ---
>> > 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 -- [email protected]
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
Hi Ivo,
On Mon, Dec 4, 2017 at 11:00 PM, Ivo Sieben <[email protected]> wrote:
> 2017-12-04 10:17 GMT+01:00 Geert Uytterhoeven <[email protected]>:
>>> 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 -- [email protected]
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
Hi Rob,
On Tue, Dec 5, 2017 at 9:57 AM, Geert Uytterhoeven <[email protected]> wrote:
> On Mon, Dec 4, 2017 at 10:17 PM, Rob Herring <[email protected]> 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
>>> <[email protected]> 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 <[email protected]>
>>> > ---
>>> > 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 -- [email protected]
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
On Tue, Dec 5, 2017 at 3:09 AM, Geert Uytterhoeven <[email protected]> wrote:
> Hi Rob,
>
> On Tue, Dec 5, 2017 at 9:57 AM, Geert Uytterhoeven <[email protected]> wrote:
>> On Mon, Dec 4, 2017 at 10:17 PM, Rob Herring <[email protected]> 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
>>>> <[email protected]> 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 <[email protected]>
>>>> > ---
>>>> > 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
Hi Rob,
On Tue, Dec 5, 2017 at 2:56 PM, Rob Herring <[email protected]> wrote:
> On Tue, Dec 5, 2017 at 3:09 AM, Geert Uytterhoeven <[email protected]> wrote:
>> On Tue, Dec 5, 2017 at 9:57 AM, Geert Uytterhoeven <[email protected]> wrote:
>>> On Mon, Dec 4, 2017 at 10:17 PM, Rob Herring <[email protected]> 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
>>>>> <[email protected]> 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 <[email protected]>
>>>>> > ---
>>>>> > 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 -- [email protected]
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