Re: [PATCH v14 03/15] mtd: spi-nor: add support for DTR protocol

[Pratyush Yadav]

On 01/10/20 07:46AM, Tudor.Ambarus@xxxxxxxxxxxxx wrote:
> On 9/30/20 9:57 PM, Pratyush Yadav wrote:
> > @@ -2387,12 +2496,16 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor 
> > *nor, u32 *hwcaps)
> >         struct spi_nor_flash_parameter *params = nor->params;
> >         unsigned int cap;
> > 
> > -       /* DTR modes are not supported yet, mask them all. */
> > -       *hwcaps &= ~SNOR_HWCAPS_DTR;
> > -
> >         /* X-X-X modes are not supported yet, mask them all. */
> >         *hwcaps &= ~SNOR_HWCAPS_X_X_X;
> > 
> > +       /*
> > +        * If the reset line is broken, we do not want to enter a stateful
> > +        * mode.
> > +        */
> > +       if (nor->flags & SNOR_F_BROKEN_RESET)
> > +               *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR);
> 
> SNOR_HWCAPS_X_X_X is already masked out above. Do we need to do it again?

That might change later and the person removing that line might not 
remember or even know to add it back here.
 
> > +
> >         for (cap = 0; cap < sizeof(*hwcaps) * BITS_PER_BYTE; cap++) {
> >                 int rdidx, ppidx;
> > 
> > @@ -2967,7 +3098,9 @@ static int spi_nor_init(struct spi_nor *nor)
> >                 return err;
> >         }
> > 
> > -       if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
> > +       if (nor->addr_width == 4 &&
> > +           nor->read_proto != SNOR_PROTO_8_8_8_DTR &&
> > +           !(nor->flags & SNOR_F_4B_OPCODES)) {
> >                 /*
> >                  * If the RESET# pin isn't hooked up properly, or the system
> >                  * otherwise doesn't perform a reset command in the boot
> > @@ -3024,7 +3157,21 @@ static const struct flash_info *spi_nor_match_id(struct spi_nor *nor,
> > 
> >  static int spi_nor_set_addr_width(struct spi_nor *nor)
> >  {
> > -       if (nor->addr_width) {
> > +       if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
> 
> This should come as an "else if". We need to let the posibility to retrieve
> addr_width from SFDP, the standard knows better than us.

Ok. Will fix.
 
> With these addressed, one can add:
> 
> Reviewed-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>
> 
> > +               /*
> > +                * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
> > +                * in this protocol an odd address width cannot be used because
> > +                * then the address phase would only span a cycle and a half.
> > +                * Half a cycle would be left over. We would then have to start
> > +                * the dummy phase in the middle of a cycle and so too the data
> > +                * phase, and we will end the transaction with half a cycle left
> > +                * over.
> > +                *
> > +                * Force all 8D-8D-8D flashes to use an address width of 4 to
> > +                * avoid this situation.
> > +                */
> > +               nor->addr_width = 4;
> > +       } else if (nor->addr_width) {
> >                 /* already configured from SFDP */
> >         } else if (nor->info->addr_width) {
> >                 nor->addr_width = nor->info->addr_width;

-- 
Regards,
Pratyush Yadav
Texas Instruments India