| Message ID | 20230706110331.19794-3-mmkurbanov@sberdevices.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: amlogic-spifc-a1: fixes and improvements for amlogic-spifc-a1 | expand |
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>
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));
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?
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.
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 --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));
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(+)