[RFC 0/6] Add support for Tegra20/30 NOR bus controller

[RFC 0/6] Add support for Tegra20/30 NOR bus controller

[Mirza Krak]

From: Mirza Krak <mirza.krak@gmail.com>

Hi.

Sending this series as RFC since I have some uncertainties.

What I am attempting to do with this series is to add support for the NOR flash
controller which can be found on Tegra20 and Tegra30 SOCs. This controller is
also references as SNOR and GMI in the NVIDIA TRM.

I have tested this series on a Tegra30 using a Colibri T30 SOM on a custom
carrier board which has multiple CAN controllers (SJA1000) connected to its
NOR bus.

Now to my question:

- I am not sure about the name of the driver. There was already CLK defines
for this controller in the kernel and their naming was TEGRA_NOR and I went with
that. But I certainly like the Generic Memory Interface (GMI) name better which
is used in the NVIDIA TRM. It does seem odd to connect CAN controllers to
something called a NOR bus.

- I am also not sure about the nvidia,config property. Is it preferred to split
this up? Splitting will probably create 10 different properties, rdy_polarity,
adv_polarity, oe_we_polarity and more.

- I also wanted to verify if I am on the correct path since I am a novice and
this would be my first driver to mainline.


Best Regards,
Mirza

Mirza Krak (6):
  clk: tegra: add TEGRA20_CLK_NOR to init table
  clk: tegra: add TEGRA30_CLK_NOR to init table
  dt/bindings: Add bindings for Tegra20/30 NOR bus driver
  ARM: tegra: Add Tegra30 NOR support
  ARM: tegra: Add Tegra20 NOR support
  bus: Add support for Tegra NOR controller

 .../devicetree/bindings/bus/nvidia,tegra20-nor.txt |  73 +++++++++++++
 arch/arm/boot/dts/tegra20.dtsi                     |  12 +++
 arch/arm/boot/dts/tegra30.dtsi                     |  11 ++
 drivers/bus/Kconfig                                |   7 ++
 drivers/bus/Makefile                               |   1 +
 drivers/bus/tegra-nor.c                            | 118 +++++++++++++++++++++
 drivers/clk/tegra/clk-tegra20.c                    |   1 +
 drivers/clk/tegra/clk-tegra30.c                    |   1 +
 8 files changed, 224 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
 create mode 100644 drivers/bus/tegra-nor.c

--
2.1.4

[RFC 1/6] clk: tegra: add TEGRA20_CLK_NOR to init table

[Mirza Krak]

From: Mirza Krak <mirza.krak@gmail.com>

Add TEGRA20_CLK_NOR to init tabel and set a "sane" default rate.

Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
---
 drivers/clk/tegra/clk-tegra20.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index 837e5cb..aefc044 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -1047,6 +1047,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
 	{ TEGRA20_CLK_SDMMC3, TEGRA20_CLK_PLL_P, 48000000, 0 },
 	{ TEGRA20_CLK_SDMMC4, TEGRA20_CLK_PLL_P, 48000000, 0 },
 	{ TEGRA20_CLK_SPI, TEGRA20_CLK_PLL_P, 20000000, 0 },
+	{ TEGRA20_CLK_NOR, TEGRA20_CLK_PLL_P, 86500000, 0 },
 	{ TEGRA20_CLK_SBC1, TEGRA20_CLK_PLL_P, 100000000, 0 },
 	{ TEGRA20_CLK_SBC2, TEGRA20_CLK_PLL_P, 100000000, 0 },
 	{ TEGRA20_CLK_SBC3, TEGRA20_CLK_PLL_P, 100000000, 0 },
-- 
2.1.4

[RFC 2/6] clk: tegra: add TEGRA30_CLK_NOR to init table

[Mirza Krak]

From: Mirza Krak <mirza.krak@gmail.com>

Add TEGRA30_CLK_NOR to init table and set a sane default rate.

Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
---
 drivers/clk/tegra/clk-tegra30.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
index 9396f49..5eda262 100644
--- a/drivers/clk/tegra/clk-tegra30.c
+++ b/drivers/clk/tegra/clk-tegra30.c
@@ -1361,6 +1361,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
 	{ TEGRA30_CLK_SDMMC1, TEGRA30_CLK_PLL_P, 48000000, 0 },
 	{ TEGRA30_CLK_SDMMC2, TEGRA30_CLK_PLL_P, 48000000, 0 },
 	{ TEGRA30_CLK_SDMMC3, TEGRA30_CLK_PLL_P, 48000000, 0 },
+	{ TEGRA30_CLK_NOR, TEGRA30_CLK_PLL_P, 86500000, 0 },
 	{ TEGRA30_CLK_PLL_M, TEGRA30_CLK_CLK_MAX, 0, 1 },
 	{ TEGRA30_CLK_PCLK, TEGRA30_CLK_CLK_MAX, 0, 1 },
 	{ TEGRA30_CLK_CSITE, TEGRA30_CLK_CLK_MAX, 0, 1 },
--
2.1.4

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Mirza Krak]

From: Mirza Krak <mirza.krak@gmail.com>

Document the devicetree bindings for NOR bus driver found on Tegra20 and
Tegra30 SOCs

Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
---
 .../devicetree/bindings/bus/nvidia,tegra20-nor.txt | 73 ++++++++++++++++++++++
 1 file changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt

diff --git a/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
new file mode 100644
index 0000000..9ee4a66
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
@@ -0,0 +1,73 @@
+Device tree bindings for NVIDIA Tegra20/30 NOR Bus
+
+The NOR controller supports a number of memory types, including synchronous NOR,
+asynchronous NOR, and other flash memories with similar interfaces, such as
+MuxOneNAND. One could also connect high speed devices like FPGAs, DSPs,
+CAN chips, Wi-Fi chips etc.
+
+The actual devices are instantiated from the child nodes of a NOR node.
+
+Required properties:
+
+ - compatible: should be "nvidia,tegra20-nor", "nvidia,tegra30-nor"
+ - reg: Should contain NOR controller registers location and length.
+ - clocks: Must contain one entry, for the module clock.
+   See ../clocks/clock-bindings.txt for details.
+ - resets : Must contain an entry for each entry in reset-names.
+   See ../reset/reset.txt for details.
+ - reset-names : Must include the following entries:
+  - nor
+ - #address-cells: Must be set to 2 to allow memory address translation
+ - #size-cells:	Must be set to 1 to allow CS address passing
+ - ranges: Must be set up to reflect the memory layout with four integer
+ 		values for each chip-select line in use.
+ - nvidia,config: This property represents the SNOR_CONFIG_0 register.
+
+Note that the NOR controller does not have any internal chip-select address
+decoding and if you want to access multiple devices external chip-select
+decoding must be provided.
+
+Optional properties:
+
+ - nvidia,cs-timing: The timing array represents the SNOR_TIMING0_0 and
+   SNOR_TIMING1_0 registers for the NOR controller. If unset reset-values will
+   be used. See reference documentation for detailed description of the timing
+   registers.
+
+Example with two SJA1000 CAN controllers connected to the NOR bus:
+
+	nor at 70009000 {
+		status = "okay";
+		compatible = "nvidia,tegra20-nor", "nvidia,tegra30-nor";
+		reg = <0x70009000 0x1000>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+		clocks = <&tegra_car TEGRA30_CLK_NOR>;
+		resets = < &tegra_car 42>;
+		reset-names = "nor";
+		ranges = <
+			0 0 0x48000000 0x00000100
+			1 0 0x48040000 0x00000100
+		>;
+
+		can at 0,0 {
+			compatible = "nxp,sja1000";
+			reg = <0 0 0x100>;
+			interrupt-parent = <&gpio>;
+			interrupts = <TEGRA_GPIO(B, 5) IRQ_TYPE_EDGE_RISING>;
+			nxp,external-clock-frequency = <24000000>;
+			nxp,tx-output-config = <0x16>;
+			nxp,clock-out-frequency = <24000000>;
+			reg-io-width = <2>;
+		};
+
+
+		can at 1,0 {
+			compatible = "nxp,sja1000";
+			reg = <1 0 0x100>;
+			interrupt-parent = <&gpio>;
+			interrupts = <TEGRA_GPIO(A, 6) IRQ_TYPE_EDGE_RISING>;
+			nxp,external-clock-frequency = <24000000>;
+			nxp,tx-output-config = <0x16>;
+			reg-io-width = <2>;
+	};
-- 
2.1.4

[RFC 4/6] ARM: tegra: Add Tegra30 NOR support

[Mirza Krak]

From: Mirza Krak <mirza.krak@gmail.com>

Add a device node for the NOR controller found on Tegra30.

Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
---
 arch/arm/boot/dts/tegra30.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 5030065..002db10 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -439,6 +439,17 @@
 		status = "disabled";
 	};
 
+	nor at 70009000 {
+		compatible = "nvidia,tegra20-nor", "nvidia,tegra30-nor";
+		reg = <0x70009000 0x1000>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+		clocks = <&tegra_car TEGRA30_CLK_NOR>;
+		resets = < &tegra_car 42>;
+		reset-names = "nor";
+		status = "disabled";
+	};
+
 	pwm: pwm at 7000a000 {
 		compatible = "nvidia,tegra30-pwm", "nvidia,tegra20-pwm";
 		reg = <0x7000a000 0x100>;
-- 
2.1.4

[RFC 5/6] ARM: tegra: Add Tegra20 NOR support

[Mirza Krak]

From: Mirza Krak <mirza.krak@gmail.com>

Add a device node for the NOR controller found on Tegra20.

Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
---
 arch/arm/boot/dts/tegra20.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 2207c08..fa8b3c7 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -376,6 +376,18 @@
 		status = "disabled";
 	};

+
+	nor at 70009000 {
+		compatible = "nvidia,tegra20-nor", "nvidia,tegra30-nor";
+		reg = <70009000 0x1000>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+		clocks = <&tegra_car TEGRA20_CLK_NOR>;
+		resets = < &tegra_car 42>;
+		reset-names = "nor";
+		status = "disabled";
+	};
+
 	pwm: pwm at 7000a000 {
 		compatible = "nvidia,tegra20-pwm";
 		reg = <0x7000a000 0x100>;
--
2.1.4

[RFC 6/6] bus: Add support for Tegra NOR controller

[Mirza Krak]

From: Mirza Krak <mirza.krak@gmail.com>

The NOR bus can be used to connect high-speed devices such as NOR flash,
FPGAs, DSPs, CAN chips, Wi-Fi chips etc.

Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
---
 drivers/bus/Kconfig     |   7 +++
 drivers/bus/Makefile    |   1 +
 drivers/bus/tegra-nor.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 126 insertions(+)
 create mode 100644 drivers/bus/tegra-nor.c

diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 3b205e2..b74be7d8 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -145,6 +145,13 @@ config TEGRA_ACONNECT
 	  Driver for the Tegra ACONNECT bus which is used to interface with
 	  the devices inside the Audio Processing Engine (APE) for Tegra210.

+config TEGRA_NOR
+	tristate "Nvidia Tegra NOR flash bus driver a.k.a GMI/SNOR"
+		depends on ARCH_TEGRA_2x_SOC || ARCH_TEGRA_3x_SOC
+		depends on OF
+		help
+			Driver for NOR flash bus found on Tegra30/20 SOC`s.
+
 config UNIPHIER_SYSTEM_BUS
 	tristate "UniPhier System Bus driver"
 	depends on ARCH_UNIPHIER && OF
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index ac84cc4..46d0129 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -18,5 +18,6 @@ obj-$(CONFIG_OMAP_OCP2SCP)	+= omap-ocp2scp.o
 obj-$(CONFIG_SUNXI_RSB)		+= sunxi-rsb.o
 obj-$(CONFIG_SIMPLE_PM_BUS)	+= simple-pm-bus.o
 obj-$(CONFIG_TEGRA_ACONNECT)	+= tegra-aconnect.o
+obj-$(CONFIG_TEGRA_NOR)		+= tegra-nor.o
 obj-$(CONFIG_UNIPHIER_SYSTEM_BUS)	+= uniphier-system-bus.o
 obj-$(CONFIG_VEXPRESS_CONFIG)	+= vexpress-config.o
diff --git a/drivers/bus/tegra-nor.c b/drivers/bus/tegra-nor.c
new file mode 100644
index 0000000..1e48113
--- /dev/null
+++ b/drivers/bus/tegra-nor.c
@@ -0,0 +1,118 @@
+/*
+ * Driver for Nvidia NOR Flash bus a.k.a SNOR/GMI.
+ *
+ * Copyright (C) 2016 Host Mobility AB. All rights reserved.
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+
+#define TEGRA_NOR_TIMING_REG_COUNT	2
+
+#define TEGRA_NOR_CONFIG			0x00
+#define TEGRA_NOR_STATUS			0x04
+#define TEGRA_NOR_ADDR_PTR			0x08
+#define TEGRA_NOR_AHB_ADDR_PTR		0x0c
+#define TEGRA_NOR_TIMING0			0x10
+#define TEGRA_NOR_TIMING1			0x14
+#define TEGRA_NOR_MIO_CONFIG		0x18
+#define TEGRA_NOR_MIO_TIMING		0x1c
+#define TEGRA_NOR_DMA_CONFIG		0x20
+#define TEGRA_NOR_CS_MUX_CONFIG		0x24
+
+#define TEGRA_NOR_CONFIG_GO				BIT(31)
+
+static const struct of_device_id nor_id_table[] = {
+	/* Tegra30 */
+	{ .compatible = "nvidia,tegra30-nor", .data = NULL, },
+	/* Tegra20 */
+	{ .compatible = "nvidia,tegra20-nor", .data = NULL, },
+
+	{ }
+};
+MODULE_DEVICE_TABLE(of, nor_id_table);
+
+
+static int __init nor_parse_dt(struct platform_device *pdev,
+				void __iomem *base)
+{
+	struct device_node *of_node = pdev->dev.of_node;
+	u32 config, timing[TEGRA_NOR_TIMING_REG_COUNT];
+	int ret;
+
+	ret = of_property_read_u32_array(of_node, "nvidia,cs-timing",
+					 timing, TEGRA_NOR_TIMING_REG_COUNT);
+	if (!ret) {
+		writel(timing[0], base + TEGRA_NOR_TIMING0);
+		writel(timing[1], base + TEGRA_NOR_TIMING1);
+	}
+
+	ret = of_property_read_u32(of_node, "nvidia,config", &config);
+	if (ret)
+		return ret;
+
+	config |= TEGRA_NOR_CONFIG_GO;
+	writel(config, base + TEGRA_NOR_CONFIG);
+
+	if (of_get_child_count(of_node))
+		ret = of_platform_populate(of_node,
+				   of_default_bus_match_table,
+				   NULL, &pdev->dev);
+
+	return ret;
+}
+
+static int __init nor_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct clk *clk;
+	void __iomem *base;
+	int ret;
+
+	/* get the resource */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	/* get the clock */
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		return ret;
+
+	/* parse the device node */
+	ret = nor_parse_dt(pdev, base);
+	if (ret) {
+		dev_err(&pdev->dev, "%s fail to create devices.\n",
+			pdev->dev.of_node->full_name);
+		clk_disable_unprepare(clk);
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "Driver registered.\n");
+	return 0;
+}
+
+static struct platform_driver nor_driver = {
+	.driver = {
+		.name		= "tegra-nor",
+		.of_match_table	= nor_id_table,
+	},
+};
+module_platform_driver_probe(nor_driver, nor_probe);
+
+MODULE_AUTHOR("Mirza Krak <mirza.krak@gmail.com");
+MODULE_DESCRIPTION("NVIDIA Tegra NOR Bus Driver");
+MODULE_LICENSE("GPL");
--
2.1.4

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Rob Herring]

On Tue, Jul 19, 2016 at 03:36:34PM +0200, Mirza Krak wrote:
> From: Mirza Krak <mirza.krak@gmail.com>
> 
> Document the devicetree bindings for NOR bus driver found on Tegra20 and
> Tegra30 SOCs
> 
> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
> ---
>  .../devicetree/bindings/bus/nvidia,tegra20-nor.txt | 73 ++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> 
> diff --git a/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> new file mode 100644
> index 0000000..9ee4a66
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> @@ -0,0 +1,73 @@
> +Device tree bindings for NVIDIA Tegra20/30 NOR Bus
> +
> +The NOR controller supports a number of memory types, including synchronous NOR,
> +asynchronous NOR, and other flash memories with similar interfaces, such as
> +MuxOneNAND. One could also connect high speed devices like FPGAs, DSPs,
> +CAN chips, Wi-Fi chips etc.
> +
> +The actual devices are instantiated from the child nodes of a NOR node.
> +
> +Required properties:
> +
> + - compatible: should be "nvidia,tegra20-nor", "nvidia,tegra30-nor"
> + - reg: Should contain NOR controller registers location and length.
> + - clocks: Must contain one entry, for the module clock.
> +   See ../clocks/clock-bindings.txt for details.
> + - resets : Must contain an entry for each entry in reset-names.
> +   See ../reset/reset.txt for details.
> + - reset-names : Must include the following entries:
> +  - nor
> + - #address-cells: Must be set to 2 to allow memory address translation
> + - #size-cells:	Must be set to 1 to allow CS address passing
> + - ranges: Must be set up to reflect the memory layout with four integer
> + 		values for each chip-select line in use.
> + - nvidia,config: This property represents the SNOR_CONFIG_0 register.
> +
> +Note that the NOR controller does not have any internal chip-select address
> +decoding and if you want to access multiple devices external chip-select
> +decoding must be provided.

Then what are the 2 chip selects in ranges?

Rob

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Mirza Krak]

2016-07-20 14:44 GMT+02:00 Rob Herring <robh@kernel.org>:
> On Tue, Jul 19, 2016 at 03:36:34PM +0200, Mirza Krak wrote:
>> From: Mirza Krak <mirza.krak@gmail.com>
>>
>> Document the devicetree bindings for NOR bus driver found on Tegra20 and
>> Tegra30 SOCs
>>
>> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
>> ---
>>  .../devicetree/bindings/bus/nvidia,tegra20-nor.txt | 73 ++++++++++++++++++++++
>>  1 file changed, 73 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
>>
>> diff --git a/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
>> new file mode 100644
>> index 0000000..9ee4a66
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
>> @@ -0,0 +1,73 @@
>> +Device tree bindings for NVIDIA Tegra20/30 NOR Bus
>> +
>> +The NOR controller supports a number of memory types, including synchronous NOR,
>> +asynchronous NOR, and other flash memories with similar interfaces, such as
>> +MuxOneNAND. One could also connect high speed devices like FPGAs, DSPs,
>> +CAN chips, Wi-Fi chips etc.
>> +
>> +The actual devices are instantiated from the child nodes of a NOR node.
>> +
>> +Required properties:
>> +
>> + - compatible: should be "nvidia,tegra20-nor", "nvidia,tegra30-nor"
>> + - reg: Should contain NOR controller registers location and length.
>> + - clocks: Must contain one entry, for the module clock.
>> +   See ../clocks/clock-bindings.txt for details.
>> + - resets : Must contain an entry for each entry in reset-names.
>> +   See ../reset/reset.txt for details.
>> + - reset-names : Must include the following entries:
>> +  - nor
>> + - #address-cells: Must be set to 2 to allow memory address translation
>> + - #size-cells:      Must be set to 1 to allow CS address passing
>> + - ranges: Must be set up to reflect the memory layout with four integer
>> +             values for each chip-select line in use.
>> + - nvidia,config: This property represents the SNOR_CONFIG_0 register.
>> +
>> +Note that the NOR controller does not have any internal chip-select address
>> +decoding and if you want to access multiple devices external chip-select
>> +decoding must be provided.
>
> Then what are the 2 chip selects in ranges?
>
> Rob

Those two chip selects are actually a representation of a external
decoding logic based on what we use on our board. Even though it the
NOR controller only supports one single chip select I wanted to give
an example on how one could create more chip-selects with an external
logic and what it would look like in the device tree representation.

I realize that the bindings should include above explanation or
something similar.

-- 
Med V?nliga H?lsningar / Best Regards

Mirza Krak
mirza.krak at gmail.com
Mobile: +46730280622

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Jon Hunter]

On 19/07/16 14:36, Mirza Krak wrote:
> From: Mirza Krak <mirza.krak@gmail.com>
> 
> Document the devicetree bindings for NOR bus driver found on Tegra20 and
> Tegra30 SOCs
> 
> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
> ---
>  .../devicetree/bindings/bus/nvidia,tegra20-nor.txt | 73 ++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> 
> diff --git a/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> new file mode 100644
> index 0000000..9ee4a66
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> @@ -0,0 +1,73 @@
> +Device tree bindings for NVIDIA Tegra20/30 NOR Bus
> +
> +The NOR controller supports a number of memory types, including synchronous NOR,
> +asynchronous NOR, and other flash memories with similar interfaces, such as
> +MuxOneNAND. One could also connect high speed devices like FPGAs, DSPs,
> +CAN chips, Wi-Fi chips etc.

Nit-pick ... the Tegra documentation refers to this controller as the
GMI (general memory interface) or SNOR (sync-NOR) controller because it
is not just limited to NOR as you mentioned. I see references to GMI in
the Tegra pinctrl driver and so may be we should use this name.

> +
> +The actual devices are instantiated from the child nodes of a NOR node.
> +
> +Required properties:
> +
> + - compatible: should be "nvidia,tegra20-nor", "nvidia,tegra30-nor"

I see at least one difference at the register level between Tegra20 and
Tegra30 and so I think this should be something like ...

 - compatible : Should contain one of the following:
	For Tegra20 must contain "nvidia,tegra20-gmi".
 	For Tegra30 must contain "nvidia,tegra30-gmi".

> + - reg: Should contain NOR controller registers location and length.
> + - clocks: Must contain one entry, for the module clock.
> +   See ../clocks/clock-bindings.txt for details.
> + - resets : Must contain an entry for each entry in reset-names.
> +   See ../reset/reset.txt for details.
> + - reset-names : Must include the following entries:
> +  - nor
> + - #address-cells: Must be set to 2 to allow memory address translation
> + - #size-cells:	Must be set to 1 to allow CS address passing
> + - ranges: Must be set up to reflect the memory layout with four integer
> + 		values for each chip-select line in use.
> + - nvidia,config: This property represents the SNOR_CONFIG_0 register.

There is also a SNOR_MIO_CONFIG for the MIO address space and so I think
that this should be nvidia,snor-config to be explicit. It might be nice
to also add a "nvidia,mio-config" while you are at it as well, however,
that could always be done later. If you do, then the
"nvidia,snor-config" becomes optional depending on whether you are using
the SNOR or MIO address space.

Thierry, Stephen, do prefer all the fields on the config registers are
broken out? There are quite a few but I am not sure what we typically
recommend here?

> +
> +Note that the NOR controller does not have any internal chip-select address
> +decoding and if you want to access multiple devices external chip-select
> +decoding must be provided.

Although it is true, you do have the MIO address space and so you could
support two devices via the SNOR address space and MIO address space
(assuming that the MIO can be used for the 2nd device).

Furthermore, if you do have external logic to support multiple devices
this would assume that the devices use the same timing and so are
probably the same type. It also assumes both can fit in the 256MB
address range. May be worth mentioning.

The GMI does have 8 chip selects and I believe the purpose of these is
to allow you to address more than the 256MB range. However, I believe to
do this it require software intervention to change the current CS that
is in use.

I wonder if it is worth mentioning that the chip-select specified in the
"nvidia,config" prop should match that in the "ranges" prop unless you
have some external decoding logic to provide an external chip-select.
Which raises a question, what does the chip-select in the ranges
actually represent? I am not sure if there is a common practice here for
device tree when boards have external logic to provide additional
chip-selects. I am sure this is quite common.

> +Optional properties:
> +
> + - nvidia,cs-timing: The timing array represents the SNOR_TIMING0_0 and
> +   SNOR_TIMING1_0 registers for the NOR controller. If unset reset-values will
> +   be used. See reference documentation for detailed description of the timing
> +   registers.
> +
> +Example with two SJA1000 CAN controllers connected to the NOR bus:
> +
> +	nor at 70009000 {
> +		status = "okay";
> +		compatible = "nvidia,tegra20-nor", "nvidia,tegra30-nor";
> +		reg = <0x70009000 0x1000>;
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		clocks = <&tegra_car TEGRA30_CLK_NOR>;
> +		resets = < &tegra_car 42>;
> +		reset-names = "nor";
> +		ranges = <
> +			0 0 0x48000000 0x00000100
> +			1 0 0x48040000 0x00000100
> +		>;

The "nvidia,config" appears to be missing here.

Cheers
Jon

-- 
nvpublic

[RFC 6/6] bus: Add support for Tegra NOR controller

[Jon Hunter]

On 19/07/16 14:36, Mirza Krak wrote:
> From: Mirza Krak <mirza.krak@gmail.com>
> 
> The NOR bus can be used to connect high-speed devices such as NOR flash,
> FPGAs, DSPs, CAN chips, Wi-Fi chips etc.
> 
> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
> ---
>  drivers/bus/Kconfig     |   7 +++
>  drivers/bus/Makefile    |   1 +
>  drivers/bus/tegra-nor.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 126 insertions(+)
>  create mode 100644 drivers/bus/tegra-nor.c
> 
> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
> index 3b205e2..b74be7d8 100644
> --- a/drivers/bus/Kconfig
> +++ b/drivers/bus/Kconfig
> @@ -145,6 +145,13 @@ config TEGRA_ACONNECT
>  	  Driver for the Tegra ACONNECT bus which is used to interface with
>  	  the devices inside the Audio Processing Engine (APE) for Tegra210.
> 
> +config TEGRA_NOR
> +	tristate "Nvidia Tegra NOR flash bus driver a.k.a GMI/SNOR"
> +		depends on ARCH_TEGRA_2x_SOC || ARCH_TEGRA_3x_SOC
> +		depends on OF
> +		help
> +			Driver for NOR flash bus found on Tegra30/20 SOC`s.

It is actually present on all Tegra's and so I would drop the 30/20.

> +
>  config UNIPHIER_SYSTEM_BUS
>  	tristate "UniPhier System Bus driver"
>  	depends on ARCH_UNIPHIER && OF
> diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
> index ac84cc4..46d0129 100644
> --- a/drivers/bus/Makefile
> +++ b/drivers/bus/Makefile
> @@ -18,5 +18,6 @@ obj-$(CONFIG_OMAP_OCP2SCP)	+= omap-ocp2scp.o
>  obj-$(CONFIG_SUNXI_RSB)		+= sunxi-rsb.o
>  obj-$(CONFIG_SIMPLE_PM_BUS)	+= simple-pm-bus.o
>  obj-$(CONFIG_TEGRA_ACONNECT)	+= tegra-aconnect.o
> +obj-$(CONFIG_TEGRA_NOR)		+= tegra-nor.o
>  obj-$(CONFIG_UNIPHIER_SYSTEM_BUS)	+= uniphier-system-bus.o
>  obj-$(CONFIG_VEXPRESS_CONFIG)	+= vexpress-config.o
> diff --git a/drivers/bus/tegra-nor.c b/drivers/bus/tegra-nor.c
> new file mode 100644
> index 0000000..1e48113
> --- /dev/null
> +++ b/drivers/bus/tegra-nor.c
> @@ -0,0 +1,118 @@
> +/*
> + * Driver for Nvidia NOR Flash bus a.k.a SNOR/GMI.
> + *
> + * Copyright (C) 2016 Host Mobility AB. All rights reserved.
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/mfd/syscon.h>

Is this needed?

> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>

Or this?

> +
> +#define TEGRA_NOR_TIMING_REG_COUNT	2
> +
> +#define TEGRA_NOR_CONFIG			0x00
> +#define TEGRA_NOR_STATUS			0x04
> +#define TEGRA_NOR_ADDR_PTR			0x08
> +#define TEGRA_NOR_AHB_ADDR_PTR		0x0c
> +#define TEGRA_NOR_TIMING0			0x10
> +#define TEGRA_NOR_TIMING1			0x14
> +#define TEGRA_NOR_MIO_CONFIG		0x18
> +#define TEGRA_NOR_MIO_TIMING		0x1c
> +#define TEGRA_NOR_DMA_CONFIG		0x20
> +#define TEGRA_NOR_CS_MUX_CONFIG		0x24

Not all of these are used. It is good to define them and I wonder if we
should add support for MIO while are at it :-)

> +
> +#define TEGRA_NOR_CONFIG_GO				BIT(31)
> +
> +static const struct of_device_id nor_id_table[] = {
> +	/* Tegra30 */

I don't think this comment is needed.

> +	{ .compatible = "nvidia,tegra30-nor", .data = NULL, },

You don't need to set data to NULL.

> +	/* Tegra20 */
> +	{ .compatible = "nvidia,tegra20-nor", .data = NULL, },

Same here.

> +
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, nor_id_table);
> +
> +
> +static int __init nor_parse_dt(struct platform_device *pdev,
> +				void __iomem *base)
> +{
> +	struct device_node *of_node = pdev->dev.of_node;
> +	u32 config, timing[TEGRA_NOR_TIMING_REG_COUNT];
> +	int ret;
> +
> +	ret = of_property_read_u32_array(of_node, "nvidia,cs-timing",
> +					 timing, TEGRA_NOR_TIMING_REG_COUNT);
> +	if (!ret) {
> +		writel(timing[0], base + TEGRA_NOR_TIMING0);
> +		writel(timing[1], base + TEGRA_NOR_TIMING1);
> +	}
> +
> +	ret = of_property_read_u32(of_node, "nvidia,config", &config);
> +	if (ret)
> +		return ret;
> +
> +	config |= TEGRA_NOR_CONFIG_GO;
> +	writel(config, base + TEGRA_NOR_CONFIG);
> +
> +	if (of_get_child_count(of_node))
> +		ret = of_platform_populate(of_node,
> +				   of_default_bus_match_table,
> +				   NULL, &pdev->dev);
> +
> +	return ret;
> +}
> +
> +static int __init nor_probe(struct platform_device *pdev)
> +{

I would drop the __init.

> +	struct resource *res;
> +	struct clk *clk;
> +	void __iomem *base;
> +	int ret;
> +
> +	/* get the resource */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	/* get the clock */
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret)
> +		return ret;
> +
> +	/* parse the device node */
> +	ret = nor_parse_dt(pdev, base);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s fail to create devices.\n",
> +			pdev->dev.of_node->full_name);
> +		clk_disable_unprepare(clk);
> +		return ret;
> +	}
> +
> +	dev_info(&pdev->dev, "Driver registered.\n");
> +	return 0;
> +}
> +
> +static struct platform_driver nor_driver = {
> +	.driver = {
> +		.name		= "tegra-nor",
> +		.of_match_table	= nor_id_table,
> +	},
> +};

The driver should have a remove function given that we can build as a
module.

> +module_platform_driver_probe(nor_driver, nor_probe);

I would use "tegra_nor" namespace for all the structs, functions, etc.
However, we may prefer to go with GMI and in which case tegra_gmi_probe,
etc.

Cheers
Jon

-- 
nvpublic

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Jon Hunter]

On 20/07/16 20:28, Mirza Krak wrote:
> 2016-07-20 14:44 GMT+02:00 Rob Herring <robh@kernel.org>:
>> On Tue, Jul 19, 2016 at 03:36:34PM +0200, Mirza Krak wrote:
>>> From: Mirza Krak <mirza.krak@gmail.com>
>>>
>>> Document the devicetree bindings for NOR bus driver found on Tegra20 and
>>> Tegra30 SOCs
>>>
>>> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
>>> ---
>>>  .../devicetree/bindings/bus/nvidia,tegra20-nor.txt | 73 ++++++++++++++++++++++
>>>  1 file changed, 73 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
>>> new file mode 100644
>>> index 0000000..9ee4a66
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
>>> @@ -0,0 +1,73 @@
>>> +Device tree bindings for NVIDIA Tegra20/30 NOR Bus
>>> +
>>> +The NOR controller supports a number of memory types, including synchronous NOR,
>>> +asynchronous NOR, and other flash memories with similar interfaces, such as
>>> +MuxOneNAND. One could also connect high speed devices like FPGAs, DSPs,
>>> +CAN chips, Wi-Fi chips etc.
>>> +
>>> +The actual devices are instantiated from the child nodes of a NOR node.
>>> +
>>> +Required properties:
>>> +
>>> + - compatible: should be "nvidia,tegra20-nor", "nvidia,tegra30-nor"
>>> + - reg: Should contain NOR controller registers location and length.
>>> + - clocks: Must contain one entry, for the module clock.
>>> +   See ../clocks/clock-bindings.txt for details.
>>> + - resets : Must contain an entry for each entry in reset-names.
>>> +   See ../reset/reset.txt for details.
>>> + - reset-names : Must include the following entries:
>>> +  - nor
>>> + - #address-cells: Must be set to 2 to allow memory address translation
>>> + - #size-cells:      Must be set to 1 to allow CS address passing
>>> + - ranges: Must be set up to reflect the memory layout with four integer
>>> +             values for each chip-select line in use.
>>> + - nvidia,config: This property represents the SNOR_CONFIG_0 register.
>>> +
>>> +Note that the NOR controller does not have any internal chip-select address
>>> +decoding and if you want to access multiple devices external chip-select
>>> +decoding must be provided.
>>
>> Then what are the 2 chip selects in ranges?
>>
>> Rob
> 
> Those two chip selects are actually a representation of a external
> decoding logic based on what we use on our board. Even though it the
> NOR controller only supports one single chip select I wanted to give
> an example on how one could create more chip-selects with an external
> logic and what it would look like in the device tree representation.

Technically, the GMI/SNOR controller supports 8 chip-selects, however,
unlike some devices, it appears that software has to select the active
chip-select. Although this sounds odd, I believe that the idea is that
in order to support devices greater than 256MB (external address space
for available NOR/async devices) you can use the chip-selects to page
through memory greater than this 256MB range. At least that it my
(limited) understanding!

Cheers
Jon

-- 
nvpublic

[RFC 6/6] bus: Add support for Tegra NOR controller

[Jon Hunter]

On 19/07/16 14:36, Mirza Krak wrote:
> From: Mirza Krak <mirza.krak@gmail.com>
> 
> The NOR bus can be used to connect high-speed devices such as NOR flash,
> FPGAs, DSPs, CAN chips, Wi-Fi chips etc.
> 
> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
> ---
>  drivers/bus/Kconfig     |   7 +++
>  drivers/bus/Makefile    |   1 +
>  drivers/bus/tegra-nor.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 126 insertions(+)
>  create mode 100644 drivers/bus/tegra-nor.c

...

> +static int __init nor_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct clk *clk;
> +	void __iomem *base;
> +	int ret;
> +
> +	/* get the resource */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	/* get the clock */
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret)
> +		return ret;
> +
> +	/* parse the device node */
> +	ret = nor_parse_dt(pdev, base);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s fail to create devices.\n",
> +			pdev->dev.of_node->full_name);
> +		clk_disable_unprepare(clk);
> +		return ret;
> +	}
> +
> +	dev_info(&pdev->dev, "Driver registered.\n");
> +	return 0;
> +}

The reset is defined in the binding, but never used. Should we make sure
the reset is de-asserted in the probe?

Jon

-- 
nvpublic

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Mirza Krak]

2016-07-21 11:56 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>> +
>> +The NOR controller supports a number of memory types, including synchronous NOR,
>> +asynchronous NOR, and other flash memories with similar interfaces, such as
>> +MuxOneNAND. One could also connect high speed devices like FPGAs, DSPs,
>> +CAN chips, Wi-Fi chips etc.
>
> Nit-pick ... the Tegra documentation refers to this controller as the
> GMI (general memory interface) or SNOR (sync-NOR) controller because it
> is not just limited to NOR as you mentioned. I see references to GMI in
> the Tegra pinctrl driver and so may be we should use this name.

ACK.


>> +Required properties:
>> +
>> + - compatible: should be "nvidia,tegra20-nor", "nvidia,tegra30-nor"
>
> I see at least one difference at the register level between Tegra20 and
> Tegra30 and so I think this should be something like ...
>
>  - compatible : Should contain one of the following:
>         For Tegra20 must contain "nvidia,tegra20-gmi".
>         For Tegra30 must contain "nvidia,tegra30-gmi".

ACK. Just curious, which register was it? I only checked that they
have the same count of registers.

>> + - nvidia,config: This property represents the SNOR_CONFIG_0 register.
>
> There is also a SNOR_MIO_CONFIG for the MIO address space and so I think
> that this should be nvidia,snor-config to be explicit. It might be nice
> to also add a "nvidia,mio-config" while you are at it as well, however,
> that could always be done later. If you do, then the
> "nvidia,snor-config" becomes optional depending on whether you are using
> the SNOR or MIO address space.

ACK the nvidia,snor-config part, will though wait for further comments
regarding what to do with the config registers, break-out or keep it
is a one property / register.

Regarding mio-config, not sure about if I would like to include that
part in this stage. If you feel strongly about this we can do it. If
it only comes to down to replicate the same configurations that we do
for SNOR to MIO then I do not see much of a problem, but would like
SNOR to be accepted and would not like the MIO part to halt this. But
then again this up to you guys.

>
>> +
>> +Note that the NOR controller does not have any internal chip-select address
>> +decoding and if you want to access multiple devices external chip-select
>> +decoding must be provided.
>
> Although it is true, you do have the MIO address space and so you could
> support two devices via the SNOR address space and MIO address space
> (assuming that the MIO can be used for the 2nd device).

This is true. If we include MIO support above could be added to the bindings.

>
> Furthermore, if you do have external logic to support multiple devices
> this would assume that the devices use the same timing and so are
> probably the same type. It also assumes both can fit in the 256MB
> address range. May be worth mentioning.

ACK.

>
> The GMI does have 8 chip selects and I believe the purpose of these is
> to allow you to address more than the 256MB range. However, I believe to
> do this it require software intervention to change the current CS that
> is in use.

Yes that is true. One has to modify the SNOR_CONFIG register to choose
a different CS pin.

>
> I wonder if it is worth mentioning that the chip-select specified in the
> "nvidia,config" prop should match that in the "ranges" prop unless you
> have some external decoding logic to provide an external chip-select.
> Which raises a question, what does the chip-select in the ranges
> actually represent? I am not sure if there is a common practice here for
> device tree when boards have external logic to provide additional
> chip-selects. I am sure this is quite common.

I do not understand why CS pin setting in nvidia,config need to match
the "ranges" prop? Other then maybe cosmetics.

If we do not have any external decoding logic to create more
chip-selects we only have ONE chip-select, and that one should always
be indexed as 0? Regardless of which CS pin is used. Because
ultimately what we configure in SNOR_CONFIG is which PIN(function) to
use as chip-select. The address space remains the same.

>> +Example with two SJA1000 CAN controllers connected to the NOR bus:
>> +
>> +     nor at 70009000 {
>> +             status = "okay";
>> +             compatible = "nvidia,tegra20-nor", "nvidia,tegra30-nor";
>> +             reg = <0x70009000 0x1000>;
>> +             #address-cells = <2>;
>> +             #size-cells = <1>;
>> +             clocks = <&tegra_car TEGRA30_CLK_NOR>;
>> +             resets = < &tegra_car 42>;
>> +             reset-names = "nor";
>> +             ranges = <
>> +                     0 0 0x48000000 0x00000100
>> +                     1 0 0x48040000 0x00000100
>> +             >;
>
> The "nvidia,config" appears to be missing here.

Indeed it is. ACK.

Thank you Jonanthan for your feedback.

Best Regards,
Mirza

[RFC 6/6] bus: Add support for Tegra NOR controller

[Mirza Krak]

2016-07-21 12:15 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>
> On 19/07/16 14:36, Mirza Krak wrote:
>> From: Mirza Krak <mirza.krak@gmail.com>
>> +config TEGRA_NOR
>> +     tristate "Nvidia Tegra NOR flash bus driver a.k.a GMI/SNOR"
>> +             depends on ARCH_TEGRA_2x_SOC || ARCH_TEGRA_3x_SOC
>> +             depends on OF
>> +             help
>> +                     Driver for NOR flash bus found on Tegra30/20 SOC`s.
>
> It is actually present on all Tegra's and so I would drop the 30/20.

ACK.

>> +++ b/drivers/bus/tegra-nor.c
>> @@ -0,0 +1,118 @@
>> +/*
>> + * Driver for Nvidia NOR Flash bus a.k.a SNOR/GMI.
>> + *
>> + * Copyright (C) 2016 Host Mobility AB. All rights reserved.
>> + *
>> + * This file is licensed under the terms of the GNU General Public
>> + * License version 2. This program is licensed "as is" without any
>> + * warranty of any kind, whether express or implied.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +#include <linux/mfd/syscon.h>
>
> Is this needed?

It is not. ACK.

>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/regmap.h>
>
> Or this?

It is not. ACK.

>
>> +
>> +#define TEGRA_NOR_TIMING_REG_COUNT   2
>> +
>> +#define TEGRA_NOR_CONFIG                     0x00
>> +#define TEGRA_NOR_STATUS                     0x04
>> +#define TEGRA_NOR_ADDR_PTR                   0x08
>> +#define TEGRA_NOR_AHB_ADDR_PTR               0x0c
>> +#define TEGRA_NOR_TIMING0                    0x10
>> +#define TEGRA_NOR_TIMING1                    0x14
>> +#define TEGRA_NOR_MIO_CONFIG         0x18
>> +#define TEGRA_NOR_MIO_TIMING         0x1c
>> +#define TEGRA_NOR_DMA_CONFIG         0x20
>> +#define TEGRA_NOR_CS_MUX_CONFIG              0x24
>
> Not all of these are used. It is good to define them and I wonder if we
> should add support for MIO while are at it :-)

We can come back to this once I have all SNOR stuff in order.

>
>> +
>> +#define TEGRA_NOR_CONFIG_GO                          BIT(31)
>> +
>> +static const struct of_device_id nor_id_table[] = {
>> +     /* Tegra30 */
>
> I don't think this comment is needed.

ACK.

>
>> +     { .compatible = "nvidia,tegra30-nor", .data = NULL, },
>
> You don't need to set data to NULL.

ACK.

>
>> +     /* Tegra20 */
>> +     { .compatible = "nvidia,tegra20-nor", .data = NULL, },
>
> Same here.

ACK

>> +static int __init nor_probe(struct platform_device *pdev)
>> +{
>
> I would drop the __init.

ACK.

>> +static struct platform_driver nor_driver = {
>> +     .driver = {
>> +             .name           = "tegra-nor",
>> +             .of_match_table = nor_id_table,
>> +     },
>> +};
>
> The driver should have a remove function given that we can build as a
> module.

At the moment I do not know what we would do in a remove function and
hence me not adding one.

>
>> +module_platform_driver_probe(nor_driver, nor_probe);
>
> I would use "tegra_nor" namespace for all the structs, functions, etc.
> However, we may prefer to go with GMI and in which case tegra_gmi_probe,
> etc.

ACK. Who gets the last call on what we should call the driver? It
seems that we both think GMI is a better name, do we need a third? :).

Thank you Jonathan for your feedback.

Best Regards,
Mirza

[RFC 6/6] bus: Add support for Tegra NOR controller

[Mirza Krak]

2016-07-21 17:12 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>
> The reset is defined in the binding, but never used. Should we make sure
> the reset is de-asserted in the probe?

Yes, will do that.

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Jon Hunter]

On 21/07/16 21:10, Mirza Krak wrote:
> 2016-07-21 11:56 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>>> +
>>> +The NOR controller supports a number of memory types, including synchronous NOR,
>>> +asynchronous NOR, and other flash memories with similar interfaces, such as
>>> +MuxOneNAND. One could also connect high speed devices like FPGAs, DSPs,
>>> +CAN chips, Wi-Fi chips etc.
>>
>> Nit-pick ... the Tegra documentation refers to this controller as the
>> GMI (general memory interface) or SNOR (sync-NOR) controller because it
>> is not just limited to NOR as you mentioned. I see references to GMI in
>> the Tegra pinctrl driver and so may be we should use this name.
> 
> ACK.
> 
> 
>>> +Required properties:
>>> +
>>> + - compatible: should be "nvidia,tegra20-nor", "nvidia,tegra30-nor"
>>
>> I see at least one difference at the register level between Tegra20 and
>> Tegra30 and so I think this should be something like ...
>>
>>  - compatible : Should contain one of the following:
>>         For Tegra20 must contain "nvidia,tegra20-gmi".
>>         For Tegra30 must contain "nvidia,tegra30-gmi".
> 
> ACK. Just curious, which register was it? I only checked that they
> have the same count of registers.

Register counts are the same, but the SNOR_CONFIG_0 is not identical. I
believe only Tegra30 defines bit 14.

>>> + - nvidia,config: This property represents the SNOR_CONFIG_0 register.
>>
>> There is also a SNOR_MIO_CONFIG for the MIO address space and so I think
>> that this should be nvidia,snor-config to be explicit. It might be nice
>> to also add a "nvidia,mio-config" while you are at it as well, however,
>> that could always be done later. If you do, then the
>> "nvidia,snor-config" becomes optional depending on whether you are using
>> the SNOR or MIO address space.
> 
> ACK the nvidia,snor-config part, will though wait for further comments
> regarding what to do with the config registers, break-out or keep it
> is a one property / register.
> 
> Regarding mio-config, not sure about if I would like to include that
> part in this stage. If you feel strongly about this we can do it. If
> it only comes to down to replicate the same configurations that we do
> for SNOR to MIO then I do not see much of a problem, but would like
> SNOR to be accepted and would not like the MIO part to halt this. But
> then again this up to you guys.
> 
>>
>>> +
>>> +Note that the NOR controller does not have any internal chip-select address
>>> +decoding and if you want to access multiple devices external chip-select
>>> +decoding must be provided.
>>
>> Although it is true, you do have the MIO address space and so you could
>> support two devices via the SNOR address space and MIO address space
>> (assuming that the MIO can be used for the 2nd device).
> 
> This is true. If we include MIO support above could be added to the bindings.
> 
>>
>> Furthermore, if you do have external logic to support multiple devices
>> this would assume that the devices use the same timing and so are
>> probably the same type. It also assumes both can fit in the 256MB
>> address range. May be worth mentioning.
> 
> ACK.
> 
>>
>> The GMI does have 8 chip selects and I believe the purpose of these is
>> to allow you to address more than the 256MB range. However, I believe to
>> do this it require software intervention to change the current CS that
>> is in use.
> 
> Yes that is true. One has to modify the SNOR_CONFIG register to choose
> a different CS pin.
> 
>>
>> I wonder if it is worth mentioning that the chip-select specified in the
>> "nvidia,config" prop should match that in the "ranges" prop unless you
>> have some external decoding logic to provide an external chip-select.
>> Which raises a question, what does the chip-select in the ranges
>> actually represent? I am not sure if there is a common practice here for
>> device tree when boards have external logic to provide additional
>> chip-selects. I am sure this is quite common.
> 
> I do not understand why CS pin setting in nvidia,config need to match
> the "ranges" prop? Other then maybe cosmetics.

Yes it would be cosmetic. That said, I even wonder if CS needs to be
exposed at all given that they all map to the same CPU address space.
Couldn't your binding for the CAN devices be as follows?

nor at 70009000 {
	...

	can at 48000000 {
		...
	};

	can at 48040000 {
		...
	};
};

Problem is if you did have devices on different chip-selects then how
would these be handled? They could not point to the same physical
address. I am not sure if there is a way to do that in DT?

Cheers
Jon

-- 
nvpublic

[RFC 6/6] bus: Add support for Tegra NOR controller

[Jon Hunter]

On 21/07/16 21:42, Mirza Krak wrote:
> 2016-07-21 12:15 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>>
>> On 19/07/16 14:36, Mirza Krak wrote:
>>> From: Mirza Krak <mirza.krak@gmail.com>
>>> +config TEGRA_NOR
>>> +     tristate "Nvidia Tegra NOR flash bus driver a.k.a GMI/SNOR"
>>> +             depends on ARCH_TEGRA_2x_SOC || ARCH_TEGRA_3x_SOC
>>> +             depends on OF
>>> +             help
>>> +                     Driver for NOR flash bus found on Tegra30/20 SOC`s.
>>
>> It is actually present on all Tegra's and so I would drop the 30/20.
> 
> ACK.
> 
>>> +++ b/drivers/bus/tegra-nor.c
>>> @@ -0,0 +1,118 @@
>>> +/*
>>> + * Driver for Nvidia NOR Flash bus a.k.a SNOR/GMI.
>>> + *
>>> + * Copyright (C) 2016 Host Mobility AB. All rights reserved.
>>> + *
>>> + * This file is licensed under the terms of the GNU General Public
>>> + * License version 2. This program is licensed "as is" without any
>>> + * warranty of any kind, whether express or implied.
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/io.h>
>>> +#include <linux/mfd/syscon.h>
>>
>> Is this needed?
> 
> It is not. ACK.
> 
>>
>>> +#include <linux/module.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/regmap.h>
>>
>> Or this?
> 
> It is not. ACK.
> 
>>
>>> +
>>> +#define TEGRA_NOR_TIMING_REG_COUNT   2
>>> +
>>> +#define TEGRA_NOR_CONFIG                     0x00
>>> +#define TEGRA_NOR_STATUS                     0x04
>>> +#define TEGRA_NOR_ADDR_PTR                   0x08
>>> +#define TEGRA_NOR_AHB_ADDR_PTR               0x0c
>>> +#define TEGRA_NOR_TIMING0                    0x10
>>> +#define TEGRA_NOR_TIMING1                    0x14
>>> +#define TEGRA_NOR_MIO_CONFIG         0x18
>>> +#define TEGRA_NOR_MIO_TIMING         0x1c
>>> +#define TEGRA_NOR_DMA_CONFIG         0x20
>>> +#define TEGRA_NOR_CS_MUX_CONFIG              0x24
>>
>> Not all of these are used. It is good to define them and I wonder if we
>> should add support for MIO while are at it :-)
> 
> We can come back to this once I have all SNOR stuff in order.
> 
>>
>>> +
>>> +#define TEGRA_NOR_CONFIG_GO                          BIT(31)
>>> +
>>> +static const struct of_device_id nor_id_table[] = {
>>> +     /* Tegra30 */
>>
>> I don't think this comment is needed.
> 
> ACK.
> 
>>
>>> +     { .compatible = "nvidia,tegra30-nor", .data = NULL, },
>>
>> You don't need to set data to NULL.
> 
> ACK.
> 
>>
>>> +     /* Tegra20 */
>>> +     { .compatible = "nvidia,tegra20-nor", .data = NULL, },
>>
>> Same here.
> 
> ACK
> 
>>> +static int __init nor_probe(struct platform_device *pdev)
>>> +{
>>
>> I would drop the __init.
> 
> ACK.
> 
>>> +static struct platform_driver nor_driver = {
>>> +     .driver = {
>>> +             .name           = "tegra-nor",
>>> +             .of_match_table = nor_id_table,
>>> +     },
>>> +};
>>
>> The driver should have a remove function given that we can build as a
>> module.
> 
> At the moment I do not know what we would do in a remove function and
> hence me not adding one.

Should just be the inverse of the probe (although there is no inverse
for the parsing DT bit). If you don't wish to add a remove, that is
fine, but make the driver a 'bool' and not 'tristate' in the Kconfig so
it cannot be configured as a module.

>>
>>> +module_platform_driver_probe(nor_driver, nor_probe);
>>
>> I would use "tegra_nor" namespace for all the structs, functions, etc.
>> However, we may prefer to go with GMI and in which case tegra_gmi_probe,
>> etc.
> 
> ACK. Who gets the last call on what we should call the driver? It
> seems that we both think GMI is a better name, do we need a third? :).

The patches would have to go via Thierry and so ultimately, Thierry.
However, I can't imagine he would object to GMI ;-)

Jon

-- 
nvpublic

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Mirza Krak]

2016-07-22 11:32 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>
> On 21/07/16 21:10, Mirza Krak wrote:
>> 2016-07-21 11:56 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>>>
>>> I wonder if it is worth mentioning that the chip-select specified in the
>>> "nvidia,config" prop should match that in the "ranges" prop unless you
>>> have some external decoding logic to provide an external chip-select.
>>> Which raises a question, what does the chip-select in the ranges
>>> actually represent? I am not sure if there is a common practice here for
>>> device tree when boards have external logic to provide additional
>>> chip-selects. I am sure this is quite common.
>>
>> I do not understand why CS pin setting in nvidia,config need to match
>> the "ranges" prop? Other then maybe cosmetics.
>
> Yes it would be cosmetic. That said, I even wonder if CS needs to be
> exposed at all given that they all map to the same CPU address space.
> Couldn't your binding for the CAN devices be as follows?
>
> nor at 70009000 {
>         ...
>
>         can at 48000000 {
>                 ...
>         };
>
>         can at 48040000 {
>                 ...
>         };
> };

This has also crossed my mind, maybe just get rid of the "ranges" prop
and do like you have above. But then again I do not know what is
preferred so I went with "ranges" prop initially.


>
> Problem is if you did have devices on different chip-selects then how
> would these be handled? They could not point to the same physical
> address. I am not sure if there is a way to do that in DT?

Having trouble following your though here. We do not have "different"
chip-selects?

Best Regards
Mirza

[RFC 6/6] bus: Add support for Tegra NOR controller

[Mirza Krak]

2016-07-22 11:38 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>>> The driver should have a remove function given that we can build as a
>>> module.
>>
>> At the moment I do not know what we would do in a remove function and
>> hence me not adding one.
>
> Should just be the inverse of the probe (although there is no inverse
> for the parsing DT bit). If you don't wish to add a remove, that is
> fine, but make the driver a 'bool' and not 'tristate' in the Kconfig so
> it cannot be configured as a module.

I understand the concept of a remove function, but I use devm_ calls
for all resources. These should be handled by the device core on a
driver detach?

One thing came to mind now that could be done in a remove method and
that is clearing the CONFIG_GO bit, or I could just do that first on
probe instead to make sure the controller is stopped.

Ok, one more thing came to mind, and that is depopulating the child
devices. Got it remove function it is then.

>>>> +module_platform_driver_probe(nor_driver, nor_probe);
>>>
>>> I would use "tegra_nor" namespace for all the structs, functions, etc.
>>> However, we may prefer to go with GMI and in which case tegra_gmi_probe,
>>> etc.
>>
>> ACK. Who gets the last call on what we should call the driver? It
>> seems that we both think GMI is a better name, do we need a third? :).
>
> The patches would have to go via Thierry and so ultimately, Thierry.
> However, I can't imagine he would object to GMI ;-)

Eagerly awaiting Thierry`s comments :).

Best Regards,
Mirza

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Jon Hunter]

On 22/07/16 20:07, Mirza Krak wrote:
> 2016-07-22 11:32 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>>
>> On 21/07/16 21:10, Mirza Krak wrote:
>>> 2016-07-21 11:56 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>>>>
>>>> I wonder if it is worth mentioning that the chip-select specified in the
>>>> "nvidia,config" prop should match that in the "ranges" prop unless you
>>>> have some external decoding logic to provide an external chip-select.
>>>> Which raises a question, what does the chip-select in the ranges
>>>> actually represent? I am not sure if there is a common practice here for
>>>> device tree when boards have external logic to provide additional
>>>> chip-selects. I am sure this is quite common.
>>>
>>> I do not understand why CS pin setting in nvidia,config need to match
>>> the "ranges" prop? Other then maybe cosmetics.
>>
>> Yes it would be cosmetic. That said, I even wonder if CS needs to be
>> exposed at all given that they all map to the same CPU address space.
>> Couldn't your binding for the CAN devices be as follows?
>>
>> nor at 70009000 {
>>         ...
>>
>>         can at 48000000 {
>>                 ...
>>         };
>>
>>         can at 48040000 {
>>                 ...
>>         };
>> };
> 
> This has also crossed my mind, maybe just get rid of the "ranges" prop
> and do like you have above. But then again I do not know what is
> preferred so I went with "ranges" prop initially.
> 
> 
>>
>> Problem is if you did have devices on different chip-selects then how
>> would these be handled? They could not point to the same physical
>> address. I am not sure if there is a way to do that in DT?
> 
> Having trouble following your though here. We do not have "different"
> chip-selects?

I meant that the device has multiple chip-selects and so I was not sure
the best way to handle this for the GMI.

Jon

-- 
nvpublic

[RFC 6/6] bus: Add support for Tegra NOR controller

[Jon Hunter]

On 22/07/16 20:18, Mirza Krak wrote:
> 2016-07-22 11:38 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>>>> The driver should have a remove function given that we can build as a
>>>> module.
>>>
>>> At the moment I do not know what we would do in a remove function and
>>> hence me not adding one.
>>
>> Should just be the inverse of the probe (although there is no inverse
>> for the parsing DT bit). If you don't wish to add a remove, that is
>> fine, but make the driver a 'bool' and not 'tristate' in the Kconfig so
>> it cannot be configured as a module.
> 
> I understand the concept of a remove function, but I use devm_ calls
> for all resources. These should be handled by the device core on a
> driver detach?

There is one clock that needs to be disabled in the remove.

Cheers
Jon

-- 
nvpublic

[RFC 6/6] bus: Add support for Tegra NOR controller

[Thierry Reding]

On Fri, Jul 22, 2016 at 09:18:37PM +0200, Mirza Krak wrote:
> 2016-07-22 11:38 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
> >>> The driver should have a remove function given that we can build as a
> >>> module.
> >>
> >> At the moment I do not know what we would do in a remove function and
> >> hence me not adding one.
> >
> > Should just be the inverse of the probe (although there is no inverse
> > for the parsing DT bit). If you don't wish to add a remove, that is
> > fine, but make the driver a 'bool' and not 'tristate' in the Kconfig so
> > it cannot be configured as a module.
> 
> I understand the concept of a remove function, but I use devm_ calls
> for all resources. These should be handled by the device core on a
> driver detach?
> 
> One thing came to mind now that could be done in a remove method and
> that is clearing the CONFIG_GO bit, or I could just do that first on
> probe instead to make sure the controller is stopped.
> 
> Ok, one more thing came to mind, and that is depopulating the child
> devices. Got it remove function it is then.
> 
> >>>> +module_platform_driver_probe(nor_driver, nor_probe);
> >>>
> >>> I would use "tegra_nor" namespace for all the structs, functions, etc.
> >>> However, we may prefer to go with GMI and in which case tegra_gmi_probe,
> >>> etc.
> >>
> >> ACK. Who gets the last call on what we should call the driver? It
> >> seems that we both think GMI is a better name, do we need a third? :).
> >
> > The patches would have to go via Thierry and so ultimately, Thierry.
> > However, I can't imagine he would object to GMI ;-)
> 
> Eagerly awaiting Thierry`s comments :).

Let's go with GMI. The TRM has a mix of GMI vs. SNOR, but as Jon points
out the pinmux doesn't mention SNOR, so in order to hopefully reduce the
confusion, let's stick with GMI.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160725/d1437c1a/attachment.sig>

[RFC 6/6] bus: Add support for Tegra NOR controller

[Thierry Reding]

On Tue, Jul 19, 2016 at 03:36:37PM +0200, Mirza Krak wrote:
> From: Mirza Krak <mirza.krak@gmail.com>
> 
> The NOR bus can be used to connect high-speed devices such as NOR flash,
> FPGAs, DSPs, CAN chips, Wi-Fi chips etc.
> 
> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
> ---
>  drivers/bus/Kconfig     |   7 +++
>  drivers/bus/Makefile    |   1 +
>  drivers/bus/tegra-nor.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 126 insertions(+)
>  create mode 100644 drivers/bus/tegra-nor.c
> 
> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
> index 3b205e2..b74be7d8 100644
> --- a/drivers/bus/Kconfig
> +++ b/drivers/bus/Kconfig
> @@ -145,6 +145,13 @@ config TEGRA_ACONNECT
>  	  Driver for the Tegra ACONNECT bus which is used to interface with
>  	  the devices inside the Audio Processing Engine (APE) for Tegra210.
> 
> +config TEGRA_NOR
> +	tristate "Nvidia Tegra NOR flash bus driver a.k.a GMI/SNOR"
> +		depends on ARCH_TEGRA_2x_SOC || ARCH_TEGRA_3x_SOC

I think an ARCH_TEGRA dependency is enough here.

> +		depends on OF

You can drop this because Tegra has been OF-only for a long time now.

> +		help
> +			Driver for NOR flash bus found on Tegra30/20 SOC`s.

Maybe make this "Driver for GMI controller found on NVIDIA Tegra SoCs."
or similar in light of the name change.

>  config UNIPHIER_SYSTEM_BUS
>  	tristate "UniPhier System Bus driver"
>  	depends on ARCH_UNIPHIER && OF
> diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
> index ac84cc4..46d0129 100644
> --- a/drivers/bus/Makefile
> +++ b/drivers/bus/Makefile
> @@ -18,5 +18,6 @@ obj-$(CONFIG_OMAP_OCP2SCP)	+= omap-ocp2scp.o
>  obj-$(CONFIG_SUNXI_RSB)		+= sunxi-rsb.o
>  obj-$(CONFIG_SIMPLE_PM_BUS)	+= simple-pm-bus.o
>  obj-$(CONFIG_TEGRA_ACONNECT)	+= tegra-aconnect.o
> +obj-$(CONFIG_TEGRA_NOR)		+= tegra-nor.o
>  obj-$(CONFIG_UNIPHIER_SYSTEM_BUS)	+= uniphier-system-bus.o
>  obj-$(CONFIG_VEXPRESS_CONFIG)	+= vexpress-config.o
> diff --git a/drivers/bus/tegra-nor.c b/drivers/bus/tegra-nor.c
> new file mode 100644
> index 0000000..1e48113
> --- /dev/null
> +++ b/drivers/bus/tegra-nor.c
> @@ -0,0 +1,118 @@
> +/*
> + * Driver for Nvidia NOR Flash bus a.k.a SNOR/GMI.

Nit: I encourage the use of "NVIDIA" as spelling for consistency.

> + *
> + * Copyright (C) 2016 Host Mobility AB. All rights reserved.
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +
> +#define TEGRA_NOR_TIMING_REG_COUNT	2
> +
> +#define TEGRA_NOR_CONFIG			0x00
> +#define TEGRA_NOR_STATUS			0x04
> +#define TEGRA_NOR_ADDR_PTR			0x08
> +#define TEGRA_NOR_AHB_ADDR_PTR		0x0c
> +#define TEGRA_NOR_TIMING0			0x10
> +#define TEGRA_NOR_TIMING1			0x14
> +#define TEGRA_NOR_MIO_CONFIG		0x18
> +#define TEGRA_NOR_MIO_TIMING		0x1c
> +#define TEGRA_NOR_DMA_CONFIG		0x20
> +#define TEGRA_NOR_CS_MUX_CONFIG		0x24
> +
> +#define TEGRA_NOR_CONFIG_GO				BIT(31)
> +
> +static const struct of_device_id nor_id_table[] = {
> +	/* Tegra30 */
> +	{ .compatible = "nvidia,tegra30-nor", .data = NULL, },
> +	/* Tegra20 */
> +	{ .compatible = "nvidia,tegra20-nor", .data = NULL, },
> +
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, nor_id_table);
> +
> +
> +static int __init nor_parse_dt(struct platform_device *pdev,
> +				void __iomem *base)
> +{
> +	struct device_node *of_node = pdev->dev.of_node;
> +	u32 config, timing[TEGRA_NOR_TIMING_REG_COUNT];
> +	int ret;
> +
> +	ret = of_property_read_u32_array(of_node, "nvidia,cs-timing",
> +					 timing, TEGRA_NOR_TIMING_REG_COUNT);
> +	if (!ret) {
> +		writel(timing[0], base + TEGRA_NOR_TIMING0);
> +		writel(timing[1], base + TEGRA_NOR_TIMING1);
> +	}
> +
> +	ret = of_property_read_u32(of_node, "nvidia,config", &config);
> +	if (ret)
> +		return ret;
> +
> +	config |= TEGRA_NOR_CONFIG_GO;
> +	writel(config, base + TEGRA_NOR_CONFIG);

My preference would be for the tegra_gmi_parse_dt() function not to do
any register programming. Instead I think it would be better to store
any of the parameters inside a struct tegra_gmi and do the programming
from within tegra_gmi_probe() (or via an other function such as
tegra_gmi_initialize(), called from tegra_gmi_probe()).

The reason is that you'll most likely want to do this programming when
you resume from suspend, and you could reuse tegra_gmi_initialize() to
do that.

> +
> +	if (of_get_child_count(of_node))
> +		ret = of_platform_populate(of_node,
> +				   of_default_bus_match_table,
> +				   NULL, &pdev->dev);

Why the extra check? of_platform_populate() is almost a no-op if there
aren't any children anyway. What it will do is set the OF_POPULATED_BUS
flag, but I think we want that irrespective of whether or not there are
any children.

Was there any problem with calling it unconditionally that made you opt
for the extra check?

Also, I think you can call of_platform_default_populate(), which is a
little shorter than the above because you can omit the match table.

> +
> +	return ret;
> +}
> +
> +static int __init nor_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct clk *clk;
> +	void __iomem *base;
> +	int ret;
> +
> +	/* get the resource */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	/* get the clock */
> +	clk = devm_clk_get(&pdev->dev, NULL);

Let's use a consumer name here. The problem with a NULL consumer name is
that it effectively restricts the DT binding. It means that whatever the
clock is its name needs to be the first in a clock-names property. We'll
likely get away with this because there's only one clock, but should we
ever need to add another we'd need to add wording to the device tree
bindings that the "gmi" entry needs to be first.

I'm not sure if that's clear, so I'll try to explain in other words: If
you pass a NULL consumer name to clk_get() it will simply use the first
clock in the clocks property. If you want to later extend the DT binding
by adding a clock in a backwards-compatible way, you'll need to add the
restriction that the "gmi" clock (the one that was previously unnamed)
must be the first in the clock-names and clocks properties.

That's all a little confusing, so let's avoid this by giving it a proper
consumer name to begin with.

> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret)
> +		return ret;
> +
> +	/* parse the device node */
> +	ret = nor_parse_dt(pdev, base);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s fail to create devices.\n",
> +			pdev->dev.of_node->full_name);
> +		clk_disable_unprepare(clk);
> +		return ret;
> +	}

The good thing if you don't do any programming in tegra_gmi_parse_dt(),
is that you can easily move the clk_prepare_enable() call to here where
things can't fail anymore, so you don't have to add cleanup code.

> +
> +	dev_info(&pdev->dev, "Driver registered.\n");

Please avoid this kind of output. Users expect that everything will work
so you want to let them know when something goes wrong, and be quiet
when all goes as expected.

> +	return 0;
> +}
> +
> +static struct platform_driver nor_driver = {
> +	.driver = {
> +		.name		= "tegra-nor",
> +		.of_match_table	= nor_id_table,
> +	},
> +};
> +module_platform_driver_probe(nor_driver, nor_probe);
> +
> +MODULE_AUTHOR("Mirza Krak <mirza.krak@gmail.com");
> +MODULE_DESCRIPTION("NVIDIA Tegra NOR Bus Driver");
> +MODULE_LICENSE("GPL");

You're header comment says GPL version 2, which means that the
MODULE_LICENSE needs to be "GPL v2". "GPL" means "version 2 or later".

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160725/add9c977/attachment-0001.sig>

[RFC 1/6] clk: tegra: add TEGRA20_CLK_NOR to init table

[Thierry Reding]

On Tue, Jul 19, 2016 at 03:36:32PM +0200, Mirza Krak wrote:
> From: Mirza Krak <mirza.krak@gmail.com>
> 
> Add TEGRA20_CLK_NOR to init tabel and set a "sane" default rate.
> 
> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
> ---
>  drivers/clk/tegra/clk-tegra20.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index 837e5cb..aefc044 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -1047,6 +1047,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
>  	{ TEGRA20_CLK_SDMMC3, TEGRA20_CLK_PLL_P, 48000000, 0 },
>  	{ TEGRA20_CLK_SDMMC4, TEGRA20_CLK_PLL_P, 48000000, 0 },
>  	{ TEGRA20_CLK_SPI, TEGRA20_CLK_PLL_P, 20000000, 0 },
> +	{ TEGRA20_CLK_NOR, TEGRA20_CLK_PLL_P, 86500000, 0 },

Yay for inconsistent naming in the hardware. It would've been nice if
this clock was called GMI. Oh well...

Could you perhaps explain in the commit message why 86.5 MHz is a sane
default? I'm totally unfamiliar with this controller, so maybe it's
self-explanatory, but it seems a rather odd value for a clock frequency.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160725/8f6048d2/attachment.sig>

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Thierry Reding]

On Tue, Jul 19, 2016 at 03:36:34PM +0200, Mirza Krak wrote:
> From: Mirza Krak <mirza.krak@gmail.com>
> 
> Document the devicetree bindings for NOR bus driver found on Tegra20 and
> Tegra30 SOCs
> 
> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
> ---
>  .../devicetree/bindings/bus/nvidia,tegra20-nor.txt | 73 ++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> 
> diff --git a/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> new file mode 100644
> index 0000000..9ee4a66
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> @@ -0,0 +1,73 @@
> +Device tree bindings for NVIDIA Tegra20/30 NOR Bus
> +
> +The NOR controller supports a number of memory types, including synchronous NOR,
> +asynchronous NOR, and other flash memories with similar interfaces, such as
> +MuxOneNAND. One could also connect high speed devices like FPGAs, DSPs,
> +CAN chips, Wi-Fi chips etc.
> +
> +The actual devices are instantiated from the child nodes of a NOR node.
> +
> +Required properties:
> +
> + - compatible: should be "nvidia,tegra20-nor", "nvidia,tegra30-nor"
> + - reg: Should contain NOR controller registers location and length.
> + - clocks: Must contain one entry, for the module clock.
> +   See ../clocks/clock-bindings.txt for details.
> + - resets : Must contain an entry for each entry in reset-names.
> +   See ../reset/reset.txt for details.
> + - reset-names : Must include the following entries:
> +  - nor
> + - #address-cells: Must be set to 2 to allow memory address translation
> + - #size-cells:	Must be set to 1 to allow CS address passing
> + - ranges: Must be set up to reflect the memory layout with four integer
> + 		values for each chip-select line in use.
> + - nvidia,config: This property represents the SNOR_CONFIG_0 register.

I'd prefer if this was split out into separate properties. It's somewhat
friendlier in my opinion to let people know what each of the settings is
along with any units and such, rather than point them at the TRM, which
they may or may not have access to.

Or which not be available anymore.

> +
> +Note that the NOR controller does not have any internal chip-select address
> +decoding and if you want to access multiple devices external chip-select
> +decoding must be provided.
> +
> +Optional properties:
> +
> + - nvidia,cs-timing: The timing array represents the SNOR_TIMING0_0 and
> +   SNOR_TIMING1_0 registers for the NOR controller. If unset reset-values will
> +   be used. See reference documentation for detailed description of the timing
> +   registers.

Are the reset values sensible? Are they reasonable defaults for a class
of use-cases? I'm thinking that if they aren't we might as well make it
a required property.

Also, I'd prefer if this was split out into individual fields for the
same reasons as the SNOR_CONFIG_0 register property.

> +
> +Example with two SJA1000 CAN controllers connected to the NOR bus:
> +
> +	nor at 70009000 {
> +		status = "okay";
> +		compatible = "nvidia,tegra20-nor", "nvidia,tegra30-nor";
> +		reg = <0x70009000 0x1000>;
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		clocks = <&tegra_car TEGRA30_CLK_NOR>;
> +		resets = < &tegra_car 42>;

No space between < and &, please.

> +		reset-names = "nor";
> +		ranges = <
> +			0 0 0x48000000 0x00000100
> +			1 0 0x48040000 0x00000100
> +		>;
> +
> +		can at 0,0 {
> +			compatible = "nxp,sja1000";
> +			reg = <0 0 0x100>;
> +			interrupt-parent = <&gpio>;
> +			interrupts = <TEGRA_GPIO(B, 5) IRQ_TYPE_EDGE_RISING>;
> +			nxp,external-clock-frequency = <24000000>;
> +			nxp,tx-output-config = <0x16>;
> +			nxp,clock-out-frequency = <24000000>;
> +			reg-io-width = <2>;
> +		};
> +
> +

There's an extra blank line here.

> +		can at 1,0 {
> +			compatible = "nxp,sja1000";
> +			reg = <1 0 0x100>;
> +			interrupt-parent = <&gpio>;
> +			interrupts = <TEGRA_GPIO(A, 6) IRQ_TYPE_EDGE_RISING>;
> +			nxp,external-clock-frequency = <24000000>;
> +			nxp,tx-output-config = <0x16>;
> +			reg-io-width = <2>;
> +	};

I'm somewhat confused about how this hardware works. My understanding
was that each chip gets mapped to the whole range of the NOR address
range (0x48000000 - 0x4fffffff on Tegra30).

The above suggests that one of the CAN controllers gets mapped to an
address 0x48000000 and the other gets mapped to 0x48040000. But why do
we even need a chip-select at all in that case? If both chips don't use
any overlapping memory region, what good does the chip-select do?

Also, it seems to me that you'd have to program the SNOR_CONFIG_0
register in order to select a specific chip, but I don't see anything in
the driver access that register after the initial write of the register.

I would've expected this to require some sort of infrastructure to allow
devices connected to the GMI controller to acquire the bus via some API
to select their chip.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160725/37527efd/attachment.sig>

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Thierry Reding]

On Thu, Jul 21, 2016 at 11:26:09AM +0100, Jon Hunter wrote:
> 
> On 20/07/16 20:28, Mirza Krak wrote:
> > 2016-07-20 14:44 GMT+02:00 Rob Herring <robh@kernel.org>:
> >> On Tue, Jul 19, 2016 at 03:36:34PM +0200, Mirza Krak wrote:
> >>> From: Mirza Krak <mirza.krak@gmail.com>
> >>>
> >>> Document the devicetree bindings for NOR bus driver found on Tegra20 and
> >>> Tegra30 SOCs
> >>>
> >>> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
> >>> ---
> >>>  .../devicetree/bindings/bus/nvidia,tegra20-nor.txt | 73 ++++++++++++++++++++++
> >>>  1 file changed, 73 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> >>> new file mode 100644
> >>> index 0000000..9ee4a66
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> >>> @@ -0,0 +1,73 @@
> >>> +Device tree bindings for NVIDIA Tegra20/30 NOR Bus
> >>> +
> >>> +The NOR controller supports a number of memory types, including synchronous NOR,
> >>> +asynchronous NOR, and other flash memories with similar interfaces, such as
> >>> +MuxOneNAND. One could also connect high speed devices like FPGAs, DSPs,
> >>> +CAN chips, Wi-Fi chips etc.
> >>> +
> >>> +The actual devices are instantiated from the child nodes of a NOR node.
> >>> +
> >>> +Required properties:
> >>> +
> >>> + - compatible: should be "nvidia,tegra20-nor", "nvidia,tegra30-nor"
> >>> + - reg: Should contain NOR controller registers location and length.
> >>> + - clocks: Must contain one entry, for the module clock.
> >>> +   See ../clocks/clock-bindings.txt for details.
> >>> + - resets : Must contain an entry for each entry in reset-names.
> >>> +   See ../reset/reset.txt for details.
> >>> + - reset-names : Must include the following entries:
> >>> +  - nor
> >>> + - #address-cells: Must be set to 2 to allow memory address translation
> >>> + - #size-cells:      Must be set to 1 to allow CS address passing
> >>> + - ranges: Must be set up to reflect the memory layout with four integer
> >>> +             values for each chip-select line in use.
> >>> + - nvidia,config: This property represents the SNOR_CONFIG_0 register.
> >>> +
> >>> +Note that the NOR controller does not have any internal chip-select address
> >>> +decoding and if you want to access multiple devices external chip-select
> >>> +decoding must be provided.
> >>
> >> Then what are the 2 chip selects in ranges?
> >>
> >> Rob
> > 
> > Those two chip selects are actually a representation of a external
> > decoding logic based on what we use on our board. Even though it the
> > NOR controller only supports one single chip select I wanted to give
> > an example on how one could create more chip-selects with an external
> > logic and what it would look like in the device tree representation.
> 
> Technically, the GMI/SNOR controller supports 8 chip-selects, however,
> unlike some devices, it appears that software has to select the active
> chip-select. Although this sounds odd, I believe that the idea is that
> in order to support devices greater than 256MB (external address space
> for available NOR/async devices) you can use the chip-selects to page
> through memory greater than this 256MB range. At least that it my
> (limited) understanding!

Actually I had assumed that software would at some point need to select
the active chip to switch between multiple connected chips. I suppose it
could be possible to have multiple chips share the same chip-select and
decode the address lines to determine whether they're being accessed or
not.

What I don't understand, and it's further complicated by the fact that
external chip-selects are being used, is how does the controller get
told what chip to select? It seems to me like it would always access the
same chips because the SNOR_CONFIG_0 register is only ever written on
->probe().

For external chip selects, how do they tie in with all this? Who gets to
implement this logic? Wouldn't we need to abstract this away somehow so
that we can support whatever board designers will come up with?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160725/6c449a04/attachment.sig>

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Thierry Reding]

On Thu, Jul 21, 2016 at 10:56:44AM +0100, Jon Hunter wrote:
> 
> On 19/07/16 14:36, Mirza Krak wrote:
> > From: Mirza Krak <mirza.krak@gmail.com>
> > 
> > Document the devicetree bindings for NOR bus driver found on Tegra20 and
> > Tegra30 SOCs
> > 
> > Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
> > ---
> >  .../devicetree/bindings/bus/nvidia,tegra20-nor.txt | 73 ++++++++++++++++++++++
> >  1 file changed, 73 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> > new file mode 100644
> > index 0000000..9ee4a66
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> > @@ -0,0 +1,73 @@
> > +Device tree bindings for NVIDIA Tegra20/30 NOR Bus
> > +
> > +The NOR controller supports a number of memory types, including synchronous NOR,
> > +asynchronous NOR, and other flash memories with similar interfaces, such as
> > +MuxOneNAND. One could also connect high speed devices like FPGAs, DSPs,
> > +CAN chips, Wi-Fi chips etc.
> 
> Nit-pick ... the Tegra documentation refers to this controller as the
> GMI (general memory interface) or SNOR (sync-NOR) controller because it
> is not just limited to NOR as you mentioned. I see references to GMI in
> the Tegra pinctrl driver and so may be we should use this name.
> 
> > +
> > +The actual devices are instantiated from the child nodes of a NOR node.
> > +
> > +Required properties:
> > +
> > + - compatible: should be "nvidia,tegra20-nor", "nvidia,tegra30-nor"
> 
> I see at least one difference at the register level between Tegra20 and
> Tegra30 and so I think this should be something like ...
> 
>  - compatible : Should contain one of the following:
> 	For Tegra20 must contain "nvidia,tegra20-gmi".
>  	For Tegra30 must contain "nvidia,tegra30-gmi".
> 
> > + - reg: Should contain NOR controller registers location and length.
> > + - clocks: Must contain one entry, for the module clock.
> > +   See ../clocks/clock-bindings.txt for details.
> > + - resets : Must contain an entry for each entry in reset-names.
> > +   See ../reset/reset.txt for details.
> > + - reset-names : Must include the following entries:
> > +  - nor
> > + - #address-cells: Must be set to 2 to allow memory address translation
> > + - #size-cells:	Must be set to 1 to allow CS address passing
> > + - ranges: Must be set up to reflect the memory layout with four integer
> > + 		values for each chip-select line in use.
> > + - nvidia,config: This property represents the SNOR_CONFIG_0 register.
> 
> There is also a SNOR_MIO_CONFIG for the MIO address space and so I think
> that this should be nvidia,snor-config to be explicit. It might be nice
> to also add a "nvidia,mio-config" while you are at it as well, however,
> that could always be done later. If you do, then the
> "nvidia,snor-config" becomes optional depending on whether you are using
> the SNOR or MIO address space.
> 
> Thierry, Stephen, do prefer all the fields on the config registers are
> broken out? There are quite a few but I am not sure what we typically
> recommend here?

As I said elsewhere, I prefer breaking the fields out into separate
properties because that makes it a lot easier to write the DT. Rather
than having to go and manually assemble 32-bit values for this register
and the timing registers, it must be a lot easier to look at datasheets
and copy the values into the corresponding DT properties.

> > +Note that the NOR controller does not have any internal chip-select address
> > +decoding and if you want to access multiple devices external chip-select
> > +decoding must be provided.
> 
> Although it is true, you do have the MIO address space and so you could
> support two devices via the SNOR address space and MIO address space
> (assuming that the MIO can be used for the 2nd device).

Now I'm even more confused. If the GMI controller itself can't select a
chip, what is the SNOR_SEL field in the SNOR_CONFIG_0 register for? Does
that not select a specific chip?

> Furthermore, if you do have external logic to support multiple devices
> this would assume that the devices use the same timing and so are
> probably the same type. It also assumes both can fit in the 256MB
> address range. May be worth mentioning.

Similarly if you switch between different devices, wouldn't you have to
reprogram the timing registers if they are different?

The way I remember this kind of interface to work (it's been a long time
since I used one) is that in order to operate on a chip you need to
acquire the bus first. Typically that would be an API exposed by the bus
driver or some framework that the bus driver registers with. That API
arbitrates between multiple devices on the bus and makes sure that the
proper chip select is asserted and timing is programmed when you're
granted access. A driver that has acquired the bus can then perform what
operations they need and release the bus when done.

SPI uses a mechanism like this, for example.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160725/ba5be04a/attachment-0001.sig>

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Thierry Reding]

On Thu, Jul 21, 2016 at 10:10:49PM +0200, Mirza Krak wrote:
> 2016-07-21 11:56 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
> >> +
> >> +The NOR controller supports a number of memory types, including synchronous NOR,
> >> +asynchronous NOR, and other flash memories with similar interfaces, such as
> >> +MuxOneNAND. One could also connect high speed devices like FPGAs, DSPs,
> >> +CAN chips, Wi-Fi chips etc.
> >
> > Nit-pick ... the Tegra documentation refers to this controller as the
> > GMI (general memory interface) or SNOR (sync-NOR) controller because it
> > is not just limited to NOR as you mentioned. I see references to GMI in
> > the Tegra pinctrl driver and so may be we should use this name.
> 
> ACK.
> 
> 
> >> +Required properties:
> >> +
> >> + - compatible: should be "nvidia,tegra20-nor", "nvidia,tegra30-nor"
> >
> > I see at least one difference at the register level between Tegra20 and
> > Tegra30 and so I think this should be something like ...
> >
> >  - compatible : Should contain one of the following:
> >         For Tegra20 must contain "nvidia,tegra20-gmi".
> >         For Tegra30 must contain "nvidia,tegra30-gmi".
> 
> ACK. Just curious, which register was it? I only checked that they
> have the same count of registers.
> 
> >> + - nvidia,config: This property represents the SNOR_CONFIG_0 register.
> >
> > There is also a SNOR_MIO_CONFIG for the MIO address space and so I think
> > that this should be nvidia,snor-config to be explicit. It might be nice
> > to also add a "nvidia,mio-config" while you are at it as well, however,
> > that could always be done later. If you do, then the
> > "nvidia,snor-config" becomes optional depending on whether you are using
> > the SNOR or MIO address space.
> 
> ACK the nvidia,snor-config part, will though wait for further comments
> regarding what to do with the config registers, break-out or keep it
> is a one property / register.
> 
> Regarding mio-config, not sure about if I would like to include that
> part in this stage. If you feel strongly about this we can do it. If
> it only comes to down to replicate the same configurations that we do
> for SNOR to MIO then I do not see much of a problem, but would like
> SNOR to be accepted and would not like the MIO part to halt this. But
> then again this up to you guys.

What's the difference between SNOR and MIO? Sorry if I'm being dense but
a quick look around the internet didn't yield anything related. I'd be
happy to read up if somebody can provide a link.

> > I wonder if it is worth mentioning that the chip-select specified in the
> > "nvidia,config" prop should match that in the "ranges" prop unless you
> > have some external decoding logic to provide an external chip-select.
> > Which raises a question, what does the chip-select in the ranges
> > actually represent? I am not sure if there is a common practice here for
> > device tree when boards have external logic to provide additional
> > chip-selects. I am sure this is quite common.
> 
> I do not understand why CS pin setting in nvidia,config need to match
> the "ranges" prop? Other then maybe cosmetics.
> 
> If we do not have any external decoding logic to create more
> chip-selects we only have ONE chip-select, and that one should always
> be indexed as 0? Regardless of which CS pin is used. Because
> ultimately what we configure in SNOR_CONFIG is which PIN(function) to
> use as chip-select. The address space remains the same.

Is that really so? Looking at the list of pins there are 8 CS outputs
from the GMI controller. That and the presence of the SNOR_SEL field in
the SNOR_CONFIG_0 register indicate to me that you can use software to
assert any of the CS outputs (though only one at the same time, which
makes sense since you want to avoid writing to multiple chips at once).

Of course the documentation is to blame here, it doesn't go into any
detail at all about how to use the GMI controller.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160725/69ed915c/attachment.sig>

[RFC 6/6] bus: Add support for Tegra NOR controller

[Mirza Krak]

2016-07-25 13:14 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> On Tue, Jul 19, 2016 at 03:36:37PM +0200, Mirza Krak wrote:
>> +config TEGRA_NOR
>> +     tristate "Nvidia Tegra NOR flash bus driver a.k.a GMI/SNOR"
>> +             depends on ARCH_TEGRA_2x_SOC || ARCH_TEGRA_3x_SOC
>
> I think an ARCH_TEGRA dependency is enough here.

ACK.

>
>> +             depends on OF
>
> You can drop this because Tegra has been OF-only for a long time now.

ACK.

>
>> +             help
>> +                     Driver for NOR flash bus found on Tegra30/20 SOC`s.
>
> Maybe make this "Driver for GMI controller found on NVIDIA Tegra SoCs."
> or similar in light of the name change.

ACK.

>> --- /dev/null
>> +++ b/drivers/bus/tegra-nor.c
>> @@ -0,0 +1,118 @@
>> +/*
>> + * Driver for Nvidia NOR Flash bus a.k.a SNOR/GMI.
>
> Nit: I encourage the use of "NVIDIA" as spelling for consistency.

ACK.


>> +static int __init nor_parse_dt(struct platform_device *pdev,
>> +                             void __iomem *base)
>> +{
>> +     struct device_node *of_node = pdev->dev.of_node;
>> +     u32 config, timing[TEGRA_NOR_TIMING_REG_COUNT];
>> +     int ret;
>> +
>> +     ret = of_property_read_u32_array(of_node, "nvidia,cs-timing",
>> +                                      timing, TEGRA_NOR_TIMING_REG_COUNT);
>> +     if (!ret) {
>> +             writel(timing[0], base + TEGRA_NOR_TIMING0);
>> +             writel(timing[1], base + TEGRA_NOR_TIMING1);
>> +     }
>> +
>> +     ret = of_property_read_u32(of_node, "nvidia,config", &config);
>> +     if (ret)
>> +             return ret;
>> +
>> +     config |= TEGRA_NOR_CONFIG_GO;
>> +     writel(config, base + TEGRA_NOR_CONFIG);
>
> My preference would be for the tegra_gmi_parse_dt() function not to do
> any register programming. Instead I think it would be better to store
> any of the parameters inside a struct tegra_gmi and do the programming
> from within tegra_gmi_probe() (or via an other function such as
> tegra_gmi_initialize(), called from tegra_gmi_probe()).
>
> The reason is that you'll most likely want to do this programming when
> you resume from suspend, and you could reuse tegra_gmi_initialize() to
> do that.

ACK.

>
>> +
>> +     if (of_get_child_count(of_node))
>> +             ret = of_platform_populate(of_node,
>> +                                of_default_bus_match_table,
>> +                                NULL, &pdev->dev);
>
> Why the extra check? of_platform_populate() is almost a no-op if there
> aren't any children anyway. What it will do is set the OF_POPULATED_BUS
> flag, but I think we want that irrespective of whether or not there are
> any children.
>
> Was there any problem with calling it unconditionally that made you opt
> for the extra check?

I have not tested calling it unconditionally. Used another driver as
reference and they had that condition (drivers/bus/imx-weim.c), that
driver does not mention why the condition is there. But will test
removing the condition and see what happens.

>
> Also, I think you can call of_platform_default_populate(), which is a
> little shorter than the above because you can omit the match table.

Will look in to this.

>
>> +
>> +     return ret;
>> +}
>> +
>> +static int __init nor_probe(struct platform_device *pdev)
>> +{
>> +     struct resource *res;
>> +     struct clk *clk;
>> +     void __iomem *base;
>> +     int ret;
>> +
>> +     /* get the resource */
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     base = devm_ioremap_resource(&pdev->dev, res);
>> +     if (IS_ERR(base))
>> +             return PTR_ERR(base);
>> +
>> +     /* get the clock */
>> +     clk = devm_clk_get(&pdev->dev, NULL);
>
> Let's use a consumer name here. The problem with a NULL consumer name is
> that it effectively restricts the DT binding. It means that whatever the
> clock is its name needs to be the first in a clock-names property. We'll
> likely get away with this because there's only one clock, but should we
> ever need to add another we'd need to add wording to the device tree
> bindings that the "gmi" entry needs to be first.
>
> I'm not sure if that's clear, so I'll try to explain in other words: If
> you pass a NULL consumer name to clk_get() it will simply use the first
> clock in the clocks property. If you want to later extend the DT binding
> by adding a clock in a backwards-compatible way, you'll need to add the
> restriction that the "gmi" clock (the one that was previously unnamed)
> must be the first in the clock-names and clocks properties.
>
> That's all a little confusing, so let's avoid this by giving it a proper
> consumer name to begin with.

Got it. Will add a proper consumer name.

>
>> +     if (IS_ERR(clk))
>> +             return PTR_ERR(clk);
>> +
>> +     ret = clk_prepare_enable(clk);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /* parse the device node */
>> +     ret = nor_parse_dt(pdev, base);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "%s fail to create devices.\n",
>> +                     pdev->dev.of_node->full_name);
>> +             clk_disable_unprepare(clk);
>> +             return ret;
>> +     }
>
> The good thing if you don't do any programming in tegra_gmi_parse_dt(),
> is that you can easily move the clk_prepare_enable() call to here where
> things can't fail anymore, so you don't have to add cleanup code.

ACK.

>
>> +
>> +     dev_info(&pdev->dev, "Driver registered.\n");
>
> Please avoid this kind of output. Users expect that everything will work
> so you want to let them know when something goes wrong, and be quiet
> when all goes as expected.

Will remove that.

>> +static struct platform_driver nor_driver = {
>> +     .driver = {
>> +             .name           = "tegra-nor",
>> +             .of_match_table = nor_id_table,
>> +     },
>> +};
>> +module_platform_driver_probe(nor_driver, nor_probe);
>> +
>> +MODULE_AUTHOR("Mirza Krak <mirza.krak@gmail.com");
>> +MODULE_DESCRIPTION("NVIDIA Tegra NOR Bus Driver");
>> +MODULE_LICENSE("GPL");
>
> You're header comment says GPL version 2, which means that the
> MODULE_LICENSE needs to be "GPL v2". "GPL" means "version 2 or later".

Ok, I do not really mind it being GPL version 2 or later so will
change the header comment instead if that is ok.


Best regards,
Mirza

[RFC 1/6] clk: tegra: add TEGRA20_CLK_NOR to init table

[Mirza Krak]

2016-07-25 13:17 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> On Tue, Jul 19, 2016 at 03:36:32PM +0200, Mirza Krak wrote:
>> From: Mirza Krak <mirza.krak@gmail.com>
>>
>> Add TEGRA20_CLK_NOR to init tabel and set a "sane" default rate.
>>
>> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
>> ---
>>  drivers/clk/tegra/clk-tegra20.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
>> index 837e5cb..aefc044 100644
>> --- a/drivers/clk/tegra/clk-tegra20.c
>> +++ b/drivers/clk/tegra/clk-tegra20.c
>> @@ -1047,6 +1047,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
>>       { TEGRA20_CLK_SDMMC3, TEGRA20_CLK_PLL_P, 48000000, 0 },
>>       { TEGRA20_CLK_SDMMC4, TEGRA20_CLK_PLL_P, 48000000, 0 },
>>       { TEGRA20_CLK_SPI, TEGRA20_CLK_PLL_P, 20000000, 0 },
>> +     { TEGRA20_CLK_NOR, TEGRA20_CLK_PLL_P, 86500000, 0 },
>
> Yay for inconsistent naming in the hardware. It would've been nice if
> this clock was called GMI. Oh well...

I am allowed to change clk name? Can do that when I re-spin this series.

>
> Could you perhaps explain in the commit message why 86.5 MHz is a sane
> default? I'm totally unfamiliar with this controller, so maybe it's
> self-explanatory, but it seems a rather odd value for a clock frequency.

I used a value that I have in a downstream kernel based on the L4T.
This frequency is well tested and has worked for me, and wanted to
avoid setting it to maximum rate. My guess is that they used 86.5 MHz
because 92 MHz is max rate on Tegra2 and they put it slightly below
that.

What is otherwise recommended when initializing clocks? The rate could
depend on the chip that is attached but otherwise I would like to set
it close to max (or max) rate for performance.

Best Regards,
Mirza

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Jon Hunter]

On 25/07/16 13:10, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Thu, Jul 21, 2016 at 10:10:49PM +0200, Mirza Krak wrote:
>> 2016-07-21 11:56 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>>>> +
>>>> +The NOR controller supports a number of memory types, including synchronous NOR,
>>>> +asynchronous NOR, and other flash memories with similar interfaces, such as
>>>> +MuxOneNAND. One could also connect high speed devices like FPGAs, DSPs,
>>>> +CAN chips, Wi-Fi chips etc.
>>>
>>> Nit-pick ... the Tegra documentation refers to this controller as the
>>> GMI (general memory interface) or SNOR (sync-NOR) controller because it
>>> is not just limited to NOR as you mentioned. I see references to GMI in
>>> the Tegra pinctrl driver and so may be we should use this name.
>>
>> ACK.
>>
>>
>>>> +Required properties:
>>>> +
>>>> + - compatible: should be "nvidia,tegra20-nor", "nvidia,tegra30-nor"
>>>
>>> I see at least one difference at the register level between Tegra20 and
>>> Tegra30 and so I think this should be something like ...
>>>
>>>  - compatible : Should contain one of the following:
>>>         For Tegra20 must contain "nvidia,tegra20-gmi".
>>>         For Tegra30 must contain "nvidia,tegra30-gmi".
>>
>> ACK. Just curious, which register was it? I only checked that they
>> have the same count of registers.
>>
>>>> + - nvidia,config: This property represents the SNOR_CONFIG_0 register.
>>>
>>> There is also a SNOR_MIO_CONFIG for the MIO address space and so I think
>>> that this should be nvidia,snor-config to be explicit. It might be nice
>>> to also add a "nvidia,mio-config" while you are at it as well, however,
>>> that could always be done later. If you do, then the
>>> "nvidia,snor-config" becomes optional depending on whether you are using
>>> the SNOR or MIO address space.
>>
>> ACK the nvidia,snor-config part, will though wait for further comments
>> regarding what to do with the config registers, break-out or keep it
>> is a one property / register.
>>
>> Regarding mio-config, not sure about if I would like to include that
>> part in this stage. If you feel strongly about this we can do it. If
>> it only comes to down to replicate the same configurations that we do
>> for SNOR to MIO then I do not see much of a problem, but would like
>> SNOR to be accepted and would not like the MIO part to halt this. But
>> then again this up to you guys.
> 
> What's the difference between SNOR and MIO? Sorry if I'm being dense but
> a quick look around the internet didn't yield anything related. I'd be
> happy to read up if somebody can provide a link.

I am not sure where this term MIO comes from (may be an NVIDIA term),
but from looking at the MIO_CONFIG register, it looks like a basic
16/32-bit interface with configurable read/write strobe timing. Does not
support bursting or address/data multiplexing that the SNOR interface
does. So may be it is used for interfacing to external devices such as
FIFOs, UARTs, I2C expanders, etc.

Cheers
Jon

-- 
nvpublic

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Mirza Krak]

2016-07-25 13:30 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> On Tue, Jul 19, 2016 at 03:36:34PM +0200, Mirza Krak wrote:
>> From: Mirza Krak <mirza.krak@gmail.com>
>>
>> Document the devicetree bindings for NOR bus driver found on Tegra20 and
>> Tegra30 SOCs
>>
>> +
>> +Required properties:
>> +
>> + - compatible: should be "nvidia,tegra20-nor", "nvidia,tegra30-nor"
>> + - reg: Should contain NOR controller registers location and length.
>> + - clocks: Must contain one entry, for the module clock.
>> +   See ../clocks/clock-bindings.txt for details.
>> + - resets : Must contain an entry for each entry in reset-names.
>> +   See ../reset/reset.txt for details.
>> + - reset-names : Must include the following entries:
>> +  - nor
>> + - #address-cells: Must be set to 2 to allow memory address translation
>> + - #size-cells:      Must be set to 1 to allow CS address passing
>> + - ranges: Must be set up to reflect the memory layout with four integer
>> +             values for each chip-select line in use.
>> + - nvidia,config: This property represents the SNOR_CONFIG_0 register.
>
> I'd prefer if this was split out into separate properties. It's somewhat
> friendlier in my opinion to let people know what each of the settings is
> along with any units and such, rather than point them at the TRM, which
> they may or may not have access to.
>
> Or which not be available anymore.

Will split it out.

>
>> +
>> +Note that the NOR controller does not have any internal chip-select address
>> +decoding and if you want to access multiple devices external chip-select
>> +decoding must be provided.
>> +
>> +Optional properties:
>> +
>> + - nvidia,cs-timing: The timing array represents the SNOR_TIMING0_0 and
>> +   SNOR_TIMING1_0 registers for the NOR controller. If unset reset-values will
>> +   be used. See reference documentation for detailed description of the timing
>> +   registers.
>
> Are the reset values sensible? Are they reasonable defaults for a class
> of use-cases? I'm thinking that if they aren't we might as well make it
> a required property.
>
> Also, I'd prefer if this was split out into individual fields for the
> same reasons as the SNOR_CONFIG_0 register property.

I have tested the default values with two different chip types, and
both have worked without me adjusting any timings. The tested chips
are SJA1000 (CAN) and 16C550 (UART). So I would say that they are
sensible.

Will split it out as well.

>
>> +
>> +Example with two SJA1000 CAN controllers connected to the NOR bus:
>> +
>> +     nor at 70009000 {
>> +             status = "okay";
>> +             compatible = "nvidia,tegra20-nor", "nvidia,tegra30-nor";
>> +             reg = <0x70009000 0x1000>;
>> +             #address-cells = <2>;
>> +             #size-cells = <1>;
>> +             clocks = <&tegra_car TEGRA30_CLK_NOR>;
>> +             resets = < &tegra_car 42>;
>
> No space between < and &, please.

ACK.

>
>> +             reset-names = "nor";
>> +             ranges = <
>> +                     0 0 0x48000000 0x00000100
>> +                     1 0 0x48040000 0x00000100
>> +             >;
>> +
>> +             can at 0,0 {
>> +                     compatible = "nxp,sja1000";
>> +                     reg = <0 0 0x100>;
>> +                     interrupt-parent = <&gpio>;
>> +                     interrupts = <TEGRA_GPIO(B, 5) IRQ_TYPE_EDGE_RISING>;
>> +                     nxp,external-clock-frequency = <24000000>;
>> +                     nxp,tx-output-config = <0x16>;
>> +                     nxp,clock-out-frequency = <24000000>;
>> +                     reg-io-width = <2>;
>> +             };
>> +
>> +
>
> There's an extra blank line here.

ACK.

>
>> +             can at 1,0 {
>> +                     compatible = "nxp,sja1000";
>> +                     reg = <1 0 0x100>;
>> +                     interrupt-parent = <&gpio>;
>> +                     interrupts = <TEGRA_GPIO(A, 6) IRQ_TYPE_EDGE_RISING>;
>> +                     nxp,external-clock-frequency = <24000000>;
>> +                     nxp,tx-output-config = <0x16>;
>> +                     reg-io-width = <2>;
>> +     };
>
> I'm somewhat confused about how this hardware works. My understanding
> was that each chip gets mapped to the whole range of the NOR address
> range (0x48000000 - 0x4fffffff on Tegra30).

I can understand the confusion.

>
> The above suggests that one of the CAN controllers gets mapped to an
> address 0x48000000 and the other gets mapped to 0x48040000. But why do
> we even need a chip-select at all in that case? If both chips don't use
> any overlapping memory region, what good does the chip-select do?

If we take a look on similar controllers found on others SOCs they
usually define an address range / chip-select.

Example (weim):
ranges = <
0 0 0x10000000 0x02000000
1 0 0x12000000 0x01000000
2 0 0x13000000 0x01000000
3 0 0x14000000 0x01000000
4 0 0x15000000 0x01000000
5 0 0x16000000 0x01000000
>;

Which means that you all ready have an address mapped to PIN function.

But Tegra GMI controller is a first for me, where you do not have this
kind of setup in hardware. Usually you also have a timing register /
chip-select so that you can connect different chip types.

The lack of hardware support do decode an address to a chip-select PIN
function, we have implemented this our self externally.

We actually have 6 different chips connected to the GMI bus and the
"ranges" would be:
  ranges = <
   0 0 0x48000000 0x00000100
   1 0 0x48040000 0x00000100
   2 0 0x48080000 0x00000100
   3 0 0x480A0000 0x00000100
   4 0 0x480C0000 0x00000100
   5 0 0x480E0000 0x00000100
  >;

And this not nothing complicated, small number of AND gates and that is it.

The chip-select signal is necessary for the access characteristics, so
we need to translate an address to an chip-select so that the chip
knows the host CPU wants to talk to it.

Do not know if I made anything more clear here :).

>
> Also, it seems to me that you'd have to program the SNOR_CONFIG_0
> register in order to select a specific chip, but I don't see anything in
> the driver access that register after the initial write of the register.

This is only setup at probe.

>
> I would've expected this to require some sort of infrastructure to allow
> devices connected to the GMI controller to acquire the bus via some API
> to select their chip.

Yes, ultimately you would need some sort of infrastructure to allow
devices to acquire the GMI bus if you want to solve this in software.
But at the moment I do not see such an infrastructure in place, and is
it feasible to add one specifically for the GMI controller? If one
such infrastructure was in place we would need to modify all the
drivers that want to use to include Tegra specific infrastructure to
access the GMI bus?

Since my knowledge is limited it hard for me to comment on this, maybe
there is a simple way of doing this?


Best Regards,
Mirza

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Mirza Krak]

2016-07-25 13:36 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> On Thu, Jul 21, 2016 at 11:26:09AM +0100, Jon Hunter wrote:
>>
>> On 20/07/16 20:28, Mirza Krak wrote:
>> > 2016-07-20 14:44 GMT+02:00 Rob Herring <robh@kernel.org>:
>> >> On Tue, Jul 19, 2016 at 03:36:34PM +0200, Mirza Krak wrote:
>> >>> From: Mirza Krak <mirza.krak@gmail.com>
>> >>>
>> >>> Document the devicetree bindings for NOR bus driver found on Tegra20 and
>> >>> Tegra30 SOCs
>> >>>
>> >>> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
>> >>> ---
>> >>>  .../devicetree/bindings/bus/nvidia,tegra20-nor.txt | 73 ++++++++++++++++++++++
>> >>>  1 file changed, 73 insertions(+)
>> >>>  create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
>> >>>
>> >>> diff --git a/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
>> >>> new file mode 100644
>> >>> index 0000000..9ee4a66
>> >>> --- /dev/null
>> >>> +++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
>> >>> @@ -0,0 +1,73 @@
>> >>> +Device tree bindings for NVIDIA Tegra20/30 NOR Bus
>> >>> +
>> >>> +The NOR controller supports a number of memory types, including synchronous NOR,
>> >>> +asynchronous NOR, and other flash memories with similar interfaces, such as
>> >>> +MuxOneNAND. One could also connect high speed devices like FPGAs, DSPs,
>> >>> +CAN chips, Wi-Fi chips etc.
>> >>> +
>> >>> +The actual devices are instantiated from the child nodes of a NOR node.
>> >>> +
>> >>> +Required properties:
>> >>> +
>> >>> + - compatible: should be "nvidia,tegra20-nor", "nvidia,tegra30-nor"
>> >>> + - reg: Should contain NOR controller registers location and length.
>> >>> + - clocks: Must contain one entry, for the module clock.
>> >>> +   See ../clocks/clock-bindings.txt for details.
>> >>> + - resets : Must contain an entry for each entry in reset-names.
>> >>> +   See ../reset/reset.txt for details.
>> >>> + - reset-names : Must include the following entries:
>> >>> +  - nor
>> >>> + - #address-cells: Must be set to 2 to allow memory address translation
>> >>> + - #size-cells:      Must be set to 1 to allow CS address passing
>> >>> + - ranges: Must be set up to reflect the memory layout with four integer
>> >>> +             values for each chip-select line in use.
>> >>> + - nvidia,config: This property represents the SNOR_CONFIG_0 register.
>> >>> +
>> >>> +Note that the NOR controller does not have any internal chip-select address
>> >>> +decoding and if you want to access multiple devices external chip-select
>> >>> +decoding must be provided.
>> >>
>> >> Then what are the 2 chip selects in ranges?
>> >>
>> >> Rob
>> >
>> > Those two chip selects are actually a representation of a external
>> > decoding logic based on what we use on our board. Even though it the
>> > NOR controller only supports one single chip select I wanted to give
>> > an example on how one could create more chip-selects with an external
>> > logic and what it would look like in the device tree representation.
>>
>> Technically, the GMI/SNOR controller supports 8 chip-selects, however,
>> unlike some devices, it appears that software has to select the active
>> chip-select. Although this sounds odd, I believe that the idea is that
>> in order to support devices greater than 256MB (external address space
>> for available NOR/async devices) you can use the chip-selects to page
>> through memory greater than this 256MB range. At least that it my
>> (limited) understanding!
>
> Actually I had assumed that software would at some point need to select
> the active chip to switch between multiple connected chips. I suppose it
> could be possible to have multiple chips share the same chip-select and
> decode the address lines to determine whether they're being accessed or
> not.
>
> What I don't understand, and it's further complicated by the fact that
> external chip-selects are being used, is how does the controller get
> told what chip to select? It seems to me like it would always access the
> same chips because the SNOR_CONFIG_0 register is only ever written on
> ->probe().
>
> For external chip selects, how do they tie in with all this? Who gets to
> implement this logic? Wouldn't we need to abstract this away somehow so
> that we can support whatever board designers will come up with?
>
> Thierry

You answered it your self :).

>I suppose it
> could be possible to have multiple chips share the same chip-select and
> decode the address lines to determine whether they're being accessed or
> not.

That is what we do and is what I refer to as external chip-selects.


Best Regards,
Mirza

[RFC 1/6] clk: tegra: add TEGRA20_CLK_NOR to init table

[Thierry Reding]

On Mon, Jul 25, 2016 at 02:28:48PM +0200, Mirza Krak wrote:
> 2016-07-25 13:17 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> > On Tue, Jul 19, 2016 at 03:36:32PM +0200, Mirza Krak wrote:
> >> From: Mirza Krak <mirza.krak@gmail.com>
> >>
> >> Add TEGRA20_CLK_NOR to init tabel and set a "sane" default rate.
> >>
> >> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
> >> ---
> >>  drivers/clk/tegra/clk-tegra20.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> >> index 837e5cb..aefc044 100644
> >> --- a/drivers/clk/tegra/clk-tegra20.c
> >> +++ b/drivers/clk/tegra/clk-tegra20.c
> >> @@ -1047,6 +1047,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
> >>       { TEGRA20_CLK_SDMMC3, TEGRA20_CLK_PLL_P, 48000000, 0 },
> >>       { TEGRA20_CLK_SDMMC4, TEGRA20_CLK_PLL_P, 48000000, 0 },
> >>       { TEGRA20_CLK_SPI, TEGRA20_CLK_PLL_P, 20000000, 0 },
> >> +     { TEGRA20_CLK_NOR, TEGRA20_CLK_PLL_P, 86500000, 0 },
> >
> > Yay for inconsistent naming in the hardware. It would've been nice if
> > this clock was called GMI. Oh well...
> 
> I am allowed to change clk name? Can do that when I re-spin this series.

No, leave it as-is. The clock is referred to as NOR in the clock and
reset controller. I was just saying that it would've been nice if it had
been named GMI consistently.

> > Could you perhaps explain in the commit message why 86.5 MHz is a sane
> > default? I'm totally unfamiliar with this controller, so maybe it's
> > self-explanatory, but it seems a rather odd value for a clock frequency.
> 
> I used a value that I have in a downstream kernel based on the L4T.
> This frequency is well tested and has worked for me, and wanted to
> avoid setting it to maximum rate. My guess is that they used 86.5 MHz
> because 92 MHz is max rate on Tegra2 and they put it slightly below
> that.
> 
> What is otherwise recommended when initializing clocks? The rate could
> depend on the chip that is attached but otherwise I would like to set
> it close to max (or max) rate for performance.

I think we usually set the default rate to the maximum and let drivers
clock them down as they see fit. This is so that things run fast by
default and we often don't have much in the way of power management, at
least not when drivers are first merged. Maximum clock rate ensure that
we get good performance by default, rather than having to rely on
drivers to kick it up a notch.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160725/0a60c906/attachment.sig>

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Thierry Reding]

On Mon, Jul 25, 2016 at 03:20:44PM +0200, Mirza Krak wrote:
> 2016-07-25 13:36 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> > On Thu, Jul 21, 2016 at 11:26:09AM +0100, Jon Hunter wrote:
> >>
> >> On 20/07/16 20:28, Mirza Krak wrote:
> >> > 2016-07-20 14:44 GMT+02:00 Rob Herring <robh@kernel.org>:
> >> >> On Tue, Jul 19, 2016 at 03:36:34PM +0200, Mirza Krak wrote:
> >> >>> From: Mirza Krak <mirza.krak@gmail.com>
> >> >>>
> >> >>> Document the devicetree bindings for NOR bus driver found on Tegra20 and
> >> >>> Tegra30 SOCs
> >> >>>
> >> >>> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
> >> >>> ---
> >> >>>  .../devicetree/bindings/bus/nvidia,tegra20-nor.txt | 73 ++++++++++++++++++++++
> >> >>>  1 file changed, 73 insertions(+)
> >> >>>  create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> >> >>>
> >> >>> diff --git a/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> >> >>> new file mode 100644
> >> >>> index 0000000..9ee4a66
> >> >>> --- /dev/null
> >> >>> +++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> >> >>> @@ -0,0 +1,73 @@
> >> >>> +Device tree bindings for NVIDIA Tegra20/30 NOR Bus
> >> >>> +
> >> >>> +The NOR controller supports a number of memory types, including synchronous NOR,
> >> >>> +asynchronous NOR, and other flash memories with similar interfaces, such as
> >> >>> +MuxOneNAND. One could also connect high speed devices like FPGAs, DSPs,
> >> >>> +CAN chips, Wi-Fi chips etc.
> >> >>> +
> >> >>> +The actual devices are instantiated from the child nodes of a NOR node.
> >> >>> +
> >> >>> +Required properties:
> >> >>> +
> >> >>> + - compatible: should be "nvidia,tegra20-nor", "nvidia,tegra30-nor"
> >> >>> + - reg: Should contain NOR controller registers location and length.
> >> >>> + - clocks: Must contain one entry, for the module clock.
> >> >>> +   See ../clocks/clock-bindings.txt for details.
> >> >>> + - resets : Must contain an entry for each entry in reset-names.
> >> >>> +   See ../reset/reset.txt for details.
> >> >>> + - reset-names : Must include the following entries:
> >> >>> +  - nor
> >> >>> + - #address-cells: Must be set to 2 to allow memory address translation
> >> >>> + - #size-cells:      Must be set to 1 to allow CS address passing
> >> >>> + - ranges: Must be set up to reflect the memory layout with four integer
> >> >>> +             values for each chip-select line in use.
> >> >>> + - nvidia,config: This property represents the SNOR_CONFIG_0 register.
> >> >>> +
> >> >>> +Note that the NOR controller does not have any internal chip-select address
> >> >>> +decoding and if you want to access multiple devices external chip-select
> >> >>> +decoding must be provided.
> >> >>
> >> >> Then what are the 2 chip selects in ranges?
> >> >>
> >> >> Rob
> >> >
> >> > Those two chip selects are actually a representation of a external
> >> > decoding logic based on what we use on our board. Even though it the
> >> > NOR controller only supports one single chip select I wanted to give
> >> > an example on how one could create more chip-selects with an external
> >> > logic and what it would look like in the device tree representation.
> >>
> >> Technically, the GMI/SNOR controller supports 8 chip-selects, however,
> >> unlike some devices, it appears that software has to select the active
> >> chip-select. Although this sounds odd, I believe that the idea is that
> >> in order to support devices greater than 256MB (external address space
> >> for available NOR/async devices) you can use the chip-selects to page
> >> through memory greater than this 256MB range. At least that it my
> >> (limited) understanding!
> >
> > Actually I had assumed that software would at some point need to select
> > the active chip to switch between multiple connected chips. I suppose it
> > could be possible to have multiple chips share the same chip-select and
> > decode the address lines to determine whether they're being accessed or
> > not.
> >
> > What I don't understand, and it's further complicated by the fact that
> > external chip-selects are being used, is how does the controller get
> > told what chip to select? It seems to me like it would always access the
> > same chips because the SNOR_CONFIG_0 register is only ever written on
> > ->probe().
> >
> > For external chip selects, how do they tie in with all this? Who gets to
> > implement this logic? Wouldn't we need to abstract this away somehow so
> > that we can support whatever board designers will come up with?
> >
> > Thierry
> 
> You answered it your self :).
> 
> >I suppose it
> > could be possible to have multiple chips share the same chip-select and
> > decode the address lines to determine whether they're being accessed or
> > not.
> 
> That is what we do and is what I refer to as external chip-selects.

Okay, so there aren't actually chips or pins that serve as external chip
selects, but rather the GMI address lines are used to select the chip? I
guess that's more like traditional address decoding rather than chip
select. Anyway, understanding how your board design works helps devising
a device tree binding that is flexible enough to support production
devices.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160725/979c652a/attachment.sig>

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Mirza Krak]

2016-07-25 13:59 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> On Thu, Jul 21, 2016 at 10:56:44AM +0100, Jon Hunter wrote:
>>
>
>> > +Note that the NOR controller does not have any internal chip-select address
>> > +decoding and if you want to access multiple devices external chip-select
>> > +decoding must be provided.
>>
>> Although it is true, you do have the MIO address space and so you could
>> support two devices via the SNOR address space and MIO address space
>> (assuming that the MIO can be used for the 2nd device).
>
> Now I'm even more confused. If the GMI controller itself can't select a
> chip, what is the SNOR_SEL field in the SNOR_CONFIG_0 register for? Does
> that not select a specific chip?
>
>> Furthermore, if you do have external logic to support multiple devices
>> this would assume that the devices use the same timing and so are
>> probably the same type. It also assumes both can fit in the 256MB
>> address range. May be worth mentioning.
>
> Similarly if you switch between different devices, wouldn't you have to
> reprogram the timing registers if they are different?
>
> The way I remember this kind of interface to work (it's been a long time
> since I used one) is that in order to operate on a chip you need to
> acquire the bus first. Typically that would be an API exposed by the bus
> driver or some framework that the bus driver registers with. That API
> arbitrates between multiple devices on the bus and makes sure that the
> proper chip select is asserted and timing is programmed when you're
> granted access. A driver that has acquired the bus can then perform what
> operations they need and release the bus when done.
>
> SPI uses a mechanism like this, for example.
>
> Thierry

>From my experience (maybe not as long as yours :)) but these kind of
things would be handled by the controller. At least with previous SOCs
that I have used, PXA270, PXA300 and i.MX SOCs.

That it has an address range per chip-select PIN and timing registers
per chip-select. And thus eliminating a need for a infrastructure or
framework.


Best regards,
Mirza

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Thierry Reding]

On Mon, Jul 25, 2016 at 02:09:18PM +0100, Jon Hunter wrote:
> 
> On 25/07/16 13:10, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Thu, Jul 21, 2016 at 10:10:49PM +0200, Mirza Krak wrote:
> >> 2016-07-21 11:56 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
> >>>> +
> >>>> +The NOR controller supports a number of memory types, including synchronous NOR,
> >>>> +asynchronous NOR, and other flash memories with similar interfaces, such as
> >>>> +MuxOneNAND. One could also connect high speed devices like FPGAs, DSPs,
> >>>> +CAN chips, Wi-Fi chips etc.
> >>>
> >>> Nit-pick ... the Tegra documentation refers to this controller as the
> >>> GMI (general memory interface) or SNOR (sync-NOR) controller because it
> >>> is not just limited to NOR as you mentioned. I see references to GMI in
> >>> the Tegra pinctrl driver and so may be we should use this name.
> >>
> >> ACK.
> >>
> >>
> >>>> +Required properties:
> >>>> +
> >>>> + - compatible: should be "nvidia,tegra20-nor", "nvidia,tegra30-nor"
> >>>
> >>> I see at least one difference at the register level between Tegra20 and
> >>> Tegra30 and so I think this should be something like ...
> >>>
> >>>  - compatible : Should contain one of the following:
> >>>         For Tegra20 must contain "nvidia,tegra20-gmi".
> >>>         For Tegra30 must contain "nvidia,tegra30-gmi".
> >>
> >> ACK. Just curious, which register was it? I only checked that they
> >> have the same count of registers.
> >>
> >>>> + - nvidia,config: This property represents the SNOR_CONFIG_0 register.
> >>>
> >>> There is also a SNOR_MIO_CONFIG for the MIO address space and so I think
> >>> that this should be nvidia,snor-config to be explicit. It might be nice
> >>> to also add a "nvidia,mio-config" while you are at it as well, however,
> >>> that could always be done later. If you do, then the
> >>> "nvidia,snor-config" becomes optional depending on whether you are using
> >>> the SNOR or MIO address space.
> >>
> >> ACK the nvidia,snor-config part, will though wait for further comments
> >> regarding what to do with the config registers, break-out or keep it
> >> is a one property / register.
> >>
> >> Regarding mio-config, not sure about if I would like to include that
> >> part in this stage. If you feel strongly about this we can do it. If
> >> it only comes to down to replicate the same configurations that we do
> >> for SNOR to MIO then I do not see much of a problem, but would like
> >> SNOR to be accepted and would not like the MIO part to halt this. But
> >> then again this up to you guys.
> > 
> > What's the difference between SNOR and MIO? Sorry if I'm being dense but
> > a quick look around the internet didn't yield anything related. I'd be
> > happy to read up if somebody can provide a link.
> 
> I am not sure where this term MIO comes from (may be an NVIDIA term),
> but from looking at the MIO_CONFIG register, it looks like a basic
> 16/32-bit interface with configurable read/write strobe timing. Does not
> support bursting or address/data multiplexing that the SNOR interface
> does. So may be it is used for interfacing to external devices such as
> FIFOs, UARTs, I2C expanders, etc.

Yes, looks like some sort of parallel interface to connect external
devices and make them act like MMIO. Perhaps MIO is supposed to be
"memory I/O". I've found some vague references to MIO == multi-I/O,
but that was always related to serial flash (essentially something
like QSPI).

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160725/ce37864a/attachment.sig>

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Mirza Krak]

2016-07-25 15:27 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> On Mon, Jul 25, 2016 at 03:20:44PM +0200, Mirza Krak wrote:
>> 2016-07-25 13:36 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
>> > On Thu, Jul 21, 2016 at 11:26:09AM +0100, Jon Hunter wrote:
>> >>
>> >> On 20/07/16 20:28, Mirza Krak wrote:
>> >> > 2016-07-20 14:44 GMT+02:00 Rob Herring <robh@kernel.org>:
>> >> >> On Tue, Jul 19, 2016 at 03:36:34PM +0200, Mirza Krak wrote:
>> >> >>> From: Mirza Krak <mirza.krak@gmail.com>
>> >> >>>
>> >> >>> Document the devicetree bindings for NOR bus driver found on Tegra20 and
>> >> >>> Tegra30 SOCs
>> >> >>>
>> >> >>> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
>> >> >>> ---
>> >> >>>  .../devicetree/bindings/bus/nvidia,tegra20-nor.txt | 73 ++++++++++++++++++++++
>> >> >>>  1 file changed, 73 insertions(+)
>> >> >>>  create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
>> >> >>>
>> >> >>> diff --git a/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
>> >> >>> new file mode 100644
>> >> >>> index 0000000..9ee4a66
>> >> >>> --- /dev/null
>> >> >>> +++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
>> >> >>> @@ -0,0 +1,73 @@
>> >> >>> +Device tree bindings for NVIDIA Tegra20/30 NOR Bus
>> >> >>> +
>> >> >>> +The NOR controller supports a number of memory types, including synchronous NOR,
>> >> >>> +asynchronous NOR, and other flash memories with similar interfaces, such as
>> >> >>> +MuxOneNAND. One could also connect high speed devices like FPGAs, DSPs,
>> >> >>> +CAN chips, Wi-Fi chips etc.
>> >> >>> +
>> >> >>> +The actual devices are instantiated from the child nodes of a NOR node.
>> >> >>> +
>> >> >>> +Required properties:
>> >> >>> +
>> >> >>> + - compatible: should be "nvidia,tegra20-nor", "nvidia,tegra30-nor"
>> >> >>> + - reg: Should contain NOR controller registers location and length.
>> >> >>> + - clocks: Must contain one entry, for the module clock.
>> >> >>> +   See ../clocks/clock-bindings.txt for details.
>> >> >>> + - resets : Must contain an entry for each entry in reset-names.
>> >> >>> +   See ../reset/reset.txt for details.
>> >> >>> + - reset-names : Must include the following entries:
>> >> >>> +  - nor
>> >> >>> + - #address-cells: Must be set to 2 to allow memory address translation
>> >> >>> + - #size-cells:      Must be set to 1 to allow CS address passing
>> >> >>> + - ranges: Must be set up to reflect the memory layout with four integer
>> >> >>> +             values for each chip-select line in use.
>> >> >>> + - nvidia,config: This property represents the SNOR_CONFIG_0 register.
>> >> >>> +
>> >> >>> +Note that the NOR controller does not have any internal chip-select address
>> >> >>> +decoding and if you want to access multiple devices external chip-select
>> >> >>> +decoding must be provided.
>> >> >>
>> >> >> Then what are the 2 chip selects in ranges?
>> >> >>
>> >> >> Rob
>> >> >
>> >> > Those two chip selects are actually a representation of a external
>> >> > decoding logic based on what we use on our board. Even though it the
>> >> > NOR controller only supports one single chip select I wanted to give
>> >> > an example on how one could create more chip-selects with an external
>> >> > logic and what it would look like in the device tree representation.
>> >>
>> >> Technically, the GMI/SNOR controller supports 8 chip-selects, however,
>> >> unlike some devices, it appears that software has to select the active
>> >> chip-select. Although this sounds odd, I believe that the idea is that
>> >> in order to support devices greater than 256MB (external address space
>> >> for available NOR/async devices) you can use the chip-selects to page
>> >> through memory greater than this 256MB range. At least that it my
>> >> (limited) understanding!
>> >
>> > Actually I had assumed that software would at some point need to select
>> > the active chip to switch between multiple connected chips. I suppose it
>> > could be possible to have multiple chips share the same chip-select and
>> > decode the address lines to determine whether they're being accessed or
>> > not.
>> >
>> > What I don't understand, and it's further complicated by the fact that
>> > external chip-selects are being used, is how does the controller get
>> > told what chip to select? It seems to me like it would always access the
>> > same chips because the SNOR_CONFIG_0 register is only ever written on
>> > ->probe().
>> >
>> > For external chip selects, how do they tie in with all this? Who gets to
>> > implement this logic? Wouldn't we need to abstract this away somehow so
>> > that we can support whatever board designers will come up with?
>> >
>> > Thierry
>>
>> You answered it your self :).
>>
>> >I suppose it
>> > could be possible to have multiple chips share the same chip-select and
>> > decode the address lines to determine whether they're being accessed or
>> > not.
>>
>> That is what we do and is what I refer to as external chip-selects.
>
> Okay, so there aren't actually chips or pins that serve as external chip
> selects, but rather the GMI address lines are used to select the chip? I
> guess that's more like traditional address decoding rather than chip
> select. Anyway, understanding how your board design works helps devising
> a device tree binding that is flexible enough to support production
> devices.
>
> Thierry

This is the most accurate descriptor.

> GMI address lines are used to select the chip

Best Regards
Mirza

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Jon Hunter]

On 25/07/16 12:59, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Thu, Jul 21, 2016 at 10:56:44AM +0100, Jon Hunter wrote:
>>
>> On 19/07/16 14:36, Mirza Krak wrote:
>>> From: Mirza Krak <mirza.krak@gmail.com>
>>>
>>> Document the devicetree bindings for NOR bus driver found on Tegra20 and
>>> Tegra30 SOCs
>>>
>>> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
>>> ---
>>>  .../devicetree/bindings/bus/nvidia,tegra20-nor.txt | 73 ++++++++++++++++++++++
>>>  1 file changed, 73 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
>>> new file mode 100644
>>> index 0000000..9ee4a66
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
>>> @@ -0,0 +1,73 @@
>>> +Device tree bindings for NVIDIA Tegra20/30 NOR Bus
>>> +
>>> +The NOR controller supports a number of memory types, including synchronous NOR,
>>> +asynchronous NOR, and other flash memories with similar interfaces, such as
>>> +MuxOneNAND. One could also connect high speed devices like FPGAs, DSPs,
>>> +CAN chips, Wi-Fi chips etc.
>>
>> Nit-pick ... the Tegra documentation refers to this controller as the
>> GMI (general memory interface) or SNOR (sync-NOR) controller because it
>> is not just limited to NOR as you mentioned. I see references to GMI in
>> the Tegra pinctrl driver and so may be we should use this name.
>>
>>> +
>>> +The actual devices are instantiated from the child nodes of a NOR node.
>>> +
>>> +Required properties:
>>> +
>>> + - compatible: should be "nvidia,tegra20-nor", "nvidia,tegra30-nor"
>>
>> I see at least one difference at the register level between Tegra20 and
>> Tegra30 and so I think this should be something like ...
>>
>>  - compatible : Should contain one of the following:
>> 	For Tegra20 must contain "nvidia,tegra20-gmi".
>>  	For Tegra30 must contain "nvidia,tegra30-gmi".
>>
>>> + - reg: Should contain NOR controller registers location and length.
>>> + - clocks: Must contain one entry, for the module clock.
>>> +   See ../clocks/clock-bindings.txt for details.
>>> + - resets : Must contain an entry for each entry in reset-names.
>>> +   See ../reset/reset.txt for details.
>>> + - reset-names : Must include the following entries:
>>> +  - nor
>>> + - #address-cells: Must be set to 2 to allow memory address translation
>>> + - #size-cells:	Must be set to 1 to allow CS address passing
>>> + - ranges: Must be set up to reflect the memory layout with four integer
>>> + 		values for each chip-select line in use.
>>> + - nvidia,config: This property represents the SNOR_CONFIG_0 register.
>>
>> There is also a SNOR_MIO_CONFIG for the MIO address space and so I think
>> that this should be nvidia,snor-config to be explicit. It might be nice
>> to also add a "nvidia,mio-config" while you are at it as well, however,
>> that could always be done later. If you do, then the
>> "nvidia,snor-config" becomes optional depending on whether you are using
>> the SNOR or MIO address space.
>>
>> Thierry, Stephen, do prefer all the fields on the config registers are
>> broken out? There are quite a few but I am not sure what we typically
>> recommend here?
> 
> As I said elsewhere, I prefer breaking the fields out into separate
> properties because that makes it a lot easier to write the DT. Rather
> than having to go and manually assemble 32-bit values for this register
> and the timing registers, it must be a lot easier to look at datasheets
> and copy the values into the corresponding DT properties.

That's what I thought :-)

>>> +Note that the NOR controller does not have any internal chip-select address
>>> +decoding and if you want to access multiple devices external chip-select
>>> +decoding must be provided.
>>
>> Although it is true, you do have the MIO address space and so you could
>> support two devices via the SNOR address space and MIO address space
>> (assuming that the MIO can be used for the 2nd device).
> 
> Now I'm even more confused. If the GMI controller itself can't select a
> chip, what is the SNOR_SEL field in the SNOR_CONFIG_0 register for? Does
> that not select a specific chip?

So the GMI has 8 chip-selects and these can be used by either the SNOR
interface for MIO interface. As you mentioned the chip-select used for
the SNOR interface is configured by the field SNOR_SEL in the
SNOR_CONFIG and similarly the chip-select for the MIO interface is
configured by the MIO_SEL field in the MIO_CONFIG register.

Looking at the Tegra20 TRM, the SNOR interface is mapped to the address
range 0xd0000000-0xdfffffff and the MIO interface is mapped to the
address range 0xe0000000-0xefffffff. If chip-select 0 is used for SNOR
and chip-select 1 is used for MIO, you can support two devices at the
same time and they will be accessible via different address ranges.
Whether you can use the MIO interface for the 2nd device is another
question.

If you want to support two 256MB NOR devices on the SNOR interface, then
you would need to reconfigure the SNOR_CONFIG each time you swap between
the two devices.

>> Furthermore, if you do have external logic to support multiple devices
>> this would assume that the devices use the same timing and so are
>> probably the same type. It also assumes both can fit in the 256MB
>> address range. May be worth mentioning.
> 
> Similarly if you switch between different devices, wouldn't you have to
> reprogram the timing registers if they are different?

Yes.

> The way I remember this kind of interface to work (it's been a long time
> since I used one) is that in order to operate on a chip you need to
> acquire the bus first. Typically that would be an API exposed by the bus
> driver or some framework that the bus driver registers with. That API
> arbitrates between multiple devices on the bus and makes sure that the
> proper chip select is asserted and timing is programmed when you're
> granted access. A driver that has acquired the bus can then perform what
> operations they need and release the bus when done.
> 
> SPI uses a mechanism like this, for example.

That would make sense. However, I am not sure how that would work with
the client drivers, such as the CAN driver Mirza is using, that wishes
to read/write directly to the SNOR address space. I am guessing that SPI
works like I2C and buffers up the requests and performs them in sequence.

Jon

-- 
nvpublic

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Thierry Reding]

On Mon, Jul 25, 2016 at 03:30:34PM +0200, Mirza Krak wrote:
> 2016-07-25 13:59 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> > On Thu, Jul 21, 2016 at 10:56:44AM +0100, Jon Hunter wrote:
> >>
> >
> >> > +Note that the NOR controller does not have any internal chip-select address
> >> > +decoding and if you want to access multiple devices external chip-select
> >> > +decoding must be provided.
> >>
> >> Although it is true, you do have the MIO address space and so you could
> >> support two devices via the SNOR address space and MIO address space
> >> (assuming that the MIO can be used for the 2nd device).
> >
> > Now I'm even more confused. If the GMI controller itself can't select a
> > chip, what is the SNOR_SEL field in the SNOR_CONFIG_0 register for? Does
> > that not select a specific chip?
> >
> >> Furthermore, if you do have external logic to support multiple devices
> >> this would assume that the devices use the same timing and so are
> >> probably the same type. It also assumes both can fit in the 256MB
> >> address range. May be worth mentioning.
> >
> > Similarly if you switch between different devices, wouldn't you have to
> > reprogram the timing registers if they are different?
> >
> > The way I remember this kind of interface to work (it's been a long time
> > since I used one) is that in order to operate on a chip you need to
> > acquire the bus first. Typically that would be an API exposed by the bus
> > driver or some framework that the bus driver registers with. That API
> > arbitrates between multiple devices on the bus and makes sure that the
> > proper chip select is asserted and timing is programmed when you're
> > granted access. A driver that has acquired the bus can then perform what
> > operations they need and release the bus when done.
> >
> > SPI uses a mechanism like this, for example.
> >
> > Thierry
> 
> From my experience (maybe not as long as yours :)) but these kind of
> things would be handled by the controller. At least with previous SOCs
> that I have used, PXA270, PXA300 and i.MX SOCs.
> 
> That it has an address range per chip-select PIN and timing registers
> per chip-select. And thus eliminating a need for a infrastructure or
> framework.

Okay, so the controllers have a translation table that needs to be
programmed and which maps address ranges to chip-selects. That's a nifty
feature, but I think it's also fairly specialized. In such a setup there
doesn't need to be a concept of chip-selects in software because it's
all transparently handled by the controller. Effectively the only time a
chip-select is needed is during the initial programming of the
controller when the translation table is set up.

>From a software point of view the devices are then addressed by memory
address alone, so they aren't on a "manually switched" bus using chip-
selects.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160725/e85f0eda/attachment.sig>

[RFC 6/6] bus: Add support for Tegra NOR controller

[Thierry Reding]

On Mon, Jul 25, 2016 at 02:17:11PM +0200, Mirza Krak wrote:
> 2016-07-25 13:14 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> > On Tue, Jul 19, 2016 at 03:36:37PM +0200, Mirza Krak wrote:
[...]
> >> +
> >> +     if (of_get_child_count(of_node))
> >> +             ret = of_platform_populate(of_node,
> >> +                                of_default_bus_match_table,
> >> +                                NULL, &pdev->dev);
> >
> > Why the extra check? of_platform_populate() is almost a no-op if there
> > aren't any children anyway. What it will do is set the OF_POPULATED_BUS
> > flag, but I think we want that irrespective of whether or not there are
> > any children.
> >
> > Was there any problem with calling it unconditionally that made you opt
> > for the extra check?
> 
> I have not tested calling it unconditionally. Used another driver as
> reference and they had that condition (drivers/bus/imx-weim.c), that
> driver does not mention why the condition is there. But will test
> removing the condition and see what happens.

If that works fine for Tegra, it might be a good idea to get rid of the
call in the imx-weim.c driver, too.

> >> +static struct platform_driver nor_driver = {
> >> +     .driver = {
> >> +             .name           = "tegra-nor",
> >> +             .of_match_table = nor_id_table,
> >> +     },
> >> +};
> >> +module_platform_driver_probe(nor_driver, nor_probe);
> >> +
> >> +MODULE_AUTHOR("Mirza Krak <mirza.krak@gmail.com");
> >> +MODULE_DESCRIPTION("NVIDIA Tegra NOR Bus Driver");
> >> +MODULE_LICENSE("GPL");
> >
> > You're header comment says GPL version 2, which means that the
> > MODULE_LICENSE needs to be "GPL v2". "GPL" means "version 2 or later".
> 
> Ok, I do not really mind it being GPL version 2 or later so will
> change the header comment instead if that is ok.

I think "GPL v2" is traditionally more common in kernel code, but as the
author the decision is obviously yours.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160725/daba8235/attachment.sig>

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Thierry Reding]

On Mon, Jul 25, 2016 at 02:36:35PM +0100, Jon Hunter wrote:
> 
> On 25/07/16 12:59, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Thu, Jul 21, 2016 at 10:56:44AM +0100, Jon Hunter wrote:
> >>
> >> On 19/07/16 14:36, Mirza Krak wrote:
> >>> From: Mirza Krak <mirza.krak@gmail.com>
> >>>
> >>> Document the devicetree bindings for NOR bus driver found on Tegra20 and
> >>> Tegra30 SOCs
> >>>
> >>> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
> >>> ---
> >>>  .../devicetree/bindings/bus/nvidia,tegra20-nor.txt | 73 ++++++++++++++++++++++
> >>>  1 file changed, 73 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> >>> new file mode 100644
> >>> index 0000000..9ee4a66
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-nor.txt
> >>> @@ -0,0 +1,73 @@
> >>> +Device tree bindings for NVIDIA Tegra20/30 NOR Bus
> >>> +
> >>> +The NOR controller supports a number of memory types, including synchronous NOR,
> >>> +asynchronous NOR, and other flash memories with similar interfaces, such as
> >>> +MuxOneNAND. One could also connect high speed devices like FPGAs, DSPs,
> >>> +CAN chips, Wi-Fi chips etc.
> >>
> >> Nit-pick ... the Tegra documentation refers to this controller as the
> >> GMI (general memory interface) or SNOR (sync-NOR) controller because it
> >> is not just limited to NOR as you mentioned. I see references to GMI in
> >> the Tegra pinctrl driver and so may be we should use this name.
> >>
> >>> +
> >>> +The actual devices are instantiated from the child nodes of a NOR node.
> >>> +
> >>> +Required properties:
> >>> +
> >>> + - compatible: should be "nvidia,tegra20-nor", "nvidia,tegra30-nor"
> >>
> >> I see at least one difference at the register level between Tegra20 and
> >> Tegra30 and so I think this should be something like ...
> >>
> >>  - compatible : Should contain one of the following:
> >> 	For Tegra20 must contain "nvidia,tegra20-gmi".
> >>  	For Tegra30 must contain "nvidia,tegra30-gmi".
> >>
> >>> + - reg: Should contain NOR controller registers location and length.
> >>> + - clocks: Must contain one entry, for the module clock.
> >>> +   See ../clocks/clock-bindings.txt for details.
> >>> + - resets : Must contain an entry for each entry in reset-names.
> >>> +   See ../reset/reset.txt for details.
> >>> + - reset-names : Must include the following entries:
> >>> +  - nor
> >>> + - #address-cells: Must be set to 2 to allow memory address translation
> >>> + - #size-cells:	Must be set to 1 to allow CS address passing
> >>> + - ranges: Must be set up to reflect the memory layout with four integer
> >>> + 		values for each chip-select line in use.
> >>> + - nvidia,config: This property represents the SNOR_CONFIG_0 register.
> >>
> >> There is also a SNOR_MIO_CONFIG for the MIO address space and so I think
> >> that this should be nvidia,snor-config to be explicit. It might be nice
> >> to also add a "nvidia,mio-config" while you are at it as well, however,
> >> that could always be done later. If you do, then the
> >> "nvidia,snor-config" becomes optional depending on whether you are using
> >> the SNOR or MIO address space.
> >>
> >> Thierry, Stephen, do prefer all the fields on the config registers are
> >> broken out? There are quite a few but I am not sure what we typically
> >> recommend here?
> > 
> > As I said elsewhere, I prefer breaking the fields out into separate
> > properties because that makes it a lot easier to write the DT. Rather
> > than having to go and manually assemble 32-bit values for this register
> > and the timing registers, it must be a lot easier to look at datasheets
> > and copy the values into the corresponding DT properties.
> 
> That's what I thought :-)
> 
> >>> +Note that the NOR controller does not have any internal chip-select address
> >>> +decoding and if you want to access multiple devices external chip-select
> >>> +decoding must be provided.
> >>
> >> Although it is true, you do have the MIO address space and so you could
> >> support two devices via the SNOR address space and MIO address space
> >> (assuming that the MIO can be used for the 2nd device).
> > 
> > Now I'm even more confused. If the GMI controller itself can't select a
> > chip, what is the SNOR_SEL field in the SNOR_CONFIG_0 register for? Does
> > that not select a specific chip?
> 
> So the GMI has 8 chip-selects and these can be used by either the SNOR
> interface for MIO interface. As you mentioned the chip-select used for
> the SNOR interface is configured by the field SNOR_SEL in the
> SNOR_CONFIG and similarly the chip-select for the MIO interface is
> configured by the MIO_SEL field in the MIO_CONFIG register.
> 
> Looking at the Tegra20 TRM, the SNOR interface is mapped to the address
> range 0xd0000000-0xdfffffff and the MIO interface is mapped to the
> address range 0xe0000000-0xefffffff. If chip-select 0 is used for SNOR
> and chip-select 1 is used for MIO, you can support two devices at the
> same time and they will be accessible via different address ranges.
> Whether you can use the MIO interface for the 2nd device is another
> question.
> 
> If you want to support two 256MB NOR devices on the SNOR interface, then
> you would need to reconfigure the SNOR_CONFIG each time you swap between
> the two devices.
> 
> >> Furthermore, if you do have external logic to support multiple devices
> >> this would assume that the devices use the same timing and so are
> >> probably the same type. It also assumes both can fit in the 256MB
> >> address range. May be worth mentioning.
> > 
> > Similarly if you switch between different devices, wouldn't you have to
> > reprogram the timing registers if they are different?
> 
> Yes.

Okay, good. I'm relieved that I wasn't completely mistaken about how
this kind of hardware works. =)

> > The way I remember this kind of interface to work (it's been a long time
> > since I used one) is that in order to operate on a chip you need to
> > acquire the bus first. Typically that would be an API exposed by the bus
> > driver or some framework that the bus driver registers with. That API
> > arbitrates between multiple devices on the bus and makes sure that the
> > proper chip select is asserted and timing is programmed when you're
> > granted access. A driver that has acquired the bus can then perform what
> > operations they need and release the bus when done.
> > 
> > SPI uses a mechanism like this, for example.
> 
> That would make sense. However, I am not sure how that would work with
> the client drivers, such as the CAN driver Mirza is using, that wishes
> to read/write directly to the SNOR address space. I am guessing that SPI
> works like I2C and buffers up the requests and performs them in sequence.

Yes, you would of course have to guard accesses by the drivers with
calls to an API that acquire and release the bus. Not that it's very
nice, but in the absence of automatic address to CS translation that
really is the only way to solve it (explicit chip-select).

It doesn't seem like we currently have infrastructure for it, but we
would need something like that to solve this in the general case.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160725/81966d43/attachment.sig>

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Mirza Krak]

2016-07-25 15:39 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> On Mon, Jul 25, 2016 at 03:30:34PM +0200, Mirza Krak wrote:
>> 2016-07-25 13:59 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
>> > On Thu, Jul 21, 2016 at 10:56:44AM +0100, Jon Hunter wrote:
>> >>
>> >
>> >> > +Note that the NOR controller does not have any internal chip-select address
>> >> > +decoding and if you want to access multiple devices external chip-select
>> >> > +decoding must be provided.
>> >>
>> >> Although it is true, you do have the MIO address space and so you could
>> >> support two devices via the SNOR address space and MIO address space
>> >> (assuming that the MIO can be used for the 2nd device).
>> >
>> > Now I'm even more confused. If the GMI controller itself can't select a
>> > chip, what is the SNOR_SEL field in the SNOR_CONFIG_0 register for? Does
>> > that not select a specific chip?
>> >
>> >> Furthermore, if you do have external logic to support multiple devices
>> >> this would assume that the devices use the same timing and so are
>> >> probably the same type. It also assumes both can fit in the 256MB
>> >> address range. May be worth mentioning.
>> >
>> > Similarly if you switch between different devices, wouldn't you have to
>> > reprogram the timing registers if they are different?
>> >
>> > The way I remember this kind of interface to work (it's been a long time
>> > since I used one) is that in order to operate on a chip you need to
>> > acquire the bus first. Typically that would be an API exposed by the bus
>> > driver or some framework that the bus driver registers with. That API
>> > arbitrates between multiple devices on the bus and makes sure that the
>> > proper chip select is asserted and timing is programmed when you're
>> > granted access. A driver that has acquired the bus can then perform what
>> > operations they need and release the bus when done.
>> >
>> > SPI uses a mechanism like this, for example.
>> >
>> > Thierry
>>
>> From my experience (maybe not as long as yours :)) but these kind of
>> things would be handled by the controller. At least with previous SOCs
>> that I have used, PXA270, PXA300 and i.MX SOCs.
>>
>> That it has an address range per chip-select PIN and timing registers
>> per chip-select. And thus eliminating a need for a infrastructure or
>> framework.
>
> Okay, so the controllers have a translation table that needs to be
> programmed and which maps address ranges to chip-selects. That's a nifty
> feature, but I think it's also fairly specialized. In such a setup there
> doesn't need to be a concept of chip-selects in software because it's
> all transparently handled by the controller. Effectively the only time a
> chip-select is needed is during the initial programming of the
> controller when the translation table is set up.

Yes, and in some cases you can not "program" the map as it is fixed in hardware.

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Thierry Reding]

On Mon, Jul 25, 2016 at 03:16:28PM +0200, Mirza Krak wrote:
> 2016-07-25 13:30 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
[...]
> > The above suggests that one of the CAN controllers gets mapped to an
> > address 0x48000000 and the other gets mapped to 0x48040000. But why do
> > we even need a chip-select at all in that case? If both chips don't use
> > any overlapping memory region, what good does the chip-select do?
> 
> If we take a look on similar controllers found on others SOCs they
> usually define an address range / chip-select.
> 
> Example (weim):
> ranges = <
> 0 0 0x10000000 0x02000000
> 1 0 0x12000000 0x01000000
> 2 0 0x13000000 0x01000000
> 3 0 0x14000000 0x01000000
> 4 0 0x15000000 0x01000000
> 5 0 0x16000000 0x01000000
> >;
> 
> Which means that you all ready have an address mapped to PIN function.
> 
> But Tegra GMI controller is a first for me, where you do not have this
> kind of setup in hardware. Usually you also have a timing register /
> chip-select so that you can connect different chip types.
> 
> The lack of hardware support do decode an address to a chip-select PIN
> function, we have implemented this our self externally.
> 
> We actually have 6 different chips connected to the GMI bus and the
> "ranges" would be:
>   ranges = <
>    0 0 0x48000000 0x00000100
>    1 0 0x48040000 0x00000100
>    2 0 0x48080000 0x00000100
>    3 0 0x480A0000 0x00000100
>    4 0 0x480C0000 0x00000100
>    5 0 0x480E0000 0x00000100
>   >;
> 
> And this not nothing complicated, small number of AND gates and that is it.
> 
> The chip-select signal is necessary for the access characteristics, so
> we need to translate an address to an chip-select so that the chip
> knows the host CPU wants to talk to it.
> 
> Do not know if I made anything more clear here :).

Yes, that clarifies many things. The presence of an external, address-
based chip-select is essential information in order to describe this
setup properly.

Given that the external chip select is entirely invisible to software, I
think a more accurate description of your setup would be:

	gmi at 70090000 {
		...

		/* for the chip select */
		#address-cells = <1>;
		#size-cells = <0>;

		/*
		 * Technically this could be used to translate the range from
		 * 0x48000000 to 0x4fffffff into a different range, but that
		 * no longer works because of the #address-cells. Does this
		 * matter?
		 */
		ranges;

		bus at 0 {
			compatible = "simple-bus";
			reg = <0>;

			#address-cells = <1>;
			#size-cells = <1>;

			can at 48000000 {
				reg = <0x48000000 0x100>;
				...
			};

			can at 48040000 {
				reg = <0x48040000 0x100>;
				...
			};
		};
	};

That omits any reference to the external chip select, which I think
makes sense because it's something that software is completely unaware
of.

Perhaps one important question: does your setup use the GMI's CS lines
in any way? Or does it simply get ignored?

If it gets ignored, I suppose one could encode this as a special case:

	gmi at 70090000 {
		...

		/* simple address translation */
		#address-cells = <1>;
		#size-cells = <1>;

		ranges = <0x48000000 0x48000000 0x08000000>;

		can at 48000000 {
			...
			reg = <0x48000000 0x100>;
			...
		};

		can at 48040000 {
			...
			reg = <0x48000000 0x100>;
			...
		};
	};

We could use that special case in order to make the driver behave in a
special way that suits your setup, namely without explicit chip-select.
The case where #size-cells = <0> could be treated as the explicit chip-
select case, that somebody could implement in the future if they need
it. All other cases could be treated as meaning that the chip-select is
automatically handled externally (like in your setup) and we simply
statically configure the controller with chip select 0 (kind of in the
way your current patch series does).

Rob, what do you think about the above?

> > Also, it seems to me that you'd have to program the SNOR_CONFIG_0
> > register in order to select a specific chip, but I don't see anything in
> > the driver access that register after the initial write of the register.
> 
> This is only setup at probe.
> 
> >
> > I would've expected this to require some sort of infrastructure to allow
> > devices connected to the GMI controller to acquire the bus via some API
> > to select their chip.
> 
> Yes, ultimately you would need some sort of infrastructure to allow
> devices to acquire the GMI bus if you want to solve this in software.
> But at the moment I do not see such an infrastructure in place, and is
> it feasible to add one specifically for the GMI controller? If one
> such infrastructure was in place we would need to modify all the
> drivers that want to use to include Tegra specific infrastructure to
> access the GMI bus?
> 
> Since my knowledge is limited it hard for me to comment on this, maybe
> there is a simple way of doing this?

I don't think there's a simple way to do this. In order to properly
implement it we'd need to implement a generic infrastructure for chip
selects so that drivers such as the one for your CAN controller can be
written without tying them specifically to the Tegra GMI controller.

>From what you and Jon were saying it sounds like the drivers are
completely agnostic of any chip-select, so conversion won't be easy.
But technically if these chips take a chip-select as input then it's
always possible to hook them up to a controller that doesn't do this
automatic translation of address to chip-select, so eventually some
setup is bound to come along where they'd need explicit chip-select
handling as well.

I don't think it's fair to require you to implement this infrastructure
if you don't actually need it. At the same time I want to be cautious
and make sure we keep the driver and binding flexible enough to allow
us to implement explicit chip-selects should we later need them.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160725/374df929/attachment.sig>

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Mirza Krak]

2016-07-25 16:15 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> On Mon, Jul 25, 2016 at 03:16:28PM +0200, Mirza Krak wrote:
>> 2016-07-25 13:30 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> [...]
>> > The above suggests that one of the CAN controllers gets mapped to an
>> > address 0x48000000 and the other gets mapped to 0x48040000. But why do
>> > we even need a chip-select at all in that case? If both chips don't use
>> > any overlapping memory region, what good does the chip-select do?
>>
>> If we take a look on similar controllers found on others SOCs they
>> usually define an address range / chip-select.
>>
>> Example (weim):
>> ranges = <
>> 0 0 0x10000000 0x02000000
>> 1 0 0x12000000 0x01000000
>> 2 0 0x13000000 0x01000000
>> 3 0 0x14000000 0x01000000
>> 4 0 0x15000000 0x01000000
>> 5 0 0x16000000 0x01000000
>> >;
>>
>> Which means that you all ready have an address mapped to PIN function.
>>
>> But Tegra GMI controller is a first for me, where you do not have this
>> kind of setup in hardware. Usually you also have a timing register /
>> chip-select so that you can connect different chip types.
>>
>> The lack of hardware support do decode an address to a chip-select PIN
>> function, we have implemented this our self externally.
>>
>> We actually have 6 different chips connected to the GMI bus and the
>> "ranges" would be:
>>   ranges = <
>>    0 0 0x48000000 0x00000100
>>    1 0 0x48040000 0x00000100
>>    2 0 0x48080000 0x00000100
>>    3 0 0x480A0000 0x00000100
>>    4 0 0x480C0000 0x00000100
>>    5 0 0x480E0000 0x00000100
>>   >;
>>
>> And this not nothing complicated, small number of AND gates and that is it.
>>
>> The chip-select signal is necessary for the access characteristics, so
>> we need to translate an address to an chip-select so that the chip
>> knows the host CPU wants to talk to it.
>>
>> Do not know if I made anything more clear here :).
>
> Yes, that clarifies many things. The presence of an external, address-
> based chip-select is essential information in order to describe this
> setup properly.
>
> Given that the external chip select is entirely invisible to software, I
> think a more accurate description of your setup would be:
>
>         gmi at 70090000 {
>                 ...
>
>                 /* for the chip select */
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 /*
>                  * Technically this could be used to translate the range from
>                  * 0x48000000 to 0x4fffffff into a different range, but that
>                  * no longer works because of the #address-cells. Does this
>                  * matter?
>                  */
>                 ranges;
>
>                 bus at 0 {
>                         compatible = "simple-bus";
>                         reg = <0>;
>
>                         #address-cells = <1>;
>                         #size-cells = <1>;
>
>                         can at 48000000 {
>                                 reg = <0x48000000 0x100>;
>                                 ...
>                         };
>
>                         can at 48040000 {
>                                 reg = <0x48040000 0x100>;
>                                 ...
>                         };
>                 };
>         };
>
> That omits any reference to the external chip select, which I think
> makes sense because it's something that software is completely unaware
> of.
>
> Perhaps one important question: does your setup use the GMI's CS lines
> in any way? Or does it simply get ignored?

Yes, we use the GMI`s CS line. It is important that is present because
it is ANDED with address lines and NOT ALE line. Especially since we
run the GMI controller in AD_MUX mode, which means that we use same
pins for address and data and the access happens in two phases, one
address latch phase where CS is not asserted but ALE is, second
read/write phase where CS must be asserted. In this case we need the
GMI`s CS line to determine which phase we are in.

Will give above a test run.

>
> If it gets ignored, I suppose one could encode this as a special case:
>
>         gmi at 70090000 {
>                 ...
>
>                 /* simple address translation */
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>
>                 ranges = <0x48000000 0x48000000 0x08000000>;
>
>                 can at 48000000 {
>                         ...
>                         reg = <0x48000000 0x100>;
>                         ...
>                 };
>
>                 can at 48040000 {
>                         ...
>                         reg = <0x48000000 0x100>;
>                         ...
>                 };
>         };
>
> We could use that special case in order to make the driver behave in a
> special way that suits your setup, namely without explicit chip-select.
> The case where #size-cells = <0> could be treated as the explicit chip-
> select case, that somebody could implement in the future if they need
> it. All other cases could be treated as meaning that the chip-select is
> automatically handled externally (like in your setup) and we simply
> statically configure the controller with chip select 0 (kind of in the
> way your current patch series does).
>
> Rob, what do you think about the above?
>
>> > Also, it seems to me that you'd have to program the SNOR_CONFIG_0
>> > register in order to select a specific chip, but I don't see anything in
>> > the driver access that register after the initial write of the register.
>>
>> This is only setup at probe.
>>
>> >
>> > I would've expected this to require some sort of infrastructure to allow
>> > devices connected to the GMI controller to acquire the bus via some API
>> > to select their chip.
>>
>> Yes, ultimately you would need some sort of infrastructure to allow
>> devices to acquire the GMI bus if you want to solve this in software.
>> But at the moment I do not see such an infrastructure in place, and is
>> it feasible to add one specifically for the GMI controller? If one
>> such infrastructure was in place we would need to modify all the
>> drivers that want to use to include Tegra specific infrastructure to
>> access the GMI bus?
>>
>> Since my knowledge is limited it hard for me to comment on this, maybe
>> there is a simple way of doing this?
>
> I don't think there's a simple way to do this. In order to properly
> implement it we'd need to implement a generic infrastructure for chip
> selects so that drivers such as the one for your CAN controller can be
> written without tying them specifically to the Tegra GMI controller.
>
> From what you and Jon were saying it sounds like the drivers are
> completely agnostic of any chip-select, so conversion won't be easy.
> But technically if these chips take a chip-select as input then it's
> always possible to hook them up to a controller that doesn't do this
> automatic translation of address to chip-select, so eventually some
> setup is bound to come along where they'd need explicit chip-select
> handling as well.
>
> I don't think it's fair to require you to implement this infrastructure
> if you don't actually need it. At the same time I want to be cautious
> and make sure we keep the driver and binding flexible enough to allow
> us to implement explicit chip-selects should we later need them.
>
> Thierry

I understand, and thank you for not requiring me to implement such a
infrastructure :)

And thank you for your feedback.

Best Regards,
Mirza

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Jon Hunter]

On 25/07/16 15:38, Mirza Krak wrote:
> 2016-07-25 16:15 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
>> On Mon, Jul 25, 2016 at 03:16:28PM +0200, Mirza Krak wrote:
>>> 2016-07-25 13:30 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
>> [...]
>>>> The above suggests that one of the CAN controllers gets mapped to an
>>>> address 0x48000000 and the other gets mapped to 0x48040000. But why do
>>>> we even need a chip-select at all in that case? If both chips don't use
>>>> any overlapping memory region, what good does the chip-select do?
>>>
>>> If we take a look on similar controllers found on others SOCs they
>>> usually define an address range / chip-select.
>>>
>>> Example (weim):
>>> ranges = <
>>> 0 0 0x10000000 0x02000000
>>> 1 0 0x12000000 0x01000000
>>> 2 0 0x13000000 0x01000000
>>> 3 0 0x14000000 0x01000000
>>> 4 0 0x15000000 0x01000000
>>> 5 0 0x16000000 0x01000000
>>>> ;
>>>
>>> Which means that you all ready have an address mapped to PIN function.
>>>
>>> But Tegra GMI controller is a first for me, where you do not have this
>>> kind of setup in hardware. Usually you also have a timing register /
>>> chip-select so that you can connect different chip types.
>>>
>>> The lack of hardware support do decode an address to a chip-select PIN
>>> function, we have implemented this our self externally.
>>>
>>> We actually have 6 different chips connected to the GMI bus and the
>>> "ranges" would be:
>>>   ranges = <
>>>    0 0 0x48000000 0x00000100
>>>    1 0 0x48040000 0x00000100
>>>    2 0 0x48080000 0x00000100
>>>    3 0 0x480A0000 0x00000100
>>>    4 0 0x480C0000 0x00000100
>>>    5 0 0x480E0000 0x00000100
>>>   >;
>>>
>>> And this not nothing complicated, small number of AND gates and that is it.
>>>
>>> The chip-select signal is necessary for the access characteristics, so
>>> we need to translate an address to an chip-select so that the chip
>>> knows the host CPU wants to talk to it.
>>>
>>> Do not know if I made anything more clear here :).
>>
>> Yes, that clarifies many things. The presence of an external, address-
>> based chip-select is essential information in order to describe this
>> setup properly.
>>
>> Given that the external chip select is entirely invisible to software, I
>> think a more accurate description of your setup would be:
>>
>>         gmi at 70090000 {
>>                 ...
>>
>>                 /* for the chip select */
>>                 #address-cells = <1>;
>>                 #size-cells = <0>;
>>
>>                 /*
>>                  * Technically this could be used to translate the range from
>>                  * 0x48000000 to 0x4fffffff into a different range, but that
>>                  * no longer works because of the #address-cells. Does this
>>                  * matter?
>>                  */
>>                 ranges;
>>
>>                 bus at 0 {
>>                         compatible = "simple-bus";
>>                         reg = <0>;
>>
>>                         #address-cells = <1>;
>>                         #size-cells = <1>;
>>
>>                         can at 48000000 {
>>                                 reg = <0x48000000 0x100>;
>>                                 ...
>>                         };
>>
>>                         can at 48040000 {
>>                                 reg = <0x48040000 0x100>;
>>                                 ...
>>                         };
>>                 };
>>         };
>>
>> That omits any reference to the external chip select, which I think
>> makes sense because it's something that software is completely unaware
>> of.
>>
>> Perhaps one important question: does your setup use the GMI's CS lines
>> in any way? Or does it simply get ignored?
> 
> Yes, we use the GMI`s CS line. It is important that is present because
> it is ANDED with address lines and NOT ALE line. Especially since we
> run the GMI controller in AD_MUX mode, which means that we use same
> pins for address and data and the access happens in two phases, one
> address latch phase where CS is not asserted but ALE is, second
> read/write phase where CS must be asserted. In this case we need the
> GMI`s CS line to determine which phase we are in.

Even though the CS is used, we could still implement the driver such
that if only 1 CS is defined in the binding, we then statically program
the configuration at probe. For now, if there is more than one, we can
return an error from probe as it is not supported or default to the
first CS defined and warn?

Jon

-- 
nvpublic

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Thierry Reding]

On Mon, Jul 25, 2016 at 04:01:49PM +0100, Jon Hunter wrote:
> 
> On 25/07/16 15:38, Mirza Krak wrote:
> > 2016-07-25 16:15 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> >> On Mon, Jul 25, 2016 at 03:16:28PM +0200, Mirza Krak wrote:
> >>> 2016-07-25 13:30 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> >> [...]
> >>>> The above suggests that one of the CAN controllers gets mapped to an
> >>>> address 0x48000000 and the other gets mapped to 0x48040000. But why do
> >>>> we even need a chip-select at all in that case? If both chips don't use
> >>>> any overlapping memory region, what good does the chip-select do?
> >>>
> >>> If we take a look on similar controllers found on others SOCs they
> >>> usually define an address range / chip-select.
> >>>
> >>> Example (weim):
> >>> ranges = <
> >>> 0 0 0x10000000 0x02000000
> >>> 1 0 0x12000000 0x01000000
> >>> 2 0 0x13000000 0x01000000
> >>> 3 0 0x14000000 0x01000000
> >>> 4 0 0x15000000 0x01000000
> >>> 5 0 0x16000000 0x01000000
> >>>> ;
> >>>
> >>> Which means that you all ready have an address mapped to PIN function.
> >>>
> >>> But Tegra GMI controller is a first for me, where you do not have this
> >>> kind of setup in hardware. Usually you also have a timing register /
> >>> chip-select so that you can connect different chip types.
> >>>
> >>> The lack of hardware support do decode an address to a chip-select PIN
> >>> function, we have implemented this our self externally.
> >>>
> >>> We actually have 6 different chips connected to the GMI bus and the
> >>> "ranges" would be:
> >>>   ranges = <
> >>>    0 0 0x48000000 0x00000100
> >>>    1 0 0x48040000 0x00000100
> >>>    2 0 0x48080000 0x00000100
> >>>    3 0 0x480A0000 0x00000100
> >>>    4 0 0x480C0000 0x00000100
> >>>    5 0 0x480E0000 0x00000100
> >>>   >;
> >>>
> >>> And this not nothing complicated, small number of AND gates and that is it.
> >>>
> >>> The chip-select signal is necessary for the access characteristics, so
> >>> we need to translate an address to an chip-select so that the chip
> >>> knows the host CPU wants to talk to it.
> >>>
> >>> Do not know if I made anything more clear here :).
> >>
> >> Yes, that clarifies many things. The presence of an external, address-
> >> based chip-select is essential information in order to describe this
> >> setup properly.
> >>
> >> Given that the external chip select is entirely invisible to software, I
> >> think a more accurate description of your setup would be:
> >>
> >>         gmi at 70090000 {
> >>                 ...
> >>
> >>                 /* for the chip select */
> >>                 #address-cells = <1>;
> >>                 #size-cells = <0>;
> >>
> >>                 /*
> >>                  * Technically this could be used to translate the range from
> >>                  * 0x48000000 to 0x4fffffff into a different range, but that
> >>                  * no longer works because of the #address-cells. Does this
> >>                  * matter?
> >>                  */
> >>                 ranges;
> >>
> >>                 bus at 0 {
> >>                         compatible = "simple-bus";
> >>                         reg = <0>;
> >>
> >>                         #address-cells = <1>;
> >>                         #size-cells = <1>;
> >>
> >>                         can at 48000000 {
> >>                                 reg = <0x48000000 0x100>;
> >>                                 ...
> >>                         };
> >>
> >>                         can at 48040000 {
> >>                                 reg = <0x48040000 0x100>;
> >>                                 ...
> >>                         };
> >>                 };
> >>         };
> >>
> >> That omits any reference to the external chip select, which I think
> >> makes sense because it's something that software is completely unaware
> >> of.
> >>
> >> Perhaps one important question: does your setup use the GMI's CS lines
> >> in any way? Or does it simply get ignored?
> > 
> > Yes, we use the GMI`s CS line. It is important that is present because
> > it is ANDED with address lines and NOT ALE line. Especially since we
> > run the GMI controller in AD_MUX mode, which means that we use same
> > pins for address and data and the access happens in two phases, one
> > address latch phase where CS is not asserted but ALE is, second
> > read/write phase where CS must be asserted. In this case we need the
> > GMI`s CS line to determine which phase we are in.
> 
> Even though the CS is used, we could still implement the driver such
> that if only 1 CS is defined in the binding, we then statically program
> the configuration at probe. For now, if there is more than one, we can
> return an error from probe as it is not supported or default to the
> first CS defined and warn?

I think either would work. Possibly better to do the latter because at
least some devices will work that way.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160725/eabe4ac1/attachment.sig>

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Mirza Krak]

2016-07-25 16:15 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> On Mon, Jul 25, 2016 at 03:16:28PM +0200, Mirza Krak wrote:
>> 2016-07-25 13:30 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
>> >
>> > I would've expected this to require some sort of infrastructure to allow
>> > devices connected to the GMI controller to acquire the bus via some API
>> > to select their chip.
>>
>> Yes, ultimately you would need some sort of infrastructure to allow
>> devices to acquire the GMI bus if you want to solve this in software.
>> But at the moment I do not see such an infrastructure in place, and is
>> it feasible to add one specifically for the GMI controller? If one
>> such infrastructure was in place we would need to modify all the
>> drivers that want to use to include Tegra specific infrastructure to
>> access the GMI bus?
>>
>> Since my knowledge is limited it hard for me to comment on this, maybe
>> there is a simple way of doing this?
>
> I don't think there's a simple way to do this. In order to properly
> implement it we'd need to implement a generic infrastructure for chip
> selects so that drivers such as the one for your CAN controller can be
> written without tying them specifically to the Tegra GMI controller.
>
> From what you and Jon were saying it sounds like the drivers are
> completely agnostic of any chip-select, so conversion won't be easy.
> But technically if these chips take a chip-select as input then it's
> always possible to hook them up to a controller that doesn't do this
> automatic translation of address to chip-select, so eventually some
> setup is bound to come along where they'd need explicit chip-select
> handling as well.
>
> I don't think it's fair to require you to implement this infrastructure
> if you don't actually need it. At the same time I want to be cautious
> and make sure we keep the driver and binding flexible enough to allow
> us to implement explicit chip-selects should we later need them.
>
> Thierry

One thing that should be noted, and that is the GMI controller also
supports a DMA master mode (feature for the future?).

I do not really know how this effects the binding we are discussing
but wanted to put it out there.

Best Regards,
Mirza

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Thierry Reding]

On Mon, Jul 25, 2016 at 09:59:34PM +0200, Mirza Krak wrote:
> 2016-07-25 16:15 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> > On Mon, Jul 25, 2016 at 03:16:28PM +0200, Mirza Krak wrote:
> >> 2016-07-25 13:30 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> >> >
> >> > I would've expected this to require some sort of infrastructure to allow
> >> > devices connected to the GMI controller to acquire the bus via some API
> >> > to select their chip.
> >>
> >> Yes, ultimately you would need some sort of infrastructure to allow
> >> devices to acquire the GMI bus if you want to solve this in software.
> >> But at the moment I do not see such an infrastructure in place, and is
> >> it feasible to add one specifically for the GMI controller? If one
> >> such infrastructure was in place we would need to modify all the
> >> drivers that want to use to include Tegra specific infrastructure to
> >> access the GMI bus?
> >>
> >> Since my knowledge is limited it hard for me to comment on this, maybe
> >> there is a simple way of doing this?
> >
> > I don't think there's a simple way to do this. In order to properly
> > implement it we'd need to implement a generic infrastructure for chip
> > selects so that drivers such as the one for your CAN controller can be
> > written without tying them specifically to the Tegra GMI controller.
> >
> > From what you and Jon were saying it sounds like the drivers are
> > completely agnostic of any chip-select, so conversion won't be easy.
> > But technically if these chips take a chip-select as input then it's
> > always possible to hook them up to a controller that doesn't do this
> > automatic translation of address to chip-select, so eventually some
> > setup is bound to come along where they'd need explicit chip-select
> > handling as well.
> >
> > I don't think it's fair to require you to implement this infrastructure
> > if you don't actually need it. At the same time I want to be cautious
> > and make sure we keep the driver and binding flexible enough to allow
> > us to implement explicit chip-selects should we later need them.
> >
> > Thierry
> 
> One thing that should be noted, and that is the GMI controller also
> supports a DMA master mode (feature for the future?).
> 
> I do not really know how this effects the binding we are discussing
> but wanted to put it out there.

Yes, that had occurred to me as well. I don't really have any good ideas
on how to use that other than to implement it as a DMA engine driver and
have drivers use those, if available.

But I don't think we have to worry about it right now. If we ever need
it, the binding can be extended in a backwards-compatible way.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160726/3bab8918/attachment.sig>

[RFC 3/6] dt/bindings: Add bindings for Tegra20/30 NOR bus driver

[Mirza Krak]

2016-07-25 16:15 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> On Mon, Jul 25, 2016 at 03:16:28PM +0200, Mirza Krak wrote:
>> 2016-07-25 13:30 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> Yes, that clarifies many things. The presence of an external, address-
> based chip-select is essential information in order to describe this
> setup properly.
>
> Given that the external chip select is entirely invisible to software, I
> think a more accurate description of your setup would be:
>
>         gmi at 70090000 {
>                 ...
>
>                 /* for the chip select */
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 /*
>                  * Technically this could be used to translate the range from
>                  * 0x48000000 to 0x4fffffff into a different range, but that
>                  * no longer works because of the #address-cells. Does this
>                  * matter?
>                  */
>                 ranges;
>
>                 bus at 0 {
>                         compatible = "simple-bus";
>                         reg = <0>;
>
>                         #address-cells = <1>;
>                         #size-cells = <1>;
>
>                         can at 48000000 {
>                                 reg = <0x48000000 0x100>;
>                                 ...
>                         };
>
>                         can at 48040000 {
>                                 reg = <0x48040000 0x100>;
>                                 ...
>                         };
>                 };
>         };
>

Finally got around to test this. Above example had some issues, or I
am doing something wrong.

First of, the address parser does not seem to like that #size-cells =
<0> when ranges are empty. Got following warning from device tree
compiler:
Warning (ranges_format): /nor at 70009000 has empty "ranges" property but
its #size-cells (0) differs from / (1)

and on boot:
[    0.399357] prom_parse: Bad cell count for /nor at 70009000/bus at 0

Got it to work if I changed to (also had to add an empty ranges prop
in bus node):

gmi at 70009000 {
    #address-cells = <1>;
    #size-cells = <1>;
    ranges;

    bus at 0,0 {
                compatible = "simple-bus";
                reg = <0 0>;
                ranges;

                #address-cells = <1>;
                #size-cells = <1>;

                can at 48000000 {
                    reg = <0x48000000 0x100>;
                    ...
                };


                can at 48040000 {
                    reg = <0x48040000 0x100>;
                    ...
                };
}

But I wonder is there something wrong with below example (which does
work), that is omitting the bus node:

gmi at 70009000 {
    #address-cells = <1>;
    #size-cells = <1>;
    ranges;

    can at 48000000 {
        reg = <0x48000000 0x100>;
        ...
    };

    can at 48040000 {
        reg = <0x48040000 0x100>;
        ...
    };
}

Just feel that I need to duplicate information if add an bus node.

Best Regards,
Mirza