Re: [PATCH] spi: fsl-qspi: Reprogram the clock rate when the operation frequency changes

[han.xu]

On 26/09/17 04:10PM, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>
> 
> fsl_qspi_select_mem() returns early when the chip select has not changed,
> which happens before it reaches clk_set_rate(). Since the rate is now
> taken from the spi-mem operation rather than from the SPI device, the
> controller honours op->max_freq exactly once per chip select and ignores
> it for every operation after that.
> 
> q->selected is only reset to -1 in fsl_qspi_default_setup(), i.e. at probe
> and on resume, so on the common single chip select board the very first
> operation latches a rate that all subsequent operations inherit, whatever
> frequency they asked for.
> 
> This results in operations being issued with the wrong frequency.
> 
> Cache the operation frequency the clock was programmed for next to the
> selected chip select, and redo the clock setup when either changes.
> 
> Fixes: 2438db5253eb ("spi: fsl-qspi: Support per spi-mem operation frequency switches")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>

Acked-by: Han Xu <han.xu@xxxxxxx>

> ---
> The bug is visible on an i.MX6UL board with a Winbond SPI NAND and
> spi-max-frequency = <104000000>: the QSPI clock keeps the rate programmed
> for the first operation for the lifetime of the system, regardless of what
> later operations ask for.
> 
> Here is a simple showcase with a SPI NAND where the max freq for the
> read ops was artificially limited to 25 MHz.
> 
> cd /sys/kernel/debug/tracing
> echo 1 > events/clk/clk_set_rate/enable
> echo 1 > events/spi-mem/spi_mem_start_op/enable
> echo > trace
> dd if=/dev/mtd0 of=/dev/null bs=1k count=4
> 
> Without fix the clock is set to the high rate initially and never changed:
> 
> clk_set_rate: qspi1_podf 396000000
> clk_set_rate: qspi1 396000000
> [...]
> spi_mem_start_op: 21e0000.spi 1S-1S-0S @104000000 Hz op=[13-00-00-00] len=0 tx=[]
> spi_mem_start_op: 21e0000.spi 1S-1S-1S @104000000 Hz op=[0f-c0] len=1 tx=[]
> spi_mem_start_op: 21e0000.spi 1S-4S-4S @25000000 Hz op=[eb-00-00-ff-ff] len=1024 tx=[]
> spi_mem_start_op: 21e0000.spi 1S-4S-4S @25000000 Hz op=[eb-04-00-ff-ff] len=1024 tx=[]
> spi_mem_start_op: 21e0000.spi 1S-1S-0S @104000000 Hz op=[13-00-00-00] len=0 tx=[]
> spi_mem_start_op: 21e0000.spi 1S-1S-1S @104000000 Hz op=[0f-c0] len=1 tx=[]
> spi_mem_start_op: 21e0000.spi 1S-4S-4S @25000000 Hz op=[eb-00-00-ff-ff] len=1024 tx=[]
> spi_mem_start_op: 21e0000.spi 1S-4S-4S @25000000 Hz op=[eb-04-00-ff-ff] len=1024 tx=[]
> 
> With the fix the clock is actually changed according to what was
> requested by the op:
> 
> spi_mem_start_op: 21e0000.spi 1S-1S-0S @104000000 Hz op=[13-00-00-00] len=0 tx=[]
> clk_set_rate: qspi1_podf 396000000
> clk_set_rate: qspi1 396000000
> spi_mem_start_op: 21e0000.spi 1S-1S-1S @104000000 Hz op=[0f-c0] len=1 tx=[]
> spi_mem_start_op: 21e0000.spi 1S-4S-4S @25000000 Hz op=[eb-00-00-ff-ff] len=1024 tx=[]
> clk_set_rate: qspi1_podf 99000000
> clk_set_rate: qspi1 99000000
> spi_mem_start_op: 21e0000.spi 1S-4S-4S @25000000 Hz op=[eb-04-00-ff-ff] len=1024 tx=[]
> spi_mem_start_op: 21e0000.spi 1S-1S-0S @104000000 Hz op=[13-00-00-00] len=0 tx=[]
> clk_set_rate: qspi1_podf 396000000
> clk_set_rate: qspi1 396000000
> spi_mem_start_op: 21e0000.spi 1S-1S-1S @104000000 Hz op=[0f-c0] len=1 tx=[]
> spi_mem_start_op: 21e0000.spi 1S-4S-4S @25000000 Hz op=[eb-00-00-ff-ff] len=1024 tx=[]
> clk_set_rate: qspi1_podf 99000000
> clk_set_rate: qspi1 99000000
> spi_mem_start_op: 21e0000.spi 1S-4S-4S @25000000 Hz op=[eb-04-00-ff-ff] len=1024 tx=[]
> ---
>  drivers/spi/spi-fsl-qspi.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-fsl-qspi.c b/drivers/spi/spi-fsl-qspi.c
> index 57358851029b..d2c2090442f8 100644
> --- a/drivers/spi/spi-fsl-qspi.c
> +++ b/drivers/spi/spi-fsl-qspi.c
> @@ -289,6 +289,7 @@ struct fsl_qspi {
>  	struct pm_qos_request pm_qos_req;
>  	struct device *dev;
>  	int selected;
> +	u32 selected_freq;
>  	u32 memmap_phy;
>  };
>  
> @@ -551,7 +552,8 @@ static void fsl_qspi_select_mem(struct fsl_qspi *q, struct spi_device *spi,
>  	unsigned long rate = op->max_freq;
>  	int ret;
>  
> -	if (q->selected == spi_get_chipselect(spi, 0))
> +	if (q->selected == spi_get_chipselect(spi, 0) &&
> +	    q->selected_freq == op->max_freq)
>  		return;
>  
>  	if (needs_4x_clock(q))
> @@ -571,6 +573,7 @@ static void fsl_qspi_select_mem(struct fsl_qspi *q, struct spi_device *spi,
>  	}
>  
>  	q->selected = spi_get_chipselect(spi, 0);
> +	q->selected_freq = op->max_freq;
>  
>  	fsl_qspi_invalidate(q);
>  }
> 
> ---
> base-commit: 238650ef6c7c7cca08e032527329424c9fbd70e5
> change-id: 20260917-fsl-qspi-freq-op-fix-f38fedb13100
> 
> Best regards,
> --  
> Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>
>