[PATCH v2 0/2] spi: amlogic-spifc-a1: fixes and improvements for amlogic-spifc-a1

[PATCH v2 0/2] spi: amlogic-spifc-a1: fixes and improvements for amlogic-spifc-a1

[Martin Kurbanov]

This series adds support for max_speed_hz and implement adjust_op_size()
callback.

Changes v2 since v1 at [1]:
  - Make cosmetic changes as Andy suggested at [1]

Links:
  [1] https://lore.kernel.org/all/20230703094518.53755-1-mmkurbanov@sberdevices.ru/

Martin Kurbanov (2):
  spi: amlogic-spifc-a1: implement adjust_op_size()
  spi: amlogic-spifc-a1: add support for max_speed_hz

 drivers/spi/spi-amlogic-spifc-a1.c | 86 ++++++++++++++++--------------
 1 file changed, 45 insertions(+), 41 deletions(-)

--
2.40.0

[PATCH v2 1/2] spi: amlogic-spifc-a1: implement adjust_op_size()

[Martin Kurbanov]

This enhancement eliminates the need for a loop in the
amlogic_spifc_a1_exec_op() function and allows the SPI core to
dynamically divide transactions into appropriately sized chunks.

Signed-off-by: Martin Kurbanov <mmkurbanov@sberdevices.ru>
---
 drivers/spi/spi-amlogic-spifc-a1.c | 66 +++++++++++-------------------
 1 file changed, 25 insertions(+), 41 deletions(-)

diff --git a/drivers/spi/spi-amlogic-spifc-a1.c b/drivers/spi/spi-amlogic-spifc-a1.c
index 3c4224c38399..a92e4fc23396 100644
--- a/drivers/spi/spi-amlogic-spifc-a1.c
+++ b/drivers/spi/spi-amlogic-spifc-a1.c
@@ -72,7 +72,7 @@
 
 #define SPIFC_A1_USER_DBUF_ADDR_REG	0x248
 
-#define SPIFC_A1_BUFFER_SIZE		512
+#define SPIFC_A1_BUFFER_SIZE		512U
 
 #define SPIFC_A1_MAX_HZ			200000000
 #define SPIFC_A1_MIN_HZ			1000000
@@ -240,61 +240,44 @@ static int amlogic_spifc_a1_exec_op(struct spi_mem *mem,
 {
 	struct amlogic_spifc_a1 *spifc =
 		spi_controller_get_devdata(mem->spi->controller);
-	size_t off, nbytes = op->data.nbytes;
-	u32 cmd_cfg, addr_cfg, dummy_cfg, dmode;
+	size_t data_size = op->data.nbytes;
 	int ret;
 
 	amlogic_spifc_a1_user_init(spifc);
+	amlogic_spifc_a1_set_cmd(spifc, SPIFC_A1_USER_CMD(op));
 
-	cmd_cfg = SPIFC_A1_USER_CMD(op);
-	amlogic_spifc_a1_set_cmd(spifc, cmd_cfg);
+	if (op->addr.nbytes)
+		amlogic_spifc_a1_set_addr(spifc, op->addr.val,
+					  SPIFC_A1_USER_ADDR(op));
 
-	if (op->addr.nbytes) {
-		addr_cfg = SPIFC_A1_USER_ADDR(op);
-		amlogic_spifc_a1_set_addr(spifc, op->addr.val, addr_cfg);
-	}
-
-	if (op->dummy.nbytes) {
-		dummy_cfg = SPIFC_A1_USER_DUMMY(op);
-		amlogic_spifc_a1_set_dummy(spifc, dummy_cfg);
-	}
-
-	if (!op->data.nbytes)
-		return amlogic_spifc_a1_request(spifc, false);
-
-	dmode = ilog2(op->data.buswidth);
-	off = 0;
-
-	do {
-		size_t block_size = min_t(size_t, nbytes, SPIFC_A1_BUFFER_SIZE);
-
-		amlogic_spifc_a1_set_cmd(spifc, cmd_cfg);
+	if (op->dummy.nbytes)
+		amlogic_spifc_a1_set_dummy(spifc, SPIFC_A1_USER_DUMMY(op));
 
-		if (op->addr.nbytes)
-			amlogic_spifc_a1_set_addr(spifc, op->addr.val + off,
-						  addr_cfg);
-
-		if (op->dummy.nbytes)
-			amlogic_spifc_a1_set_dummy(spifc, dummy_cfg);
+	if (data_size) {
+		u32 mode = ilog2(op->data.buswidth);
 
 		writel(0, spifc->base + SPIFC_A1_USER_DBUF_ADDR_REG);
 
 		if (op->data.dir == SPI_MEM_DATA_IN)
-			ret = amlogic_spifc_a1_read(spifc,
-						    op->data.buf.in + off,
-						    block_size, dmode);
+			ret = amlogic_spifc_a1_read(spifc, op->data.buf.in,
+						    data_size, mode);
 		else
-			ret = amlogic_spifc_a1_write(spifc,
-						     op->data.buf.out + off,
-						     block_size, dmode);
-
-		nbytes -= block_size;
-		off += block_size;
-	} while (nbytes != 0 && !ret);
+			ret = amlogic_spifc_a1_write(spifc, op->data.buf.out,
+						     data_size, mode);
+	} else {
+		ret = amlogic_spifc_a1_request(spifc, false);
+	}
 
 	return ret;
 }
 
+static int amlogic_spifc_a1_adjust_op_size(struct spi_mem *mem,
+					   struct spi_mem_op *op)
+{
+	op->data.nbytes = min(op->data.nbytes, SPIFC_A1_BUFFER_SIZE);
+	return 0;
+}
+
 static void amlogic_spifc_a1_hw_init(struct amlogic_spifc_a1 *spifc)
 {
 	u32 regv;
@@ -314,6 +297,7 @@ static void amlogic_spifc_a1_hw_init(struct amlogic_spifc_a1 *spifc)
 
 static const struct spi_controller_mem_ops amlogic_spifc_a1_mem_ops = {
 	.exec_op = amlogic_spifc_a1_exec_op,
+	.adjust_op_size = amlogic_spifc_a1_adjust_op_size,
 };
 
 static int amlogic_spifc_a1_probe(struct platform_device *pdev)
-- 
2.40.0

Re: [PATCH v2 1/2] spi: amlogic-spifc-a1: implement adjust_op_size()

[Neil Armstrong]

On 06/07/2023 13:03, Martin Kurbanov wrote:
> This enhancement eliminates the need for a loop in the
> amlogic_spifc_a1_exec_op() function and allows the SPI core to
> dynamically divide transactions into appropriately sized chunks.
> 
> Signed-off-by: Martin Kurbanov <mmkurbanov@sberdevices.ru>
> ---
>   drivers/spi/spi-amlogic-spifc-a1.c | 66 +++++++++++-------------------
>   1 file changed, 25 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/spi/spi-amlogic-spifc-a1.c b/drivers/spi/spi-amlogic-spifc-a1.c
> index 3c4224c38399..a92e4fc23396 100644
> --- a/drivers/spi/spi-amlogic-spifc-a1.c
> +++ b/drivers/spi/spi-amlogic-spifc-a1.c
> @@ -72,7 +72,7 @@
>   
>   #define SPIFC_A1_USER_DBUF_ADDR_REG	0x248
>   
> -#define SPIFC_A1_BUFFER_SIZE		512
> +#define SPIFC_A1_BUFFER_SIZE		512U
>   
>   #define SPIFC_A1_MAX_HZ			200000000
>   #define SPIFC_A1_MIN_HZ			1000000
> @@ -240,61 +240,44 @@ static int amlogic_spifc_a1_exec_op(struct spi_mem *mem,
>   {
>   	struct amlogic_spifc_a1 *spifc =
>   		spi_controller_get_devdata(mem->spi->controller);
> -	size_t off, nbytes = op->data.nbytes;
> -	u32 cmd_cfg, addr_cfg, dummy_cfg, dmode;
> +	size_t data_size = op->data.nbytes;
>   	int ret;
>   
>   	amlogic_spifc_a1_user_init(spifc);
> +	amlogic_spifc_a1_set_cmd(spifc, SPIFC_A1_USER_CMD(op));
>   
> -	cmd_cfg = SPIFC_A1_USER_CMD(op);
> -	amlogic_spifc_a1_set_cmd(spifc, cmd_cfg);
> +	if (op->addr.nbytes)
> +		amlogic_spifc_a1_set_addr(spifc, op->addr.val,
> +					  SPIFC_A1_USER_ADDR(op));
>   
> -	if (op->addr.nbytes) {
> -		addr_cfg = SPIFC_A1_USER_ADDR(op);
> -		amlogic_spifc_a1_set_addr(spifc, op->addr.val, addr_cfg);
> -	}
> -
> -	if (op->dummy.nbytes) {
> -		dummy_cfg = SPIFC_A1_USER_DUMMY(op);
> -		amlogic_spifc_a1_set_dummy(spifc, dummy_cfg);
> -	}
> -
> -	if (!op->data.nbytes)
> -		return amlogic_spifc_a1_request(spifc, false);
> -
> -	dmode = ilog2(op->data.buswidth);
> -	off = 0;
> -
> -	do {
> -		size_t block_size = min_t(size_t, nbytes, SPIFC_A1_BUFFER_SIZE);
> -
> -		amlogic_spifc_a1_set_cmd(spifc, cmd_cfg);
> +	if (op->dummy.nbytes)
> +		amlogic_spifc_a1_set_dummy(spifc, SPIFC_A1_USER_DUMMY(op));
>   
> -		if (op->addr.nbytes)
> -			amlogic_spifc_a1_set_addr(spifc, op->addr.val + off,
> -						  addr_cfg);
> -
> -		if (op->dummy.nbytes)
> -			amlogic_spifc_a1_set_dummy(spifc, dummy_cfg);
> +	if (data_size) {
> +		u32 mode = ilog2(op->data.buswidth);
>   
>   		writel(0, spifc->base + SPIFC_A1_USER_DBUF_ADDR_REG);
>   
>   		if (op->data.dir == SPI_MEM_DATA_IN)
> -			ret = amlogic_spifc_a1_read(spifc,
> -						    op->data.buf.in + off,
> -						    block_size, dmode);
> +			ret = amlogic_spifc_a1_read(spifc, op->data.buf.in,
> +						    data_size, mode);
>   		else
> -			ret = amlogic_spifc_a1_write(spifc,
> -						     op->data.buf.out + off,
> -						     block_size, dmode);
> -
> -		nbytes -= block_size;
> -		off += block_size;
> -	} while (nbytes != 0 && !ret);
> +			ret = amlogic_spifc_a1_write(spifc, op->data.buf.out,
> +						     data_size, mode);
> +	} else {
> +		ret = amlogic_spifc_a1_request(spifc, false);
> +	}
>   
>   	return ret;
>   }
>   
> +static int amlogic_spifc_a1_adjust_op_size(struct spi_mem *mem,
> +					   struct spi_mem_op *op)
> +{
> +	op->data.nbytes = min(op->data.nbytes, SPIFC_A1_BUFFER_SIZE);
> +	return 0;
> +}
> +
>   static void amlogic_spifc_a1_hw_init(struct amlogic_spifc_a1 *spifc)
>   {
>   	u32 regv;
> @@ -314,6 +297,7 @@ static void amlogic_spifc_a1_hw_init(struct amlogic_spifc_a1 *spifc)
>   
>   static const struct spi_controller_mem_ops amlogic_spifc_a1_mem_ops = {
>   	.exec_op = amlogic_spifc_a1_exec_op,
> +	.adjust_op_size = amlogic_spifc_a1_adjust_op_size,
>   };
>   
>   static int amlogic_spifc_a1_probe(struct platform_device *pdev)

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

[PATCH v2 2/2] spi: amlogic-spifc-a1: add support for max_speed_hz

[Martin Kurbanov]

This patch sets the clock rate (spi_transfer->max_speed_hz) from the
amlogic_spifc_a1_exec_op().

Signed-off-by: Martin Kurbanov <mmkurbanov@sberdevices.ru>
---
 drivers/spi/spi-amlogic-spifc-a1.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/spi/spi-amlogic-spifc-a1.c b/drivers/spi/spi-amlogic-spifc-a1.c
index a92e4fc23396..605e9e40455c 100644
--- a/drivers/spi/spi-amlogic-spifc-a1.c
+++ b/drivers/spi/spi-amlogic-spifc-a1.c
@@ -107,6 +107,7 @@ struct amlogic_spifc_a1 {
 	struct clk *clk;
 	struct device *dev;
 	void __iomem *base;
+	u32 curr_speed_hz;
 };
 
 static int amlogic_spifc_a1_request(struct amlogic_spifc_a1 *spifc, bool read)
@@ -235,6 +236,21 @@ static int amlogic_spifc_a1_write(struct amlogic_spifc_a1 *spifc,
 	return amlogic_spifc_a1_request(spifc, false);
 }
 
+static int amlogic_spifc_a1_set_freq(struct amlogic_spifc_a1 *spifc, u32 freq)
+{
+	int ret;
+
+	if (freq == spifc->curr_speed_hz)
+		return 0;
+
+	ret = clk_set_rate(spifc->clk, freq);
+	if (ret)
+		return ret;
+
+	spifc->curr_speed_hz = freq;
+	return 0;
+}
+
 static int amlogic_spifc_a1_exec_op(struct spi_mem *mem,
 				    const struct spi_mem_op *op)
 {
@@ -243,6 +259,10 @@ static int amlogic_spifc_a1_exec_op(struct spi_mem *mem,
 	size_t data_size = op->data.nbytes;
 	int ret;
 
+	ret = amlogic_spifc_a1_set_freq(spifc, mem->spi->max_speed_hz);
+	if (ret)
+		return ret;
+
 	amlogic_spifc_a1_user_init(spifc);
 	amlogic_spifc_a1_set_cmd(spifc, SPIFC_A1_USER_CMD(op));
 
-- 
2.40.0

Re: [PATCH v2 2/2] spi: amlogic-spifc-a1: add support for max_speed_hz

[Neil Armstrong]

On 06/07/2023 13:03, Martin Kurbanov wrote:
> This patch sets the clock rate (spi_transfer->max_speed_hz) from the
> amlogic_spifc_a1_exec_op().
> 
> Signed-off-by: Martin Kurbanov <mmkurbanov@sberdevices.ru>
> ---
>   drivers/spi/spi-amlogic-spifc-a1.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/spi/spi-amlogic-spifc-a1.c b/drivers/spi/spi-amlogic-spifc-a1.c
> index a92e4fc23396..605e9e40455c 100644
> --- a/drivers/spi/spi-amlogic-spifc-a1.c
> +++ b/drivers/spi/spi-amlogic-spifc-a1.c
> @@ -107,6 +107,7 @@ struct amlogic_spifc_a1 {
>   	struct clk *clk;
>   	struct device *dev;
>   	void __iomem *base;
> +	u32 curr_speed_hz;
>   };
>   
>   static int amlogic_spifc_a1_request(struct amlogic_spifc_a1 *spifc, bool read)
> @@ -235,6 +236,21 @@ static int amlogic_spifc_a1_write(struct amlogic_spifc_a1 *spifc,
>   	return amlogic_spifc_a1_request(spifc, false);
>   }
>   
> +static int amlogic_spifc_a1_set_freq(struct amlogic_spifc_a1 *spifc, u32 freq)
> +{
> +	int ret;
> +
> +	if (freq == spifc->curr_speed_hz)
> +		return 0;
> +
> +	ret = clk_set_rate(spifc->clk, freq);
> +	if (ret)
> +		return ret;
> +
> +	spifc->curr_speed_hz = freq;
> +	return 0;
> +}
> +
>   static int amlogic_spifc_a1_exec_op(struct spi_mem *mem,
>   				    const struct spi_mem_op *op)
>   {
> @@ -243,6 +259,10 @@ static int amlogic_spifc_a1_exec_op(struct spi_mem *mem,
>   	size_t data_size = op->data.nbytes;
>   	int ret;
>   
> +	ret = amlogic_spifc_a1_set_freq(spifc, mem->spi->max_speed_hz);
> +	if (ret)
> +		return ret;
> +
>   	amlogic_spifc_a1_user_init(spifc);
>   	amlogic_spifc_a1_set_cmd(spifc, SPIFC_A1_USER_CMD(op));
>   

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH v2 2/2] spi: amlogic-spifc-a1: add support for max_speed_hz

[Jerome Brunet]

On Thu 06 Jul 2023 at 14:03, Martin Kurbanov <mmkurbanov@sberdevices.ru> wrote:

> This patch sets the clock rate (spi_transfer->max_speed_hz) from the
> amlogic_spifc_a1_exec_op().
>
> Signed-off-by: Martin Kurbanov <mmkurbanov@sberdevices.ru>
> ---
>  drivers/spi/spi-amlogic-spifc-a1.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/drivers/spi/spi-amlogic-spifc-a1.c b/drivers/spi/spi-amlogic-spifc-a1.c
> index a92e4fc23396..605e9e40455c 100644
> --- a/drivers/spi/spi-amlogic-spifc-a1.c
> +++ b/drivers/spi/spi-amlogic-spifc-a1.c
> @@ -107,6 +107,7 @@ struct amlogic_spifc_a1 {
>  	struct clk *clk;
>  	struct device *dev;
>  	void __iomem *base;
> +	u32 curr_speed_hz;
>  };
>  
>  static int amlogic_spifc_a1_request(struct amlogic_spifc_a1 *spifc, bool read)
> @@ -235,6 +236,21 @@ static int amlogic_spifc_a1_write(struct amlogic_spifc_a1 *spifc,
>  	return amlogic_spifc_a1_request(spifc, false);
>  }
>  
> +static int amlogic_spifc_a1_set_freq(struct amlogic_spifc_a1 *spifc, u32 freq)
> +{
> +	int ret;
> +
> +	if (freq == spifc->curr_speed_hz)
> +		return 0;
> +
> +	ret = clk_set_rate(spifc->clk, freq);
> +	if (ret)
> +		return ret;
> +
> +	spifc->curr_speed_hz = freq;

There is no guarantee that clk_set_rate() has set the rate you have
requested, at least not precisely. You should call clk_get_rate() here.

> +	return 0;
> +}
> +
>  static int amlogic_spifc_a1_exec_op(struct spi_mem *mem,
>  				    const struct spi_mem_op *op)
>  {
> @@ -243,6 +259,10 @@ static int amlogic_spifc_a1_exec_op(struct spi_mem *mem,
>  	size_t data_size = op->data.nbytes;
>  	int ret;
>  
> +	ret = amlogic_spifc_a1_set_freq(spifc, mem->spi->max_speed_hz);
> +	if (ret)
> +		return ret;
> +
>  	amlogic_spifc_a1_user_init(spifc);
>  	amlogic_spifc_a1_set_cmd(spifc, SPIFC_A1_USER_CMD(op));

Re: [PATCH v2 2/2] spi: amlogic-spifc-a1: add support for max_speed_hz

[Martin Kurbanov]

On 11.07.2023 10:25, Jerome Brunet wrote:
>>  
>> +static int amlogic_spifc_a1_set_freq(struct amlogic_spifc_a1 *spifc, u32 freq)
>> +{
>> +	int ret;
>> +
>> +	if (freq == spifc->curr_speed_hz)
>> +		return 0;
>> +
>> +	ret = clk_set_rate(spifc->clk, freq);
>> +	if (ret)
>> +		return ret;
>> +
>> +	spifc->curr_speed_hz = freq;
> 
> There is no guarantee that clk_set_rate() has set the rate you have
> requested, at least not precisely. You should call clk_get_rate() here.
> 

Hello Jerome, thank you for the feedback.
Are you referring to a situation where there is a change in the rate due
to a request from another client, such as a sibling driver with the same
parent clock?

-- 
Best Regards,
Martin Kurbanov

Re: [PATCH v2 2/2] spi: amlogic-spifc-a1: add support for max_speed_hz

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

On Thu, Jul 20, 2023 at 06:41:11PM +0300, Martin Kurbanov wrote:
> On 11.07.2023 10:25, Jerome Brunet wrote:

> >> +	ret = clk_set_rate(spifc->clk, freq);
> >> +	if (ret)
> >> +		return ret;

> >> +	spifc->curr_speed_hz = freq;

> > There is no guarantee that clk_set_rate() has set the rate you have
> > requested, at least not precisely. You should call clk_get_rate() here.

> Are you referring to a situation where there is a change in the rate due
> to a request from another client, such as a sibling driver with the same
> parent clock?

The clock may simply not be able to generate exactly the rate you
requested, the rate will be rounded to some value that the clock can
actually generate.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] spi: amlogic-spifc-a1: add support for max_speed_hz

[Martin Kurbanov]

On 20.07.2023 18:46, Mark Brown wrote:
> On Thu, Jul 20, 2023 at 06:41:11PM +0300, Martin Kurbanov wrote:
>> On 11.07.2023 10:25, Jerome Brunet wrote:
> 
>>>> +	ret = clk_set_rate(spifc->clk, freq);
>>>> +	if (ret)
>>>> +		return ret;
> 
>>>> +	spifc->curr_speed_hz = freq;
> 
>>> There is no guarantee that clk_set_rate() has set the rate you have
>>> requested, at least not precisely. You should call clk_get_rate() here.
> 
>> Are you referring to a situation where there is a change in the rate due
>> to a request from another client, such as a sibling driver with the same
>> parent clock?
> 
> The clock may simply not be able to generate exactly the rate you
> requested, the rate will be rounded to some value that the clock can
> actually generate.

Yes, I understand the situation. However, I am comparing it with the
requested frequency rather than the actual one that has been set.
Therefore, I asked Jerome for clarification regarding whether the
frequency can be changed by another client (driver). Maybe, it's better
to remove this condition at all? CCF has a cached rate value and doesn't
run 'set' operation if it's not needed.

-- 
Best Regards,
Martin Kurbanov

Re: [PATCH v2 0/2] spi: amlogic-spifc-a1: fixes and improvements for amlogic-spifc-a1

[Mark Brown]

On Thu, 06 Jul 2023 14:03:29 +0300, Martin Kurbanov wrote:
> This series adds support for max_speed_hz and implement adjust_op_size()
> callback.
> 
> Changes v2 since v1 at [1]:
>   - Make cosmetic changes as Andy suggested at [1]
> 
> Links:
>   [1] https://lore.kernel.org/all/20230703094518.53755-1-mmkurbanov@sberdevices.ru/
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/2] spi: amlogic-spifc-a1: implement adjust_op_size()
      commit: 9bee51722cdc1b32193d4ddf6ea6952d666d8f13
[2/2] spi: amlogic-spifc-a1: add support for max_speed_hz
      commit: 54b8422cc64d6236024ec7d72bc63ca3ca90b87f

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark