| Message ID | 1480084688-24677-8-git-send-email-phil.edworthy@renesas.com |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
On 11/25/2016 03:38 PM, 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@renesas.com> Looks fine to me, but Dinh/Chin, can you check whether this doesn't break SoCFPGA ? > --- > 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; > + 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); >
Hi Phil, 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@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. 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 > > > ________________________________ > > Confidentiality Notice. > This message may contain information that is confidential or > otherwise protected from disclosure. If you are not the intended > recipient, you are hereby notified that any use, disclosure, > dissemination, distribution, or copying of this message, or any > attachments, is strictly prohibited. If you have received this > message in error, please advise the sender by reply e-mail, and > delete the message and any attachments. Thank you.
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@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
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; + 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);
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@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(-)