[U-Boot] [PATCH v2 17/18] spi: mtk_qspi: add qspi driver for MT7629 SoC

[Guochun Mao]

On Fri, 2018-11-23 at 11:13 +0530, Jagan Teki wrote:
> On Thu, Nov 22, 2018 at 2:29 PM Guochun Mao <guochun.mao at mediatek.com> wrote:
> >
> > On Thu, 2018-11-22 at 11:51 +0530, Jagan Teki wrote:
> > > On Wed, Nov 21, 2018 at 5:16 PM Guochun Mao <guochun.mao at mediatek.com> wrote:
> > > >
> > > > Hi Jagan,
> > > >
> > > > On Wed, 2018-11-21 at 15:08 +0530, Jagan Teki wrote:
> > > >
> > > > > > > > +static int mtk_qspi_tx_rx(struct mtk_qspi_priv *priv)
> > > > > > > > +{
> > > > > > > > +       int len = 1 + priv->txlen + priv->rxlen;
> > > > > > > > +       int i, ret, idx;
> > > > > > > > +
> > > > > > > > +       if (len > MTK_QSPI_MAX_SHIFT)
> > > > > > > > +               return -ERR_INVAL;
> > > > > > > > +
> > > > > > > > +       writeb(len * 8, &priv->regs->cnt);
> > > > > > > > +
> > > > > > > > +       /* start at PRGDATA5, go down to PRGDATA0 */
> > > > > > > > +       idx = MTK_QSPI_MAX_RX_TX_SHIFT - 1;
> > > > > > > > +
> > > > > > > > +       /* opcode */
> > > > > > > > +       writeb(priv->op, &priv->regs->prgdata[idx]);
> > > > > > > > +       idx--;
> > > > > > > > +
> > > > > > > > +       /* program TX data */
> > > > > > > > +       for (i = 0; i < priv->txlen; i++, idx--)
> > > > > > > > +               writeb(priv->tx[i], &priv->regs->prgdata[idx]);
> > > > > > > > +
> > > > > > > > +       /* clear out rest of TX registers */
> > > > > > > > +       while (idx >= 0) {
> > > > > > > > +               writeb(0, &priv->regs->prgdata[idx]);
> > > > > > > > +               idx--;
> > > > > > > > +       }
> > > > > > > > +
> > > > > > > > +       ret = mtk_qspi_execute_cmd(priv, MTK_QSPI_PRG_CMD);
> > > > > > >
> > > > > > > What does this execute do?
> > > > > > It send command to flash, and latch data if need.
> > > > > >
> > > > > > > does it intiate the controller register
> > > > > > > based flash command or so?
> > > > > > No, it doesn't.
> > > > > >
> > > > > > >
> > > > > > > Do you have Linux driver on the controller?
> > > > > > Yes, it's drivers/mtd/spi-nor/mtk-quadspi.c
> > > > >
> > > > > This sounds more specific to flash controller rather than spi driver.
> > > > > Can you try to write driver in mtd side itself, like Linux spi-nor.
> > > > > use UCLASS_SPI_FLASH
> > > >
> > > > Sorry, I'm a little confused.
> > > > There are many files(***qspi.c) those that used for accessing spi flash
> > > > under folder drivers/spi/.
> > > > However, there's no specific flash controller driver implemented under
> > > > drives/mtd, only common spi_flash framework. It's different with kernel.
> > > > It seems that we only need implement the spi control logic of
> > > > spi-flash-controller(this part not based on flash, it only do
> > > > data-xfer), and spi_flash framework will work well base on it.
> > > > Isn't that the purpose of this architecture?
> > >
> > > Understand your point, to be precise the driver may a fall in trouble
> > > after sometime, if there is an additional flash specific features will
> > > attach in future. ie reason few of flash controllers in drivers/spi
> > > were unable to move further to add their features. and ie also reason
> > > for your driver in Linux which resides in spi-nor.
> > >
> > > You can directly write UCLASS_SPI_FLASH like sf_dataflash, I can help
> > > if any issues with probing setup etc.
> >
> > Hi Jagan,
> >
> > Could you reconsider this driver?
> >
> > The code can match our needs for a considerable time, it's simple and
> > doesn't disrupt current architecture.
> >
> > If we switched to MTD_SPI_FLASH, there will inevitably be some
> > duplicated codes. The current version of code adopts the sf/spi
> > framework, it can guarantee compatibility and code reusability ratio.
> >
> > If spi-mem or spi-nor was introduced to uboot in future, we can quickly
> > implement new version code.
> 
> Thanks.
> 
> Applied to u-boot-spi/master

Thanks for your great effort for the review.

Guochun