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

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

[Venkatesh Yadav Abbarapu]

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@amd.com>
Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@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;
+
 	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);
-- 
2.25.1

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

[Michal Simek]

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@amd.com>
> Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@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;
> +
>   	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

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

[Prasanth Mantena]

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@amd.com>
> > Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@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


> >   	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

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

[Abbarapu, Venkatesh]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Prasanth,

> -----Original Message-----
> From: Prasanth Mantena <p-mantena@ti.com>
> Sent: Friday, July 11, 2025 2:00 AM
> To: Simek, Michal <michal.simek@amd.com>
> Cc: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de;
> tudor.ambarus@linaro.org; j-humphreys@ti.com; marex@denx.de;
> jagan@amarulasolutions.com; vigneshr@ti.com; u-kumar1@ti.com;
> trini@konsulko.com; seanga2@gmail.com; caleb.connolly@linaro.org;
> sjg@chromium.org; william.zhang@broadcom.com; stefan_b@posteo.net;
> quentin.schulz@cherry.de; Takahiro.Kuwano@infineon.com; git (AMD-Xilinx)
> <git@amd.com>; Ashok Reddy Soma <ashok.reddy.soma@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@amd.com>
> > > Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@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
>
>
> > >           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

Re: [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@ti.com>
> > Sent: Friday, July 11, 2025 2:00 AM
> > To: Simek, Michal <michal.simek@amd.com>
> > Cc: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de;
> > tudor.ambarus@linaro.org; j-humphreys@ti.com; marex@denx.de;
> > jagan@amarulasolutions.com; vigneshr@ti.com; u-kumar1@ti.com;
> > trini@konsulko.com; seanga2@gmail.com; caleb.connolly@linaro.org;
> > sjg@chromium.org; william.zhang@broadcom.com; stefan_b@posteo.net;
> > quentin.schulz@cherry.de; Takahiro.Kuwano@infineon.com; git (AMD-Xilinx)
> > <git@amd.com>; Ashok Reddy Soma <ashok.reddy.soma@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@amd.com>
> > > > Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@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