RE: [PATCH v2 2/5] spi: imx: mx51-ecspi: Move some initialisation to prepare_message hook.

[Robin Gong <yibin.gong@xxxxxxx>]

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> Sent: 2018年11月30日 18:11
> To: Robin Gong <yibin.gong@xxxxxxx>
> Cc: Mark Brown <broonie@xxxxxxxxxx>; Marek Vasut <marex@xxxxxxx>;
> dl-linux-imx <linux-imx@xxxxxxx>; kernel@xxxxxxxxxxxxxx;
> linux-spi@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 2/5] spi: imx: mx51-ecspi: Move some initialisation to
> prepare_message hook.
> 
> Hello Robin,
> 
> On Thu, Nov 29, 2018 at 12:53:33PM +0000, Robin Gong wrote:
> > > -----Original Message-----
> > > From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > > Sent: 2018年11月26日 17:18
> > > To: Robin Gong <yibin.gong@xxxxxxx>
> > > Cc: Mark Brown <broonie@xxxxxxxxxx>; Marek Vasut <marex@xxxxxxx>;
> > > dl-linux-imx <linux-imx@xxxxxxx>; kernel@xxxxxxxxxxxxxx;
> > > linux-spi@xxxxxxxxxxxxxxx
> > > Subject: Re: [PATCH v2 2/5] spi: imx: mx51-ecspi: Move some
> > > initialisation to prepare_message hook.
> > >
> > > Hello Robin,
> > >
> > > On Mon, Nov 26, 2018 at 09:05:53AM +0000, Robin Gong wrote:
> > > > > -----Original Message-----
> > > > > From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > > > > Sent: 2018年11月23日 16:52
> > > > > To: Mark Brown <broonie@xxxxxxxxxx>; Robin Gong
> > > <yibin.gong@xxxxxxx>
> > > > > Cc: Marek Vasut <marex@xxxxxxx>; dl-linux-imx
> > > > > <linux-imx@xxxxxxx>; kernel@xxxxxxxxxxxxxx;
> > > > > linux-spi@xxxxxxxxxxxxxxx
> > > > > Subject: [PATCH v2 2/5] spi: imx: mx51-ecspi: Move some
> > > > > initialisation to prepare_message hook.
> > > > >
> > > > > The relevant difference between prepare_message and config is
> > > > > that the former is run before the CS signal is asserted. So the
> > > > > polarity of the CLK line must be configured in prepare_message
> > > > > as an edge generated by config might already result in a latch of the
> MOSI line.
> > > > Is this a real problem fix patch or just refine in theory? I have
> > > > quick test and saw a big performance downgrade below on
> > > > i.mx6sl-evk
> > > > board(1.6MB/s->706KB/s)
> > >
> > > This is a real problem. If you have two spi devices on the bus with
> > > need modes with different CPOL it can happen that after a transfer
> > > on one CPOL is changed for the other while SS is already active and
> > > this is already interpreted as the first latch.
> > >
> > > > root@imx6qpdlsolox:~# dd if=/dev/mtd0 of=/dev/null bs=1M count=1
> > > > 1+0 records in
> > > > 1+0 records out
> > > > 1048576 bytes (1.0 MB, 1.0 MiB) copied, 1.48557 s, 706 kB/s
> > >
> > > I didn't test this, but I'm surprised about that. For
> > > single-transfer messages the effect should just be that with the
> > > patch the following
> > > happens:
> > >
> > > 	setup_spi_imx_registers()
> > > 	activate_gpio_for_SS()
> > >
> > > instead of
> > >
> > > 	activate_gpio_for_SS()
> > > 	setup_spi_imx_registers()
> > Yes, in theory your patch should not cause such performance issue
> > since you just move some jobs of spi configure ahead of
> > activate_gpio_for_SS(). But unfortunately, MX51_ECSPI_CTRL_BL_OFFSET
> > may be broken by PIO transfer in case some small data, such as small
> command send to SPI-NOR before 1M data reading.
> > Please refer to dynamic_burst related code of spi_imx_push(). If I
> > keep BL setting in
> > mx51_ecspi_prepare_transfer() instead of mx51_ecspi_prepare_message(),
> > the issue is gone. Below is my fix, please merge it in V3.
> >
> > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index
> > 32d1c4f..05f9e50 100644
> > --- a/drivers/spi/spi-imx.c
> > +++ b/drivers/spi/spi-imx.c
> > @@ -510,13 +510,6 @@ static int mx51_ecspi_prepare_message(struct
> spi_imx_data *spi_imx,
> >         /* set chip select to use */
> >         ctrl |= MX51_ECSPI_CTRL_CS(spi->chip_select);
> >
> > -       if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
> > -               ctrl |= (spi_imx->slave_burst * 8 - 1)
> > -                       << MX51_ECSPI_CTRL_BL_OFFSET;
> > -       else
> > -               ctrl |= (spi_imx->bits_per_word - 1)
> > -                       << MX51_ECSPI_CTRL_BL_OFFSET;
> > -
> >         /*
> >          * The ctrl register must be written first, with the EN bit set other
> >          * registers must not be written to.
> > @@ -570,6 +563,15 @@ static int mx51_ecspi_prepare_transfer(struct
> spi_imx_data *spi_imx,
> >         u32 ctrl = readl(spi_imx->base + MX51_ECSPI_CTRL);
> >         u32 clk = t->speed_hz, delay;
> >
> > +       /* Clear BL filed and set the right value */
> > +       ctrl &= ~MX51_ECSPI_CTRL_BL_MASK;
> > +       if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
> > +               ctrl |= (spi_imx->slave_burst * 8 - 1)
> > +                       << MX51_ECSPI_CTRL_BL_OFFSET;
> > +       else
> > +               ctrl |= (spi_imx->bits_per_word - 1)
> > +                       << MX51_ECSPI_CTRL_BL_OFFSET;
> > +
> 
> Even if I sent out v3 now, I wonder if the problem is just that I didn't clear
> MX51_ECSPI_CTRL_BL_MASK in mx51_ecspi_prepare_message()?
Yes, now V3 works on my side :), and I have other few comments. 
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König
> |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> pengutronix.de%2F&amp;data=02%7C01%7Cyibin.gong%40nxp.com%7Cf7233
> 64dd51541ee041608d656ac27ba%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C636791694728543175&amp;sdata=OLIzu5IDtKST6%2FRs8bbOiup
> MRJsXVG6mF7aH003XxJU%3D&amp;reserved=0  |