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/[email protected]/
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
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 <[email protected]>
---
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
This patch sets the clock rate (spi_transfer->max_speed_hz) from the
amlogic_spifc_a1_exec_op().
Signed-off-by: Martin Kurbanov <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> ---
> 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 <[email protected]>
On Thu 06 Jul 2023 at 14:03, Martin Kurbanov <[email protected]> wrote:
> This patch sets the clock rate (spi_transfer->max_speed_hz) from the
> amlogic_spifc_a1_exec_op().
>
> Signed-off-by: Martin Kurbanov <[email protected]>
> ---
> 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 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/[email protected]/
>
> [...]
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
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
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.
--
Best Regards,
Martin Kurbanov