* [PATCH 0/2] Add rockchip serial flash controller support
@ 2016-11-11 9:16 ` Shawn Lin
0 siblings, 0 replies; 20+ messages in thread
From: Shawn Lin @ 2016-11-11 9:16 UTC (permalink / raw)
To: Rob Herring, David Woodhouse, Brian Norris
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Heiko Stuebner,
Shawn Lin
This pathset is gonna support serial flash controller
, namely SFC, found on Rockchip RK1108 platform.
Feature:
(1) Support x1, x2, x4 data bits mode
(2) Support up to 4 chip select
(3) Support two independent clock domain: AHB clock and SPI clock
(4) Support DMA master up to 16KB/transfer
Test environment:
This patchset is testing on RK1108 evb boards with Winboud flash
(w25q256) and working find with PIO or DMA mode.
How-to:
Any rockchip guys who are interested in testing it could refer to
the following steps:
(1) enable CONFIG_MTD_M25P80
(2) enable CONFIG_SPI_ROCKCHIP_SFC
(3) enable CONFIG_MTD_CMDLINE_PARTS
(4) enable CONFIG_SQUASHFS
(4) CONFIG_CMDLINE="root=/dev/mtdblock2
mtdparts=spi-nor:256k@0(loader)ro,8m(kernel)ro,7m(rootfs),-(freedisk)"
Of course, you should check the partition layout if you modify it. Also
you could pass it from your loader to the kernel's cmdline.
(5) Add dts support:
nor_flash: sfc@301c0000 {
compatible = "rockchip,rk1108-sfc", "rockchip,sfc";
#address-cells = <1>;
#size-cells = <0>;
clocks = <&cru SCLK_SFC>, <&cru HCLK_SFC>;
clock-names = "sfc", "hsfc";
interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
reg = <0x301c0000 0x1000>;
/* If you want to use PIO mode, activate this */
#rockchip,sfc-no-dma;
spi-nor@0 {
compatible = "jedec,spi-nor";
spi-max-frequency = <12000000>;
reg = <0>;
}
};
please make sure your DT's mdtid matchs what you assgin to the
mdtparts(cmdline), namely they are both *spi-nor* here.
With enabling DBG for cmdlinepart.c, you could get following log and
boot kernel and rootfs successfully.
[ 0.481420] rockchip-sfc 301c0000.sfc: w25q256 (32768 Kbytes)
[ 0.481962] DEBUG-CMDLINE-PART: parsing
<256k@0(loader)ro,8m(kernel)ro,7m(rootfs)ro,-(freedisk)>
[ 0.482897] DEBUG-CMDLINE-PART: partition 3: name
<freedisk>, offset ffffffffffffffff, size ffffffffffffffff, mask flags 0
[ 0.484021] DEBUG-CMDLINE-PART: partition 2: name
<rootfs>, offset ffffffffffffffff, size 700000, mask flags 400
[ 0.485066] DEBUG-CMDLINE-PART: partition 1: name
<kernel>, offset ffffffffffffffff, size 800000, mask flags 400
[ 0.486108] DEBUG-CMDLINE-PART: partition 0: name
<loader>, offset 0, size 40000, mask flags 400
[ 0.487152] DEBUG-CMDLINE-PART: mtdid=<spi-nor> num_parts=<4>
[ 0.487827] 4 cmdlinepart partitions found on MTD device spi-nor
[ 0.488370] Creating 4 MTD partitions on "spi-nor":
[ 0.488826] 0x000000000000-0x000000040000 : "loader"
[ 0.492340] 0x000000040000-0x000000840000 : "kernel"
[ 0.495679] 0x000000840000-0x000000f40000 : "rootfs"
[ 0.499241] 0x000000f40000-0x000002000000 : "freedisk"
[root@arm-linux]#
[root@arm-linux]#mount
/dev/root on / type squashfs (ro,relatime)
devtmpfs on /dev type devtmpfs
(rw,relatime,size=26124k,nr_inodes=6531,mode=755)
proc on /proc type proc (rw,relatime)
none on /tmp type ramfs (rw,relatime)
none on /var type ramfs (rw,relatime)
sysfs on /sys type sysfs (rw,relatime)
debug on /sys/kernel/debug type debugfs (rw,relatime)
none on /dev/pts type devpts (rw,relatime,mode=600,ptmxmode=000)
Shawn Lin (2):
mtd: spi-nor: Bindings for Rockchip serial flash controller
mtd: spi-nor: add rockchip serial flash controller driver
.../devicetree/bindings/mtd/rockchip-sfc.txt | 31 +
MAINTAINERS | 8 +
drivers/mtd/spi-nor/Kconfig | 7 +
drivers/mtd/spi-nor/Makefile | 1 +
drivers/mtd/spi-nor/rockchip-sfc.c | 953 +++++++++++++++++++++
5 files changed, 1000 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mtd/rockchip-sfc.txt
create mode 100644 drivers/mtd/spi-nor/rockchip-sfc.c
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 0/2] Add rockchip serial flash controller support
@ 2016-11-11 9:16 ` Shawn Lin
0 siblings, 0 replies; 20+ messages in thread
From: Shawn Lin @ 2016-11-11 9:16 UTC (permalink / raw)
To: Rob Herring, David Woodhouse, Brian Norris
Cc: devicetree, linux-rockchip, linux-mtd, Heiko Stuebner, Shawn Lin
This pathset is gonna support serial flash controller
, namely SFC, found on Rockchip RK1108 platform.
Feature:
(1) Support x1, x2, x4 data bits mode
(2) Support up to 4 chip select
(3) Support two independent clock domain: AHB clock and SPI clock
(4) Support DMA master up to 16KB/transfer
Test environment:
This patchset is testing on RK1108 evb boards with Winboud flash
(w25q256) and working find with PIO or DMA mode.
How-to:
Any rockchip guys who are interested in testing it could refer to
the following steps:
(1) enable CONFIG_MTD_M25P80
(2) enable CONFIG_SPI_ROCKCHIP_SFC
(3) enable CONFIG_MTD_CMDLINE_PARTS
(4) enable CONFIG_SQUASHFS
(4) CONFIG_CMDLINE="root=/dev/mtdblock2
mtdparts=spi-nor:256k@0(loader)ro,8m(kernel)ro,7m(rootfs),-(freedisk)"
Of course, you should check the partition layout if you modify it. Also
you could pass it from your loader to the kernel's cmdline.
(5) Add dts support:
nor_flash: sfc@301c0000 {
compatible = "rockchip,rk1108-sfc", "rockchip,sfc";
#address-cells = <1>;
#size-cells = <0>;
clocks = <&cru SCLK_SFC>, <&cru HCLK_SFC>;
clock-names = "sfc", "hsfc";
interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
reg = <0x301c0000 0x1000>;
/* If you want to use PIO mode, activate this */
#rockchip,sfc-no-dma;
spi-nor@0 {
compatible = "jedec,spi-nor";
spi-max-frequency = <12000000>;
reg = <0>;
}
};
please make sure your DT's mdtid matchs what you assgin to the
mdtparts(cmdline), namely they are both *spi-nor* here.
With enabling DBG for cmdlinepart.c, you could get following log and
boot kernel and rootfs successfully.
[ 0.481420] rockchip-sfc 301c0000.sfc: w25q256 (32768 Kbytes)
[ 0.481962] DEBUG-CMDLINE-PART: parsing
<256k@0(loader)ro,8m(kernel)ro,7m(rootfs)ro,-(freedisk)>
[ 0.482897] DEBUG-CMDLINE-PART: partition 3: name
<freedisk>, offset ffffffffffffffff, size ffffffffffffffff, mask flags 0
[ 0.484021] DEBUG-CMDLINE-PART: partition 2: name
<rootfs>, offset ffffffffffffffff, size 700000, mask flags 400
[ 0.485066] DEBUG-CMDLINE-PART: partition 1: name
<kernel>, offset ffffffffffffffff, size 800000, mask flags 400
[ 0.486108] DEBUG-CMDLINE-PART: partition 0: name
<loader>, offset 0, size 40000, mask flags 400
[ 0.487152] DEBUG-CMDLINE-PART: mtdid=<spi-nor> num_parts=<4>
[ 0.487827] 4 cmdlinepart partitions found on MTD device spi-nor
[ 0.488370] Creating 4 MTD partitions on "spi-nor":
[ 0.488826] 0x000000000000-0x000000040000 : "loader"
[ 0.492340] 0x000000040000-0x000000840000 : "kernel"
[ 0.495679] 0x000000840000-0x000000f40000 : "rootfs"
[ 0.499241] 0x000000f40000-0x000002000000 : "freedisk"
[root@arm-linux]#
[root@arm-linux]#mount
/dev/root on / type squashfs (ro,relatime)
devtmpfs on /dev type devtmpfs
(rw,relatime,size=26124k,nr_inodes=6531,mode=755)
proc on /proc type proc (rw,relatime)
none on /tmp type ramfs (rw,relatime)
none on /var type ramfs (rw,relatime)
sysfs on /sys type sysfs (rw,relatime)
debug on /sys/kernel/debug type debugfs (rw,relatime)
none on /dev/pts type devpts (rw,relatime,mode=600,ptmxmode=000)
Shawn Lin (2):
mtd: spi-nor: Bindings for Rockchip serial flash controller
mtd: spi-nor: add rockchip serial flash controller driver
.../devicetree/bindings/mtd/rockchip-sfc.txt | 31 +
MAINTAINERS | 8 +
drivers/mtd/spi-nor/Kconfig | 7 +
drivers/mtd/spi-nor/Makefile | 1 +
drivers/mtd/spi-nor/rockchip-sfc.c | 953 +++++++++++++++++++++
5 files changed, 1000 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mtd/rockchip-sfc.txt
create mode 100644 drivers/mtd/spi-nor/rockchip-sfc.c
--
1.9.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/2] mtd: spi-nor: Bindings for Rockchip serial flash controller
2016-11-11 9:16 ` Shawn Lin
@ 2016-11-11 9:16 ` Shawn Lin
-1 siblings, 0 replies; 20+ messages in thread
From: Shawn Lin @ 2016-11-11 9:16 UTC (permalink / raw)
To: Rob Herring, David Woodhouse, Brian Norris
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Heiko Stuebner,
Shawn Lin
Add binding document for the Rockchip serial flash controller.
Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
.../devicetree/bindings/mtd/rockchip-sfc.txt | 31 ++++++++++++++++++++++
1 file changed, 31 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mtd/rockchip-sfc.txt
diff --git a/Documentation/devicetree/bindings/mtd/rockchip-sfc.txt b/Documentation/devicetree/bindings/mtd/rockchip-sfc.txt
new file mode 100644
index 0000000..28430ce
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/rockchip-sfc.txt
@@ -0,0 +1,31 @@
+Rockchip Serial Flash Controller
+
+Required properties:
+- compatible : Should be
+ "rockchip,rk1108-sfc", "rockchip,sfc" for ROCKCHIP RK1108.
+- address-cells : Should be 1.
+- size-cells : Should be 0.
+- clocks: Must contain two entries for each entry in clock-names.
+- clock-names: Shall be "sfc" for the transfer-clock, and "hsfc" for
+ the peripheral clock.
+- interrupts : Should contain the interrupt for the device.
+- reg: Physical base address of the controller and length of memory mapped.
+
+Optional properties:
+- rockchip,sfc-no-dma: Indicate the controller doesn't support dma transfer.
+
+Example:
+nor_flash: sfc@301c0000 {
+ compatible = "rockchip,rk1108-sfc", "rockchip,sfc";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clocks = <&cru SCLK_SFC>, <&cru HCLK_SFC>;
+ clock-names = "sfc", "hsfc";
+ interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
+ reg = <0x301c0000 0x1000>;
+ spi-nor@0 {
+ compatible = "jedec,spi-nor";
+ spi-max-frequency = <12000000>;
+ reg = <0>;
+ };
+};
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 1/2] mtd: spi-nor: Bindings for Rockchip serial flash controller
@ 2016-11-11 9:16 ` Shawn Lin
0 siblings, 0 replies; 20+ messages in thread
From: Shawn Lin @ 2016-11-11 9:16 UTC (permalink / raw)
To: Rob Herring, David Woodhouse, Brian Norris
Cc: devicetree, linux-rockchip, linux-mtd, Heiko Stuebner, Shawn Lin
Add binding document for the Rockchip serial flash controller.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
.../devicetree/bindings/mtd/rockchip-sfc.txt | 31 ++++++++++++++++++++++
1 file changed, 31 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mtd/rockchip-sfc.txt
diff --git a/Documentation/devicetree/bindings/mtd/rockchip-sfc.txt b/Documentation/devicetree/bindings/mtd/rockchip-sfc.txt
new file mode 100644
index 0000000..28430ce
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/rockchip-sfc.txt
@@ -0,0 +1,31 @@
+Rockchip Serial Flash Controller
+
+Required properties:
+- compatible : Should be
+ "rockchip,rk1108-sfc", "rockchip,sfc" for ROCKCHIP RK1108.
+- address-cells : Should be 1.
+- size-cells : Should be 0.
+- clocks: Must contain two entries for each entry in clock-names.
+- clock-names: Shall be "sfc" for the transfer-clock, and "hsfc" for
+ the peripheral clock.
+- interrupts : Should contain the interrupt for the device.
+- reg: Physical base address of the controller and length of memory mapped.
+
+Optional properties:
+- rockchip,sfc-no-dma: Indicate the controller doesn't support dma transfer.
+
+Example:
+nor_flash: sfc@301c0000 {
+ compatible = "rockchip,rk1108-sfc", "rockchip,sfc";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clocks = <&cru SCLK_SFC>, <&cru HCLK_SFC>;
+ clock-names = "sfc", "hsfc";
+ interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
+ reg = <0x301c0000 0x1000>;
+ spi-nor@0 {
+ compatible = "jedec,spi-nor";
+ spi-max-frequency = <12000000>;
+ reg = <0>;
+ };
+};
--
1.9.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/2] mtd: spi-nor: add rockchip serial flash controller driver
2016-11-11 9:16 ` Shawn Lin
@ 2016-11-11 9:16 ` Shawn Lin
-1 siblings, 0 replies; 20+ messages in thread
From: Shawn Lin @ 2016-11-11 9:16 UTC (permalink / raw)
To: Rob Herring, David Woodhouse, Brian Norris
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Heiko Stuebner,
Shawn Lin
Add rockchip serial flash controller driver
Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
MAINTAINERS | 8 +
drivers/mtd/spi-nor/Kconfig | 7 +
drivers/mtd/spi-nor/Makefile | 1 +
drivers/mtd/spi-nor/rockchip-sfc.c | 953 +++++++++++++++++++++++++++++++++++++
4 files changed, 969 insertions(+)
create mode 100644 drivers/mtd/spi-nor/rockchip-sfc.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 1cd38a7..eb7e06d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10266,6 +10266,14 @@ L: linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
S: Odd Fixes
F: drivers/tty/serial/rp2.*
+ROCKCHIP SERIAL FLASH CONTROLLER DRIVER
+M: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
+L: linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
+L: linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
+S: Maintained
+F: Documentation/devicetree/bindings/mtd/rockchip-sfc.txt
+F: drivers/mtd/spi-nor/rockchip-sfc.c
+
ROSE NETWORK LAYER
M: Ralf Baechle <ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
L: linux-hams-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index 4a682ee..48c5e0e 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -65,6 +65,13 @@ config SPI_HISI_SFC
help
This enables support for hisilicon SPI-NOR flash controller.
+config SPI_ROCKCHIP_SFC
+ tristate "Rockchip Serial Flash Controller(SFC)"
+ depends on ARCH_ROCKCHIP || COMPILE_TEST
+ depends on HAS_IOMEM && HAS_DMA
+ help
+ This enables support for rockchip serial flash controller.
+
config SPI_NXP_SPIFI
tristate "NXP SPI Flash Interface (SPIFI)"
depends on OF && (ARCH_LPC18XX || COMPILE_TEST)
diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
index 121695e..364d4c6 100644
--- a/drivers/mtd/spi-nor/Makefile
+++ b/drivers/mtd/spi-nor/Makefile
@@ -5,3 +5,4 @@ obj-$(CONFIG_SPI_FSL_QUADSPI) += fsl-quadspi.o
obj-$(CONFIG_SPI_HISI_SFC) += hisi-sfc.o
obj-$(CONFIG_MTD_MT81xx_NOR) += mtk-quadspi.o
obj-$(CONFIG_SPI_NXP_SPIFI) += nxp-spifi.o
+obj-$(CONFIG_SPI_ROCKCHIP_SFC) += rockchip-sfc.o
diff --git a/drivers/mtd/spi-nor/rockchip-sfc.c b/drivers/mtd/spi-nor/rockchip-sfc.c
new file mode 100644
index 0000000..83930d6
--- /dev/null
+++ b/drivers/mtd/spi-nor/rockchip-sfc.c
@@ -0,0 +1,953 @@
+/*
+ * Rockchip Serial Flash Controller Driver
+ *
+ * Copyright (c) 2016, Rockchip Inc.
+ * Author: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * 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/bitops.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/dma-mapping.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/spi-nor.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/* System control */
+#define SFC_CTRL 0x0
+#define SFC_CTRL_COMMON_BITS_1 0x0
+#define SFC_CTRL_COMMON_BITS_2 0x1
+#define SFC_CTRL_COMMON_BITS_4 0x2
+#define SFC_CTRL_DATA_BITS_SHIFT 12
+#define SFC_CTRL_DATA_BITS_MASK 0x3
+#define SFC_CTRL_ADDR_BITS_SHIFT 10
+#define SFC_CTRL_ADDR_BITS_MASK 0x3
+#define SFC_CTRL_CMD_BITS_SHIFT 8
+#define SFC_CTRL_CMD_BITS_MASK 0x3
+#define SFC_CTRL_IDLE_CYC_SHIFT 4
+#define SFC_CTRL_IDLE_CYC_MASK 0xf
+#define SFC_CTRL_PHASE_SEL_SHIFT 1
+#define SFC_CTRL_PHASE_SEL_NEGETIVE 0x1
+#define SFC_CTRL_PHASE_SEL_POSITIVE 0x0
+#define SFC_CTRL_MODE_SEL_SHIFT 0
+#define SFC_CTRL_MODE_MASK 0x1
+
+/* Interrypt mask */
+#define SFC_IMR 0x4
+#define SFC_IMR_RX_FULL BIT(0)
+#define SFC_IMR_RX_UFLOW BIT(1)
+#define SFC_IMR_TX_OFLOW BIT(2)
+#define SFC_IMR_TX_EMPTY BIT(3)
+#define SFC_IMR_TRAN_FINISH BIT(4)
+#define SFC_IMR_BUS_ERR BIT(5)
+#define SFC_IMR_NSPI_ERR BIT(6)
+#define SFC_IMR_DMA BIT(7)
+/* Interrupt clear */
+#define SFC_ICLR 0x8
+#define SFC_ICLR_RX_FULL BIT(0)
+#define SFC_ICLR_RX_UFLOW BIT(1)
+#define SFC_ICLR_TX_OFLOW BIT(2)
+#define SFC_ICLR_TX_EMPTY BIT(3)
+#define SFC_ICLR_TRAN_FINISH BIT(4)
+#define SFC_ICLR_BUS_ERR BIT(5)
+#define SFC_ICLR_NSPI_ERR BIT(6)
+#define SFC_ICLR_DMA BIT(7)
+/* FIFO threshold level */
+#define SFC_FTLR 0xc
+#define SFC_FTLR_TX_SHIFT 0
+#define SFC_FTLR_TX_MASK 0x1f
+#define SFC_FTLR_RX_SHIFT 8
+#define SFC_FTLR_RX_MASK 0x1f
+/* Reset FSM and FIFO */
+#define SFC_RCVR 0x10
+#define SFC_RCVR_RESET BIT(0)
+/* Enhanced mode */
+#define SFC_AX 0x14
+#define SFC_AX_MODE_BIT_SHIFT 0
+#define SFC_AX_MODE_BIT_MASK 0xff
+/* Address Bit number */
+#define SFC_ABIT 0x18
+#define SFC_ABIT_ADD_SHIFT 0
+#define SFC_ABIT_ADD_MASK 0x1f
+/* Interrupt status */
+#define SFC_ISR 0x1c
+#define SFC_ISR_COMMON_ACTIVE 0x1
+#define SFC_ISR_COMMON_MASK 0x1
+#define SFC_ISR_RX_FULL_SHIFT 0
+#define SFC_ISR_RX_UFLOW_SHIFT 1
+#define SFC_ISR_TX_OFLOW_SHIFT 2
+#define SFC_ISR_TX_EMPTY_SHIFT 3
+#define SFC_ISR_TX_FINISH_SHIFT 4
+#define SFC_ISR_BUS_ERR_SHIFT 5
+#define SFC_ISR_NSPI_ERR_SHIFT 6
+#define SFC_ISR_DMA_SHIFT 7
+/* FIFO status */
+#define SFC_FSR 0x20
+#define SFC_FSR_TX_IS_FULL 0x1
+#define SFC_FSR_TX_FULL_SHIFT 0
+#define SFC_FSR_TX_FULL_MASK 0x1
+#define SFC_FSR_TX_IS_EMPTY 0x1
+#define SFC_FSR_TX_EMPTY_SHIFT 1
+#define SFC_FSR_TX_EMPTY_MASK 0x1
+#define SFC_FSR_RX_IS_EMPTY 0x1
+#define SFC_FSR_RX_EMPTY_SHIFT 2
+#define SFC_FSR_RX_EMPTY_MASK 0x1
+#define SFC_FSR_RX_IS_FULL 0x1
+#define SFC_FSR_RX_FULL_SHIFT 3
+#define SFC_FSR_RX_FULL_MASK 0x1
+#define SFC_FSR_TX_WATER_LVL_SHIFT 8
+#define SFC_FSR_TX_WATER_LVL_MASK 0x1f
+#define SFC_FSR_RX_WATER_LVL_SHIFT 16
+#define SFC_FSR_RX_WATER_LVL_MASK 0x1f
+/* FSM status */
+#define SFC_SR 0x24
+#define SFC_SR_IS_IDLE 0x0
+#define SFC_SR_IS_BUSY 0x1
+/* Raw interrupt status */
+#define SFC_RISR 0x28
+#define SFC_RISR_COMMON_ACTIVE 0x1
+#define SFC_RISR_COMMON_MASK 0x1
+#define SFC_RISR_RX_FULL_SHIFT 0
+#define SFC_RISR_RX_UFLOW_SHIFT 1
+#define SFC_RISR_TX_OFLOW_SHIFT 2
+#define SFC_RISR_TX_EMPTY_SHIFT 3
+#define SFC_RISR_TRAN_FINISH_SHIFT 4
+#define SFC_RISR_BUS_ERR_SHIFT 5
+#define SFC_RISR_NSPI_ERR_SHIFT 6
+#define SFC_RISR_DMA_SHIFT 7
+/* Master trigger */
+#define SFC_DMA_TRIGGER 0x80
+/* Src or Dst addr for master */
+#define SFC_DMA_ADDR 0x84
+/* Command */
+#define SFC_CMD 0x100
+#define SFC_CMD_IDX_SHIFT 0
+#define SFC_CMD_IDX_MASK 0xff
+#define SFC_CMD_DUMMY_SHIFT 8
+#define SFC_CMD_DUMMY_MASK 0xf
+#define SFC_CMD_DIR_RD 0
+#define SFC_CMD_DIR_WR 1
+#define SFC_CMD_DIR_SHIFT 12
+#define SFC_CMD_DIR_MASK 0x1
+#define SFC_CMD_CONTI_RD 1
+#define SFC_CMD_CONTI_RD_SHIFT 13
+#define SFC_CMD_ADDR_ZERO 0
+#define SFC_CMD_ADDR_24BITS 0x1
+#define SFC_CMD_ADDR_32BITS 0x2
+#define SFC_CMD_ADDR_FRS 0x3
+#define SFC_CMD_ADDR_SHIFT 14
+#define SFC_CMD_TRAN_BYTES_SHIFT 16
+#define SFC_CMD_TRAN_BYTES_MASK 0x3fff
+#define SFC_CMD_CS_SHIFT 30
+#define SFC_CMD_CS_MASK 0x3
+/* Address */
+#define SFC_ADDR 0x104
+/* Data */
+#define SFC_DATA 0x108
+
+#define SFC_MAX_CHIP_NUM 4
+#define SFC_MAX_IDLE_RETRY 10000
+#define SFC_WAIT_IDLE_TIMEOUT 1000000
+#define SFC_DMA_MAX_LEN 0x4000
+#define SFC_DMA_MAX_MASK (SFC_DMA_MAX_LEN - 1)
+#define SFC_IRQ_TRAN_FINISH BIT(SFC_RISR_TRAN_FINISH_SHIFT)
+
+enum rockchip_sfc_iftype {
+ IF_TYPE_STD,
+ IF_TYPE_DUAL,
+ IF_TYPE_QUAD,
+};
+
+struct rockchip_sfc {
+ struct device *dev;
+ struct mutex lock;
+ void __iomem *regbase;
+ struct clk *hclk;
+ struct clk *clk;
+ void *buffer;
+ dma_addr_t dma_buffer;
+ struct completion cp;
+ struct spi_nor *nor[SFC_MAX_CHIP_NUM];
+ u32 num_chip;
+ bool use_dma;
+ bool negative_edge;
+};
+
+struct rockchip_sfc_priv {
+ u32 cs;
+ u32 clk_rate;
+ struct rockchip_sfc *sfc;
+};
+
+static int get_if_type(enum read_mode flash_read)
+{
+ enum rockchip_sfc_iftype if_type;
+
+ switch (flash_read) {
+ case SPI_NOR_DUAL:
+ if_type = IF_TYPE_DUAL;
+ break;
+ case SPI_NOR_QUAD:
+ if_type = IF_TYPE_QUAD;
+ break;
+ case SPI_NOR_NORMAL:
+ case SPI_NOR_FAST:
+ default:
+ if_type = IF_TYPE_STD;
+ break;
+ }
+
+ return if_type;
+}
+
+static int rockchip_sfc_reset(struct rockchip_sfc *sfc)
+{
+ unsigned long timeout = jiffies + HZ;
+ int err = -ETIMEDOUT;
+ u32 status;
+
+ writel_relaxed(SFC_RCVR_RESET, sfc->regbase + SFC_RCVR);
+
+ while (time_before(jiffies, timeout)) {
+ status = readl_relaxed(sfc->regbase + SFC_RCVR);
+ if (!(status & SFC_RCVR_RESET)) {
+ err = 0;
+ break;
+ }
+ msleep(20);
+ }
+
+ if (err)
+ dev_err(sfc->dev, "SFC reset never finished\n");
+
+ writel_relaxed(SFC_ICLR_RX_FULL | SFC_ICLR_RX_UFLOW |
+ SFC_ICLR_TX_OFLOW | SFC_ICLR_TX_EMPTY |
+ SFC_ICLR_TRAN_FINISH | SFC_ICLR_BUS_ERR |
+ SFC_ICLR_NSPI_ERR | SFC_ICLR_DMA,
+ sfc->regbase + SFC_ICLR);
+ return err;
+}
+
+static int rockchip_sfc_init(struct rockchip_sfc *sfc)
+{
+ int err;
+
+ err = rockchip_sfc_reset(sfc);
+ if (err)
+ return err;
+
+ /* Mask all eight interrupts */
+ writel_relaxed(0xff, sfc->regbase + SFC_IMR);
+ /* Phase configure */
+ if (sfc->negative_edge)
+ writel_relaxed(SFC_CTRL_PHASE_SEL_NEGETIVE <<
+ SFC_CTRL_PHASE_SEL_SHIFT,
+ sfc->regbase + SFC_CTRL);
+ return 0;
+}
+
+static int rockchip_sfc_prep(struct spi_nor *nor, enum spi_nor_ops ops)
+{
+ struct rockchip_sfc_priv *priv = nor->priv;
+ struct rockchip_sfc *sfc = priv->sfc;
+ int ret;
+
+ mutex_lock(&sfc->lock);
+
+ ret = clk_set_rate(sfc->clk, priv->clk_rate);
+ if (ret)
+ goto out;
+
+ ret = clk_prepare_enable(sfc->clk);
+ if (ret)
+ goto out;
+
+ return 0;
+
+out:
+ mutex_unlock(&sfc->lock);
+ return ret;
+}
+
+static void rockchip_sfc_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
+{
+ struct rockchip_sfc_priv *priv = nor->priv;
+ struct rockchip_sfc *sfc = priv->sfc;
+
+ clk_disable_unprepare(sfc->clk);
+ mutex_unlock(&sfc->lock);
+}
+
+static int rockchip_sfc_wait_op_finish(struct rockchip_sfc *sfc)
+{
+ unsigned long timeout = jiffies + 2 * HZ;
+ int err = -ETIMEDOUT;
+ u32 status;
+
+ /*
+ * Note: tx and rx share the same fifo, so the rx's water level
+ * is the same as rx's, which means this function could be reused
+ * for checking the read operations as well.
+ */
+ while (time_before(jiffies, timeout)) {
+ status = readl_relaxed(sfc->regbase + SFC_FSR);
+ if (((status >> SFC_FSR_TX_EMPTY_SHIFT) &
+ SFC_FSR_TX_EMPTY_MASK) == SFC_FSR_TX_IS_EMPTY) {
+ err = 0;
+ break;
+ }
+ msleep(20);
+ }
+
+ if (err)
+ dev_err(sfc->dev, "SFC tx never empty\n");
+
+ return err;
+}
+
+static int rockchip_sfc_op_reg(struct spi_nor *nor,
+ u8 opcode, int len, u8 optype)
+{
+ struct rockchip_sfc_priv *priv = nor->priv;
+ struct rockchip_sfc *sfc = priv->sfc;
+ u32 reg;
+
+ if (((readl_relaxed(sfc->regbase + SFC_FSR) >> SFC_FSR_TX_EMPTY_SHIFT) &
+ SFC_FSR_TX_EMPTY_MASK) != SFC_FSR_TX_IS_EMPTY ||
+ ((readl_relaxed(sfc->regbase + SFC_FSR) >>
+ SFC_FSR_RX_EMPTY_SHIFT) &
+ SFC_FSR_RX_EMPTY_MASK) != SFC_FSR_RX_IS_EMPTY ||
+ (readl_relaxed(sfc->regbase + SFC_SR) == SFC_SR_IS_BUSY))
+ rockchip_sfc_reset(sfc);
+
+ reg = (opcode & SFC_CMD_IDX_MASK) << SFC_CMD_IDX_SHIFT;
+ reg |= (len & SFC_CMD_TRAN_BYTES_MASK) << SFC_CMD_TRAN_BYTES_SHIFT;
+ reg |= (priv->cs & SFC_CMD_CS_MASK) << SFC_CMD_CS_SHIFT;
+ reg |= (optype & SFC_CMD_DIR_MASK) << SFC_CMD_DIR_SHIFT;
+
+ writel_relaxed(reg, sfc->regbase + SFC_CMD);
+
+ return rockchip_sfc_wait_op_finish(sfc);
+}
+
+static int rockchip_sfc_read_reg(struct spi_nor *nor, u8 opcode,
+ u8 *buf, int len)
+{
+ struct rockchip_sfc_priv *priv = nor->priv;
+ struct rockchip_sfc *sfc = priv->sfc;
+ int ret;
+ u32 tmp;
+ u32 i;
+
+ ret = rockchip_sfc_op_reg(nor, opcode, len, SFC_CMD_DIR_RD);
+ if (ret)
+ return ret;
+
+ while (len > 0) {
+ tmp = readl_relaxed(sfc->regbase + SFC_DATA);
+ for (i = 0; i < len; i++)
+ *buf++ = (u8)((tmp >> (i * 8)) & 0xff);
+ len = len - 4;
+ }
+
+ return 0;
+}
+
+static int rockchip_sfc_write_reg(struct spi_nor *nor, u8 opcode,
+ u8 *buf, int len)
+{
+ struct rockchip_sfc_priv *priv = nor->priv;
+ struct rockchip_sfc *sfc = priv->sfc;
+ u32 words, i;
+
+ /* Align bytes to words */
+ words = (len + 3) >> 2;
+
+ for (i = 0; i < words; i++)
+ writel_relaxed(*(buf + 4 * i), sfc->regbase + SFC_DATA);
+
+ return rockchip_sfc_op_reg(nor, opcode, len, SFC_CMD_DIR_WR);
+}
+
+static int rockchip_sfc_dma_transfer(struct spi_nor *nor, loff_t from_to,
+ dma_addr_t dma_buf, size_t len, u8 op_type)
+{
+ struct rockchip_sfc_priv *priv = nor->priv;
+ struct rockchip_sfc *sfc = priv->sfc;
+ u32 reg;
+ u8 if_type = 0;
+
+ init_completion(&sfc->cp);
+
+ writel_relaxed(SFC_ICLR_RX_FULL | SFC_ICLR_RX_UFLOW |
+ SFC_ICLR_TX_OFLOW | SFC_ICLR_TX_EMPTY |
+ SFC_ICLR_TRAN_FINISH | SFC_ICLR_BUS_ERR |
+ SFC_ICLR_NSPI_ERR | SFC_ICLR_DMA,
+ sfc->regbase + SFC_ICLR);
+
+ /* Enable transfer complete interrupt */
+ reg = readl_relaxed(sfc->regbase + SFC_IMR);
+ reg &= ~SFC_IMR_TRAN_FINISH;
+ writel_relaxed(reg, sfc->regbase + SFC_IMR);
+
+ if (op_type == SFC_CMD_DIR_WR)
+ reg = (SFC_CMD_DIR_WR << SFC_CMD_DIR_SHIFT) |
+ ((nor->program_opcode & SFC_CMD_IDX_MASK) <<
+ SFC_CMD_IDX_SHIFT);
+ else
+ reg = (SFC_CMD_DIR_RD << SFC_CMD_DIR_SHIFT) |
+ ((nor->read_opcode & SFC_CMD_IDX_MASK) <<
+ SFC_CMD_IDX_SHIFT);
+
+ reg |= ((nor->addr_width == 4) ? SFC_CMD_ADDR_32BITS
+ : SFC_CMD_ADDR_24BITS) << SFC_CMD_ADDR_SHIFT;
+
+ if_type = get_if_type(nor->flash_read);
+ writel_relaxed(if_type << SFC_CTRL_DATA_BITS_SHIFT |
+ if_type << SFC_CTRL_ADDR_BITS_SHIFT |
+ if_type << SFC_CTRL_CMD_BITS_SHIFT |
+ sfc->negative_edge ?
+ SFC_CTRL_PHASE_SEL_NEGETIVE << SFC_CTRL_PHASE_SEL_SHIFT :
+ SFC_CTRL_PHASE_SEL_POSITIVE << SFC_CTRL_PHASE_SEL_SHIFT,
+ sfc->regbase + SFC_CTRL);
+
+ reg |= (priv->cs & SFC_CMD_CS_MASK) << SFC_CMD_CS_SHIFT;
+ reg |= (len & SFC_CMD_TRAN_BYTES_MASK) << SFC_CMD_TRAN_BYTES_SHIFT;
+
+ if (op_type == SFC_CMD_DIR_RD)
+ reg |= (nor->read_dummy & SFC_CMD_DUMMY_MASK) <<
+ SFC_CMD_DUMMY_SHIFT;
+
+ /* Should minus one as 0x0 means 1 bit flash address */
+ writel_relaxed(nor->addr_width * 8 - 1, sfc->regbase + SFC_ABIT);
+ writel_relaxed(reg, sfc->regbase + SFC_CMD);
+ writel_relaxed(from_to, sfc->regbase + SFC_ADDR);
+ writel_relaxed(dma_buf, sfc->regbase + SFC_DMA_ADDR);
+
+ /* Start dma */
+ writel_relaxed(0x1, sfc->regbase + SFC_DMA_TRIGGER);
+
+ /* Wait for the interrupt. */
+ if (!wait_for_completion_timeout(&sfc->cp, msecs_to_jiffies(2000))) {
+ dev_err(sfc->dev, "DMA wait for transfer finish timeout.");
+ return -ETIMEDOUT;
+ }
+
+ /* Disable transfer finish interrupt */
+ reg = readl_relaxed(sfc->regbase + SFC_IMR);
+ reg |= SFC_IMR_TRAN_FINISH;
+ writel_relaxed(reg, sfc->regbase + SFC_IMR);
+
+ return rockchip_sfc_wait_op_finish(sfc);
+}
+
+static inline int rockchip_sfc_pio_write(struct rockchip_sfc *sfc, u_char *buf,
+ size_t len)
+{
+ u32 words, tx_wl, count, i;
+ unsigned long timeout;
+ int ret = 0;
+ u32 *tbuf = (u32 *)buf;
+
+ /* Align bytes to words */
+ words = (len + 3) >> 2;
+
+ while (words) {
+ tx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
+ SFC_FSR_TX_WATER_LVL_SHIFT) &
+ SFC_FSR_TX_WATER_LVL_MASK;
+
+ if (tx_wl > 0) {
+ count = min_t(u32, words, tx_wl);
+ for (i = 0; i < count; i++) {
+ writel_relaxed(*tbuf++,
+ sfc->regbase + SFC_DATA);
+ words--;
+ }
+
+ if (words == 0)
+ break;
+ timeout = 0;
+ } else {
+ mdelay(1);
+ if (timeout++ > SFC_MAX_IDLE_RETRY) {
+ ret = -ETIMEDOUT;
+ break;
+ }
+ }
+ }
+
+ if (ret)
+ return ret;
+ else
+ return rockchip_sfc_wait_op_finish(sfc);
+}
+
+static inline int rockchip_sfc_pio_read(struct rockchip_sfc *sfc, u_char *buf,
+ size_t len)
+{
+ u32 words, rx_wl, count, i;
+ unsigned long timeout;
+ int ret = 0;
+ u32 tmp;
+ u32 *tbuf = (u32 *)buf;
+ u_char *tbuf2;
+
+ words = len >> 2;
+ /* Get the remained bytes */
+ len = len & 0x3;
+
+ while (words) {
+ rx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
+ SFC_FSR_RX_WATER_LVL_SHIFT) &
+ SFC_FSR_RX_WATER_LVL_MASK;
+
+ if (rx_wl > 0) {
+ count = min_t(u32, words, rx_wl);
+ for (i = 0; i < count; i++) {
+ *tbuf++ = readl_relaxed(sfc->regbase +
+ SFC_DATA);
+ words--;
+ }
+
+ if (words == 0)
+ break;
+ timeout = 0;
+ } else {
+ mdelay(1);
+ if (timeout++ > SFC_MAX_IDLE_RETRY) {
+ ret = -ETIMEDOUT;
+ break;
+ }
+ }
+ }
+
+ if (ret)
+ return ret;
+
+ /* Read the remained bytes */
+ timeout = 0;
+ tbuf2 = (u_char *)tbuf;
+ while (len) {
+ rx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
+ SFC_FSR_RX_WATER_LVL_SHIFT) &
+ SFC_FSR_RX_WATER_LVL_MASK;
+ if (rx_wl > 0) {
+ tmp = readl_relaxed(sfc->regbase + SFC_DATA);
+ for (i = 0; i < len; i++)
+ tbuf2[i] = (u8)((tmp >> (i * 8)) & 0xff);
+ goto done;
+ } else {
+ mdelay(1);
+ if (timeout++ > SFC_MAX_IDLE_RETRY) {
+ ret = -ETIMEDOUT;
+ break;
+ }
+ }
+ }
+done:
+ if (ret)
+ return ret;
+ else
+ return rockchip_sfc_wait_op_finish(sfc);
+}
+
+static int rockchip_sfc_pio_transfer(struct spi_nor *nor, loff_t from_to,
+ size_t len, u_char *buf, u8 op_type)
+{
+ struct rockchip_sfc_priv *priv = nor->priv;
+ struct rockchip_sfc *sfc = priv->sfc;
+ u32 reg;
+ u8 if_type = 0;
+
+ if (op_type == SFC_CMD_DIR_WR)
+ reg = (SFC_CMD_DIR_WR << SFC_CMD_DIR_SHIFT) |
+ ((nor->program_opcode & SFC_CMD_IDX_MASK) <<
+ SFC_CMD_IDX_SHIFT);
+ else
+ reg = (SFC_CMD_DIR_RD << SFC_CMD_DIR_SHIFT) |
+ ((nor->read_opcode & SFC_CMD_IDX_MASK) <<
+ SFC_CMD_IDX_SHIFT);
+
+ reg |= ((nor->addr_width == 4) ? SFC_CMD_ADDR_32BITS
+ : SFC_CMD_ADDR_24BITS) << SFC_CMD_ADDR_SHIFT;
+
+ if_type = get_if_type(nor->flash_read);
+ writel_relaxed(if_type << SFC_CTRL_DATA_BITS_SHIFT |
+ if_type << SFC_CTRL_ADDR_BITS_SHIFT |
+ if_type << SFC_CTRL_CMD_BITS_SHIFT |
+ sfc->negative_edge ?
+ SFC_CTRL_PHASE_SEL_NEGETIVE << SFC_CTRL_PHASE_SEL_SHIFT :
+ SFC_CTRL_PHASE_SEL_POSITIVE << SFC_CTRL_PHASE_SEL_SHIFT,
+ sfc->regbase + SFC_CTRL);
+
+ reg |= (priv->cs & SFC_CMD_CS_MASK) << SFC_CMD_CS_SHIFT;
+ reg |= (len & SFC_CMD_TRAN_BYTES_MASK) << SFC_CMD_TRAN_BYTES_SHIFT;
+
+ if (op_type == SFC_CMD_DIR_RD)
+ reg |= (nor->read_dummy & SFC_CMD_DUMMY_MASK) <<
+ SFC_CMD_DUMMY_SHIFT;
+
+ /* Should minus one as 0x0 means 1 bit flash address */
+ writel_relaxed(nor->addr_width * 8 - 1, sfc->regbase + SFC_ABIT);
+ writel_relaxed(reg, sfc->regbase + SFC_CMD);
+ writel_relaxed(from_to, sfc->regbase + SFC_ADDR);
+
+ if (op_type == SFC_CMD_DIR_WR)
+ return rockchip_sfc_pio_write(sfc, buf, len);
+ else
+ return rockchip_sfc_pio_read(sfc, buf, len);
+}
+
+static ssize_t rockchip_sfc_read(struct spi_nor *nor, loff_t from, size_t len,
+ u_char *read_buf)
+{
+ struct rockchip_sfc_priv *priv = nor->priv;
+ struct rockchip_sfc *sfc = priv->sfc;
+ size_t offset;
+ int ret;
+ dma_addr_t dma_addr = 0;
+
+ if (!sfc->use_dma)
+ goto no_dma;
+
+ for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
+ size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
+
+ dma_addr = dma_map_single(NULL, (void *)read_buf,
+ trans, DMA_FROM_DEVICE);
+ if (dma_mapping_error(sfc->dev, dma_addr))
+ dma_addr = 0;
+
+ /* Fail to map dma, use pre-allocated area instead */
+ ret = rockchip_sfc_dma_transfer(nor, from + offset,
+ dma_addr ? dma_addr :
+ sfc->dma_buffer,
+ trans, SFC_CMD_DIR_RD);
+ if (ret) {
+ dev_warn(nor->dev, "DMA read timeout\n");
+ return ret;
+ }
+ if (!dma_addr)
+ memcpy(read_buf + offset, sfc->buffer, trans);
+ }
+
+ return len;
+
+no_dma:
+ ret = rockchip_sfc_pio_transfer(nor, from, len,
+ read_buf, SFC_CMD_DIR_RD);
+ if (ret) {
+ dev_warn(nor->dev, "PIO read timeout\n");
+ return ret;
+ }
+ return len;
+}
+
+static ssize_t rockchip_sfc_write(struct spi_nor *nor, loff_t to,
+ size_t len, const u_char *write_buf)
+{
+ struct rockchip_sfc_priv *priv = nor->priv;
+ struct rockchip_sfc *sfc = priv->sfc;
+ size_t offset;
+ int ret;
+ dma_addr_t dma_addr = 0;
+
+ if (!sfc->use_dma)
+ goto no_dma;
+
+ for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
+ size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
+
+ dma_addr = dma_map_single(NULL, (void *)write_buf,
+ trans, DMA_TO_DEVICE);
+ if (dma_mapping_error(sfc->dev, dma_addr)) {
+ dma_addr = 0;
+ memcpy(sfc->buffer, write_buf + offset, trans);
+ }
+
+ /* Fail to map dma, use pre-allocated area instead */
+ ret = rockchip_sfc_dma_transfer(nor, to + offset,
+ dma_addr ? dma_addr :
+ sfc->dma_buffer,
+ trans, SFC_CMD_DIR_WR);
+ if (dma_addr)
+ dma_unmap_single(NULL, dma_addr,
+ trans, DMA_TO_DEVICE);
+ if (ret) {
+ dev_warn(nor->dev, "DMA write timeout\n");
+ return ret;
+ }
+ }
+
+ return len;
+no_dma:
+ ret = rockchip_sfc_pio_transfer(nor, to, len,
+ (u_char *)write_buf, SFC_CMD_DIR_WR);
+ if (ret) {
+ dev_warn(nor->dev, "PIO write timeout\n");
+ return ret;
+ }
+ return len;
+}
+
+/**
+ * Get spi flash device information and register it as a mtd device.
+ */
+static int rockchip_sfc_register(struct device_node *np,
+ struct rockchip_sfc *sfc)
+{
+ struct device *dev = sfc->dev;
+ struct spi_nor *nor;
+ struct rockchip_sfc_priv *priv;
+ struct mtd_info *mtd;
+ int ret;
+
+ nor = devm_kzalloc(dev, sizeof(*nor), GFP_KERNEL);
+ if (!nor)
+ return -ENOMEM;
+
+ nor->dev = dev;
+ spi_nor_set_flash_node(nor, np);
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ ret = of_property_read_u32(np, "reg", &priv->cs);
+ if (ret) {
+ dev_err(dev, "No reg property for %s\n",
+ np->full_name);
+ return ret;
+ }
+
+ ret = of_property_read_u32(np, "spi-max-frequency",
+ &priv->clk_rate);
+ if (ret) {
+ dev_err(dev, "No spi-max-frequency property for %s\n",
+ np->full_name);
+ return ret;
+ }
+
+ priv->sfc = sfc;
+ nor->priv = priv;
+
+ nor->prepare = rockchip_sfc_prep;
+ nor->unprepare = rockchip_sfc_unprep;
+ nor->read_reg = rockchip_sfc_read_reg;
+ nor->write_reg = rockchip_sfc_write_reg;
+ nor->read = rockchip_sfc_read;
+ nor->write = rockchip_sfc_write;
+ nor->erase = NULL;
+ ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
+ if (ret)
+ return ret;
+
+ mtd = &nor->mtd;
+ mtd->name = np->name;
+ ret = mtd_device_register(mtd, NULL, 0);
+ if (ret)
+ return ret;
+
+ sfc->nor[sfc->num_chip] = nor;
+ sfc->num_chip++;
+ return 0;
+}
+
+static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
+{
+ int i;
+
+ for (i = 0; i < sfc->num_chip; i++)
+ mtd_device_unregister(&sfc->nor[i]->mtd);
+}
+
+static int rockchip_sfc_register_all(struct rockchip_sfc *sfc)
+{
+ struct device *dev = sfc->dev;
+ struct device_node *np;
+ int ret;
+
+ for_each_available_child_of_node(dev->of_node, np) {
+ ret = rockchip_sfc_register(np, sfc);
+ if (ret)
+ goto fail;
+
+ if (sfc->num_chip == SFC_MAX_CHIP_NUM) {
+ dev_warn(dev, "Exceeds the max cs limitation\n");
+ break;
+ }
+ }
+
+ return 0;
+
+fail:
+ dev_err(dev, "Failed to register all chip\n");
+ rockchip_sfc_unregister_all(sfc);
+ return ret;
+}
+
+static irqreturn_t rockchip_sfc_irq_handler(int irq, void *dev_id)
+{
+ struct rockchip_sfc *sfc = dev_id;
+ u32 reg;
+
+ reg = readl_relaxed(sfc->regbase + SFC_RISR);
+ dev_dbg(sfc->dev, "Get irq: 0x%x\n", reg);
+
+ /* Clear interrupt */
+ writel_relaxed(reg, sfc->regbase + SFC_ICLR);
+
+ if (reg & SFC_IRQ_TRAN_FINISH)
+ complete(&sfc->cp);
+
+ return IRQ_HANDLED;
+}
+
+static int rockchip_sfc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+ struct rockchip_sfc *sfc;
+ int ret;
+
+ sfc = devm_kzalloc(dev, sizeof(*sfc), GFP_KERNEL);
+ if (!sfc)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, sfc);
+ sfc->dev = dev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ sfc->regbase = devm_ioremap_resource(dev, res);
+ if (IS_ERR(sfc->regbase))
+ return PTR_ERR(sfc->regbase);
+
+ sfc->clk = devm_clk_get(&pdev->dev, "sfc");
+ if (IS_ERR(sfc->clk)) {
+ dev_err(&pdev->dev, "Failed to get sfc interface clk\n");
+ return PTR_ERR(sfc->clk);
+ }
+
+ sfc->hclk = devm_clk_get(&pdev->dev, "hsfc");
+ if (IS_ERR(sfc->hclk)) {
+ dev_err(&pdev->dev, "Failed to get sfc ahp clk\n");
+ return PTR_ERR(sfc->hclk);
+ }
+
+ ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
+ if (ret) {
+ dev_warn(dev, "Unable to set dma mask\n");
+ return ret;
+ }
+
+ sfc->buffer = dmam_alloc_coherent(dev, SFC_DMA_MAX_LEN,
+ &sfc->dma_buffer, GFP_KERNEL);
+ if (!sfc->buffer)
+ return -ENOMEM;
+
+ mutex_init(&sfc->lock);
+
+ ret = clk_prepare_enable(sfc->hclk);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to enable hclk\n");
+ goto err_hclk;
+ }
+
+ ret = clk_prepare_enable(sfc->clk);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to enable clk\n");
+ goto err_clk;
+ }
+
+ if (of_property_read_bool(sfc->dev->of_node, "rockchip,sfc-no-dma"))
+ sfc->use_dma = false;
+ else
+ sfc->use_dma = true;
+
+ if (of_device_is_compatible(sfc->dev->of_node,
+ "rockchip,rk1108-sfc"))
+ sfc->negative_edge = true;
+ else
+ sfc->negative_edge = false;
+
+ /* Find the irq */
+ ret = platform_get_irq(pdev, 0);
+ if (ret < 0) {
+ dev_err(dev, "Failed to get the irq\n");
+ goto err_irq;
+ }
+
+ ret = devm_request_irq(dev, ret, rockchip_sfc_irq_handler,
+ 0, pdev->name, sfc);
+ if (ret) {
+ dev_err(dev, "Failed to request irq\n");
+ goto err_irq;
+ }
+
+ ret = rockchip_sfc_init(sfc);
+ if (ret)
+ goto err_init;
+
+ ret = rockchip_sfc_register_all(sfc);
+ if (ret)
+ goto err_init;
+
+ clk_disable_unprepare(sfc->clk);
+ return 0;
+
+err_irq:
+err_init:
+ clk_disable_unprepare(sfc->clk);
+err_clk:
+ clk_disable_unprepare(sfc->hclk);
+err_hclk:
+ mutex_destroy(&sfc->lock);
+ return ret;
+}
+
+static int rockchip_sfc_remove(struct platform_device *pdev)
+{
+ struct rockchip_sfc *sfc = platform_get_drvdata(pdev);
+
+ rockchip_sfc_unregister_all(sfc);
+ mutex_destroy(&sfc->lock);
+ clk_disable_unprepare(sfc->clk);
+ clk_disable_unprepare(sfc->hclk);
+ return 0;
+}
+
+static const struct of_device_id rockchip_sfc_dt_ids[] = {
+ { .compatible = "rockchip,sfc"},
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rockchip_sfc_dt_ids);
+
+static struct platform_driver rockchip_sfc_driver = {
+ .driver = {
+ .name = "rockchip-sfc",
+ .of_match_table = rockchip_sfc_dt_ids,
+ },
+ .probe = rockchip_sfc_probe,
+ .remove = rockchip_sfc_remove,
+};
+module_platform_driver(rockchip_sfc_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Rockchip Serial Flash Controller Driver");
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/2] mtd: spi-nor: add rockchip serial flash controller driver
@ 2016-11-11 9:16 ` Shawn Lin
0 siblings, 0 replies; 20+ messages in thread
From: Shawn Lin @ 2016-11-11 9:16 UTC (permalink / raw)
To: Rob Herring, David Woodhouse, Brian Norris
Cc: devicetree, linux-rockchip, linux-mtd, Heiko Stuebner, Shawn Lin
Add rockchip serial flash controller driver
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
MAINTAINERS | 8 +
drivers/mtd/spi-nor/Kconfig | 7 +
drivers/mtd/spi-nor/Makefile | 1 +
drivers/mtd/spi-nor/rockchip-sfc.c | 953 +++++++++++++++++++++++++++++++++++++
4 files changed, 969 insertions(+)
create mode 100644 drivers/mtd/spi-nor/rockchip-sfc.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 1cd38a7..eb7e06d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10266,6 +10266,14 @@ L: linux-serial@vger.kernel.org
S: Odd Fixes
F: drivers/tty/serial/rp2.*
+ROCKCHIP SERIAL FLASH CONTROLLER DRIVER
+M: Shawn Lin <shawn.lin@rock-chips.com>
+L: linux-mtd@lists.infradead.org
+L: linux-rockchip@lists.infradead.org
+S: Maintained
+F: Documentation/devicetree/bindings/mtd/rockchip-sfc.txt
+F: drivers/mtd/spi-nor/rockchip-sfc.c
+
ROSE NETWORK LAYER
M: Ralf Baechle <ralf@linux-mips.org>
L: linux-hams@vger.kernel.org
diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index 4a682ee..48c5e0e 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -65,6 +65,13 @@ config SPI_HISI_SFC
help
This enables support for hisilicon SPI-NOR flash controller.
+config SPI_ROCKCHIP_SFC
+ tristate "Rockchip Serial Flash Controller(SFC)"
+ depends on ARCH_ROCKCHIP || COMPILE_TEST
+ depends on HAS_IOMEM && HAS_DMA
+ help
+ This enables support for rockchip serial flash controller.
+
config SPI_NXP_SPIFI
tristate "NXP SPI Flash Interface (SPIFI)"
depends on OF && (ARCH_LPC18XX || COMPILE_TEST)
diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
index 121695e..364d4c6 100644
--- a/drivers/mtd/spi-nor/Makefile
+++ b/drivers/mtd/spi-nor/Makefile
@@ -5,3 +5,4 @@ obj-$(CONFIG_SPI_FSL_QUADSPI) += fsl-quadspi.o
obj-$(CONFIG_SPI_HISI_SFC) += hisi-sfc.o
obj-$(CONFIG_MTD_MT81xx_NOR) += mtk-quadspi.o
obj-$(CONFIG_SPI_NXP_SPIFI) += nxp-spifi.o
+obj-$(CONFIG_SPI_ROCKCHIP_SFC) += rockchip-sfc.o
diff --git a/drivers/mtd/spi-nor/rockchip-sfc.c b/drivers/mtd/spi-nor/rockchip-sfc.c
new file mode 100644
index 0000000..83930d6
--- /dev/null
+++ b/drivers/mtd/spi-nor/rockchip-sfc.c
@@ -0,0 +1,953 @@
+/*
+ * Rockchip Serial Flash Controller Driver
+ *
+ * Copyright (c) 2016, Rockchip Inc.
+ * Author: Shawn Lin <shawn.lin@rock-chips.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * 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/bitops.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/dma-mapping.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/spi-nor.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/* System control */
+#define SFC_CTRL 0x0
+#define SFC_CTRL_COMMON_BITS_1 0x0
+#define SFC_CTRL_COMMON_BITS_2 0x1
+#define SFC_CTRL_COMMON_BITS_4 0x2
+#define SFC_CTRL_DATA_BITS_SHIFT 12
+#define SFC_CTRL_DATA_BITS_MASK 0x3
+#define SFC_CTRL_ADDR_BITS_SHIFT 10
+#define SFC_CTRL_ADDR_BITS_MASK 0x3
+#define SFC_CTRL_CMD_BITS_SHIFT 8
+#define SFC_CTRL_CMD_BITS_MASK 0x3
+#define SFC_CTRL_IDLE_CYC_SHIFT 4
+#define SFC_CTRL_IDLE_CYC_MASK 0xf
+#define SFC_CTRL_PHASE_SEL_SHIFT 1
+#define SFC_CTRL_PHASE_SEL_NEGETIVE 0x1
+#define SFC_CTRL_PHASE_SEL_POSITIVE 0x0
+#define SFC_CTRL_MODE_SEL_SHIFT 0
+#define SFC_CTRL_MODE_MASK 0x1
+
+/* Interrypt mask */
+#define SFC_IMR 0x4
+#define SFC_IMR_RX_FULL BIT(0)
+#define SFC_IMR_RX_UFLOW BIT(1)
+#define SFC_IMR_TX_OFLOW BIT(2)
+#define SFC_IMR_TX_EMPTY BIT(3)
+#define SFC_IMR_TRAN_FINISH BIT(4)
+#define SFC_IMR_BUS_ERR BIT(5)
+#define SFC_IMR_NSPI_ERR BIT(6)
+#define SFC_IMR_DMA BIT(7)
+/* Interrupt clear */
+#define SFC_ICLR 0x8
+#define SFC_ICLR_RX_FULL BIT(0)
+#define SFC_ICLR_RX_UFLOW BIT(1)
+#define SFC_ICLR_TX_OFLOW BIT(2)
+#define SFC_ICLR_TX_EMPTY BIT(3)
+#define SFC_ICLR_TRAN_FINISH BIT(4)
+#define SFC_ICLR_BUS_ERR BIT(5)
+#define SFC_ICLR_NSPI_ERR BIT(6)
+#define SFC_ICLR_DMA BIT(7)
+/* FIFO threshold level */
+#define SFC_FTLR 0xc
+#define SFC_FTLR_TX_SHIFT 0
+#define SFC_FTLR_TX_MASK 0x1f
+#define SFC_FTLR_RX_SHIFT 8
+#define SFC_FTLR_RX_MASK 0x1f
+/* Reset FSM and FIFO */
+#define SFC_RCVR 0x10
+#define SFC_RCVR_RESET BIT(0)
+/* Enhanced mode */
+#define SFC_AX 0x14
+#define SFC_AX_MODE_BIT_SHIFT 0
+#define SFC_AX_MODE_BIT_MASK 0xff
+/* Address Bit number */
+#define SFC_ABIT 0x18
+#define SFC_ABIT_ADD_SHIFT 0
+#define SFC_ABIT_ADD_MASK 0x1f
+/* Interrupt status */
+#define SFC_ISR 0x1c
+#define SFC_ISR_COMMON_ACTIVE 0x1
+#define SFC_ISR_COMMON_MASK 0x1
+#define SFC_ISR_RX_FULL_SHIFT 0
+#define SFC_ISR_RX_UFLOW_SHIFT 1
+#define SFC_ISR_TX_OFLOW_SHIFT 2
+#define SFC_ISR_TX_EMPTY_SHIFT 3
+#define SFC_ISR_TX_FINISH_SHIFT 4
+#define SFC_ISR_BUS_ERR_SHIFT 5
+#define SFC_ISR_NSPI_ERR_SHIFT 6
+#define SFC_ISR_DMA_SHIFT 7
+/* FIFO status */
+#define SFC_FSR 0x20
+#define SFC_FSR_TX_IS_FULL 0x1
+#define SFC_FSR_TX_FULL_SHIFT 0
+#define SFC_FSR_TX_FULL_MASK 0x1
+#define SFC_FSR_TX_IS_EMPTY 0x1
+#define SFC_FSR_TX_EMPTY_SHIFT 1
+#define SFC_FSR_TX_EMPTY_MASK 0x1
+#define SFC_FSR_RX_IS_EMPTY 0x1
+#define SFC_FSR_RX_EMPTY_SHIFT 2
+#define SFC_FSR_RX_EMPTY_MASK 0x1
+#define SFC_FSR_RX_IS_FULL 0x1
+#define SFC_FSR_RX_FULL_SHIFT 3
+#define SFC_FSR_RX_FULL_MASK 0x1
+#define SFC_FSR_TX_WATER_LVL_SHIFT 8
+#define SFC_FSR_TX_WATER_LVL_MASK 0x1f
+#define SFC_FSR_RX_WATER_LVL_SHIFT 16
+#define SFC_FSR_RX_WATER_LVL_MASK 0x1f
+/* FSM status */
+#define SFC_SR 0x24
+#define SFC_SR_IS_IDLE 0x0
+#define SFC_SR_IS_BUSY 0x1
+/* Raw interrupt status */
+#define SFC_RISR 0x28
+#define SFC_RISR_COMMON_ACTIVE 0x1
+#define SFC_RISR_COMMON_MASK 0x1
+#define SFC_RISR_RX_FULL_SHIFT 0
+#define SFC_RISR_RX_UFLOW_SHIFT 1
+#define SFC_RISR_TX_OFLOW_SHIFT 2
+#define SFC_RISR_TX_EMPTY_SHIFT 3
+#define SFC_RISR_TRAN_FINISH_SHIFT 4
+#define SFC_RISR_BUS_ERR_SHIFT 5
+#define SFC_RISR_NSPI_ERR_SHIFT 6
+#define SFC_RISR_DMA_SHIFT 7
+/* Master trigger */
+#define SFC_DMA_TRIGGER 0x80
+/* Src or Dst addr for master */
+#define SFC_DMA_ADDR 0x84
+/* Command */
+#define SFC_CMD 0x100
+#define SFC_CMD_IDX_SHIFT 0
+#define SFC_CMD_IDX_MASK 0xff
+#define SFC_CMD_DUMMY_SHIFT 8
+#define SFC_CMD_DUMMY_MASK 0xf
+#define SFC_CMD_DIR_RD 0
+#define SFC_CMD_DIR_WR 1
+#define SFC_CMD_DIR_SHIFT 12
+#define SFC_CMD_DIR_MASK 0x1
+#define SFC_CMD_CONTI_RD 1
+#define SFC_CMD_CONTI_RD_SHIFT 13
+#define SFC_CMD_ADDR_ZERO 0
+#define SFC_CMD_ADDR_24BITS 0x1
+#define SFC_CMD_ADDR_32BITS 0x2
+#define SFC_CMD_ADDR_FRS 0x3
+#define SFC_CMD_ADDR_SHIFT 14
+#define SFC_CMD_TRAN_BYTES_SHIFT 16
+#define SFC_CMD_TRAN_BYTES_MASK 0x3fff
+#define SFC_CMD_CS_SHIFT 30
+#define SFC_CMD_CS_MASK 0x3
+/* Address */
+#define SFC_ADDR 0x104
+/* Data */
+#define SFC_DATA 0x108
+
+#define SFC_MAX_CHIP_NUM 4
+#define SFC_MAX_IDLE_RETRY 10000
+#define SFC_WAIT_IDLE_TIMEOUT 1000000
+#define SFC_DMA_MAX_LEN 0x4000
+#define SFC_DMA_MAX_MASK (SFC_DMA_MAX_LEN - 1)
+#define SFC_IRQ_TRAN_FINISH BIT(SFC_RISR_TRAN_FINISH_SHIFT)
+
+enum rockchip_sfc_iftype {
+ IF_TYPE_STD,
+ IF_TYPE_DUAL,
+ IF_TYPE_QUAD,
+};
+
+struct rockchip_sfc {
+ struct device *dev;
+ struct mutex lock;
+ void __iomem *regbase;
+ struct clk *hclk;
+ struct clk *clk;
+ void *buffer;
+ dma_addr_t dma_buffer;
+ struct completion cp;
+ struct spi_nor *nor[SFC_MAX_CHIP_NUM];
+ u32 num_chip;
+ bool use_dma;
+ bool negative_edge;
+};
+
+struct rockchip_sfc_priv {
+ u32 cs;
+ u32 clk_rate;
+ struct rockchip_sfc *sfc;
+};
+
+static int get_if_type(enum read_mode flash_read)
+{
+ enum rockchip_sfc_iftype if_type;
+
+ switch (flash_read) {
+ case SPI_NOR_DUAL:
+ if_type = IF_TYPE_DUAL;
+ break;
+ case SPI_NOR_QUAD:
+ if_type = IF_TYPE_QUAD;
+ break;
+ case SPI_NOR_NORMAL:
+ case SPI_NOR_FAST:
+ default:
+ if_type = IF_TYPE_STD;
+ break;
+ }
+
+ return if_type;
+}
+
+static int rockchip_sfc_reset(struct rockchip_sfc *sfc)
+{
+ unsigned long timeout = jiffies + HZ;
+ int err = -ETIMEDOUT;
+ u32 status;
+
+ writel_relaxed(SFC_RCVR_RESET, sfc->regbase + SFC_RCVR);
+
+ while (time_before(jiffies, timeout)) {
+ status = readl_relaxed(sfc->regbase + SFC_RCVR);
+ if (!(status & SFC_RCVR_RESET)) {
+ err = 0;
+ break;
+ }
+ msleep(20);
+ }
+
+ if (err)
+ dev_err(sfc->dev, "SFC reset never finished\n");
+
+ writel_relaxed(SFC_ICLR_RX_FULL | SFC_ICLR_RX_UFLOW |
+ SFC_ICLR_TX_OFLOW | SFC_ICLR_TX_EMPTY |
+ SFC_ICLR_TRAN_FINISH | SFC_ICLR_BUS_ERR |
+ SFC_ICLR_NSPI_ERR | SFC_ICLR_DMA,
+ sfc->regbase + SFC_ICLR);
+ return err;
+}
+
+static int rockchip_sfc_init(struct rockchip_sfc *sfc)
+{
+ int err;
+
+ err = rockchip_sfc_reset(sfc);
+ if (err)
+ return err;
+
+ /* Mask all eight interrupts */
+ writel_relaxed(0xff, sfc->regbase + SFC_IMR);
+ /* Phase configure */
+ if (sfc->negative_edge)
+ writel_relaxed(SFC_CTRL_PHASE_SEL_NEGETIVE <<
+ SFC_CTRL_PHASE_SEL_SHIFT,
+ sfc->regbase + SFC_CTRL);
+ return 0;
+}
+
+static int rockchip_sfc_prep(struct spi_nor *nor, enum spi_nor_ops ops)
+{
+ struct rockchip_sfc_priv *priv = nor->priv;
+ struct rockchip_sfc *sfc = priv->sfc;
+ int ret;
+
+ mutex_lock(&sfc->lock);
+
+ ret = clk_set_rate(sfc->clk, priv->clk_rate);
+ if (ret)
+ goto out;
+
+ ret = clk_prepare_enable(sfc->clk);
+ if (ret)
+ goto out;
+
+ return 0;
+
+out:
+ mutex_unlock(&sfc->lock);
+ return ret;
+}
+
+static void rockchip_sfc_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
+{
+ struct rockchip_sfc_priv *priv = nor->priv;
+ struct rockchip_sfc *sfc = priv->sfc;
+
+ clk_disable_unprepare(sfc->clk);
+ mutex_unlock(&sfc->lock);
+}
+
+static int rockchip_sfc_wait_op_finish(struct rockchip_sfc *sfc)
+{
+ unsigned long timeout = jiffies + 2 * HZ;
+ int err = -ETIMEDOUT;
+ u32 status;
+
+ /*
+ * Note: tx and rx share the same fifo, so the rx's water level
+ * is the same as rx's, which means this function could be reused
+ * for checking the read operations as well.
+ */
+ while (time_before(jiffies, timeout)) {
+ status = readl_relaxed(sfc->regbase + SFC_FSR);
+ if (((status >> SFC_FSR_TX_EMPTY_SHIFT) &
+ SFC_FSR_TX_EMPTY_MASK) == SFC_FSR_TX_IS_EMPTY) {
+ err = 0;
+ break;
+ }
+ msleep(20);
+ }
+
+ if (err)
+ dev_err(sfc->dev, "SFC tx never empty\n");
+
+ return err;
+}
+
+static int rockchip_sfc_op_reg(struct spi_nor *nor,
+ u8 opcode, int len, u8 optype)
+{
+ struct rockchip_sfc_priv *priv = nor->priv;
+ struct rockchip_sfc *sfc = priv->sfc;
+ u32 reg;
+
+ if (((readl_relaxed(sfc->regbase + SFC_FSR) >> SFC_FSR_TX_EMPTY_SHIFT) &
+ SFC_FSR_TX_EMPTY_MASK) != SFC_FSR_TX_IS_EMPTY ||
+ ((readl_relaxed(sfc->regbase + SFC_FSR) >>
+ SFC_FSR_RX_EMPTY_SHIFT) &
+ SFC_FSR_RX_EMPTY_MASK) != SFC_FSR_RX_IS_EMPTY ||
+ (readl_relaxed(sfc->regbase + SFC_SR) == SFC_SR_IS_BUSY))
+ rockchip_sfc_reset(sfc);
+
+ reg = (opcode & SFC_CMD_IDX_MASK) << SFC_CMD_IDX_SHIFT;
+ reg |= (len & SFC_CMD_TRAN_BYTES_MASK) << SFC_CMD_TRAN_BYTES_SHIFT;
+ reg |= (priv->cs & SFC_CMD_CS_MASK) << SFC_CMD_CS_SHIFT;
+ reg |= (optype & SFC_CMD_DIR_MASK) << SFC_CMD_DIR_SHIFT;
+
+ writel_relaxed(reg, sfc->regbase + SFC_CMD);
+
+ return rockchip_sfc_wait_op_finish(sfc);
+}
+
+static int rockchip_sfc_read_reg(struct spi_nor *nor, u8 opcode,
+ u8 *buf, int len)
+{
+ struct rockchip_sfc_priv *priv = nor->priv;
+ struct rockchip_sfc *sfc = priv->sfc;
+ int ret;
+ u32 tmp;
+ u32 i;
+
+ ret = rockchip_sfc_op_reg(nor, opcode, len, SFC_CMD_DIR_RD);
+ if (ret)
+ return ret;
+
+ while (len > 0) {
+ tmp = readl_relaxed(sfc->regbase + SFC_DATA);
+ for (i = 0; i < len; i++)
+ *buf++ = (u8)((tmp >> (i * 8)) & 0xff);
+ len = len - 4;
+ }
+
+ return 0;
+}
+
+static int rockchip_sfc_write_reg(struct spi_nor *nor, u8 opcode,
+ u8 *buf, int len)
+{
+ struct rockchip_sfc_priv *priv = nor->priv;
+ struct rockchip_sfc *sfc = priv->sfc;
+ u32 words, i;
+
+ /* Align bytes to words */
+ words = (len + 3) >> 2;
+
+ for (i = 0; i < words; i++)
+ writel_relaxed(*(buf + 4 * i), sfc->regbase + SFC_DATA);
+
+ return rockchip_sfc_op_reg(nor, opcode, len, SFC_CMD_DIR_WR);
+}
+
+static int rockchip_sfc_dma_transfer(struct spi_nor *nor, loff_t from_to,
+ dma_addr_t dma_buf, size_t len, u8 op_type)
+{
+ struct rockchip_sfc_priv *priv = nor->priv;
+ struct rockchip_sfc *sfc = priv->sfc;
+ u32 reg;
+ u8 if_type = 0;
+
+ init_completion(&sfc->cp);
+
+ writel_relaxed(SFC_ICLR_RX_FULL | SFC_ICLR_RX_UFLOW |
+ SFC_ICLR_TX_OFLOW | SFC_ICLR_TX_EMPTY |
+ SFC_ICLR_TRAN_FINISH | SFC_ICLR_BUS_ERR |
+ SFC_ICLR_NSPI_ERR | SFC_ICLR_DMA,
+ sfc->regbase + SFC_ICLR);
+
+ /* Enable transfer complete interrupt */
+ reg = readl_relaxed(sfc->regbase + SFC_IMR);
+ reg &= ~SFC_IMR_TRAN_FINISH;
+ writel_relaxed(reg, sfc->regbase + SFC_IMR);
+
+ if (op_type == SFC_CMD_DIR_WR)
+ reg = (SFC_CMD_DIR_WR << SFC_CMD_DIR_SHIFT) |
+ ((nor->program_opcode & SFC_CMD_IDX_MASK) <<
+ SFC_CMD_IDX_SHIFT);
+ else
+ reg = (SFC_CMD_DIR_RD << SFC_CMD_DIR_SHIFT) |
+ ((nor->read_opcode & SFC_CMD_IDX_MASK) <<
+ SFC_CMD_IDX_SHIFT);
+
+ reg |= ((nor->addr_width == 4) ? SFC_CMD_ADDR_32BITS
+ : SFC_CMD_ADDR_24BITS) << SFC_CMD_ADDR_SHIFT;
+
+ if_type = get_if_type(nor->flash_read);
+ writel_relaxed(if_type << SFC_CTRL_DATA_BITS_SHIFT |
+ if_type << SFC_CTRL_ADDR_BITS_SHIFT |
+ if_type << SFC_CTRL_CMD_BITS_SHIFT |
+ sfc->negative_edge ?
+ SFC_CTRL_PHASE_SEL_NEGETIVE << SFC_CTRL_PHASE_SEL_SHIFT :
+ SFC_CTRL_PHASE_SEL_POSITIVE << SFC_CTRL_PHASE_SEL_SHIFT,
+ sfc->regbase + SFC_CTRL);
+
+ reg |= (priv->cs & SFC_CMD_CS_MASK) << SFC_CMD_CS_SHIFT;
+ reg |= (len & SFC_CMD_TRAN_BYTES_MASK) << SFC_CMD_TRAN_BYTES_SHIFT;
+
+ if (op_type == SFC_CMD_DIR_RD)
+ reg |= (nor->read_dummy & SFC_CMD_DUMMY_MASK) <<
+ SFC_CMD_DUMMY_SHIFT;
+
+ /* Should minus one as 0x0 means 1 bit flash address */
+ writel_relaxed(nor->addr_width * 8 - 1, sfc->regbase + SFC_ABIT);
+ writel_relaxed(reg, sfc->regbase + SFC_CMD);
+ writel_relaxed(from_to, sfc->regbase + SFC_ADDR);
+ writel_relaxed(dma_buf, sfc->regbase + SFC_DMA_ADDR);
+
+ /* Start dma */
+ writel_relaxed(0x1, sfc->regbase + SFC_DMA_TRIGGER);
+
+ /* Wait for the interrupt. */
+ if (!wait_for_completion_timeout(&sfc->cp, msecs_to_jiffies(2000))) {
+ dev_err(sfc->dev, "DMA wait for transfer finish timeout.");
+ return -ETIMEDOUT;
+ }
+
+ /* Disable transfer finish interrupt */
+ reg = readl_relaxed(sfc->regbase + SFC_IMR);
+ reg |= SFC_IMR_TRAN_FINISH;
+ writel_relaxed(reg, sfc->regbase + SFC_IMR);
+
+ return rockchip_sfc_wait_op_finish(sfc);
+}
+
+static inline int rockchip_sfc_pio_write(struct rockchip_sfc *sfc, u_char *buf,
+ size_t len)
+{
+ u32 words, tx_wl, count, i;
+ unsigned long timeout;
+ int ret = 0;
+ u32 *tbuf = (u32 *)buf;
+
+ /* Align bytes to words */
+ words = (len + 3) >> 2;
+
+ while (words) {
+ tx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
+ SFC_FSR_TX_WATER_LVL_SHIFT) &
+ SFC_FSR_TX_WATER_LVL_MASK;
+
+ if (tx_wl > 0) {
+ count = min_t(u32, words, tx_wl);
+ for (i = 0; i < count; i++) {
+ writel_relaxed(*tbuf++,
+ sfc->regbase + SFC_DATA);
+ words--;
+ }
+
+ if (words == 0)
+ break;
+ timeout = 0;
+ } else {
+ mdelay(1);
+ if (timeout++ > SFC_MAX_IDLE_RETRY) {
+ ret = -ETIMEDOUT;
+ break;
+ }
+ }
+ }
+
+ if (ret)
+ return ret;
+ else
+ return rockchip_sfc_wait_op_finish(sfc);
+}
+
+static inline int rockchip_sfc_pio_read(struct rockchip_sfc *sfc, u_char *buf,
+ size_t len)
+{
+ u32 words, rx_wl, count, i;
+ unsigned long timeout;
+ int ret = 0;
+ u32 tmp;
+ u32 *tbuf = (u32 *)buf;
+ u_char *tbuf2;
+
+ words = len >> 2;
+ /* Get the remained bytes */
+ len = len & 0x3;
+
+ while (words) {
+ rx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
+ SFC_FSR_RX_WATER_LVL_SHIFT) &
+ SFC_FSR_RX_WATER_LVL_MASK;
+
+ if (rx_wl > 0) {
+ count = min_t(u32, words, rx_wl);
+ for (i = 0; i < count; i++) {
+ *tbuf++ = readl_relaxed(sfc->regbase +
+ SFC_DATA);
+ words--;
+ }
+
+ if (words == 0)
+ break;
+ timeout = 0;
+ } else {
+ mdelay(1);
+ if (timeout++ > SFC_MAX_IDLE_RETRY) {
+ ret = -ETIMEDOUT;
+ break;
+ }
+ }
+ }
+
+ if (ret)
+ return ret;
+
+ /* Read the remained bytes */
+ timeout = 0;
+ tbuf2 = (u_char *)tbuf;
+ while (len) {
+ rx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
+ SFC_FSR_RX_WATER_LVL_SHIFT) &
+ SFC_FSR_RX_WATER_LVL_MASK;
+ if (rx_wl > 0) {
+ tmp = readl_relaxed(sfc->regbase + SFC_DATA);
+ for (i = 0; i < len; i++)
+ tbuf2[i] = (u8)((tmp >> (i * 8)) & 0xff);
+ goto done;
+ } else {
+ mdelay(1);
+ if (timeout++ > SFC_MAX_IDLE_RETRY) {
+ ret = -ETIMEDOUT;
+ break;
+ }
+ }
+ }
+done:
+ if (ret)
+ return ret;
+ else
+ return rockchip_sfc_wait_op_finish(sfc);
+}
+
+static int rockchip_sfc_pio_transfer(struct spi_nor *nor, loff_t from_to,
+ size_t len, u_char *buf, u8 op_type)
+{
+ struct rockchip_sfc_priv *priv = nor->priv;
+ struct rockchip_sfc *sfc = priv->sfc;
+ u32 reg;
+ u8 if_type = 0;
+
+ if (op_type == SFC_CMD_DIR_WR)
+ reg = (SFC_CMD_DIR_WR << SFC_CMD_DIR_SHIFT) |
+ ((nor->program_opcode & SFC_CMD_IDX_MASK) <<
+ SFC_CMD_IDX_SHIFT);
+ else
+ reg = (SFC_CMD_DIR_RD << SFC_CMD_DIR_SHIFT) |
+ ((nor->read_opcode & SFC_CMD_IDX_MASK) <<
+ SFC_CMD_IDX_SHIFT);
+
+ reg |= ((nor->addr_width == 4) ? SFC_CMD_ADDR_32BITS
+ : SFC_CMD_ADDR_24BITS) << SFC_CMD_ADDR_SHIFT;
+
+ if_type = get_if_type(nor->flash_read);
+ writel_relaxed(if_type << SFC_CTRL_DATA_BITS_SHIFT |
+ if_type << SFC_CTRL_ADDR_BITS_SHIFT |
+ if_type << SFC_CTRL_CMD_BITS_SHIFT |
+ sfc->negative_edge ?
+ SFC_CTRL_PHASE_SEL_NEGETIVE << SFC_CTRL_PHASE_SEL_SHIFT :
+ SFC_CTRL_PHASE_SEL_POSITIVE << SFC_CTRL_PHASE_SEL_SHIFT,
+ sfc->regbase + SFC_CTRL);
+
+ reg |= (priv->cs & SFC_CMD_CS_MASK) << SFC_CMD_CS_SHIFT;
+ reg |= (len & SFC_CMD_TRAN_BYTES_MASK) << SFC_CMD_TRAN_BYTES_SHIFT;
+
+ if (op_type == SFC_CMD_DIR_RD)
+ reg |= (nor->read_dummy & SFC_CMD_DUMMY_MASK) <<
+ SFC_CMD_DUMMY_SHIFT;
+
+ /* Should minus one as 0x0 means 1 bit flash address */
+ writel_relaxed(nor->addr_width * 8 - 1, sfc->regbase + SFC_ABIT);
+ writel_relaxed(reg, sfc->regbase + SFC_CMD);
+ writel_relaxed(from_to, sfc->regbase + SFC_ADDR);
+
+ if (op_type == SFC_CMD_DIR_WR)
+ return rockchip_sfc_pio_write(sfc, buf, len);
+ else
+ return rockchip_sfc_pio_read(sfc, buf, len);
+}
+
+static ssize_t rockchip_sfc_read(struct spi_nor *nor, loff_t from, size_t len,
+ u_char *read_buf)
+{
+ struct rockchip_sfc_priv *priv = nor->priv;
+ struct rockchip_sfc *sfc = priv->sfc;
+ size_t offset;
+ int ret;
+ dma_addr_t dma_addr = 0;
+
+ if (!sfc->use_dma)
+ goto no_dma;
+
+ for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
+ size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
+
+ dma_addr = dma_map_single(NULL, (void *)read_buf,
+ trans, DMA_FROM_DEVICE);
+ if (dma_mapping_error(sfc->dev, dma_addr))
+ dma_addr = 0;
+
+ /* Fail to map dma, use pre-allocated area instead */
+ ret = rockchip_sfc_dma_transfer(nor, from + offset,
+ dma_addr ? dma_addr :
+ sfc->dma_buffer,
+ trans, SFC_CMD_DIR_RD);
+ if (ret) {
+ dev_warn(nor->dev, "DMA read timeout\n");
+ return ret;
+ }
+ if (!dma_addr)
+ memcpy(read_buf + offset, sfc->buffer, trans);
+ }
+
+ return len;
+
+no_dma:
+ ret = rockchip_sfc_pio_transfer(nor, from, len,
+ read_buf, SFC_CMD_DIR_RD);
+ if (ret) {
+ dev_warn(nor->dev, "PIO read timeout\n");
+ return ret;
+ }
+ return len;
+}
+
+static ssize_t rockchip_sfc_write(struct spi_nor *nor, loff_t to,
+ size_t len, const u_char *write_buf)
+{
+ struct rockchip_sfc_priv *priv = nor->priv;
+ struct rockchip_sfc *sfc = priv->sfc;
+ size_t offset;
+ int ret;
+ dma_addr_t dma_addr = 0;
+
+ if (!sfc->use_dma)
+ goto no_dma;
+
+ for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
+ size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
+
+ dma_addr = dma_map_single(NULL, (void *)write_buf,
+ trans, DMA_TO_DEVICE);
+ if (dma_mapping_error(sfc->dev, dma_addr)) {
+ dma_addr = 0;
+ memcpy(sfc->buffer, write_buf + offset, trans);
+ }
+
+ /* Fail to map dma, use pre-allocated area instead */
+ ret = rockchip_sfc_dma_transfer(nor, to + offset,
+ dma_addr ? dma_addr :
+ sfc->dma_buffer,
+ trans, SFC_CMD_DIR_WR);
+ if (dma_addr)
+ dma_unmap_single(NULL, dma_addr,
+ trans, DMA_TO_DEVICE);
+ if (ret) {
+ dev_warn(nor->dev, "DMA write timeout\n");
+ return ret;
+ }
+ }
+
+ return len;
+no_dma:
+ ret = rockchip_sfc_pio_transfer(nor, to, len,
+ (u_char *)write_buf, SFC_CMD_DIR_WR);
+ if (ret) {
+ dev_warn(nor->dev, "PIO write timeout\n");
+ return ret;
+ }
+ return len;
+}
+
+/**
+ * Get spi flash device information and register it as a mtd device.
+ */
+static int rockchip_sfc_register(struct device_node *np,
+ struct rockchip_sfc *sfc)
+{
+ struct device *dev = sfc->dev;
+ struct spi_nor *nor;
+ struct rockchip_sfc_priv *priv;
+ struct mtd_info *mtd;
+ int ret;
+
+ nor = devm_kzalloc(dev, sizeof(*nor), GFP_KERNEL);
+ if (!nor)
+ return -ENOMEM;
+
+ nor->dev = dev;
+ spi_nor_set_flash_node(nor, np);
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ ret = of_property_read_u32(np, "reg", &priv->cs);
+ if (ret) {
+ dev_err(dev, "No reg property for %s\n",
+ np->full_name);
+ return ret;
+ }
+
+ ret = of_property_read_u32(np, "spi-max-frequency",
+ &priv->clk_rate);
+ if (ret) {
+ dev_err(dev, "No spi-max-frequency property for %s\n",
+ np->full_name);
+ return ret;
+ }
+
+ priv->sfc = sfc;
+ nor->priv = priv;
+
+ nor->prepare = rockchip_sfc_prep;
+ nor->unprepare = rockchip_sfc_unprep;
+ nor->read_reg = rockchip_sfc_read_reg;
+ nor->write_reg = rockchip_sfc_write_reg;
+ nor->read = rockchip_sfc_read;
+ nor->write = rockchip_sfc_write;
+ nor->erase = NULL;
+ ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
+ if (ret)
+ return ret;
+
+ mtd = &nor->mtd;
+ mtd->name = np->name;
+ ret = mtd_device_register(mtd, NULL, 0);
+ if (ret)
+ return ret;
+
+ sfc->nor[sfc->num_chip] = nor;
+ sfc->num_chip++;
+ return 0;
+}
+
+static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
+{
+ int i;
+
+ for (i = 0; i < sfc->num_chip; i++)
+ mtd_device_unregister(&sfc->nor[i]->mtd);
+}
+
+static int rockchip_sfc_register_all(struct rockchip_sfc *sfc)
+{
+ struct device *dev = sfc->dev;
+ struct device_node *np;
+ int ret;
+
+ for_each_available_child_of_node(dev->of_node, np) {
+ ret = rockchip_sfc_register(np, sfc);
+ if (ret)
+ goto fail;
+
+ if (sfc->num_chip == SFC_MAX_CHIP_NUM) {
+ dev_warn(dev, "Exceeds the max cs limitation\n");
+ break;
+ }
+ }
+
+ return 0;
+
+fail:
+ dev_err(dev, "Failed to register all chip\n");
+ rockchip_sfc_unregister_all(sfc);
+ return ret;
+}
+
+static irqreturn_t rockchip_sfc_irq_handler(int irq, void *dev_id)
+{
+ struct rockchip_sfc *sfc = dev_id;
+ u32 reg;
+
+ reg = readl_relaxed(sfc->regbase + SFC_RISR);
+ dev_dbg(sfc->dev, "Get irq: 0x%x\n", reg);
+
+ /* Clear interrupt */
+ writel_relaxed(reg, sfc->regbase + SFC_ICLR);
+
+ if (reg & SFC_IRQ_TRAN_FINISH)
+ complete(&sfc->cp);
+
+ return IRQ_HANDLED;
+}
+
+static int rockchip_sfc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+ struct rockchip_sfc *sfc;
+ int ret;
+
+ sfc = devm_kzalloc(dev, sizeof(*sfc), GFP_KERNEL);
+ if (!sfc)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, sfc);
+ sfc->dev = dev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ sfc->regbase = devm_ioremap_resource(dev, res);
+ if (IS_ERR(sfc->regbase))
+ return PTR_ERR(sfc->regbase);
+
+ sfc->clk = devm_clk_get(&pdev->dev, "sfc");
+ if (IS_ERR(sfc->clk)) {
+ dev_err(&pdev->dev, "Failed to get sfc interface clk\n");
+ return PTR_ERR(sfc->clk);
+ }
+
+ sfc->hclk = devm_clk_get(&pdev->dev, "hsfc");
+ if (IS_ERR(sfc->hclk)) {
+ dev_err(&pdev->dev, "Failed to get sfc ahp clk\n");
+ return PTR_ERR(sfc->hclk);
+ }
+
+ ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
+ if (ret) {
+ dev_warn(dev, "Unable to set dma mask\n");
+ return ret;
+ }
+
+ sfc->buffer = dmam_alloc_coherent(dev, SFC_DMA_MAX_LEN,
+ &sfc->dma_buffer, GFP_KERNEL);
+ if (!sfc->buffer)
+ return -ENOMEM;
+
+ mutex_init(&sfc->lock);
+
+ ret = clk_prepare_enable(sfc->hclk);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to enable hclk\n");
+ goto err_hclk;
+ }
+
+ ret = clk_prepare_enable(sfc->clk);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to enable clk\n");
+ goto err_clk;
+ }
+
+ if (of_property_read_bool(sfc->dev->of_node, "rockchip,sfc-no-dma"))
+ sfc->use_dma = false;
+ else
+ sfc->use_dma = true;
+
+ if (of_device_is_compatible(sfc->dev->of_node,
+ "rockchip,rk1108-sfc"))
+ sfc->negative_edge = true;
+ else
+ sfc->negative_edge = false;
+
+ /* Find the irq */
+ ret = platform_get_irq(pdev, 0);
+ if (ret < 0) {
+ dev_err(dev, "Failed to get the irq\n");
+ goto err_irq;
+ }
+
+ ret = devm_request_irq(dev, ret, rockchip_sfc_irq_handler,
+ 0, pdev->name, sfc);
+ if (ret) {
+ dev_err(dev, "Failed to request irq\n");
+ goto err_irq;
+ }
+
+ ret = rockchip_sfc_init(sfc);
+ if (ret)
+ goto err_init;
+
+ ret = rockchip_sfc_register_all(sfc);
+ if (ret)
+ goto err_init;
+
+ clk_disable_unprepare(sfc->clk);
+ return 0;
+
+err_irq:
+err_init:
+ clk_disable_unprepare(sfc->clk);
+err_clk:
+ clk_disable_unprepare(sfc->hclk);
+err_hclk:
+ mutex_destroy(&sfc->lock);
+ return ret;
+}
+
+static int rockchip_sfc_remove(struct platform_device *pdev)
+{
+ struct rockchip_sfc *sfc = platform_get_drvdata(pdev);
+
+ rockchip_sfc_unregister_all(sfc);
+ mutex_destroy(&sfc->lock);
+ clk_disable_unprepare(sfc->clk);
+ clk_disable_unprepare(sfc->hclk);
+ return 0;
+}
+
+static const struct of_device_id rockchip_sfc_dt_ids[] = {
+ { .compatible = "rockchip,sfc"},
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rockchip_sfc_dt_ids);
+
+static struct platform_driver rockchip_sfc_driver = {
+ .driver = {
+ .name = "rockchip-sfc",
+ .of_match_table = rockchip_sfc_dt_ids,
+ },
+ .probe = rockchip_sfc_probe,
+ .remove = rockchip_sfc_remove,
+};
+module_platform_driver(rockchip_sfc_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Rockchip Serial Flash Controller Driver");
--
1.9.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nor: Bindings for Rockchip serial flash controller
2016-11-11 9:16 ` Shawn Lin
@ 2016-11-15 15:05 ` Rob Herring
-1 siblings, 0 replies; 20+ messages in thread
From: Rob Herring @ 2016-11-15 15:05 UTC (permalink / raw)
To: Shawn Lin
Cc: David Woodhouse, Brian Norris, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Heiko Stuebner
On Fri, Nov 11, 2016 at 05:16:05PM +0800, Shawn Lin wrote:
> Add binding document for the Rockchip serial flash controller.
>
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
>
> .../devicetree/bindings/mtd/rockchip-sfc.txt | 31 ++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mtd/rockchip-sfc.txt
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nor: Bindings for Rockchip serial flash controller
@ 2016-11-15 15:05 ` Rob Herring
0 siblings, 0 replies; 20+ messages in thread
From: Rob Herring @ 2016-11-15 15:05 UTC (permalink / raw)
To: Shawn Lin
Cc: David Woodhouse, Brian Norris, devicetree, linux-rockchip,
linux-mtd, Heiko Stuebner
On Fri, Nov 11, 2016 at 05:16:05PM +0800, Shawn Lin wrote:
> Add binding document for the Rockchip serial flash controller.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
> .../devicetree/bindings/mtd/rockchip-sfc.txt | 31 ++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mtd/rockchip-sfc.txt
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nor: add rockchip serial flash controller driver
2016-11-11 9:16 ` Shawn Lin
@ 2016-11-15 20:52 ` Marek Vasut
-1 siblings, 0 replies; 20+ messages in thread
From: Marek Vasut @ 2016-11-15 20:52 UTC (permalink / raw)
To: Shawn Lin, Rob Herring, David Woodhouse, Brian Norris
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On 11/11/2016 10:16 AM, Shawn Lin wrote:
> Add rockchip serial flash controller driver
>
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
[...]
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 4a682ee..48c5e0e 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -65,6 +65,13 @@ config SPI_HISI_SFC
> help
> This enables support for hisilicon SPI-NOR flash controller.
>
> +config SPI_ROCKCHIP_SFC
Keep this list sorted please.
> + tristate "Rockchip Serial Flash Controller(SFC)"
> + depends on ARCH_ROCKCHIP || COMPILE_TEST
> + depends on HAS_IOMEM && HAS_DMA
> + help
> + This enables support for rockchip serial flash controller.
> +
> config SPI_NXP_SPIFI
> tristate "NXP SPI Flash Interface (SPIFI)"
> depends on OF && (ARCH_LPC18XX || COMPILE_TEST)
[...]
> +/* Interrypt mask */
Interrupt :)
> +#define SFC_IMR 0x4
> +#define SFC_IMR_RX_FULL BIT(0)
> +#define SFC_IMR_RX_UFLOW BIT(1)
> +#define SFC_IMR_TX_OFLOW BIT(2)
> +#define SFC_IMR_TX_EMPTY BIT(3)
> +#define SFC_IMR_TRAN_FINISH BIT(4)
> +#define SFC_IMR_BUS_ERR BIT(5)
> +#define SFC_IMR_NSPI_ERR BIT(6)
> +#define SFC_IMR_DMA BIT(7)
[...]
> +enum rockchip_sfc_iftype {
> + IF_TYPE_STD,
> + IF_TYPE_DUAL,
> + IF_TYPE_QUAD,
> +};
> +
> +struct rockchip_sfc {
> + struct device *dev;
> + struct mutex lock;
> + void __iomem *regbase;
> + struct clk *hclk;
> + struct clk *clk;
> + void *buffer;
> + dma_addr_t dma_buffer;
The naming (buffer) could use some improvement or comment for clarification.
> + struct completion cp;
> + struct spi_nor *nor[SFC_MAX_CHIP_NUM];
Should be MAX_CHIPSELECT_NUM , for clarity.
> + u32 num_chip;
u8
> + bool use_dma;
> + bool negative_edge;
Negative edge ... of what ?
> +};
> +
> +struct rockchip_sfc_priv {
> + u32 cs;
Doesn't this board support only 4 CS ? Use u8 :-)
> + u32 clk_rate;
> + struct rockchip_sfc *sfc;
> +};
> +
> +static int get_if_type(enum read_mode flash_read)
> +{
> + enum rockchip_sfc_iftype if_type;
> +
> + switch (flash_read) {
> + case SPI_NOR_DUAL:
> + if_type = IF_TYPE_DUAL;
> + break;
> + case SPI_NOR_QUAD:
> + if_type = IF_TYPE_QUAD;
> + break;
> + case SPI_NOR_NORMAL:
> + case SPI_NOR_FAST:
> + default:
Should the default case really fall back to 1-bit mode or should it
rather report error ?
> + if_type = IF_TYPE_STD;
> + break;
> + }
> +
> + return if_type;
> +}
> +
> +static int rockchip_sfc_reset(struct rockchip_sfc *sfc)
> +{
> + unsigned long timeout = jiffies + HZ;
> + int err = -ETIMEDOUT;
> + u32 status;
> +
> + writel_relaxed(SFC_RCVR_RESET, sfc->regbase + SFC_RCVR);
> +
> + while (time_before(jiffies, timeout)) {
Would readl_poll_*() from include/linux/iopoll.h help here ?
> + status = readl_relaxed(sfc->regbase + SFC_RCVR);
> + if (!(status & SFC_RCVR_RESET)) {
> + err = 0;
> + break;
> + }
> + msleep(20);
> + }
> +
> + if (err)
> + dev_err(sfc->dev, "SFC reset never finished\n");
Should the writel() below be executed if an error happened ?
> + writel_relaxed(SFC_ICLR_RX_FULL | SFC_ICLR_RX_UFLOW |
> + SFC_ICLR_TX_OFLOW | SFC_ICLR_TX_EMPTY |
> + SFC_ICLR_TRAN_FINISH | SFC_ICLR_BUS_ERR |
> + SFC_ICLR_NSPI_ERR | SFC_ICLR_DMA,
> + sfc->regbase + SFC_ICLR);
> + return err;
> +}
> +
> +static int rockchip_sfc_init(struct rockchip_sfc *sfc)
> +{
> + int err;
> +
> + err = rockchip_sfc_reset(sfc);
> + if (err)
> + return err;
> +
> + /* Mask all eight interrupts */
> + writel_relaxed(0xff, sfc->regbase + SFC_IMR);
> + /* Phase configure */
What phase ? Please clarify the comment. Also, don't you have to
configure the register if sfc->negative_edge == 0 too ?
> + if (sfc->negative_edge)
> + writel_relaxed(SFC_CTRL_PHASE_SEL_NEGETIVE <<
> + SFC_CTRL_PHASE_SEL_SHIFT,
> + sfc->regbase + SFC_CTRL);
> + return 0;
> +}
> +
> +static int rockchip_sfc_prep(struct spi_nor *nor, enum spi_nor_ops ops)
> +{
> + struct rockchip_sfc_priv *priv = nor->priv;
> + struct rockchip_sfc *sfc = priv->sfc;
> + int ret;
> +
> + mutex_lock(&sfc->lock);
> +
> + ret = clk_set_rate(sfc->clk, priv->clk_rate);
> + if (ret)
> + goto out;
> +
> + ret = clk_prepare_enable(sfc->clk);
> + if (ret)
> + goto out;
> +
> + return 0;
> +
> +out:
> + mutex_unlock(&sfc->lock);
> + return ret;
> +}
> +
> +static void rockchip_sfc_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
> +{
> + struct rockchip_sfc_priv *priv = nor->priv;
> + struct rockchip_sfc *sfc = priv->sfc;
> +
> + clk_disable_unprepare(sfc->clk);
> + mutex_unlock(&sfc->lock);
> +}
> +
> +static int rockchip_sfc_wait_op_finish(struct rockchip_sfc *sfc)
> +{
> + unsigned long timeout = jiffies + 2 * HZ;
> + int err = -ETIMEDOUT;
> + u32 status;
> +
> + /*
> + * Note: tx and rx share the same fifo, so the rx's water level
> + * is the same as rx's, which means this function could be reused
> + * for checking the read operations as well.
> + */
> + while (time_before(jiffies, timeout)) {
readl_poll_*() ?
> + status = readl_relaxed(sfc->regbase + SFC_FSR);
> + if (((status >> SFC_FSR_TX_EMPTY_SHIFT) &
> + SFC_FSR_TX_EMPTY_MASK) == SFC_FSR_TX_IS_EMPTY) {
> + err = 0;
> + break;
> + }
> + msleep(20);
> + }
> +
> + if (err)
> + dev_err(sfc->dev, "SFC tx never empty\n");
> +
> + return err;
> +}
> +
> +static int rockchip_sfc_op_reg(struct spi_nor *nor,
> + u8 opcode, int len, u8 optype)
> +{
> + struct rockchip_sfc_priv *priv = nor->priv;
> + struct rockchip_sfc *sfc = priv->sfc;
> + u32 reg;
> +
> + if (((readl_relaxed(sfc->regbase + SFC_FSR) >> SFC_FSR_TX_EMPTY_SHIFT) &
> + SFC_FSR_TX_EMPTY_MASK) != SFC_FSR_TX_IS_EMPTY ||
> + ((readl_relaxed(sfc->regbase + SFC_FSR) >>
> + SFC_FSR_RX_EMPTY_SHIFT) &
> + SFC_FSR_RX_EMPTY_MASK) != SFC_FSR_RX_IS_EMPTY ||
> + (readl_relaxed(sfc->regbase + SFC_SR) == SFC_SR_IS_BUSY))
> + rockchip_sfc_reset(sfc);
This is chaos, please fix this condition so it's actually readable. You
can ie. read the FSR into a variable, do your shifting/anding magic and
then do if (var1 || var2 || var3) {} .
> + reg = (opcode & SFC_CMD_IDX_MASK) << SFC_CMD_IDX_SHIFT;
> + reg |= (len & SFC_CMD_TRAN_BYTES_MASK) << SFC_CMD_TRAN_BYTES_SHIFT;
> + reg |= (priv->cs & SFC_CMD_CS_MASK) << SFC_CMD_CS_SHIFT;
> + reg |= (optype & SFC_CMD_DIR_MASK) << SFC_CMD_DIR_SHIFT;
> +
> + writel_relaxed(reg, sfc->regbase + SFC_CMD);
> +
> + return rockchip_sfc_wait_op_finish(sfc);
> +}
> +
> +static int rockchip_sfc_read_reg(struct spi_nor *nor, u8 opcode,
> + u8 *buf, int len)
> +{
> + struct rockchip_sfc_priv *priv = nor->priv;
> + struct rockchip_sfc *sfc = priv->sfc;
> + int ret;
> + u32 tmp;
> + u32 i;
> +
> + ret = rockchip_sfc_op_reg(nor, opcode, len, SFC_CMD_DIR_RD);
> + if (ret)
> + return ret;
> +
> + while (len > 0) {
> + tmp = readl_relaxed(sfc->regbase + SFC_DATA);
> + for (i = 0; i < len; i++)
> + *buf++ = (u8)((tmp >> (i * 8)) & 0xff);
Won't this fail for len > 4 ?
Also, you can use ioread32_rep() here, but (!) that won't work for
unaligned reads, which I dunno if they can happen here, but please do
double-check.
> + len = len - 4;
> + }
> +
> + return 0;
> +}
> +
> +static int rockchip_sfc_write_reg(struct spi_nor *nor, u8 opcode,
> + u8 *buf, int len)
> +{
> + struct rockchip_sfc_priv *priv = nor->priv;
> + struct rockchip_sfc *sfc = priv->sfc;
> + u32 words, i;
> +
> + /* Align bytes to words */
> + words = (len + 3) >> 2;
> +
> + for (i = 0; i < words; i++)
> + writel_relaxed(*(buf + 4 * i), sfc->regbase + SFC_DATA);
See above about the ioread32_rep()/iowrite32_rep(), but careful about
unaligned (len % 4 != 0) case.
> + return rockchip_sfc_op_reg(nor, opcode, len, SFC_CMD_DIR_WR);
> +}
> +
> +static int rockchip_sfc_dma_transfer(struct spi_nor *nor, loff_t from_to,
> + dma_addr_t dma_buf, size_t len, u8 op_type)
> +{
> + struct rockchip_sfc_priv *priv = nor->priv;
> + struct rockchip_sfc *sfc = priv->sfc;
> + u32 reg;
> + u8 if_type = 0;
> +
> + init_completion(&sfc->cp);
> +
> + writel_relaxed(SFC_ICLR_RX_FULL | SFC_ICLR_RX_UFLOW |
> + SFC_ICLR_TX_OFLOW | SFC_ICLR_TX_EMPTY |
> + SFC_ICLR_TRAN_FINISH | SFC_ICLR_BUS_ERR |
> + SFC_ICLR_NSPI_ERR | SFC_ICLR_DMA,
> + sfc->regbase + SFC_ICLR);
> +
> + /* Enable transfer complete interrupt */
> + reg = readl_relaxed(sfc->regbase + SFC_IMR);
> + reg &= ~SFC_IMR_TRAN_FINISH;
> + writel_relaxed(reg, sfc->regbase + SFC_IMR);
> +
> + if (op_type == SFC_CMD_DIR_WR)
> + reg = (SFC_CMD_DIR_WR << SFC_CMD_DIR_SHIFT) |
> + ((nor->program_opcode & SFC_CMD_IDX_MASK) <<
> + SFC_CMD_IDX_SHIFT);
> + else
> + reg = (SFC_CMD_DIR_RD << SFC_CMD_DIR_SHIFT) |
> + ((nor->read_opcode & SFC_CMD_IDX_MASK) <<
> + SFC_CMD_IDX_SHIFT);
reg = nor->read_opcode & SFC_CMD_IDX_MASK) << SFC_CMD_IDX_SHIFT;
reg |= op_type << SFC_CMD_DIR_SHIFT;
> + reg |= ((nor->addr_width == 4) ? SFC_CMD_ADDR_32BITS
> + : SFC_CMD_ADDR_24BITS) << SFC_CMD_ADDR_SHIFT;
Why don't you just define those SFC_CMD_ADDR_24BITS and co. with the
shift in those bitfields already ? Then you wouldn't have to riddle this
driver with FOO << BAR, but you'd only have FOO all over the place.
> + if_type = get_if_type(nor->flash_read);
> + writel_relaxed(if_type << SFC_CTRL_DATA_BITS_SHIFT |
> + if_type << SFC_CTRL_ADDR_BITS_SHIFT |
> + if_type << SFC_CTRL_CMD_BITS_SHIFT |
Parenthesis missing around the statements ,
(if_type << FOO) | (... << bar)
> + sfc->negative_edge ?
> + SFC_CTRL_PHASE_SEL_NEGETIVE << SFC_CTRL_PHASE_SEL_SHIFT :
> + SFC_CTRL_PHASE_SEL_POSITIVE << SFC_CTRL_PHASE_SEL_SHIFT,
> + sfc->regbase + SFC_CTRL);
> +
> + reg |= (priv->cs & SFC_CMD_CS_MASK) << SFC_CMD_CS_SHIFT;
> + reg |= (len & SFC_CMD_TRAN_BYTES_MASK) << SFC_CMD_TRAN_BYTES_SHIFT;
> +
> + if (op_type == SFC_CMD_DIR_RD)
> + reg |= (nor->read_dummy & SFC_CMD_DUMMY_MASK) <<
> + SFC_CMD_DUMMY_SHIFT;
Just define SFC_CMD_DUMMY(x) \
(((x) & SFC_CMD_DUMMY_MASK) << SFC_CMD_DUMMY_SHIFT)
And then use it ... reg |= SFC_CMD_DUMMY(nor->read_dummy);
> + /* Should minus one as 0x0 means 1 bit flash address */
> + writel_relaxed(nor->addr_width * 8 - 1, sfc->regbase + SFC_ABIT);
> + writel_relaxed(reg, sfc->regbase + SFC_CMD);
> + writel_relaxed(from_to, sfc->regbase + SFC_ADDR);
> + writel_relaxed(dma_buf, sfc->regbase + SFC_DMA_ADDR);
I hope the DMA buffer management is implemented correctly and you're not
running into any weird cache issues.
> + /* Start dma */
> + writel_relaxed(0x1, sfc->regbase + SFC_DMA_TRIGGER);
> +
> + /* Wait for the interrupt. */
> + if (!wait_for_completion_timeout(&sfc->cp, msecs_to_jiffies(2000))) {
> + dev_err(sfc->dev, "DMA wait for transfer finish timeout.");
> + return -ETIMEDOUT;
> + }
> +
> + /* Disable transfer finish interrupt */
> + reg = readl_relaxed(sfc->regbase + SFC_IMR);
> + reg |= SFC_IMR_TRAN_FINISH;
> + writel_relaxed(reg, sfc->regbase + SFC_IMR);
> +
> + return rockchip_sfc_wait_op_finish(sfc);
> +}
> +
> +static inline int rockchip_sfc_pio_write(struct rockchip_sfc *sfc, u_char *buf,
> + size_t len)
> +{
> + u32 words, tx_wl, count, i;
> + unsigned long timeout;
> + int ret = 0;
> + u32 *tbuf = (u32 *)buf;
> +
> + /* Align bytes to words */
> + words = (len + 3) >> 2;
> +
> + while (words) {
See iowrite32_rep() above, but I suspect you'll run into problems with
$len which is not multiple of 4 .
> + tx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
> + SFC_FSR_TX_WATER_LVL_SHIFT) &
> + SFC_FSR_TX_WATER_LVL_MASK;
> +
> + if (tx_wl > 0) {
> + count = min_t(u32, words, tx_wl);
> + for (i = 0; i < count; i++) {
> + writel_relaxed(*tbuf++,
> + sfc->regbase + SFC_DATA);
> + words--;
> + }
> +
> + if (words == 0)
> + break;
> + timeout = 0;
> + } else {
> + mdelay(1);
> + if (timeout++ > SFC_MAX_IDLE_RETRY) {
> + ret = -ETIMEDOUT;
> + break;
> + }
> + }
> + }
> +
> + if (ret)
> + return ret;
> + else
> + return rockchip_sfc_wait_op_finish(sfc);
> +}
> +
> +static inline int rockchip_sfc_pio_read(struct rockchip_sfc *sfc, u_char *buf,
> + size_t len)
> +{
> + u32 words, rx_wl, count, i;
> + unsigned long timeout;
> + int ret = 0;
> + u32 tmp;
> + u32 *tbuf = (u32 *)buf;
> + u_char *tbuf2;
> +
> + words = len >> 2;
> + /* Get the remained bytes */
> + len = len & 0x3;
See above.
> + while (words) {
> + rx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
> + SFC_FSR_RX_WATER_LVL_SHIFT) &
> + SFC_FSR_RX_WATER_LVL_MASK;
> +
> + if (rx_wl > 0) {
> + count = min_t(u32, words, rx_wl);
> + for (i = 0; i < count; i++) {
> + *tbuf++ = readl_relaxed(sfc->regbase +
> + SFC_DATA);
> + words--;
> + }
> +
> + if (words == 0)
> + break;
> + timeout = 0;
> + } else {
> + mdelay(1);
> + if (timeout++ > SFC_MAX_IDLE_RETRY) {
> + ret = -ETIMEDOUT;
> + break;
> + }
> + }
> + }
> +
> + if (ret)
> + return ret;
> +
> + /* Read the remained bytes */
> + timeout = 0;
> + tbuf2 = (u_char *)tbuf;
> + while (len) {
> + rx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
> + SFC_FSR_RX_WATER_LVL_SHIFT) &
> + SFC_FSR_RX_WATER_LVL_MASK;
> + if (rx_wl > 0) {
> + tmp = readl_relaxed(sfc->regbase + SFC_DATA);
> + for (i = 0; i < len; i++)
> + tbuf2[i] = (u8)((tmp >> (i * 8)) & 0xff);
> + goto done;
> + } else {
> + mdelay(1);
> + if (timeout++ > SFC_MAX_IDLE_RETRY) {
> + ret = -ETIMEDOUT;
> + break;
> + }
> + }
> + }
> +done:
> + if (ret)
> + return ret;
> + else
> + return rockchip_sfc_wait_op_finish(sfc);
> +}
> +
> +static int rockchip_sfc_pio_transfer(struct spi_nor *nor, loff_t from_to,
> + size_t len, u_char *buf, u8 op_type)
> +{
> + struct rockchip_sfc_priv *priv = nor->priv;
> + struct rockchip_sfc *sfc = priv->sfc;
> + u32 reg;
> + u8 if_type = 0;
> +
> + if (op_type == SFC_CMD_DIR_WR)
> + reg = (SFC_CMD_DIR_WR << SFC_CMD_DIR_SHIFT) |
> + ((nor->program_opcode & SFC_CMD_IDX_MASK) <<
> + SFC_CMD_IDX_SHIFT);
> + else
> + reg = (SFC_CMD_DIR_RD << SFC_CMD_DIR_SHIFT) |
> + ((nor->read_opcode & SFC_CMD_IDX_MASK) <<
> + SFC_CMD_IDX_SHIFT);
See above regarding this condition. I think you can factor out this
common code too. Also nuke the bitshifts , see my comments on
rockchip_sfc_dma_transfer .
> + reg |= ((nor->addr_width == 4) ? SFC_CMD_ADDR_32BITS
> + : SFC_CMD_ADDR_24BITS) << SFC_CMD_ADDR_SHIFT;
> +
> + if_type = get_if_type(nor->flash_read);
> + writel_relaxed(if_type << SFC_CTRL_DATA_BITS_SHIFT |
> + if_type << SFC_CTRL_ADDR_BITS_SHIFT |
> + if_type << SFC_CTRL_CMD_BITS_SHIFT |
> + sfc->negative_edge ?
> + SFC_CTRL_PHASE_SEL_NEGETIVE << SFC_CTRL_PHASE_SEL_SHIFT :
> + SFC_CTRL_PHASE_SEL_POSITIVE << SFC_CTRL_PHASE_SEL_SHIFT,
> + sfc->regbase + SFC_CTRL);
> +
> + reg |= (priv->cs & SFC_CMD_CS_MASK) << SFC_CMD_CS_SHIFT;
> + reg |= (len & SFC_CMD_TRAN_BYTES_MASK) << SFC_CMD_TRAN_BYTES_SHIFT;
> +
> + if (op_type == SFC_CMD_DIR_RD)
> + reg |= (nor->read_dummy & SFC_CMD_DUMMY_MASK) <<
> + SFC_CMD_DUMMY_SHIFT;
> +
> + /* Should minus one as 0x0 means 1 bit flash address */
> + writel_relaxed(nor->addr_width * 8 - 1, sfc->regbase + SFC_ABIT);
> + writel_relaxed(reg, sfc->regbase + SFC_CMD);
> + writel_relaxed(from_to, sfc->regbase + SFC_ADDR);
> +
> + if (op_type == SFC_CMD_DIR_WR)
> + return rockchip_sfc_pio_write(sfc, buf, len);
> + else
> + return rockchip_sfc_pio_read(sfc, buf, len);
> +}
> +
> +static ssize_t rockchip_sfc_read(struct spi_nor *nor, loff_t from, size_t len,
> + u_char *read_buf)
> +{
> + struct rockchip_sfc_priv *priv = nor->priv;
> + struct rockchip_sfc *sfc = priv->sfc;
> + size_t offset;
> + int ret;
> + dma_addr_t dma_addr = 0;
> +
> + if (!sfc->use_dma)
> + goto no_dma;
> +
> + for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
> + size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
> +
> + dma_addr = dma_map_single(NULL, (void *)read_buf,
> + trans, DMA_FROM_DEVICE);
> + if (dma_mapping_error(sfc->dev, dma_addr))
> + dma_addr = 0;
> +
> + /* Fail to map dma, use pre-allocated area instead */
> + ret = rockchip_sfc_dma_transfer(nor, from + offset,
> + dma_addr ? dma_addr :
> + sfc->dma_buffer,
> + trans, SFC_CMD_DIR_RD);
> + if (ret) {
> + dev_warn(nor->dev, "DMA read timeout\n");
> + return ret;
> + }
> + if (!dma_addr)
> + memcpy(read_buf + offset, sfc->buffer, trans);
> + }
> +
> + return len;
> +
> +no_dma:
> + ret = rockchip_sfc_pio_transfer(nor, from, len,
> + read_buf, SFC_CMD_DIR_RD);
> + if (ret) {
> + dev_warn(nor->dev, "PIO read timeout\n");
> + return ret;
> + }
> + return len;
> +}
> +
> +static ssize_t rockchip_sfc_write(struct spi_nor *nor, loff_t to,
> + size_t len, const u_char *write_buf)
> +{
> + struct rockchip_sfc_priv *priv = nor->priv;
> + struct rockchip_sfc *sfc = priv->sfc;
> + size_t offset;
> + int ret;
> + dma_addr_t dma_addr = 0;
> +
> + if (!sfc->use_dma)
> + goto no_dma;
Seems like there's a lot of similarity between read/write .
> + for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
> + size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
> +
> + dma_addr = dma_map_single(NULL, (void *)write_buf,
> + trans, DMA_TO_DEVICE);
> + if (dma_mapping_error(sfc->dev, dma_addr)) {
> + dma_addr = 0;
> + memcpy(sfc->buffer, write_buf + offset, trans);
> + }
> +
> + /* Fail to map dma, use pre-allocated area instead */
> + ret = rockchip_sfc_dma_transfer(nor, to + offset,
> + dma_addr ? dma_addr :
> + sfc->dma_buffer,
> + trans, SFC_CMD_DIR_WR);
> + if (dma_addr)
> + dma_unmap_single(NULL, dma_addr,
> + trans, DMA_TO_DEVICE);
> + if (ret) {
> + dev_warn(nor->dev, "DMA write timeout\n");
> + return ret;
> + }
> + }
> +
> + return len;
> +no_dma:
> + ret = rockchip_sfc_pio_transfer(nor, to, len,
> + (u_char *)write_buf, SFC_CMD_DIR_WR);
> + if (ret) {
> + dev_warn(nor->dev, "PIO write timeout\n");
> + return ret;
> + }
> + return len;
> +}
> +
> +/**
> + * Get spi flash device information and register it as a mtd device.
> + */
> +static int rockchip_sfc_register(struct device_node *np,
> + struct rockchip_sfc *sfc)
> +{
> + struct device *dev = sfc->dev;
> + struct spi_nor *nor;
> + struct rockchip_sfc_priv *priv;
> + struct mtd_info *mtd;
> + int ret;
> +
> + nor = devm_kzalloc(dev, sizeof(*nor), GFP_KERNEL);
> + if (!nor)
> + return -ENOMEM;
You can embed struct spi_nor in struct rockchip_sfc_priv and drop this
allocation . Also it'd be a good idea to rename rockchip_sfc_priv to
something like rockchip_sfc_chip_priv to make it explicit this is a
per-chip private data -- which you can even pre-allocate in rockchi_sfc
structure as a static array of (four) such structures (see cadence qspi
driver for how this is done there).
> + nor->dev = dev;
> + spi_nor_set_flash_node(nor, np);
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + ret = of_property_read_u32(np, "reg", &priv->cs);
> + if (ret) {
> + dev_err(dev, "No reg property for %s\n",
> + np->full_name);
> + return ret;
> + }
> +
> + ret = of_property_read_u32(np, "spi-max-frequency",
> + &priv->clk_rate);
> + if (ret) {
> + dev_err(dev, "No spi-max-frequency property for %s\n",
> + np->full_name);
> + return ret;
> + }
> +
> + priv->sfc = sfc;
> + nor->priv = priv;
> +
> + nor->prepare = rockchip_sfc_prep;
> + nor->unprepare = rockchip_sfc_unprep;
> + nor->read_reg = rockchip_sfc_read_reg;
> + nor->write_reg = rockchip_sfc_write_reg;
> + nor->read = rockchip_sfc_read;
> + nor->write = rockchip_sfc_write;
> + nor->erase = NULL;
> + ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
> + if (ret)
> + return ret;
> +
> + mtd = &nor->mtd;
> + mtd->name = np->name;
> + ret = mtd_device_register(mtd, NULL, 0);
> + if (ret)
> + return ret;
> +
> + sfc->nor[sfc->num_chip] = nor;
> + sfc->num_chip++;
> + return 0;
> +}
> +
> +static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
> +{
> + int i;
> +
> + for (i = 0; i < sfc->num_chip; i++)
> + mtd_device_unregister(&sfc->nor[i]->mtd);
> +}
> +
> +static int rockchip_sfc_register_all(struct rockchip_sfc *sfc)
> +{
> + struct device *dev = sfc->dev;
> + struct device_node *np;
> + int ret;
> +
> + for_each_available_child_of_node(dev->of_node, np) {
> + ret = rockchip_sfc_register(np, sfc);
> + if (ret)
> + goto fail;
> +
> + if (sfc->num_chip == SFC_MAX_CHIP_NUM) {
> + dev_warn(dev, "Exceeds the max cs limitation\n");
> + break;
> + }
> + }
> +
> + return 0;
> +
> +fail:
> + dev_err(dev, "Failed to register all chip\n");
> + rockchip_sfc_unregister_all(sfc);
See cadence qspi where we only unregister the registered flashes.
Implement it the same way here.
> + return ret;
> +}
> +
> +static irqreturn_t rockchip_sfc_irq_handler(int irq, void *dev_id)
> +{
> + struct rockchip_sfc *sfc = dev_id;
> + u32 reg;
> +
> + reg = readl_relaxed(sfc->regbase + SFC_RISR);
> + dev_dbg(sfc->dev, "Get irq: 0x%x\n", reg);
> +
> + /* Clear interrupt */
> + writel_relaxed(reg, sfc->regbase + SFC_ICLR);
> +
> + if (reg & SFC_IRQ_TRAN_FINISH)
> + complete(&sfc->cp);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int rockchip_sfc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> + struct rockchip_sfc *sfc;
> + int ret;
> +
> + sfc = devm_kzalloc(dev, sizeof(*sfc), GFP_KERNEL);
> + if (!sfc)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, sfc);
> + sfc->dev = dev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + sfc->regbase = devm_ioremap_resource(dev, res);
> + if (IS_ERR(sfc->regbase))
> + return PTR_ERR(sfc->regbase);
> +
> + sfc->clk = devm_clk_get(&pdev->dev, "sfc");
> + if (IS_ERR(sfc->clk)) {
> + dev_err(&pdev->dev, "Failed to get sfc interface clk\n");
> + return PTR_ERR(sfc->clk);
> + }
> +
> + sfc->hclk = devm_clk_get(&pdev->dev, "hsfc");
> + if (IS_ERR(sfc->hclk)) {
> + dev_err(&pdev->dev, "Failed to get sfc ahp clk\n");
> + return PTR_ERR(sfc->hclk);
> + }
> +
> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> + if (ret) {
> + dev_warn(dev, "Unable to set dma mask\n");
> + return ret;
> + }
> +
> + sfc->buffer = dmam_alloc_coherent(dev, SFC_DMA_MAX_LEN,
> + &sfc->dma_buffer, GFP_KERNEL);
> + if (!sfc->buffer)
> + return -ENOMEM;
> +
> + mutex_init(&sfc->lock);
> +
> + ret = clk_prepare_enable(sfc->hclk);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to enable hclk\n");
> + goto err_hclk;
> + }
> +
> + ret = clk_prepare_enable(sfc->clk);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to enable clk\n");
> + goto err_clk;
> + }
> +
> + if (of_property_read_bool(sfc->dev->of_node, "rockchip,sfc-no-dma"))
> + sfc->use_dma = false;
> + else
> + sfc->use_dma = true;
sfc->use_dma = !of_property_read_bool(sfc->dev->of_node,
"rockchip,sfc-no-dma");
> + if (of_device_is_compatible(sfc->dev->of_node,
> + "rockchip,rk1108-sfc"))
> + sfc->negative_edge = true;
> + else
> + sfc->negative_edge = false;
See above
> + /* Find the irq */
> + ret = platform_get_irq(pdev, 0);
> + if (ret < 0) {
> + dev_err(dev, "Failed to get the irq\n");
> + goto err_irq;
> + }
> +
> + ret = devm_request_irq(dev, ret, rockchip_sfc_irq_handler,
> + 0, pdev->name, sfc);
> + if (ret) {
> + dev_err(dev, "Failed to request irq\n");
> + goto err_irq;
> + }
> +
> + ret = rockchip_sfc_init(sfc);
> + if (ret)
> + goto err_init;
> +
> + ret = rockchip_sfc_register_all(sfc);
> + if (ret)
> + goto err_init;
> +
> + clk_disable_unprepare(sfc->clk);
> + return 0;
> +
> +err_irq:
> +err_init:
Drop the err_irq: label unless you plan to handle the error (which you
should).
> + clk_disable_unprepare(sfc->clk);
> +err_clk:
> + clk_disable_unprepare(sfc->hclk);
> +err_hclk:
> + mutex_destroy(&sfc->lock);
> + return ret;
> +}
> +
> +static int rockchip_sfc_remove(struct platform_device *pdev)
> +{
> + struct rockchip_sfc *sfc = platform_get_drvdata(pdev);
> +
> + rockchip_sfc_unregister_all(sfc);
> + mutex_destroy(&sfc->lock);
> + clk_disable_unprepare(sfc->clk);
> + clk_disable_unprepare(sfc->hclk);
> + return 0;
> +}
> +
> +static const struct of_device_id rockchip_sfc_dt_ids[] = {
> + { .compatible = "rockchip,sfc"},
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, rockchip_sfc_dt_ids);
> +
> +static struct platform_driver rockchip_sfc_driver = {
> + .driver = {
> + .name = "rockchip-sfc",
> + .of_match_table = rockchip_sfc_dt_ids,
> + },
> + .probe = rockchip_sfc_probe,
> + .remove = rockchip_sfc_remove,
> +};
> +module_platform_driver(rockchip_sfc_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Rockchip Serial Flash Controller Driver");
MODULE_AUTHOR is missing
--
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nor: add rockchip serial flash controller driver
@ 2016-11-15 20:52 ` Marek Vasut
0 siblings, 0 replies; 20+ messages in thread
From: Marek Vasut @ 2016-11-15 20:52 UTC (permalink / raw)
To: Shawn Lin, Rob Herring, David Woodhouse, Brian Norris
Cc: devicetree, linux-mtd, Heiko Stuebner, linux-rockchip
On 11/11/2016 10:16 AM, Shawn Lin wrote:
> Add rockchip serial flash controller driver
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
[...]
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 4a682ee..48c5e0e 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -65,6 +65,13 @@ config SPI_HISI_SFC
> help
> This enables support for hisilicon SPI-NOR flash controller.
>
> +config SPI_ROCKCHIP_SFC
Keep this list sorted please.
> + tristate "Rockchip Serial Flash Controller(SFC)"
> + depends on ARCH_ROCKCHIP || COMPILE_TEST
> + depends on HAS_IOMEM && HAS_DMA
> + help
> + This enables support for rockchip serial flash controller.
> +
> config SPI_NXP_SPIFI
> tristate "NXP SPI Flash Interface (SPIFI)"
> depends on OF && (ARCH_LPC18XX || COMPILE_TEST)
[...]
> +/* Interrypt mask */
Interrupt :)
> +#define SFC_IMR 0x4
> +#define SFC_IMR_RX_FULL BIT(0)
> +#define SFC_IMR_RX_UFLOW BIT(1)
> +#define SFC_IMR_TX_OFLOW BIT(2)
> +#define SFC_IMR_TX_EMPTY BIT(3)
> +#define SFC_IMR_TRAN_FINISH BIT(4)
> +#define SFC_IMR_BUS_ERR BIT(5)
> +#define SFC_IMR_NSPI_ERR BIT(6)
> +#define SFC_IMR_DMA BIT(7)
[...]
> +enum rockchip_sfc_iftype {
> + IF_TYPE_STD,
> + IF_TYPE_DUAL,
> + IF_TYPE_QUAD,
> +};
> +
> +struct rockchip_sfc {
> + struct device *dev;
> + struct mutex lock;
> + void __iomem *regbase;
> + struct clk *hclk;
> + struct clk *clk;
> + void *buffer;
> + dma_addr_t dma_buffer;
The naming (buffer) could use some improvement or comment for clarification.
> + struct completion cp;
> + struct spi_nor *nor[SFC_MAX_CHIP_NUM];
Should be MAX_CHIPSELECT_NUM , for clarity.
> + u32 num_chip;
u8
> + bool use_dma;
> + bool negative_edge;
Negative edge ... of what ?
> +};
> +
> +struct rockchip_sfc_priv {
> + u32 cs;
Doesn't this board support only 4 CS ? Use u8 :-)
> + u32 clk_rate;
> + struct rockchip_sfc *sfc;
> +};
> +
> +static int get_if_type(enum read_mode flash_read)
> +{
> + enum rockchip_sfc_iftype if_type;
> +
> + switch (flash_read) {
> + case SPI_NOR_DUAL:
> + if_type = IF_TYPE_DUAL;
> + break;
> + case SPI_NOR_QUAD:
> + if_type = IF_TYPE_QUAD;
> + break;
> + case SPI_NOR_NORMAL:
> + case SPI_NOR_FAST:
> + default:
Should the default case really fall back to 1-bit mode or should it
rather report error ?
> + if_type = IF_TYPE_STD;
> + break;
> + }
> +
> + return if_type;
> +}
> +
> +static int rockchip_sfc_reset(struct rockchip_sfc *sfc)
> +{
> + unsigned long timeout = jiffies + HZ;
> + int err = -ETIMEDOUT;
> + u32 status;
> +
> + writel_relaxed(SFC_RCVR_RESET, sfc->regbase + SFC_RCVR);
> +
> + while (time_before(jiffies, timeout)) {
Would readl_poll_*() from include/linux/iopoll.h help here ?
> + status = readl_relaxed(sfc->regbase + SFC_RCVR);
> + if (!(status & SFC_RCVR_RESET)) {
> + err = 0;
> + break;
> + }
> + msleep(20);
> + }
> +
> + if (err)
> + dev_err(sfc->dev, "SFC reset never finished\n");
Should the writel() below be executed if an error happened ?
> + writel_relaxed(SFC_ICLR_RX_FULL | SFC_ICLR_RX_UFLOW |
> + SFC_ICLR_TX_OFLOW | SFC_ICLR_TX_EMPTY |
> + SFC_ICLR_TRAN_FINISH | SFC_ICLR_BUS_ERR |
> + SFC_ICLR_NSPI_ERR | SFC_ICLR_DMA,
> + sfc->regbase + SFC_ICLR);
> + return err;
> +}
> +
> +static int rockchip_sfc_init(struct rockchip_sfc *sfc)
> +{
> + int err;
> +
> + err = rockchip_sfc_reset(sfc);
> + if (err)
> + return err;
> +
> + /* Mask all eight interrupts */
> + writel_relaxed(0xff, sfc->regbase + SFC_IMR);
> + /* Phase configure */
What phase ? Please clarify the comment. Also, don't you have to
configure the register if sfc->negative_edge == 0 too ?
> + if (sfc->negative_edge)
> + writel_relaxed(SFC_CTRL_PHASE_SEL_NEGETIVE <<
> + SFC_CTRL_PHASE_SEL_SHIFT,
> + sfc->regbase + SFC_CTRL);
> + return 0;
> +}
> +
> +static int rockchip_sfc_prep(struct spi_nor *nor, enum spi_nor_ops ops)
> +{
> + struct rockchip_sfc_priv *priv = nor->priv;
> + struct rockchip_sfc *sfc = priv->sfc;
> + int ret;
> +
> + mutex_lock(&sfc->lock);
> +
> + ret = clk_set_rate(sfc->clk, priv->clk_rate);
> + if (ret)
> + goto out;
> +
> + ret = clk_prepare_enable(sfc->clk);
> + if (ret)
> + goto out;
> +
> + return 0;
> +
> +out:
> + mutex_unlock(&sfc->lock);
> + return ret;
> +}
> +
> +static void rockchip_sfc_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
> +{
> + struct rockchip_sfc_priv *priv = nor->priv;
> + struct rockchip_sfc *sfc = priv->sfc;
> +
> + clk_disable_unprepare(sfc->clk);
> + mutex_unlock(&sfc->lock);
> +}
> +
> +static int rockchip_sfc_wait_op_finish(struct rockchip_sfc *sfc)
> +{
> + unsigned long timeout = jiffies + 2 * HZ;
> + int err = -ETIMEDOUT;
> + u32 status;
> +
> + /*
> + * Note: tx and rx share the same fifo, so the rx's water level
> + * is the same as rx's, which means this function could be reused
> + * for checking the read operations as well.
> + */
> + while (time_before(jiffies, timeout)) {
readl_poll_*() ?
> + status = readl_relaxed(sfc->regbase + SFC_FSR);
> + if (((status >> SFC_FSR_TX_EMPTY_SHIFT) &
> + SFC_FSR_TX_EMPTY_MASK) == SFC_FSR_TX_IS_EMPTY) {
> + err = 0;
> + break;
> + }
> + msleep(20);
> + }
> +
> + if (err)
> + dev_err(sfc->dev, "SFC tx never empty\n");
> +
> + return err;
> +}
> +
> +static int rockchip_sfc_op_reg(struct spi_nor *nor,
> + u8 opcode, int len, u8 optype)
> +{
> + struct rockchip_sfc_priv *priv = nor->priv;
> + struct rockchip_sfc *sfc = priv->sfc;
> + u32 reg;
> +
> + if (((readl_relaxed(sfc->regbase + SFC_FSR) >> SFC_FSR_TX_EMPTY_SHIFT) &
> + SFC_FSR_TX_EMPTY_MASK) != SFC_FSR_TX_IS_EMPTY ||
> + ((readl_relaxed(sfc->regbase + SFC_FSR) >>
> + SFC_FSR_RX_EMPTY_SHIFT) &
> + SFC_FSR_RX_EMPTY_MASK) != SFC_FSR_RX_IS_EMPTY ||
> + (readl_relaxed(sfc->regbase + SFC_SR) == SFC_SR_IS_BUSY))
> + rockchip_sfc_reset(sfc);
This is chaos, please fix this condition so it's actually readable. You
can ie. read the FSR into a variable, do your shifting/anding magic and
then do if (var1 || var2 || var3) {} .
> + reg = (opcode & SFC_CMD_IDX_MASK) << SFC_CMD_IDX_SHIFT;
> + reg |= (len & SFC_CMD_TRAN_BYTES_MASK) << SFC_CMD_TRAN_BYTES_SHIFT;
> + reg |= (priv->cs & SFC_CMD_CS_MASK) << SFC_CMD_CS_SHIFT;
> + reg |= (optype & SFC_CMD_DIR_MASK) << SFC_CMD_DIR_SHIFT;
> +
> + writel_relaxed(reg, sfc->regbase + SFC_CMD);
> +
> + return rockchip_sfc_wait_op_finish(sfc);
> +}
> +
> +static int rockchip_sfc_read_reg(struct spi_nor *nor, u8 opcode,
> + u8 *buf, int len)
> +{
> + struct rockchip_sfc_priv *priv = nor->priv;
> + struct rockchip_sfc *sfc = priv->sfc;
> + int ret;
> + u32 tmp;
> + u32 i;
> +
> + ret = rockchip_sfc_op_reg(nor, opcode, len, SFC_CMD_DIR_RD);
> + if (ret)
> + return ret;
> +
> + while (len > 0) {
> + tmp = readl_relaxed(sfc->regbase + SFC_DATA);
> + for (i = 0; i < len; i++)
> + *buf++ = (u8)((tmp >> (i * 8)) & 0xff);
Won't this fail for len > 4 ?
Also, you can use ioread32_rep() here, but (!) that won't work for
unaligned reads, which I dunno if they can happen here, but please do
double-check.
> + len = len - 4;
> + }
> +
> + return 0;
> +}
> +
> +static int rockchip_sfc_write_reg(struct spi_nor *nor, u8 opcode,
> + u8 *buf, int len)
> +{
> + struct rockchip_sfc_priv *priv = nor->priv;
> + struct rockchip_sfc *sfc = priv->sfc;
> + u32 words, i;
> +
> + /* Align bytes to words */
> + words = (len + 3) >> 2;
> +
> + for (i = 0; i < words; i++)
> + writel_relaxed(*(buf + 4 * i), sfc->regbase + SFC_DATA);
See above about the ioread32_rep()/iowrite32_rep(), but careful about
unaligned (len % 4 != 0) case.
> + return rockchip_sfc_op_reg(nor, opcode, len, SFC_CMD_DIR_WR);
> +}
> +
> +static int rockchip_sfc_dma_transfer(struct spi_nor *nor, loff_t from_to,
> + dma_addr_t dma_buf, size_t len, u8 op_type)
> +{
> + struct rockchip_sfc_priv *priv = nor->priv;
> + struct rockchip_sfc *sfc = priv->sfc;
> + u32 reg;
> + u8 if_type = 0;
> +
> + init_completion(&sfc->cp);
> +
> + writel_relaxed(SFC_ICLR_RX_FULL | SFC_ICLR_RX_UFLOW |
> + SFC_ICLR_TX_OFLOW | SFC_ICLR_TX_EMPTY |
> + SFC_ICLR_TRAN_FINISH | SFC_ICLR_BUS_ERR |
> + SFC_ICLR_NSPI_ERR | SFC_ICLR_DMA,
> + sfc->regbase + SFC_ICLR);
> +
> + /* Enable transfer complete interrupt */
> + reg = readl_relaxed(sfc->regbase + SFC_IMR);
> + reg &= ~SFC_IMR_TRAN_FINISH;
> + writel_relaxed(reg, sfc->regbase + SFC_IMR);
> +
> + if (op_type == SFC_CMD_DIR_WR)
> + reg = (SFC_CMD_DIR_WR << SFC_CMD_DIR_SHIFT) |
> + ((nor->program_opcode & SFC_CMD_IDX_MASK) <<
> + SFC_CMD_IDX_SHIFT);
> + else
> + reg = (SFC_CMD_DIR_RD << SFC_CMD_DIR_SHIFT) |
> + ((nor->read_opcode & SFC_CMD_IDX_MASK) <<
> + SFC_CMD_IDX_SHIFT);
reg = nor->read_opcode & SFC_CMD_IDX_MASK) << SFC_CMD_IDX_SHIFT;
reg |= op_type << SFC_CMD_DIR_SHIFT;
> + reg |= ((nor->addr_width == 4) ? SFC_CMD_ADDR_32BITS
> + : SFC_CMD_ADDR_24BITS) << SFC_CMD_ADDR_SHIFT;
Why don't you just define those SFC_CMD_ADDR_24BITS and co. with the
shift in those bitfields already ? Then you wouldn't have to riddle this
driver with FOO << BAR, but you'd only have FOO all over the place.
> + if_type = get_if_type(nor->flash_read);
> + writel_relaxed(if_type << SFC_CTRL_DATA_BITS_SHIFT |
> + if_type << SFC_CTRL_ADDR_BITS_SHIFT |
> + if_type << SFC_CTRL_CMD_BITS_SHIFT |
Parenthesis missing around the statements ,
(if_type << FOO) | (... << bar)
> + sfc->negative_edge ?
> + SFC_CTRL_PHASE_SEL_NEGETIVE << SFC_CTRL_PHASE_SEL_SHIFT :
> + SFC_CTRL_PHASE_SEL_POSITIVE << SFC_CTRL_PHASE_SEL_SHIFT,
> + sfc->regbase + SFC_CTRL);
> +
> + reg |= (priv->cs & SFC_CMD_CS_MASK) << SFC_CMD_CS_SHIFT;
> + reg |= (len & SFC_CMD_TRAN_BYTES_MASK) << SFC_CMD_TRAN_BYTES_SHIFT;
> +
> + if (op_type == SFC_CMD_DIR_RD)
> + reg |= (nor->read_dummy & SFC_CMD_DUMMY_MASK) <<
> + SFC_CMD_DUMMY_SHIFT;
Just define SFC_CMD_DUMMY(x) \
(((x) & SFC_CMD_DUMMY_MASK) << SFC_CMD_DUMMY_SHIFT)
And then use it ... reg |= SFC_CMD_DUMMY(nor->read_dummy);
> + /* Should minus one as 0x0 means 1 bit flash address */
> + writel_relaxed(nor->addr_width * 8 - 1, sfc->regbase + SFC_ABIT);
> + writel_relaxed(reg, sfc->regbase + SFC_CMD);
> + writel_relaxed(from_to, sfc->regbase + SFC_ADDR);
> + writel_relaxed(dma_buf, sfc->regbase + SFC_DMA_ADDR);
I hope the DMA buffer management is implemented correctly and you're not
running into any weird cache issues.
> + /* Start dma */
> + writel_relaxed(0x1, sfc->regbase + SFC_DMA_TRIGGER);
> +
> + /* Wait for the interrupt. */
> + if (!wait_for_completion_timeout(&sfc->cp, msecs_to_jiffies(2000))) {
> + dev_err(sfc->dev, "DMA wait for transfer finish timeout.");
> + return -ETIMEDOUT;
> + }
> +
> + /* Disable transfer finish interrupt */
> + reg = readl_relaxed(sfc->regbase + SFC_IMR);
> + reg |= SFC_IMR_TRAN_FINISH;
> + writel_relaxed(reg, sfc->regbase + SFC_IMR);
> +
> + return rockchip_sfc_wait_op_finish(sfc);
> +}
> +
> +static inline int rockchip_sfc_pio_write(struct rockchip_sfc *sfc, u_char *buf,
> + size_t len)
> +{
> + u32 words, tx_wl, count, i;
> + unsigned long timeout;
> + int ret = 0;
> + u32 *tbuf = (u32 *)buf;
> +
> + /* Align bytes to words */
> + words = (len + 3) >> 2;
> +
> + while (words) {
See iowrite32_rep() above, but I suspect you'll run into problems with
$len which is not multiple of 4 .
> + tx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
> + SFC_FSR_TX_WATER_LVL_SHIFT) &
> + SFC_FSR_TX_WATER_LVL_MASK;
> +
> + if (tx_wl > 0) {
> + count = min_t(u32, words, tx_wl);
> + for (i = 0; i < count; i++) {
> + writel_relaxed(*tbuf++,
> + sfc->regbase + SFC_DATA);
> + words--;
> + }
> +
> + if (words == 0)
> + break;
> + timeout = 0;
> + } else {
> + mdelay(1);
> + if (timeout++ > SFC_MAX_IDLE_RETRY) {
> + ret = -ETIMEDOUT;
> + break;
> + }
> + }
> + }
> +
> + if (ret)
> + return ret;
> + else
> + return rockchip_sfc_wait_op_finish(sfc);
> +}
> +
> +static inline int rockchip_sfc_pio_read(struct rockchip_sfc *sfc, u_char *buf,
> + size_t len)
> +{
> + u32 words, rx_wl, count, i;
> + unsigned long timeout;
> + int ret = 0;
> + u32 tmp;
> + u32 *tbuf = (u32 *)buf;
> + u_char *tbuf2;
> +
> + words = len >> 2;
> + /* Get the remained bytes */
> + len = len & 0x3;
See above.
> + while (words) {
> + rx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
> + SFC_FSR_RX_WATER_LVL_SHIFT) &
> + SFC_FSR_RX_WATER_LVL_MASK;
> +
> + if (rx_wl > 0) {
> + count = min_t(u32, words, rx_wl);
> + for (i = 0; i < count; i++) {
> + *tbuf++ = readl_relaxed(sfc->regbase +
> + SFC_DATA);
> + words--;
> + }
> +
> + if (words == 0)
> + break;
> + timeout = 0;
> + } else {
> + mdelay(1);
> + if (timeout++ > SFC_MAX_IDLE_RETRY) {
> + ret = -ETIMEDOUT;
> + break;
> + }
> + }
> + }
> +
> + if (ret)
> + return ret;
> +
> + /* Read the remained bytes */
> + timeout = 0;
> + tbuf2 = (u_char *)tbuf;
> + while (len) {
> + rx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
> + SFC_FSR_RX_WATER_LVL_SHIFT) &
> + SFC_FSR_RX_WATER_LVL_MASK;
> + if (rx_wl > 0) {
> + tmp = readl_relaxed(sfc->regbase + SFC_DATA);
> + for (i = 0; i < len; i++)
> + tbuf2[i] = (u8)((tmp >> (i * 8)) & 0xff);
> + goto done;
> + } else {
> + mdelay(1);
> + if (timeout++ > SFC_MAX_IDLE_RETRY) {
> + ret = -ETIMEDOUT;
> + break;
> + }
> + }
> + }
> +done:
> + if (ret)
> + return ret;
> + else
> + return rockchip_sfc_wait_op_finish(sfc);
> +}
> +
> +static int rockchip_sfc_pio_transfer(struct spi_nor *nor, loff_t from_to,
> + size_t len, u_char *buf, u8 op_type)
> +{
> + struct rockchip_sfc_priv *priv = nor->priv;
> + struct rockchip_sfc *sfc = priv->sfc;
> + u32 reg;
> + u8 if_type = 0;
> +
> + if (op_type == SFC_CMD_DIR_WR)
> + reg = (SFC_CMD_DIR_WR << SFC_CMD_DIR_SHIFT) |
> + ((nor->program_opcode & SFC_CMD_IDX_MASK) <<
> + SFC_CMD_IDX_SHIFT);
> + else
> + reg = (SFC_CMD_DIR_RD << SFC_CMD_DIR_SHIFT) |
> + ((nor->read_opcode & SFC_CMD_IDX_MASK) <<
> + SFC_CMD_IDX_SHIFT);
See above regarding this condition. I think you can factor out this
common code too. Also nuke the bitshifts , see my comments on
rockchip_sfc_dma_transfer .
> + reg |= ((nor->addr_width == 4) ? SFC_CMD_ADDR_32BITS
> + : SFC_CMD_ADDR_24BITS) << SFC_CMD_ADDR_SHIFT;
> +
> + if_type = get_if_type(nor->flash_read);
> + writel_relaxed(if_type << SFC_CTRL_DATA_BITS_SHIFT |
> + if_type << SFC_CTRL_ADDR_BITS_SHIFT |
> + if_type << SFC_CTRL_CMD_BITS_SHIFT |
> + sfc->negative_edge ?
> + SFC_CTRL_PHASE_SEL_NEGETIVE << SFC_CTRL_PHASE_SEL_SHIFT :
> + SFC_CTRL_PHASE_SEL_POSITIVE << SFC_CTRL_PHASE_SEL_SHIFT,
> + sfc->regbase + SFC_CTRL);
> +
> + reg |= (priv->cs & SFC_CMD_CS_MASK) << SFC_CMD_CS_SHIFT;
> + reg |= (len & SFC_CMD_TRAN_BYTES_MASK) << SFC_CMD_TRAN_BYTES_SHIFT;
> +
> + if (op_type == SFC_CMD_DIR_RD)
> + reg |= (nor->read_dummy & SFC_CMD_DUMMY_MASK) <<
> + SFC_CMD_DUMMY_SHIFT;
> +
> + /* Should minus one as 0x0 means 1 bit flash address */
> + writel_relaxed(nor->addr_width * 8 - 1, sfc->regbase + SFC_ABIT);
> + writel_relaxed(reg, sfc->regbase + SFC_CMD);
> + writel_relaxed(from_to, sfc->regbase + SFC_ADDR);
> +
> + if (op_type == SFC_CMD_DIR_WR)
> + return rockchip_sfc_pio_write(sfc, buf, len);
> + else
> + return rockchip_sfc_pio_read(sfc, buf, len);
> +}
> +
> +static ssize_t rockchip_sfc_read(struct spi_nor *nor, loff_t from, size_t len,
> + u_char *read_buf)
> +{
> + struct rockchip_sfc_priv *priv = nor->priv;
> + struct rockchip_sfc *sfc = priv->sfc;
> + size_t offset;
> + int ret;
> + dma_addr_t dma_addr = 0;
> +
> + if (!sfc->use_dma)
> + goto no_dma;
> +
> + for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
> + size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
> +
> + dma_addr = dma_map_single(NULL, (void *)read_buf,
> + trans, DMA_FROM_DEVICE);
> + if (dma_mapping_error(sfc->dev, dma_addr))
> + dma_addr = 0;
> +
> + /* Fail to map dma, use pre-allocated area instead */
> + ret = rockchip_sfc_dma_transfer(nor, from + offset,
> + dma_addr ? dma_addr :
> + sfc->dma_buffer,
> + trans, SFC_CMD_DIR_RD);
> + if (ret) {
> + dev_warn(nor->dev, "DMA read timeout\n");
> + return ret;
> + }
> + if (!dma_addr)
> + memcpy(read_buf + offset, sfc->buffer, trans);
> + }
> +
> + return len;
> +
> +no_dma:
> + ret = rockchip_sfc_pio_transfer(nor, from, len,
> + read_buf, SFC_CMD_DIR_RD);
> + if (ret) {
> + dev_warn(nor->dev, "PIO read timeout\n");
> + return ret;
> + }
> + return len;
> +}
> +
> +static ssize_t rockchip_sfc_write(struct spi_nor *nor, loff_t to,
> + size_t len, const u_char *write_buf)
> +{
> + struct rockchip_sfc_priv *priv = nor->priv;
> + struct rockchip_sfc *sfc = priv->sfc;
> + size_t offset;
> + int ret;
> + dma_addr_t dma_addr = 0;
> +
> + if (!sfc->use_dma)
> + goto no_dma;
Seems like there's a lot of similarity between read/write .
> + for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
> + size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
> +
> + dma_addr = dma_map_single(NULL, (void *)write_buf,
> + trans, DMA_TO_DEVICE);
> + if (dma_mapping_error(sfc->dev, dma_addr)) {
> + dma_addr = 0;
> + memcpy(sfc->buffer, write_buf + offset, trans);
> + }
> +
> + /* Fail to map dma, use pre-allocated area instead */
> + ret = rockchip_sfc_dma_transfer(nor, to + offset,
> + dma_addr ? dma_addr :
> + sfc->dma_buffer,
> + trans, SFC_CMD_DIR_WR);
> + if (dma_addr)
> + dma_unmap_single(NULL, dma_addr,
> + trans, DMA_TO_DEVICE);
> + if (ret) {
> + dev_warn(nor->dev, "DMA write timeout\n");
> + return ret;
> + }
> + }
> +
> + return len;
> +no_dma:
> + ret = rockchip_sfc_pio_transfer(nor, to, len,
> + (u_char *)write_buf, SFC_CMD_DIR_WR);
> + if (ret) {
> + dev_warn(nor->dev, "PIO write timeout\n");
> + return ret;
> + }
> + return len;
> +}
> +
> +/**
> + * Get spi flash device information and register it as a mtd device.
> + */
> +static int rockchip_sfc_register(struct device_node *np,
> + struct rockchip_sfc *sfc)
> +{
> + struct device *dev = sfc->dev;
> + struct spi_nor *nor;
> + struct rockchip_sfc_priv *priv;
> + struct mtd_info *mtd;
> + int ret;
> +
> + nor = devm_kzalloc(dev, sizeof(*nor), GFP_KERNEL);
> + if (!nor)
> + return -ENOMEM;
You can embed struct spi_nor in struct rockchip_sfc_priv and drop this
allocation . Also it'd be a good idea to rename rockchip_sfc_priv to
something like rockchip_sfc_chip_priv to make it explicit this is a
per-chip private data -- which you can even pre-allocate in rockchi_sfc
structure as a static array of (four) such structures (see cadence qspi
driver for how this is done there).
> + nor->dev = dev;
> + spi_nor_set_flash_node(nor, np);
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + ret = of_property_read_u32(np, "reg", &priv->cs);
> + if (ret) {
> + dev_err(dev, "No reg property for %s\n",
> + np->full_name);
> + return ret;
> + }
> +
> + ret = of_property_read_u32(np, "spi-max-frequency",
> + &priv->clk_rate);
> + if (ret) {
> + dev_err(dev, "No spi-max-frequency property for %s\n",
> + np->full_name);
> + return ret;
> + }
> +
> + priv->sfc = sfc;
> + nor->priv = priv;
> +
> + nor->prepare = rockchip_sfc_prep;
> + nor->unprepare = rockchip_sfc_unprep;
> + nor->read_reg = rockchip_sfc_read_reg;
> + nor->write_reg = rockchip_sfc_write_reg;
> + nor->read = rockchip_sfc_read;
> + nor->write = rockchip_sfc_write;
> + nor->erase = NULL;
> + ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
> + if (ret)
> + return ret;
> +
> + mtd = &nor->mtd;
> + mtd->name = np->name;
> + ret = mtd_device_register(mtd, NULL, 0);
> + if (ret)
> + return ret;
> +
> + sfc->nor[sfc->num_chip] = nor;
> + sfc->num_chip++;
> + return 0;
> +}
> +
> +static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
> +{
> + int i;
> +
> + for (i = 0; i < sfc->num_chip; i++)
> + mtd_device_unregister(&sfc->nor[i]->mtd);
> +}
> +
> +static int rockchip_sfc_register_all(struct rockchip_sfc *sfc)
> +{
> + struct device *dev = sfc->dev;
> + struct device_node *np;
> + int ret;
> +
> + for_each_available_child_of_node(dev->of_node, np) {
> + ret = rockchip_sfc_register(np, sfc);
> + if (ret)
> + goto fail;
> +
> + if (sfc->num_chip == SFC_MAX_CHIP_NUM) {
> + dev_warn(dev, "Exceeds the max cs limitation\n");
> + break;
> + }
> + }
> +
> + return 0;
> +
> +fail:
> + dev_err(dev, "Failed to register all chip\n");
> + rockchip_sfc_unregister_all(sfc);
See cadence qspi where we only unregister the registered flashes.
Implement it the same way here.
> + return ret;
> +}
> +
> +static irqreturn_t rockchip_sfc_irq_handler(int irq, void *dev_id)
> +{
> + struct rockchip_sfc *sfc = dev_id;
> + u32 reg;
> +
> + reg = readl_relaxed(sfc->regbase + SFC_RISR);
> + dev_dbg(sfc->dev, "Get irq: 0x%x\n", reg);
> +
> + /* Clear interrupt */
> + writel_relaxed(reg, sfc->regbase + SFC_ICLR);
> +
> + if (reg & SFC_IRQ_TRAN_FINISH)
> + complete(&sfc->cp);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int rockchip_sfc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> + struct rockchip_sfc *sfc;
> + int ret;
> +
> + sfc = devm_kzalloc(dev, sizeof(*sfc), GFP_KERNEL);
> + if (!sfc)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, sfc);
> + sfc->dev = dev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + sfc->regbase = devm_ioremap_resource(dev, res);
> + if (IS_ERR(sfc->regbase))
> + return PTR_ERR(sfc->regbase);
> +
> + sfc->clk = devm_clk_get(&pdev->dev, "sfc");
> + if (IS_ERR(sfc->clk)) {
> + dev_err(&pdev->dev, "Failed to get sfc interface clk\n");
> + return PTR_ERR(sfc->clk);
> + }
> +
> + sfc->hclk = devm_clk_get(&pdev->dev, "hsfc");
> + if (IS_ERR(sfc->hclk)) {
> + dev_err(&pdev->dev, "Failed to get sfc ahp clk\n");
> + return PTR_ERR(sfc->hclk);
> + }
> +
> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> + if (ret) {
> + dev_warn(dev, "Unable to set dma mask\n");
> + return ret;
> + }
> +
> + sfc->buffer = dmam_alloc_coherent(dev, SFC_DMA_MAX_LEN,
> + &sfc->dma_buffer, GFP_KERNEL);
> + if (!sfc->buffer)
> + return -ENOMEM;
> +
> + mutex_init(&sfc->lock);
> +
> + ret = clk_prepare_enable(sfc->hclk);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to enable hclk\n");
> + goto err_hclk;
> + }
> +
> + ret = clk_prepare_enable(sfc->clk);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to enable clk\n");
> + goto err_clk;
> + }
> +
> + if (of_property_read_bool(sfc->dev->of_node, "rockchip,sfc-no-dma"))
> + sfc->use_dma = false;
> + else
> + sfc->use_dma = true;
sfc->use_dma = !of_property_read_bool(sfc->dev->of_node,
"rockchip,sfc-no-dma");
> + if (of_device_is_compatible(sfc->dev->of_node,
> + "rockchip,rk1108-sfc"))
> + sfc->negative_edge = true;
> + else
> + sfc->negative_edge = false;
See above
> + /* Find the irq */
> + ret = platform_get_irq(pdev, 0);
> + if (ret < 0) {
> + dev_err(dev, "Failed to get the irq\n");
> + goto err_irq;
> + }
> +
> + ret = devm_request_irq(dev, ret, rockchip_sfc_irq_handler,
> + 0, pdev->name, sfc);
> + if (ret) {
> + dev_err(dev, "Failed to request irq\n");
> + goto err_irq;
> + }
> +
> + ret = rockchip_sfc_init(sfc);
> + if (ret)
> + goto err_init;
> +
> + ret = rockchip_sfc_register_all(sfc);
> + if (ret)
> + goto err_init;
> +
> + clk_disable_unprepare(sfc->clk);
> + return 0;
> +
> +err_irq:
> +err_init:
Drop the err_irq: label unless you plan to handle the error (which you
should).
> + clk_disable_unprepare(sfc->clk);
> +err_clk:
> + clk_disable_unprepare(sfc->hclk);
> +err_hclk:
> + mutex_destroy(&sfc->lock);
> + return ret;
> +}
> +
> +static int rockchip_sfc_remove(struct platform_device *pdev)
> +{
> + struct rockchip_sfc *sfc = platform_get_drvdata(pdev);
> +
> + rockchip_sfc_unregister_all(sfc);
> + mutex_destroy(&sfc->lock);
> + clk_disable_unprepare(sfc->clk);
> + clk_disable_unprepare(sfc->hclk);
> + return 0;
> +}
> +
> +static const struct of_device_id rockchip_sfc_dt_ids[] = {
> + { .compatible = "rockchip,sfc"},
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, rockchip_sfc_dt_ids);
> +
> +static struct platform_driver rockchip_sfc_driver = {
> + .driver = {
> + .name = "rockchip-sfc",
> + .of_match_table = rockchip_sfc_dt_ids,
> + },
> + .probe = rockchip_sfc_probe,
> + .remove = rockchip_sfc_remove,
> +};
> +module_platform_driver(rockchip_sfc_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Rockchip Serial Flash Controller Driver");
MODULE_AUTHOR is missing
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nor: add rockchip serial flash controller driver
2016-11-15 20:52 ` Marek Vasut
@ 2016-11-16 1:59 ` Shawn Lin
-1 siblings, 0 replies; 20+ messages in thread
From: Shawn Lin @ 2016-11-16 1:59 UTC (permalink / raw)
To: Marek Vasut, Rob Herring, David Woodhouse, Brian Norris
Cc: Shawn Lin, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Hi Marek,
Thanks for reviewing my patch, and it's really
helpful to improve it. :) See my feedback for
your comments below.
On 2016/11/16 4:52, Marek Vasut wrote:
> On 11/11/2016 10:16 AM, Shawn Lin wrote:
>> Add rockchip serial flash controller driver
>>
>> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>
>
> [...]
>
>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index 4a682ee..48c5e0e 100644
>> --- a/drivers/mtd/spi-nor/Kconfig
>> +++ b/drivers/mtd/spi-nor/Kconfig
>> @@ -65,6 +65,13 @@ config SPI_HISI_SFC
>> help
>> This enables support for hisilicon SPI-NOR flash controller.
>>
>> +config SPI_ROCKCHIP_SFC
>
> Keep this list sorted please.
yup, will fix.
>
>> + tristate "Rockchip Serial Flash Controller(SFC)"
>> + depends on ARCH_ROCKCHIP || COMPILE_TEST
>> + depends on HAS_IOMEM && HAS_DMA
>> + help
>> + This enables support for rockchip serial flash controller.
>> +
>> config SPI_NXP_SPIFI
>> tristate "NXP SPI Flash Interface (SPIFI)"
>> depends on OF && (ARCH_LPC18XX || COMPILE_TEST)
>
>
> [...]
>
>> +/* Interrypt mask */
>
> Interrupt :)
will fix. :)
>
>> +#define SFC_IMR 0x4
>> +#define SFC_IMR_RX_FULL BIT(0)
>> +#define SFC_IMR_RX_UFLOW BIT(1)
>> +#define SFC_IMR_TX_OFLOW BIT(2)
>> +#define SFC_IMR_TX_EMPTY BIT(3)
>> +#define SFC_IMR_TRAN_FINISH BIT(4)
>> +#define SFC_IMR_BUS_ERR BIT(5)
>> +#define SFC_IMR_NSPI_ERR BIT(6)
>> +#define SFC_IMR_DMA BIT(7)
>
>
> [...]
>
>> +enum rockchip_sfc_iftype {
>> + IF_TYPE_STD,
>> + IF_TYPE_DUAL,
>> + IF_TYPE_QUAD,
>> +};
>> +
>> +struct rockchip_sfc {
>> + struct device *dev;
>> + struct mutex lock;
>> + void __iomem *regbase;
>> + struct clk *hclk;
>> + struct clk *clk;
>> + void *buffer;
>> + dma_addr_t dma_buffer;
>
> The naming (buffer) could use some improvement or comment for clarification.
>
>> + struct completion cp;
>> + struct spi_nor *nor[SFC_MAX_CHIP_NUM];
>
> Should be MAX_CHIPSELECT_NUM , for clarity.
seems sane, will fix.
>
>> + u32 num_chip;
>
> u8
>
>> + bool use_dma;
>> + bool negative_edge;
>
> Negative edge ... of what ?
For how the inner sample logic to letch the data. It should be
configured differently for different Socs. I will add a comment
here to clarify it.
>
>> +};
>> +
>> +struct rockchip_sfc_priv {
>> + u32 cs;
>
> Doesn't this board support only 4 CS ? Use u8 :-)
>
good catch, will fix it.
>> + u32 clk_rate;
>> + struct rockchip_sfc *sfc;
>> +};
>> +
>> +static int get_if_type(enum read_mode flash_read)
>> +{
>> + enum rockchip_sfc_iftype if_type;
>> +
>> + switch (flash_read) {
>> + case SPI_NOR_DUAL:
>> + if_type = IF_TYPE_DUAL;
>> + break;
>> + case SPI_NOR_QUAD:
>> + if_type = IF_TYPE_QUAD;
>> + break;
>> + case SPI_NOR_NORMAL:
>> + case SPI_NOR_FAST:
>> + default:
>
> Should the default case really fall back to 1-bit mode or should it
> rather report error ?
It's derived from nor->flash_read, so personlly I think there should
never fall into the default case, but it would make sense to return
-EINVAL instead of falling into 1-bit mode. I will fix it.
>
>> + if_type = IF_TYPE_STD;
>> + break;
>> + }
>> +
>> + return if_type;
>> +}
>> +
>> +static int rockchip_sfc_reset(struct rockchip_sfc *sfc)
>> +{
>> + unsigned long timeout = jiffies + HZ;
>> + int err = -ETIMEDOUT;
>> + u32 status;
>> +
>> + writel_relaxed(SFC_RCVR_RESET, sfc->regbase + SFC_RCVR);
>> +
>> + while (time_before(jiffies, timeout)) {
>
> Would readl_poll_*() from include/linux/iopoll.h help here ?
>
Brilliant, it looks great to me.
>> + status = readl_relaxed(sfc->regbase + SFC_RCVR);
>> + if (!(status & SFC_RCVR_RESET)) {
>> + err = 0;
>> + break;
>> + }
>> + msleep(20);
>> + }
>> +
>> + if (err)
>> + dev_err(sfc->dev, "SFC reset never finished\n");
>
> Should the writel() below be executed if an error happened ?
It's needed since when doing reset after failing to finish
a previous transfer for whatever reason, we could observed some of the
interrupts triggered. Although we masked them but we could still
find them from the raw interrupt status register. So cleaning
it explicitly make senses.
>
>> + writel_relaxed(SFC_ICLR_RX_FULL | SFC_ICLR_RX_UFLOW |
>> + SFC_ICLR_TX_OFLOW | SFC_ICLR_TX_EMPTY |
>> + SFC_ICLR_TRAN_FINISH | SFC_ICLR_BUS_ERR |
>> + SFC_ICLR_NSPI_ERR | SFC_ICLR_DMA,
>> + sfc->regbase + SFC_ICLR);
>> + return err;
>> +}
>> +
>> +static int rockchip_sfc_init(struct rockchip_sfc *sfc)
>> +{
>> + int err;
>> +
>> + err = rockchip_sfc_reset(sfc);
>> + if (err)
>> + return err;
>> +
>> + /* Mask all eight interrupts */
>> + writel_relaxed(0xff, sfc->regbase + SFC_IMR);
>> + /* Phase configure */
>
> What phase ? Please clarify the comment. Also, don't you have to
> configure the register if sfc->negative_edge == 0 too ?
will elaborate more.
Your comment makes me think it twice. The loader should
already set this but we could override it agian in case
of some other code clear it. I was assuming that the reset
value of it meets what we need for "sfc->negative_edge == 0",
but I think I am wrong since we still need to prevent some other
whatever code modified it before jumping into rockchip_sfc_probe.
So yes, I will fix it. :)
>
>> + if (sfc->negative_edge)
>> + writel_relaxed(SFC_CTRL_PHASE_SEL_NEGETIVE <<
>> + SFC_CTRL_PHASE_SEL_SHIFT,
>> + sfc->regbase + SFC_CTRL);
>> + return 0;
>> +}
>> +
>> +static int rockchip_sfc_prep(struct spi_nor *nor, enum spi_nor_ops ops)
>> +{
>> + struct rockchip_sfc_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + int ret;
>> +
>> + mutex_lock(&sfc->lock);
>> +
>> + ret = clk_set_rate(sfc->clk, priv->clk_rate);
>> + if (ret)
>> + goto out;
>> +
>> + ret = clk_prepare_enable(sfc->clk);
>> + if (ret)
>> + goto out;
>> +
>> + return 0;
>> +
>> +out:
>> + mutex_unlock(&sfc->lock);
>> + return ret;
>> +}
>> +
>> +static void rockchip_sfc_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
>> +{
>> + struct rockchip_sfc_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> +
>> + clk_disable_unprepare(sfc->clk);
>> + mutex_unlock(&sfc->lock);
>> +}
>> +
>> +static int rockchip_sfc_wait_op_finish(struct rockchip_sfc *sfc)
>> +{
>> + unsigned long timeout = jiffies + 2 * HZ;
>> + int err = -ETIMEDOUT;
>> + u32 status;
>> +
>> + /*
>> + * Note: tx and rx share the same fifo, so the rx's water level
>> + * is the same as rx's, which means this function could be reused
>> + * for checking the read operations as well.
>> + */
>> + while (time_before(jiffies, timeout)) {
>
> readl_poll_*() ?
sure.
>
>> + status = readl_relaxed(sfc->regbase + SFC_FSR);
>> + if (((status >> SFC_FSR_TX_EMPTY_SHIFT) &
>> + SFC_FSR_TX_EMPTY_MASK) == SFC_FSR_TX_IS_EMPTY) {
>> + err = 0;
>> + break;
>> + }
>> + msleep(20);
>> + }
>> +
>> + if (err)
>> + dev_err(sfc->dev, "SFC tx never empty\n");
>> +
>> + return err;
>> +}
>> +
>> +static int rockchip_sfc_op_reg(struct spi_nor *nor,
>> + u8 opcode, int len, u8 optype)
>> +{
>> + struct rockchip_sfc_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + u32 reg;
>> +
>> + if (((readl_relaxed(sfc->regbase + SFC_FSR) >> SFC_FSR_TX_EMPTY_SHIFT) &
>> + SFC_FSR_TX_EMPTY_MASK) != SFC_FSR_TX_IS_EMPTY ||
>> + ((readl_relaxed(sfc->regbase + SFC_FSR) >>
>> + SFC_FSR_RX_EMPTY_SHIFT) &
>> + SFC_FSR_RX_EMPTY_MASK) != SFC_FSR_RX_IS_EMPTY ||
>> + (readl_relaxed(sfc->regbase + SFC_SR) == SFC_SR_IS_BUSY))
>> + rockchip_sfc_reset(sfc);
>
> This is chaos, please fix this condition so it's actually readable. You
> can ie. read the FSR into a variable, do your shifting/anding magic and
> then do if (var1 || var2 || var3) {} .
okay, will make it more clearly.
>
>> + reg = (opcode & SFC_CMD_IDX_MASK) << SFC_CMD_IDX_SHIFT;
>> + reg |= (len & SFC_CMD_TRAN_BYTES_MASK) << SFC_CMD_TRAN_BYTES_SHIFT;
>> + reg |= (priv->cs & SFC_CMD_CS_MASK) << SFC_CMD_CS_SHIFT;
>> + reg |= (optype & SFC_CMD_DIR_MASK) << SFC_CMD_DIR_SHIFT;
>> +
>> + writel_relaxed(reg, sfc->regbase + SFC_CMD);
>> +
>> + return rockchip_sfc_wait_op_finish(sfc);
>> +}
>> +
>> +static int rockchip_sfc_read_reg(struct spi_nor *nor, u8 opcode,
>> + u8 *buf, int len)
>> +{
>> + struct rockchip_sfc_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + int ret;
>> + u32 tmp;
>> + u32 i;
>> +
>> + ret = rockchip_sfc_op_reg(nor, opcode, len, SFC_CMD_DIR_RD);
>> + if (ret)
>> + return ret;
>> +
>> + while (len > 0) {
>> + tmp = readl_relaxed(sfc->regbase + SFC_DATA);
>> + for (i = 0; i < len; i++)
>> + *buf++ = (u8)((tmp >> (i * 8)) & 0xff);
>
> Won't this fail for len > 4 ?
nope, this loop will reduce 4 for each successful readl. And
reading the remained bytes which isn't aligned to DWORD, isn't it?
>
> Also, you can use ioread32_rep() here, but (!) that won't work for
> unaligned reads, which I dunno if they can happen here, but please do
> double-check.
yes, I have checked this API as well as others like memcpy_{to,from}io
, etc. They will generate a external abort for arm core as the unaligned
(DWORD) read/write via AHB aren't supported by Rockchip Socs. So I have
to open code these stuff. This could be easily found for other
upstreamed rockchip drivers. :)
>
>> + len = len - 4;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int rockchip_sfc_write_reg(struct spi_nor *nor, u8 opcode,
>> + u8 *buf, int len)
>> +{
>> + struct rockchip_sfc_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + u32 words, i;
>> +
>> + /* Align bytes to words */
>> + words = (len + 3) >> 2;
>> +
>> + for (i = 0; i < words; i++)
>> + writel_relaxed(*(buf + 4 * i), sfc->regbase + SFC_DATA);
>
> See above about the ioread32_rep()/iowrite32_rep(), but careful about
> unaligned (len % 4 != 0) case.
Ditto for above. :)
>
>> + return rockchip_sfc_op_reg(nor, opcode, len, SFC_CMD_DIR_WR);
>> +}
>> +
>> +static int rockchip_sfc_dma_transfer(struct spi_nor *nor, loff_t from_to,
>> + dma_addr_t dma_buf, size_t len, u8 op_type)
>> +{
>> + struct rockchip_sfc_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + u32 reg;
>> + u8 if_type = 0;
>> +
>> + init_completion(&sfc->cp);
>> +
>> + writel_relaxed(SFC_ICLR_RX_FULL | SFC_ICLR_RX_UFLOW |
>> + SFC_ICLR_TX_OFLOW | SFC_ICLR_TX_EMPTY |
>> + SFC_ICLR_TRAN_FINISH | SFC_ICLR_BUS_ERR |
>> + SFC_ICLR_NSPI_ERR | SFC_ICLR_DMA,
>> + sfc->regbase + SFC_ICLR);
>> +
>> + /* Enable transfer complete interrupt */
>> + reg = readl_relaxed(sfc->regbase + SFC_IMR);
>> + reg &= ~SFC_IMR_TRAN_FINISH;
>> + writel_relaxed(reg, sfc->regbase + SFC_IMR);
>> +
>> + if (op_type == SFC_CMD_DIR_WR)
>> + reg = (SFC_CMD_DIR_WR << SFC_CMD_DIR_SHIFT) |
>> + ((nor->program_opcode & SFC_CMD_IDX_MASK) <<
>> + SFC_CMD_IDX_SHIFT);
>> + else
>> + reg = (SFC_CMD_DIR_RD << SFC_CMD_DIR_SHIFT) |
>> + ((nor->read_opcode & SFC_CMD_IDX_MASK) <<
>> + SFC_CMD_IDX_SHIFT);
>
> reg = nor->read_opcode & SFC_CMD_IDX_MASK) << SFC_CMD_IDX_SHIFT;
> reg |= op_type << SFC_CMD_DIR_SHIFT;
will improve.
>
>> + reg |= ((nor->addr_width == 4) ? SFC_CMD_ADDR_32BITS
>> + : SFC_CMD_ADDR_24BITS) << SFC_CMD_ADDR_SHIFT;
>
> Why don't you just define those SFC_CMD_ADDR_24BITS and co. with the
> shift in those bitfields already ? Then you wouldn't have to riddle this
> driver with FOO << BAR, but you'd only have FOO all over the place.
>
sounds good, will improve them..
>> + if_type = get_if_type(nor->flash_read);
>> + writel_relaxed(if_type << SFC_CTRL_DATA_BITS_SHIFT |
>> + if_type << SFC_CTRL_ADDR_BITS_SHIFT |
>> + if_type << SFC_CTRL_CMD_BITS_SHIFT |
>
> Parenthesis missing around the statements ,
> (if_type << FOO) | (... << bar)
>
will improve it.
>> + sfc->negative_edge ?
>> + SFC_CTRL_PHASE_SEL_NEGETIVE << SFC_CTRL_PHASE_SEL_SHIFT :
>> + SFC_CTRL_PHASE_SEL_POSITIVE << SFC_CTRL_PHASE_SEL_SHIFT,
>> + sfc->regbase + SFC_CTRL);
>> +
>> + reg |= (priv->cs & SFC_CMD_CS_MASK) << SFC_CMD_CS_SHIFT;
>> + reg |= (len & SFC_CMD_TRAN_BYTES_MASK) << SFC_CMD_TRAN_BYTES_SHIFT;
>> +
>> + if (op_type == SFC_CMD_DIR_RD)
>> + reg |= (nor->read_dummy & SFC_CMD_DUMMY_MASK) <<
>> + SFC_CMD_DUMMY_SHIFT;
>
> Just define SFC_CMD_DUMMY(x) \
> (((x) & SFC_CMD_DUMMY_MASK) << SFC_CMD_DUMMY_SHIFT)
>
> And then use it ... reg |= SFC_CMD_DUMMY(nor->read_dummy);
sure.
>
>> + /* Should minus one as 0x0 means 1 bit flash address */
>> + writel_relaxed(nor->addr_width * 8 - 1, sfc->regbase + SFC_ABIT);
>> + writel_relaxed(reg, sfc->regbase + SFC_CMD);
>> + writel_relaxed(from_to, sfc->regbase + SFC_ADDR);
>> + writel_relaxed(dma_buf, sfc->regbase + SFC_DMA_ADDR);
>
> I hope the DMA buffer management is implemented correctly and you're not
> running into any weird cache issues.
you are right. I should sync it after doing read and before doing write.
>
>> + /* Start dma */
>> + writel_relaxed(0x1, sfc->regbase + SFC_DMA_TRIGGER);
>> +
>> + /* Wait for the interrupt. */
>> + if (!wait_for_completion_timeout(&sfc->cp, msecs_to_jiffies(2000))) {
>> + dev_err(sfc->dev, "DMA wait for transfer finish timeout.");
>> + return -ETIMEDOUT;
>> + }
>> +
>> + /* Disable transfer finish interrupt */
>> + reg = readl_relaxed(sfc->regbase + SFC_IMR);
>> + reg |= SFC_IMR_TRAN_FINISH;
>> + writel_relaxed(reg, sfc->regbase + SFC_IMR);
>> +
>> + return rockchip_sfc_wait_op_finish(sfc);
>> +}
>> +
>> +static inline int rockchip_sfc_pio_write(struct rockchip_sfc *sfc, u_char *buf,
>> + size_t len)
>> +{
>> + u32 words, tx_wl, count, i;
>> + unsigned long timeout;
>> + int ret = 0;
>> + u32 *tbuf = (u32 *)buf;
>> +
>> + /* Align bytes to words */
>> + words = (len + 3) >> 2;
>> +
>> + while (words) {
>
> See iowrite32_rep() above, but I suspect you'll run into problems with
> $len which is not multiple of 4 .
>
Ditto for above.
>> + tx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
>> + SFC_FSR_TX_WATER_LVL_SHIFT) &
>> + SFC_FSR_TX_WATER_LVL_MASK;
>> +
>> + if (tx_wl > 0) {
>> + count = min_t(u32, words, tx_wl);
>> + for (i = 0; i < count; i++) {
>> + writel_relaxed(*tbuf++,
>> + sfc->regbase + SFC_DATA);
>> + words--;
>> + }
>> +
>> + if (words == 0)
>> + break;
>> + timeout = 0;
>> + } else {
>> + mdelay(1);
>> + if (timeout++ > SFC_MAX_IDLE_RETRY) {
>> + ret = -ETIMEDOUT;
>> + break;
>> + }
>> + }
>> + }
>> +
>> + if (ret)
>> + return ret;
>> + else
>> + return rockchip_sfc_wait_op_finish(sfc);
>> +}
>> +
>> +static inline int rockchip_sfc_pio_read(struct rockchip_sfc *sfc, u_char *buf,
>> + size_t len)
>> +{
>> + u32 words, rx_wl, count, i;
>> + unsigned long timeout;
>> + int ret = 0;
>> + u32 tmp;
>> + u32 *tbuf = (u32 *)buf;
>> + u_char *tbuf2;
>> +
>> + words = len >> 2;
>> + /* Get the remained bytes */
>> + len = len & 0x3;
>
> See above.
>
>> + while (words) {
>> + rx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
>> + SFC_FSR_RX_WATER_LVL_SHIFT) &
>> + SFC_FSR_RX_WATER_LVL_MASK;
>> +
>> + if (rx_wl > 0) {
>> + count = min_t(u32, words, rx_wl);
>> + for (i = 0; i < count; i++) {
>> + *tbuf++ = readl_relaxed(sfc->regbase +
>> + SFC_DATA);
>> + words--;
>> + }
>> +
>> + if (words == 0)
>> + break;
>> + timeout = 0;
>> + } else {
>> + mdelay(1);
>> + if (timeout++ > SFC_MAX_IDLE_RETRY) {
>> + ret = -ETIMEDOUT;
>> + break;
>> + }
>> + }
>> + }
>> +
>> + if (ret)
>> + return ret;
>> +
>> + /* Read the remained bytes */
>> + timeout = 0;
>> + tbuf2 = (u_char *)tbuf;
>> + while (len) {
>> + rx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
>> + SFC_FSR_RX_WATER_LVL_SHIFT) &
>> + SFC_FSR_RX_WATER_LVL_MASK;
>> + if (rx_wl > 0) {
>> + tmp = readl_relaxed(sfc->regbase + SFC_DATA);
>> + for (i = 0; i < len; i++)
>> + tbuf2[i] = (u8)((tmp >> (i * 8)) & 0xff);
>> + goto done;
>> + } else {
>> + mdelay(1);
>> + if (timeout++ > SFC_MAX_IDLE_RETRY) {
>> + ret = -ETIMEDOUT;
>> + break;
>> + }
>> + }
>> + }
>> +done:
>> + if (ret)
>> + return ret;
>> + else
>> + return rockchip_sfc_wait_op_finish(sfc);
>> +}
>> +
>> +static int rockchip_sfc_pio_transfer(struct spi_nor *nor, loff_t from_to,
>> + size_t len, u_char *buf, u8 op_type)
>> +{
>> + struct rockchip_sfc_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + u32 reg;
>> + u8 if_type = 0;
>> +
>> + if (op_type == SFC_CMD_DIR_WR)
>> + reg = (SFC_CMD_DIR_WR << SFC_CMD_DIR_SHIFT) |
>> + ((nor->program_opcode & SFC_CMD_IDX_MASK) <<
>> + SFC_CMD_IDX_SHIFT);
>> + else
>> + reg = (SFC_CMD_DIR_RD << SFC_CMD_DIR_SHIFT) |
>> + ((nor->read_opcode & SFC_CMD_IDX_MASK) <<
>> + SFC_CMD_IDX_SHIFT);
>
> See above regarding this condition. I think you can factor out this
> common code too. Also nuke the bitshifts , see my comments on
> rockchip_sfc_dma_transfer .
>
sure. Will fix the bitshifts and factor out the common code.
>> + reg |= ((nor->addr_width == 4) ? SFC_CMD_ADDR_32BITS
>> + : SFC_CMD_ADDR_24BITS) << SFC_CMD_ADDR_SHIFT;
>> +
>> + if_type = get_if_type(nor->flash_read);
>> + writel_relaxed(if_type << SFC_CTRL_DATA_BITS_SHIFT |
>> + if_type << SFC_CTRL_ADDR_BITS_SHIFT |
>> + if_type << SFC_CTRL_CMD_BITS_SHIFT |
>> + sfc->negative_edge ?
>> + SFC_CTRL_PHASE_SEL_NEGETIVE << SFC_CTRL_PHASE_SEL_SHIFT :
>> + SFC_CTRL_PHASE_SEL_POSITIVE << SFC_CTRL_PHASE_SEL_SHIFT,
>> + sfc->regbase + SFC_CTRL);
>> +
>> + reg |= (priv->cs & SFC_CMD_CS_MASK) << SFC_CMD_CS_SHIFT;
>> + reg |= (len & SFC_CMD_TRAN_BYTES_MASK) << SFC_CMD_TRAN_BYTES_SHIFT;
>> +
>> + if (op_type == SFC_CMD_DIR_RD)
>> + reg |= (nor->read_dummy & SFC_CMD_DUMMY_MASK) <<
>> + SFC_CMD_DUMMY_SHIFT;
>> +
>> + /* Should minus one as 0x0 means 1 bit flash address */
>> + writel_relaxed(nor->addr_width * 8 - 1, sfc->regbase + SFC_ABIT);
>> + writel_relaxed(reg, sfc->regbase + SFC_CMD);
>> + writel_relaxed(from_to, sfc->regbase + SFC_ADDR);
>> +
>> + if (op_type == SFC_CMD_DIR_WR)
>> + return rockchip_sfc_pio_write(sfc, buf, len);
>> + else
>> + return rockchip_sfc_pio_read(sfc, buf, len);
>> +}
>> +
>> +static ssize_t rockchip_sfc_read(struct spi_nor *nor, loff_t from, size_t len,
>> + u_char *read_buf)
>> +{
>> + struct rockchip_sfc_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + size_t offset;
>> + int ret;
>> + dma_addr_t dma_addr = 0;
>> +
>> + if (!sfc->use_dma)
>> + goto no_dma;
>> +
>> + for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
>> + size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
>> +
>> + dma_addr = dma_map_single(NULL, (void *)read_buf,
>> + trans, DMA_FROM_DEVICE);
>> + if (dma_mapping_error(sfc->dev, dma_addr))
>> + dma_addr = 0;
>> +
>> + /* Fail to map dma, use pre-allocated area instead */
>> + ret = rockchip_sfc_dma_transfer(nor, from + offset,
>> + dma_addr ? dma_addr :
>> + sfc->dma_buffer,
>> + trans, SFC_CMD_DIR_RD);
>> + if (ret) {
>> + dev_warn(nor->dev, "DMA read timeout\n");
>> + return ret;
>> + }
>> + if (!dma_addr)
>> + memcpy(read_buf + offset, sfc->buffer, trans);
>> + }
>> +
>> + return len;
>> +
>> +no_dma:
>> + ret = rockchip_sfc_pio_transfer(nor, from, len,
>> + read_buf, SFC_CMD_DIR_RD);
>> + if (ret) {
>> + dev_warn(nor->dev, "PIO read timeout\n");
>> + return ret;
>> + }
>> + return len;
>> +}
>> +
>> +static ssize_t rockchip_sfc_write(struct spi_nor *nor, loff_t to,
>> + size_t len, const u_char *write_buf)
>> +{
>> + struct rockchip_sfc_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + size_t offset;
>> + int ret;
>> + dma_addr_t dma_addr = 0;
>> +
>> + if (!sfc->use_dma)
>> + goto no_dma;
>
> Seems like there's a lot of similarity between read/write .
I was thinking to combine read/write with a extra argument to
indicate WR/RD. But as we could see still some differece between
WR and RD and there are already some condiction checks. So it
will make the code hard to read with stuffing lots of condition
checks. So I splited out read and write strightforward. :)
>
>> + for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
>> + size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
>> +
>> + dma_addr = dma_map_single(NULL, (void *)write_buf,
>> + trans, DMA_TO_DEVICE);
>> + if (dma_mapping_error(sfc->dev, dma_addr)) {
>> + dma_addr = 0;
>> + memcpy(sfc->buffer, write_buf + offset, trans);
>> + }
>> +
>> + /* Fail to map dma, use pre-allocated area instead */
>> + ret = rockchip_sfc_dma_transfer(nor, to + offset,
>> + dma_addr ? dma_addr :
>> + sfc->dma_buffer,
>> + trans, SFC_CMD_DIR_WR);
>> + if (dma_addr)
>> + dma_unmap_single(NULL, dma_addr,
>> + trans, DMA_TO_DEVICE);
>> + if (ret) {
>> + dev_warn(nor->dev, "DMA write timeout\n");
>> + return ret;
>> + }
>> + }
>> +
>> + return len;
>> +no_dma:
>> + ret = rockchip_sfc_pio_transfer(nor, to, len,
>> + (u_char *)write_buf, SFC_CMD_DIR_WR);
>> + if (ret) {
>> + dev_warn(nor->dev, "PIO write timeout\n");
>> + return ret;
>> + }
>> + return len;
>> +}
>> +
>> +/**
>> + * Get spi flash device information and register it as a mtd device.
>> + */
>> +static int rockchip_sfc_register(struct device_node *np,
>> + struct rockchip_sfc *sfc)
>> +{
>> + struct device *dev = sfc->dev;
>> + struct spi_nor *nor;
>> + struct rockchip_sfc_priv *priv;
>> + struct mtd_info *mtd;
>> + int ret;
>> +
>> + nor = devm_kzalloc(dev, sizeof(*nor), GFP_KERNEL);
>> + if (!nor)
>> + return -ENOMEM;
>
> You can embed struct spi_nor in struct rockchip_sfc_priv and drop this
> allocation . Also it'd be a good idea to rename rockchip_sfc_priv to
> something like rockchip_sfc_chip_priv to make it explicit this is a
> per-chip private data -- which you can even pre-allocate in rockchi_sfc
> structure as a static array of (four) such structures (see cadence qspi
> driver for how this is done there).
>
sure, will improve it.
>> + nor->dev = dev;
>> + spi_nor_set_flash_node(nor, np);
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + ret = of_property_read_u32(np, "reg", &priv->cs);
>> + if (ret) {
>> + dev_err(dev, "No reg property for %s\n",
>> + np->full_name);
>> + return ret;
>> + }
>> +
>> + ret = of_property_read_u32(np, "spi-max-frequency",
>> + &priv->clk_rate);
>> + if (ret) {
>> + dev_err(dev, "No spi-max-frequency property for %s\n",
>> + np->full_name);
>> + return ret;
>> + }
>> +
>> + priv->sfc = sfc;
>> + nor->priv = priv;
>> +
>> + nor->prepare = rockchip_sfc_prep;
>> + nor->unprepare = rockchip_sfc_unprep;
>> + nor->read_reg = rockchip_sfc_read_reg;
>> + nor->write_reg = rockchip_sfc_write_reg;
>> + nor->read = rockchip_sfc_read;
>> + nor->write = rockchip_sfc_write;
>> + nor->erase = NULL;
>> + ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
>> + if (ret)
>> + return ret;
>> +
>> + mtd = &nor->mtd;
>> + mtd->name = np->name;
>> + ret = mtd_device_register(mtd, NULL, 0);
>> + if (ret)
>> + return ret;
>> +
>> + sfc->nor[sfc->num_chip] = nor;
>> + sfc->num_chip++;
>> + return 0;
>> +}
>> +
>> +static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < sfc->num_chip; i++)
>> + mtd_device_unregister(&sfc->nor[i]->mtd);
>> +}
>> +
>> +static int rockchip_sfc_register_all(struct rockchip_sfc *sfc)
>> +{
>> + struct device *dev = sfc->dev;
>> + struct device_node *np;
>> + int ret;
>> +
>> + for_each_available_child_of_node(dev->of_node, np) {
>> + ret = rockchip_sfc_register(np, sfc);
>> + if (ret)
>> + goto fail;
>> +
>> + if (sfc->num_chip == SFC_MAX_CHIP_NUM) {
>> + dev_warn(dev, "Exceeds the max cs limitation\n");
>> + break;
>> + }
>> + }
>> +
>> + return 0;
>> +
>> +fail:
>> + dev_err(dev, "Failed to register all chip\n");
>> + rockchip_sfc_unregister_all(sfc);
>
> See cadence qspi where we only unregister the registered flashes.
> Implement it the same way here.
>
yup, but I'm afraid that rockchip_sfc_unregister_all confused you
as it actually unregisters the registered ones, not for all.
static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
{
int i;
for (i = 0; i < sfc->num_chip; i++)
mtd_device_unregister(&sfc->nor[i]->mtd);
}
sfc->num_chip stands for how many flashes registered successfully.
>> + return ret;
>> +}
>> +
>> +static irqreturn_t rockchip_sfc_irq_handler(int irq, void *dev_id)
>> +{
>> + struct rockchip_sfc *sfc = dev_id;
>> + u32 reg;
>> +
>> + reg = readl_relaxed(sfc->regbase + SFC_RISR);
>> + dev_dbg(sfc->dev, "Get irq: 0x%x\n", reg);
>> +
>> + /* Clear interrupt */
>> + writel_relaxed(reg, sfc->regbase + SFC_ICLR);
>> +
>> + if (reg & SFC_IRQ_TRAN_FINISH)
>> + complete(&sfc->cp);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int rockchip_sfc_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct resource *res;
>> + struct rockchip_sfc *sfc;
>> + int ret;
>> +
>> + sfc = devm_kzalloc(dev, sizeof(*sfc), GFP_KERNEL);
>> + if (!sfc)
>> + return -ENOMEM;
>> +
>> + platform_set_drvdata(pdev, sfc);
>> + sfc->dev = dev;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + sfc->regbase = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(sfc->regbase))
>> + return PTR_ERR(sfc->regbase);
>> +
>> + sfc->clk = devm_clk_get(&pdev->dev, "sfc");
>> + if (IS_ERR(sfc->clk)) {
>> + dev_err(&pdev->dev, "Failed to get sfc interface clk\n");
>> + return PTR_ERR(sfc->clk);
>> + }
>> +
>> + sfc->hclk = devm_clk_get(&pdev->dev, "hsfc");
>> + if (IS_ERR(sfc->hclk)) {
>> + dev_err(&pdev->dev, "Failed to get sfc ahp clk\n");
>> + return PTR_ERR(sfc->hclk);
>> + }
>> +
>> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>> + if (ret) {
>> + dev_warn(dev, "Unable to set dma mask\n");
>> + return ret;
>> + }
>> +
>> + sfc->buffer = dmam_alloc_coherent(dev, SFC_DMA_MAX_LEN,
>> + &sfc->dma_buffer, GFP_KERNEL);
>> + if (!sfc->buffer)
>> + return -ENOMEM;
>> +
>> + mutex_init(&sfc->lock);
>> +
>> + ret = clk_prepare_enable(sfc->hclk);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to enable hclk\n");
>> + goto err_hclk;
>> + }
>> +
>> + ret = clk_prepare_enable(sfc->clk);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to enable clk\n");
>> + goto err_clk;
>> + }
>> +
>> + if (of_property_read_bool(sfc->dev->of_node, "rockchip,sfc-no-dma"))
>> + sfc->use_dma = false;
>> + else
>> + sfc->use_dma = true;
>
> sfc->use_dma = !of_property_read_bool(sfc->dev->of_node,
> "rockchip,sfc-no-dma");
>
will improve.
>> + if (of_device_is_compatible(sfc->dev->of_node,
>> + "rockchip,rk1108-sfc"))
>> + sfc->negative_edge = true;
>> + else
>> + sfc->negative_edge = false;
>
> See above
>
Ditto.
>> + /* Find the irq */
>> + ret = platform_get_irq(pdev, 0);
>> + if (ret < 0) {
>> + dev_err(dev, "Failed to get the irq\n");
>> + goto err_irq;
>> + }
>> +
>> + ret = devm_request_irq(dev, ret, rockchip_sfc_irq_handler,
>> + 0, pdev->name, sfc);
>> + if (ret) {
>> + dev_err(dev, "Failed to request irq\n");
>> + goto err_irq;
>> + }
>> +
>> + ret = rockchip_sfc_init(sfc);
>> + if (ret)
>> + goto err_init;
>> +
>> + ret = rockchip_sfc_register_all(sfc);
>> + if (ret)
>> + goto err_init;
>> +
>> + clk_disable_unprepare(sfc->clk);
>> + return 0;
>> +
>> +err_irq:
>> +err_init:
>
> Drop the err_irq: label unless you plan to handle the error (which you
> should).
will remove.
>
>> + clk_disable_unprepare(sfc->clk);
>> +err_clk:
>> + clk_disable_unprepare(sfc->hclk);
>> +err_hclk:
>> + mutex_destroy(&sfc->lock);
>> + return ret;
>> +}
>> +
>> +static int rockchip_sfc_remove(struct platform_device *pdev)
>> +{
>> + struct rockchip_sfc *sfc = platform_get_drvdata(pdev);
>> +
>> + rockchip_sfc_unregister_all(sfc);
>> + mutex_destroy(&sfc->lock);
>> + clk_disable_unprepare(sfc->clk);
>> + clk_disable_unprepare(sfc->hclk);
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id rockchip_sfc_dt_ids[] = {
>> + { .compatible = "rockchip,sfc"},
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, rockchip_sfc_dt_ids);
>> +
>> +static struct platform_driver rockchip_sfc_driver = {
>> + .driver = {
>> + .name = "rockchip-sfc",
>> + .of_match_table = rockchip_sfc_dt_ids,
>> + },
>> + .probe = rockchip_sfc_probe,
>> + .remove = rockchip_sfc_remove,
>> +};
>> +module_platform_driver(rockchip_sfc_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("Rockchip Serial Flash Controller Driver");
>
> MODULE_AUTHOR is missing
sure.
>
>
>
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nor: add rockchip serial flash controller driver
@ 2016-11-16 1:59 ` Shawn Lin
0 siblings, 0 replies; 20+ messages in thread
From: Shawn Lin @ 2016-11-16 1:59 UTC (permalink / raw)
To: Marek Vasut, Rob Herring, David Woodhouse, Brian Norris
Cc: Shawn Lin, devicetree, linux-mtd, Heiko Stuebner, linux-rockchip
Hi Marek,
Thanks for reviewing my patch, and it's really
helpful to improve it. :) See my feedback for
your comments below.
On 2016/11/16 4:52, Marek Vasut wrote:
> On 11/11/2016 10:16 AM, Shawn Lin wrote:
>> Add rockchip serial flash controller driver
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>
>
> [...]
>
>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index 4a682ee..48c5e0e 100644
>> --- a/drivers/mtd/spi-nor/Kconfig
>> +++ b/drivers/mtd/spi-nor/Kconfig
>> @@ -65,6 +65,13 @@ config SPI_HISI_SFC
>> help
>> This enables support for hisilicon SPI-NOR flash controller.
>>
>> +config SPI_ROCKCHIP_SFC
>
> Keep this list sorted please.
yup, will fix.
>
>> + tristate "Rockchip Serial Flash Controller(SFC)"
>> + depends on ARCH_ROCKCHIP || COMPILE_TEST
>> + depends on HAS_IOMEM && HAS_DMA
>> + help
>> + This enables support for rockchip serial flash controller.
>> +
>> config SPI_NXP_SPIFI
>> tristate "NXP SPI Flash Interface (SPIFI)"
>> depends on OF && (ARCH_LPC18XX || COMPILE_TEST)
>
>
> [...]
>
>> +/* Interrypt mask */
>
> Interrupt :)
will fix. :)
>
>> +#define SFC_IMR 0x4
>> +#define SFC_IMR_RX_FULL BIT(0)
>> +#define SFC_IMR_RX_UFLOW BIT(1)
>> +#define SFC_IMR_TX_OFLOW BIT(2)
>> +#define SFC_IMR_TX_EMPTY BIT(3)
>> +#define SFC_IMR_TRAN_FINISH BIT(4)
>> +#define SFC_IMR_BUS_ERR BIT(5)
>> +#define SFC_IMR_NSPI_ERR BIT(6)
>> +#define SFC_IMR_DMA BIT(7)
>
>
> [...]
>
>> +enum rockchip_sfc_iftype {
>> + IF_TYPE_STD,
>> + IF_TYPE_DUAL,
>> + IF_TYPE_QUAD,
>> +};
>> +
>> +struct rockchip_sfc {
>> + struct device *dev;
>> + struct mutex lock;
>> + void __iomem *regbase;
>> + struct clk *hclk;
>> + struct clk *clk;
>> + void *buffer;
>> + dma_addr_t dma_buffer;
>
> The naming (buffer) could use some improvement or comment for clarification.
>
>> + struct completion cp;
>> + struct spi_nor *nor[SFC_MAX_CHIP_NUM];
>
> Should be MAX_CHIPSELECT_NUM , for clarity.
seems sane, will fix.
>
>> + u32 num_chip;
>
> u8
>
>> + bool use_dma;
>> + bool negative_edge;
>
> Negative edge ... of what ?
For how the inner sample logic to letch the data. It should be
configured differently for different Socs. I will add a comment
here to clarify it.
>
>> +};
>> +
>> +struct rockchip_sfc_priv {
>> + u32 cs;
>
> Doesn't this board support only 4 CS ? Use u8 :-)
>
good catch, will fix it.
>> + u32 clk_rate;
>> + struct rockchip_sfc *sfc;
>> +};
>> +
>> +static int get_if_type(enum read_mode flash_read)
>> +{
>> + enum rockchip_sfc_iftype if_type;
>> +
>> + switch (flash_read) {
>> + case SPI_NOR_DUAL:
>> + if_type = IF_TYPE_DUAL;
>> + break;
>> + case SPI_NOR_QUAD:
>> + if_type = IF_TYPE_QUAD;
>> + break;
>> + case SPI_NOR_NORMAL:
>> + case SPI_NOR_FAST:
>> + default:
>
> Should the default case really fall back to 1-bit mode or should it
> rather report error ?
It's derived from nor->flash_read, so personlly I think there should
never fall into the default case, but it would make sense to return
-EINVAL instead of falling into 1-bit mode. I will fix it.
>
>> + if_type = IF_TYPE_STD;
>> + break;
>> + }
>> +
>> + return if_type;
>> +}
>> +
>> +static int rockchip_sfc_reset(struct rockchip_sfc *sfc)
>> +{
>> + unsigned long timeout = jiffies + HZ;
>> + int err = -ETIMEDOUT;
>> + u32 status;
>> +
>> + writel_relaxed(SFC_RCVR_RESET, sfc->regbase + SFC_RCVR);
>> +
>> + while (time_before(jiffies, timeout)) {
>
> Would readl_poll_*() from include/linux/iopoll.h help here ?
>
Brilliant, it looks great to me.
>> + status = readl_relaxed(sfc->regbase + SFC_RCVR);
>> + if (!(status & SFC_RCVR_RESET)) {
>> + err = 0;
>> + break;
>> + }
>> + msleep(20);
>> + }
>> +
>> + if (err)
>> + dev_err(sfc->dev, "SFC reset never finished\n");
>
> Should the writel() below be executed if an error happened ?
It's needed since when doing reset after failing to finish
a previous transfer for whatever reason, we could observed some of the
interrupts triggered. Although we masked them but we could still
find them from the raw interrupt status register. So cleaning
it explicitly make senses.
>
>> + writel_relaxed(SFC_ICLR_RX_FULL | SFC_ICLR_RX_UFLOW |
>> + SFC_ICLR_TX_OFLOW | SFC_ICLR_TX_EMPTY |
>> + SFC_ICLR_TRAN_FINISH | SFC_ICLR_BUS_ERR |
>> + SFC_ICLR_NSPI_ERR | SFC_ICLR_DMA,
>> + sfc->regbase + SFC_ICLR);
>> + return err;
>> +}
>> +
>> +static int rockchip_sfc_init(struct rockchip_sfc *sfc)
>> +{
>> + int err;
>> +
>> + err = rockchip_sfc_reset(sfc);
>> + if (err)
>> + return err;
>> +
>> + /* Mask all eight interrupts */
>> + writel_relaxed(0xff, sfc->regbase + SFC_IMR);
>> + /* Phase configure */
>
> What phase ? Please clarify the comment. Also, don't you have to
> configure the register if sfc->negative_edge == 0 too ?
will elaborate more.
Your comment makes me think it twice. The loader should
already set this but we could override it agian in case
of some other code clear it. I was assuming that the reset
value of it meets what we need for "sfc->negative_edge == 0",
but I think I am wrong since we still need to prevent some other
whatever code modified it before jumping into rockchip_sfc_probe.
So yes, I will fix it. :)
>
>> + if (sfc->negative_edge)
>> + writel_relaxed(SFC_CTRL_PHASE_SEL_NEGETIVE <<
>> + SFC_CTRL_PHASE_SEL_SHIFT,
>> + sfc->regbase + SFC_CTRL);
>> + return 0;
>> +}
>> +
>> +static int rockchip_sfc_prep(struct spi_nor *nor, enum spi_nor_ops ops)
>> +{
>> + struct rockchip_sfc_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + int ret;
>> +
>> + mutex_lock(&sfc->lock);
>> +
>> + ret = clk_set_rate(sfc->clk, priv->clk_rate);
>> + if (ret)
>> + goto out;
>> +
>> + ret = clk_prepare_enable(sfc->clk);
>> + if (ret)
>> + goto out;
>> +
>> + return 0;
>> +
>> +out:
>> + mutex_unlock(&sfc->lock);
>> + return ret;
>> +}
>> +
>> +static void rockchip_sfc_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
>> +{
>> + struct rockchip_sfc_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> +
>> + clk_disable_unprepare(sfc->clk);
>> + mutex_unlock(&sfc->lock);
>> +}
>> +
>> +static int rockchip_sfc_wait_op_finish(struct rockchip_sfc *sfc)
>> +{
>> + unsigned long timeout = jiffies + 2 * HZ;
>> + int err = -ETIMEDOUT;
>> + u32 status;
>> +
>> + /*
>> + * Note: tx and rx share the same fifo, so the rx's water level
>> + * is the same as rx's, which means this function could be reused
>> + * for checking the read operations as well.
>> + */
>> + while (time_before(jiffies, timeout)) {
>
> readl_poll_*() ?
sure.
>
>> + status = readl_relaxed(sfc->regbase + SFC_FSR);
>> + if (((status >> SFC_FSR_TX_EMPTY_SHIFT) &
>> + SFC_FSR_TX_EMPTY_MASK) == SFC_FSR_TX_IS_EMPTY) {
>> + err = 0;
>> + break;
>> + }
>> + msleep(20);
>> + }
>> +
>> + if (err)
>> + dev_err(sfc->dev, "SFC tx never empty\n");
>> +
>> + return err;
>> +}
>> +
>> +static int rockchip_sfc_op_reg(struct spi_nor *nor,
>> + u8 opcode, int len, u8 optype)
>> +{
>> + struct rockchip_sfc_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + u32 reg;
>> +
>> + if (((readl_relaxed(sfc->regbase + SFC_FSR) >> SFC_FSR_TX_EMPTY_SHIFT) &
>> + SFC_FSR_TX_EMPTY_MASK) != SFC_FSR_TX_IS_EMPTY ||
>> + ((readl_relaxed(sfc->regbase + SFC_FSR) >>
>> + SFC_FSR_RX_EMPTY_SHIFT) &
>> + SFC_FSR_RX_EMPTY_MASK) != SFC_FSR_RX_IS_EMPTY ||
>> + (readl_relaxed(sfc->regbase + SFC_SR) == SFC_SR_IS_BUSY))
>> + rockchip_sfc_reset(sfc);
>
> This is chaos, please fix this condition so it's actually readable. You
> can ie. read the FSR into a variable, do your shifting/anding magic and
> then do if (var1 || var2 || var3) {} .
okay, will make it more clearly.
>
>> + reg = (opcode & SFC_CMD_IDX_MASK) << SFC_CMD_IDX_SHIFT;
>> + reg |= (len & SFC_CMD_TRAN_BYTES_MASK) << SFC_CMD_TRAN_BYTES_SHIFT;
>> + reg |= (priv->cs & SFC_CMD_CS_MASK) << SFC_CMD_CS_SHIFT;
>> + reg |= (optype & SFC_CMD_DIR_MASK) << SFC_CMD_DIR_SHIFT;
>> +
>> + writel_relaxed(reg, sfc->regbase + SFC_CMD);
>> +
>> + return rockchip_sfc_wait_op_finish(sfc);
>> +}
>> +
>> +static int rockchip_sfc_read_reg(struct spi_nor *nor, u8 opcode,
>> + u8 *buf, int len)
>> +{
>> + struct rockchip_sfc_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + int ret;
>> + u32 tmp;
>> + u32 i;
>> +
>> + ret = rockchip_sfc_op_reg(nor, opcode, len, SFC_CMD_DIR_RD);
>> + if (ret)
>> + return ret;
>> +
>> + while (len > 0) {
>> + tmp = readl_relaxed(sfc->regbase + SFC_DATA);
>> + for (i = 0; i < len; i++)
>> + *buf++ = (u8)((tmp >> (i * 8)) & 0xff);
>
> Won't this fail for len > 4 ?
nope, this loop will reduce 4 for each successful readl. And
reading the remained bytes which isn't aligned to DWORD, isn't it?
>
> Also, you can use ioread32_rep() here, but (!) that won't work for
> unaligned reads, which I dunno if they can happen here, but please do
> double-check.
yes, I have checked this API as well as others like memcpy_{to,from}io
, etc. They will generate a external abort for arm core as the unaligned
(DWORD) read/write via AHB aren't supported by Rockchip Socs. So I have
to open code these stuff. This could be easily found for other
upstreamed rockchip drivers. :)
>
>> + len = len - 4;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int rockchip_sfc_write_reg(struct spi_nor *nor, u8 opcode,
>> + u8 *buf, int len)
>> +{
>> + struct rockchip_sfc_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + u32 words, i;
>> +
>> + /* Align bytes to words */
>> + words = (len + 3) >> 2;
>> +
>> + for (i = 0; i < words; i++)
>> + writel_relaxed(*(buf + 4 * i), sfc->regbase + SFC_DATA);
>
> See above about the ioread32_rep()/iowrite32_rep(), but careful about
> unaligned (len % 4 != 0) case.
Ditto for above. :)
>
>> + return rockchip_sfc_op_reg(nor, opcode, len, SFC_CMD_DIR_WR);
>> +}
>> +
>> +static int rockchip_sfc_dma_transfer(struct spi_nor *nor, loff_t from_to,
>> + dma_addr_t dma_buf, size_t len, u8 op_type)
>> +{
>> + struct rockchip_sfc_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + u32 reg;
>> + u8 if_type = 0;
>> +
>> + init_completion(&sfc->cp);
>> +
>> + writel_relaxed(SFC_ICLR_RX_FULL | SFC_ICLR_RX_UFLOW |
>> + SFC_ICLR_TX_OFLOW | SFC_ICLR_TX_EMPTY |
>> + SFC_ICLR_TRAN_FINISH | SFC_ICLR_BUS_ERR |
>> + SFC_ICLR_NSPI_ERR | SFC_ICLR_DMA,
>> + sfc->regbase + SFC_ICLR);
>> +
>> + /* Enable transfer complete interrupt */
>> + reg = readl_relaxed(sfc->regbase + SFC_IMR);
>> + reg &= ~SFC_IMR_TRAN_FINISH;
>> + writel_relaxed(reg, sfc->regbase + SFC_IMR);
>> +
>> + if (op_type == SFC_CMD_DIR_WR)
>> + reg = (SFC_CMD_DIR_WR << SFC_CMD_DIR_SHIFT) |
>> + ((nor->program_opcode & SFC_CMD_IDX_MASK) <<
>> + SFC_CMD_IDX_SHIFT);
>> + else
>> + reg = (SFC_CMD_DIR_RD << SFC_CMD_DIR_SHIFT) |
>> + ((nor->read_opcode & SFC_CMD_IDX_MASK) <<
>> + SFC_CMD_IDX_SHIFT);
>
> reg = nor->read_opcode & SFC_CMD_IDX_MASK) << SFC_CMD_IDX_SHIFT;
> reg |= op_type << SFC_CMD_DIR_SHIFT;
will improve.
>
>> + reg |= ((nor->addr_width == 4) ? SFC_CMD_ADDR_32BITS
>> + : SFC_CMD_ADDR_24BITS) << SFC_CMD_ADDR_SHIFT;
>
> Why don't you just define those SFC_CMD_ADDR_24BITS and co. with the
> shift in those bitfields already ? Then you wouldn't have to riddle this
> driver with FOO << BAR, but you'd only have FOO all over the place.
>
sounds good, will improve them..
>> + if_type = get_if_type(nor->flash_read);
>> + writel_relaxed(if_type << SFC_CTRL_DATA_BITS_SHIFT |
>> + if_type << SFC_CTRL_ADDR_BITS_SHIFT |
>> + if_type << SFC_CTRL_CMD_BITS_SHIFT |
>
> Parenthesis missing around the statements ,
> (if_type << FOO) | (... << bar)
>
will improve it.
>> + sfc->negative_edge ?
>> + SFC_CTRL_PHASE_SEL_NEGETIVE << SFC_CTRL_PHASE_SEL_SHIFT :
>> + SFC_CTRL_PHASE_SEL_POSITIVE << SFC_CTRL_PHASE_SEL_SHIFT,
>> + sfc->regbase + SFC_CTRL);
>> +
>> + reg |= (priv->cs & SFC_CMD_CS_MASK) << SFC_CMD_CS_SHIFT;
>> + reg |= (len & SFC_CMD_TRAN_BYTES_MASK) << SFC_CMD_TRAN_BYTES_SHIFT;
>> +
>> + if (op_type == SFC_CMD_DIR_RD)
>> + reg |= (nor->read_dummy & SFC_CMD_DUMMY_MASK) <<
>> + SFC_CMD_DUMMY_SHIFT;
>
> Just define SFC_CMD_DUMMY(x) \
> (((x) & SFC_CMD_DUMMY_MASK) << SFC_CMD_DUMMY_SHIFT)
>
> And then use it ... reg |= SFC_CMD_DUMMY(nor->read_dummy);
sure.
>
>> + /* Should minus one as 0x0 means 1 bit flash address */
>> + writel_relaxed(nor->addr_width * 8 - 1, sfc->regbase + SFC_ABIT);
>> + writel_relaxed(reg, sfc->regbase + SFC_CMD);
>> + writel_relaxed(from_to, sfc->regbase + SFC_ADDR);
>> + writel_relaxed(dma_buf, sfc->regbase + SFC_DMA_ADDR);
>
> I hope the DMA buffer management is implemented correctly and you're not
> running into any weird cache issues.
you are right. I should sync it after doing read and before doing write.
>
>> + /* Start dma */
>> + writel_relaxed(0x1, sfc->regbase + SFC_DMA_TRIGGER);
>> +
>> + /* Wait for the interrupt. */
>> + if (!wait_for_completion_timeout(&sfc->cp, msecs_to_jiffies(2000))) {
>> + dev_err(sfc->dev, "DMA wait for transfer finish timeout.");
>> + return -ETIMEDOUT;
>> + }
>> +
>> + /* Disable transfer finish interrupt */
>> + reg = readl_relaxed(sfc->regbase + SFC_IMR);
>> + reg |= SFC_IMR_TRAN_FINISH;
>> + writel_relaxed(reg, sfc->regbase + SFC_IMR);
>> +
>> + return rockchip_sfc_wait_op_finish(sfc);
>> +}
>> +
>> +static inline int rockchip_sfc_pio_write(struct rockchip_sfc *sfc, u_char *buf,
>> + size_t len)
>> +{
>> + u32 words, tx_wl, count, i;
>> + unsigned long timeout;
>> + int ret = 0;
>> + u32 *tbuf = (u32 *)buf;
>> +
>> + /* Align bytes to words */
>> + words = (len + 3) >> 2;
>> +
>> + while (words) {
>
> See iowrite32_rep() above, but I suspect you'll run into problems with
> $len which is not multiple of 4 .
>
Ditto for above.
>> + tx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
>> + SFC_FSR_TX_WATER_LVL_SHIFT) &
>> + SFC_FSR_TX_WATER_LVL_MASK;
>> +
>> + if (tx_wl > 0) {
>> + count = min_t(u32, words, tx_wl);
>> + for (i = 0; i < count; i++) {
>> + writel_relaxed(*tbuf++,
>> + sfc->regbase + SFC_DATA);
>> + words--;
>> + }
>> +
>> + if (words == 0)
>> + break;
>> + timeout = 0;
>> + } else {
>> + mdelay(1);
>> + if (timeout++ > SFC_MAX_IDLE_RETRY) {
>> + ret = -ETIMEDOUT;
>> + break;
>> + }
>> + }
>> + }
>> +
>> + if (ret)
>> + return ret;
>> + else
>> + return rockchip_sfc_wait_op_finish(sfc);
>> +}
>> +
>> +static inline int rockchip_sfc_pio_read(struct rockchip_sfc *sfc, u_char *buf,
>> + size_t len)
>> +{
>> + u32 words, rx_wl, count, i;
>> + unsigned long timeout;
>> + int ret = 0;
>> + u32 tmp;
>> + u32 *tbuf = (u32 *)buf;
>> + u_char *tbuf2;
>> +
>> + words = len >> 2;
>> + /* Get the remained bytes */
>> + len = len & 0x3;
>
> See above.
>
>> + while (words) {
>> + rx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
>> + SFC_FSR_RX_WATER_LVL_SHIFT) &
>> + SFC_FSR_RX_WATER_LVL_MASK;
>> +
>> + if (rx_wl > 0) {
>> + count = min_t(u32, words, rx_wl);
>> + for (i = 0; i < count; i++) {
>> + *tbuf++ = readl_relaxed(sfc->regbase +
>> + SFC_DATA);
>> + words--;
>> + }
>> +
>> + if (words == 0)
>> + break;
>> + timeout = 0;
>> + } else {
>> + mdelay(1);
>> + if (timeout++ > SFC_MAX_IDLE_RETRY) {
>> + ret = -ETIMEDOUT;
>> + break;
>> + }
>> + }
>> + }
>> +
>> + if (ret)
>> + return ret;
>> +
>> + /* Read the remained bytes */
>> + timeout = 0;
>> + tbuf2 = (u_char *)tbuf;
>> + while (len) {
>> + rx_wl = (readl_relaxed(sfc->regbase + SFC_FSR) >>
>> + SFC_FSR_RX_WATER_LVL_SHIFT) &
>> + SFC_FSR_RX_WATER_LVL_MASK;
>> + if (rx_wl > 0) {
>> + tmp = readl_relaxed(sfc->regbase + SFC_DATA);
>> + for (i = 0; i < len; i++)
>> + tbuf2[i] = (u8)((tmp >> (i * 8)) & 0xff);
>> + goto done;
>> + } else {
>> + mdelay(1);
>> + if (timeout++ > SFC_MAX_IDLE_RETRY) {
>> + ret = -ETIMEDOUT;
>> + break;
>> + }
>> + }
>> + }
>> +done:
>> + if (ret)
>> + return ret;
>> + else
>> + return rockchip_sfc_wait_op_finish(sfc);
>> +}
>> +
>> +static int rockchip_sfc_pio_transfer(struct spi_nor *nor, loff_t from_to,
>> + size_t len, u_char *buf, u8 op_type)
>> +{
>> + struct rockchip_sfc_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + u32 reg;
>> + u8 if_type = 0;
>> +
>> + if (op_type == SFC_CMD_DIR_WR)
>> + reg = (SFC_CMD_DIR_WR << SFC_CMD_DIR_SHIFT) |
>> + ((nor->program_opcode & SFC_CMD_IDX_MASK) <<
>> + SFC_CMD_IDX_SHIFT);
>> + else
>> + reg = (SFC_CMD_DIR_RD << SFC_CMD_DIR_SHIFT) |
>> + ((nor->read_opcode & SFC_CMD_IDX_MASK) <<
>> + SFC_CMD_IDX_SHIFT);
>
> See above regarding this condition. I think you can factor out this
> common code too. Also nuke the bitshifts , see my comments on
> rockchip_sfc_dma_transfer .
>
sure. Will fix the bitshifts and factor out the common code.
>> + reg |= ((nor->addr_width == 4) ? SFC_CMD_ADDR_32BITS
>> + : SFC_CMD_ADDR_24BITS) << SFC_CMD_ADDR_SHIFT;
>> +
>> + if_type = get_if_type(nor->flash_read);
>> + writel_relaxed(if_type << SFC_CTRL_DATA_BITS_SHIFT |
>> + if_type << SFC_CTRL_ADDR_BITS_SHIFT |
>> + if_type << SFC_CTRL_CMD_BITS_SHIFT |
>> + sfc->negative_edge ?
>> + SFC_CTRL_PHASE_SEL_NEGETIVE << SFC_CTRL_PHASE_SEL_SHIFT :
>> + SFC_CTRL_PHASE_SEL_POSITIVE << SFC_CTRL_PHASE_SEL_SHIFT,
>> + sfc->regbase + SFC_CTRL);
>> +
>> + reg |= (priv->cs & SFC_CMD_CS_MASK) << SFC_CMD_CS_SHIFT;
>> + reg |= (len & SFC_CMD_TRAN_BYTES_MASK) << SFC_CMD_TRAN_BYTES_SHIFT;
>> +
>> + if (op_type == SFC_CMD_DIR_RD)
>> + reg |= (nor->read_dummy & SFC_CMD_DUMMY_MASK) <<
>> + SFC_CMD_DUMMY_SHIFT;
>> +
>> + /* Should minus one as 0x0 means 1 bit flash address */
>> + writel_relaxed(nor->addr_width * 8 - 1, sfc->regbase + SFC_ABIT);
>> + writel_relaxed(reg, sfc->regbase + SFC_CMD);
>> + writel_relaxed(from_to, sfc->regbase + SFC_ADDR);
>> +
>> + if (op_type == SFC_CMD_DIR_WR)
>> + return rockchip_sfc_pio_write(sfc, buf, len);
>> + else
>> + return rockchip_sfc_pio_read(sfc, buf, len);
>> +}
>> +
>> +static ssize_t rockchip_sfc_read(struct spi_nor *nor, loff_t from, size_t len,
>> + u_char *read_buf)
>> +{
>> + struct rockchip_sfc_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + size_t offset;
>> + int ret;
>> + dma_addr_t dma_addr = 0;
>> +
>> + if (!sfc->use_dma)
>> + goto no_dma;
>> +
>> + for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
>> + size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
>> +
>> + dma_addr = dma_map_single(NULL, (void *)read_buf,
>> + trans, DMA_FROM_DEVICE);
>> + if (dma_mapping_error(sfc->dev, dma_addr))
>> + dma_addr = 0;
>> +
>> + /* Fail to map dma, use pre-allocated area instead */
>> + ret = rockchip_sfc_dma_transfer(nor, from + offset,
>> + dma_addr ? dma_addr :
>> + sfc->dma_buffer,
>> + trans, SFC_CMD_DIR_RD);
>> + if (ret) {
>> + dev_warn(nor->dev, "DMA read timeout\n");
>> + return ret;
>> + }
>> + if (!dma_addr)
>> + memcpy(read_buf + offset, sfc->buffer, trans);
>> + }
>> +
>> + return len;
>> +
>> +no_dma:
>> + ret = rockchip_sfc_pio_transfer(nor, from, len,
>> + read_buf, SFC_CMD_DIR_RD);
>> + if (ret) {
>> + dev_warn(nor->dev, "PIO read timeout\n");
>> + return ret;
>> + }
>> + return len;
>> +}
>> +
>> +static ssize_t rockchip_sfc_write(struct spi_nor *nor, loff_t to,
>> + size_t len, const u_char *write_buf)
>> +{
>> + struct rockchip_sfc_priv *priv = nor->priv;
>> + struct rockchip_sfc *sfc = priv->sfc;
>> + size_t offset;
>> + int ret;
>> + dma_addr_t dma_addr = 0;
>> +
>> + if (!sfc->use_dma)
>> + goto no_dma;
>
> Seems like there's a lot of similarity between read/write .
I was thinking to combine read/write with a extra argument to
indicate WR/RD. But as we could see still some differece between
WR and RD and there are already some condiction checks. So it
will make the code hard to read with stuffing lots of condition
checks. So I splited out read and write strightforward. :)
>
>> + for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
>> + size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
>> +
>> + dma_addr = dma_map_single(NULL, (void *)write_buf,
>> + trans, DMA_TO_DEVICE);
>> + if (dma_mapping_error(sfc->dev, dma_addr)) {
>> + dma_addr = 0;
>> + memcpy(sfc->buffer, write_buf + offset, trans);
>> + }
>> +
>> + /* Fail to map dma, use pre-allocated area instead */
>> + ret = rockchip_sfc_dma_transfer(nor, to + offset,
>> + dma_addr ? dma_addr :
>> + sfc->dma_buffer,
>> + trans, SFC_CMD_DIR_WR);
>> + if (dma_addr)
>> + dma_unmap_single(NULL, dma_addr,
>> + trans, DMA_TO_DEVICE);
>> + if (ret) {
>> + dev_warn(nor->dev, "DMA write timeout\n");
>> + return ret;
>> + }
>> + }
>> +
>> + return len;
>> +no_dma:
>> + ret = rockchip_sfc_pio_transfer(nor, to, len,
>> + (u_char *)write_buf, SFC_CMD_DIR_WR);
>> + if (ret) {
>> + dev_warn(nor->dev, "PIO write timeout\n");
>> + return ret;
>> + }
>> + return len;
>> +}
>> +
>> +/**
>> + * Get spi flash device information and register it as a mtd device.
>> + */
>> +static int rockchip_sfc_register(struct device_node *np,
>> + struct rockchip_sfc *sfc)
>> +{
>> + struct device *dev = sfc->dev;
>> + struct spi_nor *nor;
>> + struct rockchip_sfc_priv *priv;
>> + struct mtd_info *mtd;
>> + int ret;
>> +
>> + nor = devm_kzalloc(dev, sizeof(*nor), GFP_KERNEL);
>> + if (!nor)
>> + return -ENOMEM;
>
> You can embed struct spi_nor in struct rockchip_sfc_priv and drop this
> allocation . Also it'd be a good idea to rename rockchip_sfc_priv to
> something like rockchip_sfc_chip_priv to make it explicit this is a
> per-chip private data -- which you can even pre-allocate in rockchi_sfc
> structure as a static array of (four) such structures (see cadence qspi
> driver for how this is done there).
>
sure, will improve it.
>> + nor->dev = dev;
>> + spi_nor_set_flash_node(nor, np);
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + ret = of_property_read_u32(np, "reg", &priv->cs);
>> + if (ret) {
>> + dev_err(dev, "No reg property for %s\n",
>> + np->full_name);
>> + return ret;
>> + }
>> +
>> + ret = of_property_read_u32(np, "spi-max-frequency",
>> + &priv->clk_rate);
>> + if (ret) {
>> + dev_err(dev, "No spi-max-frequency property for %s\n",
>> + np->full_name);
>> + return ret;
>> + }
>> +
>> + priv->sfc = sfc;
>> + nor->priv = priv;
>> +
>> + nor->prepare = rockchip_sfc_prep;
>> + nor->unprepare = rockchip_sfc_unprep;
>> + nor->read_reg = rockchip_sfc_read_reg;
>> + nor->write_reg = rockchip_sfc_write_reg;
>> + nor->read = rockchip_sfc_read;
>> + nor->write = rockchip_sfc_write;
>> + nor->erase = NULL;
>> + ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
>> + if (ret)
>> + return ret;
>> +
>> + mtd = &nor->mtd;
>> + mtd->name = np->name;
>> + ret = mtd_device_register(mtd, NULL, 0);
>> + if (ret)
>> + return ret;
>> +
>> + sfc->nor[sfc->num_chip] = nor;
>> + sfc->num_chip++;
>> + return 0;
>> +}
>> +
>> +static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < sfc->num_chip; i++)
>> + mtd_device_unregister(&sfc->nor[i]->mtd);
>> +}
>> +
>> +static int rockchip_sfc_register_all(struct rockchip_sfc *sfc)
>> +{
>> + struct device *dev = sfc->dev;
>> + struct device_node *np;
>> + int ret;
>> +
>> + for_each_available_child_of_node(dev->of_node, np) {
>> + ret = rockchip_sfc_register(np, sfc);
>> + if (ret)
>> + goto fail;
>> +
>> + if (sfc->num_chip == SFC_MAX_CHIP_NUM) {
>> + dev_warn(dev, "Exceeds the max cs limitation\n");
>> + break;
>> + }
>> + }
>> +
>> + return 0;
>> +
>> +fail:
>> + dev_err(dev, "Failed to register all chip\n");
>> + rockchip_sfc_unregister_all(sfc);
>
> See cadence qspi where we only unregister the registered flashes.
> Implement it the same way here.
>
yup, but I'm afraid that rockchip_sfc_unregister_all confused you
as it actually unregisters the registered ones, not for all.
static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
{
int i;
for (i = 0; i < sfc->num_chip; i++)
mtd_device_unregister(&sfc->nor[i]->mtd);
}
sfc->num_chip stands for how many flashes registered successfully.
>> + return ret;
>> +}
>> +
>> +static irqreturn_t rockchip_sfc_irq_handler(int irq, void *dev_id)
>> +{
>> + struct rockchip_sfc *sfc = dev_id;
>> + u32 reg;
>> +
>> + reg = readl_relaxed(sfc->regbase + SFC_RISR);
>> + dev_dbg(sfc->dev, "Get irq: 0x%x\n", reg);
>> +
>> + /* Clear interrupt */
>> + writel_relaxed(reg, sfc->regbase + SFC_ICLR);
>> +
>> + if (reg & SFC_IRQ_TRAN_FINISH)
>> + complete(&sfc->cp);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int rockchip_sfc_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct resource *res;
>> + struct rockchip_sfc *sfc;
>> + int ret;
>> +
>> + sfc = devm_kzalloc(dev, sizeof(*sfc), GFP_KERNEL);
>> + if (!sfc)
>> + return -ENOMEM;
>> +
>> + platform_set_drvdata(pdev, sfc);
>> + sfc->dev = dev;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + sfc->regbase = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(sfc->regbase))
>> + return PTR_ERR(sfc->regbase);
>> +
>> + sfc->clk = devm_clk_get(&pdev->dev, "sfc");
>> + if (IS_ERR(sfc->clk)) {
>> + dev_err(&pdev->dev, "Failed to get sfc interface clk\n");
>> + return PTR_ERR(sfc->clk);
>> + }
>> +
>> + sfc->hclk = devm_clk_get(&pdev->dev, "hsfc");
>> + if (IS_ERR(sfc->hclk)) {
>> + dev_err(&pdev->dev, "Failed to get sfc ahp clk\n");
>> + return PTR_ERR(sfc->hclk);
>> + }
>> +
>> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>> + if (ret) {
>> + dev_warn(dev, "Unable to set dma mask\n");
>> + return ret;
>> + }
>> +
>> + sfc->buffer = dmam_alloc_coherent(dev, SFC_DMA_MAX_LEN,
>> + &sfc->dma_buffer, GFP_KERNEL);
>> + if (!sfc->buffer)
>> + return -ENOMEM;
>> +
>> + mutex_init(&sfc->lock);
>> +
>> + ret = clk_prepare_enable(sfc->hclk);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to enable hclk\n");
>> + goto err_hclk;
>> + }
>> +
>> + ret = clk_prepare_enable(sfc->clk);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to enable clk\n");
>> + goto err_clk;
>> + }
>> +
>> + if (of_property_read_bool(sfc->dev->of_node, "rockchip,sfc-no-dma"))
>> + sfc->use_dma = false;
>> + else
>> + sfc->use_dma = true;
>
> sfc->use_dma = !of_property_read_bool(sfc->dev->of_node,
> "rockchip,sfc-no-dma");
>
will improve.
>> + if (of_device_is_compatible(sfc->dev->of_node,
>> + "rockchip,rk1108-sfc"))
>> + sfc->negative_edge = true;
>> + else
>> + sfc->negative_edge = false;
>
> See above
>
Ditto.
>> + /* Find the irq */
>> + ret = platform_get_irq(pdev, 0);
>> + if (ret < 0) {
>> + dev_err(dev, "Failed to get the irq\n");
>> + goto err_irq;
>> + }
>> +
>> + ret = devm_request_irq(dev, ret, rockchip_sfc_irq_handler,
>> + 0, pdev->name, sfc);
>> + if (ret) {
>> + dev_err(dev, "Failed to request irq\n");
>> + goto err_irq;
>> + }
>> +
>> + ret = rockchip_sfc_init(sfc);
>> + if (ret)
>> + goto err_init;
>> +
>> + ret = rockchip_sfc_register_all(sfc);
>> + if (ret)
>> + goto err_init;
>> +
>> + clk_disable_unprepare(sfc->clk);
>> + return 0;
>> +
>> +err_irq:
>> +err_init:
>
> Drop the err_irq: label unless you plan to handle the error (which you
> should).
will remove.
>
>> + clk_disable_unprepare(sfc->clk);
>> +err_clk:
>> + clk_disable_unprepare(sfc->hclk);
>> +err_hclk:
>> + mutex_destroy(&sfc->lock);
>> + return ret;
>> +}
>> +
>> +static int rockchip_sfc_remove(struct platform_device *pdev)
>> +{
>> + struct rockchip_sfc *sfc = platform_get_drvdata(pdev);
>> +
>> + rockchip_sfc_unregister_all(sfc);
>> + mutex_destroy(&sfc->lock);
>> + clk_disable_unprepare(sfc->clk);
>> + clk_disable_unprepare(sfc->hclk);
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id rockchip_sfc_dt_ids[] = {
>> + { .compatible = "rockchip,sfc"},
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, rockchip_sfc_dt_ids);
>> +
>> +static struct platform_driver rockchip_sfc_driver = {
>> + .driver = {
>> + .name = "rockchip-sfc",
>> + .of_match_table = rockchip_sfc_dt_ids,
>> + },
>> + .probe = rockchip_sfc_probe,
>> + .remove = rockchip_sfc_remove,
>> +};
>> +module_platform_driver(rockchip_sfc_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("Rockchip Serial Flash Controller Driver");
>
> MODULE_AUTHOR is missing
sure.
>
>
>
--
Best Regards
Shawn Lin
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nor: add rockchip serial flash controller driver
2016-11-16 1:59 ` Shawn Lin
@ 2016-11-20 21:11 ` Marek Vasut
-1 siblings, 0 replies; 20+ messages in thread
From: Marek Vasut @ 2016-11-20 21:11 UTC (permalink / raw)
To: Shawn Lin, Rob Herring, David Woodhouse, Brian Norris
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On 11/16/2016 02:59 AM, Shawn Lin wrote:
> Hi Marek,
Hi,
[...]
>>> +static int rockchip_sfc_read_reg(struct spi_nor *nor, u8 opcode,
>>> + u8 *buf, int len)
>>> +{
>>> + struct rockchip_sfc_priv *priv = nor->priv;
>>> + struct rockchip_sfc *sfc = priv->sfc;
>>> + int ret;
>>> + u32 tmp;
>>> + u32 i;
>>> +
>>> + ret = rockchip_sfc_op_reg(nor, opcode, len, SFC_CMD_DIR_RD);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + while (len > 0) {
>>> + tmp = readl_relaxed(sfc->regbase + SFC_DATA);
>>> + for (i = 0; i < len; i++)
>>> + *buf++ = (u8)((tmp >> (i * 8)) & 0xff);
>>
>> Won't this fail for len > 4 ?
>
> nope, this loop will reduce 4 for each successful readl. And
> reading the remained bytes which isn't aligned to DWORD, isn't it?
Try for len = 8 ... it will write 8 bytes to the buf, but half of them
would be zero. I believe it should look like:
for (i = 0; i < 4 /* was len */; i++)
*buf++ = (u8)((tmp >> (i * 8)) & 0xff);
>>
>> Also, you can use ioread32_rep() here, but (!) that won't work for
>> unaligned reads, which I dunno if they can happen here, but please do
>> double-check.
>
> yes, I have checked this API as well as others like memcpy_{to,from}io
> , etc. They will generate a external abort for arm core as the unaligned
> (DWORD) read/write via AHB aren't supported by Rockchip Socs. So I have
> to open code these stuff. This could be easily found for other
> upstreamed rockchip drivers. :)
This is normal, but you can still use the _rep variant if you handle the
corner cases.
>>
>>> + len = len - 4;
>>> + }
>>> +
>>> + return 0;
>>> +}
[...]
>>> +static ssize_t rockchip_sfc_write(struct spi_nor *nor, loff_t to,
>>> + size_t len, const u_char *write_buf)
>>> +{
>>> + struct rockchip_sfc_priv *priv = nor->priv;
>>> + struct rockchip_sfc *sfc = priv->sfc;
>>> + size_t offset;
>>> + int ret;
>>> + dma_addr_t dma_addr = 0;
>>> +
>>> + if (!sfc->use_dma)
>>> + goto no_dma;
>>
>> Seems like there's a lot of similarity between read/write .
>
> I was thinking to combine read/write with a extra argument to
> indicate WR/RD. But as we could see still some differece between
> WR and RD and there are already some condiction checks. So it
> will make the code hard to read with stuffing lots of condition
> checks. So I splited out read and write strightforward. :)
Hrm, is it that bad ?
>>> + for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
>>> + size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
>>> +
>>> + dma_addr = dma_map_single(NULL, (void *)write_buf,
>>> + trans, DMA_TO_DEVICE);
>>> + if (dma_mapping_error(sfc->dev, dma_addr)) {
>>> + dma_addr = 0;
>>> + memcpy(sfc->buffer, write_buf + offset, trans);
>>> + }
>>> +
>>> + /* Fail to map dma, use pre-allocated area instead */
>>> + ret = rockchip_sfc_dma_transfer(nor, to + offset,
>>> + dma_addr ? dma_addr :
>>> + sfc->dma_buffer,
>>> + trans, SFC_CMD_DIR_WR);
>>> + if (dma_addr)
>>> + dma_unmap_single(NULL, dma_addr,
>>> + trans, DMA_TO_DEVICE);
>>> + if (ret) {
>>> + dev_warn(nor->dev, "DMA write timeout\n");
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> + return len;
>>> +no_dma:
>>> + ret = rockchip_sfc_pio_transfer(nor, to, len,
>>> + (u_char *)write_buf, SFC_CMD_DIR_WR);
>>> + if (ret) {
>>> + dev_warn(nor->dev, "PIO write timeout\n");
>>> + return ret;
>>> + }
>>> + return len;
>>> +}
[...]
>>> +static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i < sfc->num_chip; i++)
>>> + mtd_device_unregister(&sfc->nor[i]->mtd);
>>> +}
>>> +
>>> +static int rockchip_sfc_register_all(struct rockchip_sfc *sfc)
>>> +{
>>> + struct device *dev = sfc->dev;
>>> + struct device_node *np;
>>> + int ret;
>>> +
>>> + for_each_available_child_of_node(dev->of_node, np) {
>>> + ret = rockchip_sfc_register(np, sfc);
>>> + if (ret)
>>> + goto fail;
>>> +
>>> + if (sfc->num_chip == SFC_MAX_CHIP_NUM) {
>>> + dev_warn(dev, "Exceeds the max cs limitation\n");
>>> + break;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +fail:
>>> + dev_err(dev, "Failed to register all chip\n");
>>> + rockchip_sfc_unregister_all(sfc);
>>
>> See cadence qspi where we only unregister the registered flashes.
>> Implement it the same way here.
>>
>
> yup, but I'm afraid that rockchip_sfc_unregister_all confused you
> as it actually unregisters the registered ones, not for all.
>
> static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
> {
> int i;
>
> for (i = 0; i < sfc->num_chip; i++)
> mtd_device_unregister(&sfc->nor[i]->mtd);
> }
>
> sfc->num_chip stands for how many flashes registered successfully.
Does it work if you have a hole in there ? Like if you have a flash on
chipselect 0 and chipselect 2 ?
>>> + return ret;
>>> +}
[...]
--
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nor: add rockchip serial flash controller driver
@ 2016-11-20 21:11 ` Marek Vasut
0 siblings, 0 replies; 20+ messages in thread
From: Marek Vasut @ 2016-11-20 21:11 UTC (permalink / raw)
To: Shawn Lin, Rob Herring, David Woodhouse, Brian Norris
Cc: devicetree, linux-mtd, Heiko Stuebner, linux-rockchip
On 11/16/2016 02:59 AM, Shawn Lin wrote:
> Hi Marek,
Hi,
[...]
>>> +static int rockchip_sfc_read_reg(struct spi_nor *nor, u8 opcode,
>>> + u8 *buf, int len)
>>> +{
>>> + struct rockchip_sfc_priv *priv = nor->priv;
>>> + struct rockchip_sfc *sfc = priv->sfc;
>>> + int ret;
>>> + u32 tmp;
>>> + u32 i;
>>> +
>>> + ret = rockchip_sfc_op_reg(nor, opcode, len, SFC_CMD_DIR_RD);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + while (len > 0) {
>>> + tmp = readl_relaxed(sfc->regbase + SFC_DATA);
>>> + for (i = 0; i < len; i++)
>>> + *buf++ = (u8)((tmp >> (i * 8)) & 0xff);
>>
>> Won't this fail for len > 4 ?
>
> nope, this loop will reduce 4 for each successful readl. And
> reading the remained bytes which isn't aligned to DWORD, isn't it?
Try for len = 8 ... it will write 8 bytes to the buf, but half of them
would be zero. I believe it should look like:
for (i = 0; i < 4 /* was len */; i++)
*buf++ = (u8)((tmp >> (i * 8)) & 0xff);
>>
>> Also, you can use ioread32_rep() here, but (!) that won't work for
>> unaligned reads, which I dunno if they can happen here, but please do
>> double-check.
>
> yes, I have checked this API as well as others like memcpy_{to,from}io
> , etc. They will generate a external abort for arm core as the unaligned
> (DWORD) read/write via AHB aren't supported by Rockchip Socs. So I have
> to open code these stuff. This could be easily found for other
> upstreamed rockchip drivers. :)
This is normal, but you can still use the _rep variant if you handle the
corner cases.
>>
>>> + len = len - 4;
>>> + }
>>> +
>>> + return 0;
>>> +}
[...]
>>> +static ssize_t rockchip_sfc_write(struct spi_nor *nor, loff_t to,
>>> + size_t len, const u_char *write_buf)
>>> +{
>>> + struct rockchip_sfc_priv *priv = nor->priv;
>>> + struct rockchip_sfc *sfc = priv->sfc;
>>> + size_t offset;
>>> + int ret;
>>> + dma_addr_t dma_addr = 0;
>>> +
>>> + if (!sfc->use_dma)
>>> + goto no_dma;
>>
>> Seems like there's a lot of similarity between read/write .
>
> I was thinking to combine read/write with a extra argument to
> indicate WR/RD. But as we could see still some differece between
> WR and RD and there are already some condiction checks. So it
> will make the code hard to read with stuffing lots of condition
> checks. So I splited out read and write strightforward. :)
Hrm, is it that bad ?
>>> + for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
>>> + size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
>>> +
>>> + dma_addr = dma_map_single(NULL, (void *)write_buf,
>>> + trans, DMA_TO_DEVICE);
>>> + if (dma_mapping_error(sfc->dev, dma_addr)) {
>>> + dma_addr = 0;
>>> + memcpy(sfc->buffer, write_buf + offset, trans);
>>> + }
>>> +
>>> + /* Fail to map dma, use pre-allocated area instead */
>>> + ret = rockchip_sfc_dma_transfer(nor, to + offset,
>>> + dma_addr ? dma_addr :
>>> + sfc->dma_buffer,
>>> + trans, SFC_CMD_DIR_WR);
>>> + if (dma_addr)
>>> + dma_unmap_single(NULL, dma_addr,
>>> + trans, DMA_TO_DEVICE);
>>> + if (ret) {
>>> + dev_warn(nor->dev, "DMA write timeout\n");
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> + return len;
>>> +no_dma:
>>> + ret = rockchip_sfc_pio_transfer(nor, to, len,
>>> + (u_char *)write_buf, SFC_CMD_DIR_WR);
>>> + if (ret) {
>>> + dev_warn(nor->dev, "PIO write timeout\n");
>>> + return ret;
>>> + }
>>> + return len;
>>> +}
[...]
>>> +static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i < sfc->num_chip; i++)
>>> + mtd_device_unregister(&sfc->nor[i]->mtd);
>>> +}
>>> +
>>> +static int rockchip_sfc_register_all(struct rockchip_sfc *sfc)
>>> +{
>>> + struct device *dev = sfc->dev;
>>> + struct device_node *np;
>>> + int ret;
>>> +
>>> + for_each_available_child_of_node(dev->of_node, np) {
>>> + ret = rockchip_sfc_register(np, sfc);
>>> + if (ret)
>>> + goto fail;
>>> +
>>> + if (sfc->num_chip == SFC_MAX_CHIP_NUM) {
>>> + dev_warn(dev, "Exceeds the max cs limitation\n");
>>> + break;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +fail:
>>> + dev_err(dev, "Failed to register all chip\n");
>>> + rockchip_sfc_unregister_all(sfc);
>>
>> See cadence qspi where we only unregister the registered flashes.
>> Implement it the same way here.
>>
>
> yup, but I'm afraid that rockchip_sfc_unregister_all confused you
> as it actually unregisters the registered ones, not for all.
>
> static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
> {
> int i;
>
> for (i = 0; i < sfc->num_chip; i++)
> mtd_device_unregister(&sfc->nor[i]->mtd);
> }
>
> sfc->num_chip stands for how many flashes registered successfully.
Does it work if you have a hole in there ? Like if you have a flash on
chipselect 0 and chipselect 2 ?
>>> + return ret;
>>> +}
[...]
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nor: add rockchip serial flash controller driver
2016-11-20 21:11 ` Marek Vasut
@ 2016-11-21 2:51 ` Shawn Lin
-1 siblings, 0 replies; 20+ messages in thread
From: Shawn Lin @ 2016-11-21 2:51 UTC (permalink / raw)
To: Marek Vasut, Rob Herring, David Woodhouse, Brian Norris
Cc: shawn.lin-TNX95d0MmH7DzftRWevZcw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Hi Marek,
On 2016/11/21 5:11, Marek Vasut wrote:
> On 11/16/2016 02:59 AM, Shawn Lin wrote:
>> Hi Marek,
>
> Hi,
>
> [...]
>
>>>> +static int rockchip_sfc_read_reg(struct spi_nor *nor, u8 opcode,
>>>> + u8 *buf, int len)
>>>> +{
>>>> + struct rockchip_sfc_priv *priv = nor->priv;
>>>> + struct rockchip_sfc *sfc = priv->sfc;
>>>> + int ret;
>>>> + u32 tmp;
>>>> + u32 i;
>>>> +
>>>> + ret = rockchip_sfc_op_reg(nor, opcode, len, SFC_CMD_DIR_RD);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + while (len > 0) {
>>>> + tmp = readl_relaxed(sfc->regbase + SFC_DATA);
>>>> + for (i = 0; i < len; i++)
>>>> + *buf++ = (u8)((tmp >> (i * 8)) & 0xff);
>>>
>>> Won't this fail for len > 4 ?
>>
>> nope, this loop will reduce 4 for each successful readl. And
>> reading the remained bytes which isn't aligned to DWORD, isn't it?
>
> Try for len = 8 ... it will write 8 bytes to the buf, but half of them
> would be zero. I believe it should look like:
>
> for (i = 0; i < 4 /* was len */; i++)
> *buf++ = (u8)((tmp >> (i * 8)) & 0xff);
>
you're right, I was misunderstanding your comment and fixed it in V2. :)
>>>
>>> Also, you can use ioread32_rep() here, but (!) that won't work for
>>> unaligned reads, which I dunno if they can happen here, but please do
>>> double-check.
>>
>> yes, I have checked this API as well as others like memcpy_{to,from}io
>> , etc. They will generate a external abort for arm core as the unaligned
>> (DWORD) read/write via AHB aren't supported by Rockchip Socs. So I have
>> to open code these stuff. This could be easily found for other
>> upstreamed rockchip drivers. :)
>
> This is normal, but you can still use the _rep variant if you handle the
> corner cases.
>
Sure, I will keep improving it once more comment for my v2 sent last
friday there. :)
>>>
>>>> + len = len - 4;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>
> [...]
>
>>>> +static ssize_t rockchip_sfc_write(struct spi_nor *nor, loff_t to,
>>>> + size_t len, const u_char *write_buf)
>>>> +{
>>>> + struct rockchip_sfc_priv *priv = nor->priv;
>>>> + struct rockchip_sfc *sfc = priv->sfc;
>>>> + size_t offset;
>>>> + int ret;
>>>> + dma_addr_t dma_addr = 0;
>>>> +
>>>> + if (!sfc->use_dma)
>>>> + goto no_dma;
>>>
>>> Seems like there's a lot of similarity between read/write .
>>
>> I was thinking to combine read/write with a extra argument to
>> indicate WR/RD. But as we could see still some differece between
>> WR and RD and there are already some condiction checks. So it
>> will make the code hard to read with stuffing lots of condition
>> checks. So I splited out read and write strightforward. :)
>
> Hrm, is it that bad ?
>
>>>> + for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
>>>> + size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
>>>> +
>>>> + dma_addr = dma_map_single(NULL, (void *)write_buf,
>>>> + trans, DMA_TO_DEVICE);
>>>> + if (dma_mapping_error(sfc->dev, dma_addr)) {
>>>> + dma_addr = 0;
>>>> + memcpy(sfc->buffer, write_buf + offset, trans);
>>>> + }
>>>> +
>>>> + /* Fail to map dma, use pre-allocated area instead */
>>>> + ret = rockchip_sfc_dma_transfer(nor, to + offset,
>>>> + dma_addr ? dma_addr :
>>>> + sfc->dma_buffer,
>>>> + trans, SFC_CMD_DIR_WR);
>>>> + if (dma_addr)
>>>> + dma_unmap_single(NULL, dma_addr,
>>>> + trans, DMA_TO_DEVICE);
>>>> + if (ret) {
>>>> + dev_warn(nor->dev, "DMA write timeout\n");
>>>> + return ret;
>>>> + }
>>>> + }
>>>> +
>>>> + return len;
>>>> +no_dma:
>>>> + ret = rockchip_sfc_pio_transfer(nor, to, len,
>>>> + (u_char *)write_buf, SFC_CMD_DIR_WR);
>>>> + if (ret) {
>>>> + dev_warn(nor->dev, "PIO write timeout\n");
>>>> + return ret;
>>>> + }
>>>> + return len;
>>>> +}
>
> [...]
>
>>>> +static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
>>>> +{
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < sfc->num_chip; i++)
>>>> + mtd_device_unregister(&sfc->nor[i]->mtd);
>>>> +}
>>>> +
>>>> +static int rockchip_sfc_register_all(struct rockchip_sfc *sfc)
>>>> +{
>>>> + struct device *dev = sfc->dev;
>>>> + struct device_node *np;
>>>> + int ret;
>>>> +
>>>> + for_each_available_child_of_node(dev->of_node, np) {
>>>> + ret = rockchip_sfc_register(np, sfc);
>>>> + if (ret)
>>>> + goto fail;
>>>> +
>>>> + if (sfc->num_chip == SFC_MAX_CHIP_NUM) {
>>>> + dev_warn(dev, "Exceeds the max cs limitation\n");
>>>> + break;
>>>> + }
>>>> + }
>>>> +
>>>> + return 0;
>>>> +
>>>> +fail:
>>>> + dev_err(dev, "Failed to register all chip\n");
>>>> + rockchip_sfc_unregister_all(sfc);
>>>
>>> See cadence qspi where we only unregister the registered flashes.
>>> Implement it the same way here.
>>>
>>
>> yup, but I'm afraid that rockchip_sfc_unregister_all confused you
>> as it actually unregisters the registered ones, not for all.
>>
>> static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
>> {
>> int i;
>>
>> for (i = 0; i < sfc->num_chip; i++)
>> mtd_device_unregister(&sfc->nor[i]->mtd);
>> }
>>
>> sfc->num_chip stands for how many flashes registered successfully.
>
> Does it work if you have a hole in there ? Like if you have a flash on
> chipselect 0 and chipselect 2 ?
Yes it does, as it won't leave a room for chipselect 1 whose node isn't
present, which means there isn't a hole in there at all. :)
>
>>>> + return ret;
>>>> +}
>
> [...]
>
>
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nor: add rockchip serial flash controller driver
@ 2016-11-21 2:51 ` Shawn Lin
0 siblings, 0 replies; 20+ messages in thread
From: Shawn Lin @ 2016-11-21 2:51 UTC (permalink / raw)
To: Marek Vasut, Rob Herring, David Woodhouse, Brian Norris
Cc: shawn.lin, devicetree, linux-mtd, Heiko Stuebner, linux-rockchip
Hi Marek,
On 2016/11/21 5:11, Marek Vasut wrote:
> On 11/16/2016 02:59 AM, Shawn Lin wrote:
>> Hi Marek,
>
> Hi,
>
> [...]
>
>>>> +static int rockchip_sfc_read_reg(struct spi_nor *nor, u8 opcode,
>>>> + u8 *buf, int len)
>>>> +{
>>>> + struct rockchip_sfc_priv *priv = nor->priv;
>>>> + struct rockchip_sfc *sfc = priv->sfc;
>>>> + int ret;
>>>> + u32 tmp;
>>>> + u32 i;
>>>> +
>>>> + ret = rockchip_sfc_op_reg(nor, opcode, len, SFC_CMD_DIR_RD);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + while (len > 0) {
>>>> + tmp = readl_relaxed(sfc->regbase + SFC_DATA);
>>>> + for (i = 0; i < len; i++)
>>>> + *buf++ = (u8)((tmp >> (i * 8)) & 0xff);
>>>
>>> Won't this fail for len > 4 ?
>>
>> nope, this loop will reduce 4 for each successful readl. And
>> reading the remained bytes which isn't aligned to DWORD, isn't it?
>
> Try for len = 8 ... it will write 8 bytes to the buf, but half of them
> would be zero. I believe it should look like:
>
> for (i = 0; i < 4 /* was len */; i++)
> *buf++ = (u8)((tmp >> (i * 8)) & 0xff);
>
you're right, I was misunderstanding your comment and fixed it in V2. :)
>>>
>>> Also, you can use ioread32_rep() here, but (!) that won't work for
>>> unaligned reads, which I dunno if they can happen here, but please do
>>> double-check.
>>
>> yes, I have checked this API as well as others like memcpy_{to,from}io
>> , etc. They will generate a external abort for arm core as the unaligned
>> (DWORD) read/write via AHB aren't supported by Rockchip Socs. So I have
>> to open code these stuff. This could be easily found for other
>> upstreamed rockchip drivers. :)
>
> This is normal, but you can still use the _rep variant if you handle the
> corner cases.
>
Sure, I will keep improving it once more comment for my v2 sent last
friday there. :)
>>>
>>>> + len = len - 4;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>
> [...]
>
>>>> +static ssize_t rockchip_sfc_write(struct spi_nor *nor, loff_t to,
>>>> + size_t len, const u_char *write_buf)
>>>> +{
>>>> + struct rockchip_sfc_priv *priv = nor->priv;
>>>> + struct rockchip_sfc *sfc = priv->sfc;
>>>> + size_t offset;
>>>> + int ret;
>>>> + dma_addr_t dma_addr = 0;
>>>> +
>>>> + if (!sfc->use_dma)
>>>> + goto no_dma;
>>>
>>> Seems like there's a lot of similarity between read/write .
>>
>> I was thinking to combine read/write with a extra argument to
>> indicate WR/RD. But as we could see still some differece between
>> WR and RD and there are already some condiction checks. So it
>> will make the code hard to read with stuffing lots of condition
>> checks. So I splited out read and write strightforward. :)
>
> Hrm, is it that bad ?
>
>>>> + for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
>>>> + size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
>>>> +
>>>> + dma_addr = dma_map_single(NULL, (void *)write_buf,
>>>> + trans, DMA_TO_DEVICE);
>>>> + if (dma_mapping_error(sfc->dev, dma_addr)) {
>>>> + dma_addr = 0;
>>>> + memcpy(sfc->buffer, write_buf + offset, trans);
>>>> + }
>>>> +
>>>> + /* Fail to map dma, use pre-allocated area instead */
>>>> + ret = rockchip_sfc_dma_transfer(nor, to + offset,
>>>> + dma_addr ? dma_addr :
>>>> + sfc->dma_buffer,
>>>> + trans, SFC_CMD_DIR_WR);
>>>> + if (dma_addr)
>>>> + dma_unmap_single(NULL, dma_addr,
>>>> + trans, DMA_TO_DEVICE);
>>>> + if (ret) {
>>>> + dev_warn(nor->dev, "DMA write timeout\n");
>>>> + return ret;
>>>> + }
>>>> + }
>>>> +
>>>> + return len;
>>>> +no_dma:
>>>> + ret = rockchip_sfc_pio_transfer(nor, to, len,
>>>> + (u_char *)write_buf, SFC_CMD_DIR_WR);
>>>> + if (ret) {
>>>> + dev_warn(nor->dev, "PIO write timeout\n");
>>>> + return ret;
>>>> + }
>>>> + return len;
>>>> +}
>
> [...]
>
>>>> +static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
>>>> +{
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < sfc->num_chip; i++)
>>>> + mtd_device_unregister(&sfc->nor[i]->mtd);
>>>> +}
>>>> +
>>>> +static int rockchip_sfc_register_all(struct rockchip_sfc *sfc)
>>>> +{
>>>> + struct device *dev = sfc->dev;
>>>> + struct device_node *np;
>>>> + int ret;
>>>> +
>>>> + for_each_available_child_of_node(dev->of_node, np) {
>>>> + ret = rockchip_sfc_register(np, sfc);
>>>> + if (ret)
>>>> + goto fail;
>>>> +
>>>> + if (sfc->num_chip == SFC_MAX_CHIP_NUM) {
>>>> + dev_warn(dev, "Exceeds the max cs limitation\n");
>>>> + break;
>>>> + }
>>>> + }
>>>> +
>>>> + return 0;
>>>> +
>>>> +fail:
>>>> + dev_err(dev, "Failed to register all chip\n");
>>>> + rockchip_sfc_unregister_all(sfc);
>>>
>>> See cadence qspi where we only unregister the registered flashes.
>>> Implement it the same way here.
>>>
>>
>> yup, but I'm afraid that rockchip_sfc_unregister_all confused you
>> as it actually unregisters the registered ones, not for all.
>>
>> static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
>> {
>> int i;
>>
>> for (i = 0; i < sfc->num_chip; i++)
>> mtd_device_unregister(&sfc->nor[i]->mtd);
>> }
>>
>> sfc->num_chip stands for how many flashes registered successfully.
>
> Does it work if you have a hole in there ? Like if you have a flash on
> chipselect 0 and chipselect 2 ?
Yes it does, as it won't leave a room for chipselect 1 whose node isn't
present, which means there isn't a hole in there at all. :)
>
>>>> + return ret;
>>>> +}
>
> [...]
>
>
--
Best Regards
Shawn Lin
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nor: add rockchip serial flash controller driver
2016-11-21 2:51 ` Shawn Lin
@ 2016-11-25 13:54 ` Marek Vasut
-1 siblings, 0 replies; 20+ messages in thread
From: Marek Vasut @ 2016-11-25 13:54 UTC (permalink / raw)
To: Shawn Lin, Rob Herring, David Woodhouse, Brian Norris
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On 11/21/2016 03:51 AM, Shawn Lin wrote:
> Hi Marek,
Hi!
[...]
>>> sfc->num_chip stands for how many flashes registered successfully.
>>
>> Does it work if you have a hole in there ? Like if you have a flash on
>> chipselect 0 and chipselect 2 ?
>
> Yes it does, as it won't leave a room for chipselect 1 whose node isn't
> present, which means there isn't a hole in there at all. :)
Ah , because you are not indexing those NORs with their CS number , right ?
--
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nor: add rockchip serial flash controller driver
@ 2016-11-25 13:54 ` Marek Vasut
0 siblings, 0 replies; 20+ messages in thread
From: Marek Vasut @ 2016-11-25 13:54 UTC (permalink / raw)
To: Shawn Lin, Rob Herring, David Woodhouse, Brian Norris
Cc: devicetree, linux-mtd, Heiko Stuebner, linux-rockchip
On 11/21/2016 03:51 AM, Shawn Lin wrote:
> Hi Marek,
Hi!
[...]
>>> sfc->num_chip stands for how many flashes registered successfully.
>>
>> Does it work if you have a hole in there ? Like if you have a flash on
>> chipselect 0 and chipselect 2 ?
>
> Yes it does, as it won't leave a room for chipselect 1 whose node isn't
> present, which means there isn't a hole in there at all. :)
Ah , because you are not indexing those NORs with their CS number , right ?
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nor: add rockchip serial flash controller driver
2016-11-25 13:54 ` Marek Vasut
@ 2016-11-30 0:57 ` Shawn Lin
-1 siblings, 0 replies; 20+ messages in thread
From: Shawn Lin @ 2016-11-30 0:57 UTC (permalink / raw)
To: Marek Vasut, Rob Herring, David Woodhouse, Brian Norris
Cc: shawn.lin-TNX95d0MmH7DzftRWevZcw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Heiko Stuebner,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
在 2016/11/25 21:54, Marek Vasut 写道:
> On 11/21/2016 03:51 AM, Shawn Lin wrote:
>> Hi Marek,
>
> Hi!
>
> [...]
>
>>>> sfc->num_chip stands for how many flashes registered successfully.
>>>
>>> Does it work if you have a hole in there ? Like if you have a flash on
>>> chipselect 0 and chipselect 2 ?
>>
>> Yes it does, as it won't leave a room for chipselect 1 whose node isn't
>> present, which means there isn't a hole in there at all. :)
>
> Ah , because you are not indexing those NORs with their CS number , right ?
yes. :)
>
>
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nor: add rockchip serial flash controller driver
@ 2016-11-30 0:57 ` Shawn Lin
0 siblings, 0 replies; 20+ messages in thread
From: Shawn Lin @ 2016-11-30 0:57 UTC (permalink / raw)
To: Marek Vasut, Rob Herring, David Woodhouse, Brian Norris
Cc: shawn.lin, devicetree, linux-mtd, Heiko Stuebner, linux-rockchip
在 2016/11/25 21:54, Marek Vasut 写道:
> On 11/21/2016 03:51 AM, Shawn Lin wrote:
>> Hi Marek,
>
> Hi!
>
> [...]
>
>>>> sfc->num_chip stands for how many flashes registered successfully.
>>>
>>> Does it work if you have a hole in there ? Like if you have a flash on
>>> chipselect 0 and chipselect 2 ?
>>
>> Yes it does, as it won't leave a room for chipselect 1 whose node isn't
>> present, which means there isn't a hole in there at all. :)
>
> Ah , because you are not indexing those NORs with their CS number , right ?
yes. :)
>
>
--
Best Regards
Shawn Lin
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2016-11-30 0:57 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-11 9:16 [PATCH 0/2] Add rockchip serial flash controller support Shawn Lin
2016-11-11 9:16 ` Shawn Lin
[not found] ` <1478855766-151673-1-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2016-11-11 9:16 ` [PATCH 1/2] mtd: spi-nor: Bindings for Rockchip serial flash controller Shawn Lin
2016-11-11 9:16 ` Shawn Lin
[not found] ` <1478855766-151673-2-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2016-11-15 15:05 ` Rob Herring
2016-11-15 15:05 ` Rob Herring
2016-11-11 9:16 ` [PATCH 2/2] mtd: spi-nor: add rockchip serial flash controller driver Shawn Lin
2016-11-11 9:16 ` Shawn Lin
[not found] ` <1478855766-151673-3-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2016-11-15 20:52 ` Marek Vasut
2016-11-15 20:52 ` Marek Vasut
[not found] ` <b1420e9d-b6a1-7fe8-4381-e32e0bc7dd53-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2016-11-16 1:59 ` Shawn Lin
2016-11-16 1:59 ` Shawn Lin
[not found] ` <775a8918-bc93-218a-808d-2a160137ad56-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2016-11-20 21:11 ` Marek Vasut
2016-11-20 21:11 ` Marek Vasut
[not found] ` <8f8213a1-f694-8159-fdbd-5e607c8aaaa2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2016-11-21 2:51 ` Shawn Lin
2016-11-21 2:51 ` Shawn Lin
[not found] ` <76670120-dacd-ef0e-cf36-cbf549b19853-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2016-11-25 13:54 ` Marek Vasut
2016-11-25 13:54 ` Marek Vasut
[not found] ` <b513a489-5913-ffe8-69f1-7201943a05a3-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2016-11-30 0:57 ` Shawn Lin
2016-11-30 0:57 ` Shawn Lin
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.