Re: [PATCH] mtd: spi-nor: boya: add support for boya by25q128as

[<Tudor.Ambarus@microchip.com>]

On 4/6/21 7:11 PM, George Brooke wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi again Tudor,

Hey,

> I've been busy with other things but I sat down today to have a
> hard
> look into this.

Cool, thanks for the documentation work!
> 
> Tudor.Ambarus@microchip.com writes:
> 
>> On 2/13/21 7:10 PM, George Brooke wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless
>>> you know the content is safe
>>>
>>> Hello Tudor,
>>
>> Hi, George,
>>
>> Sorry for the long delay :(. I added in To: Chris and Andreas,
>> they
>> encounter a similar problem with other vendors.
>>
>>>
>>> Tudor.Ambarus@microchip.com writes:
>>>
>>>> Hi, George,
>>>>
>>>> On 2/7/21 2:30 PM, George Brooke wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless
>>>>> you know the content is safe
>>>>>
>>>>> Adds support for the Boya Microelectronics BY25Q128AS 128
>>>>> Mbit
>>>>> flash.
>>>>> I tested this on the Creality WB-01 embedded device which
>>>>> uses
>>>>> this,
>>>>> although that was with OpenWrt which is still using 5.4 so I
>>>>> had to
>>>>> do a bit of porting work. Don't see how that would make much
>>>>> of
>>>>> a
>>>>> difference though.
>>>>>
>>>>> Signed-off-by: George Brooke <figgyc@figgyc.uk>
>>>>> ---
>>>>>  drivers/mtd/spi-nor/Makefile |  1 +
>>>>>  drivers/mtd/spi-nor/boya.c   | 23 +++++++++++++++++++++++
>>>>>  drivers/mtd/spi-nor/core.c   |  1 +
>>>>>  drivers/mtd/spi-nor/core.h   |  1 +
>>>>>  4 files changed, 26 insertions(+)
>>>>>  create mode 100644 drivers/mtd/spi-nor/boya.c
>>>>>
>>>>> diff --git a/drivers/mtd/spi-nor/Makefile
>>>>> b/drivers/mtd/spi-nor/Makefile
>>>>> index 653923896205..7d1551fbfbaa 100644
>>>>> --- a/drivers/mtd/spi-nor/Makefile
>>>>> +++ b/drivers/mtd/spi-nor/Makefile
>>>>> @@ -2,6 +2,7 @@
>>>>>
>>>>>  spi-nor-objs                   := core.o sfdp.o
>>>>>  spi-nor-objs                   += atmel.o
>>>>> +spi-nor-objs                   += boya.o
>>>>>  spi-nor-objs                   += catalyst.o
>>>>>  spi-nor-objs                   += eon.o
>>>>>  spi-nor-objs                   += esmt.o
>>>>> diff --git a/drivers/mtd/spi-nor/boya.c
>>>>> b/drivers/mtd/spi-nor/boya.c
>>>>> new file mode 100644
>>>>> index 000000000000..014b0087048a
>>>>> --- /dev/null
>>>>> +++ b/drivers/mtd/spi-nor/boya.c
>>>>> @@ -0,0 +1,23 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +/*
>>>>> + * Copyright (C) 2005, Intec Automation Inc.
>>>>> + * Copyright (C) 2014, Freescale Semiconductor, Inc.
>>>>> + */
>>>>> +
>>>>> +#include <linux/mtd/spi-nor.h>
>>>>> +
>>>>> +#include "core.h"
>>>>> +
>>>>> +static const struct flash_info boya_parts[] = {
>>>>> +       /* Boya */
>>>>> +       { "by25q128as", INFO(0x684018, 0, 64 * 1024, 256,
>>>>
>>>> The manufacturer’s identification code is defined by one or
>>>> more
>>>> eight (8) bit fields each consisting of seven (7) data bits
>>>> plus
>>>> one (1)
>>>> odd parity bit. It is a single field limiting the possible
>>>> number of
>>>> vendors to 126. To expand the maximum number of identification
>>>> codes a
>>>> continuation scheme has been defined.
>>>>
>>>> According to JEP106BA, the manufacturer ID for Boya should be
>>>> preceded by
>>>> eight continuation codes. So I would expect the manufacturer
>>>> ID
>>>> for this
>>>> flash to be: 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f,
>>>> 0x68.
>>>>
>>>> Without the continuation codes, we will have collisions
>>>> between
>>>> manufacturer IDs, Convex Computer being an example.
>>>>
>>>> I see that the datasheet [1] for this flash doesn't specify
>>>> anything
>>>> about the continuation codes, so I suspect that Boya just got
>>>> it
>>>> wrong.
>>>
>>> It appears you are right. I thought it would be the best idea
>>> to
>>> actually
>>> interact with the flash chip and read its responses, so I found
>>> a
>>> tool
>>> called spincl [2] to send some commands to it with a Raspberry
>>> Pi:
>>>
>>> $ spincl -ib -m0 -c0 -s0 -p0 6 0x90
>>> 0x00 0x00 0x00 0x00 0x68 0x17
>>
>> I wasn't aware of the 0x90 command, it's not in the
>> include/linux/mtd/spi-nor.h either. Do you know what 0x17 means?
>>
> 0x90 (and 0x92 for dual spi, 0x94 for quad spi) seems to be in
> Winbond
> chips [5], Boya chips [1], the ESMT chip [4] and an XMC chip [6],
> from
> the datasheets I've looked at so far.
> They all seem to implement it in the same way: a "24 bit address"
> which
> is just a flag for device ID first or manufacturer ID first, then
> output
> one and the other repeatedly.
> 
> It isn't in the Micron MT25QU128ABA [7] or the Atmel AT25FS010 [8]
> at
> the very least, so it's not universal.
> 
> Interestingly the description of 0x90 in the XMC datasheet is
> suspiciously similar to the one in the Winbond datasheet.
> 
> 0x17 is just listed in the BY25Q128AS datasheet in the "ID
> Definition
> Table"; I'm fairly sure it's just arbitrarily decided by the
> vendor for
> each chip type.
> 
>>> $ spincl -ib -m0 -c0 -s0 -p0 4 0x9F
>>> 0x00 0x68 0x40 0x18
>>> and indeed there doesn't seem to be any continuation codes.
>>>
>>>> We'll have to check other datasheets from Boya and see if they
>>>> got
>>>> their manufacturer ID wrong for all their flashes. We'll have
>>>> to
>>>> add
>>>
>>> The BY25Q64AS [3] appears to be described similarly.
>>>
>>>> some fixup mechanism for the manufacturers ID handling, in
>>>> order
>>>> to
>>>> avoid collisions with other manufacturers IDs.
>>>
>>> I looked into this a bit more, and what I'm realising is that
>>> I'm
>>> not sure if
>>> there even is any mechanism to deliver the continuation codes
>>> within the
>>> base SPI-NOR standard? Take esmt.c: the f25l32qa has a device
>>> id
>>> 0x8c4116.
>>> JEP106BA attribytes 8c to Monolithic Memories in the first
>>> bank,
>>> while
>>> Elite Flash Storage (presumably an alias of ESMT) should be
>>> identifying
>>> as 0x7f, 0x7f, 0x7f, 0x8c. Its datasheet [4] appears to be
>>> equally
>>> sparse on
>>> detail. To my untrained eye, this seems to be the exact same
>>> situation we
>>> find ourselves in here. (You probably know a lot more about
>>> this
>>> then I do -
>>> if I'm wrong do point it out!)
>>
>> I don't, but on a quick search I see that Elite Flash Storage
>> may be
>> an alias for ESMT, so probably you're right. I wasn't aware of
>> the
>> esmt problem, thanks for pointing it out.
>>
>>>
>>> That said I can't seem to find any formalised definition of
>>> what
>>> the 0x90
>>> "manufacturer and device ID" command is actually supposed to do
>>> in
>>> the case of
>>> a manufacturer ID that isn't in the first bank. Likewise with
>>> 0x9f
>>> "JEDEC ID".
>>
>> I don't know if there's a standardized definition. Maybe we can
>> shuffle
>> through datasheets and check what manufacturers are saying about
>> this.
>>
> I decided to look at all the manufacturer IDs currently in spi-nor
> to
> see if there is any consensus on the handling of device ID
> commands.
> 
> In terms of manufacturer ID banks:
> - Catalyst and Everspin are non-JEDEC
> - Atmel, Fujitsu, Macronix, SST, Micron/ST, Spansion/Cypress,
>  Winbond
> use manufacturer codes in the first bank, so no special handling
> is
> expected
> - EON, ESMT, GigaDevice, ISSI and XMC are all in banks other than
>  the

I have an ISSI flash and a GD somewhere around here, I'll give them
a try and check if they define the continuation codes.

> first bank, so basically the same situation as I mentioned with
> ESMT
> in the previous email.
> 
> There's two especially interesting cases:
> 
> - Xilinx S3AN flashes use 0x1f, despite 0x1f bank 1 is
>  Atmel/Adesto.
> In fact, 3S700AN has device ID 0x1f2500 which is collides with
> Atmel
> at45db081d, for example. I did some digging and its user guide
> [10] says
> system flash is "architecturally similar to the Adesto DataFlash
> SPI
> Flash memories" of which it shares ID numbers, so this is not a
> collision but just the IP is licensed and they didn't change the
> ID.

oh, the horror. Similar is not identical, I wonder if there are any
differences between the two or not.

> In fact the AT45DB081D also has [11] the strange 264-byte pages
> option
> in the Xilinx driver, so they might actually be functionally
> identical.

We'll have to investigate this thoroughly. Anyway, your findings are a
very good starting point.

> 
> - XMC flashes start 0x20. XMC has 0x20 in bank 10, but 0x20 bank 1
>  is
> actually owned by Micron/ST who also make spi-nor flash chips.
> There
> don't seem to be any direct ID collisions yet though thankfully.
> 
> As a result I was hoping XMC chips may have some sort of
> continuation
> code handling but they don't seem to.
> For instance XM25QH64A [6]: the datasheet does specify fairly
> clearly
> that the manufacturer ID is only one byte in both 0x90 and 0x9f.
> I think this is just an unfortunate coincidence and not a
> relabeling
> since Micron chips don't have 0x90.
> 
> It seems like every 0x9f implementation I have looked at only
> outputs the last byte of the manufacturer ID code, no
> continuations.
> The AT45DB081D datasheet does actually mention the existence of
> JEP106
> continuation codes but 0x1f is in the first bank so they do not
> actually
> exist on that device.

OK. All your findings reveal a bigger problem that I've anticipated.

> 
>>> Do you know of any flashes made by companies not in the first
>>> bank
>>> that
>>> identify themselves correctly?
>>
>> No, I've worked only with flashes that have their ID in the
>> first bank.
>>
>>>
>>> As far as I can tell the only proper method available is in
>>> SFDP,
>>> where the
>>> Parameter ID of a vendor specific table would be the bank
>>> number
>>> in the MSB
>>> and the actual manufacturer code in the LSB. However, this is a
>>> very
>>> over-engineered solution, wouldn't work on devices with no
>>> vendor
>>> specific
>>> tables, and in this case, I couldn't even get the SFDP table to
>>> read out of
>>> the by25q128as at all:
>>> $ spincl -ib -m0 -c0 -s0 -p0 32 0x5A
>>> 0x00 0x00 0x00 0x00 0x00 0xff 0xff 0xff 0xff 0xff 0xff 0xff
>>> 0xff
>>> 0xff 0xff ...
>>> (This may very well be a peculiarity of the way I'm accessing
>>> it,
>>> but in any
>>> case I don't think this fix would be a very good idea.)
>>
>> yeah, maybe the tool is broken, because I see that the SFDP
>> table
>> is defined in the datasheet. The manufacturer specific table
>> identification looks broken too. MSB should have been 0x08 and
>> LSB
>> 0x68, whereas they have for MSB 0xff and LSB 0x0b? We can hack
>>
> I think you're confusing my chip with the XTX chip Chris is
> looking at.
> It [9] has a 0xff 0x0b table. I can't see the SFDP table in my
> datasheet?

oh, yes, you're right. I was talking about [9].

>> the sfdp read to dump the tables. Or use Michael's sfdp dump
>> sysfs
>> patches:
>>
>> https://patchwork.ozlabs.org/project/linux-mtd/list/?series=235475
>>
> Good idea, I can try and do some device tree so I can connect to
> my mtd
> chip on a raspberry pi on a recent kernel to see if I can run
> Michael's
> patches. That'd take me a while though so I'll get back to you on
> that.
> Not that I'm particularly sure it would help our situation very
> much.
> I also see Chris has dumped his while I was writing this email and
> it
> does seem to match with the XTX datasheet.
> 
>>>
>>> I'm not sure what would actually make a good fixup in this
>>> scenario. We'd need
>>> something that could differentiate a Boya chip from any other
>>> hypothetical
>>> 0x68 manufacturer flash.
>>> Maybe even the missing SFDP output would make a good detection
>>> routine, but
>>> that would need testing with a better SPI inspection method and
>>> across multiple
>>> flash chips because I doubt that's intentional - surely if SFDP
>>> didn't work it
>>> would just not be described in the datasheet. Probably better
>>> to
>>> think of
>>> something other than that though.
>>>
>>
>> The first collision between a flash from the first bank and any
>> other
>> flashes from the rest of the banks can be avoided if we move all
>> the
>> manufacturers with continuation codes at the end of the
>> manufacturers
>> array, so that at the flash detection code they are treated as
>> last.
>>
>> Collision between flashes with continuation codes is more
>> difficult
>> to solve and would require differentiation at run-time. Will
>> have to
>> check jesd216 to see if there's other unique method to identify
>> a flash.
>> Can you check it?
>>
> I've read through the entirety of JESD216D.01 and JESD251A and I'm
> pretty sure SFDP vendor-specific tables are the only place in the
> entire
> standard where manufacturer ID codes are mentioned at all.
> 
> It seems kind of crazy to me that our main mechanism of flash ID
> isn't
> standardised anywhere, despite the fact that almost every chip
> supports
> it in a similar way. I really feel like I must be missing
> something but
> at this point I wouldn't know where else to look.

I'll duplicate the effort and check the standards too. If we'll
not be able to identify a flash identification mechanism, then
we can try to contact the jedec committee, describe the problem,
and ask for feedback.

> 
> I am led to the conclusion that so far, the common solution to
> JEP106
> continuation codes and banks is to pretend they don't exist and
> hope
> nobody has any collisions. That basically just leaves us to
> distinguishing between manufacturers using unique commands.
> 
> 0x90 might be a good command for that? It's semi vendor specific
> and has> some sort of unique system, but it still doesn't properly
> implement
> JEDEC manufacturer IDs so it definitely doesn't rule out the
> possibility
> of collision, and it won't work universally.
> The SFDP vendor-specific tables solution also technically exists
> but the
> XTX chip shows that's probably not the best idea. Overall this
> doesn't
> seem very ideal and I'm not confident on what the best solution
> is.

I'll have to do some documentation work, and then maybe we'll try to
contact JEDEC JC-42.4. But if things are as bad as we find right now,
I would say that we can deal with this in an incremental way, as follows:
1/ move all the manufacturers with continuation codes at the end of the
manufacturers array -> we'll avoid the collisions between the flashes
without continuation codes and those with continuation codes.
2/ for collisions between flashes with continuation codes we'll try to
differentiate between them at run-time (sfdp, vendor specific commands, etc)
3/ if we can't find a run-time differentiator, we'll be forced to introduce
compatibles with the flash names, which I don't like, but ...

I'll think more about this, maybe I'll come with something better. I'll try
the issi and gd and let you know about the outcome. Also about if I find
something else in the standards. Michael, Pratyush and Vignesh my intervene
too. I'll raise this to their attention.

Cheers,
ta

> 
> Thanks,
> figgyc
> 
> [5]
> https://cdn.datasheetspdf.com/pdf-down/W/2/5/W25Q128BV_Winbond.pdf
> [6] http://www.xmcwh.com/Uploads/2018-02-05/5a77d2cf60e04.pdf
> [7]
> https://media-www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25q/die-rev-a/mt25q_qlhs_u_128_aba_0.pdf
> [8]
> https://media.digikey.com/pdf/Data%20Sheets/Atmel%20PDFs/AT25FS010.pdf
> [9]
> https://datasheet.lcsc.com/szlcsc/2005251034_XTX-XT25F128BSSIGT_C558844.pdf
> [10]
> https://www.xilinx.com/support/documentation/user_guides/ug333.pdf
> [11] https://www.adestotech.com/wp-content/uploads/doc3596.pdf
> 
>>
>> Cheers,
>> ta
>>
>>> Thanks,
>>> figgyc
>>>
>>> [2] https://github.com/CrosseyeJack/spincl
>>> [3]
>>> https://datasheet.lcsc.com/szlcsc/1904091402_BOYAMICRO-BY25Q64ASSIG_C383793.pdf
>>> [4]
>>> https://www.esmt.com.tw/upload/pdf/ESMT/datasheets/F25L32QA.pdf
>>>
>>>>
>>>> Cheers,
>>>> ta
>>>>
>>>> [1]
>>>> https://datasheet.lcsc.com/szlcsc/1904091402_BOYAMICRO-BY25Q128ASSIG_C383794.pdf
>>>>
>>>>> +                       SECT_4K | SPI_NOR_DUAL_READ |
>>>>> SPI_NOR_QUAD_READ |
>>>>> +                       SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>>>>> +       },
>>>>> +};
>>>>> +
>>>>> +const struct spi_nor_manufacturer spi_nor_boya = {
>>>>> +       .name = "boya",
>>>>> +       .parts = boya_parts,
>>>>> +       .nparts = ARRAY_SIZE(boya_parts),
>>>>> +};
>>>>> diff --git a/drivers/mtd/spi-nor/core.c
>>>>> b/drivers/mtd/spi-nor/core.c
>>>>> index 20df44b753da..4d0d003e9c3f 100644
>>>>> --- a/drivers/mtd/spi-nor/core.c
>>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>>> @@ -2160,6 +2160,7 @@ int spi_nor_sr2_bit7_quad_enable(struct
>>>>> spi_nor *nor)
>>>>>
>>>>>  static const struct spi_nor_manufacturer *manufacturers[] =
>>>>>  {
>>>>>         &spi_nor_atmel,
>>>>> +       &spi_nor_boya,
>>>>>         &spi_nor_catalyst,
>>>>>         &spi_nor_eon,
>>>>>         &spi_nor_esmt,
>>>>> diff --git a/drivers/mtd/spi-nor/core.h
>>>>> b/drivers/mtd/spi-nor/core.h
>>>>> index d631ee299de3..d5ed5217228b 100644
>>>>> --- a/drivers/mtd/spi-nor/core.h
>>>>> +++ b/drivers/mtd/spi-nor/core.h
>>>>> @@ -409,6 +409,7 @@ struct spi_nor_manufacturer {
>>>>>
>>>>>  /* Manufacturer drivers. */
>>>>>  extern const struct spi_nor_manufacturer spi_nor_atmel;
>>>>> +extern const struct spi_nor_manufacturer spi_nor_boya;
>>>>>  extern const struct spi_nor_manufacturer spi_nor_catalyst;
>>>>>  extern const struct spi_nor_manufacturer spi_nor_eon;
>>>>>  extern const struct spi_nor_manufacturer spi_nor_esmt;
>>>>> -- 
>>>>> 2.30.0
>>>>>
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/