[PATCH] mtd: spi-nor: core: Introduce spi_nor_abort_octal_dtr()

[PATCH] mtd: spi-nor: core: Introduce spi_nor_abort_octal_dtr()

[Jaime Liao]

From: JaimeLiao <jaimeliao@mxic.com.tw>

Some flashes contains 8D_8D_8D information in SFDP but did not enter
octal DTR mode if conditions are not satisfied.
However, still carry the opcode and dummy cycle for the octal DTR
protocol.
So that spi_nor_abort_octal_dtr() could abort octal DTR capability
then recall spi_nor_select_read/pp() for re-select a suitable
protocol.

Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
---
 drivers/mtd/spi-nor/core.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 1c443fe568cf..7cefab1d29d5 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3083,6 +3083,29 @@ static int spi_nor_init_params(struct spi_nor *nor)
 	return spi_nor_late_init_params(nor);
 }
 
+/** spi_nor_abort_octal_dtr() - abort octal dtr capability
+ * @nor:                pointer to a 'struct spi_nor'
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_nor_abort_octal_dtr(struct spi_nor *nor)
+{
+	int err;
+
+	/* Abort octal dtr capability and re-select read and pp */
+	nor->params->hwcaps.mask &= ~SNOR_HWCAPS_X_X_X_DTR;
+	err = spi_nor_select_read(nor, nor->params->hwcaps.mask);
+	if (err)
+		return err;
+
+	err = spi_nor_select_pp(nor, nor->params->hwcaps.mask);
+	if (err)
+		return err;
+
+	return 0;
+
+}
+
 /** spi_nor_set_octal_dtr() - enable or disable Octal DTR I/O.
  * @nor:                 pointer to a 'struct spi_nor'
  * @enable:              whether to enable or disable Octal DTR
@@ -3094,14 +3117,14 @@ static int spi_nor_set_octal_dtr(struct spi_nor *nor, bool enable)
 	int ret;
 
 	if (!nor->params->set_octal_dtr)
-		return 0;
+		return spi_nor_abort_octal_dtr(nor);
 
 	if (!(nor->read_proto == SNOR_PROTO_8_8_8_DTR &&
 	      nor->write_proto == SNOR_PROTO_8_8_8_DTR))
-		return 0;
+		return spi_nor_abort_octal_dtr(nor);
 
 	if (!(nor->flags & SNOR_F_IO_MODE_EN_VOLATILE))
-		return 0;
+		return spi_nor_abort_octal_dtr(nor);
 
 	ret = nor->params->set_octal_dtr(nor, enable);
 	if (ret)
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: core: Introduce spi_nor_abort_octal_dtr()

[Michael Walle]

> Some flashes contains 8D_8D_8D information in SFDP but did not enter
> octal DTR mode if conditions are not satisfied.

What exactly are these conditions? Rather than "abort" the octal mode,
the flash shouldn't have that capability in the first place.

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: core: Introduce spi_nor_abort_octal_dtr()

[liao jaime]

Hi Michael

>
> > Some flashes contains 8D_8D_8D information in SFDP but did not enter
> > octal DTR mode if conditions are not satisfied.
>
> What exactly are these conditions? Rather than "abort" the octal mode,
> the flash shouldn't have that capability in the first place.
"Abort" is not a good word.
Is it better for using "discard"?
3 conditions should be satisfied before enable octal dtr mode.
1. function hook in nor->params->set_octal_dtr
2. nor->read_proto and nor->write_proto are SNOR_PROTO_8_8_8_DTR
3. nor->flags & SNOR_F_IO_MODE_EN_VOLATILE
Flash driver still bring 8D_8D_8D protocol instructions in 1s-1s-1s mode if
conditions are not satisfied.
In a case, flash ID didn't include in vendor's ID table.
It will be "spi-nor-generic".
8D-8D-8D information could be parsed in SFDP but nor->params->set_octal_dtr
didn't hook vendor specific function for enabling octal dtr mode.
So that it still bring 8D protocol in 1s-1s-1s mode and have no chance
to select 1-1-8 protocol.
I think it may better to re-select a suitable protocol for this case.

>
> -michael
Thanks
Jaime

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: core: Introduce spi_nor_abort_octal_dtr()

[Michael Walle]

Hi,

>> > Some flashes contains 8D_8D_8D information in SFDP but did not enter
>> > octal DTR mode if conditions are not satisfied.
>> 
>> What exactly are these conditions? Rather than "abort" the octal mode,
>> the flash shouldn't have that capability in the first place.
> "Abort" is not a good word.
> Is it better for using "discard"?
> 3 conditions should be satisfied before enable octal dtr mode.
> 1. function hook in nor->params->set_octal_dtr
> 2. nor->read_proto and nor->write_proto are SNOR_PROTO_8_8_8_DTR
> 3. nor->flags & SNOR_F_IO_MODE_EN_VOLATILE
> Flash driver still bring 8D_8D_8D protocol instructions in 1s-1s-1s 
> mode if
> conditions are not satisfied.
> In a case, flash ID didn't include in vendor's ID table.
> It will be "spi-nor-generic".
> 8D-8D-8D information could be parsed in SFDP but 
> nor->params->set_octal_dtr
> didn't hook vendor specific function for enabling octal dtr mode.
> So that it still bring 8D protocol in 1s-1s-1s mode and have no chance
> to select 1-1-8 protocol.
> I think it may better to re-select a suitable protocol for this case.

Just that we are on the same page. You are using the spi-nor-generic
driver, but that driver will elect the 8d8d8d protocol but that won't
work, because we don't have a .set_octal_dtr. Therefore, the fallback
is 1s1s1s. Correct?

To me it seems, that spi_nor_select_{read,pp,erase}() will select
the wrong commands/proto. So it should be fixed there. Probably
shared_mask is wrong.

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: core: Introduce spi_nor_abort_octal_dtr()

[liao jaime]

Hi Michael


>
> Hi,
>
> >> > Some flashes contains 8D_8D_8D information in SFDP but did not enter
> >> > octal DTR mode if conditions are not satisfied.
> >>
> >> What exactly are these conditions? Rather than "abort" the octal mode,
> >> the flash shouldn't have that capability in the first place.
> > "Abort" is not a good word.
> > Is it better for using "discard"?
> > 3 conditions should be satisfied before enable octal dtr mode.
> > 1. function hook in nor->params->set_octal_dtr
> > 2. nor->read_proto and nor->write_proto are SNOR_PROTO_8_8_8_DTR
> > 3. nor->flags & SNOR_F_IO_MODE_EN_VOLATILE
> > Flash driver still bring 8D_8D_8D protocol instructions in 1s-1s-1s
> > mode if
> > conditions are not satisfied.
> > In a case, flash ID didn't include in vendor's ID table.
> > It will be "spi-nor-generic".
> > 8D-8D-8D information could be parsed in SFDP but
> > nor->params->set_octal_dtr
> > didn't hook vendor specific function for enabling octal dtr mode.
> > So that it still bring 8D protocol in 1s-1s-1s mode and have no chance
> > to select 1-1-8 protocol.
> > I think it may better to re-select a suitable protocol for this case.
>
> Just that we are on the same page. You are using the spi-nor-generic
> driver, but that driver will elect the 8d8d8d protocol but that won't
> work, because we don't have a .set_octal_dtr. Therefore, the fallback
> is 1s1s1s. Correct?
Yes.
Based on my understanding, typically, flash that support the 1-1-8 feature
also tends to support 8D-8D-8D feature.
However, in the case of spi-nor-generic, we don't have a .set_octal_dtr.
Therefore, I believe that if octal DTR mode cannot be enabled due to
this, it might be necessary to reconsider and choose a suitable feature.

>
> To me it seems, that spi_nor_select_{read,pp,erase}() will select
> the wrong commands/proto. So it should be fixed there. Probably
> shared_mask is wrong.
Features in shared_mask have include 2 parts as below.
1. Flash support the feature (by parse SFDP or ID table flags)
2. spi host controller support the feature (In spi_nor_spimem_adjust_hwcaps)

But I think Octal DTR is a special case because, in addition to the 2 points
mentioned above, there are also other conditions that need to be met.
My goal is to enable Octal DTR mode for Octal flashes listed in ID table.
For those not in ID table, choosing spi_nor_generic should allow selecting
either 1-1-8 protocol or, at the very least, 1-1-1 protocol.

>
> -michael

Thanks
Jaime

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: core: Introduce spi_nor_abort_octal_dtr()

[Michael Walle]

Hi,

>> To me it seems, that spi_nor_select_{read,pp,erase}() will select
>> the wrong commands/proto. So it should be fixed there. Probably
>> shared_mask is wrong.
> Features in shared_mask have include 2 parts as below.
> 1. Flash support the feature (by parse SFDP or ID table flags)
> 2. spi host controller support the feature (In 
> spi_nor_spimem_adjust_hwcaps)
> 
> But I think Octal DTR is a special case because, in addition to the 2 
> points
> mentioned above, there are also other conditions that need to be met.

Correct. And btw we would have the same problem for quad_enable if we 
wouldn't
have a default.

So in spi_nor_default_setup() I'd suggest to add something along

if (!nor->quad_enable)
   shared_mask &= ~SNOR_HWCAPS_4_4_4;

/* skip octal command mode if we don't have a .octal_enable callback */
if (!nor->octal_enable)
   shared_mask &= ~SNOR_HWCAPS_8_8_8;

#define SNOR_HWCAPS_4_4_4 (SNOR_HWCAPS_READ_4_4_4 | 
SNOR_HWCAPS_PP_4_4_4)

etc.


> My goal is to enable Octal DTR mode for Octal flashes listed in ID 
> table.
> For those not in ID table, choosing spi_nor_generic should allow 
> selecting
> either 1-1-8 protocol or, at the very least, 1-1-1 protocol.

That's understood.

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: core: Introduce spi_nor_abort_octal_dtr()

[liao jaime]

Hi Michael


>
> Hi,
>
> >> To me it seems, that spi_nor_select_{read,pp,erase}() will select
> >> the wrong commands/proto. So it should be fixed there. Probably
> >> shared_mask is wrong.
> > Features in shared_mask have include 2 parts as below.
> > 1. Flash support the feature (by parse SFDP or ID table flags)
> > 2. spi host controller support the feature (In
> > spi_nor_spimem_adjust_hwcaps)
> >
> > But I think Octal DTR is a special case because, in addition to the 2
> > points
> > mentioned above, there are also other conditions that need to be met.
>
> Correct. And btw we would have the same problem for quad_enable if we
> wouldn't
> have a default.
>
> So in spi_nor_default_setup() I'd suggest to add something along
>
> if (!nor->quad_enable)
>    shared_mask &= ~SNOR_HWCAPS_4_4_4;
I think this check is unnecessary for quad_enable.
Because of "spi_nor_sr2_bit1_quad_enable" would be a default
method for enabling quad mode in spi_nor_init_default_params().

>
> /* skip octal command mode if we don't have a .octal_enable callback */
> if (!nor->octal_enable)
>    shared_mask &= ~SNOR_HWCAPS_8_8_8;
Sound great.

Could I follow the suggestion for preparing v2 patch?
BTW, I think discard is better than abort.
What do you think?

>
> #define SNOR_HWCAPS_4_4_4 (SNOR_HWCAPS_READ_4_4_4 |
> SNOR_HWCAPS_PP_4_4_4)
>
> etc.
>
>
> > My goal is to enable Octal DTR mode for Octal flashes listed in ID
> > table.
> > For those not in ID table, choosing spi_nor_generic should allow
> > selecting
> > either 1-1-8 protocol or, at the very least, 1-1-1 protocol.
>
> That's understood.
>
> -michael

Thanks
Jaime

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: core: Introduce spi_nor_abort_octal_dtr()

[Michael Walle]

Hi,

>> >> To me it seems, that spi_nor_select_{read,pp,erase}() will select
>> >> the wrong commands/proto. So it should be fixed there. Probably
>> >> shared_mask is wrong.
>> > Features in shared_mask have include 2 parts as below.
>> > 1. Flash support the feature (by parse SFDP or ID table flags)
>> > 2. spi host controller support the feature (In
>> > spi_nor_spimem_adjust_hwcaps)
>> >
>> > But I think Octal DTR is a special case because, in addition to the 2
>> > points
>> > mentioned above, there are also other conditions that need to be met.
>> 
>> Correct. And btw we would have the same problem for quad_enable if we
>> wouldn't
>> have a default.
>> 
>> So in spi_nor_default_setup() I'd suggest to add something along
>> 
>> if (!nor->quad_enable)
>>    shared_mask &= ~SNOR_HWCAPS_4_4_4;
> I think this check is unnecessary for quad_enable.
> Because of "spi_nor_sr2_bit1_quad_enable" would be a default
> method for enabling quad mode in spi_nor_init_default_params().

Yes it is at the moment. But if a flash happen to have no
quad_enable we'll have a problem for whatever reason. so for the
sake of completeness we should mask that bit.

>> /* skip octal command mode if we don't have a .octal_enable callback 
>> */
>> if (!nor->octal_enable)
>>    shared_mask &= ~SNOR_HWCAPS_8_8_8;
> Sound great.
> 
> Could I follow the suggestion for preparing v2 patch?

sure.

> BTW, I think discard is better than abort.
> What do you think?

Where do you need that word? You shouln'd need any more
than the code above.

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: core: Introduce spi_nor_abort_octal_dtr()

[liao jaime]

Hi Michael


>
> Hi,
>
> >> >> To me it seems, that spi_nor_select_{read,pp,erase}() will select
> >> >> the wrong commands/proto. So it should be fixed there. Probably
> >> >> shared_mask is wrong.
> >> > Features in shared_mask have include 2 parts as below.
> >> > 1. Flash support the feature (by parse SFDP or ID table flags)
> >> > 2. spi host controller support the feature (In
> >> > spi_nor_spimem_adjust_hwcaps)
> >> >
> >> > But I think Octal DTR is a special case because, in addition to the 2
> >> > points
> >> > mentioned above, there are also other conditions that need to be met.
> >>
> >> Correct. And btw we would have the same problem for quad_enable if we
> >> wouldn't
> >> have a default.
> >>
> >> So in spi_nor_default_setup() I'd suggest to add something along
> >>
> >> if (!nor->quad_enable)
> >>    shared_mask &= ~SNOR_HWCAPS_4_4_4;
> > I think this check is unnecessary for quad_enable.
> > Because of "spi_nor_sr2_bit1_quad_enable" would be a default
> > method for enabling quad mode in spi_nor_init_default_params().
>
> Yes it is at the moment. But if a flash happen to have no
> quad_enable we'll have a problem for whatever reason. so for the
> sake of completeness we should mask that bit.
Got it.

>
> >> /* skip octal command mode if we don't have a .octal_enable callback
> >> */
> >> if (!nor->octal_enable)
> >>    shared_mask &= ~SNOR_HWCAPS_8_8_8;
> > Sound great.
> >
> > Could I follow the suggestion for preparing v2 patch?
>
> sure.
>
> > BTW, I think discard is better than abort.
> > What do you think?
>
> Where do you need that word? You shouln'd need any more
> than the code above.
Ok.

>
> -michael

Thanks
Jaime

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: core: Introduce spi_nor_abort_octal_dtr()

[Pratyush Yadav]

Hi,

On Wed, Dec 13 2023, liao jaime wrote:

> Hi Michael
>
>
>>
>> Hi,
>>
>> >> > Some flashes contains 8D_8D_8D information in SFDP but did not enter
>> >> > octal DTR mode if conditions are not satisfied.
>> >>
>> >> What exactly are these conditions? Rather than "abort" the octal mode,
>> >> the flash shouldn't have that capability in the first place.
>> > "Abort" is not a good word.
>> > Is it better for using "discard"?
>> > 3 conditions should be satisfied before enable octal dtr mode.
>> > 1. function hook in nor->params->set_octal_dtr
>> > 2. nor->read_proto and nor->write_proto are SNOR_PROTO_8_8_8_DTR
>> > 3. nor->flags & SNOR_F_IO_MODE_EN_VOLATILE
>> > Flash driver still bring 8D_8D_8D protocol instructions in 1s-1s-1s
>> > mode if
>> > conditions are not satisfied.
>> > In a case, flash ID didn't include in vendor's ID table.
>> > It will be "spi-nor-generic".
>> > 8D-8D-8D information could be parsed in SFDP but
>> > nor->params->set_octal_dtr
>> > didn't hook vendor specific function for enabling octal dtr mode.
>> > So that it still bring 8D protocol in 1s-1s-1s mode and have no chance
>> > to select 1-1-8 protocol.
>> > I think it may better to re-select a suitable protocol for this case.
>>
>> Just that we are on the same page. You are using the spi-nor-generic
>> driver, but that driver will elect the 8d8d8d protocol but that won't
>> work, because we don't have a .set_octal_dtr. Therefore, the fallback
>> is 1s1s1s. Correct?
> Yes.
> Based on my understanding, typically, flash that support the 1-1-8 feature
> also tends to support 8D-8D-8D feature.
> However, in the case of spi-nor-generic, we don't have a .set_octal_dtr.
> Therefore, I believe that if octal DTR mode cannot be enabled due to
> this, it might be necessary to reconsider and choose a suitable feature.

Can't you parse the SCCR to learn how to enable 8D-8D-8D mode for
generic flash drivers? That way you can set octal_enable() for generic
flashes as well.

To be clear, I still think you should implement Michael's suggestions
about masking out SNOR_HWCAPS_8_8_8. This is an enhancement that you can
add on top to enable Octal DTR for generic flashes via SFDP. You seem to
have a flash capable of doing that so might as well enable its full
powers.

[...]

-- 
Regards,
Pratyush Yadav



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/