[PATCH] spi: cadence_qspi: Set tshsl_ns to at least one sclk_ns

[Prasanth Mantena]

On 15:12, Abbarapu, Venkatesh wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> Hi Prasanth,
> 
> > -----Original Message-----
> > From: Prasanth Mantena <p-mantena at ti.com>
> > Sent: Friday, July 11, 2025 2:00 AM
> > To: Simek, Michal <michal.simek at amd.com>
> > Cc: Abbarapu, Venkatesh <venkatesh.abbarapu at amd.com>; u-boot at lists.denx.de;
> > tudor.ambarus at linaro.org; j-humphreys at ti.com; marex at denx.de;
> > jagan at amarulasolutions.com; vigneshr at ti.com; u-kumar1 at ti.com;
> > trini at konsulko.com; seanga2 at gmail.com; caleb.connolly at linaro.org;
> > sjg at chromium.org; william.zhang at broadcom.com; stefan_b at posteo.net;
> > quentin.schulz at cherry.de; Takahiro.Kuwano at infineon.com; git (AMD-Xilinx)
> > <git at amd.com>; Ashok Reddy Soma <ashok.reddy.soma at amd.com>
> > Subject: Re: [PATCH] spi: cadence_qspi: Set tshsl_ns to at least one sclk_ns
> >
> > On 15:07, Michal Simek wrote:
> > >
> > >
> > > On 7/2/25 08:57, Venkatesh Yadav Abbarapu wrote:
> > > > tshsl_ns is the clock delay for chip select deassert. This is the
> > > > delay in master reference clocks for the length that the master mode
> > > > chip select outputs are de-asserted between transactions.
> > > >
> > > > The minimum delay is always SCLK period to ensure the chip select is
> > > > never re-asserted within one SCLK period.
> > > >
> > > > That is why tshsl_ns delay should be at least one sclk_ns value. If
> > > > it is less than sclk_ns, set it equal to sclk_ns.
> > > >
> > > > Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma at amd.com>
> > > > Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu at amd.com>
> > > > ---
> > > >   drivers/spi/cadence_qspi_apb.c | 4 ++++
> > > >   1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/drivers/spi/cadence_qspi_apb.c
> > > > b/drivers/spi/cadence_qspi_apb.c index 65fb2d8f9fb..4696c09f754
> > > > 100644
> > > > --- a/drivers/spi/cadence_qspi_apb.c
> > > > +++ b/drivers/spi/cadence_qspi_apb.c
> > > > @@ -303,6 +303,10 @@ void cadence_qspi_apb_delay(void *reg_base,
> > > >                   tshsl_ns -= sclk_ns + ref_clk_ns;
> > > >           if (tchsh_ns >= sclk_ns + 3 * ref_clk_ns)
> > > >                   tchsh_ns -= sclk_ns + 3 * ref_clk_ns;
> > > > +
> > > > + if (tshsl_ns < sclk_ns)
> > > > +         tshsl_ns = sclk_ns;
> > > > +
> >
> > Hi Venkatesh,
> >
> > Just referring to the Controller datasheet, I found this in the register map for the tshsl
> > delay register field.
> >
> > Ref : 2.3.4. Device Delay Register
> > "
> > The minimum delay for chip select to be de-asserted (CSDA=0)
> > is:
> > 1 sclk_out + 1 ref_clk to ensure the chip select is never re- asserted within an
> > sclk_out period.
> >
> > "
> > if this delay configured is exactly equals to sclk_out, and if sclk_out roundsup exactly
> > to ref_clk without a headroom, then the next transaction would start right after next
> > spike, unless if the controller adds any delay apart from whats configured in this
> > register.
> > May be thats why it is having that one ref_clk tick extra added acc to reg map.
> > Please help me understand this.
> >
> > Thanks,
> > Prasanth
> 
> If the calculated tshsl_ns value (after potentially subtracting the inherent delay) falls below sclk_ns, this forces it up to sclk_ns. This ensures that even if your initial request or the controller's base delay logic could result in a shorter actual delay, the minimum 1 SCLK idle time is always guaranteed by overriding it. Ensures CS# remains asserted long enough for the slave device to properly latch data. Avoids signal integrity issues due to too short hold times.
> 
> Thanks
> Venkatesh

Hi Venkatesh,

I understood the purpose of the override that is being done here. When
you are overriding it equal to sck_ns, what I understand is it should
have been with one more ref_clk_ns along with the sclk_out_ns
considering the data/next transaction has been given a ref_clk_ns
buffer. I have put the quoted text in previous reply, where the register
map of the controller states it to be minimum "1 sclk_out + 1 ref_clk".
Thats straight out of the controller document. Can you please justify,
why is ref_clk_ns would not be needed.

Thanks
Prasanth

> >
> >
> > > >           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);
> > >
> > > Applied.
> > > M