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

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@amd.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-amd.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

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@amd.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  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;
>

Hi Tudor,

On 11/11/2024 at 13:36:15 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

> 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@amd.com>
>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> ---
>>  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?

Good idea. I think I used AMD_SPI_MIN_HZ to follow what was done
somewhere else, but that's fine.

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

This one is useless, the spi core already takes care of this check, I'll
drop it in a separate patch.

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

Once the above check removed (it does not make much sense there), the
function can return void.

Cheers,
Miquèl