| Message ID | 20231219102103.92738-1-jaimeliao.tw@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] mtd: spi-nor: core: Discard HW capabilities if no enable function | expand |
Am 2023-12-19 11:21, schrieb Jaime Liao: > From: JaimeLiao <jaimeliao@mxic.com.tw> > > Discard corresponding HW capabilities to prevent carrying the > wrong protocol if no QUAD/Octal DTR enable function hooked. > > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw> Reviewed-by: Michael Walle <michael@walle.cc> -michael
On 19.12.2023 12:21, Jaime Liao wrote: > From: JaimeLiao <jaimeliao@mxic.com.tw> > > Discard corresponding HW capabilities to prevent carrying the > wrong protocol if no QUAD/Octal DTR enable function hooked. > > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw> > --- > changes in v2 > - Add SNOR_HWCAPS_8_8_8_DTR > - Restore the enable function judgement in spi_nor_set_octal_dtr() > - Restore the enable function judgement in spi_nor_quad_enable() > --- > drivers/mtd/spi-nor/core.c | 7 +++++++ > include/linux/mtd/spi-nor.h | 6 ++++++ > 2 files changed, 13 insertions(+) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 1c443fe568cf..14359101c6cf 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -2621,6 +2621,13 @@ static int spi_nor_default_setup(struct spi_nor *nor, > */ > shared_mask = hwcaps->mask & params->hwcaps.mask; > > + /* Mask out Octal DTR if no enable function */ > + if (!params->set_octal_dtr) > + shared_mask &= ~SNOR_HWCAPS_8_8_8_DTR; > + > + if (!params->quad_enable) > + shared_mask &= ~SNOR_HWCAPS_4_4_4; > + > if (nor->spimem) { > /* > * When called from spi_nor_probe(), all caps are set and we > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index cdcfe0fd2e7d..78a119192ee0 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -266,6 +266,12 @@ struct spi_nor_hwcaps { > #define SNOR_HWCAPS_PP_8_8_8 BIT(22) > #define SNOR_HWCAPS_PP_8_8_8_DTR BIT(23) > > +#define SNOR_HWCAPS_4_4_4 (SNOR_HWCAPS_READ_4_4_4 | \ > + SNOR_HWCAPS_PP_4_4_4) quad enable applies for 1-1-4 and 1-4-4 as well. How about changing this to: #define SNOR_HWCAPS_QUAD (SNOR_HWCAPS_READ_QUAD | \ SNOR_HWCAPS_PP_QUAD) > + > +#define SNOR_HWCAPS_8_8_8_DTR (SNOR_HWCAPS_READ_8_8_8_DTR | \ > + SNOR_HWCAPS_PP_8_8_8_DTR) > + > #define SNOR_HWCAPS_X_X_X (SNOR_HWCAPS_READ_2_2_2 | \ > SNOR_HWCAPS_READ_4_4_4 | \ > SNOR_HWCAPS_READ_8_8_8 | \
On 19.12.2023 12:21, Jaime Liao wrote: > From: JaimeLiao <jaimeliao@mxic.com.tw> > > Discard corresponding HW capabilities to prevent carrying the > wrong protocol if no QUAD/Octal DTR enable function hooked. > > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw> > --- > changes in v2 > - Add SNOR_HWCAPS_8_8_8_DTR > - Restore the enable function judgement in spi_nor_set_octal_dtr() > - Restore the enable function judgement in spi_nor_quad_enable() > --- > drivers/mtd/spi-nor/core.c | 7 +++++++ > include/linux/mtd/spi-nor.h | 6 ++++++ > 2 files changed, 13 insertions(+) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 1c443fe568cf..14359101c6cf 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -2621,6 +2621,13 @@ static int spi_nor_default_setup(struct spi_nor *nor, > */ > shared_mask = hwcaps->mask & params->hwcaps.mask; > > + /* Mask out Octal DTR if no enable function */ > + if (!params->set_octal_dtr) > + shared_mask &= ~SNOR_HWCAPS_8_8_8_DTR; > + > + if (!params->quad_enable) > + shared_mask &= ~SNOR_HWCAPS_4_4_4; and these should have been in the late init hook, and instead discard them from the params->hwcaps.mask. And why is this extra check needed in the first place? For octal I assume it's needed by macronix, where the flash is octal capable, but there's no octal enable method defined yet. What about the quad mode? Is there any flash that has no quad enable method defined but still caries quad mode in its hwcaps? > + > if (nor->spimem) { > /* > * When called from spi_nor_probe(), all caps are set and we > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index cdcfe0fd2e7d..78a119192ee0 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -266,6 +266,12 @@ struct spi_nor_hwcaps { > #define SNOR_HWCAPS_PP_8_8_8 BIT(22) > #define SNOR_HWCAPS_PP_8_8_8_DTR BIT(23) > > +#define SNOR_HWCAPS_4_4_4 (SNOR_HWCAPS_READ_4_4_4 | \ > + SNOR_HWCAPS_PP_4_4_4) > + > +#define SNOR_HWCAPS_8_8_8_DTR (SNOR_HWCAPS_READ_8_8_8_DTR | \ > + SNOR_HWCAPS_PP_8_8_8_DTR) > + > #define SNOR_HWCAPS_X_X_X (SNOR_HWCAPS_READ_2_2_2 | \ > SNOR_HWCAPS_READ_4_4_4 | \ > SNOR_HWCAPS_READ_8_8_8 | \
Am 2023-12-20 09:07, schrieb Tudor Ambarus: > On 19.12.2023 12:21, Jaime Liao wrote: >> From: JaimeLiao <jaimeliao@mxic.com.tw> >> >> Discard corresponding HW capabilities to prevent carrying the >> wrong protocol if no QUAD/Octal DTR enable function hooked. >> >> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw> >> --- >> changes in v2 >> - Add SNOR_HWCAPS_8_8_8_DTR >> - Restore the enable function judgement in spi_nor_set_octal_dtr() >> - Restore the enable function judgement in spi_nor_quad_enable() >> --- >> drivers/mtd/spi-nor/core.c | 7 +++++++ >> include/linux/mtd/spi-nor.h | 6 ++++++ >> 2 files changed, 13 insertions(+) >> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >> index 1c443fe568cf..14359101c6cf 100644 >> --- a/drivers/mtd/spi-nor/core.c >> +++ b/drivers/mtd/spi-nor/core.c >> @@ -2621,6 +2621,13 @@ static int spi_nor_default_setup(struct spi_nor >> *nor, >> */ >> shared_mask = hwcaps->mask & params->hwcaps.mask; >> >> + /* Mask out Octal DTR if no enable function */ >> + if (!params->set_octal_dtr) >> + shared_mask &= ~SNOR_HWCAPS_8_8_8_DTR; >> + >> + if (!params->quad_enable) >> + shared_mask &= ~SNOR_HWCAPS_4_4_4; >> + >> if (nor->spimem) { >> /* >> * When called from spi_nor_probe(), all caps are set and we >> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h >> index cdcfe0fd2e7d..78a119192ee0 100644 >> --- a/include/linux/mtd/spi-nor.h >> +++ b/include/linux/mtd/spi-nor.h >> @@ -266,6 +266,12 @@ struct spi_nor_hwcaps { >> #define SNOR_HWCAPS_PP_8_8_8 BIT(22) >> #define SNOR_HWCAPS_PP_8_8_8_DTR BIT(23) >> >> +#define SNOR_HWCAPS_4_4_4 (SNOR_HWCAPS_READ_4_4_4 | \ >> + SNOR_HWCAPS_PP_4_4_4) > > quad enable applies for 1-1-4 and 1-4-4 as well. How about changing > this to: Of course. You are right. I'm not even sure 4_4_4 falls into the category for our quad_enable, because there's a special mode, sometimes called qpi, for opcodes transferred in quad mode. -michael
Am 2023-12-20 09:20, schrieb Tudor Ambarus: > On 19.12.2023 12:21, Jaime Liao wrote: >> From: JaimeLiao <jaimeliao@mxic.com.tw> >> >> Discard corresponding HW capabilities to prevent carrying the >> wrong protocol if no QUAD/Octal DTR enable function hooked. >> >> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw> >> --- >> changes in v2 >> - Add SNOR_HWCAPS_8_8_8_DTR >> - Restore the enable function judgement in spi_nor_set_octal_dtr() >> - Restore the enable function judgement in spi_nor_quad_enable() >> --- >> drivers/mtd/spi-nor/core.c | 7 +++++++ >> include/linux/mtd/spi-nor.h | 6 ++++++ >> 2 files changed, 13 insertions(+) >> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >> index 1c443fe568cf..14359101c6cf 100644 >> --- a/drivers/mtd/spi-nor/core.c >> +++ b/drivers/mtd/spi-nor/core.c >> @@ -2621,6 +2621,13 @@ static int spi_nor_default_setup(struct spi_nor >> *nor, >> */ >> shared_mask = hwcaps->mask & params->hwcaps.mask; >> >> + /* Mask out Octal DTR if no enable function */ >> + if (!params->set_octal_dtr) >> + shared_mask &= ~SNOR_HWCAPS_8_8_8_DTR; >> + >> + if (!params->quad_enable) >> + shared_mask &= ~SNOR_HWCAPS_4_4_4; > > and these should have been in the late init hook, and instead discard > them from the params->hwcaps.mask. Maybe there is a better place to mask these bits. But IMHO the core should do it on itself and we shouldn't need to provide an extra hook function for every driver ourselves. The core knows that there is no .octal_enable op and thus it shouldn't even try to enable this mode. > And why is this extra check needed in the first place? For octal I > assume it's needed by macronix, where the flash is octal capable, but > there's no octal enable method defined yet. Because it is the generic spi nor flash driver, which doesn't provide any of these enable x-mode helpers. > What about the quad mode? Is > there any flash that has no quad enable method defined but still caries > quad mode in its hwcaps? No it is for completeness and correctness. At the moment we are always setting a (random) default quad enable op, due to legacy reasons. -michael >> + >> if (nor->spimem) { >> /* >> * When called from spi_nor_probe(), all caps are set and we >> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h >> index cdcfe0fd2e7d..78a119192ee0 100644 >> --- a/include/linux/mtd/spi-nor.h >> +++ b/include/linux/mtd/spi-nor.h >> @@ -266,6 +266,12 @@ struct spi_nor_hwcaps { >> #define SNOR_HWCAPS_PP_8_8_8 BIT(22) >> #define SNOR_HWCAPS_PP_8_8_8_DTR BIT(23) >> >> +#define SNOR_HWCAPS_4_4_4 (SNOR_HWCAPS_READ_4_4_4 | \ >> + SNOR_HWCAPS_PP_4_4_4) >> + >> +#define SNOR_HWCAPS_8_8_8_DTR (SNOR_HWCAPS_READ_8_8_8_DTR | \ >> + SNOR_HWCAPS_PP_8_8_8_DTR) >> + >> #define SNOR_HWCAPS_X_X_X (SNOR_HWCAPS_READ_2_2_2 | \ >> SNOR_HWCAPS_READ_4_4_4 | \ >> SNOR_HWCAPS_READ_8_8_8 | \
On 20.12.2023 10:50, Michael Walle wrote: > Am 2023-12-20 09:20, schrieb Tudor Ambarus: >> On 19.12.2023 12:21, Jaime Liao wrote: >>> From: JaimeLiao <jaimeliao@mxic.com.tw> >>> >>> Discard corresponding HW capabilities to prevent carrying the >>> wrong protocol if no QUAD/Octal DTR enable function hooked. >>> >>> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw> >>> --- >>> changes in v2 >>> - Add SNOR_HWCAPS_8_8_8_DTR >>> - Restore the enable function judgement in spi_nor_set_octal_dtr() >>> - Restore the enable function judgement in spi_nor_quad_enable() >>> --- >>> drivers/mtd/spi-nor/core.c | 7 +++++++ >>> include/linux/mtd/spi-nor.h | 6 ++++++ >>> 2 files changed, 13 insertions(+) >>> >>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >>> index 1c443fe568cf..14359101c6cf 100644 >>> --- a/drivers/mtd/spi-nor/core.c >>> +++ b/drivers/mtd/spi-nor/core.c >>> @@ -2621,6 +2621,13 @@ static int spi_nor_default_setup(struct >>> spi_nor *nor, >>> */ >>> shared_mask = hwcaps->mask & params->hwcaps.mask; >>> >>> + /* Mask out Octal DTR if no enable function */ >>> + if (!params->set_octal_dtr) >>> + shared_mask &= ~SNOR_HWCAPS_8_8_8_DTR; >>> + >>> + if (!params->quad_enable) >>> + shared_mask &= ~SNOR_HWCAPS_4_4_4; >> >> and these should have been in the late init hook, and instead discard >> them from the params->hwcaps.mask. > > Maybe there is a better place to mask these bits. But IMHO the core > should do it on itself and we shouldn't need to provide an extra > hook function for every driver ourselves. The core knows that there > is no .octal_enable op and thus it shouldn't even try to enable > this mode. I meant in the core, at the end of the late_init_params(). Here: https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/tree/drivers/mtd/spi-nor/core.c?h=spi-nor/next#n2962 > >> And why is this extra check needed in the first place? For octal I >> assume it's needed by macronix, where the flash is octal capable, but >> there's no octal enable method defined yet. > > Because it is the generic spi nor flash driver, which doesn't provide > any of these enable x-mode helpers. right, that was my guess too. The commit message has to be updated. > >> What about the quad mode? Is >> there any flash that has no quad enable method defined but still caries >> quad mode in its hwcaps? > > No it is for completeness and correctness. At the moment we are always > setting a (random) default quad enable op, due to legacy reasons. > Still, the commit message shall indicate this. Can't add extra checks out of the blue. Especially since nobody is affected. Cheers, ta
Hi, >>> and these should have been in the late init hook, and instead discard >>> them from the params->hwcaps.mask. >> >> Maybe there is a better place to mask these bits. But IMHO the core >> should do it on itself and we shouldn't need to provide an extra >> hook function for every driver ourselves. The core knows that there >> is no .octal_enable op and thus it shouldn't even try to enable >> this mode. > > I meant in the core, at the end of the late_init_params(). Here: > https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/tree/drivers/mtd/spi-nor/core.c?h=spi-nor/next#n2962 Sounds good. >> No it is for completeness and correctness. At the moment we are always >> setting a (random) default quad enable op, due to legacy reasons. >> > > Still, the commit message shall indicate this. Can't add extra checks > out of the blue. Especially since nobody is affected. Ok :) -michael
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 1c443fe568cf..14359101c6cf 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -2621,6 +2621,13 @@ static int spi_nor_default_setup(struct spi_nor *nor, */ shared_mask = hwcaps->mask & params->hwcaps.mask; + /* Mask out Octal DTR if no enable function */ + if (!params->set_octal_dtr) + shared_mask &= ~SNOR_HWCAPS_8_8_8_DTR; + + if (!params->quad_enable) + shared_mask &= ~SNOR_HWCAPS_4_4_4; + if (nor->spimem) { /* * When called from spi_nor_probe(), all caps are set and we diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index cdcfe0fd2e7d..78a119192ee0 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -266,6 +266,12 @@ struct spi_nor_hwcaps { #define SNOR_HWCAPS_PP_8_8_8 BIT(22) #define SNOR_HWCAPS_PP_8_8_8_DTR BIT(23) +#define SNOR_HWCAPS_4_4_4 (SNOR_HWCAPS_READ_4_4_4 | \ + SNOR_HWCAPS_PP_4_4_4) + +#define SNOR_HWCAPS_8_8_8_DTR (SNOR_HWCAPS_READ_8_8_8_DTR | \ + SNOR_HWCAPS_PP_8_8_8_DTR) + #define SNOR_HWCAPS_X_X_X (SNOR_HWCAPS_READ_2_2_2 | \ SNOR_HWCAPS_READ_4_4_4 | \ SNOR_HWCAPS_READ_8_8_8 | \