Erez <erezgeva2@xxxxxxxxx> writes: > On Thu, 11 Jul 2024 at 21:57, Michael Walle <mwalle@xxxxxxxxxx> wrote: >> On Thu Jul 11, 2024 at 8:57 PM CEST, Erez wrote: >> > Yes, I think we should. >> > >> > Reading the specification provided publicly by Macronix. >> > For all the JEDEC IDs with the no SFDP flag in drivers/mtd/spi-nor/macronix.c >> > All of them have a new version or a new chip with the same JEDEC ID >> > that supports SFDP. >> > There are 2 chips that Macronix does not provide spec. in public. >> > I can ask Macronix technical support on these 2 chips. >> >> We don't add flashes we cannot test. > > I did not suggest adding anything new. > I refer to the list of chips we already have in drivers/mtd/spi-nor/macronix.c > I presume someone tested them before adding them to the list in the past. > And probably the old chip did not have the SFDP table back then. > > What I checked with the chip specifications is that all Macronix chips > since 2010 have SFDP. > > The situation today is that all Macronix chips that are NOT in the > Macronix table work based on the SFDP table. > But new chips that use a JEDEC found in the Macronix table, skip the > SFDP table and use the setting of the old chip. Not entirely true. Those that entries in the Macronix table that has SPI_NOR_DUAL_READ and/or SPI_NOR_QUAD_READ in no_sfdp_flags is caught by the magic flags matching in spi_nor_init_params_deprecated() and will have spi_nor_parse_sfdp() called from spi_nor_sfdp_init_params_deprecated(). So flashes reusing ID for these will have the SFDP tables parsed. The rest of the entries in the Macronix table is not so lucky. When a replacement chip (with the same ID) is used, it will not be configured with the values found in SFDP table. > So I suggest we read the SFDP table for all Macronix chips. Based on their strategy of re-using flash ID, I think this is a sane approach. > Old Macronix chips that do not have SFDP will use the setting from the > Macronix table. i.e backward compatible. > While new chips which do have an SFDP table will work with the new > setting we find in the table. Yes, if we apply the new SPI_NOR_TRY_SFDP flag to the matching table entries. > Of course, we might have issues in parsing the SFDP table itself. > So we fix them as developers report and send chip ID and part number > with the SFDP table content. > I do not see the point of "hiding" with the old setting. > Anyhow, as we do not like the IDs table and keep it for backward-compatible, > so it only makes sense we should use the SFDP table as much as possible. > > My check was to ensure Tudor that all Macronix chips have SFDP whether > they are in the IDs table or not and we are not wasting a no-op on a > chip which can not have an SFDP table. > > All I suggest is we add the new 'SPI_NOR_TRY_SFDP' flag, to all > Macronix chips.. Which will try to read the SFDP to any Macronix chip. Makes sense. But obviously comes with a risk as we won't be able to test all chips for regression when doing that. /Esben
