Message ID | ccff78b42bab8d6eb0ffa2127f4fe6c4daddf6af.1652063152.git.Takahiro.Kuwano@infineon.com |
---|---|
State | Superseded |
Delegated to: | Pratyush Yadav |
Headers | show |
Series | mtd: spi-nor: Add support for Infineon s25hl-t/s25hs-t | expand |
Am 2022-05-10 00:10, schrieb tkuw584924@gmail.com: > From: Tudor Ambarus <tudor.ambarus@microchip.com> > > At the SFDP parsing time we should not change members of struct > spi_nor, > but instead fill members of struct spi_nor_flash_parameters which could > leter on be used by callers. The caller will then decide if SFDP params > should be used and more importantly when they should be used. Clean the > code flow and don't initialize nor->addr_nbytes at SFDP parsing time. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > Tested-By: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> Nice! Reviewed-by: Michael Walle <michael@walle.cc>
On 10/05/22 07:10AM, tkuw584924@gmail.com wrote: > From: Tudor Ambarus <tudor.ambarus@microchip.com> > > At the SFDP parsing time we should not change members of struct spi_nor, > but instead fill members of struct spi_nor_flash_parameters which could > leter on be used by callers. The caller will then decide if SFDP params s/leter/later/ > should be used and more importantly when they should be used. Clean the > code flow and don't initialize nor->addr_nbytes at SFDP parsing time. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > Tested-By: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> > --- > drivers/mtd/spi-nor/core.c | 5 ++--- > drivers/mtd/spi-nor/core.h | 2 ++ > drivers/mtd/spi-nor/sfdp.c | 18 ++++++------------ > 3 files changed, 10 insertions(+), 15 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 7db6b41d7c30..dd71deba9f11 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -2272,8 +2272,8 @@ static int spi_nor_default_setup(struct spi_nor *nor, > > static int spi_nor_set_addr_nbytes(struct spi_nor *nor) > { > - if (nor->addr_nbytes) { > - /* already configured from SFDP */ > + if (nor->params->addr_nbytes) { > + nor->addr_nbytes = nor->params->addr_nbytes; > } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) { > /* > * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So > @@ -2518,7 +2518,6 @@ static void spi_nor_sfdp_init_params_deprecated(struct spi_nor *nor) > > if (spi_nor_parse_sfdp(nor)) { > memcpy(nor->params, &sfdp_params, sizeof(*nor->params)); > - nor->addr_nbytes = 0; > nor->flags &= ~SNOR_F_4B_OPCODES; > } > } > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > index fe7683fe1b4d..41df8bc8e008 100644 > --- a/drivers/mtd/spi-nor/core.h > +++ b/drivers/mtd/spi-nor/core.h > @@ -345,6 +345,7 @@ struct spi_nor_otp { > * @writesize Minimal writable flash unit size. Defaults to 1. Set to > * ECC unit size for ECC-ed flashes. > * @page_size: the page size of the SPI NOR flash memory. > + * @addr_nbytes: number of address bytes to send. > * @rdsr_dummy: dummy cycles needed for Read Status Register command > * in octal DTR mode. > * @rdsr_addr_nbytes: dummy address bytes needed for Read Status Register > @@ -377,6 +378,7 @@ struct spi_nor_flash_parameter { > u64 size; > u32 writesize; > u32 page_size; > + u8 addr_nbytes; > u8 rdsr_dummy; > u8 rdsr_addr_nbytes; > > diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c > index 058ce218d2af..90d7c25f7281 100644 > --- a/drivers/mtd/spi-nor/sfdp.c > +++ b/drivers/mtd/spi-nor/sfdp.c > @@ -178,19 +178,16 @@ static int spi_nor_read_raw(struct spi_nor *nor, u32 addr, size_t len, u8 *buf) > static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr, > size_t len, void *buf) > { > - u8 addr_nbytes; > int ret; > > - addr_nbytes = nor->addr_nbytes; > - > nor->read_opcode = SPINOR_OP_RDSFDP; > nor->addr_nbytes = 3; > nor->read_dummy = 8; > > ret = spi_nor_read_raw(nor, addr, len, buf); > > - nor->addr_nbytes = addr_nbytes; > /* Restore setup to its uninitialized state. */ > + nor->addr_nbytes = 0; > nor->read_opcode = 0; > nor->read_dummy = 0; > > @@ -461,11 +458,11 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, > 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_nbytes = 3; > + params->addr_nbytes = 3; > break; > > case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY: > - nor->addr_nbytes = 4; > + params->addr_nbytes = 4; > break; > > default: > @@ -652,7 +649,7 @@ static u8 spi_nor_smpt_addr_nbytes(const struct spi_nor *nor, const u32 settings > return 4; > case SMPT_CMD_ADDRESS_LEN_USE_CURRENT: > default: > - return nor->addr_nbytes; > + return nor->params->addr_nbytes; > } > } > > @@ -689,7 +686,6 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt, > u32 addr; > int err; > u8 i; > - u8 addr_nbytes; > u8 read_data_mask, map_id; > > /* Use a kmalloc'ed bounce buffer to guarantee it is DMA-able. */ > @@ -697,8 +693,6 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt, > if (!buf) > return ERR_PTR(-ENOMEM); > > - addr_nbytes = nor->addr_nbytes; > - > map_id = 0; > /* Determine if there are any optional Detection Command Descriptors */ > for (i = 0; i < smpt_len; i += 2) { > @@ -753,8 +747,8 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt, > /* fall through */ > out: > kfree(buf); > - nor->addr_nbytes = addr_nbytes; > /* Restore setup to its uninitialized state. */ > + nor->addr_nbytes = 0; Same as before, I don't think this function should know or care about the default or uninitialised values. > nor->read_dummy = 0; > nor->read_opcode = 0; > return ret; > @@ -1096,7 +1090,7 @@ static int spi_nor_parse_4bait(struct spi_nor *nor, > * Spansion memory. However this quirk is no longer needed with new > * SFDP compliant memories. > */ > - nor->addr_nbytes = 4; > + params->addr_nbytes = 4; Patch looks good at first glance but I have not looked very carefully if it might cause some small issues. > nor->flags |= SNOR_F_4B_OPCODES | SNOR_F_HAS_4BAIT; > > /* fall through */ > -- > 2.25.1 > issi.c's is25lp256_post_bfpt_fixups() also sets nor->addr_nbytes. Should that be updated to use params->addr_nbytes instead?
On 5/31/22 14:30, Pratyush Yadav wrote: > issi.c's is25lp256_post_bfpt_fixups() also sets nor->addr_nbytes. Should > that be updated to use params->addr_nbytes instead? Yes, it should, good catch! I've revisited this patch, otherwise it looks good. ta
On 5/31/22 14:30, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 10/05/22 07:10AM, tkuw584924@gmail.com wrote: >> From: Tudor Ambarus <tudor.ambarus@microchip.com> >> >> At the SFDP parsing time we should not change members of struct spi_nor, >> but instead fill members of struct spi_nor_flash_parameters which could >> leter on be used by callers. The caller will then decide if SFDP params > > s/leter/later/ > >> should be used and more importantly when they should be used. Clean the >> code flow and don't initialize nor->addr_nbytes at SFDP parsing time. >> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >> Tested-By: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> >> --- >> drivers/mtd/spi-nor/core.c | 5 ++--- >> drivers/mtd/spi-nor/core.h | 2 ++ >> drivers/mtd/spi-nor/sfdp.c | 18 ++++++------------ >> 3 files changed, 10 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >> index 7db6b41d7c30..dd71deba9f11 100644 >> --- a/drivers/mtd/spi-nor/core.c >> +++ b/drivers/mtd/spi-nor/core.c >> @@ -2272,8 +2272,8 @@ static int spi_nor_default_setup(struct spi_nor *nor, >> >> static int spi_nor_set_addr_nbytes(struct spi_nor *nor) >> { >> - if (nor->addr_nbytes) { >> - /* already configured from SFDP */ >> + if (nor->params->addr_nbytes) { >> + nor->addr_nbytes = nor->params->addr_nbytes; >> } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) { >> /* >> * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So >> @@ -2518,7 +2518,6 @@ static void spi_nor_sfdp_init_params_deprecated(struct spi_nor *nor) >> >> if (spi_nor_parse_sfdp(nor)) { >> memcpy(nor->params, &sfdp_params, sizeof(*nor->params)); >> - nor->addr_nbytes = 0; >> nor->flags &= ~SNOR_F_4B_OPCODES; >> } >> } >> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h >> index fe7683fe1b4d..41df8bc8e008 100644 >> --- a/drivers/mtd/spi-nor/core.h >> +++ b/drivers/mtd/spi-nor/core.h >> @@ -345,6 +345,7 @@ struct spi_nor_otp { >> * @writesize Minimal writable flash unit size. Defaults to 1. Set to >> * ECC unit size for ECC-ed flashes. >> * @page_size: the page size of the SPI NOR flash memory. >> + * @addr_nbytes: number of address bytes to send. >> * @rdsr_dummy: dummy cycles needed for Read Status Register command >> * in octal DTR mode. >> * @rdsr_addr_nbytes: dummy address bytes needed for Read Status Register >> @@ -377,6 +378,7 @@ struct spi_nor_flash_parameter { >> u64 size; >> u32 writesize; >> u32 page_size; >> + u8 addr_nbytes; >> u8 rdsr_dummy; >> u8 rdsr_addr_nbytes; >> >> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c >> index 058ce218d2af..90d7c25f7281 100644 >> --- a/drivers/mtd/spi-nor/sfdp.c >> +++ b/drivers/mtd/spi-nor/sfdp.c >> @@ -178,19 +178,16 @@ static int spi_nor_read_raw(struct spi_nor *nor, u32 addr, size_t len, u8 *buf) >> static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr, >> size_t len, void *buf) >> { >> - u8 addr_nbytes; >> int ret; >> >> - addr_nbytes = nor->addr_nbytes; >> - >> nor->read_opcode = SPINOR_OP_RDSFDP; >> nor->addr_nbytes = 3; >> nor->read_dummy = 8; >> >> ret = spi_nor_read_raw(nor, addr, len, buf); >> >> - nor->addr_nbytes = addr_nbytes; >> /* Restore setup to its uninitialized state. */ >> + nor->addr_nbytes = 0; >> nor->read_opcode = 0; >> nor->read_dummy = 0; >> >> @@ -461,11 +458,11 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, >> 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_nbytes = 3; >> + params->addr_nbytes = 3; >> break; >> >> case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY: >> - nor->addr_nbytes = 4; >> + params->addr_nbytes = 4; >> break; >> >> default: >> @@ -652,7 +649,7 @@ static u8 spi_nor_smpt_addr_nbytes(const struct spi_nor *nor, const u32 settings >> return 4; >> case SMPT_CMD_ADDRESS_LEN_USE_CURRENT: >> default: >> - return nor->addr_nbytes; >> + return nor->params->addr_nbytes; >> } >> } >> >> @@ -689,7 +686,6 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt, >> u32 addr; >> int err; >> u8 i; >> - u8 addr_nbytes; >> u8 read_data_mask, map_id; >> >> /* Use a kmalloc'ed bounce buffer to guarantee it is DMA-able. */ >> @@ -697,8 +693,6 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt, >> if (!buf) >> return ERR_PTR(-ENOMEM); >> >> - addr_nbytes = nor->addr_nbytes; >> - >> map_id = 0; >> /* Determine if there are any optional Detection Command Descriptors */ >> for (i = 0; i < smpt_len; i += 2) { >> @@ -753,8 +747,8 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt, >> /* fall through */ >> out: >> kfree(buf); >> - nor->addr_nbytes = addr_nbytes; >> /* Restore setup to its uninitialized state. */ >> + nor->addr_nbytes = 0; > > Same as before, I don't think this function should know or care about > the default or uninitialised values. > well, maybe. nor->addr_nbytes comes with value zero when this method is called. We gratuitously save it's zero value in a local variable and then we restore it. The restore of the value could signify to the reader that nor->addr_nbytes is already initialized with a sane value, which is not the case, so I find the code more readable in my version. But I don't mind too much, so I'll drop this particular change. Thanks, ta >> nor->read_dummy = 0; >> nor->read_opcode = 0; >> return ret; >> @@ -1096,7 +1090,7 @@ static int spi_nor_parse_4bait(struct spi_nor *nor, >> * Spansion memory. However this quirk is no longer needed with new >> * SFDP compliant memories. >> */ >> - nor->addr_nbytes = 4; >> + params->addr_nbytes = 4; > > Patch looks good at first glance but I have not looked very carefully if > it might cause some small issues. > >> nor->flags |= SNOR_F_4B_OPCODES | SNOR_F_HAS_4BAIT; >> >> /* fall through */ >> -- >> 2.25.1 >> > > issi.c's is25lp256_post_bfpt_fixups() also sets nor->addr_nbytes. Should > that be updated to use params->addr_nbytes instead? > > -- > Regards, > Pratyush Yadav > Texas Instruments Inc.
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 7db6b41d7c30..dd71deba9f11 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -2272,8 +2272,8 @@ static int spi_nor_default_setup(struct spi_nor *nor, static int spi_nor_set_addr_nbytes(struct spi_nor *nor) { - if (nor->addr_nbytes) { - /* already configured from SFDP */ + if (nor->params->addr_nbytes) { + nor->addr_nbytes = nor->params->addr_nbytes; } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) { /* * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So @@ -2518,7 +2518,6 @@ static void spi_nor_sfdp_init_params_deprecated(struct spi_nor *nor) if (spi_nor_parse_sfdp(nor)) { memcpy(nor->params, &sfdp_params, sizeof(*nor->params)); - nor->addr_nbytes = 0; nor->flags &= ~SNOR_F_4B_OPCODES; } } diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h index fe7683fe1b4d..41df8bc8e008 100644 --- a/drivers/mtd/spi-nor/core.h +++ b/drivers/mtd/spi-nor/core.h @@ -345,6 +345,7 @@ struct spi_nor_otp { * @writesize Minimal writable flash unit size. Defaults to 1. Set to * ECC unit size for ECC-ed flashes. * @page_size: the page size of the SPI NOR flash memory. + * @addr_nbytes: number of address bytes to send. * @rdsr_dummy: dummy cycles needed for Read Status Register command * in octal DTR mode. * @rdsr_addr_nbytes: dummy address bytes needed for Read Status Register @@ -377,6 +378,7 @@ struct spi_nor_flash_parameter { u64 size; u32 writesize; u32 page_size; + u8 addr_nbytes; u8 rdsr_dummy; u8 rdsr_addr_nbytes; diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c index 058ce218d2af..90d7c25f7281 100644 --- a/drivers/mtd/spi-nor/sfdp.c +++ b/drivers/mtd/spi-nor/sfdp.c @@ -178,19 +178,16 @@ static int spi_nor_read_raw(struct spi_nor *nor, u32 addr, size_t len, u8 *buf) static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr, size_t len, void *buf) { - u8 addr_nbytes; int ret; - addr_nbytes = nor->addr_nbytes; - nor->read_opcode = SPINOR_OP_RDSFDP; nor->addr_nbytes = 3; nor->read_dummy = 8; ret = spi_nor_read_raw(nor, addr, len, buf); - nor->addr_nbytes = addr_nbytes; /* Restore setup to its uninitialized state. */ + nor->addr_nbytes = 0; nor->read_opcode = 0; nor->read_dummy = 0; @@ -461,11 +458,11 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, 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_nbytes = 3; + params->addr_nbytes = 3; break; case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY: - nor->addr_nbytes = 4; + params->addr_nbytes = 4; break; default: @@ -652,7 +649,7 @@ static u8 spi_nor_smpt_addr_nbytes(const struct spi_nor *nor, const u32 settings return 4; case SMPT_CMD_ADDRESS_LEN_USE_CURRENT: default: - return nor->addr_nbytes; + return nor->params->addr_nbytes; } } @@ -689,7 +686,6 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt, u32 addr; int err; u8 i; - u8 addr_nbytes; u8 read_data_mask, map_id; /* Use a kmalloc'ed bounce buffer to guarantee it is DMA-able. */ @@ -697,8 +693,6 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt, if (!buf) return ERR_PTR(-ENOMEM); - addr_nbytes = nor->addr_nbytes; - map_id = 0; /* Determine if there are any optional Detection Command Descriptors */ for (i = 0; i < smpt_len; i += 2) { @@ -753,8 +747,8 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt, /* fall through */ out: kfree(buf); - nor->addr_nbytes = addr_nbytes; /* Restore setup to its uninitialized state. */ + nor->addr_nbytes = 0; nor->read_dummy = 0; nor->read_opcode = 0; return ret; @@ -1096,7 +1090,7 @@ static int spi_nor_parse_4bait(struct spi_nor *nor, * Spansion memory. However this quirk is no longer needed with new * SFDP compliant memories. */ - nor->addr_nbytes = 4; + params->addr_nbytes = 4; nor->flags |= SNOR_F_4B_OPCODES | SNOR_F_HAS_4BAIT; /* fall through */