* [PATCH v2 0/3] Add support for the Airoha EN7523 SPI controller
@ 2022-09-27 11:32 Bert Vermeulen
2022-09-27 11:32 ` [PATCH v2 1/3] dt-bindings: arm: airoha: Add documentation for Airoha " Bert Vermeulen
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Bert Vermeulen @ 2022-09-27 11:32 UTC (permalink / raw)
To: Mark Brown, Rob Herring, Krzysztof Kozlowski, linux-spi,
devicetree, linux-kernel
Cc: Bert Vermeulen, John Crispin, Benjamin Larsson
This driver will likely also work on other Airoha SoCs, but this will
need testing. Only basic single- and dual-bit transfers are supported
for now, with DMA mode support yet to come.
v2:
- Drop clock name
- Make driver callbacks static, as reported by the bot
- Syntax fixes in DT binding docs
Bert Vermeulen (3):
dt-bindings: arm: airoha: Add documentation for Airoha SPI controller
spi: Add support for the Airoha EN7523 SoC SPI controller
ARM: dts: en7523: Add SPI node
.../bindings/spi/airoha,en7523-spi.yaml | 45 +++
arch/arm/boot/dts/en7523-evb.dts | 20 ++
arch/arm/boot/dts/en7523.dtsi | 10 +
drivers/spi/Kconfig | 7 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-en7523.c | 305 ++++++++++++++++++
6 files changed, 388 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/airoha,en7523-spi.yaml
create mode 100644 drivers/spi/spi-en7523.c
--
2.25.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/3] dt-bindings: arm: airoha: Add documentation for Airoha SPI controller
2022-09-27 11:32 [PATCH v2 0/3] Add support for the Airoha EN7523 SPI controller Bert Vermeulen
@ 2022-09-27 11:32 ` Bert Vermeulen
2022-09-29 22:21 ` Rob Herring
2022-09-30 10:29 ` Krzysztof Kozlowski
2022-09-27 11:32 ` [PATCH v2 2/3] spi: Add support for the Airoha EN7523 SoC " Bert Vermeulen
2022-09-27 11:32 ` [PATCH v2 3/3] ARM: dts: en7523: Add SPI node Bert Vermeulen
2 siblings, 2 replies; 8+ messages in thread
From: Bert Vermeulen @ 2022-09-27 11:32 UTC (permalink / raw)
To: Mark Brown, Rob Herring, Krzysztof Kozlowski, linux-spi,
devicetree, linux-kernel
Cc: Bert Vermeulen, John Crispin, Benjamin Larsson
Create documentation for accessing the Airoha EN7523 SPI controller.
Signed-off-by: Bert Vermeulen <bert@biot.com>
---
.../bindings/spi/airoha,en7523-spi.yaml | 45 +++++++++++++++++++
1 file changed, 45 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/airoha,en7523-spi.yaml
diff --git a/Documentation/devicetree/bindings/spi/airoha,en7523-spi.yaml b/Documentation/devicetree/bindings/spi/airoha,en7523-spi.yaml
new file mode 100644
index 000000000000..8f4936512a99
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/airoha,en7523-spi.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/airoha,en7523-spi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Airoha EN7523 SPI controller
+
+maintainers:
+ - Bert Vermeulen <bert@biot.com>
+
+description: |
+ This binding describes the SPI controller on Airoha EN7523 SoCs.
+
+allOf:
+ - $ref: spi-controller.yaml#
+
+properties:
+ compatible:
+ items:
+ - const: airoha,en7523-spi
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - clocks
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/en7523-clk.h>
+ spi0: spi@1fa10000 {
+ compatible = "airoha,en7523-spi";
+ reg = <0x1fa10000 0x10000>;
+ clocks = <&scu EN7523_CLK_SPI>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
--
2.25.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/3] spi: Add support for the Airoha EN7523 SoC SPI controller
2022-09-27 11:32 [PATCH v2 0/3] Add support for the Airoha EN7523 SPI controller Bert Vermeulen
2022-09-27 11:32 ` [PATCH v2 1/3] dt-bindings: arm: airoha: Add documentation for Airoha " Bert Vermeulen
@ 2022-09-27 11:32 ` Bert Vermeulen
2022-09-27 12:23 ` Mark Brown
2022-09-27 11:32 ` [PATCH v2 3/3] ARM: dts: en7523: Add SPI node Bert Vermeulen
2 siblings, 1 reply; 8+ messages in thread
From: Bert Vermeulen @ 2022-09-27 11:32 UTC (permalink / raw)
To: Mark Brown, Rob Herring, Krzysztof Kozlowski, linux-spi,
devicetree, linux-kernel
Cc: Bert Vermeulen, John Crispin, Benjamin Larsson
This SPI driver primarily drives the SoC-internal state machine doing
the actual transfers. This allows transfers to be transparently queued
in hardware.
DMA mode is not yet supported.
Signed-off-by: Bert Vermeulen <bert@biot.com>
---
drivers/spi/Kconfig | 7 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-en7523.c | 305 +++++++++++++++++++++++++++++++++++++++
3 files changed, 313 insertions(+)
create mode 100644 drivers/spi/spi-en7523.c
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index e32f6a2058ae..11b8c7ea0226 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -57,6 +57,13 @@ config SPI_MEM
comment "SPI Master Controller Drivers"
+config SPI_AIROHA_EN7523
+ bool "Airoha EN7523 SPI controller support"
+ depends on ARCH_AIROHA
+ default ARCH_AIROHA
+ help
+ This enables SPI controller support for the Airoha EN7523 SoC.
+
config SPI_ALTERA
tristate "Altera SPI Controller platform driver"
select SPI_ALTERA_CORE
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 15d2f3835e45..8c360f764869 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_SPI_SPIDEV) += spidev.o
obj-$(CONFIG_SPI_LOOPBACK_TEST) += spi-loopback-test.o
# SPI master controller drivers (bus)
+obj-$(CONFIG_SPI_AIROHA_EN7523) += spi-en7523.o
obj-$(CONFIG_SPI_ALTERA) += spi-altera-platform.o
obj-$(CONFIG_SPI_ALTERA_CORE) += spi-altera-core.o
obj-$(CONFIG_SPI_ALTERA_DFL) += spi-altera-dfl.o
diff --git a/drivers/spi/spi-en7523.c b/drivers/spi/spi-en7523.c
new file mode 100644
index 000000000000..e8e12cd6f597
--- /dev/null
+++ b/drivers/spi/spi-en7523.c
@@ -0,0 +1,305 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/clk.h>
+#include <linux/spi/spi.h>
+#include <dt-bindings/clock/en7523-clk.h>
+
+
+#define ENSPI_READ_IDLE_EN 0x0004
+#define ENSPI_MTX_MODE_TOG 0x0014
+#define ENSPI_RDCTL_FSM 0x0018
+#define ENSPI_MANUAL_EN 0x0020
+#define ENSPI_MANUAL_OPFIFO_EMPTY 0x0024
+#define ENSPI_MANUAL_OPFIFO_WDATA 0x0028
+#define ENSPI_MANUAL_OPFIFO_FULL 0x002C
+#define ENSPI_MANUAL_OPFIFO_WR 0x0030
+#define ENSPI_MANUAL_DFIFO_FULL 0x0034
+#define ENSPI_MANUAL_DFIFO_WDATA 0x0038
+#define ENSPI_MANUAL_DFIFO_EMPTY 0x003C
+#define ENSPI_MANUAL_DFIFO_RD 0x0040
+#define ENSPI_MANUAL_DFIFO_RDATA 0x0044
+#define ENSPI_IER 0x0090
+#define ENSPI_NFI2SPI_EN 0x0130
+
+#define OP_CSH 0x00
+#define OP_CSL 0x01
+#define OP_CK 0x02
+#define OP_OUTS 0x08
+#define OP_OUTD 0x09
+#define OP_OUTQ 0x0A
+#define OP_INS 0x0C
+#define OP_INS0 0x0D
+#define OP_IND 0x0E
+#define OP_INQ 0x0F
+#define OP_OS2IS 0x10
+#define OP_OS2ID 0x11
+#define OP_OS2IQ 0x12
+#define OP_OD2IS 0x13
+#define OP_OD2ID 0x14
+#define OP_OD2IQ 0x15
+#define OP_OQ2IS 0x16
+#define OP_OQ2ID 0x17
+#define OP_OQ2IQ 0x18
+#define OP_OSNIS 0x19
+#define OP_ODNID 0x1A
+
+#define MATRIX_MODE_AUTO 1
+#define CONF_MTX_MODE_AUTO 0
+#define MANUALEN_AUTO 0
+#define MATRIX_MODE_MANUAL 0
+#define CONF_MTX_MODE_MANUAL 9
+#define MANUALEN_MANUAL 1
+
+#define ENSPI_MAX_XFER 0x1ff
+
+#define REG(x) (iobase + x)
+
+static void __iomem *iobase;
+
+
+static void opfifo_write(u32 cmd, u32 len)
+{
+ u32 tmp = ((cmd & 0x1f) << 9) | (len & 0x1ff);
+
+ writel(tmp, REG(ENSPI_MANUAL_OPFIFO_WDATA));
+
+ /* Wait for room in OPFIFO */
+ while (readl(REG(ENSPI_MANUAL_OPFIFO_FULL)))
+ cpu_relax();
+
+ /* Shift command into OPFIFO */
+ writel(1, REG(ENSPI_MANUAL_OPFIFO_WR));
+
+ /* Wait for command to finish */
+ while (!readl(REG(ENSPI_MANUAL_OPFIFO_EMPTY)))
+ cpu_relax();
+}
+
+static void set_cs(int state)
+{
+ if (state)
+ opfifo_write(OP_CSH, 1);
+ else
+ opfifo_write(OP_CSL, 1);
+}
+
+static void manual_begin_cmd(void)
+{
+ /* Disable read idle state */
+ writel(0, REG(ENSPI_READ_IDLE_EN));
+
+ /* Wait for FSM to reach idle state */
+ while (readl(REG(ENSPI_RDCTL_FSM)))
+ cpu_relax();
+
+ /* Set SPI core to manual mode */
+ writel(CONF_MTX_MODE_MANUAL, REG(ENSPI_MTX_MODE_TOG));
+ writel(MANUALEN_MANUAL, REG(ENSPI_MANUAL_EN));
+}
+
+static void manual_end_cmd(void)
+{
+ /* Set SPI core to auto mode */
+ writel(CONF_MTX_MODE_AUTO, REG(ENSPI_MTX_MODE_TOG));
+ writel(MANUALEN_AUTO, REG(ENSPI_MANUAL_EN));
+
+ /* Enable read idle state */
+ writel(1, REG(ENSPI_READ_IDLE_EN));
+}
+
+static void dfifo_read(u8 *buf, int len)
+{
+ int i;
+
+ for (i = 0; i < len; i++) {
+ /* Wait for requested data to show up in DFIFO */
+ while (readl(REG(ENSPI_MANUAL_DFIFO_EMPTY)))
+ cpu_relax();
+ buf[i] = readl(REG(ENSPI_MANUAL_DFIFO_RDATA));
+ /* Queue up next byte */
+ writel(1, REG(ENSPI_MANUAL_DFIFO_RD));
+ }
+}
+
+static void dfifo_write(const u8 *buf, int len)
+{
+ int i;
+
+ for (i = 0; i < len; i++) {
+ /* Wait for room in DFIFO */
+ while (readl(REG(ENSPI_MANUAL_DFIFO_FULL)))
+ cpu_relax();
+ writel(buf[i], REG(ENSPI_MANUAL_DFIFO_WDATA));
+ }
+}
+
+static int init_hw(struct device *dev)
+{
+ struct clk *clk;
+ int ret;
+
+ /* Disable manual/auto mode clash interrupt */
+ writel(0, REG(ENSPI_IER));
+
+ /* Disable DMA */
+ writel(0, REG(ENSPI_NFI2SPI_EN));
+
+ clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(clk))
+ return PTR_ERR(clk);
+
+ ret = clk_prepare_enable(clk);
+ if (ret)
+ return ret;
+
+ ret = clk_set_rate(clk, 40000000);
+ if (ret) {
+ clk_disable_unprepare(clk);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int xfer_read(struct spi_transfer *xfer)
+{
+ int opcode;
+ uint8_t *buf = xfer->rx_buf;
+
+ switch (xfer->rx_nbits) {
+ case SPI_NBITS_SINGLE:
+ opcode = OP_INS;
+ break;
+ case SPI_NBITS_DUAL:
+ opcode = OP_IND;
+ break;
+ }
+
+ opfifo_write(opcode, xfer->len);
+ dfifo_read(buf, xfer->len);
+
+ return xfer->len;
+}
+
+static int xfer_write(struct spi_transfer *xfer, int next_xfer_is_rx)
+{
+ int opcode;
+ const uint8_t *buf = xfer->tx_buf;
+
+ if (next_xfer_is_rx) {
+ /* need to use Ox2Ix opcode to set the core to input afterwards */
+ switch (xfer->tx_nbits) {
+ case SPI_NBITS_SINGLE:
+ opcode = OP_OS2IS;
+ break;
+ case SPI_NBITS_DUAL:
+ opcode = OP_OD2ID;
+ break;
+ }
+ } else {
+ switch (xfer->tx_nbits) {
+ case SPI_NBITS_SINGLE:
+ opcode = OP_OUTS;
+ break;
+ case SPI_NBITS_DUAL:
+ opcode = OP_OUTD;
+ break;
+ }
+ }
+
+ opfifo_write(opcode, xfer->len);
+ dfifo_write(buf, xfer->len);
+
+ return xfer->len;
+}
+
+static size_t max_transfer_size(struct spi_device *spi)
+{
+ return ENSPI_MAX_XFER;
+}
+
+static int transfer_one_message(struct spi_controller *ctrl, struct spi_message *msg)
+{
+ struct spi_transfer *xfer;
+ int next_xfer_is_rx = 0;
+
+ manual_begin_cmd();
+ set_cs(0);
+ list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+ if (xfer->tx_buf) {
+ if (!list_is_last(&xfer->transfer_list, &msg->transfers)
+ && list_next_entry(xfer, transfer_list)->rx_buf != NULL)
+ next_xfer_is_rx = 1;
+ else
+ next_xfer_is_rx = 0;
+ msg->actual_length += xfer_write(xfer, next_xfer_is_rx);
+ } else if (xfer->rx_buf) {
+ msg->actual_length += xfer_read(xfer);
+ }
+ }
+ set_cs(1);
+ manual_end_cmd();
+
+ msg->status = 0;
+ spi_finalize_current_message(ctrl);
+
+ return 0;
+}
+
+static int spi_probe(struct platform_device *pdev)
+{
+ struct spi_controller *ctrl;
+ int err;
+
+ ctrl = devm_spi_alloc_master(&pdev->dev, 0);
+ if (!ctrl) {
+ dev_err(&pdev->dev, "Error allocating SPI controller\n");
+ return -ENOMEM;
+ }
+
+ iobase = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
+ if (IS_ERR(iobase)) {
+ dev_err(&pdev->dev, "Could not map SPI register address");
+ return -ENOMEM;
+ }
+
+ err = init_hw(&pdev->dev);
+ if (err)
+ return err;
+
+ ctrl->dev.of_node = pdev->dev.of_node;
+ ctrl->flags = SPI_CONTROLLER_HALF_DUPLEX;
+ ctrl->mode_bits = SPI_RX_DUAL | SPI_TX_DUAL;
+ ctrl->max_transfer_size = max_transfer_size;
+ ctrl->transfer_one_message = transfer_one_message;
+ err = devm_spi_register_controller(&pdev->dev, ctrl);
+ if (err) {
+ dev_err(&pdev->dev, "Could not register SPI controller\n");
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
+static const struct of_device_id spi_of_ids[] = {
+ { .compatible = "airoha,en7523-spi" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, spi_of_ids);
+
+static struct platform_driver spi_driver = {
+ .probe = spi_probe,
+ .driver = {
+ .name = "airoha-en7523-spi",
+ .of_match_table = spi_of_ids,
+ },
+};
+
+module_platform_driver(spi_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Bert Vermeulen <bert@biot.com>");
+MODULE_DESCRIPTION("Airoha EN7523 SPI driver");
--
2.25.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 3/3] ARM: dts: en7523: Add SPI node
2022-09-27 11:32 [PATCH v2 0/3] Add support for the Airoha EN7523 SPI controller Bert Vermeulen
2022-09-27 11:32 ` [PATCH v2 1/3] dt-bindings: arm: airoha: Add documentation for Airoha " Bert Vermeulen
2022-09-27 11:32 ` [PATCH v2 2/3] spi: Add support for the Airoha EN7523 SoC " Bert Vermeulen
@ 2022-09-27 11:32 ` Bert Vermeulen
2 siblings, 0 replies; 8+ messages in thread
From: Bert Vermeulen @ 2022-09-27 11:32 UTC (permalink / raw)
To: Mark Brown, Rob Herring, Krzysztof Kozlowski, linux-spi,
devicetree, linux-kernel
Cc: Bert Vermeulen, John Crispin, Benjamin Larsson
This adds an SPI node for the EN7523, so far only used for hooking up
the NAND boot flash.
Signed-off-by: Bert Vermeulen <bert@biot.com>
---
arch/arm/boot/dts/en7523-evb.dts | 20 ++++++++++++++++++++
arch/arm/boot/dts/en7523.dtsi | 10 ++++++++++
2 files changed, 30 insertions(+)
diff --git a/arch/arm/boot/dts/en7523-evb.dts b/arch/arm/boot/dts/en7523-evb.dts
index f23a25cce119..50ccd58b1672 100644
--- a/arch/arm/boot/dts/en7523-evb.dts
+++ b/arch/arm/boot/dts/en7523-evb.dts
@@ -26,6 +26,26 @@ memory@80000000 {
};
};
+&spi0 {
+ nand: nand@0 {
+ compatible = "spi-nand";
+ reg = <0>;
+ nand-ecc-engine = <&nand>;
+
+ partitions {
+ compatible = "fixed-partitions";
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ partition@0 {
+ label = "u-boot";
+ reg = <0x0 0x80000>;
+ read-only;
+ };
+ };
+ };
+};
+
&gpio0 {
status = "okay";
};
diff --git a/arch/arm/boot/dts/en7523.dtsi b/arch/arm/boot/dts/en7523.dtsi
index 7f839331a777..a0d0862005fb 100644
--- a/arch/arm/boot/dts/en7523.dtsi
+++ b/arch/arm/boot/dts/en7523.dtsi
@@ -201,4 +201,14 @@ pcie_intc1: interrupt-controller {
#interrupt-cells = <1>;
};
};
+
+ spi0: spi@1fa10000 {
+ compatible = "airoha,en7523-spi";
+ reg = <0x1fa10000 0x10000>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clocks = <&scu EN7523_CLK_SPI>;
+ spi-rx-bus-width = <2>;
+ spi-tx-bus-width = <2>;
+ };
};
--
2.25.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/3] spi: Add support for the Airoha EN7523 SoC SPI controller
2022-09-27 11:32 ` [PATCH v2 2/3] spi: Add support for the Airoha EN7523 SoC " Bert Vermeulen
@ 2022-09-27 12:23 ` Mark Brown
2022-09-27 15:04 ` Bert Vermeulen
0 siblings, 1 reply; 8+ messages in thread
From: Mark Brown @ 2022-09-27 12:23 UTC (permalink / raw)
To: Bert Vermeulen
Cc: Rob Herring, Krzysztof Kozlowski, linux-spi, devicetree,
linux-kernel, John Crispin, Benjamin Larsson
[-- Attachment #1: Type: text/plain, Size: 2470 bytes --]
On Tue, Sep 27, 2022 at 01:32:28PM +0200, Bert Vermeulen wrote:
>
> +config SPI_AIROHA_EN7523
> + bool "Airoha EN7523 SPI controller support"
Why not tristate?
> + depends on ARCH_AIROHA
I don't see a reason we couldn't have an || COMPILE_TEST here to improve
coverage?
> + default ARCH_AIROHA
It's unusual to default a SPI controller on, they tend not to be ultra
critical like a clock driver or similar can be?
> +static void __iomem *iobase;
This should be driver data rather than a global, your current SoC might
only have one controller but some other model might build two and it's
fairly trivial to do.
> +static void opfifo_write(u32 cmd, u32 len)
> +{
> + u32 tmp = ((cmd & 0x1f) << 9) | (len & 0x1ff);
> +
> + writel(tmp, REG(ENSPI_MANUAL_OPFIFO_WDATA));
> +
> + /* Wait for room in OPFIFO */
> + while (readl(REG(ENSPI_MANUAL_OPFIFO_FULL)))
> + cpu_relax();
> +
Some sort of timeout would be good with these loops, if things go wrong
we'll just lock up which isn't good.
> + ret = clk_prepare_enable(clk);
> + if (ret)
> + return ret;
Nothing ever reverses this unless clk_set_rate() fails.
> + ret = clk_set_rate(clk, 40000000);
> + if (ret) {
> + clk_disable_unprepare(clk);
> + return ret;
> + }
Could this be pushed into DT via the clock bindings? The hard coded
number might need to vary by SoC.
> +static int xfer_read(struct spi_transfer *xfer)
> +{
> + int opcode;
> + uint8_t *buf = xfer->rx_buf;
> +
> + switch (xfer->rx_nbits) {
> + case SPI_NBITS_SINGLE:
> + opcode = OP_INS;
> + break;
> + case SPI_NBITS_DUAL:
> + opcode = OP_IND;
> + break;
> + }
This should have a default case that returns an error.
> +static int transfer_one_message(struct spi_controller *ctrl, struct spi_message *msg)
> +{
> + struct spi_transfer *xfer;
> + int next_xfer_is_rx = 0;
> +
> + manual_begin_cmd();
> + set_cs(0);
The driver should not be setting chip select itself, it should just
provide the set_cs() operation to the core and let the core worry about
when to call it.
> + ctrl->transfer_one_message = transfer_one_message;
> + err = devm_spi_register_controller(&pdev->dev, ctrl);
> + if (err) {
> + dev_err(&pdev->dev, "Could not register SPI controller\n");
> + return -ENODEV;
> + }
Don't discard the error code that registeration returned, include it in
the log message and pass it back to the caller.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/3] spi: Add support for the Airoha EN7523 SoC SPI controller
2022-09-27 12:23 ` Mark Brown
@ 2022-09-27 15:04 ` Bert Vermeulen
0 siblings, 0 replies; 8+ messages in thread
From: Bert Vermeulen @ 2022-09-27 15:04 UTC (permalink / raw)
To: Mark Brown
Cc: Rob Herring, Krzysztof Kozlowski, linux-spi, devicetree,
linux-kernel, John Crispin, Benjamin Larsson
On 9/27/22 14:23, Mark Brown wrote:
Hi Mark,
Thanks for reviewing.
> On Tue, Sep 27, 2022 at 01:32:28PM +0200, Bert Vermeulen wrote:
>
>>
>> +config SPI_AIROHA_EN7523
>> + bool "Airoha EN7523 SPI controller support"
>
> Why not tristate?
>
>> + depends on ARCH_AIROHA
>
> I don't see a reason we couldn't have an || COMPILE_TEST here to improve
> coverage?
>
>> + default ARCH_AIROHA
In both cases, because SPi boot flash is the only way to boot this SoC
that I know of. However as you say this may not be the case on different
SoCs, and indeed I believe this SPI core is in lots of stuff already.
So I'll fix this, and also address your other comments.
> It's unusual to default a SPI controller on, they tend not to be ultra
> critical like a clock driver or similar can be?
>
>> +static void __iomem *iobase;
>
> This should be driver data rather than a global, your current SoC might
> only have one controller but some other model might build two and it's
> fairly trivial to do.
>
>> +static void opfifo_write(u32 cmd, u32 len)
>> +{
>> + u32 tmp = ((cmd & 0x1f) << 9) | (len & 0x1ff);
>> +
>> + writel(tmp, REG(ENSPI_MANUAL_OPFIFO_WDATA));
>> +
>> + /* Wait for room in OPFIFO */
>> + while (readl(REG(ENSPI_MANUAL_OPFIFO_FULL)))
>> + cpu_relax();
>> +
>
> Some sort of timeout would be good with these loops, if things go wrong
> we'll just lock up which isn't good.
>
>> + ret = clk_prepare_enable(clk);
>> + if (ret)
>> + return ret;
>
> Nothing ever reverses this unless clk_set_rate() fails.
>
>> + ret = clk_set_rate(clk, 40000000);
>> + if (ret) {
>> + clk_disable_unprepare(clk);
>> + return ret;
>> + }
>
> Could this be pushed into DT via the clock bindings? The hard coded
> number might need to vary by SoC.
>
>> +static int xfer_read(struct spi_transfer *xfer)
>> +{
>> + int opcode;
>> + uint8_t *buf = xfer->rx_buf;
>> +
>> + switch (xfer->rx_nbits) {
>> + case SPI_NBITS_SINGLE:
>> + opcode = OP_INS;
>> + break;
>> + case SPI_NBITS_DUAL:
>> + opcode = OP_IND;
>> + break;
>> + }
>
> This should have a default case that returns an error.
>
>> +static int transfer_one_message(struct spi_controller *ctrl, struct spi_message *msg)
>> +{
>> + struct spi_transfer *xfer;
>> + int next_xfer_is_rx = 0;
>> +
>> + manual_begin_cmd();
>> + set_cs(0);
>
> The driver should not be setting chip select itself, it should just
> provide the set_cs() operation to the core and let the core worry about
> when to call it.
>
>> + ctrl->transfer_one_message = transfer_one_message;
>> + err = devm_spi_register_controller(&pdev->dev, ctrl);
>> + if (err) {
>> + dev_err(&pdev->dev, "Could not register SPI controller\n");
>> + return -ENODEV;
>> + }
>
> Don't discard the error code that registeration returned, include it in
> the log message and pass it back to the caller.
--
Bert Vermeulen
bert@biot.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: arm: airoha: Add documentation for Airoha SPI controller
2022-09-27 11:32 ` [PATCH v2 1/3] dt-bindings: arm: airoha: Add documentation for Airoha " Bert Vermeulen
@ 2022-09-29 22:21 ` Rob Herring
2022-09-30 10:29 ` Krzysztof Kozlowski
1 sibling, 0 replies; 8+ messages in thread
From: Rob Herring @ 2022-09-29 22:21 UTC (permalink / raw)
To: Bert Vermeulen
Cc: Rob Herring, devicetree, linux-kernel, John Crispin,
Krzysztof Kozlowski, Benjamin Larsson, linux-spi, Mark Brown
On Tue, 27 Sep 2022 13:32:27 +0200, Bert Vermeulen wrote:
> Create documentation for accessing the Airoha EN7523 SPI controller.
>
> Signed-off-by: Bert Vermeulen <bert@biot.com>
> ---
> .../bindings/spi/airoha,en7523-spi.yaml | 45 +++++++++++++++++++
> 1 file changed, 45 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/spi/airoha,en7523-spi.yaml
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: arm: airoha: Add documentation for Airoha SPI controller
2022-09-27 11:32 ` [PATCH v2 1/3] dt-bindings: arm: airoha: Add documentation for Airoha " Bert Vermeulen
2022-09-29 22:21 ` Rob Herring
@ 2022-09-30 10:29 ` Krzysztof Kozlowski
1 sibling, 0 replies; 8+ messages in thread
From: Krzysztof Kozlowski @ 2022-09-30 10:29 UTC (permalink / raw)
To: Bert Vermeulen, Mark Brown, Rob Herring, Krzysztof Kozlowski,
linux-spi, devicetree, linux-kernel
Cc: John Crispin, Benjamin Larsson
On 27/09/2022 13:32, Bert Vermeulen wrote:
> Create documentation for accessing the Airoha EN7523 SPI controller.
>
> Signed-off-by: Bert Vermeulen <bert@biot.com>
Use subject prefixes matching the subsystem (git log --oneline -- ...).
> ---
> .../bindings/spi/airoha,en7523-spi.yaml | 45 +++++++++++++++++++
> 1 file changed, 45 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/spi/airoha,en7523-spi.yaml
>
> diff --git a/Documentation/devicetree/bindings/spi/airoha,en7523-spi.yaml b/Documentation/devicetree/bindings/spi/airoha,en7523-spi.yaml
> new file mode 100644
> index 000000000000..8f4936512a99
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/airoha,en7523-spi.yaml
> @@ -0,0 +1,45 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/airoha,en7523-spi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Airoha EN7523 SPI controller
> +
> +maintainers:
> + - Bert Vermeulen <bert@biot.com>
> +
> +description: |
> + This binding describes the SPI controller on Airoha EN7523 SoCs.
> +
> +allOf:
> + - $ref: spi-controller.yaml#
> +
> +properties:
> + compatible:
> + items:
Drop "items:" and leading hyphen from line below.
> + - const: airoha,en7523-spi
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-09-30 11:00 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-27 11:32 [PATCH v2 0/3] Add support for the Airoha EN7523 SPI controller Bert Vermeulen
2022-09-27 11:32 ` [PATCH v2 1/3] dt-bindings: arm: airoha: Add documentation for Airoha " Bert Vermeulen
2022-09-29 22:21 ` Rob Herring
2022-09-30 10:29 ` Krzysztof Kozlowski
2022-09-27 11:32 ` [PATCH v2 2/3] spi: Add support for the Airoha EN7523 SoC " Bert Vermeulen
2022-09-27 12:23 ` Mark Brown
2022-09-27 15:04 ` Bert Vermeulen
2022-09-27 11:32 ` [PATCH v2 3/3] ARM: dts: en7523: Add SPI node Bert Vermeulen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.