| Message ID | 20231117083853.33329-7-jaimeliao.tw@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Michael Walle |
| Headers | show |
| Series | Add octal DTR support for Macronix flash | expand |
Hi, > diff --git a/drivers/mtd/spi-nor/macronix.c > b/drivers/mtd/spi-nor/macronix.c > index 48e570c04ad9..2115a25b21ce 100644 > --- a/drivers/mtd/spi-nor/macronix.c > +++ b/drivers/mtd/spi-nor/macronix.c > @@ -260,6 +260,27 @@ static const struct flash_info > macronix_nor_parts[] = { > .name = "mx66uw2g345gx0", > .n_banks = 4, > .flags = SPI_NOR_RWW, > + }, { > + .id = SNOR_ID(0xc2, 0x83, 0x39), > + .name = "mx25um25345g", > + }, { > + .id = SNOR_ID(0xc2, 0x80, 0x39), > + .name = "mx25um25645g", > + }, { > + .id = SNOR_ID(0xc2, 0x85, 0x39), > + .name = "mx25lm25645g", > + }, { > + .id = SNOR_ID(0xc2, 0x80, 0x3a), > + .name = "mx25um51245g", > + }, { > + .id = SNOR_ID(0xc2, 0x85, 0x3a), > + .name = "mx25lm51245g", > + }, { > + .id = SNOR_ID(0xc2, 0x80, 0x3b), > + .name = "mx66um1g45g", > + }, { > + .id = SNOR_ID(0xc2, 0x85, 0x3b), > + .name = "mx66lm1g45g", You need this because of the manufacturer fixup, correct? I'd like to avoid these "empty" entries if possible. The name is useless to the kernel and sometimes incorrect. Therefore, at least drop it and just list the IDs. Tudor, Pratyush, what do you think about calling the vendor fixups by just looking at the JEDEC manufacturer ID *iff* there is no entry in the flashdb? As a fallback so to speak. That would also help for the chip erase topic, because I'd presume the chip erase op is the same among the flashes of one vendor. So there could be a vendor fixup, to set the chip erase op. -michael
On 11/17/23 08:57, Michael Walle wrote: > Hi, > >> diff --git a/drivers/mtd/spi-nor/macronix.c >> b/drivers/mtd/spi-nor/macronix.c >> index 48e570c04ad9..2115a25b21ce 100644 >> --- a/drivers/mtd/spi-nor/macronix.c >> +++ b/drivers/mtd/spi-nor/macronix.c >> @@ -260,6 +260,27 @@ static const struct flash_info >> macronix_nor_parts[] = { >> .name = "mx66uw2g345gx0", >> .n_banks = 4, >> .flags = SPI_NOR_RWW, >> + }, { >> + .id = SNOR_ID(0xc2, 0x83, 0x39), >> + .name = "mx25um25345g", >> + }, { >> + .id = SNOR_ID(0xc2, 0x80, 0x39), >> + .name = "mx25um25645g", >> + }, { >> + .id = SNOR_ID(0xc2, 0x85, 0x39), >> + .name = "mx25lm25645g", >> + }, { >> + .id = SNOR_ID(0xc2, 0x80, 0x3a), >> + .name = "mx25um51245g", >> + }, { >> + .id = SNOR_ID(0xc2, 0x85, 0x3a), >> + .name = "mx25lm51245g", >> + }, { >> + .id = SNOR_ID(0xc2, 0x80, 0x3b), >> + .name = "mx66um1g45g", >> + }, { >> + .id = SNOR_ID(0xc2, 0x85, 0x3b), >> + .name = "mx66lm1g45g", > > You need this because of the manufacturer fixup, correct? I'd like to > avoid these "empty" entries if possible. The name is useless to the kernel > and sometimes incorrect. Therefore, at least drop it and just list the > IDs. > I agree. > Tudor, Pratyush, what do you think about calling the vendor fixups > by just looking at the JEDEC manufacturer ID *iff* there is no > entry in the flashdb? As a fallback so to speak. That would also > help for the chip erase topic, because I'd presume the chip erase > op is the same among the flashes of one vendor. So there could be > a vendor fixup, to set the chip erase op. > sounds good!
On 11/17/23 08:38, Jaime Liao wrote:
> Adding Macronix Octal flash for Octal DTR support.
Do all these flashes swap bytes on a 16bit boundary?
Do all these flashes correctly set BFPT_DWORD18_BYTE_ORDER_SWAPPED?
same questions on the previous patch.
Hi Jaime, [please keep the CC list in replies]. >> and sometimes incorrect. Therefore, at least drop it and just list the >> IDs. > Could I know patch 6/6 only or patch 5/6 should remove name as well? I'd say yes. Tudor? Pratyush? New flash additions without names? As, mentioned last time, for OF we might introduce an of_compatible (thats then ABI) - or - we can use the numeric ID in the device tree, as it's already used for PHY IDs or PCI IDs. Jaime, could you try to use /* Apply vendor fixups */ { .id = SNOR_ID(0xc2) } -michael
On 11/21/23 08:51, Michael Walle wrote: > Hi Jaime, > > [please keep the CC list in replies]. > >>> and sometimes incorrect. Therefore, at least drop it and just list the >>> IDs. >> Could I know patch 6/6 only or patch 5/6 should remove name as well? > > I'd say yes. Tudor? Pratyush? New flash additions without names? Why do you need a flash addition in the first place? Haven't we agreed that we'll apply the vendor fixup based on the manufacturer ID? Anyway, flash additions without names is fine by me. Cheers, ta > > As, mentioned last time, for OF we might introduce an of_compatible > (thats then ABI) - or - we can use the numeric ID in the device tree, > as it's already used for PHY IDs or PCI IDs. > > Jaime, could you try to use > > /* Apply vendor fixups */ > { .id = SNOR_ID(0xc2) } > > -michael
Hi > > > > On 11/21/23 08:51, Michael Walle wrote: > > Hi Jaime, > > > > [please keep the CC list in replies]. > > > >>> and sometimes incorrect. Therefore, at least drop it and just list the > >>> IDs. > >> Could I know patch 6/6 only or patch 5/6 should remove name as well? > > > > I'd say yes. Tudor? Pratyush? New flash additions without names? > > Why do you need a flash addition in the first place? Haven't we agreed > that we'll apply the vendor fixup based on the manufacturer ID? > > Anyway, flash additions without names is fine by me. > > Cheers, > ta > > > > As, mentioned last time, for OF we might introduce an of_compatible > > (thats then ABI) - or - we can use the numeric ID in the device tree, > > as it's already used for PHY IDs or PCI IDs. > > > > Jaime, could you try to use > > > > /* Apply vendor fixups */ > > { .id = SNOR_ID(0xc2) } I have validate some flash without "name" and looks good. But I got some log with (null) like below: SPI-NOR SPI0.0: (null) (32768 Kbytes) and zynq> cat jedec_id c2943c zynq> cat manufacturer macronix zynq> cat partname (null) Is this result acceptable to everyone? > > > > -michael Thanks Jaime
Hi, >> >>> and sometimes incorrect. Therefore, at least drop it and just list the >> >>> IDs. >> >> Could I know patch 6/6 only or patch 5/6 should remove name as well? >> > >> > I'd say yes. Tudor? Pratyush? New flash additions without names? >> >> Why do you need a flash addition in the first place? Haven't we agreed >> that we'll apply the vendor fixup based on the manufacturer ID? >> >> Anyway, flash additions without names is fine by me. >> >> Cheers, >> ta >> > >> > As, mentioned last time, for OF we might introduce an of_compatible >> > (thats then ABI) - or - we can use the numeric ID in the device tree, >> > as it's already used for PHY IDs or PCI IDs. >> > >> > Jaime, could you try to use >> > >> > /* Apply vendor fixups */ >> > { .id = SNOR_ID(0xc2) } > I have validate some flash without "name" and looks good. Great :) > But I got some log with (null) like below: > > SPI-NOR SPI0.0: (null) (32768 Kbytes) dev_info(dev, "%s (%lld Kbytes)\n", info->name ?: "", (long long)mtd->size >> 10); Maybe we should print the vendor and the name and both are optional? I don't have a strong opinion here as long as it's not "(null)" because that looks like a bug. > and > > zynq> cat jedec_id > c2943c > zynq> cat manufacturer > macronix > zynq> cat partname > (null) There is spi_nor_sysfs_is_visible() you can hide this property the same was as the manufacturer property is hidden. Also, please update Documentation/ABI/testing/sysfs-bus-spi-devices-spi-nor in this case: The attribute is optional. User space shouldn't rely on it to be present or even correct. Instead, user space should read the jedec_id attribute. -michael
diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c index 48e570c04ad9..2115a25b21ce 100644 --- a/drivers/mtd/spi-nor/macronix.c +++ b/drivers/mtd/spi-nor/macronix.c @@ -260,6 +260,27 @@ static const struct flash_info macronix_nor_parts[] = { .name = "mx66uw2g345gx0", .n_banks = 4, .flags = SPI_NOR_RWW, + }, { + .id = SNOR_ID(0xc2, 0x83, 0x39), + .name = "mx25um25345g", + }, { + .id = SNOR_ID(0xc2, 0x80, 0x39), + .name = "mx25um25645g", + }, { + .id = SNOR_ID(0xc2, 0x85, 0x39), + .name = "mx25lm25645g", + }, { + .id = SNOR_ID(0xc2, 0x80, 0x3a), + .name = "mx25um51245g", + }, { + .id = SNOR_ID(0xc2, 0x85, 0x3a), + .name = "mx25lm51245g", + }, { + .id = SNOR_ID(0xc2, 0x80, 0x3b), + .name = "mx66um1g45g", + }, { + .id = SNOR_ID(0xc2, 0x85, 0x3b), + .name = "mx66lm1g45g", } };