[PATCH 0/6] Add support for Tegra GMI bus controller

[PATCH 0/6] Add support for Tegra GMI bus controller

[Mirza Krak]

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

Hi.

This is a follow up to my previous RFC. Take a look at [1], [2] and [3] for
previous discussions.

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 the
GMI bus.

[1]. https://marc.info/?l=linux-clk&m=146893557629903&w=2
[2]. https://marc.info/?l=linux-tegra&m=146893541829801&w=2
[3]. https://marc.info/?l=linux-tegra&m=146893542429814&w=2

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 Tegra GMI controller
  ARM: tegra: Add Tegra30 GMI support
  ARM: tegra: Add Tegra20 GMI support
  bus: Add support for Tegra Generic Memory Interface

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

--
2.1.4

[PATCH 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 default rate to 92 MHz which
is max 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..13d3b5a 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, 92000000, 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

[PATCH 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 default rate to 127 MHz which
is max 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 8e2db5e..67f1677 100644
--- a/drivers/clk/tegra/clk-tegra30.c
+++ b/drivers/clk/tegra/clk-tegra30.c
@@ -1252,6 +1252,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, 127000000, 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

[PATCH 3/6] dt/bindings: Add bindings for Tegra GMI controller

[Mirza Krak]

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

Document the devicetree bindings for the Generic Memory Interface (GMI)
bus driver found on Tegra SOCs.

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

diff --git a/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt b/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt
new file mode 100644
index 0000000..046846e
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt
@@ -0,0 +1,99 @@
+Device tree bindings for NVIDIA Tegra Generic Memory Interface bus
+
+The Generic Memory Interface bus enables memory transfers between internal and
+external memory. Can be used to attach various high speed devices such as
+synchronous/asynchronous NOR, FPGA, UARTS and more.
+
+The actual devices are instantiated from the child nodes of a GMI node.
+
+Required properties:
+  - compatible : Should contain one of the following:
+        For Tegra20 must contain "nvidia,tegra20-gmi".
+        For Tegra30 must contain "nvidia,tegra30-gmi".
+ - reg: Should contain GMI controller registers location and length.
+ - clocks: Must contain an entry for each entry in clock-names.
+ - clock-names: Must include the following entries: "gmi"
+ - resets : Must contain an entry for each entry in reset-names.
+ - reset-names : Must include the following entries: "gmi"
+ - #address-cells: The number of cells used to represent physical base
+   addresses in the GMI address space.
+ - #size-cells: The number of cells used to represent the size of an address
+   range in the GMI address space.
+ - ranges: Mapping of the GMI address space to the CPU address space.
+
+Note that the GMI 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. 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 that they can fit in
+the 256MB address range.
+
+Optional properties:
+
+ - nvidia,snor-data-width-32bit: Use 32bit data-bus, default is 16bit.
+ - nvidia,snor-mux-mode: Enable address/data MUX mode.
+ - nvidia,snor-rdy-active-before-data: Assert RDY signal one cycle before data.
+   If omitted it will be asserted with data.
+ - nvidia,snor-rdy-inv: RDY signal is active high
+ - nvidia,snor-adv-inv: ADV signal is active high
+ - nvidia,snor-oe-inv: WE/OE signal is active high
+ - nvidia,snor-cs-inv: CS signal is active high
+ - nvidia,snor-cs-select: CS output pin configuration. Default is CS0
+ 	<0> : CS0
+	<1> : CS1
+	<2> : CS2
+	<3> : CS3
+	<4> : CS4
+	<5> : CS5
+	<6> : CS6
+	<7> : CS7
+
+  Note that there is some special handling for the timing values.
+  From Tegra TRM:
+  Programming 0 means 1 clock cycle: actual cycle = programmed cycle + 1
+
+ - nvidia,snor-muxed-width: Number of cycles MUX address/data asserted on the
+   bus. Valid values are 0-15, default is 1
+ - nvidia,snor-hold-width: Number of cycles CE stays asserted after the
+   de-assertion of WR_N (in case of SLAVE/MASTER Request) or OE_N
+   (in case of MASTER Request). Valid values are 0-15, default is 1
+ - nvidia,snor-adv-width: Number of cycles during which ADV stays asserted.
+   Valid values are 0-15, default is 1.
+ - nvidia,snor-ce-width: Number of cycles before CE is asserted.
+   Valid values are 0-255, default is 4
+ - nvidia,snor-we-width: Number of cycles during which WE stays asserted.
+   Valid values are 0-15, default is 1
+ - nvidia,snor-oe-width: Number of cycles during which OE stays asserted.
+   Valid values are 0-255, default is 1
+ - nvidia,snor-wait-width: Number of cycles before READY is asserted.
+   Valid values are 0-255, default is 3
+
+Example with two SJA1000 CAN controllers connected to the GMI bus:
+
+  gmi at 70090000 {
+    #address-cells = <1>;
+    #size-cells = <1>;
+    ranges;
+    nvidia,snor-mux-mode;
+    nvidia,snor-adv-inv;
+    nvidia,snor-cs-select = <4>;
+
+    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>;
+        ...
+      };
+    };
+  };
-- 
2.1.4

[PATCH 3/6] dt/bindings: Add bindings for Tegra GMI controller

[Jon Hunter]

On 06/08/16 20:40, Mirza Krak wrote:
> From: Mirza Krak <mirza.krak@gmail.com>
> 
> Document the devicetree bindings for the Generic Memory Interface (GMI)
> bus driver found on Tegra SOCs.
> 
> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
> ---
>  .../devicetree/bindings/bus/nvidia,tegra20-gmi.txt | 99 ++++++++++++++++++++++
>  1 file changed, 99 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt
> 
> diff --git a/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt b/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt
> new file mode 100644
> index 0000000..046846e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt
> @@ -0,0 +1,99 @@
> +Device tree bindings for NVIDIA Tegra Generic Memory Interface bus
> +
> +The Generic Memory Interface bus enables memory transfers between internal and
> +external memory. Can be used to attach various high speed devices such as
> +synchronous/asynchronous NOR, FPGA, UARTS and more.
> +
> +The actual devices are instantiated from the child nodes of a GMI node.
> +
> +Required properties:
> +  - compatible : Should contain one of the following:
> +        For Tegra20 must contain "nvidia,tegra20-gmi".
> +        For Tegra30 must contain "nvidia,tegra30-gmi".
> + - reg: Should contain GMI controller registers location and length.
> + - clocks: Must contain an entry for each entry in clock-names.
> + - clock-names: Must include the following entries: "gmi"
> + - resets : Must contain an entry for each entry in reset-names.
> + - reset-names : Must include the following entries: "gmi"
> + - #address-cells: The number of cells used to represent physical base
> +   addresses in the GMI address space.
> + - #size-cells: The number of cells used to represent the size of an address
> +   range in the GMI address space.
> + - ranges: Mapping of the GMI address space to the CPU address space.
> +
> +Note that the GMI 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. Furthermore, if you do have external logic to

The above is not 100% accurate. I would say that because there is no
chip-select address decoding, chip-selects either need to be managed via
software or by employing external chip-select decoding logic.

> +support multiple devices this would assume that the devices use the same
> +timing and so are probably the same type. It also assumes that they can fit in
> +the 256MB address range.

Again this is only true for the case where you have external chip-select
decoding logic, but would not be the case if software were to manage the
chip-selects.

> +
> +Optional properties:
> +
> + - nvidia,snor-data-width-32bit: Use 32bit data-bus, default is 16bit.
> + - nvidia,snor-mux-mode: Enable address/data MUX mode.
> + - nvidia,snor-rdy-active-before-data: Assert RDY signal one cycle before data.
> +   If omitted it will be asserted with data.
> + - nvidia,snor-rdy-inv: RDY signal is active high
> + - nvidia,snor-adv-inv: ADV signal is active high
> + - nvidia,snor-oe-inv: WE/OE signal is active high
> + - nvidia,snor-cs-inv: CS signal is active high
> + - nvidia,snor-cs-select: CS output pin configuration. Default is CS0

Nit ... I think "nvidia,snor-cs" is sufficient for the name. But I am
not sure if we even need this. See below.

> + 	<0> : CS0
> +	<1> : CS1
> +	<2> : CS2
> +	<3> : CS3
> +	<4> : CS4
> +	<5> : CS5
> +	<6> : CS6
> +	<7> : CS7
> +
> +  Note that there is some special handling for the timing values.
> +  From Tegra TRM:
> +  Programming 0 means 1 clock cycle: actual cycle = programmed cycle + 1
> +
> + - nvidia,snor-muxed-width: Number of cycles MUX address/data asserted on the
> +   bus. Valid values are 0-15, default is 1
> + - nvidia,snor-hold-width: Number of cycles CE stays asserted after the
> +   de-assertion of WR_N (in case of SLAVE/MASTER Request) or OE_N
> +   (in case of MASTER Request). Valid values are 0-15, default is 1
> + - nvidia,snor-adv-width: Number of cycles during which ADV stays asserted.
> +   Valid values are 0-15, default is 1.
> + - nvidia,snor-ce-width: Number of cycles before CE is asserted.
> +   Valid values are 0-255, default is 4

ce-width only occupies a 4-bit field and so the max is 15.

> + - nvidia,snor-we-width: Number of cycles during which WE stays asserted.
> +   Valid values are 0-15, default is 1
> + - nvidia,snor-oe-width: Number of cycles during which OE stays asserted.
> +   Valid values are 0-255, default is 1
> + - nvidia,snor-wait-width: Number of cycles before READY is asserted.
> +   Valid values are 0-255, default is 3
> +
> +Example with two SJA1000 CAN controllers connected to the GMI bus:
> +
> +  gmi at 70090000 {
> +    #address-cells = <1>;
> +    #size-cells = <1>;

I think 0 for size makes sense. I know that caused you problems before,
but I am wondering if ...

> +    ranges;

... ranges is needed here? If we do have it, I am wondering if it should
be a single entry for the chip-select that is being used. For now we
could only support a ranges with one entry.

	#address-cells = <1>;
	#size-cells = <1>;
	ranges = <4 0x48000000 0x00040000>;

> +    nvidia,snor-mux-mode;
> +    nvidia,snor-adv-inv;
> +    nvidia,snor-cs-select = <4>;

I would have expected these under bus at X node as they are specific to the
GMI CS.

I would also expect that the actual chip-select number is encoded in the
reg property.

> +
> +    bus at 0,0 {

bus at 4

No mention of this bus node in the above documentation.

> +      compatible = "simple-bus";
> +      reg = <0 0>;

reg = <4>;

We should look up the chip-select from the reg property.

> +      ranges;
> +
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +
> +      can at 48000000 {
> +        reg = <0x48000000 0x100>;
> +        ...
> +      };
> +
> +      can at 48040000 {
> +        reg = <0x48040000 0x100>;
> +        ...
> +      };

If we use ranges we could have ...

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

	can at 40000 {
		reg = <0x40000 0x100>;
		...
	};

Nit ... please use tabs for spacing as we do in the dtsi/dts files.

Cheers
Jon

-- 
nvpublic

[PATCH 3/6] dt/bindings: Add bindings for Tegra GMI controller

[Mirza Krak]

2016-08-08 16:44 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>
> On 06/08/16 20:40, Mirza Krak wrote:
>> From: Mirza Krak <mirza.krak@gmail.com>
>>
>> Document the devicetree bindings for the Generic Memory Interface (GMI)
>> bus driver found on Tegra SOCs.
>>
>> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
>> ---
>>  .../devicetree/bindings/bus/nvidia,tegra20-gmi.txt | 99 ++++++++++++++++++++++
>>  1 file changed, 99 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt
>>
>> diff --git a/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt b/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt
>> new file mode 100644
>> index 0000000..046846e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt
>> @@ -0,0 +1,99 @@
>> +Device tree bindings for NVIDIA Tegra Generic Memory Interface bus
>> +
>> +The Generic Memory Interface bus enables memory transfers between internal and
>> +external memory. Can be used to attach various high speed devices such as
>> +synchronous/asynchronous NOR, FPGA, UARTS and more.
>> +
>> +The actual devices are instantiated from the child nodes of a GMI node.
>> +
>> +Required properties:
>> +  - compatible : Should contain one of the following:
>> +        For Tegra20 must contain "nvidia,tegra20-gmi".
>> +        For Tegra30 must contain "nvidia,tegra30-gmi".
>> + - reg: Should contain GMI controller registers location and length.
>> + - clocks: Must contain an entry for each entry in clock-names.
>> + - clock-names: Must include the following entries: "gmi"
>> + - resets : Must contain an entry for each entry in reset-names.
>> + - reset-names : Must include the following entries: "gmi"
>> + - #address-cells: The number of cells used to represent physical base
>> +   addresses in the GMI address space.
>> + - #size-cells: The number of cells used to represent the size of an address
>> +   range in the GMI address space.
>> + - ranges: Mapping of the GMI address space to the CPU address space.
>> +
>> +Note that the GMI 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. Furthermore, if you do have external logic to
>
> The above is not 100% accurate. I would say that because there is no
> chip-select address decoding, chip-selects either need to be managed via
> software or by employing external chip-select decoding logic.

ACK, will update with the possibility of managing CS in software.
>
>> +support multiple devices this would assume that the devices use the same
>> +timing and so are probably the same type. It also assumes that they can fit in
>> +the 256MB address range.
>
> Again this is only true for the case where you have external chip-select
> decoding logic, but would not be the case if software were to manage the
> chip-selects.

ACK

>
>> +
>> +Optional properties:
>> +
>> + - nvidia,snor-data-width-32bit: Use 32bit data-bus, default is 16bit.
>> + - nvidia,snor-mux-mode: Enable address/data MUX mode.
>> + - nvidia,snor-rdy-active-before-data: Assert RDY signal one cycle before data.
>> +   If omitted it will be asserted with data.
>> + - nvidia,snor-rdy-inv: RDY signal is active high
>> + - nvidia,snor-adv-inv: ADV signal is active high
>> + - nvidia,snor-oe-inv: WE/OE signal is active high
>> + - nvidia,snor-cs-inv: CS signal is active high
>> + - nvidia,snor-cs-select: CS output pin configuration. Default is CS0
>
> Nit ... I think "nvidia,snor-cs" is sufficient for the name. But I am
> not sure if we even need this. See below.
>
>> +     <0> : CS0
>> +     <1> : CS1
>> +     <2> : CS2
>> +     <3> : CS3
>> +     <4> : CS4
>> +     <5> : CS5
>> +     <6> : CS6
>> +     <7> : CS7
>> +
>> +  Note that there is some special handling for the timing values.
>> +  From Tegra TRM:
>> +  Programming 0 means 1 clock cycle: actual cycle = programmed cycle + 1
>> +
>> + - nvidia,snor-muxed-width: Number of cycles MUX address/data asserted on the
>> +   bus. Valid values are 0-15, default is 1
>> + - nvidia,snor-hold-width: Number of cycles CE stays asserted after the
>> +   de-assertion of WR_N (in case of SLAVE/MASTER Request) or OE_N
>> +   (in case of MASTER Request). Valid values are 0-15, default is 1
>> + - nvidia,snor-adv-width: Number of cycles during which ADV stays asserted.
>> +   Valid values are 0-15, default is 1.
>> + - nvidia,snor-ce-width: Number of cycles before CE is asserted.
>> +   Valid values are 0-255, default is 4
>
> ce-width only occupies a 4-bit field and so the max is 15.

ACK.

>
>> + - nvidia,snor-we-width: Number of cycles during which WE stays asserted.
>> +   Valid values are 0-15, default is 1
>> + - nvidia,snor-oe-width: Number of cycles during which OE stays asserted.
>> +   Valid values are 0-255, default is 1
>> + - nvidia,snor-wait-width: Number of cycles before READY is asserted.
>> +   Valid values are 0-255, default is 3
>> +
>> +Example with two SJA1000 CAN controllers connected to the GMI bus:
>> +
>> +  gmi at 70090000 {
>> +    #address-cells = <1>;
>> +    #size-cells = <1>;
>
> I think 0 for size makes sense. I know that caused you problems before,
> but I am wondering if ...
>
>> +    ranges;
>
> ... ranges is needed here? If we do have it, I am wondering if it should
> be a single entry for the chip-select that is being used. For now we
> could only support a ranges with one entry.
>
>         #address-cells = <1>;
>         #size-cells = <1>;
>         ranges = <4 0x48000000 0x00040000>;

I prefer if we have "ranges" with one single entry, and warn if user enters
multiple for now, like we discussed earlier. Should have really done it in
this series.

>
>> +    nvidia,snor-mux-mode;
>> +    nvidia,snor-adv-inv;
>> +    nvidia,snor-cs-select = <4>;
>
> I would have expected these under bus at X node as they are specific to the
> GMI CS.

Yes, that is true.

>
> I would also expect that the actual chip-select number is encoded in the
> reg property.
>
>> +
>> +    bus at 0,0 {
>
> bus at 4

ACK.

>
> No mention of this bus node in the above documentation.

I was hesitant documenting it since I am not sure if we really need it
in a generic case? It does make sense in my
specific case. But what would it look like if we could maintain CS in software.

Do you then have a bus node per CS? I am guessing not. Is it enough to
document it in my "example brief"?

I should probably extend the example with some more configurations,
e.g. only one child node.

What I am thinking is something like

Example with two SJA1000 CAN controllers connected to the GMI bus.
We wrap the controllers with a  simple-bus node since they are
all connected to the same chip-select (CS4) but external address decoding is
provided in hardware:

  gmi at 70090000 {
    #address-cells = <1>;
    #size-cells = <1>;
    ranges = <4 0x48000000 0x00040000>;

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

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

      nvidia,snor-mux-mode;
      nvidia,snor-adv-inv;

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

      can at 40000 {
        reg = <0x40000 0x100>;
        ...
      };
    };
  };

Example with one SJA1000 CAN controller connected to the GMI bus
on CS4:

  gmi at 70090000 {
    #address-cells = <1>;
    #size-cells = <1>;
    ranges = <4 0x48000000 0x00040000>;

   can at 4 {
      nvidia,snor-mux-mode;
      nvidia,snor-adv-inv;
      reg = <4 0x100>;
      ...
   };
};

>
>> +      compatible = "simple-bus";
>> +      reg = <0 0>;
>
> reg = <4>;
>
> We should look up the chip-select from the reg property.

ACK.
>
>> +      ranges;
>> +
>> +      #address-cells = <1>;
>> +      #size-cells = <1>;
>> +
>> +      can at 48000000 {
>> +        reg = <0x48000000 0x100>;
>> +        ...
>> +      };
>> +
>> +      can at 48040000 {
>> +        reg = <0x48040000 0x100>;
>> +        ...
>> +      };
>
> If we use ranges we could have ...
>
>         can at 0 {
>                 reg = <0x0 0x100>;
>                 ...
>         };
>
>         can at 40000 {
>                 reg = <0x40000 0x100>;
>                 ...
>         };
>
> Nit ... please use tabs for spacing as we do in the dtsi/dts files.

ACK.

Best Regards
Mirza Krak

[PATCH 3/6] dt/bindings: Add bindings for Tegra GMI controller

[Jon Hunter]

On 09/08/16 09:40, Mirza Krak wrote:
> 2016-08-08 16:44 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>>
>> On 06/08/16 20:40, Mirza Krak wrote:
>>> From: Mirza Krak <mirza.krak@gmail.com>
>>>
>>> Document the devicetree bindings for the Generic Memory Interface (GMI)
>>> bus driver found on Tegra SOCs.
>>>
>>> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
>>> ---
>>>  .../devicetree/bindings/bus/nvidia,tegra20-gmi.txt | 99 ++++++++++++++++++++++
>>>  1 file changed, 99 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt b/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt
>>> new file mode 100644
>>> index 0000000..046846e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt
>>> @@ -0,0 +1,99 @@
>>> +Device tree bindings for NVIDIA Tegra Generic Memory Interface bus
>>> +
>>> +The Generic Memory Interface bus enables memory transfers between internal and
>>> +external memory. Can be used to attach various high speed devices such as
>>> +synchronous/asynchronous NOR, FPGA, UARTS and more.
>>> +
>>> +The actual devices are instantiated from the child nodes of a GMI node.
>>> +
>>> +Required properties:
>>> +  - compatible : Should contain one of the following:
>>> +        For Tegra20 must contain "nvidia,tegra20-gmi".
>>> +        For Tegra30 must contain "nvidia,tegra30-gmi".
>>> + - reg: Should contain GMI controller registers location and length.
>>> + - clocks: Must contain an entry for each entry in clock-names.
>>> + - clock-names: Must include the following entries: "gmi"
>>> + - resets : Must contain an entry for each entry in reset-names.
>>> + - reset-names : Must include the following entries: "gmi"
>>> + - #address-cells: The number of cells used to represent physical base
>>> +   addresses in the GMI address space.
>>> + - #size-cells: The number of cells used to represent the size of an address
>>> +   range in the GMI address space.
>>> + - ranges: Mapping of the GMI address space to the CPU address space.
>>> +
>>> +Note that the GMI 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. Furthermore, if you do have external logic to
>>
>> The above is not 100% accurate. I would say that because there is no
>> chip-select address decoding, chip-selects either need to be managed via
>> software or by employing external chip-select decoding logic.
> 
> ACK, will update with the possibility of managing CS in software.
>>
>>> +support multiple devices this would assume that the devices use the same
>>> +timing and so are probably the same type. It also assumes that they can fit in
>>> +the 256MB address range.
>>
>> Again this is only true for the case where you have external chip-select
>> decoding logic, but would not be the case if software were to manage the
>> chip-selects.
> 
> ACK
> 
>>
>>> +
>>> +Optional properties:
>>> +
>>> + - nvidia,snor-data-width-32bit: Use 32bit data-bus, default is 16bit.
>>> + - nvidia,snor-mux-mode: Enable address/data MUX mode.
>>> + - nvidia,snor-rdy-active-before-data: Assert RDY signal one cycle before data.
>>> +   If omitted it will be asserted with data.
>>> + - nvidia,snor-rdy-inv: RDY signal is active high
>>> + - nvidia,snor-adv-inv: ADV signal is active high
>>> + - nvidia,snor-oe-inv: WE/OE signal is active high
>>> + - nvidia,snor-cs-inv: CS signal is active high
>>> + - nvidia,snor-cs-select: CS output pin configuration. Default is CS0
>>
>> Nit ... I think "nvidia,snor-cs" is sufficient for the name. But I am
>> not sure if we even need this. See below.
>>
>>> +     <0> : CS0
>>> +     <1> : CS1
>>> +     <2> : CS2
>>> +     <3> : CS3
>>> +     <4> : CS4
>>> +     <5> : CS5
>>> +     <6> : CS6
>>> +     <7> : CS7
>>> +
>>> +  Note that there is some special handling for the timing values.
>>> +  From Tegra TRM:
>>> +  Programming 0 means 1 clock cycle: actual cycle = programmed cycle + 1
>>> +
>>> + - nvidia,snor-muxed-width: Number of cycles MUX address/data asserted on the
>>> +   bus. Valid values are 0-15, default is 1
>>> + - nvidia,snor-hold-width: Number of cycles CE stays asserted after the
>>> +   de-assertion of WR_N (in case of SLAVE/MASTER Request) or OE_N
>>> +   (in case of MASTER Request). Valid values are 0-15, default is 1
>>> + - nvidia,snor-adv-width: Number of cycles during which ADV stays asserted.
>>> +   Valid values are 0-15, default is 1.
>>> + - nvidia,snor-ce-width: Number of cycles before CE is asserted.
>>> +   Valid values are 0-255, default is 4
>>
>> ce-width only occupies a 4-bit field and so the max is 15.
> 
> ACK.
> 
>>
>>> + - nvidia,snor-we-width: Number of cycles during which WE stays asserted.
>>> +   Valid values are 0-15, default is 1
>>> + - nvidia,snor-oe-width: Number of cycles during which OE stays asserted.
>>> +   Valid values are 0-255, default is 1
>>> + - nvidia,snor-wait-width: Number of cycles before READY is asserted.
>>> +   Valid values are 0-255, default is 3
>>> +
>>> +Example with two SJA1000 CAN controllers connected to the GMI bus:
>>> +
>>> +  gmi at 70090000 {
>>> +    #address-cells = <1>;
>>> +    #size-cells = <1>;
>>
>> I think 0 for size makes sense. I know that caused you problems before,
>> but I am wondering if ...
>>
>>> +    ranges;
>>
>> ... ranges is needed here? If we do have it, I am wondering if it should
>> be a single entry for the chip-select that is being used. For now we
>> could only support a ranges with one entry.
>>
>>         #address-cells = <1>;
>>         #size-cells = <1>;
>>         ranges = <4 0x48000000 0x00040000>;
> 
> I prefer if we have "ranges" with one single entry, and warn if user enters
> multiple for now, like we discussed earlier. Should have really done it in
> this series.

I think I do as well.

>>
>>> +    nvidia,snor-mux-mode;
>>> +    nvidia,snor-adv-inv;
>>> +    nvidia,snor-cs-select = <4>;
>>
>> I would have expected these under bus at X node as they are specific to the
>> GMI CS.
> 
> Yes, that is true.
> 
>>
>> I would also expect that the actual chip-select number is encoded in the
>> reg property.
>>
>>> +
>>> +    bus at 0,0 {
>>
>> bus at 4
> 
> ACK.
> 
>>
>> No mention of this bus node in the above documentation.
> 
> I was hesitant documenting it since I am not sure if we really need it
> in a generic case? It does make sense in my
> specific case. But what would it look like if we could maintain CS in software.
> 
> Do you then have a bus node per CS? I am guessing not. Is it enough to
> document it in my "example brief"?

I see what you mean. May be it is fine to document with the examples
below how child devices should be populated. You should also state that
currently the GMI only supports one child device currently for the
chip-select that is being used.

Cheers
Jon

-- 
nvpublic

[PATCH 3/6] dt/bindings: Add bindings for Tegra GMI controller

[Mirza Krak]

2016-08-09 15:34 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>
> On 09/08/16 09:40, Mirza Krak wrote:
>> 2016-08-08 16:44 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>>>
>>> On 06/08/16 20:40, Mirza Krak wrote:
>>>> From: Mirza Krak <mirza.krak@gmail.com>
>>>>

<--snip-->

>>>> + - nvidia,snor-we-width: Number of cycles during which WE stays asserted.
>>>> +   Valid values are 0-15, default is 1
>>>> + - nvidia,snor-oe-width: Number of cycles during which OE stays asserted.
>>>> +   Valid values are 0-255, default is 1
>>>> + - nvidia,snor-wait-width: Number of cycles before READY is asserted.
>>>> +   Valid values are 0-255, default is 3
>>>> +
>>>> +Example with two SJA1000 CAN controllers connected to the GMI bus:
>>>> +
>>>> +  gmi at 70090000 {
>>>> +    #address-cells = <1>;
>>>> +    #size-cells = <1>;
>>>
>>> I think 0 for size makes sense. I know that caused you problems before,
>>> but I am wondering if ...
>>>
>>>> +    ranges;
>>>
>>> ... ranges is needed here? If we do have it, I am wondering if it should
>>> be a single entry for the chip-select that is being used. For now we
>>> could only support a ranges with one entry.
>>>
>>>         #address-cells = <1>;
>>>         #size-cells = <1>;
>>>         ranges = <4 0x48000000 0x00040000>;
>>
>> I prefer if we have "ranges" with one single entry, and warn if user enters
>> multiple for now, like we discussed earlier. Should have really done it in
>> this series.
>
> I think I do as well.
>
>>>
>>>> +    nvidia,snor-mux-mode;
>>>> +    nvidia,snor-adv-inv;
>>>> +    nvidia,snor-cs-select = <4>;
>>>
>>> I would have expected these under bus at X node as they are specific to the
>>> GMI CS.
>>
>> Yes, that is true.
>>
>>>
>>> I would also expect that the actual chip-select number is encoded in the
>>> reg property.
>>>
>>>> +
>>>> +    bus at 0,0 {
>>>
>>> bus at 4
>>
>> ACK.
>>
>>>
>>> No mention of this bus node in the above documentation.
>>
>> I was hesitant documenting it since I am not sure if we really need it
>> in a generic case? It does make sense in my
>> specific case. But what would it look like if we could maintain CS in software.
>>
>> Do you then have a bus node per CS? I am guessing not. Is it enough to
>> document it in my "example brief"?
>
> I see what you mean. May be it is fine to document with the examples
> below how child devices should be populated. You should also state that
> currently the GMI only supports one child device currently for the
> chip-select that is being used.

Should I really need to state that? Is it not always the case, that
you have one child device per chip-select?

Best Regards
Mirza

[PATCH 3/6] dt/bindings: Add bindings for Tegra GMI controller

[Jon Hunter]

On 09/08/16 21:48, Mirza Krak wrote:
> 2016-08-09 15:34 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>>
>> On 09/08/16 09:40, Mirza Krak wrote:
>>> 2016-08-08 16:44 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>>>>
>>>> On 06/08/16 20:40, Mirza Krak wrote:
>>>>> From: Mirza Krak <mirza.krak@gmail.com>
>>>>>
> 
> <--snip-->
> 
>>>>> + - nvidia,snor-we-width: Number of cycles during which WE stays asserted.
>>>>> +   Valid values are 0-15, default is 1
>>>>> + - nvidia,snor-oe-width: Number of cycles during which OE stays asserted.
>>>>> +   Valid values are 0-255, default is 1
>>>>> + - nvidia,snor-wait-width: Number of cycles before READY is asserted.
>>>>> +   Valid values are 0-255, default is 3
>>>>> +
>>>>> +Example with two SJA1000 CAN controllers connected to the GMI bus:
>>>>> +
>>>>> +  gmi at 70090000 {
>>>>> +    #address-cells = <1>;
>>>>> +    #size-cells = <1>;
>>>>
>>>> I think 0 for size makes sense. I know that caused you problems before,
>>>> but I am wondering if ...
>>>>
>>>>> +    ranges;
>>>>
>>>> ... ranges is needed here? If we do have it, I am wondering if it should
>>>> be a single entry for the chip-select that is being used. For now we
>>>> could only support a ranges with one entry.
>>>>
>>>>         #address-cells = <1>;
>>>>         #size-cells = <1>;
>>>>         ranges = <4 0x48000000 0x00040000>;
>>>
>>> I prefer if we have "ranges" with one single entry, and warn if user enters
>>> multiple for now, like we discussed earlier. Should have really done it in
>>> this series.
>>
>> I think I do as well.
>>
>>>>
>>>>> +    nvidia,snor-mux-mode;
>>>>> +    nvidia,snor-adv-inv;
>>>>> +    nvidia,snor-cs-select = <4>;
>>>>
>>>> I would have expected these under bus at X node as they are specific to the
>>>> GMI CS.
>>>
>>> Yes, that is true.
>>>
>>>>
>>>> I would also expect that the actual chip-select number is encoded in the
>>>> reg property.
>>>>
>>>>> +
>>>>> +    bus at 0,0 {
>>>>
>>>> bus at 4
>>>
>>> ACK.
>>>
>>>>
>>>> No mention of this bus node in the above documentation.
>>>
>>> I was hesitant documenting it since I am not sure if we really need it
>>> in a generic case? It does make sense in my
>>> specific case. But what would it look like if we could maintain CS in software.
>>>
>>> Do you then have a bus node per CS? I am guessing not. Is it enough to
>>> document it in my "example brief"?
>>
>> I see what you mean. May be it is fine to document with the examples
>> below how child devices should be populated. You should also state that
>> currently the GMI only supports one child device currently for the
>> chip-select that is being used.
> 
> Should I really need to state that? Is it not always the case, that
> you have one child device per chip-select?

I think so. Remember it is one child device for the GMI, however, that
child device could still have one than one child device (per you example
with two CANs). The GMI child represents the active chip-select and we
only currently support one with this driver.

Jon

-- 
nvpublic

[PATCH 3/6] dt/bindings: Add bindings for Tegra GMI controller

[Jon Hunter]

On 10/08/16 09:45, Jon Hunter wrote:
> 
> On 09/08/16 21:48, Mirza Krak wrote:
>> 2016-08-09 15:34 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>>>
>>> On 09/08/16 09:40, Mirza Krak wrote:
>>>> 2016-08-08 16:44 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>>>>>
>>>>> On 06/08/16 20:40, Mirza Krak wrote:
>>>>>> From: Mirza Krak <mirza.krak@gmail.com>
>>>>>>
>>
>> <--snip-->
>>
>>>>>> + - nvidia,snor-we-width: Number of cycles during which WE stays asserted.
>>>>>> +   Valid values are 0-15, default is 1
>>>>>> + - nvidia,snor-oe-width: Number of cycles during which OE stays asserted.
>>>>>> +   Valid values are 0-255, default is 1
>>>>>> + - nvidia,snor-wait-width: Number of cycles before READY is asserted.
>>>>>> +   Valid values are 0-255, default is 3
>>>>>> +
>>>>>> +Example with two SJA1000 CAN controllers connected to the GMI bus:
>>>>>> +
>>>>>> +  gmi at 70090000 {
>>>>>> +    #address-cells = <1>;
>>>>>> +    #size-cells = <1>;
>>>>>
>>>>> I think 0 for size makes sense. I know that caused you problems before,
>>>>> but I am wondering if ...
>>>>>
>>>>>> +    ranges;
>>>>>
>>>>> ... ranges is needed here? If we do have it, I am wondering if it should
>>>>> be a single entry for the chip-select that is being used. For now we
>>>>> could only support a ranges with one entry.
>>>>>
>>>>>         #address-cells = <1>;
>>>>>         #size-cells = <1>;
>>>>>         ranges = <4 0x48000000 0x00040000>;
>>>>
>>>> I prefer if we have "ranges" with one single entry, and warn if user enters
>>>> multiple for now, like we discussed earlier. Should have really done it in
>>>> this series.
>>>
>>> I think I do as well.
>>>
>>>>>
>>>>>> +    nvidia,snor-mux-mode;
>>>>>> +    nvidia,snor-adv-inv;
>>>>>> +    nvidia,snor-cs-select = <4>;
>>>>>
>>>>> I would have expected these under bus at X node as they are specific to the
>>>>> GMI CS.
>>>>
>>>> Yes, that is true.
>>>>
>>>>>
>>>>> I would also expect that the actual chip-select number is encoded in the
>>>>> reg property.
>>>>>
>>>>>> +
>>>>>> +    bus at 0,0 {
>>>>>
>>>>> bus at 4
>>>>
>>>> ACK.
>>>>
>>>>>
>>>>> No mention of this bus node in the above documentation.
>>>>
>>>> I was hesitant documenting it since I am not sure if we really need it
>>>> in a generic case? It does make sense in my
>>>> specific case. But what would it look like if we could maintain CS in software.
>>>>
>>>> Do you then have a bus node per CS? I am guessing not. Is it enough to
>>>> document it in my "example brief"?
>>>
>>> I see what you mean. May be it is fine to document with the examples
>>> below how child devices should be populated. You should also state that
>>> currently the GMI only supports one child device currently for the
>>> chip-select that is being used.
>>
>> Should I really need to state that? Is it not always the case, that
>> you have one child device per chip-select?
> 
> I think so. Remember it is one child device for the GMI, however, that
> child device could still have one than one child device (per you example

s/one than one/more than one

Jon

-- 
nvpublic

[PATCH 3/6] dt/bindings: Add bindings for Tegra GMI controller

[Mirza Krak]

2016-08-08 16:44 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>> +
>> +  gmi at 70090000 {
>> +    #address-cells = <1>;
>> +    #size-cells = <1>;
>
> I think 0 for size makes sense. I know that caused you problems before,
> but I am wondering if ...
>
>> +    ranges;
>
> ... ranges is needed here? If we do have it, I am wondering if it should
> be a single entry for the chip-select that is being used. For now we
> could only support a ranges with one entry.
>
>         #address-cells = <1>;
>         #size-cells = <1>;
>         ranges = <4 0x48000000 0x00040000>;
>
>> +    nvidia,snor-mux-mode;
>> +    nvidia,snor-adv-inv;
>> +    nvidia,snor-cs-select = <4>;
>
> I would have expected these under bus at X node as they are specific to the
> GMI CS.
>
> I would also expect that the actual chip-select number is encoded in the
> reg property.
>
>> +
>> +    bus at 0,0 {
>
> bus at 4
>
> No mention of this bus node in the above documentation.
>
>> +      compatible = "simple-bus";
>> +      reg = <0 0>;
>
> reg = <4>;
>
> We should look up the chip-select from the reg property.
>
>> +      ranges;
>> +
>> +      #address-cells = <1>;
>> +      #size-cells = <1>;
>> +
>> +      can at 48000000 {
>> +        reg = <0x48000000 0x100>;
>> +        ...
>> +      };
>> +
>> +      can at 48040000 {
>> +        reg = <0x48040000 0x100>;
>> +        ...
>> +      };
>
> If we use ranges we could have ...
>
>         can at 0 {
>                 reg = <0x0 0x100>;
>                 ...
>         };
>
>         can at 40000 {
>                 reg = <0x40000 0x100>;
>                 ...
>         };
>

Hi.

Like we discussed I am now trying to implement this but without
success and I am starting to think that it is not that simple unless I
am missing something.

Below tree

gmi at 70009000 {
     status = "okay";
     #address-cells = <1>;
     #size-cells = <1>;
     ranges = <4 0x48000000 0x7ffffff>;

     bus at 4 {
          compatible = "simple-bus";
          reg = <4 0>;
          #address-cells = <1>;
          #size-cells = <1>;
         nvidia,snor-mux-mode;
         nvidia,snor-adv-inv;

        can at 0 {
              compatible = "nxp,sja1000";
              reg = <0 0x100>;
              ....
     };

     can at 40000 {
           compatible = "nxp,sja1000";
           reg = <0x40000 0x100>;
           ....
    };
};
};

results in:

[    8.472509]    create child: /gmi at 70009000/bus at 4/can at 0
[    8.472561] OF: ** translation for device /gmi at 70009000/bus at 4/can at 0 **
[    8.472577] OF: bus is default (na=1, ns=1) on /gmi at 70009000/bus at 4
[    8.472589] OF: translating address: 00000000
[    8.472624] OF: parent bus is default (na=1, ns=1) on /gmi at 70009000
[    8.472641] OF: no ranges; cannot translate
[    8.472668] of_irq_parse_one: dev=/gmi at 70009000/bus at 4/can at 0, index=0
[    8.472687]  intspec=13 intlen=2
[    8.472726]  intsize=2 intlen=2
[    8.472740] of_irq_parse_raw:  /gpio at 6000d000:0000000d,00000001
[    8.493718] of_irq_parse_raw: ipar=/gpio at 6000d000, size=2
[    8.493737]  -> addrsize=1
[    8.493743]  -> got it !
[    8.493755] of_irq_parse_one: dev=/gmi at 70009000/bus at 4/can at 0, index=1
[    8.493771]  intspec=13 intlen=2
[    8.493790]  intsize=2 intlen=2
[    8.493800] of_irq_parse_one: dev=/gmi at 70009000/bus at 4/can at 0, index=0
[    8.493805]  intspec=13 intlen=2
[    8.493812]  intsize=2 intlen=2
[    8.493824] of_irq_parse_raw:  /gpio at 6000d000:0000000d,00000001
[    8.493829] of_irq_parse_raw: ipar=/gpio at 6000d000, size=2
[    8.493834]  -> addrsize=1
[    8.493837]  -> got it !
[    8.493869] OF: ** translation for device /gmi at 70009000/bus at 4/can at 0 **
[    8.493885] OF: bus is default (na=1, ns=1) on /gmi at 70009000/bus at 4
[    8.493892] OF: translating address: 00000000
[    8.493902] OF: parent bus is default (na=1, ns=1) on /gmi at 70009000
[    8.493906] OF: no ranges; cannot translate
[    8.493918] OF: ** translation for device /gmi at 70009000/bus at 4 **
[    8.493924] OF: bus is default (na=1, ns=1) on /gmi at 70009000
[    8.493930] OF: translating address: 00000004
[    8.493938] OF: parent bus is default (na=1, ns=1) on /
[    8.493944] OF: walking ranges...
[    8.493952] OF: default map, cp=4, s=7ffffff, da=4
[    8.493973] OF: parent translation for: 48000000
[    8.493978] OF: with offset: 0
[    8.493986] OF: one level translation: 48000000
[    8.493989] OF: reached root node
[    8.494011] of_dma_get_range: no dma-ranges found for
node(/gmi at 70009000/bus at 4/can at 0)
[    8.494037] platform 48000000.bus:can at 0: device is not dma coherent
[    8.494048] platform 48000000.bus:can at 0: device is not behind an iommu
[    8.495209] sja1000 probe entered
[    8.495212] sja1000 probe entered1

sja1000 probe fails because resource address is NULL.

The result is the same for the second child device.

I also attempted to add "ranges;" property to the bus child node,
which results in the same error on can at 0, and can at 4000 is actually
translated but it is not what I expect it to be (0x4803fff8).

And I am not quite sure how to get around this in a simple way. I can
not wrap my head around how the ranges property actually works really.

Any suggestions would be really helpful.

Best Regards
Mirza

[PATCH 3/6] dt/bindings: Add bindings for Tegra GMI controller

[Jon Hunter]

On 23/08/16 11:33, Mirza Krak wrote:

...

> Like we discussed I am now trying to implement this but without
> success and I am starting to think that it is not that simple unless I
> am missing something.
> 
> Below tree
> 
> gmi at 70009000 {
>      status = "okay";
>      #address-cells = <1>;
>      #size-cells = <1>;
>      ranges = <4 0x48000000 0x7ffffff>;
> 
>      bus at 4 {
>           compatible = "simple-bus";
>           reg = <4 0>;

I don't think you want reg here.

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

May be ranges here?

>          nvidia,snor-mux-mode;
>          nvidia,snor-adv-inv;
> 
>         can at 0 {
>               compatible = "nxp,sja1000";
>               reg = <0 0x100>;
>               ....
>      };
> 
>      can at 40000 {
>            compatible = "nxp,sja1000";
>            reg = <0x40000 0x100>;
>            ....
>     };
> };
> };

Have a look at some other drivers for example:

Documentation/devicetree/bindings/memory-controllers/arm,pl172.txt

Cheers
Jon

-- 
nvpublic

[PATCH 4/6] ARM: tegra: Add Tegra30 GMI support

[Mirza Krak]

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

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

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

diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 5030065..5fd4d29 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -439,6 +439,16 @@
 		status = "disabled";
 	};
 
+	gmi at 70009000 {
+		compatible = "nvidia,tegra30-gmi";
+		reg = <0x70009000 0x1000>;
+		clocks = <&tegra_car TEGRA30_CLK_NOR>;
+		clock-names = "gmi";
+		resets = <&tegra_car 42>;
+		reset-names = "gmi";
+		status = "disabled";
+	};
+
 	pwm: pwm at 7000a000 {
 		compatible = "nvidia,tegra30-pwm", "nvidia,tegra20-pwm";
 		reg = <0x7000a000 0x100>;
-- 
2.1.4

[PATCH 4/6] ARM: tegra: Add Tegra30 GMI support

[Jon Hunter]

On 06/08/16 20:40, Mirza Krak wrote:
> From: Mirza Krak <mirza.krak@gmail.com>
> 
> Add a device node for the GMI controller found on Tegra30.
> 
> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
> ---
>  arch/arm/boot/dts/tegra30.dtsi | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
> index 5030065..5fd4d29 100644
> --- a/arch/arm/boot/dts/tegra30.dtsi
> +++ b/arch/arm/boot/dts/tegra30.dtsi
> @@ -439,6 +439,16 @@
>  		status = "disabled";
>  	};
>  
> +	gmi at 70009000 {
> +		compatible = "nvidia,tegra30-gmi";
> +		reg = <0x70009000 0x1000>;
> +		clocks = <&tegra_car TEGRA30_CLK_NOR>;
> +		clock-names = "gmi";
> +		resets = <&tegra_car 42>;
> +		reset-names = "gmi";
> +		status = "disabled";

#address-cells, #size-cells and ranges appears to be missing.

Cheers
Jon

-- 
nvpublic

[PATCH 5/6] ARM: tegra: Add Tegra20 GMI support

[Mirza Krak]

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

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

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

diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 2207c08..9b97863 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -376,6 +376,17 @@
 		status = "disabled";
 	};
 
+
+	gmi at 70009000 {
+		compatible = "nvidia,tegra20-gmi";
+		reg = <70009000 0x1000>;
+		clocks = <&tegra_car TEGRA20_CLK_NOR>;
+		clock-names = "gmi";
+		resets = < &tegra_car 42>;
+		reset-names = "gmi";
+		status = "disabled";
+	};
+
 	pwm: pwm at 7000a000 {
 		compatible = "nvidia,tegra20-pwm";
 		reg = <0x7000a000 0x100>;
-- 
2.1.4

[PATCH 5/6] ARM: tegra: Add Tegra20 GMI support

[Jon Hunter]

On 06/08/16 20:40, Mirza Krak wrote:
> From: Mirza Krak <mirza.krak@gmail.com>
> 
> Add a device node for the GMI controller found on Tegra20.
> 
> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
> ---
>  arch/arm/boot/dts/tegra20.dtsi | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> index 2207c08..9b97863 100644
> --- a/arch/arm/boot/dts/tegra20.dtsi
> +++ b/arch/arm/boot/dts/tegra20.dtsi
> @@ -376,6 +376,17 @@
>  		status = "disabled";
>  	};
>  
> +
> +	gmi at 70009000 {
> +		compatible = "nvidia,tegra20-gmi";
> +		reg = <70009000 0x1000>;
> +		clocks = <&tegra_car TEGRA20_CLK_NOR>;
> +		clock-names = "gmi";
> +		resets = < &tegra_car 42>;
> +		reset-names = "gmi";
> +		status = "disabled";
> +	};

#address-cells, #size-cells and ranges appears to be missing.

Cheers
Jon

-- 
nvpublic

[PATCH 6/6] bus: Add support for Tegra Generic Memory Interface

[Mirza Krak]

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

The Generic Memory Interface bus can be used to connect high-speed
devices such as NOR flash, FPGAs, DSPs...

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

diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 3b205e2..88bbf2c 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_GMI
+	tristate "Tegra Generic Memory Interface bus driver"
+		depends on ARCH_TEGRA
+		help
+			Driver for the Tegra Generic Memory Interface bus which can be used
+			to attach devices such as NOR, UART, FPGA and more.
+
 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..34e2bab 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_GMI)		+= tegra-gmi.o
 obj-$(CONFIG_UNIPHIER_SYSTEM_BUS)	+= uniphier-system-bus.o
 obj-$(CONFIG_VEXPRESS_CONFIG)	+= vexpress-config.o
diff --git a/drivers/bus/tegra-gmi.c b/drivers/bus/tegra-gmi.c
new file mode 100644
index 0000000..7a45442
--- /dev/null
+++ b/drivers/bus/tegra-gmi.c
@@ -0,0 +1,224 @@
+/*
+ * Driver for NVIDIA Generic Memory Interface
+ *
+ * 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/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/reset.h>
+
+#define TEGRA_GMI_CONFIG			0x00
+#define TEGRA_GMI_CONFIG_GO			BIT(31)
+#define TEGRA_GMI_BUS_WIDTH_32BIT	BIT(30)
+#define TEGRA_GMI_MUX_MODE			BIT(28)
+#define TEGRA_GMI_RDY_BEFORE_DATA	BIT(24)
+#define TEGRA_GMI_RDY_ACTIVE_HIGH	BIT(23)
+#define TEGRA_GMI_ADV_ACTIVE_HIGH	BIT(22)
+#define TEGRA_GMI_OE_ACTIVE_HIGH	BIT(21)
+#define TEGRA_GMI_CS_ACTIVE_HIGH	BIT(20)
+#define TEGRA_GMI_CS_SELECT(x)		((x & 0x7) << 4)
+
+#define TEGRA_GMI_TIMING0			0x10
+#define TEGRA_GMI_MUXED_WIDTH(x)	((x & 0xf) << 12)
+#define TEGRA_GMI_HOLD_WIDTH(x)		((x & 0xf) << 8)
+#define TEGRA_GMI_ADV_WIDTH(x)		((x & 0xf) << 4)
+#define TEGRA_GMI_CE_WIDTH(x)		(x & 0xf)
+
+#define TEGRA_GMI_TIMING1			0x14
+#define TEGRA_GMI_WE_WIDTH(x)		((x & 0xff) << 16)
+#define TEGRA_GMI_OE_WIDTH(x)		((x & 0xff) << 8)
+#define TEGRA_GMI_WAIT_WIDTH(x)		(x & 0xff)
+
+struct tegra_gmi_priv {
+	void __iomem *base;
+
+	u32 snor_config;
+	u32 snor_timing0;
+	u32 snor_timing1;
+
+	struct clk *clk;
+};
+
+static void tegra_gmi_init(struct device *dev, struct tegra_gmi_priv *priv)
+{
+	writel(priv->snor_timing0, priv->base + TEGRA_GMI_TIMING0);
+	writel(priv->snor_timing1, priv->base + TEGRA_GMI_TIMING1);
+
+	priv->snor_config |= TEGRA_GMI_CONFIG_GO;
+	writel(priv->snor_config, priv->base + TEGRA_GMI_CONFIG);
+}
+
+static int tegra_gmi_parse_dt(struct device *dev, struct tegra_gmi_priv *priv)
+{
+	struct device_node *of_node = dev->of_node;
+	u32 property;
+
+	/* configuration */
+
+	if (of_property_read_bool(of_node, "nvidia,snor-data-width-32bit"))
+		priv->snor_config |= TEGRA_GMI_BUS_WIDTH_32BIT;
+
+	if (of_property_read_bool(of_node, "nvidia,snor-mux-mode"))
+		priv->snor_config |= TEGRA_GMI_MUX_MODE;
+
+	if (of_property_read_bool(of_node, "nvidia,snor-rdy-active-before-data"))
+		priv->snor_config |= TEGRA_GMI_RDY_BEFORE_DATA;
+
+	if (of_property_read_bool(of_node, "nvidia,snor-rdy-inv"))
+		priv->snor_config |= TEGRA_GMI_RDY_ACTIVE_HIGH;
+
+	if (of_property_read_bool(of_node, "nvidia,snor-adv-inv"))
+		priv->snor_config |= TEGRA_GMI_ADV_ACTIVE_HIGH;
+
+	if (of_property_read_bool(of_node, "nvidia,snor-oe-inv"))
+		priv->snor_config |= TEGRA_GMI_OE_ACTIVE_HIGH;
+
+	if (of_property_read_bool(of_node, "nvidia,snor-cs-inv"))
+		priv->snor_config |= TEGRA_GMI_CS_ACTIVE_HIGH;
+
+	if (!of_property_read_u32(of_node, "nvidia,snor-cs-select", &property))
+		priv->snor_config |= TEGRA_GMI_CS_SELECT(property);
+
+	/* Timing, the default values that are provided are reset values */
+
+	if (!of_property_read_u32(of_node, "nvidia,snor-muxed-width", &property))
+		priv->snor_timing0 |= TEGRA_GMI_MUXED_WIDTH(property);
+	else
+		priv->snor_timing0 |= TEGRA_GMI_MUXED_WIDTH(1);
+
+	if (!of_property_read_u32(of_node, "nvidia,snor-hold-width", &property))
+		priv->snor_timing0 |= TEGRA_GMI_HOLD_WIDTH(property);
+	else
+		priv->snor_timing0 |= TEGRA_GMI_HOLD_WIDTH(1);
+
+	if (!of_property_read_u32(of_node, "nvidia,snor-adv-width", &property))
+		priv->snor_timing0 |= TEGRA_GMI_ADV_WIDTH(property);
+	else
+		priv->snor_timing0 |= TEGRA_GMI_ADV_WIDTH(1);
+
+	if (!of_property_read_u32(of_node, "nvidia,snor-ce-width", &property))
+		priv->snor_timing0 |= TEGRA_GMI_CE_WIDTH(property);
+	else
+		priv->snor_timing0 |= TEGRA_GMI_CE_WIDTH(4);
+
+	if (!of_property_read_u32(of_node, "nvidia,snor-we-width", &property))
+		priv->snor_timing1 |= TEGRA_GMI_WE_WIDTH(property);
+	else
+		priv->snor_timing1 |= TEGRA_GMI_WE_WIDTH(1);
+
+	if (!of_property_read_u32(of_node, "nvidia,snor-oe-width", &property))
+		priv->snor_timing1 |= TEGRA_GMI_OE_WIDTH(property);
+	else
+		priv->snor_timing1 |= TEGRA_GMI_OE_WIDTH(1);
+
+	if (!of_property_read_u32(of_node, "nvidia,snor-wait-width", &property))
+		priv->snor_timing1 |= TEGRA_GMI_WAIT_WIDTH(property);
+	else
+		priv->snor_timing1 |= TEGRA_GMI_WAIT_WIDTH(3);
+
+	return of_platform_default_populate(of_node, NULL, dev);
+}
+
+static int tegra_gmi_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct clk *clk;
+	struct device *dev = &pdev->dev;
+	struct reset_control *rst;
+	struct tegra_gmi_priv *priv;
+	void __iomem *base;
+	int ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->base = base;
+
+	clk = devm_clk_get(dev, "gmi");
+	if (IS_ERR(clk)) {
+		dev_err(dev, "can not get clock\n");
+		return PTR_ERR(clk);
+	}
+
+	priv->clk = clk;
+
+	rst = devm_reset_control_get(dev, "gmi");
+	if (IS_ERR(rst)) {
+		dev_err(dev, "can not get reset\n");
+		return PTR_ERR(rst);
+	}
+
+	ret = tegra_gmi_parse_dt(dev, priv);
+	if (ret) {
+		dev_err(dev, "fail to create devices.\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		dev_err(dev, "fail to enable clock.\n");
+		return ret;
+	}
+
+	reset_control_assert(rst);
+	udelay(2);
+	reset_control_deassert(rst);
+
+	tegra_gmi_init(dev, priv);
+
+	dev_set_drvdata(dev, priv);
+
+	return 0;
+}
+
+static int tegra_gmi_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct tegra_gmi_priv *priv = dev_get_drvdata(dev);
+	void __iomem *base = priv->base;
+	u32 config;
+
+	of_platform_depopulate(dev);
+
+	config = readl(base + TEGRA_GMI_CONFIG);
+	config &= ~TEGRA_GMI_CONFIG_GO;
+	writel(config, base + TEGRA_GMI_CONFIG);
+
+	clk_disable_unprepare(priv->clk);
+
+	return 0;
+}
+
+static const struct of_device_id tegra_gmi_id_table[] = {
+	{ .compatible = "nvidia,tegra20-gmi", },
+	{ .compatible = "nvidia,tegra30-gmi", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, tegra_gmi_id_table);
+
+static struct platform_driver tegra_gmi_driver = {
+	.probe = tegra_gmi_probe,
+	.remove = tegra_gmi_remove,
+	.driver = {
+		.name		= "tegra-gmi",
+		.of_match_table	= tegra_gmi_id_table,
+	},
+};
+module_platform_driver(tegra_gmi_driver);
+
+MODULE_AUTHOR("Mirza Krak <mirza.krak@gmail.com");
+MODULE_DESCRIPTION("NVIDIA Tegra GMI Bus Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

[PATCH 6/6] bus: Add support for Tegra Generic Memory Interface

[Jon Hunter]

On 06/08/16 20:40, Mirza Krak wrote:
> From: Mirza Krak <mirza.krak@gmail.com>
> 
> The Generic Memory Interface bus can be used to connect high-speed
> devices such as NOR flash, FPGAs, DSPs...
> 
> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
> ---
>  drivers/bus/Kconfig     |   7 ++
>  drivers/bus/Makefile    |   1 +
>  drivers/bus/tegra-gmi.c | 224 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 232 insertions(+)
>  create mode 100644 drivers/bus/tegra-gmi.c
> 
> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
> index 3b205e2..88bbf2c 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_GMI
> +	tristate "Tegra Generic Memory Interface bus driver"
> +		depends on ARCH_TEGRA
> +		help
> +			Driver for the Tegra Generic Memory Interface bus which can be used
> +			to attach devices such as NOR, UART, FPGA and more.
> +
>  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..34e2bab 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_GMI)		+= tegra-gmi.o
>  obj-$(CONFIG_UNIPHIER_SYSTEM_BUS)	+= uniphier-system-bus.o
>  obj-$(CONFIG_VEXPRESS_CONFIG)	+= vexpress-config.o
> diff --git a/drivers/bus/tegra-gmi.c b/drivers/bus/tegra-gmi.c
> new file mode 100644
> index 0000000..7a45442
> --- /dev/null
> +++ b/drivers/bus/tegra-gmi.c
> @@ -0,0 +1,224 @@
> +/*
> + * Driver for NVIDIA Generic Memory Interface
> + *
> + * 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/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/reset.h>
> +
> +#define TEGRA_GMI_CONFIG			0x00
> +#define TEGRA_GMI_CONFIG_GO			BIT(31)
> +#define TEGRA_GMI_BUS_WIDTH_32BIT	BIT(30)
> +#define TEGRA_GMI_MUX_MODE			BIT(28)
> +#define TEGRA_GMI_RDY_BEFORE_DATA	BIT(24)
> +#define TEGRA_GMI_RDY_ACTIVE_HIGH	BIT(23)
> +#define TEGRA_GMI_ADV_ACTIVE_HIGH	BIT(22)
> +#define TEGRA_GMI_OE_ACTIVE_HIGH	BIT(21)
> +#define TEGRA_GMI_CS_ACTIVE_HIGH	BIT(20)
> +#define TEGRA_GMI_CS_SELECT(x)		((x & 0x7) << 4)
> +
> +#define TEGRA_GMI_TIMING0			0x10
> +#define TEGRA_GMI_MUXED_WIDTH(x)	((x & 0xf) << 12)
> +#define TEGRA_GMI_HOLD_WIDTH(x)		((x & 0xf) << 8)
> +#define TEGRA_GMI_ADV_WIDTH(x)		((x & 0xf) << 4)
> +#define TEGRA_GMI_CE_WIDTH(x)		(x & 0xf)
> +
> +#define TEGRA_GMI_TIMING1			0x14
> +#define TEGRA_GMI_WE_WIDTH(x)		((x & 0xff) << 16)
> +#define TEGRA_GMI_OE_WIDTH(x)		((x & 0xff) << 8)
> +#define TEGRA_GMI_WAIT_WIDTH(x)		(x & 0xff)
> +
> +struct tegra_gmi_priv {
> +	void __iomem *base;
> +
> +	u32 snor_config;
> +	u32 snor_timing0;
> +	u32 snor_timing1;
> +
> +	struct clk *clk;
> +};
> +
> +static void tegra_gmi_init(struct device *dev, struct tegra_gmi_priv *priv)
> +{
> +	writel(priv->snor_timing0, priv->base + TEGRA_GMI_TIMING0);
> +	writel(priv->snor_timing1, priv->base + TEGRA_GMI_TIMING1);
> +
> +	priv->snor_config |= TEGRA_GMI_CONFIG_GO;
> +	writel(priv->snor_config, priv->base + TEGRA_GMI_CONFIG);
> +}
> +
> +static int tegra_gmi_parse_dt(struct device *dev, struct tegra_gmi_priv *priv)
> +{
> +	struct device_node *of_node = dev->of_node;
> +	u32 property;
> +
> +	/* configuration */
> +
> +	if (of_property_read_bool(of_node, "nvidia,snor-data-width-32bit"))
> +		priv->snor_config |= TEGRA_GMI_BUS_WIDTH_32BIT;
> +
> +	if (of_property_read_bool(of_node, "nvidia,snor-mux-mode"))
> +		priv->snor_config |= TEGRA_GMI_MUX_MODE;
> +
> +	if (of_property_read_bool(of_node, "nvidia,snor-rdy-active-before-data"))

Line is over 80 characters.

> +		priv->snor_config |= TEGRA_GMI_RDY_BEFORE_DATA;
> +
> +	if (of_property_read_bool(of_node, "nvidia,snor-rdy-inv"))
> +		priv->snor_config |= TEGRA_GMI_RDY_ACTIVE_HIGH;
> +
> +	if (of_property_read_bool(of_node, "nvidia,snor-adv-inv"))
> +		priv->snor_config |= TEGRA_GMI_ADV_ACTIVE_HIGH;
> +
> +	if (of_property_read_bool(of_node, "nvidia,snor-oe-inv"))
> +		priv->snor_config |= TEGRA_GMI_OE_ACTIVE_HIGH;
> +
> +	if (of_property_read_bool(of_node, "nvidia,snor-cs-inv"))
> +		priv->snor_config |= TEGRA_GMI_CS_ACTIVE_HIGH;
> +
> +	if (!of_property_read_u32(of_node, "nvidia,snor-cs-select", &property))
> +		priv->snor_config |= TEGRA_GMI_CS_SELECT(property);
> +
> +	/* Timing, the default values that are provided are reset values */
> +
> +	if (!of_property_read_u32(of_node, "nvidia,snor-muxed-width", &property))

Line is over 80 characters.

> +		priv->snor_timing0 |= TEGRA_GMI_MUXED_WIDTH(property);
> +	else
> +		priv->snor_timing0 |= TEGRA_GMI_MUXED_WIDTH(1);
> +
> +	if (!of_property_read_u32(of_node, "nvidia,snor-hold-width", &property))
> +		priv->snor_timing0 |= TEGRA_GMI_HOLD_WIDTH(property);
> +	else
> +		priv->snor_timing0 |= TEGRA_GMI_HOLD_WIDTH(1);
> +
> +	if (!of_property_read_u32(of_node, "nvidia,snor-adv-width", &property))
> +		priv->snor_timing0 |= TEGRA_GMI_ADV_WIDTH(property);
> +	else
> +		priv->snor_timing0 |= TEGRA_GMI_ADV_WIDTH(1);
> +
> +	if (!of_property_read_u32(of_node, "nvidia,snor-ce-width", &property))
> +		priv->snor_timing0 |= TEGRA_GMI_CE_WIDTH(property);
> +	else
> +		priv->snor_timing0 |= TEGRA_GMI_CE_WIDTH(4);
> +
> +	if (!of_property_read_u32(of_node, "nvidia,snor-we-width", &property))
> +		priv->snor_timing1 |= TEGRA_GMI_WE_WIDTH(property);
> +	else
> +		priv->snor_timing1 |= TEGRA_GMI_WE_WIDTH(1);
> +
> +	if (!of_property_read_u32(of_node, "nvidia,snor-oe-width", &property))
> +		priv->snor_timing1 |= TEGRA_GMI_OE_WIDTH(property);
> +	else
> +		priv->snor_timing1 |= TEGRA_GMI_OE_WIDTH(1);
> +
> +	if (!of_property_read_u32(of_node, "nvidia,snor-wait-width", &property))
> +		priv->snor_timing1 |= TEGRA_GMI_WAIT_WIDTH(property);
> +	else
> +		priv->snor_timing1 |= TEGRA_GMI_WAIT_WIDTH(3);
> +
> +	return of_platform_default_populate(of_node, NULL, dev);

Seems odd to do this here. Why not as the last thing in probe once the
GMI has been setup correctly?

> +}
> +
> +static int tegra_gmi_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct clk *clk;
> +	struct device *dev = &pdev->dev;
> +	struct reset_control *rst;
> +	struct tegra_gmi_priv *priv;
> +	void __iomem *base;
> +	int ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->base = base;

If you allocate the memory first, you can get rid of this extra base
variable.

> +	clk = devm_clk_get(dev, "gmi");
> +	if (IS_ERR(clk)) {
> +		dev_err(dev, "can not get clock\n");
> +		return PTR_ERR(clk);
> +	}
> +
> +	priv->clk = clk;

Why not set priv->clk directly from calling devm_clk_get? Then you don't
this extra clk variable.

> +
> +	rst = devm_reset_control_get(dev, "gmi");
> +	if (IS_ERR(rst)) {
> +		dev_err(dev, "can not get reset\n");
> +		return PTR_ERR(rst);
> +	}
> +
> +	ret = tegra_gmi_parse_dt(dev, priv);
> +	if (ret) {
> +		dev_err(dev, "fail to create devices.\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret) {
> +		dev_err(dev, "fail to enable clock.\n");
> +		return ret;
> +	}
> +
> +	reset_control_assert(rst);
> +	udelay(2);
> +	reset_control_deassert(rst);
> +
> +	tegra_gmi_init(dev, priv);
> +
> +	dev_set_drvdata(dev, priv);
> +
> +	return 0;
> +}
> +
> +static int tegra_gmi_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;

Do you really need this dev variable?

> +	struct tegra_gmi_priv *priv = dev_get_drvdata(dev);
> +	void __iomem *base = priv->base;

Do you really need this base variable?

> +	u32 config;
> +
> +	of_platform_depopulate(dev);
> +
> +	config = readl(base + TEGRA_GMI_CONFIG);
> +	config &= ~TEGRA_GMI_CONFIG_GO;
> +	writel(config, base + TEGRA_GMI_CONFIG);
> +
> +	clk_disable_unprepare(priv->clk);

What about asserting the reset?

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id tegra_gmi_id_table[] = {
> +	{ .compatible = "nvidia,tegra20-gmi", },
> +	{ .compatible = "nvidia,tegra30-gmi", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, tegra_gmi_id_table);
> +
> +static struct platform_driver tegra_gmi_driver = {
> +	.probe = tegra_gmi_probe,
> +	.remove = tegra_gmi_remove,
> +	.driver = {
> +		.name		= "tegra-gmi",
> +		.of_match_table	= tegra_gmi_id_table,
> +	},
> +};
> +module_platform_driver(tegra_gmi_driver);
> +
> +MODULE_AUTHOR("Mirza Krak <mirza.krak@gmail.com");
> +MODULE_DESCRIPTION("NVIDIA Tegra GMI Bus Driver");
> +MODULE_LICENSE("GPL v2");
> 

Cheers
Jon

-- 
nvpublic

[PATCH 6/6] bus: Add support for Tegra Generic Memory Interface

[Mirza Krak]

2016-08-08 15:47 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>
> On 06/08/16 20:40, Mirza Krak wrote:
>> From: Mirza Krak <mirza.krak@gmail.com>
>>
>> The Generic Memory Interface bus can be used to connect high-speed
>> devices such as NOR flash, FPGAs, DSPs...
>>
>> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
>> ---
>>  drivers/bus/Kconfig     |   7 ++
>>  drivers/bus/Makefile    |   1 +
>>  drivers/bus/tegra-gmi.c | 224 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 232 insertions(+)
>>  create mode 100644 drivers/bus/tegra-gmi.c
>>

<--snip-->

>> +
>> +static int tegra_gmi_parse_dt(struct device *dev, struct tegra_gmi_priv *priv)
>> +{
>> +     struct device_node *of_node = dev->of_node;
>> +     u32 property;
>> +
>> +     /* configuration */
>> +
>> +     if (of_property_read_bool(of_node, "nvidia,snor-data-width-32bit"))
>> +             priv->snor_config |= TEGRA_GMI_BUS_WIDTH_32BIT;
>> +
>> +     if (of_property_read_bool(of_node, "nvidia,snor-mux-mode"))
>> +             priv->snor_config |= TEGRA_GMI_MUX_MODE;
>> +
>> +     if (of_property_read_bool(of_node, "nvidia,snor-rdy-active-before-data"))
>
> Line is over 80 characters.

Yes, indeed it is.

I ran checkpatch on everything and did get warnings about it being over
80 characters. But scratched my head a bit of why it complained because
it was not over 80 characters in my editor.

But now I know why. My tab size was set to 4, and then it is not over 80 :).
>
>> +             priv->snor_config |= TEGRA_GMI_RDY_BEFORE_DATA;
>> +
>> +     if (of_property_read_bool(of_node, "nvidia,snor-rdy-inv"))
>> +             priv->snor_config |= TEGRA_GMI_RDY_ACTIVE_HIGH;
>> +
>> +     if (of_property_read_bool(of_node, "nvidia,snor-adv-inv"))
>> +             priv->snor_config |= TEGRA_GMI_ADV_ACTIVE_HIGH;
>> +
>> +     if (of_property_read_bool(of_node, "nvidia,snor-oe-inv"))
>> +             priv->snor_config |= TEGRA_GMI_OE_ACTIVE_HIGH;
>> +
>> +     if (of_property_read_bool(of_node, "nvidia,snor-cs-inv"))
>> +             priv->snor_config |= TEGRA_GMI_CS_ACTIVE_HIGH;
>> +
>> +     if (!of_property_read_u32(of_node, "nvidia,snor-cs-select", &property))
>> +             priv->snor_config |= TEGRA_GMI_CS_SELECT(property);
>> +
>> +     /* Timing, the default values that are provided are reset values */
>> +
>> +     if (!of_property_read_u32(of_node, "nvidia,snor-muxed-width", &property))
>
> Line is over 80 characters.

ACK.

>
>> +             priv->snor_timing0 |= TEGRA_GMI_MUXED_WIDTH(property);
>> +     else
>> +             priv->snor_timing0 |= TEGRA_GMI_MUXED_WIDTH(1);
>> +
>> +     if (!of_property_read_u32(of_node, "nvidia,snor-hold-width", &property))
>> +             priv->snor_timing0 |= TEGRA_GMI_HOLD_WIDTH(property);
>> +     else
>> +             priv->snor_timing0 |= TEGRA_GMI_HOLD_WIDTH(1);
>> +
>> +     if (!of_property_read_u32(of_node, "nvidia,snor-adv-width", &property))
>> +             priv->snor_timing0 |= TEGRA_GMI_ADV_WIDTH(property);
>> +     else
>> +             priv->snor_timing0 |= TEGRA_GMI_ADV_WIDTH(1);
>> +
>> +     if (!of_property_read_u32(of_node, "nvidia,snor-ce-width", &property))
>> +             priv->snor_timing0 |= TEGRA_GMI_CE_WIDTH(property);
>> +     else
>> +             priv->snor_timing0 |= TEGRA_GMI_CE_WIDTH(4);
>> +
>> +     if (!of_property_read_u32(of_node, "nvidia,snor-we-width", &property))
>> +             priv->snor_timing1 |= TEGRA_GMI_WE_WIDTH(property);
>> +     else
>> +             priv->snor_timing1 |= TEGRA_GMI_WE_WIDTH(1);
>> +
>> +     if (!of_property_read_u32(of_node, "nvidia,snor-oe-width", &property))
>> +             priv->snor_timing1 |= TEGRA_GMI_OE_WIDTH(property);
>> +     else
>> +             priv->snor_timing1 |= TEGRA_GMI_OE_WIDTH(1);
>> +
>> +     if (!of_property_read_u32(of_node, "nvidia,snor-wait-width", &property))
>> +             priv->snor_timing1 |= TEGRA_GMI_WAIT_WIDTH(property);
>> +     else
>> +             priv->snor_timing1 |= TEGRA_GMI_WAIT_WIDTH(3);
>> +
>> +     return of_platform_default_populate(of_node, NULL, dev);
>
> Seems odd to do this here. Why not as the last thing in probe once the
> GMI has been setup correctly?

It is done here mostly to avoid clean up in probe. I could move it out
of tegra_gmi_parse_dt and put it right after that call but still
before clk enable? Is that less odd?

>
>> +}
>> +
>> +static int tegra_gmi_probe(struct platform_device *pdev)
>> +{
>> +     struct resource *res;
>> +     struct clk *clk;
>> +     struct device *dev = &pdev->dev;
>> +     struct reset_control *rst;
>> +     struct tegra_gmi_priv *priv;
>> +     void __iomem *base;
>> +     int ret;
>> +
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     base = devm_ioremap_resource(dev, res);
>> +     if (IS_ERR(base))
>> +             return PTR_ERR(base);
>> +
>> +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +     if (!priv)
>> +             return -ENOMEM;
>> +
>> +     priv->base = base;
>
> If you allocate the memory first, you can get rid of this extra base
> variable.

ACK.

>
>> +     clk = devm_clk_get(dev, "gmi");
>> +     if (IS_ERR(clk)) {
>> +             dev_err(dev, "can not get clock\n");
>> +             return PTR_ERR(clk);
>> +     }
>> +
>> +     priv->clk = clk;
>
> Why not set priv->clk directly from calling devm_clk_get? Then you don't
> this extra clk variable.

ACK.
>
>> +
>> +     rst = devm_reset_control_get(dev, "gmi");
>> +     if (IS_ERR(rst)) {
>> +             dev_err(dev, "can not get reset\n");
>> +             return PTR_ERR(rst);
>> +     }
>> +
>> +     ret = tegra_gmi_parse_dt(dev, priv);
>> +     if (ret) {
>> +             dev_err(dev, "fail to create devices.\n");
>> +             return ret;
>> +     }
>> +
>> +     ret = clk_prepare_enable(clk);
>> +     if (ret) {
>> +             dev_err(dev, "fail to enable clock.\n");
>> +             return ret;
>> +     }
>> +
>> +     reset_control_assert(rst);
>> +     udelay(2);
>> +     reset_control_deassert(rst);
>> +
>> +     tegra_gmi_init(dev, priv);
>> +
>> +     dev_set_drvdata(dev, priv);
>> +
>> +     return 0;
>> +}
>> +
>> +static int tegra_gmi_remove(struct platform_device *pdev)
>> +{
>> +     struct device *dev = &pdev->dev;
>
> Do you really need this dev variable?

Not really.

>
>> +     struct tegra_gmi_priv *priv = dev_get_drvdata(dev);
>> +     void __iomem *base = priv->base;
>
> Do you really need this base variable?

Not really,
>
>> +     u32 config;
>> +
>> +     of_platform_depopulate(dev);
>> +
>> +     config = readl(base + TEGRA_GMI_CONFIG);
>> +     config &= ~TEGRA_GMI_CONFIG_GO;
>> +     writel(config, base + TEGRA_GMI_CONFIG);
>> +
>> +     clk_disable_unprepare(priv->clk);
>
> What about asserting the reset?

ACK, will add that.

Best Regards
Mirza

[PATCH 6/6] bus: Add support for Tegra Generic Memory Interface

[Jon Hunter]

On 09/08/16 08:21, Mirza Krak wrote:
> 2016-08-08 15:47 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:

...

>>> +     return of_platform_default_populate(of_node, NULL, dev);
>>
>> Seems odd to do this here. Why not as the last thing in probe once the
>> GMI has been setup correctly?
> 
> It is done here mostly to avoid clean up in probe. I could move it out
> of tegra_gmi_parse_dt and put it right after that call but still
> before clk enable? Is that less odd?

I would have thought that clocks should be enabled and resets
de-asserted before we even attempt to populate any children. Hence, I
would do this last. Yes that will require some clean-up in probe if this
does fail but seems safest.

Cheers
Jon

-- 
nvpublic