| Message ID | 20231207075147.21851-1-jaimeliao.tw@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | mtd: spi-nor: core: Introduce spi_nor_abort_octal_dtr() | expand |
> 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
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
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
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
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
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
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
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
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. [...]
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)