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

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

[Mirza Krak]

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

Hi.

This patch series adds support for the Tegra GMI bus controller.

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.

I have re-based on top of latest tegra/for-next in V3. Also see individual
patches for changes in V3.

I have picked up all the comments and suggestions from V2, but I still do not
have an ACK from Rob on the bindings and discussions have halted for some time
now and I hope that this re-send could be the basis for new discussions.

See below links for previous discussions.

Comments on RFC:
https://marc.info/?l=linux-clk&m=146893557629903&w=2
https://marc.info/?l=linux-tegra&m=146893541829801&w=2
https://marc.info/?l=linux-tegra&m=146893542429814&w=2

Comments on V1:
https://marc.info/?l=linux-arm-kernel&m=147051551821122&w=2
https://marc.info/?l=linux-arm-kernel&m=147051553121150&w=2
https://marc.info/?l=linux-arm-kernel&m=147194856600627&w=2
https://marc.info/?l=linux-arm-kernel&m=147072742432211&w=2

Comments on V2:
https://marc.info/?l=devicetree&m=147522253920226&w=2
https://marc.info/?l=linux-tegra&m=147204588027687&w=2
https://marc.info/?l=linux-tegra&m=147204588027687&w=2
https://marc.info/?l=devicetree&m=147256931318922&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 | 132 ++++++++++
 arch/arm/boot/dts/tegra20.dtsi                     |  16 +-
 arch/arm/boot/dts/tegra30.dtsi                     |  13 +
 drivers/bus/Kconfig                                |   8 +
 drivers/bus/Makefile                               |   1 +
 drivers/bus/tegra-gmi.c                            | 267 +++++++++++++++++++++
 drivers/clk/tegra/clk-tegra20.c                    |   1 +
 drivers/clk/tegra/clk-tegra30.c                    |   1 +
 8 files changed, 438 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt
 create mode 100644 drivers/bus/tegra-gmi.c

--
2.1.4

[PATCH V3 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 table and set default rate to 92 MHz which
is max rate.

The maximum rate value of 92 MHz is pulled from the downstream L4T
kernel.

Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board
---

Changes in v2:
- no changes

Changes in v3:
- Added comment in commit message where I got the maximum rates from.

 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

Re: [PATCH V3 1/6] clk: tegra: add TEGRA20_CLK_NOR to init table

[Jon Hunter]

Hi Mirza,

On 27/10/16 15:01, Mirza Krak wrote:
> From: Mirza Krak <mirza.krak@gmail.com>
>
> Add TEGRA20_CLK_NOR to init table and set default rate to 92 MHz which
> is max rate.
>
> The maximum rate value of 92 MHz is pulled from the downstream L4T
> kernel.

Thanks for adding this. I assume that this is from an L4T r16 release 
with a v3.1 kernel. I had a quick poke through the kernel sources for 
v3.1 but was unable to see where this is set. Obviously v3.1 did not 
have CCF and so everything seems to be in the arch/arm/mach-tegra 
directory for setting up clocks. Can you point me to the appropriate 
sources so I can ACK this?

Cheers
Jon

-- 
nvpublic

Re: [PATCH V3 1/6] clk: tegra: add TEGRA20_CLK_NOR to init table

[Mirza Krak]

2016-11-03 11:06 GMT+01:00 Jon Hunter <jonathanh@nvidia.com>:
> Hi Mirza,
>
> On 27/10/16 15:01, Mirza Krak wrote:
>>
>> From: Mirza Krak <mirza.krak@gmail.com>
>>
>> Add TEGRA20_CLK_NOR to init table and set default rate to 92 MHz which
>> is max rate.
>>
>> The maximum rate value of 92 MHz is pulled from the downstream L4T
>> kernel.
>
>
> Thanks for adding this. I assume that this is from an L4T r16 release with a
> v3.1 kernel. I had a quick poke through the kernel sources for v3.1 but was
> unable to see where this is set. Obviously v3.1 did not have CCF and so
> everything seems to be in the arch/arm/mach-tegra directory for setting up
> clocks. Can you point me to the appropriate sources so I can ACK this?

I use the kernel sources provided by Toradex, and these sources are
based on L4T r16 release.

http://git.toradex.com/cgit/linux-toradex.git/tree/arch/arm/mach-tegra/tegra2_clocks.c?h=tegra#n2463



Best Regards
Mirza Krak

Re: [PATCH V3 1/6] clk: tegra: add TEGRA20_CLK_NOR to init table

[Mirza Krak]

2016-11-03 13:26 GMT+01:00 Mirza Krak <mirza.krak@gmail.com>:
> 2016-11-03 11:06 GMT+01:00 Jon Hunter <jonathanh@nvidia.com>:
>> Hi Mirza,
>>
>> On 27/10/16 15:01, Mirza Krak wrote:
>>>
>>> From: Mirza Krak <mirza.krak@gmail.com>
>>>
>>> Add TEGRA20_CLK_NOR to init table and set default rate to 92 MHz which
>>> is max rate.
>>>
>>> The maximum rate value of 92 MHz is pulled from the downstream L4T
>>> kernel.
>>
>>
>> Thanks for adding this. I assume that this is from an L4T r16 release with a
>> v3.1 kernel. I had a quick poke through the kernel sources for v3.1 but was
>> unable to see where this is set. Obviously v3.1 did not have CCF and so
>> everything seems to be in the arch/arm/mach-tegra directory for setting up
>> clocks. Can you point me to the appropriate sources so I can ACK this?
>
> I use the kernel sources provided by Toradex, and these sources are
> based on L4T r16 release.
>
> http://git.toradex.com/cgit/linux-toradex.git/tree/arch/arm/mach-tegra/tegra2_clocks.c?h=tegra#n2463

Ops, pre-mature send.

I also added Marcel from Toradex on CC.

The link to the source are [1] for Tegra2 and  [2] for Tegra3.

[1]. http://git.toradex.com/cgit/linux-toradex.git/tree/arch/arm/mach-tegra/tegra2_clocks.c?h=tegra#n2463
[2]. http://git.toradex.com/cgit/linux-toradex.git/tree/arch/arm/mach-tegra/tegra3_clocks.c?h=tegra#n4353

Best Regards
Mirza

Re: [PATCH V3 1/6] clk: tegra: add TEGRA20_CLK_NOR to init table

[Jon Hunter]

On 03/11/16 12:30, Mirza Krak wrote:
> 2016-11-03 13:26 GMT+01:00 Mirza Krak <mirza.krak@gmail.com>:
>> 2016-11-03 11:06 GMT+01:00 Jon Hunter <jonathanh@nvidia.com>:
>>> Hi Mirza,
>>>
>>> On 27/10/16 15:01, Mirza Krak wrote:
>>>>
>>>> From: Mirza Krak <mirza.krak@gmail.com>
>>>>
>>>> Add TEGRA20_CLK_NOR to init table and set default rate to 92 MHz which
>>>> is max rate.
>>>>
>>>> The maximum rate value of 92 MHz is pulled from the downstream L4T
>>>> kernel.
>>>
>>>
>>> Thanks for adding this. I assume that this is from an L4T r16 release with a
>>> v3.1 kernel. I had a quick poke through the kernel sources for v3.1 but was
>>> unable to see where this is set. Obviously v3.1 did not have CCF and so
>>> everything seems to be in the arch/arm/mach-tegra directory for setting up
>>> clocks. Can you point me to the appropriate sources so I can ACK this?
>>
>> I use the kernel sources provided by Toradex, and these sources are
>> based on L4T r16 release.
>>
>> http://git.toradex.com/cgit/linux-toradex.git/tree/arch/arm/mach-tegra/tegra2_clocks.c?h=tegra#n2463
>
> Ops, pre-mature send.
>
> I also added Marcel from Toradex on CC.
>
> The link to the source are [1] for Tegra2 and  [2] for Tegra3.
>
> [1]. http://git.toradex.com/cgit/linux-toradex.git/tree/arch/arm/mach-tegra/tegra2_clocks.c?h=tegra#n2463
> [2]. http://git.toradex.com/cgit/linux-toradex.git/tree/arch/arm/mach-tegra/tegra3_clocks.c?h=tegra#n4353

Great. Yes I see the same. Thanks!

Jon

-- 
nvpublic

Re: [PATCH V3 1/6] clk: tegra: add TEGRA20_CLK_NOR to init table

[Jon Hunter]

On 27/10/16 15:01, Mirza Krak wrote:
> From: Mirza Krak <mirza.krak@gmail.com>
>
> Add TEGRA20_CLK_NOR to init table and set default rate to 92 MHz which
> is max rate.
>
> The maximum rate value of 92 MHz is pulled from the downstream L4T
> kernel.
>
> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
> Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

[PATCH V3 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.

The maximum rate value of 127 MHz is pulled from the downstream L4T
kernel.

Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board
---

Changes in v2:
- no changes

Changes in v3:
- Added comment in commit message where I got the maximum rates from.

 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

Re: [PATCH V3 2/6] clk: tegra: add TEGRA30_CLK_NOR to init table

[Jon Hunter]

On 27/10/16 15:01, Mirza Krak wrote:
> 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.
>
> The maximum rate value of 127 MHz is pulled from the downstream L4T
> kernel.
>
> Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
> Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

[PATCH V3 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>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board
---

Changes in v2:
- Updated examples and some information based on comments from Jon Hunter.

Changes in v3:
- Updates ranges description based on comments from Rob Herring

 .../devicetree/bindings/bus/nvidia,tegra20-gmi.txt | 132 +++++++++++++++++++++
 1 file changed, 132 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..49bda2f
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt
@@ -0,0 +1,132 @@
+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. Should be 2.
+ - #size-cells: The number of cells used to represent the size of an address
+   range in the GMI address space. Should be 1.
+ - ranges: Must be set up to reflect the memory layout with three integer values
+   for each chip-select line in use (only one entry is supported, see below
+   comments):
+   <cs-number> <offset> <physical address of mapping> <size>
+
+Note that the GMI controller does not have any internal chip-select address
+decoding, because of that chip-selects either need to be managed via software
+or by employing external chip-select decoding logic.
+
+If external chip-select logic is used to support multiple devices it is assumed
+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. In this case only one
+child device is supported which represents the active chip-select line, see
+examples for more insight.
+
+The chip-select number is decoded from the child nodes second address cell of
+'ranges' property, if 'ranges' property is not present or empty chip-select will
+then be decoded from the first cell of the 'reg' property.
+
+Optional child cs node 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
+
+  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-15, 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. We wrap the
+controllers with a simple-bus node since they are all connected to the same
+chip-select (CS4), in this example external address decoding is provided:
+
+gmi@70090000 {
+	compatible = "nvidia,tegra20-gmi";
+	reg = <0x70009000 0x1000>;
+	#address-cells = <2>;
+	#size-cells = <1>;
+	clocks = <&tegra_car TEGRA20_CLK_NOR>;
+	clock-names = "gmi";
+	resets = <&tegra_car 42>;
+	reset-names = "gmi";
+	ranges = <4 0 0xd0000000 0xfffffff>;
+
+	status = "okay";
+
+	bus@4,0 {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0 4 0 0x40100>;
+
+		nvidia,snor-mux-mode;
+		nvidia,snor-adv-inv;
+
+		can@0 {
+			reg = <0 0x100>;
+			...
+		};
+
+		can@40000 {
+			reg = <0x40000 0x100>;
+			...
+		};
+	};
+};
+
+Example with one SJA1000 CAN controller connected to the GMI bus
+on CS4:
+
+gmi@70090000 {
+	compatible = "nvidia,tegra20-gmi";
+	reg = <0x70009000 0x1000>;
+	#address-cells = <2>;
+	#size-cells = <1>;
+	clocks = <&tegra_car TEGRA20_CLK_NOR>;
+	clock-names = "gmi";
+	resets = <&tegra_car 42>;
+	reset-names = "gmi";
+	ranges = <4 0 0xd0000000 0xfffffff>;
+
+	status = "okay";
+
+	can@4,0 {
+		reg = <4 0 0x100>;
+		nvidia,snor-mux-mode;
+		nvidia,snor-adv-inv;
+		...
+	};
+};
--
2.1.4

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

[Rob Herring]

On Thu, Oct 27, 2016 at 04:01:09PM +0200, 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>
> Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board
> ---
> 
> Changes in v2:
> - Updated examples and some information based on comments from Jon Hunter.
> 
> Changes in v3:
> - Updates ranges description based on comments from Rob Herring
> 
>  .../devicetree/bindings/bus/nvidia,tegra20-gmi.txt | 132 +++++++++++++++++++++
>  1 file changed, 132 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..49bda2f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt
> @@ -0,0 +1,132 @@
> +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. Should be 2.
> + - #size-cells: The number of cells used to represent the size of an address
> +   range in the GMI address space. Should be 1.
> + - ranges: Must be set up to reflect the memory layout with three integer values
> +   for each chip-select line in use (only one entry is supported, see below
> +   comments):
> +   <cs-number> <offset> <physical address of mapping> <size>
> +
> +Note that the GMI controller does not have any internal chip-select address
> +decoding, because of that chip-selects either need to be managed via software
> +or by employing external chip-select decoding logic.
> +
> +If external chip-select logic is used to support multiple devices it is assumed
> +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. In this case only one
> +child device is supported which represents the active chip-select line, see
> +examples for more insight.
> +
> +The chip-select number is decoded from the child nodes second address cell of
> +'ranges' property, if 'ranges' property is not present or empty chip-select will
> +then be decoded from the first cell of the 'reg' property.
> +
> +Optional child cs node 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

Inverted is meaningless unless I know what not inverted state is. Name 
the properties using "active high".

With that,

Acked-by: Rob Herring <robh@kernel.org>

[PATCH V3 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>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board
---

Changes in v2:
- added address-cells, size-cells and ranges properties

Changes in v3:
- no changes

 arch/arm/boot/dts/tegra30.dtsi | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 5030065..bbb1c00 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -439,6 +439,19 @@
 		status = "disabled";
 	};

+	gmi@70009000 {
+		compatible = "nvidia,tegra30-gmi";
+		reg = <0x70009000 0x1000>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+		ranges = <0 0 0x48000000 0x7ffffff>;
+		clocks = <&tegra_car TEGRA30_CLK_NOR>;
+		clock-names = "gmi";
+		resets = <&tegra_car 42>;
+		reset-names = "gmi";
+		status = "disabled";
+	};
+
 	pwm: pwm@7000a000 {
 		compatible = "nvidia,tegra30-pwm", "nvidia,tegra20-pwm";
 		reg = <0x7000a000 0x100>;
--
2.1.4

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

[Jon Hunter]

On 27/10/16 15:01, 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>
> Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board
> ---
>
> Changes in v2:
> - added address-cells, size-cells and ranges properties
>
> Changes in v3:
> - no changes
>
>  arch/arm/boot/dts/tegra30.dtsi | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
> index 5030065..bbb1c00 100644
> --- a/arch/arm/boot/dts/tegra30.dtsi
> +++ b/arch/arm/boot/dts/tegra30.dtsi
> @@ -439,6 +439,19 @@
>  		status = "disabled";
>  	};
>
> +	gmi@70009000 {
> +		compatible = "nvidia,tegra30-gmi";
> +		reg = <0x70009000 0x1000>;
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		ranges = <0 0 0x48000000 0x7ffffff>;
> +		clocks = <&tegra_car TEGRA30_CLK_NOR>;
> +		clock-names = "gmi";
> +		resets = <&tegra_car 42>;
> +		reset-names = "gmi";
> +		status = "disabled";
> +	};
> +

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

[PATCH V3 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>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board
---

Changes in v2:
- added address-cells, size-cells and ranges properties

Changes in v3:
- fixed range address which is not the same as Tegra30.

 arch/arm/boot/dts/tegra20.dtsi | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 2207c08..b22cddb 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -1,4 +1,4 @@
-#include <dt-bindings/clock/tegra20-car.h>
+include <dt-bindings/clock/tegra20-car.h>
 #include <dt-bindings/gpio/tegra-gpio.h>
 #include <dt-bindings/pinctrl/pinctrl-tegra.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
@@ -376,6 +376,20 @@
 		status = "disabled";
 	};

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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]

Hi Mirza,

[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.9-rc2 next-20161027]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Mirza-Krak/clk-tegra-add-TEGRA20_CLK_NOR-to-init-table/20161027-225528
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arm-davinci_all_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

>> Error: arch/arm/boot/dts/tegra20.dtsi:1.1-8 syntax error
   FATAL ERROR: Unable to parse input tree

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 20355 bytes --]

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

[Mirza Krak]

2016-10-27 19:09 GMT+02:00 kbuild test robot <lkp@intel.com>:
> Hi Mirza,
>
> [auto build test ERROR on robh/for-next]
> [also build test ERROR on v4.9-rc2 next-20161027]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> [Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
> [Check https://git-scm.com/docs/git-format-patch for more information]
>
> url:    https://github.com/0day-ci/linux/commits/Mirza-Krak/clk-tegra-add-TEGRA20_CLK_NOR-to-init-table/20161027-225528
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
> config: arm-davinci_all_defconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=arm
>
> All errors (new ones prefixed by >>):
>
>>> Error: arch/arm/boot/dts/tegra20.dtsi:1.1-8 syntax error
>    FATAL ERROR: Unable to parse input tree

That one was embarrassing. Sorry...

Best Regards
Mirza

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

[Jon Hunter]

On 27/10/16 15:01, 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>
> Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board
> ---
>
> Changes in v2:
> - added address-cells, size-cells and ranges properties
>
> Changes in v3:
> - fixed range address which is not the same as Tegra30.
>
>  arch/arm/boot/dts/tegra20.dtsi | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> index 2207c08..b22cddb 100644
> --- a/arch/arm/boot/dts/tegra20.dtsi
> +++ b/arch/arm/boot/dts/tegra20.dtsi
> @@ -1,4 +1,4 @@
> -#include <dt-bindings/clock/tegra20-car.h>
> +include <dt-bindings/clock/tegra20-car.h>

After fixing up this, can you also ...

>  #include <dt-bindings/gpio/tegra-gpio.h>
>  #include <dt-bindings/pinctrl/pinctrl-tegra.h>
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
> @@ -376,6 +376,20 @@
>  		status = "disabled";
>  	};
>
> +

Drop this additional line?

> +	gmi@70009000 {
> +		compatible = "nvidia,tegra20-gmi";
> +		reg = <0x70009000 0x1000>;
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		ranges = <0 0 0xd0000000 0xfffffff>;
> +		clocks = <&tegra_car TEGRA20_CLK_NOR>;
> +		clock-names = "gmi";
> +		resets = < &tegra_car 42>;

Get rid of this additional space?

> +		reset-names = "gmi";
> +		status = "disabled";
> +	};
> +

Otherwise ...

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

[PATCH V3 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>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board
---

Changes in v2:
 - Fixed some checkpatch errors
 - Re-ordered probe to get rid of local variables
 - Moved of_platform_default_populate call to the end of probe
 - Use the timing and configuration properties from the child device
 - Added warning if more then 1 child device exist

Changes in v3:
 - added helper function to disable the controller which is used in remove and
 on error.
 - Added logic to parse CS# from "ranges" property with fallback to "reg"
 property

 drivers/bus/Kconfig     |   8 ++
 drivers/bus/Makefile    |   1 +
 drivers/bus/tegra-gmi.c | 267 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 276 insertions(+)
 create mode 100644 drivers/bus/tegra-gmi.c

diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 4ed7d26..2e75a7f 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -141,6 +141,14 @@ 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..dd9623e
--- /dev/null
+++ b/drivers/bus/tegra-gmi.c
@@ -0,0 +1,267 @@
+/*
+ * 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;
+	struct reset_control *rst;
+	struct clk *clk;
+
+	u32 snor_config;
+	u32 snor_timing0;
+	u32 snor_timing1;
+};
+
+static void tegra_gmi_disable(struct tegra_gmi_priv *priv)
+{
+	u32 config;
+
+	/* stop GMI operation */
+	config = readl(priv->base + TEGRA_GMI_CONFIG);
+	config &= ~TEGRA_GMI_CONFIG_GO;
+	writel(config, priv->base + TEGRA_GMI_CONFIG);
+
+	reset_control_assert(priv->rst);
+	clk_disable_unprepare(priv->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 *child = of_get_next_available_child(dev->of_node,
+		NULL);
+	u32 property, ranges[4];
+	int ret;
+
+	if (!child) {
+		dev_warn(dev, "no child nodes found\n");
+		return 0;
+	}
+
+	/*
+	 * We currently only support one child device due to lack of
+	 * chip-select address decoding. Which means that we only have one
+	 * chip-select line from the GMI controller.
+	 */
+	if (of_get_child_count(dev->of_node) > 1)
+		dev_warn(dev, "only one child device is supported.");
+
+	if (of_property_read_bool(child, "nvidia,snor-data-width-32bit"))
+		priv->snor_config |= TEGRA_GMI_BUS_WIDTH_32BIT;
+
+	if (of_property_read_bool(child, "nvidia,snor-mux-mode"))
+		priv->snor_config |= TEGRA_GMI_MUX_MODE;
+
+	if (of_property_read_bool(child, "nvidia,snor-rdy-active-before-data"))
+		priv->snor_config |= TEGRA_GMI_RDY_BEFORE_DATA;
+
+	if (of_property_read_bool(child, "nvidia,snor-rdy-inv"))
+		priv->snor_config |= TEGRA_GMI_RDY_ACTIVE_HIGH;
+
+	if (of_property_read_bool(child, "nvidia,snor-adv-inv"))
+		priv->snor_config |= TEGRA_GMI_ADV_ACTIVE_HIGH;
+
+	if (of_property_read_bool(child, "nvidia,snor-oe-inv"))
+		priv->snor_config |= TEGRA_GMI_OE_ACTIVE_HIGH;
+
+	if (of_property_read_bool(child, "nvidia,snor-cs-inv"))
+		priv->snor_config |= TEGRA_GMI_CS_ACTIVE_HIGH;
+
+	/* Decode the CS# */
+	ret = of_property_read_u32_array(child, "ranges", ranges, 4);
+	if (ret < 0) {
+		/* Invalid binding */
+		if (ret == -EOVERFLOW) {
+			dev_err(dev, "invalid ranges length\n");
+			goto error_cs_decode;
+		}
+
+		/*
+		 * If we reach here it means that the child node has an empty
+		 * ranges or it does not exist at all. Attempt to decode the
+		 * CS# from the reg property instead.
+		 */
+		ret = of_property_read_u32(child, "reg", &property);
+		if (ret < 0) {
+			dev_err(dev, "no reg property found\n");
+			goto error_cs_decode;
+		}
+	} else {
+		property = ranges[1];
+	}
+
+	priv->snor_config |= TEGRA_GMI_CS_SELECT(property);
+
+	/* The default values that are provided below are reset values */
+	if (!of_property_read_u32(child, "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(child, "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(child, "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(child, "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(child, "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(child, "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(child, "nvidia,snor-wait-width", &property))
+		priv->snor_timing1 |= TEGRA_GMI_WAIT_WIDTH(property);
+	else
+		priv->snor_timing1 |= TEGRA_GMI_WAIT_WIDTH(3);
+
+error_cs_decode:
+	if (ret < 0)
+		dev_err(dev, "failed to decode chip-select number\n");
+
+	of_node_put(child);
+	return ret;
+}
+
+static int tegra_gmi_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct device *dev = &pdev->dev;
+	struct tegra_gmi_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->clk = devm_clk_get(dev, "gmi");
+	if (IS_ERR(priv->clk)) {
+		dev_err(dev, "can not get clock\n");
+		return PTR_ERR(priv->clk);
+	}
+
+	priv->rst = devm_reset_control_get(dev, "gmi");
+	if (IS_ERR(priv->rst)) {
+		dev_err(dev, "can not get reset\n");
+		return PTR_ERR(priv->rst);
+	}
+
+	ret = tegra_gmi_parse_dt(dev, priv);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret) {
+		dev_err(dev, "fail to enable clock.\n");
+		return ret;
+	}
+
+	reset_control_assert(priv->rst);
+	udelay(2);
+	reset_control_deassert(priv->rst);
+
+	tegra_gmi_init(dev, priv);
+
+	ret = of_platform_default_populate(dev->of_node, NULL, dev);
+	if (ret < 0) {
+		dev_err(dev, "fail to create devices.\n");
+		tegra_gmi_disable(priv);
+		return ret;
+	}
+
+	dev_set_drvdata(dev, priv);
+
+	return 0;
+}
+
+static int tegra_gmi_remove(struct platform_device *pdev)
+{
+	struct tegra_gmi_priv *priv = dev_get_drvdata(&pdev->dev);
+
+	of_platform_depopulate(&pdev->dev);
+
+	tegra_gmi_disable(priv);
+
+	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

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

[Jon Hunter]

On 27/10/16 15:01, 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>
> Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board
> ---
>
> Changes in v2:
>  - Fixed some checkpatch errors
>  - Re-ordered probe to get rid of local variables
>  - Moved of_platform_default_populate call to the end of probe
>  - Use the timing and configuration properties from the child device
>  - Added warning if more then 1 child device exist
>
> Changes in v3:
>  - added helper function to disable the controller which is used in remove and
>  on error.
>  - Added logic to parse CS# from "ranges" property with fallback to "reg"
>  property
>
>  drivers/bus/Kconfig     |   8 ++
>  drivers/bus/Makefile    |   1 +
>  drivers/bus/tegra-gmi.c | 267 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 276 insertions(+)
>  create mode 100644 drivers/bus/tegra-gmi.c
>
> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
> index 4ed7d26..2e75a7f 100644
> --- a/drivers/bus/Kconfig
> +++ b/drivers/bus/Kconfig
> @@ -141,6 +141,14 @@ 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.
> +
> +

Nit-pick ... only one additional line above is needed to be consistent 
with the rest of the file.

>  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..dd9623e
> --- /dev/null
> +++ b/drivers/bus/tegra-gmi.c
> @@ -0,0 +1,267 @@
> +/*
> + * 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;
> +	struct reset_control *rst;
> +	struct clk *clk;
> +
> +	u32 snor_config;
> +	u32 snor_timing0;
> +	u32 snor_timing1;
> +};
> +
> +static void tegra_gmi_disable(struct tegra_gmi_priv *priv)
> +{
> +	u32 config;
> +
> +	/* stop GMI operation */
> +	config = readl(priv->base + TEGRA_GMI_CONFIG);
> +	config &= ~TEGRA_GMI_CONFIG_GO;
> +	writel(config, priv->base + TEGRA_GMI_CONFIG);
> +
> +	reset_control_assert(priv->rst);
> +	clk_disable_unprepare(priv->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 *child = of_get_next_available_child(dev->of_node,
> +		NULL);
> +	u32 property, ranges[4];
> +	int ret;
> +
> +	if (!child) {
> +		dev_warn(dev, "no child nodes found\n");
> +		return 0;

Don't we want to return an error here? Otherwise, we will call 
tegra_gmi_init() with an invalid configuration.

> +	}
> +
> +	/*
> +	 * We currently only support one child device due to lack of
> +	 * chip-select address decoding. Which means that we only have one
> +	 * chip-select line from the GMI controller.
> +	 */
> +	if (of_get_child_count(dev->of_node) > 1)
> +		dev_warn(dev, "only one child device is supported.");
> +
> +	if (of_property_read_bool(child, "nvidia,snor-data-width-32bit"))
> +		priv->snor_config |= TEGRA_GMI_BUS_WIDTH_32BIT;
> +
> +	if (of_property_read_bool(child, "nvidia,snor-mux-mode"))
> +		priv->snor_config |= TEGRA_GMI_MUX_MODE;
> +
> +	if (of_property_read_bool(child, "nvidia,snor-rdy-active-before-data"))
> +		priv->snor_config |= TEGRA_GMI_RDY_BEFORE_DATA;
> +
> +	if (of_property_read_bool(child, "nvidia,snor-rdy-inv"))
> +		priv->snor_config |= TEGRA_GMI_RDY_ACTIVE_HIGH;
> +
> +	if (of_property_read_bool(child, "nvidia,snor-adv-inv"))
> +		priv->snor_config |= TEGRA_GMI_ADV_ACTIVE_HIGH;
> +
> +	if (of_property_read_bool(child, "nvidia,snor-oe-inv"))
> +		priv->snor_config |= TEGRA_GMI_OE_ACTIVE_HIGH;
> +
> +	if (of_property_read_bool(child, "nvidia,snor-cs-inv"))
> +		priv->snor_config |= TEGRA_GMI_CS_ACTIVE_HIGH;
> +
> +	/* Decode the CS# */
> +	ret = of_property_read_u32_array(child, "ranges", ranges, 4);
> +	if (ret < 0) {
> +		/* Invalid binding */
> +		if (ret == -EOVERFLOW) {
> +			dev_err(dev, "invalid ranges length\n");
> +			goto error_cs_decode;
> +		}
> +
> +		/*
> +		 * If we reach here it means that the child node has an empty
> +		 * ranges or it does not exist at all. Attempt to decode the
> +		 * CS# from the reg property instead.
> +		 */
> +		ret = of_property_read_u32(child, "reg", &property);
> +		if (ret < 0) {
> +			dev_err(dev, "no reg property found\n");
> +			goto error_cs_decode;
> +		}
> +	} else {
> +		property = ranges[1];
> +	}
> +
> +	priv->snor_config |= TEGRA_GMI_CS_SELECT(property);

Should we make sure the CS is a valid value before setting?

> +
> +	/* The default values that are provided below are reset values */
> +	if (!of_property_read_u32(child, "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(child, "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(child, "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(child, "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(child, "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(child, "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(child, "nvidia,snor-wait-width", &property))
> +		priv->snor_timing1 |= TEGRA_GMI_WAIT_WIDTH(property);
> +	else
> +		priv->snor_timing1 |= TEGRA_GMI_WAIT_WIDTH(3);
> +
> +error_cs_decode:
> +	if (ret < 0)
> +		dev_err(dev, "failed to decode chip-select number\n");

Nit do we need another error message here? Can we add the "failed to 
decode CS" part the earlier message?

> +
> +	of_node_put(child);
> +	return ret;
> +}
> +
> +static int tegra_gmi_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct device *dev = &pdev->dev;
> +	struct tegra_gmi_priv *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	priv->clk = devm_clk_get(dev, "gmi");
> +	if (IS_ERR(priv->clk)) {
> +		dev_err(dev, "can not get clock\n");
> +		return PTR_ERR(priv->clk);
> +	}
> +
> +	priv->rst = devm_reset_control_get(dev, "gmi");
> +	if (IS_ERR(priv->rst)) {
> +		dev_err(dev, "can not get reset\n");
> +		return PTR_ERR(priv->rst);
> +	}
> +
> +	ret = tegra_gmi_parse_dt(dev, priv);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret) {
> +		dev_err(dev, "fail to enable clock.\n");
> +		return ret;
> +	}
> +
> +	reset_control_assert(priv->rst);
> +	udelay(2);
> +	reset_control_deassert(priv->rst);
> +
> +	tegra_gmi_init(dev, priv);
> +
> +	ret = of_platform_default_populate(dev->of_node, NULL, dev);
> +	if (ret < 0) {
> +		dev_err(dev, "fail to create devices.\n");
> +		tegra_gmi_disable(priv);
> +		return ret;
> +	}
> +
> +	dev_set_drvdata(dev, priv);
> +
> +	return 0;
> +}
> +
> +static int tegra_gmi_remove(struct platform_device *pdev)
> +{
> +	struct tegra_gmi_priv *priv = dev_get_drvdata(&pdev->dev);
> +
> +	of_platform_depopulate(&pdev->dev);
> +
> +	tegra_gmi_disable(priv);
> +
> +	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
>

Cheers
Jon

-- 
nvpublic

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

[Mirza Krak]

2016-11-03 11:51 GMT+01:00 Jon Hunter <jonathanh@nvidia.com>:
>
> On 27/10/16 15:01, 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>
>> Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
>> Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board
>> ---
>>
>> Changes in v2:
>>  - Fixed some checkpatch errors
>>  - Re-ordered probe to get rid of local variables
>>  - Moved of_platform_default_populate call to the end of probe
>>  - Use the timing and configuration properties from the child device
>>  - Added warning if more then 1 child device exist
>>
>> Changes in v3:
>>  - added helper function to disable the controller which is used in remove
>> and
>>  on error.
>>  - Added logic to parse CS# from "ranges" property with fallback to "reg"
>>  property
>>
>>  drivers/bus/Kconfig     |   8 ++
>>  drivers/bus/Makefile    |   1 +
>>  drivers/bus/tegra-gmi.c | 267
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 276 insertions(+)
>>  create mode 100644 drivers/bus/tegra-gmi.c
>>
>> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
>> index 4ed7d26..2e75a7f 100644
>> --- a/drivers/bus/Kconfig
>> +++ b/drivers/bus/Kconfig
>> @@ -141,6 +141,14 @@ 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.
>> +
>> +
>
>
> Nit-pick ... only one additional line above is needed to be consistent with
> the rest of the file.

Will fix that.

>
>
>>  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..dd9623e
>> --- /dev/null
>> +++ b/drivers/bus/tegra-gmi.c
>> @@ -0,0 +1,267 @@
>> +/*
>> + * 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;
>> +       struct reset_control *rst;
>> +       struct clk *clk;
>> +
>> +       u32 snor_config;
>> +       u32 snor_timing0;
>> +       u32 snor_timing1;
>> +};
>> +
>> +static void tegra_gmi_disable(struct tegra_gmi_priv *priv)
>> +{
>> +       u32 config;
>> +
>> +       /* stop GMI operation */
>> +       config = readl(priv->base + TEGRA_GMI_CONFIG);
>> +       config &= ~TEGRA_GMI_CONFIG_GO;
>> +       writel(config, priv->base + TEGRA_GMI_CONFIG);
>> +
>> +       reset_control_assert(priv->rst);
>> +       clk_disable_unprepare(priv->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 *child =
>> of_get_next_available_child(dev->of_node,
>> +               NULL);
>> +       u32 property, ranges[4];
>> +       int ret;
>> +
>> +       if (!child) {
>> +               dev_warn(dev, "no child nodes found\n");
>> +               return 0;
>
>
> Don't we want to return an error here? Otherwise, we will call
> tegra_gmi_init() with an invalid configuration.

True, we probably want that. My thought was that we might accept a
tegra-gmi node without any child nodes and just print a warning. But
since it is the child node that holds the bus configuration it makes
sense to fail probe due to no child nodes.

>
>
>> +       }
>> +
>> +       /*
>> +        * We currently only support one child device due to lack of
>> +        * chip-select address decoding. Which means that we only have one
>> +        * chip-select line from the GMI controller.
>> +        */
>> +       if (of_get_child_count(dev->of_node) > 1)
>> +               dev_warn(dev, "only one child device is supported.");
>> +
>> +       if (of_property_read_bool(child, "nvidia,snor-data-width-32bit"))
>> +               priv->snor_config |= TEGRA_GMI_BUS_WIDTH_32BIT;
>> +
>> +       if (of_property_read_bool(child, "nvidia,snor-mux-mode"))
>> +               priv->snor_config |= TEGRA_GMI_MUX_MODE;
>> +
>> +       if (of_property_read_bool(child,
>> "nvidia,snor-rdy-active-before-data"))
>> +               priv->snor_config |= TEGRA_GMI_RDY_BEFORE_DATA;
>> +
>> +       if (of_property_read_bool(child, "nvidia,snor-rdy-inv"))
>> +               priv->snor_config |= TEGRA_GMI_RDY_ACTIVE_HIGH;
>> +
>> +       if (of_property_read_bool(child, "nvidia,snor-adv-inv"))
>> +               priv->snor_config |= TEGRA_GMI_ADV_ACTIVE_HIGH;
>> +
>> +       if (of_property_read_bool(child, "nvidia,snor-oe-inv"))
>> +               priv->snor_config |= TEGRA_GMI_OE_ACTIVE_HIGH;
>> +
>> +       if (of_property_read_bool(child, "nvidia,snor-cs-inv"))
>> +               priv->snor_config |= TEGRA_GMI_CS_ACTIVE_HIGH;
>> +
>> +       /* Decode the CS# */
>> +       ret = of_property_read_u32_array(child, "ranges", ranges, 4);
>> +       if (ret < 0) {
>> +               /* Invalid binding */
>> +               if (ret == -EOVERFLOW) {
>> +                       dev_err(dev, "invalid ranges length\n");
>> +                       goto error_cs_decode;
>> +               }
>> +
>> +               /*
>> +                * If we reach here it means that the child node has an
>> empty
>> +                * ranges or it does not exist at all. Attempt to decode
>> the
>> +                * CS# from the reg property instead.
>> +                */
>> +               ret = of_property_read_u32(child, "reg", &property);
>> +               if (ret < 0) {
>> +                       dev_err(dev, "no reg property found\n");
>> +                       goto error_cs_decode;
>> +               }
>> +       } else {
>> +               property = ranges[1];
>> +       }
>> +
>> +       priv->snor_config |= TEGRA_GMI_CS_SELECT(property);
>
>
> Should we make sure the CS is a valid value before setting?

The TEGRA_GMI_CS_SELECT(x) macro will truncate any erroneous CS value.
But yeah we could do a sanity check instead and return an error if it
is invalid.

>
>
>> +
>> +       /* The default values that are provided below are reset values */
>> +       if (!of_property_read_u32(child, "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(child, "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(child, "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(child, "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(child, "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(child, "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(child, "nvidia,snor-wait-width",
>> &property))
>> +               priv->snor_timing1 |= TEGRA_GMI_WAIT_WIDTH(property);
>> +       else
>> +               priv->snor_timing1 |= TEGRA_GMI_WAIT_WIDTH(3);
>> +
>> +error_cs_decode:
>> +       if (ret < 0)
>> +               dev_err(dev, "failed to decode chip-select number\n");
>
>
> Nit do we need another error message here? Can we add the "failed to decode
> CS" part the earlier message?

Does it make sense to drop the two earlier messages instead and keep this one?

Best Regards
Mirza

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

[Jon Hunter]

On 03/11/16 13:08, Mirza Krak wrote:
> 2016-11-03 11:51 GMT+01:00 Jon Hunter <jonathanh@nvidia.com>:
>>
>> On 27/10/16 15:01, 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>
>>> Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
>>> Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board
>>> ---
>>>
>>> Changes in v2:
>>>  - Fixed some checkpatch errors
>>>  - Re-ordered probe to get rid of local variables
>>>  - Moved of_platform_default_populate call to the end of probe
>>>  - Use the timing and configuration properties from the child device
>>>  - Added warning if more then 1 child device exist
>>>
>>> Changes in v3:
>>>  - added helper function to disable the controller which is used in remove
>>> and
>>>  on error.
>>>  - Added logic to parse CS# from "ranges" property with fallback to "reg"
>>>  property
>>>
>>>  drivers/bus/Kconfig     |   8 ++
>>>  drivers/bus/Makefile    |   1 +
>>>  drivers/bus/tegra-gmi.c | 267
>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 276 insertions(+)
>>>  create mode 100644 drivers/bus/tegra-gmi.c
>>>
>>> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
>>> index 4ed7d26..2e75a7f 100644
>>> --- a/drivers/bus/Kconfig
>>> +++ b/drivers/bus/Kconfig
>>> @@ -141,6 +141,14 @@ 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.
>>> +
>>> +
>>
>>
>> Nit-pick ... only one additional line above is needed to be consistent with
>> the rest of the file.
>
> Will fix that.
>
>>
>>
>>>  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..dd9623e
>>> --- /dev/null
>>> +++ b/drivers/bus/tegra-gmi.c
>>> @@ -0,0 +1,267 @@
>>> +/*
>>> + * 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;
>>> +       struct reset_control *rst;
>>> +       struct clk *clk;
>>> +
>>> +       u32 snor_config;
>>> +       u32 snor_timing0;
>>> +       u32 snor_timing1;
>>> +};
>>> +
>>> +static void tegra_gmi_disable(struct tegra_gmi_priv *priv)
>>> +{
>>> +       u32 config;
>>> +
>>> +       /* stop GMI operation */
>>> +       config = readl(priv->base + TEGRA_GMI_CONFIG);
>>> +       config &= ~TEGRA_GMI_CONFIG_GO;
>>> +       writel(config, priv->base + TEGRA_GMI_CONFIG);
>>> +
>>> +       reset_control_assert(priv->rst);
>>> +       clk_disable_unprepare(priv->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 *child =
>>> of_get_next_available_child(dev->of_node,
>>> +               NULL);
>>> +       u32 property, ranges[4];
>>> +       int ret;
>>> +
>>> +       if (!child) {
>>> +               dev_warn(dev, "no child nodes found\n");
>>> +               return 0;
>>
>>
>> Don't we want to return an error here? Otherwise, we will call
>> tegra_gmi_init() with an invalid configuration.
>
> True, we probably want that. My thought was that we might accept a
> tegra-gmi node without any child nodes and just print a warning. But
> since it is the child node that holds the bus configuration it makes
> sense to fail probe due to no child nodes.

I was wondering that too, but given that we then program and enable the 
GMI I think it is best to just fail for now.

>>
>>
>>> +       }
>>> +
>>> +       /*
>>> +        * We currently only support one child device due to lack of
>>> +        * chip-select address decoding. Which means that we only have one
>>> +        * chip-select line from the GMI controller.
>>> +        */
>>> +       if (of_get_child_count(dev->of_node) > 1)
>>> +               dev_warn(dev, "only one child device is supported.");
>>> +
>>> +       if (of_property_read_bool(child, "nvidia,snor-data-width-32bit"))
>>> +               priv->snor_config |= TEGRA_GMI_BUS_WIDTH_32BIT;
>>> +
>>> +       if (of_property_read_bool(child, "nvidia,snor-mux-mode"))
>>> +               priv->snor_config |= TEGRA_GMI_MUX_MODE;
>>> +
>>> +       if (of_property_read_bool(child,
>>> "nvidia,snor-rdy-active-before-data"))
>>> +               priv->snor_config |= TEGRA_GMI_RDY_BEFORE_DATA;
>>> +
>>> +       if (of_property_read_bool(child, "nvidia,snor-rdy-inv"))
>>> +               priv->snor_config |= TEGRA_GMI_RDY_ACTIVE_HIGH;
>>> +
>>> +       if (of_property_read_bool(child, "nvidia,snor-adv-inv"))
>>> +               priv->snor_config |= TEGRA_GMI_ADV_ACTIVE_HIGH;
>>> +
>>> +       if (of_property_read_bool(child, "nvidia,snor-oe-inv"))
>>> +               priv->snor_config |= TEGRA_GMI_OE_ACTIVE_HIGH;
>>> +
>>> +       if (of_property_read_bool(child, "nvidia,snor-cs-inv"))
>>> +               priv->snor_config |= TEGRA_GMI_CS_ACTIVE_HIGH;
>>> +
>>> +       /* Decode the CS# */
>>> +       ret = of_property_read_u32_array(child, "ranges", ranges, 4);
>>> +       if (ret < 0) {
>>> +               /* Invalid binding */
>>> +               if (ret == -EOVERFLOW) {
>>> +                       dev_err(dev, "invalid ranges length\n");
>>> +                       goto error_cs_decode;
>>> +               }
>>> +
>>> +               /*
>>> +                * If we reach here it means that the child node has an
>>> empty
>>> +                * ranges or it does not exist at all. Attempt to decode
>>> the
>>> +                * CS# from the reg property instead.
>>> +                */
>>> +               ret = of_property_read_u32(child, "reg", &property);
>>> +               if (ret < 0) {
>>> +                       dev_err(dev, "no reg property found\n");
>>> +                       goto error_cs_decode;
>>> +               }
>>> +       } else {
>>> +               property = ranges[1];
>>> +       }
>>> +
>>> +       priv->snor_config |= TEGRA_GMI_CS_SELECT(property);
>>
>>
>> Should we make sure the CS is a valid value before setting?
>
> The TEGRA_GMI_CS_SELECT(x) macro will truncate any erroneous CS value.
> But yeah we could do a sanity check instead and return an error if it
> is invalid.
>
>>
>>
>>> +
>>> +       /* The default values that are provided below are reset values */
>>> +       if (!of_property_read_u32(child, "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(child, "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(child, "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(child, "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(child, "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(child, "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(child, "nvidia,snor-wait-width",
>>> &property))
>>> +               priv->snor_timing1 |= TEGRA_GMI_WAIT_WIDTH(property);
>>> +       else
>>> +               priv->snor_timing1 |= TEGRA_GMI_WAIT_WIDTH(3);
>>> +
>>> +error_cs_decode:
>>> +       if (ret < 0)
>>> +               dev_err(dev, "failed to decode chip-select number\n");
>>
>>
>> Nit do we need another error message here? Can we add the "failed to decode
>> CS" part the earlier message?
>
> Does it make sense to drop the two earlier messages instead and keep this one?

I think it is nice to have specific errors so we know where it failed. 
You could just drop the above and when you add the test for making sure 
the CS is valid add another error message for that. Not a big deal 
either way. So I will leave to you to decide.

Cheers
Jon

-- 
nvpublic