diff mbox series

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

Message ID 20230706110331.19794-3-mmkurbanov@sberdevices.ru
State New
Headers
Series spi: amlogic-spifc-a1: fixes and improvements for amlogic-spifc-a1 |

Commit Message

Martin Kurbanov July 6, 2023, 11:03 a.m. UTC 
  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(+)

Comments

Neil Armstrong July 7, 2023, 7:51 a.m. UTC | #1 
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>
Jerome Brunet July 11, 2023, 7:25 a.m. UTC | #2 
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));
Martin Kurbanov July 20, 2023, 3:41 p.m. UTC | #3 
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?
Mark Brown July 20, 2023, 3:46 p.m. UTC | #4 
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.
Martin Kurbanov July 24, 2023, 3:15 p.m. UTC | #5 
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.
diff mbox series

Patch

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));