| Message ID | 20210727045222.905056-27-tudor.ambarus@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mtd: spi-nor: Handle ID collisions and clean params init | expand |
On 27/07/21 07:52AM, Tudor Ambarus wrote: > Called for all flashes, regardless if they define SFDP tables or not. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/core.c | 92 +++++++++++++++++++++----------------- > 1 file changed, 52 insertions(+), 40 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index b3a01d7d6f0b..9193317f897d 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -2543,6 +2543,56 @@ static int spi_nor_setup(struct spi_nor *nor, > return spi_nor_set_addr_width(nor); > } > > +/** > + * spi_nor_init_default_params() - Default initialization of flash parameters > + * and settings. Done for all flashes, regardless is they define SFDP tables > + * or not. > + * @nor: pointer to a 'struct spi_nor'. > + */ > +static void spi_nor_init_default_params(struct spi_nor *nor) > +{ > + struct spi_nor_flash_parameter *params = nor->params; > + const struct flash_info *info = nor->info; > + struct device_node *np = spi_nor_get_flash_node(nor); > + > + params->quad_enable = spi_nor_sr2_bit1_quad_enable; > + params->set_4byte_addr_mode = spansion_set_4byte_addr_mode; > + params->setup = spi_nor_default_setup; > + params->otp.org = &info->otp_org; > + > + /* Default to 16-bit Write Status (01h) Command */ > + nor->flags |= SNOR_F_HAS_16BIT_SR; > + > + /* Set SPI NOR sizes. */ > + params->writesize = 1; > + params->size = (u64)info->sector_size * info->n_sectors; > + params->page_size = info->page_size; I think these two lines should go in spi_nor_info_init_params() since you are using the nor info to initialize these parameters. Otherwise, what even is the difference between these two functions? > + > + if (!(info->flags & SPI_NOR_NO_FR)) { > + /* Default to Fast Read for DT and non-DT platform devices. */ > + params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST; > + > + /* Mask out Fast Read if not requested at DT instantiation. */ > + if (np && !of_property_read_bool(np, "m25p,fast-read")) > + params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; > + } > + > + /* (Fast) Read settings. */ > + params->hwcaps.mask |= SNOR_HWCAPS_READ; > + spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ], > + 0, 0, SPINOR_OP_READ, > + SNOR_PROTO_1_1_1); > + > + if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST) > + spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_FAST], > + 0, 8, SPINOR_OP_READ_FAST, > + SNOR_PROTO_1_1_1); > + /* Page Program settings. */ > + params->hwcaps.mask |= SNOR_HWCAPS_PP; > + spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP], > + SPINOR_OP_PP, SNOR_PROTO_1_1_1); > +} > + > /** > * spi_nor_manufacturer_init_params() - Initialize the flash's parameters and > * settings based on MFR register and ->default_init() hook. > @@ -2609,43 +2659,8 @@ static void spi_nor_info_init_params(struct spi_nor *nor) > struct spi_nor_flash_parameter *params = nor->params; > struct spi_nor_erase_map *map = ¶ms->erase_map; > const struct flash_info *info = nor->info; > - struct device_node *np = spi_nor_get_flash_node(nor); > u8 i, erase_mask; > > - /* Initialize default flash parameters and settings. */ > - params->quad_enable = spi_nor_sr2_bit1_quad_enable; > - params->set_4byte_addr_mode = spansion_set_4byte_addr_mode; > - params->setup = spi_nor_default_setup; > - params->otp.org = &info->otp_org; > - > - /* Default to 16-bit Write Status (01h) Command */ > - nor->flags |= SNOR_F_HAS_16BIT_SR; > - > - /* Set SPI NOR sizes. */ > - params->writesize = 1; > - params->size = (u64)info->sector_size * info->n_sectors; > - params->page_size = info->page_size; > - > - if (!(info->flags & SPI_NOR_NO_FR)) { > - /* Default to Fast Read for DT and non-DT platform devices. */ > - params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST; > - > - /* Mask out Fast Read if not requested at DT instantiation. */ > - if (np && !of_property_read_bool(np, "m25p,fast-read")) > - params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; > - } > - > - /* (Fast) Read settings. */ > - params->hwcaps.mask |= SNOR_HWCAPS_READ; > - spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ], > - 0, 0, SPINOR_OP_READ, > - SNOR_PROTO_1_1_1); > - > - if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST) > - spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_FAST], > - 0, 8, SPINOR_OP_READ_FAST, > - SNOR_PROTO_1_1_1); > - > if (info->flags & SPI_NOR_DUAL_READ) { > params->hwcaps.mask |= SNOR_HWCAPS_READ_1_1_2; > spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_1_1_2], > @@ -2674,11 +2689,6 @@ static void spi_nor_info_init_params(struct spi_nor *nor) > SNOR_PROTO_8_8_8_DTR); > } > > - /* Page Program settings. */ > - params->hwcaps.mask |= SNOR_HWCAPS_PP; > - spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP], > - SPINOR_OP_PP, SNOR_PROTO_1_1_1); > - > if (info->flags & SPI_NOR_OCTAL_DTR_PP) { > params->hwcaps.mask |= SNOR_HWCAPS_PP_8_8_8_DTR; > /* > @@ -2823,6 +2833,8 @@ static int spi_nor_init_params(struct spi_nor *nor) > if (!nor->params) > return -ENOMEM; > > + spi_nor_init_default_params(nor); > + > spi_nor_info_init_params(nor); > > spi_nor_manufacturer_init_params(nor); I am neutral towards this patch. I don't think it improves much, but at the same time it doesn't make anything worse either.
On 8/24/21 8:30 PM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 27/07/21 07:52AM, Tudor Ambarus wrote: >> Called for all flashes, regardless if they define SFDP tables or not. >> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >> --- >> drivers/mtd/spi-nor/core.c | 92 +++++++++++++++++++++----------------- >> 1 file changed, 52 insertions(+), 40 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >> index b3a01d7d6f0b..9193317f897d 100644 >> --- a/drivers/mtd/spi-nor/core.c >> +++ b/drivers/mtd/spi-nor/core.c >> @@ -2543,6 +2543,56 @@ static int spi_nor_setup(struct spi_nor *nor, >> return spi_nor_set_addr_width(nor); >> } >> >> +/** >> + * spi_nor_init_default_params() - Default initialization of flash parameters >> + * and settings. Done for all flashes, regardless is they define SFDP tables >> + * or not. >> + * @nor: pointer to a 'struct spi_nor'. >> + */ >> +static void spi_nor_init_default_params(struct spi_nor *nor) >> +{ >> + struct spi_nor_flash_parameter *params = nor->params; >> + const struct flash_info *info = nor->info; >> + struct device_node *np = spi_nor_get_flash_node(nor); >> + >> + params->quad_enable = spi_nor_sr2_bit1_quad_enable; >> + params->set_4byte_addr_mode = spansion_set_4byte_addr_mode; >> + params->setup = spi_nor_default_setup; >> + params->otp.org = &info->otp_org; >> + >> + /* Default to 16-bit Write Status (01h) Command */ >> + nor->flags |= SNOR_F_HAS_16BIT_SR; >> + >> + /* Set SPI NOR sizes. */ >> + params->writesize = 1; >> + params->size = (u64)info->sector_size * info->n_sectors; >> + params->page_size = info->page_size; > > I think these two lines should go in spi_nor_info_init_params() since > you are using the nor info to initialize these parameters. Otherwise, > what even is the difference between these two functions? I think a better name for spi_nor_info_init_params() is spi_nor_nonsfdp_info_init_params(). This method will eventually be called just for non SFDP flashes. Check conversation in 18/35. And maybe I should rename spi_nor_nonsfdp_flags_init to spi_nor_nonsfdp_info_init_snor_f. > >> + >> + if (!(info->flags & SPI_NOR_NO_FR)) { >> + /* Default to Fast Read for DT and non-DT platform devices. */ >> + params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST; >> + >> + /* Mask out Fast Read if not requested at DT instantiation. */ >> + if (np && !of_property_read_bool(np, "m25p,fast-read")) >> + params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; >> + } >> + >> + /* (Fast) Read settings. */ >> + params->hwcaps.mask |= SNOR_HWCAPS_READ; >> + spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ], >> + 0, 0, SPINOR_OP_READ, >> + SNOR_PROTO_1_1_1); >> + >> + if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST) >> + spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_FAST], >> + 0, 8, SPINOR_OP_READ_FAST, >> + SNOR_PROTO_1_1_1); >> + /* Page Program settings. */ >> + params->hwcaps.mask |= SNOR_HWCAPS_PP; >> + spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP], >> + SPINOR_OP_PP, SNOR_PROTO_1_1_1); >> +} >> + >> /** >> * spi_nor_manufacturer_init_params() - Initialize the flash's parameters and >> * settings based on MFR register and ->default_init() hook. >> @@ -2609,43 +2659,8 @@ static void spi_nor_info_init_params(struct spi_nor *nor) >> struct spi_nor_flash_parameter *params = nor->params; >> struct spi_nor_erase_map *map = ¶ms->erase_map; >> const struct flash_info *info = nor->info; >> - struct device_node *np = spi_nor_get_flash_node(nor); >> u8 i, erase_mask; >> >> - /* Initialize default flash parameters and settings. */ >> - params->quad_enable = spi_nor_sr2_bit1_quad_enable; >> - params->set_4byte_addr_mode = spansion_set_4byte_addr_mode; >> - params->setup = spi_nor_default_setup; >> - params->otp.org = &info->otp_org; >> - >> - /* Default to 16-bit Write Status (01h) Command */ >> - nor->flags |= SNOR_F_HAS_16BIT_SR; >> - >> - /* Set SPI NOR sizes. */ >> - params->writesize = 1; >> - params->size = (u64)info->sector_size * info->n_sectors; >> - params->page_size = info->page_size; >> - >> - if (!(info->flags & SPI_NOR_NO_FR)) { >> - /* Default to Fast Read for DT and non-DT platform devices. */ >> - params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST; >> - >> - /* Mask out Fast Read if not requested at DT instantiation. */ >> - if (np && !of_property_read_bool(np, "m25p,fast-read")) >> - params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; >> - } >> - >> - /* (Fast) Read settings. */ >> - params->hwcaps.mask |= SNOR_HWCAPS_READ; >> - spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ], >> - 0, 0, SPINOR_OP_READ, >> - SNOR_PROTO_1_1_1); >> - >> - if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST) >> - spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_FAST], >> - 0, 8, SPINOR_OP_READ_FAST, >> - SNOR_PROTO_1_1_1); >> - >> if (info->flags & SPI_NOR_DUAL_READ) { >> params->hwcaps.mask |= SNOR_HWCAPS_READ_1_1_2; >> spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_1_1_2], >> @@ -2674,11 +2689,6 @@ static void spi_nor_info_init_params(struct spi_nor *nor) >> SNOR_PROTO_8_8_8_DTR); >> } >> >> - /* Page Program settings. */ >> - params->hwcaps.mask |= SNOR_HWCAPS_PP; >> - spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP], >> - SPINOR_OP_PP, SNOR_PROTO_1_1_1); >> - >> if (info->flags & SPI_NOR_OCTAL_DTR_PP) { >> params->hwcaps.mask |= SNOR_HWCAPS_PP_8_8_8_DTR; >> /* >> @@ -2823,6 +2833,8 @@ static int spi_nor_init_params(struct spi_nor *nor) >> if (!nor->params) >> return -ENOMEM; >> >> + spi_nor_init_default_params(nor); >> + >> spi_nor_info_init_params(nor); >> >> spi_nor_manufacturer_init_params(nor); > > I am neutral towards this patch. I don't think it improves much, but at > the same time it doesn't make anything worse either. I think it helps readability. It splits spi_nor_init_params() into smaller logical chunks, based on the type of initialization. We should usually avoid long methods where we can split them in logical chunks, it makes the code pleasant to read. Cheers, ta > > -- > Regards, > Pratyush Yadav > Texas Instruments Inc. >
Am 2021-10-04 06:17, schrieb Tudor.Ambarus@microchip.com: > On 8/24/21 8:30 PM, Pratyush Yadav wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the content is safe >> >> On 27/07/21 07:52AM, Tudor Ambarus wrote: >>> Called for all flashes, regardless if they define SFDP tables or not. >>> >>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >>> --- >>> drivers/mtd/spi-nor/core.c | 92 >>> +++++++++++++++++++++----------------- >>> 1 file changed, 52 insertions(+), 40 deletions(-) >>> >>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >>> index b3a01d7d6f0b..9193317f897d 100644 >>> --- a/drivers/mtd/spi-nor/core.c >>> +++ b/drivers/mtd/spi-nor/core.c >>> @@ -2543,6 +2543,56 @@ static int spi_nor_setup(struct spi_nor *nor, >>> return spi_nor_set_addr_width(nor); >>> } >>> >>> +/** >>> + * spi_nor_init_default_params() - Default initialization of flash >>> parameters >>> + * and settings. Done for all flashes, regardless is they define >>> SFDP tables >>> + * or not. >>> + * @nor: pointer to a 'struct spi_nor'. >>> + */ >>> +static void spi_nor_init_default_params(struct spi_nor *nor) >>> +{ >>> + struct spi_nor_flash_parameter *params = nor->params; >>> + const struct flash_info *info = nor->info; >>> + struct device_node *np = spi_nor_get_flash_node(nor); >>> + >>> + params->quad_enable = spi_nor_sr2_bit1_quad_enable; >>> + params->set_4byte_addr_mode = spansion_set_4byte_addr_mode; >>> + params->setup = spi_nor_default_setup; >>> + params->otp.org = &info->otp_org; >>> + >>> + /* Default to 16-bit Write Status (01h) Command */ >>> + nor->flags |= SNOR_F_HAS_16BIT_SR; >>> + >>> + /* Set SPI NOR sizes. */ >>> + params->writesize = 1; >>> + params->size = (u64)info->sector_size * info->n_sectors; >>> + params->page_size = info->page_size; >> >> I think these two lines should go in spi_nor_info_init_params() since >> you are using the nor info to initialize these parameters. Otherwise, >> what even is the difference between these two functions? > > I think a better name for spi_nor_info_init_params() is > spi_nor_nonsfdp_info_init_params(). This method will eventually be > called > just for non SFDP flashes. Check conversation in 18/35. > > And maybe I should rename spi_nor_nonsfdp_flags_init to > spi_nor_nonsfdp_info_init_snor_f. Urgh. I really dislike that suffix. Somehow the flags prefix creeps into the function name. If that function is expected to only set the SNOR_F_ flags then there should be a comment. >> >>> + >>> + if (!(info->flags & SPI_NOR_NO_FR)) { >>> + /* Default to Fast Read for DT and non-DT platform >>> devices. */ >>> + params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST; >>> + >>> + /* Mask out Fast Read if not requested at DT >>> instantiation. */ >>> + if (np && !of_property_read_bool(np, "m25p,fast-read")) >>> + params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; >>> + } Hm, this looks really weird. why not: /* default to fast read */ params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST; /* unless the flash doesn't support it */ if (info->flags & SPI_NOR_NO_FR) params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; /* or the device tree doesn't explicitly request it */ if (np && !of_property_read_bool(np, "m25p,fast-read")) params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; I know the old code was the same, but it might go into another patch. The SPI_NOR_NO_FR suggests that fast read was always the default. Otherwise, it would have been SPI_NOR_HAS_FAST_READ. >>> + >>> + /* (Fast) Read settings. */ >>> + params->hwcaps.mask |= SNOR_HWCAPS_READ; >>> + spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ], >>> + 0, 0, SPINOR_OP_READ, >>> + SNOR_PROTO_1_1_1); >>> + >>> + if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST) >>> + >>> spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_FAST], >>> + 0, 8, SPINOR_OP_READ_FAST, >>> + SNOR_PROTO_1_1_1); >>> + /* Page Program settings. */ >>> + params->hwcaps.mask |= SNOR_HWCAPS_PP; >>> + spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP], >>> + SPINOR_OP_PP, SNOR_PROTO_1_1_1); >>> +} >>> + >>> /** >>> * spi_nor_manufacturer_init_params() - Initialize the flash's >>> parameters and >>> * settings based on MFR register and ->default_init() hook. >>> @@ -2609,43 +2659,8 @@ static void spi_nor_info_init_params(struct >>> spi_nor *nor) >>> struct spi_nor_flash_parameter *params = nor->params; >>> struct spi_nor_erase_map *map = ¶ms->erase_map; >>> const struct flash_info *info = nor->info; >>> - struct device_node *np = spi_nor_get_flash_node(nor); >>> u8 i, erase_mask; >>> >>> - /* Initialize default flash parameters and settings. */ >>> - params->quad_enable = spi_nor_sr2_bit1_quad_enable; >>> - params->set_4byte_addr_mode = spansion_set_4byte_addr_mode; >>> - params->setup = spi_nor_default_setup; >>> - params->otp.org = &info->otp_org; >>> - >>> - /* Default to 16-bit Write Status (01h) Command */ >>> - nor->flags |= SNOR_F_HAS_16BIT_SR; >>> - >>> - /* Set SPI NOR sizes. */ >>> - params->writesize = 1; >>> - params->size = (u64)info->sector_size * info->n_sectors; >>> - params->page_size = info->page_size; >>> - >>> - if (!(info->flags & SPI_NOR_NO_FR)) { >>> - /* Default to Fast Read for DT and non-DT platform >>> devices. */ >>> - params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST; >>> - >>> - /* Mask out Fast Read if not requested at DT >>> instantiation. */ >>> - if (np && !of_property_read_bool(np, "m25p,fast-read")) >>> - params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; >>> - } >>> - >>> - /* (Fast) Read settings. */ >>> - params->hwcaps.mask |= SNOR_HWCAPS_READ; >>> - spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ], >>> - 0, 0, SPINOR_OP_READ, >>> - SNOR_PROTO_1_1_1); >>> - >>> - if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST) >>> - >>> spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_FAST], >>> - 0, 8, SPINOR_OP_READ_FAST, >>> - SNOR_PROTO_1_1_1); >>> - >>> if (info->flags & SPI_NOR_DUAL_READ) { >>> params->hwcaps.mask |= SNOR_HWCAPS_READ_1_1_2; >>> >>> spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_1_1_2], >>> @@ -2674,11 +2689,6 @@ static void spi_nor_info_init_params(struct >>> spi_nor *nor) >>> SNOR_PROTO_8_8_8_DTR); >>> } >>> >>> - /* Page Program settings. */ >>> - params->hwcaps.mask |= SNOR_HWCAPS_PP; >>> - spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP], >>> - SPINOR_OP_PP, SNOR_PROTO_1_1_1); >>> - >>> if (info->flags & SPI_NOR_OCTAL_DTR_PP) { >>> params->hwcaps.mask |= SNOR_HWCAPS_PP_8_8_8_DTR; >>> /* >>> @@ -2823,6 +2833,8 @@ static int spi_nor_init_params(struct spi_nor >>> *nor) >>> if (!nor->params) >>> return -ENOMEM; >>> >>> + spi_nor_init_default_params(nor); >>> + >>> spi_nor_info_init_params(nor); >>> >>> spi_nor_manufacturer_init_params(nor); >> >> I am neutral towards this patch. I don't think it improves much, but >> at >> the same time it doesn't make anything worse either. > > I think it helps readability. It splits spi_nor_init_params() into > smaller logical chunks, > based on the type of initialization. We should usually avoid long > methods where we can split > them in logical chunks, it makes the code pleasant to read. I'm fine with that. Just want to say, that then we have to think about what goes where and where we draw the line. -michael
On 10/22/21 3:41 PM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Am 2021-10-04 06:17, schrieb Tudor.Ambarus@microchip.com: >> On 8/24/21 8:30 PM, Pratyush Yadav wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know >>> the content is safe >>> >>> On 27/07/21 07:52AM, Tudor Ambarus wrote: >>>> Called for all flashes, regardless if they define SFDP tables or not. >>>> >>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >>>> --- >>>> drivers/mtd/spi-nor/core.c | 92 >>>> +++++++++++++++++++++----------------- >>>> 1 file changed, 52 insertions(+), 40 deletions(-) >>>> >>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >>>> index b3a01d7d6f0b..9193317f897d 100644 >>>> --- a/drivers/mtd/spi-nor/core.c >>>> +++ b/drivers/mtd/spi-nor/core.c >>>> @@ -2543,6 +2543,56 @@ static int spi_nor_setup(struct spi_nor *nor, >>>> return spi_nor_set_addr_width(nor); >>>> } >>>> >>>> +/** >>>> + * spi_nor_init_default_params() - Default initialization of flash >>>> parameters >>>> + * and settings. Done for all flashes, regardless is they define >>>> SFDP tables >>>> + * or not. >>>> + * @nor: pointer to a 'struct spi_nor'. >>>> + */ >>>> +static void spi_nor_init_default_params(struct spi_nor *nor) >>>> +{ >>>> + struct spi_nor_flash_parameter *params = nor->params; >>>> + const struct flash_info *info = nor->info; >>>> + struct device_node *np = spi_nor_get_flash_node(nor); >>>> + >>>> + params->quad_enable = spi_nor_sr2_bit1_quad_enable; >>>> + params->set_4byte_addr_mode = spansion_set_4byte_addr_mode; >>>> + params->setup = spi_nor_default_setup; >>>> + params->otp.org = &info->otp_org; >>>> + >>>> + /* Default to 16-bit Write Status (01h) Command */ >>>> + nor->flags |= SNOR_F_HAS_16BIT_SR; >>>> + >>>> + /* Set SPI NOR sizes. */ >>>> + params->writesize = 1; >>>> + params->size = (u64)info->sector_size * info->n_sectors; >>>> + params->page_size = info->page_size; >>> >>> I think these two lines should go in spi_nor_info_init_params() since >>> you are using the nor info to initialize these parameters. Otherwise, >>> what even is the difference between these two functions? >> >> I think a better name for spi_nor_info_init_params() is >> spi_nor_nonsfdp_info_init_params(). This method will eventually be >> called >> just for non SFDP flashes. Check conversation in 18/35. >> >> And maybe I should rename spi_nor_nonsfdp_flags_init to >> spi_nor_nonsfdp_info_init_snor_f. > > Urgh. I really dislike that suffix. Somehow the flags prefix :) I'll think of something better then. > creeps into the function name. If that function is expected > to only set the SNOR_F_ flags then there should be a comment. > >>> >>>> + >>>> + if (!(info->flags & SPI_NOR_NO_FR)) { >>>> + /* Default to Fast Read for DT and non-DT platform >>>> devices. */ >>>> + params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST; >>>> + >>>> + /* Mask out Fast Read if not requested at DT >>>> instantiation. */ >>>> + if (np && !of_property_read_bool(np, "m25p,fast-read")) >>>> + params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; >>>> + } > > Hm, this looks really weird. > > why not: > > /* default to fast read */ > params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST; > > /* unless the flash doesn't support it */ > if (info->flags & SPI_NOR_NO_FR) > params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; > > /* or the device tree doesn't explicitly request it */ > if (np && !of_property_read_bool(np, "m25p,fast-read")) > params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; > > I know the old code was the same, but it might go into another > patch. The SPI_NOR_NO_FR suggests that fast read was always the right, dedicated patch. I'll check it. > default. Otherwise, it would have been SPI_NOR_HAS_FAST_READ. > >>>> + >>>> + /* (Fast) Read settings. */ >>>> + params->hwcaps.mask |= SNOR_HWCAPS_READ; >>>> + spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ], >>>> + 0, 0, SPINOR_OP_READ, >>>> + SNOR_PROTO_1_1_1); >>>> + >>>> + if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST) >>>> + >>>> spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_FAST], >>>> + 0, 8, SPINOR_OP_READ_FAST, >>>> + SNOR_PROTO_1_1_1); >>>> + /* Page Program settings. */ >>>> + params->hwcaps.mask |= SNOR_HWCAPS_PP; >>>> + spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP], >>>> + SPINOR_OP_PP, SNOR_PROTO_1_1_1); >>>> +} >>>> + >>>> /** >>>> * spi_nor_manufacturer_init_params() - Initialize the flash's >>>> parameters and >>>> * settings based on MFR register and ->default_init() hook. >>>> @@ -2609,43 +2659,8 @@ static void spi_nor_info_init_params(struct >>>> spi_nor *nor) >>>> struct spi_nor_flash_parameter *params = nor->params; >>>> struct spi_nor_erase_map *map = ¶ms->erase_map; >>>> const struct flash_info *info = nor->info; >>>> - struct device_node *np = spi_nor_get_flash_node(nor); >>>> u8 i, erase_mask; >>>> >>>> - /* Initialize default flash parameters and settings. */ >>>> - params->quad_enable = spi_nor_sr2_bit1_quad_enable; >>>> - params->set_4byte_addr_mode = spansion_set_4byte_addr_mode; >>>> - params->setup = spi_nor_default_setup; >>>> - params->otp.org = &info->otp_org; >>>> - >>>> - /* Default to 16-bit Write Status (01h) Command */ >>>> - nor->flags |= SNOR_F_HAS_16BIT_SR; >>>> - >>>> - /* Set SPI NOR sizes. */ >>>> - params->writesize = 1; >>>> - params->size = (u64)info->sector_size * info->n_sectors; >>>> - params->page_size = info->page_size; >>>> - >>>> - if (!(info->flags & SPI_NOR_NO_FR)) { >>>> - /* Default to Fast Read for DT and non-DT platform >>>> devices. */ >>>> - params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST; >>>> - >>>> - /* Mask out Fast Read if not requested at DT >>>> instantiation. */ >>>> - if (np && !of_property_read_bool(np, "m25p,fast-read")) >>>> - params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; >>>> - } >>>> - >>>> - /* (Fast) Read settings. */ >>>> - params->hwcaps.mask |= SNOR_HWCAPS_READ; >>>> - spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ], >>>> - 0, 0, SPINOR_OP_READ, >>>> - SNOR_PROTO_1_1_1); >>>> - >>>> - if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST) >>>> - >>>> spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_FAST], >>>> - 0, 8, SPINOR_OP_READ_FAST, >>>> - SNOR_PROTO_1_1_1); >>>> - >>>> if (info->flags & SPI_NOR_DUAL_READ) { >>>> params->hwcaps.mask |= SNOR_HWCAPS_READ_1_1_2; >>>> >>>> spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_1_1_2], >>>> @@ -2674,11 +2689,6 @@ static void spi_nor_info_init_params(struct >>>> spi_nor *nor) >>>> SNOR_PROTO_8_8_8_DTR); >>>> } >>>> >>>> - /* Page Program settings. */ >>>> - params->hwcaps.mask |= SNOR_HWCAPS_PP; >>>> - spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP], >>>> - SPINOR_OP_PP, SNOR_PROTO_1_1_1); >>>> - >>>> if (info->flags & SPI_NOR_OCTAL_DTR_PP) { >>>> params->hwcaps.mask |= SNOR_HWCAPS_PP_8_8_8_DTR; >>>> /* >>>> @@ -2823,6 +2833,8 @@ static int spi_nor_init_params(struct spi_nor >>>> *nor) >>>> if (!nor->params) >>>> return -ENOMEM; >>>> >>>> + spi_nor_init_default_params(nor); >>>> + >>>> spi_nor_info_init_params(nor); >>>> >>>> spi_nor_manufacturer_init_params(nor); >>> >>> I am neutral towards this patch. I don't think it improves much, but >>> at >>> the same time it doesn't make anything worse either. >> >> I think it helps readability. It splits spi_nor_init_params() into >> smaller logical chunks, >> based on the type of initialization. We should usually avoid long >> methods where we can split >> them in logical chunks, it makes the code pleasant to read. > > I'm fine with that. Just want to say, that then we have to think > about what goes where and where we draw the line. > Okay, I'll propose some guidance in the commit message.
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index b3a01d7d6f0b..9193317f897d 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -2543,6 +2543,56 @@ static int spi_nor_setup(struct spi_nor *nor, return spi_nor_set_addr_width(nor); } +/** + * spi_nor_init_default_params() - Default initialization of flash parameters + * and settings. Done for all flashes, regardless is they define SFDP tables + * or not. + * @nor: pointer to a 'struct spi_nor'. + */ +static void spi_nor_init_default_params(struct spi_nor *nor) +{ + struct spi_nor_flash_parameter *params = nor->params; + const struct flash_info *info = nor->info; + struct device_node *np = spi_nor_get_flash_node(nor); + + params->quad_enable = spi_nor_sr2_bit1_quad_enable; + params->set_4byte_addr_mode = spansion_set_4byte_addr_mode; + params->setup = spi_nor_default_setup; + params->otp.org = &info->otp_org; + + /* Default to 16-bit Write Status (01h) Command */ + nor->flags |= SNOR_F_HAS_16BIT_SR; + + /* Set SPI NOR sizes. */ + params->writesize = 1; + params->size = (u64)info->sector_size * info->n_sectors; + params->page_size = info->page_size; + + if (!(info->flags & SPI_NOR_NO_FR)) { + /* Default to Fast Read for DT and non-DT platform devices. */ + params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST; + + /* Mask out Fast Read if not requested at DT instantiation. */ + if (np && !of_property_read_bool(np, "m25p,fast-read")) + params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; + } + + /* (Fast) Read settings. */ + params->hwcaps.mask |= SNOR_HWCAPS_READ; + spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ], + 0, 0, SPINOR_OP_READ, + SNOR_PROTO_1_1_1); + + if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST) + spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_FAST], + 0, 8, SPINOR_OP_READ_FAST, + SNOR_PROTO_1_1_1); + /* Page Program settings. */ + params->hwcaps.mask |= SNOR_HWCAPS_PP; + spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP], + SPINOR_OP_PP, SNOR_PROTO_1_1_1); +} + /** * spi_nor_manufacturer_init_params() - Initialize the flash's parameters and * settings based on MFR register and ->default_init() hook. @@ -2609,43 +2659,8 @@ static void spi_nor_info_init_params(struct spi_nor *nor) struct spi_nor_flash_parameter *params = nor->params; struct spi_nor_erase_map *map = ¶ms->erase_map; const struct flash_info *info = nor->info; - struct device_node *np = spi_nor_get_flash_node(nor); u8 i, erase_mask; - /* Initialize default flash parameters and settings. */ - params->quad_enable = spi_nor_sr2_bit1_quad_enable; - params->set_4byte_addr_mode = spansion_set_4byte_addr_mode; - params->setup = spi_nor_default_setup; - params->otp.org = &info->otp_org; - - /* Default to 16-bit Write Status (01h) Command */ - nor->flags |= SNOR_F_HAS_16BIT_SR; - - /* Set SPI NOR sizes. */ - params->writesize = 1; - params->size = (u64)info->sector_size * info->n_sectors; - params->page_size = info->page_size; - - if (!(info->flags & SPI_NOR_NO_FR)) { - /* Default to Fast Read for DT and non-DT platform devices. */ - params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST; - - /* Mask out Fast Read if not requested at DT instantiation. */ - if (np && !of_property_read_bool(np, "m25p,fast-read")) - params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; - } - - /* (Fast) Read settings. */ - params->hwcaps.mask |= SNOR_HWCAPS_READ; - spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ], - 0, 0, SPINOR_OP_READ, - SNOR_PROTO_1_1_1); - - if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST) - spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_FAST], - 0, 8, SPINOR_OP_READ_FAST, - SNOR_PROTO_1_1_1); - if (info->flags & SPI_NOR_DUAL_READ) { params->hwcaps.mask |= SNOR_HWCAPS_READ_1_1_2; spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_1_1_2], @@ -2674,11 +2689,6 @@ static void spi_nor_info_init_params(struct spi_nor *nor) SNOR_PROTO_8_8_8_DTR); } - /* Page Program settings. */ - params->hwcaps.mask |= SNOR_HWCAPS_PP; - spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP], - SPINOR_OP_PP, SNOR_PROTO_1_1_1); - if (info->flags & SPI_NOR_OCTAL_DTR_PP) { params->hwcaps.mask |= SNOR_HWCAPS_PP_8_8_8_DTR; /* @@ -2823,6 +2833,8 @@ static int spi_nor_init_params(struct spi_nor *nor) if (!nor->params) return -ENOMEM; + spi_nor_init_default_params(nor); + spi_nor_info_init_params(nor); spi_nor_manufacturer_init_params(nor);
Called for all flashes, regardless if they define SFDP tables or not. Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- drivers/mtd/spi-nor/core.c | 92 +++++++++++++++++++++----------------- 1 file changed, 52 insertions(+), 40 deletions(-)