Re: [PATCH v5 05/19] mtd: spi-nor: add support for DTR protocol

[Pratyush Yadav <p.yadav@xxxxxx>]

On 22/05/20 02:30PM, masonccyang@xxxxxxxxxxx wrote:
> 
> Hi Pratyush,
> 
> 
> > +/**
> > + * spi_nor_spimem_setup_op() - Set up common properties of a spi-mem 
> op.
> > + * @nor:      pointer to a 'struct spi_nor'
> > + * @op:         pointer to the 'struct spi_mem_op' whose properties
> > + *         need to be initialized.
> > + * @proto:      the protocol from which the properties need to be set.
> > + */
> > +void spi_nor_spimem_setup_op(const struct spi_nor *nor,
> > +              struct spi_mem_op *op,
> > +              const enum spi_nor_protocol proto)
> > +{
> > +   u8 ext;
> > +
> > +   op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(proto);
> > +
> > +   if (op->addr.nbytes)
> > +      op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto);
> > +
> > +   if (op->dummy.nbytes)
> > +      op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto);
> > +
> > +   if (op->data.nbytes)
> > +      op->data.buswidth = spi_nor_get_protocol_data_nbits(proto);
> > +
> > +   if (spi_nor_protocol_is_dtr(proto)) {
> 
> As mentioned before that I am also patching mx25* which supports 8S-8S-8S 
> and 
> 8D-8D-8D mode.
> 
> please patch to spi_nor_protocol_is_8_8_8(proto) for 8S-8S-8S mode 
> support.

Like I said before, we should try to avoid creeping up the scope of this 
series. This series aims to add 8D support. Once this lands, I don't see 
why you can't 8S support on top. Unless we make a fundamental change 
that makes it impossible to add 8S support, it should stay as-is.

All that said, I fail to see why 8S would have any problems with this 
function. We just fill in the buswidths from the protocol, and adjust 
the op if it is DTR. So in case of 8S mode, this function as it is will 
fill in the buswidths to 8 for all phases. And it won't hit the if block 
here so this code is of no concern to 8S mode.
 
> > +      /*
> > +       * spi-mem supports mixed DTR modes, but right now we can only
> > +       * have all phases either DTR or STR. IOW, spi-mem can have
> > +       * something like 4S-4D-4D, but spi-nor can't. So, set all 4
> > +       * phases to either DTR or STR.
> > +       */
> 
>         if (spi_nor_protocol_is_8D_8D_8D(proto) {

No. The adjustments below apply to _all_ DTR ops, not just 8D-8D-8D 
ones. So in case someone wants to use 4D-4D-4D mode, they won't have to 
touch this code at all.
 
> > +      op->cmd.dtr = op->addr.dtr = op->dummy.dtr
> > +                = op->data.dtr = true;
> > +
> > +      /* 2 bytes per clock cycle in DTR mode. */
> > +      op->dummy.nbytes *= 2;
> 
>         }
> 
> > +
> > +      ext = spi_nor_get_cmd_ext(nor, op);
> > +      op->cmd.opcode = (op->cmd.opcode << 8) | ext;
> > +      op->cmd.nbytes = 2;
> > +   }
> > +}
> > +

-- 
Regards,
Pratyush Yadav
Texas Instruments India