| Message ID | 20200424184410.8578-6-p.yadav@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: spi-nor: add xSPI Octal DTR support | expand |
On 2020/4/25 2:43, Pratyush Yadav wrote: > JESD216D.01 says that when the address width can be 3 or 4, it defaults > to 3 and enters 4-byte mode when given the appropriate command. So, when > we see a configurable width, default to 3 and let flash that default to > 4 change it in a post-bfpt fixup. > > This fixes SMPT parsing for flashes with configurable address width. If > the SMPT descriptor advertises variable address width, we use > nor->addr_width as the address width. But since it was not set to any > value from the SFDP table, the read command uses an address width of 0, > resulting in an incorrect read being issued. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > drivers/mtd/spi-nor/sfdp.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c > index f917631c8110..5cecc4ba2141 100644 > --- a/drivers/mtd/spi-nor/sfdp.c > +++ b/drivers/mtd/spi-nor/sfdp.c > @@ -460,6 +460,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, > /* Number of address bytes. */ > switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) { > case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY: > + case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4: > nor->addr_width = 3; > break; Should we also assign address width to 3 in default condition. At least we should not leave it uninitialized here. Regards, Yicong >
Hi Yicong, On 26/04/20 11:53AM, Yicong Yang wrote: > On 2020/4/25 2:43, Pratyush Yadav wrote: > > JESD216D.01 says that when the address width can be 3 or 4, it defaults > > to 3 and enters 4-byte mode when given the appropriate command. So, when > > we see a configurable width, default to 3 and let flash that default to > > 4 change it in a post-bfpt fixup. > > > > This fixes SMPT parsing for flashes with configurable address width. If > > the SMPT descriptor advertises variable address width, we use > > nor->addr_width as the address width. But since it was not set to any > > value from the SFDP table, the read command uses an address width of 0, > > resulting in an incorrect read being issued. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > drivers/mtd/spi-nor/sfdp.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c > > index f917631c8110..5cecc4ba2141 100644 > > --- a/drivers/mtd/spi-nor/sfdp.c > > +++ b/drivers/mtd/spi-nor/sfdp.c > > @@ -460,6 +460,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, > > /* Number of address bytes. */ > > switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) { > > case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY: > > + case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4: > > nor->addr_width = 3; > > break; > > Should we also assign address width to 3 in default condition. At least we should not > leave it uninitialized here. The default condition would be taken when this field is 3. The value 3 is reserved, and so no current device should use this value. That said, I don't see any downsides of doing so. If the value 3 means something else in later revisions of the standard, this code would need to change anyway. If not, we would use a relatively sane default for devices with a faulty BFPT. I haven't received any comments on my series so far. If end up having to re-roll it, I will add this change. Otherwise, I'm not sure if it is a good idea to re-roll a 16-patch series for a one liner that isn't fixing some major bug. In that case, maybe you can send an independent patch that does this after mine is merged?
Hi Pratyush, On 2020/4/28 1:23, Pratyush Yadav wrote: > Hi Yicong, > > On 26/04/20 11:53AM, Yicong Yang wrote: >> On 2020/4/25 2:43, Pratyush Yadav wrote: >>> JESD216D.01 says that when the address width can be 3 or 4, it defaults >>> to 3 and enters 4-byte mode when given the appropriate command. So, when >>> we see a configurable width, default to 3 and let flash that default to >>> 4 change it in a post-bfpt fixup. >>> >>> This fixes SMPT parsing for flashes with configurable address width. If >>> the SMPT descriptor advertises variable address width, we use >>> nor->addr_width as the address width. But since it was not set to any >>> value from the SFDP table, the read command uses an address width of 0, >>> resulting in an incorrect read being issued. >>> >>> Signed-off-by: Pratyush Yadav <p.yadav@ti.com> >>> --- >>> drivers/mtd/spi-nor/sfdp.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c >>> index f917631c8110..5cecc4ba2141 100644 >>> --- a/drivers/mtd/spi-nor/sfdp.c >>> +++ b/drivers/mtd/spi-nor/sfdp.c >>> @@ -460,6 +460,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, >>> /* Number of address bytes. */ >>> switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) { >>> case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY: >>> + case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4: >>> nor->addr_width = 3; >>> break; >> Should we also assign address width to 3 in default condition. At least we should not >> leave it uninitialized here. > The default condition would be taken when this field is 3. The value 3 > is reserved, and so no current device should use this value. That said, > I don't see any downsides of doing so. If the value 3 means something > else in later revisions of the standard, this code would need to change > anyway. If not, we would use a relatively sane default for devices with > a faulty BFPT. The purpose is to set one possible value which may be used later in parsing smpt. In current driver, if we do nothing with the post-bfpt fixup, then the width will be unset. Otherwise, maybe the width can also be set in spi_nor_smpt_addr_width() default: + if (!nor->addr_width) + nor->addr_width = 3; return nor->addr_width; But set when parsing bfpt seems more reasonable. > I haven't received any comments on my series so far. If end up having to > re-roll it, I will add this change. Otherwise, I'm not sure if it is a > good idea to re-roll a 16-patch series for a one liner that isn't fixing > some major bug. In that case, maybe you can send an independent patch > that does this after mine is merged? Fine. :) Regards, Yicong
On Tuesday, April 28, 2020 4:34:46 AM EEST Yicong Yang wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Hi Pratyush, > > On 2020/4/28 1:23, Pratyush Yadav wrote: > > Hi Yicong, > > > > On 26/04/20 11:53AM, Yicong Yang wrote: > >> On 2020/4/25 2:43, Pratyush Yadav wrote: > >>> JESD216D.01 says that when the address width can be 3 or 4, it defaults > >>> to 3 and enters 4-byte mode when given the appropriate command. So, when > >>> we see a configurable width, default to 3 and let flash that default to > >>> 4 change it in a post-bfpt fixup. > >>> > >>> This fixes SMPT parsing for flashes with configurable address width. If > >>> the SMPT descriptor advertises variable address width, we use > >>> nor->addr_width as the address width. But since it was not set to any > >>> value from the SFDP table, the read command uses an address width of 0, > >>> resulting in an incorrect read being issued. > >>> > >>> Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > >>> --- > >>> > >>> drivers/mtd/spi-nor/sfdp.c | 1 + > >>> 1 file changed, 1 insertion(+) > >>> > >>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c > >>> index f917631c8110..5cecc4ba2141 100644 > >>> --- a/drivers/mtd/spi-nor/sfdp.c > >>> +++ b/drivers/mtd/spi-nor/sfdp.c > >>> @@ -460,6 +460,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, > >>> > >>> /* Number of address bytes. */ > >>> switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) > >>> { > >>> > >>> case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY: > >>> + case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4: > >>> nor->addr_width = 3; > >>> break; > >> > >> Should we also assign address width to 3 in default condition. At least > >> we should not leave it uninitialized here. > > > > The default condition would be taken when this field is 3. The value 3 > > is reserved, and so no current device should use this value. That said, > > I don't see any downsides of doing so. If the value 3 means something > > else in later revisions of the standard, this code would need to change > > anyway. If not, we would use a relatively sane default for devices with > > a faulty BFPT. > > The purpose is to set one possible value which may be used later in parsing > smpt. In current driver, if we do nothing with the post-bfpt fixup, then > the width will be unset. Otherwise, maybe the width can also be set in > spi_nor_smpt_addr_width() > > default: > + if (!nor->addr_width) > + nor->addr_width = 3; > return nor->addr_width; > > But set when parsing bfpt seems more reasonable. Please don't. Any deviations from the standard should be addressed with fixup hooks. > > > I haven't received any comments on my series so far. If end up having to Thanks for the patience, they are in my todo list, I will get soon to them. Cheers, ta
diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c index f917631c8110..5cecc4ba2141 100644 --- a/drivers/mtd/spi-nor/sfdp.c +++ b/drivers/mtd/spi-nor/sfdp.c @@ -460,6 +460,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, /* Number of address bytes. */ switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) { case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY: + case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4: nor->addr_width = 3; break;
JESD216D.01 says that when the address width can be 3 or 4, it defaults to 3 and enters 4-byte mode when given the appropriate command. So, when we see a configurable width, default to 3 and let flash that default to 4 change it in a post-bfpt fixup. This fixes SMPT parsing for flashes with configurable address width. If the SMPT descriptor advertises variable address width, we use nor->addr_width as the address width. But since it was not set to any value from the SFDP table, the read command uses an address width of 0, resulting in an incorrect read being issued. Signed-off-by: Pratyush Yadav <p.yadav@ti.com> --- drivers/mtd/spi-nor/sfdp.c | 1 + 1 file changed, 1 insertion(+)