[U-Boot] [PATCH v2 7/8] spi: cadence_qspi: Fix CS timings

[Phil Edworthy]

Hi Chin Liang,

On 28 November 2016 12:49 See, Chin Liang wrote:
> On Jum, 2016-11-25 at 14:38 +0000, Phil Edworthy wrote:
> >
> > The Cadence QSPI controller has specified overheads for the various
> > CS
> > times that are in addition to those programmed in to the Device Delay
> > register. The overheads are different for the delays.
> >
> > In addition, the existing code does not handle the case when the
> > delay
> > is less than a SCLK period.
> >
> > This change accurately calculates the additional delays in Ref
> > clocks.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy at renesas.com>
> > ---
> > v2:
> >  Was "spi: cadence_qspi: Fix CQSPI_CAL_DELAY calculation"
> >  Note only did the existing code not cope with the delay less than
> >  an SCLK period, but the calculation didn't round properly, and
> >  didn't take into account the delays that the QSPI Controller adds
> >  to those programmed into the Device Delay reg.
> > ---
> >  drivers/spi/cadence_qspi_apb.c | 23 ++++++++++++-----------
> >  1 file changed, 12 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/spi/cadence_qspi_apb.c
> > b/drivers/spi/cadence_qspi_apb.c
> > index 1cd636a..56ad952 100644
> > --- a/drivers/spi/cadence_qspi_apb.c
> > +++ b/drivers/spi/cadence_qspi_apb.c
> > @@ -169,9 +169,6 @@
> >         ((readl(base + CQSPI_REG_CONFIG) >>             \
> >                 CQSPI_REG_CONFIG_IDLE_LSB) & 0x1)
> >
> > -#define CQSPI_CAL_DELAY(tdelay_ns, tref_ns, tsclk_ns)          \
> > -       ((((tdelay_ns) - (tsclk_ns)) / (tref_ns)))
> > -
> >  #define CQSPI_GET_RD_SRAM_LEVEL(reg_base)                      \
> >         (((readl(reg_base + CQSPI_REG_SDRAMLEVEL)) >>   \
> >         CQSPI_REG_SDRAMLEVEL_RD_LSB) &
> CQSPI_REG_SDRAMLEVEL_RD_MASK)
> > @@ -357,16 +354,20 @@ void cadence_qspi_apb_delay(void *reg_base,
> >         cadence_qspi_apb_controller_disable(reg_base);
> >
> >         /* Convert to ns. */
> > -       ref_clk_ns = (1000000000) / ref_clk;
> > +       ref_clk_ns = DIV_ROUND_UP(1000000000, ref_clk);
> >
> >         /* Convert to ns. */
> > -       sclk_ns = (1000000000) / sclk_hz;
> > -
> > -       /* Plus 1 to round up 1 clock cycle. */
> > -       tshsl = CQSPI_CAL_DELAY(tshsl_ns, ref_clk_ns, sclk_ns) + 1;
> > -       tchsh = CQSPI_CAL_DELAY(tchsh_ns, ref_clk_ns, sclk_ns) + 1;
> > -       tslch = CQSPI_CAL_DELAY(tslch_ns, ref_clk_ns, sclk_ns) + 1;
> > -       tsd2d = CQSPI_CAL_DELAY(tsd2d_ns, ref_clk_ns, sclk_ns) + 1;
> > +       sclk_ns = DIV_ROUND_UP(1000000000, sclk_hz);
> > +
> > +       /* The controller adds additional delay to that programmed in
> > the reg */
> > +       if (tshsl_ns >= sclk_ns + ref_clk_ns)
> > +               tshsl_ns -= sclk_ns + ref_clk_ns;
> > +       if (tchsh_ns >= sclk_ns + 3 * ref_clk_ns)
> > +               tchsh_ns -= sclk_ns + 3 * ref_clk_ns;
> Any reason we need this?
> The DIV_ROUND_UP or previous + 1 in algo will ensure its more than a
> SCLK period.
In general, all of these CS timing should be optimal. I measured differences
in throughput with the sub-optimal setting. Admittedly, the difference is
small but still we should set it correctly.

> Thanks
> Chin Liang
> 
> >
> > +       tshsl = DIV_ROUND_UP(tshsl_ns, ref_clk_ns);
> > +       tchsh = DIV_ROUND_UP(tchsh_ns, ref_clk_ns);
> > +       tslch = DIV_ROUND_UP(tslch_ns, ref_clk_ns);
> > +       tsd2d = DIV_ROUND_UP(tsd2d_ns, ref_clk_ns);
> >
> >         reg = ((tshsl & CQSPI_REG_DELAY_TSHSL_MASK)
> >                         << CQSPI_REG_DELAY_TSHSL_LSB);
> > --
> > 2.7.4
> >

Thanks
Phil