Hi,
this patchset adds a driver for the SPIFC (SPI flash controller)
available in Amlogic Meson6 and Meson8 SoCs. It doesn't support DMA
and has a 64-byte unified transmit/receive buffer.
The driver has been tested on a Meson8 based device to communicate
with a Macronix mx25l1606e serial flash.
Beniamino Galvani (3):
spi: meson: Add device tree bindings documentation for SPIFC
spi: meson: Add support for Amlogic Meson SPIFC
ARM: dts: meson: add node for SPIFC
.../devicetree/bindings/spi/spi-meson.txt | 22 ++
arch/arm/boot/dts/meson.dtsi | 9 +
drivers/spi/Kconfig | 7 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-meson-spifc.c | 411 +++++++++++++++++++++
5 files changed, 450 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/spi-meson.txt
create mode 100644 drivers/spi/spi-meson-spifc.c
--
1.9.1
This is a driver for the Amlogic Meson SPIFC (SPI flash controller),
which is one of the two SPI controllers available on the SoC. It
doesn't support DMA and has a 64-byte unified transmit/receive buffer.
The device is optimized for interfacing with SPI NOR memories and
allows the execution of standard operations such as read, page
program, sector erase, etc. in a simplified way, basically toggling a
bit in a dedicated register. The driver doesn't use those predefined
commands and relies only on custom transfers.
Signed-off-by: Beniamino Galvani <[email protected]>
---
drivers/spi/Kconfig | 7 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-meson-spifc.c | 411 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 419 insertions(+)
create mode 100644 drivers/spi/spi-meson-spifc.c
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 84e7c9e..3c98a9d 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -301,6 +301,13 @@ config SPI_FSL_ESPI
From MPC8536, 85xx platform uses the controller, and all P10xx,
P20xx, P30xx,P40xx, P50xx uses this controller.
+config SPI_MESON_SPIFC
+ bool "Amlogic Meson SPIFC controller"
+ depends on ARCH_MESON
+ help
+ This enables master mode support for the SPIFC (SPI flash
+ controller) available in Amlogic Meson SoCs.
+
config SPI_OC_TINY
tristate "OpenCores tiny SPI"
depends on GPIOLIB
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 78f24ca..9b8a747 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -42,6 +42,7 @@ obj-$(CONFIG_SPI_FSL_SPI) += spi-fsl-spi.o
obj-$(CONFIG_SPI_GPIO) += spi-gpio.o
obj-$(CONFIG_SPI_IMX) += spi-imx.o
obj-$(CONFIG_SPI_LM70_LLP) += spi-lm70llp.o
+obj-$(CONFIG_SPI_MESON_SPIFC) += spi-meson-spifc.o
obj-$(CONFIG_SPI_MPC512x_PSC) += spi-mpc512x-psc.o
obj-$(CONFIG_SPI_MPC52xx_PSC) += spi-mpc52xx-psc.o
obj-$(CONFIG_SPI_MPC52xx) += spi-mpc52xx.o
diff --git a/drivers/spi/spi-meson-spifc.c b/drivers/spi/spi-meson-spifc.c
new file mode 100644
index 0000000..a0ba5d8
--- /dev/null
+++ b/drivers/spi/spi-meson-spifc.c
@@ -0,0 +1,411 @@
+/*
+ * Driver for Amlogic Meson SPI flash controller (SPIFC)
+ *
+ * Copyright (C) 2014 Beniamino Galvani <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+#include <linux/types.h>
+
+/* register map */
+#define REG_CMD 0x00
+#define REG_ADDR 0x04
+#define REG_CTRL 0x08
+#define REG_CTRL1 0x0c
+#define REG_STATUS 0x10
+#define REG_CTRL2 0x14
+#define REG_CLOCK 0x18
+#define REG_USER 0x1c
+#define REG_USER1 0x20
+#define REG_USER2 0x24
+#define REG_USER3 0x28
+#define REG_USER4 0x2c
+#define REG_SLAVE 0x30
+#define REG_SLAVE1 0x34
+#define REG_SLAVE2 0x38
+#define REG_SLAVE3 0x3c
+#define REG_C0 0x40
+#define REG_B8 0x60
+
+/* register fields */
+#define CMD_USER BIT(18)
+#define CTRL_ENABLE_AHB BIT(17)
+#define CLOCK_SOURCE BIT(31)
+#define CLOCK_DIV_SHIFT 12
+#define CLOCK_DIV_MASK (0x3f << CLOCK_DIV_SHIFT)
+#define CLOCK_CNT_HIGH_SHIFT 6
+#define CLOCK_CNT_HIGH_MASK (0x3f << CLOCK_CNT_HIGH_SHIFT)
+#define CLOCK_CNT_LOW_SHIFT 0
+#define CLOCK_CNT_LOW_MASK (0x3f << CLOCK_CNT_LOW_SHIFT)
+#define USER_DIN_EN_MS BIT(0)
+#define USER_CMP_MODE BIT(2)
+#define USER_UC_DOUT_SEL BIT(27)
+#define USER_UC_DIN_SEL BIT(28)
+#define USER_UC_MASK ((BIT(5) - 1) << 27)
+#define USER1_BN_UC_DOUT_SHIFT 17
+#define USER1_BN_UC_DOUT_MASK (0xff << 16)
+#define USER1_BN_UC_DIN_SHIFT 8
+#define USER1_BN_UC_DIN_MASK (0xff << 8)
+#define USER4_CS_ACT BIT(30)
+#define SLAVE_TRST_DONE BIT(4)
+#define SLAVE_OP_MODE BIT(30)
+#define SLAVE_SW_RST BIT(31)
+
+#define SPIFC_BUFFER_SIZE 64
+
+/**
+ * struct meson_spifc
+ * @master: the SPI master
+ * @regmap: regmap for device registers
+ * @clk: input clock of the built-in baud rate generator
+ * @device: the device structure
+ */
+struct meson_spifc {
+ struct spi_master *master;
+ struct regmap *regmap;
+ struct clk *clk;
+ struct device *dev;
+};
+
+static struct regmap_config spifc_regmap_config = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+};
+
+/**
+ * meson_spifc_wait_ready() - wait for the current operation to terminate
+ * @spifc: the Meson SPI device
+ * Return: 0 on success, a negative value on error
+ */
+static int meson_spifc_wait_ready(struct meson_spifc *spifc)
+{
+ unsigned long deadline = jiffies + msecs_to_jiffies(1000);
+ u32 data;
+
+ do {
+ regmap_read(spifc->regmap, REG_SLAVE, &data);
+ if (data & SLAVE_TRST_DONE)
+ return 0;
+ cond_resched();
+ } while (time_before(jiffies, deadline));
+
+ return -ETIMEDOUT;
+}
+
+/**
+ * meson_spifc_drain_buffer() - copy data from device buffer to memory
+ * @spifc: the Meson SPI device
+ * @buf: the destination buffer
+ * @len: number of bytes to copy
+ */
+static void meson_spifc_drain_buffer(struct meson_spifc *spifc, u8 *buf,
+ int len)
+{
+ u32 data;
+ int i = 0;
+
+ while (i < len) {
+ regmap_read(spifc->regmap, REG_C0 + i, &data);
+
+ if (len - i >= 4) {
+ *((u32 *)buf) = data;
+ buf += 4;
+ } else {
+ memcpy(buf, &data, len - i);
+ break;
+ }
+ i += 4;
+ }
+}
+
+/**
+ * meson_spifc_fill_buffer() - copy data from memory to device buffer
+ * @spifc: the Meson SPI device
+ * @buf: the source buffer
+ * @len: number of bytes to copy
+ */
+static void meson_spifc_fill_buffer(struct meson_spifc *spifc, const u8 *buf,
+ int len)
+{
+ u32 data;
+ int i = 0;
+
+ while (i < len) {
+ if (len - i >= 4)
+ data = *(u32 *)buf;
+ else
+ memcpy(&data, buf, len - i);
+
+ regmap_write(spifc->regmap, REG_C0 + i, data);
+
+ buf += 4;
+ i += 4;
+ }
+}
+
+/**
+ * meson_spifc_setup_speed() - program the clock divider
+ * @spifc: the Meson SPI device
+ * @speed: desired speed in Hz
+ */
+void meson_spifc_setup_speed(struct meson_spifc *spifc, u32 speed)
+{
+ unsigned long parent, value;
+ int n;
+
+ parent = clk_get_rate(spifc->clk);
+ n = max_t(int, parent / speed - 1, 1);
+
+ dev_dbg(spifc->dev, "parent %lu, speed %u, n %d\n", parent,
+ speed, n);
+
+ value = (n << CLOCK_DIV_SHIFT) & CLOCK_DIV_MASK;
+ value |= (n << CLOCK_CNT_LOW_SHIFT) & CLOCK_CNT_LOW_MASK;
+ value |= (((n + 1) / 2 - 1) << CLOCK_CNT_HIGH_SHIFT) &
+ CLOCK_CNT_HIGH_MASK;
+
+ regmap_write(spifc->regmap, REG_CLOCK, value);
+}
+
+/**
+ * meson_spifc_txrx() - transfer a chunk of data
+ * @spifc: the Meson SPI device
+ * @xfer: the current SPI transfer
+ * @offset: offset of the data to transfer
+ * @len: length of the data to transfer
+ * @last_xfer: whether this is the last transfer of the message
+ * @last_chunk: whether this is the last chunk of the transfer
+ * Return: 0 on success, a negative value on error
+ */
+static int meson_spifc_txrx(struct meson_spifc *spifc,
+ struct spi_transfer *xfer,
+ int offset, int len, bool last_xfer,
+ bool last_chunk)
+{
+ bool keep_cs = true;
+ int ret;
+
+ if (xfer->tx_buf)
+ meson_spifc_fill_buffer(spifc, xfer->tx_buf + offset, len);
+
+ /* enable DOUT stage */
+ regmap_update_bits(spifc->regmap, REG_USER, USER_UC_MASK,
+ USER_UC_DOUT_SEL);
+ regmap_write(spifc->regmap, REG_USER1,
+ (8 * len - 1) << USER1_BN_UC_DOUT_SHIFT);
+
+ /* enable data input during DOUT */
+ regmap_update_bits(spifc->regmap, REG_USER, USER_DIN_EN_MS,
+ USER_DIN_EN_MS);
+
+ if (last_chunk) {
+ if (last_xfer)
+ keep_cs = xfer->cs_change;
+ else
+ keep_cs = !xfer->cs_change;
+ }
+
+ regmap_update_bits(spifc->regmap, REG_USER4, USER4_CS_ACT,
+ keep_cs ? USER4_CS_ACT : 0);
+
+ /* clear transition done bit */
+ regmap_update_bits(spifc->regmap, REG_SLAVE, SLAVE_TRST_DONE, 0);
+ /* start transfer */
+ regmap_update_bits(spifc->regmap, REG_CMD, CMD_USER, CMD_USER);
+
+ ret = meson_spifc_wait_ready(spifc);
+
+ if (!ret && xfer->rx_buf)
+ meson_spifc_drain_buffer(spifc, xfer->rx_buf + offset, len);
+
+ return ret;
+}
+
+/**
+ * meson_spifc_transfer_one() - perform a single transfer
+ * @spifc: the Meson SPI device
+ * @xfer: the current SPI transfer
+ * @last_xfer: whether this is the last transfer of the message
+ * Return: 0 on success, a negative value on error
+ */
+static int meson_spifc_transfer_one(struct spi_device *spi,
+ struct spi_transfer *xfer,
+ bool last_xfer)
+{
+ struct meson_spifc *spifc = spi_master_get_devdata(spi->master);
+ int len, done = 0, ret = 0;
+
+ meson_spifc_setup_speed(spifc, xfer->speed_hz ? xfer->speed_hz :
+ spi->max_speed_hz);
+
+ regmap_update_bits(spifc->regmap, REG_CTRL, CTRL_ENABLE_AHB, 0);
+
+ while (done < xfer->len && !ret) {
+ len = min_t(int, xfer->len - done, SPIFC_BUFFER_SIZE);
+ ret = meson_spifc_txrx(spifc, xfer, done, len,
+ last_xfer, done + len >= xfer->len);
+ done += len;
+ }
+
+ regmap_update_bits(spifc->regmap, REG_CTRL, CTRL_ENABLE_AHB,
+ CTRL_ENABLE_AHB);
+
+ if (!ret && xfer->delay_usecs)
+ udelay(xfer->delay_usecs);
+
+ return ret;
+}
+
+/**
+ * meson_spifc_transfer_one_message() - transfer a single SPI message
+ * @master: the SPI master
+ * @msg: the message to transfer
+ * Return: 0 on success, a negative value on error
+ */
+static int meson_spifc_transfer_one_message(struct spi_master *master,
+ struct spi_message *msg)
+{
+ struct spi_transfer *xfer;
+ int ret = 0;
+ bool last;
+
+ list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+ last = list_is_last(&xfer->transfer_list, &msg->transfers);
+ ret = meson_spifc_transfer_one(msg->spi, xfer, last);
+ if (ret)
+ break;
+ msg->actual_length += xfer->len;
+ }
+
+ msg->status = ret;
+ spi_finalize_current_message(master);
+
+ return ret;
+}
+
+static int meson_spifc_probe(struct platform_device *pdev)
+{
+ struct spi_master *master;
+ struct meson_spifc *spifc;
+ struct resource *res;
+ void __iomem *base;
+ unsigned int rate;
+ int ret = 0;
+
+ master = spi_alloc_master(&pdev->dev, sizeof(struct meson_spifc));
+ if (!master)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, master);
+
+ spifc = spi_master_get_devdata(master);
+ memset(spifc, 0, sizeof(struct meson_spifc));
+ spifc->dev = &pdev->dev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ base = devm_ioremap_resource(spifc->dev, res);
+ if (IS_ERR(base)) {
+ ret = PTR_ERR(base);
+ goto out_err;
+ }
+
+ spifc_regmap_config.max_register = resource_size(res) - 4;
+ spifc_regmap_config.name = "amlogic,meson-spifc";
+ spifc->regmap = devm_regmap_init_mmio(spifc->dev, base,
+ &spifc_regmap_config);
+ if (IS_ERR(spifc->regmap)) {
+ ret = PTR_ERR(spifc->regmap);
+ goto out_err;
+ }
+
+ spifc->clk = devm_clk_get(spifc->dev, NULL);
+ if (IS_ERR(spifc->clk)) {
+ dev_err(spifc->dev, "missing clock\n");
+ ret = PTR_ERR(spifc->clk);
+ goto out_err;
+ }
+
+ ret = clk_prepare_enable(spifc->clk);
+ if (ret) {
+ dev_err(spifc->dev, "can't prepare clock\n");
+ goto out_err;
+ }
+
+ rate = clk_get_rate(spifc->clk);
+
+ master->bus_num = pdev->id;
+ master->num_chipselect = 1;
+ master->dev.of_node = pdev->dev.of_node;
+ master->bits_per_word_mask = SPI_BPW_MASK(8);
+ master->transfer_one_message = meson_spifc_transfer_one_message;
+ master->min_speed_hz = rate >> 6;
+ master->max_speed_hz = rate >> 1;
+
+ /* reset device */
+ regmap_update_bits(spifc->regmap, REG_SLAVE, SLAVE_SW_RST,
+ SLAVE_SW_RST);
+ /* disable compatible mode */
+ regmap_update_bits(spifc->regmap, REG_USER, USER_CMP_MODE, 0);
+ /* set master mode */
+ regmap_update_bits(spifc->regmap, REG_SLAVE, SLAVE_OP_MODE, 0);
+
+ ret = devm_spi_register_master(&pdev->dev, master);
+ if (ret) {
+ dev_err(spifc->dev, "failed to register spi master\n");
+ goto out_clk;
+ }
+
+ return 0;
+out_clk:
+ clk_disable_unprepare(spifc->clk);
+out_err:
+ spi_master_put(master);
+ return ret;
+}
+
+static int meson_spifc_remove(struct platform_device *pdev)
+{
+ struct spi_master *master = platform_get_drvdata(pdev);
+ struct meson_spifc *spifc = spi_master_get_devdata(master);
+
+ clk_disable_unprepare(spifc->clk);
+
+ return 0;
+}
+
+static const struct of_device_id meson_spifc_dt_match[] = {
+ { .compatible = "amlogic,meson6-spifc", },
+ { },
+};
+
+static struct platform_driver meson_spifc_driver = {
+ .probe = meson_spifc_probe,
+ .remove = meson_spifc_remove,
+ .driver = {
+ .name = "meson-spifc",
+ .of_match_table = of_match_ptr(meson_spifc_dt_match),
+ },
+};
+
+module_platform_driver(meson_spifc_driver);
+
+MODULE_AUTHOR("Beniamino Galvani <[email protected]>");
+MODULE_DESCRIPTION("Amlogic Meson SPIFC driver");
+MODULE_LICENSE("GPL v2");
--
1.9.1
This adds a node for the SPI Flash Controller to the Amlogic Meson
DTS.
Signed-off-by: Beniamino Galvani <[email protected]>
---
arch/arm/boot/dts/meson.dtsi | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/arm/boot/dts/meson.dtsi b/arch/arm/boot/dts/meson.dtsi
index e6539ea..d28b16e 100644
--- a/arch/arm/boot/dts/meson.dtsi
+++ b/arch/arm/boot/dts/meson.dtsi
@@ -106,5 +106,14 @@
clocks = <&clk81>;
status = "disabled";
};
+
+ spifc: spi@c1108c80 {
+ compatible = "amlogic,meson6-spifc";
+ reg = <0xc1108c80 0x80>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clocks = <&clk81>;
+ status = "disabled";
+ };
};
}; /* end of / */
--
1.9.1
This adds documentation of device tree bindings for the Amlogic Meson
SPIFC (SPI Flash Controller).
Signed-off-by: Beniamino Galvani <[email protected]>
---
.../devicetree/bindings/spi/spi-meson.txt | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/spi-meson.txt
diff --git a/Documentation/devicetree/bindings/spi/spi-meson.txt b/Documentation/devicetree/bindings/spi/spi-meson.txt
new file mode 100644
index 0000000..bb52a86
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-meson.txt
@@ -0,0 +1,22 @@
+Amlogic Meson SPI controllers
+
+* SPIFC (SPI Flash Controller)
+
+The Meson SPIFC is a controller optimized for communication with SPI
+NOR memories, without DMA support and a 64-byte unified transmit /
+receive buffer.
+
+Required properties:
+ - compatible: should be "amlogic,meson6-spifc"
+ - reg: physical base address and length of the controller registers
+ - clocks: phandle of the input clock for the baud rate generator
+ - #address-cells: should be 1
+ - #size-cells: should be 0
+
+ spi@c1108c80 {
+ compatible = "amlogic,meson6-spifc";
+ reg = <0xc1108c80 0x80>;
+ clocks = <&clk81>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
--
1.9.1
On Sun, Nov 09, 2014 at 10:25:13AM +0100, Beniamino Galvani wrote:
> This adds a node for the SPI Flash Controller to the Amlogic Meson
> DTS.
Acked-by: Mark Brown <[email protected]>
On Sun, Nov 09, 2014 at 10:25:12AM +0100, Beniamino Galvani wrote:
> +static int meson_spifc_wait_ready(struct meson_spifc *spifc)
> +{
> + unsigned long deadline = jiffies + msecs_to_jiffies(1000);
> + u32 data;
> +
> + do {
> + regmap_read(spifc->regmap, REG_SLAVE, &data);
> + if (data & SLAVE_TRST_DONE)
> + return 0;
> + cond_resched();
> + } while (time_before(jiffies, deadline));
This will busy wait for up to a second, that seems like a long time to
busy wait. We also appear to be using this for the entire duration of
the transfer which could be a fairly long time even during normal
operation if doing a large transfer such as a firmware download, or if
the bus speed is low.
> + meson_spifc_setup_speed(spifc, xfer->speed_hz ? xfer->speed_hz :
> + spi->max_speed_hz);
> +
Please avoid the ternery operator, it does nothing for legibility and in
this case it's not needed as the core will always ensure that there is a
per-transfer speed set.
> + while (done < xfer->len && !ret) {
> + len = min_t(int, xfer->len - done, SPIFC_BUFFER_SIZE);
> + ret = meson_spifc_txrx(spifc, xfer, done, len,
> + last_xfer, done + len >= xfer->len);
> + done += len;
> + }
I noticed that the handling of /CS was done in the spifc_txrx() function
- will this do the right thing if the transfer needs to be split for the
buffer size?
> + if (!ret && xfer->delay_usecs)
> + udelay(xfer->delay_usecs);
The core will do this for you if you implement this as transfer_one().
> +static int meson_spifc_transfer_one_message(struct spi_master *master,
> + struct spi_message *msg)
This appears to do nothing that the core won't do - just implement
transfer_one() and remove this.
> + spifc = spi_master_get_devdata(master);
> + memset(spifc, 0, sizeof(struct meson_spifc));
There should be no need for this memset.
> + spifc_regmap_config.max_register = resource_size(res) - 4;
> + spifc_regmap_config.name = "amlogic,meson-spifc";
If you're dynamically initializing the structure you need to work with a
copy of it rather than directly since there may be multiple instances.
I'm not seeing a reason to override the regmap name here, this is only
really intended for devices with multiple register maps.
> + ret = clk_prepare_enable(spifc->clk);
> + if (ret) {
> + dev_err(spifc->dev, "can't prepare clock\n");
> + goto out_err;
> + }
You should really implement runtime PM operations to disable this when
not in use and use auto_runtime_pm to make sure this happens.
> + master->bus_num = pdev->id;
Leave this blank for DT only devices (and for non-DT devices this won't
work if you get two different buses).
On Sun, Nov 09, 2014 at 10:17:12AM +0000, Mark Brown wrote:
> On Sun, Nov 09, 2014 at 10:25:12AM +0100, Beniamino Galvani wrote:
>
> > +static int meson_spifc_wait_ready(struct meson_spifc *spifc)
> > +{
> > + unsigned long deadline = jiffies + msecs_to_jiffies(1000);
> > + u32 data;
> > +
> > + do {
> > + regmap_read(spifc->regmap, REG_SLAVE, &data);
> > + if (data & SLAVE_TRST_DONE)
> > + return 0;
> > + cond_resched();
> > + } while (time_before(jiffies, deadline));
>
> This will busy wait for up to a second, that seems like a long time to
> busy wait. We also appear to be using this for the entire duration of
> the transfer which could be a fairly long time even during normal
> operation if doing a large transfer such as a firmware download, or if
> the bus speed is low.
Yes, probably the timeout value is too long since the maximum length
of a basic transfer is 64 bytes. Can you suggest a reasonable value?
>
> > + meson_spifc_setup_speed(spifc, xfer->speed_hz ? xfer->speed_hz :
> > + spi->max_speed_hz);
> > +
>
> Please avoid the ternery operator, it does nothing for legibility and in
> this case it's not needed as the core will always ensure that there is a
> per-transfer speed set.
Ok.
> > + while (done < xfer->len && !ret) {
> > + len = min_t(int, xfer->len - done, SPIFC_BUFFER_SIZE);
> > + ret = meson_spifc_txrx(spifc, xfer, done, len,
> > + last_xfer, done + len >= xfer->len);
> > + done += len;
> > + }
>
> I noticed that the handling of /CS was done in the spifc_txrx() function
> - will this do the right thing if the transfer needs to be split for the
> buffer size?
It should. When the transfer gets split, CS is kept active for all the
chunks and the value of CS after that depends on the value of
cs_change.
>
> > + if (!ret && xfer->delay_usecs)
> > + udelay(xfer->delay_usecs);
>
> The core will do this for you if you implement this as transfer_one().
Please correct me if I'm wrong, but I think that transfer_one() can't
be used in this case. The hardware doesn't support direct manipulation
of CS and allows only to specify if CS must be kept active after the
current transfer. So I need to know for each transfer if it's the last
and this can be achieved only implementing transfer_one_message().
>
> > +static int meson_spifc_transfer_one_message(struct spi_master *master,
> > + struct spi_message *msg)
>
> This appears to do nothing that the core won't do - just implement
> transfer_one() and remove this.
>
> > + spifc = spi_master_get_devdata(master);
> > + memset(spifc, 0, sizeof(struct meson_spifc));
>
> There should be no need for this memset.
>
> > + spifc_regmap_config.max_register = resource_size(res) - 4;
> > + spifc_regmap_config.name = "amlogic,meson-spifc";
>
> If you're dynamically initializing the structure you need to work with a
> copy of it rather than directly since there may be multiple instances.
> I'm not seeing a reason to override the regmap name here, this is only
> really intended for devices with multiple register maps.
>
> > + ret = clk_prepare_enable(spifc->clk);
> > + if (ret) {
> > + dev_err(spifc->dev, "can't prepare clock\n");
> > + goto out_err;
> > + }
>
> You should really implement runtime PM operations to disable this when
> not in use and use auto_runtime_pm to make sure this happens.
>
> > + master->bus_num = pdev->id;
>
> Leave this blank for DT only devices (and for non-DT devices this won't
> work if you get two different buses).
Ok, will do. Thanks for the review.
Beniamino
On Sun, Nov 09, 2014 at 11:56:50PM +0100, Beniamino Galvani wrote:
> On Sun, Nov 09, 2014 at 10:17:12AM +0000, Mark Brown wrote:
> > This will busy wait for up to a second, that seems like a long time to
> > busy wait. We also appear to be using this for the entire duration of
> > the transfer which could be a fairly long time even during normal
> > operation if doing a large transfer such as a firmware download, or if
> > the bus speed is low.
> Yes, probably the timeout value is too long since the maximum length
> of a basic transfer is 64 bytes. Can you suggest a reasonable value?
10ms? It depends somewhat
> > > + while (done < xfer->len && !ret) {
> > > + len = min_t(int, xfer->len - done, SPIFC_BUFFER_SIZE);
> > > + ret = meson_spifc_txrx(spifc, xfer, done, len,
> > > + last_xfer, done + len >= xfer->len);
> > > + done += len;
> > > + }
> > I noticed that the handling of /CS was done in the spifc_txrx() function
> > - will this do the right thing if the transfer needs to be split for the
> > buffer size?
> It should. When the transfer gets split, CS is kept active for all the
> chunks and the value of CS after that depends on the value of
> cs_change.
Can you be more specific about how that works? I'm just not seeing the
code that handles this.
> > > + if (!ret && xfer->delay_usecs)
> > > + udelay(xfer->delay_usecs);
> > The core will do this for you if you implement this as transfer_one().
> Please correct me if I'm wrong, but I think that transfer_one() can't
> be used in this case. The hardware doesn't support direct manipulation
> of CS and allows only to specify if CS must be kept active after the
> current transfer. So I need to know for each transfer if it's the last
> and this can be achieved only implementing transfer_one_message().
This is already in a function that's operating at the transfer_one()
level, the function is even called transfer_one() and besides it's
clearly not something specific to this hardware so should be factored
out into the core instead of open coded.
On Mon, Nov 10, 2014 at 03:11:40PM +0000, Mark Brown wrote:
> On Sun, Nov 09, 2014 at 11:56:50PM +0100, Beniamino Galvani wrote:
> > On Sun, Nov 09, 2014 at 10:17:12AM +0000, Mark Brown wrote:
>
> > > I noticed that the handling of /CS was done in the spifc_txrx() function
> > > - will this do the right thing if the transfer needs to be split for the
> > > buffer size?
>
> > It should. When the transfer gets split, CS is kept active for all the
> > chunks and the value of CS after that depends on the value of
> > cs_change.
>
> Can you be more specific about how that works? I'm just not seeing the
> code that handles this.
It's this:
static int meson_spifc_txrx(struct meson_spifc *spifc,
struct spi_transfer *xfer,
int offset, int len, bool last_xfer,
bool last_chunk)
{
bool keep_cs = true;
[...]
if (last_chunk) {
if (last_xfer)
keep_cs = xfer->cs_change;
else
keep_cs = !xfer->cs_change;
}
regmap_update_bits(spifc->regmap, REG_USER4, USER4_CS_ACT,
keep_cs ? USER4_CS_ACT : 0);
/* start transfer */
[...]
}
The USER4_CS_ACT bit specifies if CS must be kept active after the
transfer.
> > > > + if (!ret && xfer->delay_usecs)
> > > > + udelay(xfer->delay_usecs);
>
> > > The core will do this for you if you implement this as transfer_one().
>
> > Please correct me if I'm wrong, but I think that transfer_one() can't
> > be used in this case. The hardware doesn't support direct manipulation
> > of CS and allows only to specify if CS must be kept active after the
> > current transfer. So I need to know for each transfer if it's the last
> > and this can be achieved only implementing transfer_one_message().
>
> This is already in a function that's operating at the transfer_one()
> level, the function is even called transfer_one() and besides it's
> clearly not something specific to this hardware so should be factored
> out into the core instead of open coded.
A way to simplify this at core level could be to add a 'last' flag to
the spi_transfer structure and populate it before calling
transfer_one_message(); in this way, drivers that need to know the
position of the transfer in the message in order to properly handle CS
can use the generic version of transfer_one_message() instead of
reimplementing it. It seems that other existing drivers probably can
benefit from this.
Beniamino