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

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Tudor Ambarus <tudor.ambarus@linaro.org>
Cc: Richard Weinberger <richard@nod.at>,
	 Vignesh Raghavendra <vigneshr@ti.com>,
	 Pratyush Yadav <pratyush@kernel.org>,
	 Michael Walle <michael@walle.cc>,
	 linux-mtd@lists.infradead.org,  Mark Brown <broonie@kernel.org>,
	 linux-spi@vger.kernel.org,  Steam Lin <stlin2@winbond.com>,
	 Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Sanjay R Mehta <sanju.mehta@amd.com>,  Han Xu <han.xu@nxp.com>,
	 Conor Dooley <conor.dooley@microchip.com>,
	 Daire McNamara <daire.mcnamara@microchip.com>,
	 Matthias Brugger <matthias.bgg@gmail.com>,
	 AngeloGioacchino Del Regno
	<angelogioacchino.delregno@collabora.com>,
	 Haibo Chen <haibo.chen@nxp.com>,
	 Yogesh Gaur <yogeshgaur.83@gmail.com>,
	 Heiko Stuebner <heiko@sntech.de>,
	 Michal Simek <michal.simek@amd.com>
Subject: Re: [PATCH 03/24] spi: amd: Support per spi-mem operation frequency switches
Date: Fri, 13 Dec 2024 12:20:25 +0100	[thread overview]
Message-ID: <87zfkzn81y.fsf@bootlin.com> (raw)
In-Reply-To: <64ade17d-3800-4afc-847c-b8e5fc5d7360@linaro.org> (Tudor Ambarus's message of "Mon, 11 Nov 2024 13:36:15 +0000")

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/