Re: [PATCH 03/24] spi: amd: Support per spi-mem operation frequency switches

[Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>]

On 10/25/24 5:14 PM, Miquel Raynal wrote:
> Every ->exec_op() call correctly configures the spi bus speed to the
> maximum allowed frequency for the memory using the constant spi default
> parameter. Since we can now have per-operation constraints, let's use
> the value that comes from the spi-mem operation structure instead. In
> case there is no specific limitation for this operation, the default spi
> device value will be given anyway.
> 
> This controller however performed a frequency check, which is also
> observed during the ->check_op() phase.
> 
> The per-operation frequency capability is thus advertised to the spi-mem
> core.
> 
> Cc: Sanjay R Mehta <sanju.mehta@xxxxxxx>
> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> ---
>  drivers/spi/spi-amd.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
> index 2245ad54b03a..f58dc6375582 100644
> --- a/drivers/spi/spi-amd.c
> +++ b/drivers/spi/spi-amd.c
> @@ -368,6 +368,9 @@ static bool amd_spi_supports_op(struct spi_mem *mem,
>  	    op->data.buswidth > 1 || op->data.nbytes > AMD_SPI_MAX_DATA)
>  		return false;
>  
> +	if (op->max_freq < AMD_SPI_MIN_HZ)

How about using mem->spi->controller->min_speed_hz intead?

> +		return false;

I find the check fine here, but I see however that amd_set_spi_freq()
duplicates the same, returning -EINVAL when speed_hz < AMD_SPI_MIN_HZ
> +
>  	return spi_mem_default_supports_op(mem, op);
>  }
>  
> @@ -443,7 +446,7 @@ static int amd_spi_exec_mem_op(struct spi_mem *mem,
>  
>  	amd_spi = spi_controller_get_devdata(mem->spi->controller);
>  
> -	ret = amd_set_spi_freq(amd_spi, mem->spi->max_speed_hz);
> +	ret = amd_set_spi_freq(amd_spi, op->max_freq);
>  	if (ret)
>  		return ret;

however the return code is checked just on this call, and completely
ignored in the 2 other calls. I find the code a bit ugly for the non
SPIMEM case, but maybe something for the amd owner to address.

Looking good,
ta
>  
> @@ -469,6 +472,10 @@ static const struct spi_controller_mem_ops amd_spi_mem_ops = {
>  	.supports_op = amd_spi_supports_op,
>  };
>  
> +static const struct spi_controller_mem_caps amd_spi_mem_caps = {
> +	.per_op_freq = true,
> +};
> +
>  static int amd_spi_host_transfer(struct spi_controller *host,
>  				   struct spi_message *msg)
>  {
> @@ -521,6 +528,7 @@ static int amd_spi_probe(struct platform_device *pdev)
>  	host->setup = amd_spi_host_setup;
>  	host->transfer_one_message = amd_spi_host_transfer;
>  	host->mem_ops = &amd_spi_mem_ops;
> +	host->mem_caps = &amd_spi_mem_caps;
>  	host->max_transfer_size = amd_spi_max_transfer_size;
>  	host->max_message_size = amd_spi_max_transfer_size;
>