diff mbox

[v11,4/6] ARM: dts: imx6q-evi: support altera-ps-spi

Message ID 20170525172911.11467-5-stillcompiling@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Joshua Clayton May 25, 2017, 5:29 p.m. UTC
Add support for Altera V FPGA connected to an spi port
to the evi devicetree file

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
 arch/arm/boot/dts/imx6q-evi.dts | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Andreas Färber June 2, 2017, 4:30 p.m. UTC | #1
Hi,

Am 25.05.2017 um 19:29 schrieb Joshua Clayton:
> Add support for Altera V FPGA connected to an spi port

Did you mean "Altera Cyclone V"?

> to the evi devicetree file
> 
> Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> ---
>  arch/arm/boot/dts/imx6q-evi.dts | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6q-evi.dts b/arch/arm/boot/dts/imx6q-evi.dts
> index 24fe093a66db..a0cbb2d84803 100644
> --- a/arch/arm/boot/dts/imx6q-evi.dts
> +++ b/arch/arm/boot/dts/imx6q-evi.dts
> @@ -82,6 +82,15 @@
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_ecspi1 &pinctrl_ecspi1cs>;
>  	status = "okay";
> +
> +	fpga_spi: cyclonespi@0 {

"cyclonespi" does not strike me as the best node name.

I am guessing this is a sub-node of a SPI controller node, so no need to
repeat "spi", and Cyclone seems more or less implied by "altr,fpga-".

Note that the example in the bindings doc uses "evi-fpga-spi". Nodes
don't need to be (shouldn't be?) prefixed with the board. Note that
bindings examples tend to get copied a lot.

Any reason not to just use "fpga@0" in both places for simplicity?

> +		compatible = "altr,fpga-passive-serial";
> +		spi-max-frequency = <20000000>;
> +		reg = <0>;
> +		pinctrl-0 = <&pinctrl_fpgaspi>;
> +		nconfig-gpios = <&gpio4 9 GPIO_ACTIVE_LOW>;
> +		nstat-gpios = <&gpio4 11 GPIO_ACTIVE_LOW>;
> +	};
>  };
>  
>  &ecspi3 {
[snip]

Regards,
Andreas
Joshua Clayton June 2, 2017, 7:39 p.m. UTC | #2
On Friday, June 2, 2017 6:30:12 PM PDT Andreas Färber wrote:
> Hi,
> 
> Am 25.05.2017 um 19:29 schrieb Joshua Clayton:
> > Add support for Altera V FPGA connected to an spi port
> 
> Did you mean "Altera Cyclone V"?
I meant to shorten it from Altera Cyclone V to Altera FPGA.
Didn't quite make it.  Will fix and resubmit today.
> 
> > to the evi devicetree file
> > 
> > Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> > ---
> > 
> >  arch/arm/boot/dts/imx6q-evi.dts | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/imx6q-evi.dts
> > b/arch/arm/boot/dts/imx6q-evi.dts index 24fe093a66db..a0cbb2d84803 100644
> > --- a/arch/arm/boot/dts/imx6q-evi.dts
> > +++ b/arch/arm/boot/dts/imx6q-evi.dts
> > @@ -82,6 +82,15 @@
> > 
> >  	pinctrl-names = "default";
> >  	pinctrl-0 = <&pinctrl_ecspi1 &pinctrl_ecspi1cs>;
> >  	status = "okay";
> > 
> > +
> > +	fpga_spi: cyclonespi@0 {
> 
> "cyclonespi" does not strike me as the best node name.
> 
> I am guessing this is a sub-node of a SPI controller node, so no need to
> repeat "spi", and Cyclone seems more or less implied by "altr,fpga-".
True.
> 
> Note that the example in the bindings doc uses "evi-fpga-spi". Nodes
> don't need to be (shouldn't be?) prefixed with the board. Note that
> bindings examples tend to get copied a lot.
> 
> Any reason not to just use "fpga@0" in both places for simplicity?
Sure. fpga: fpga@0 is probably better.
I'll change it in both the dts and the binding doc.
> 
> > +		compatible = "altr,fpga-passive-serial";
> > +		spi-max-frequency = <20000000>;
> > +		reg = <0>;
> > +		pinctrl-0 = <&pinctrl_fpgaspi>;
> > +		nconfig-gpios = <&gpio4 9 GPIO_ACTIVE_LOW>;
> > +		nstat-gpios = <&gpio4 11 GPIO_ACTIVE_LOW>;
> > +	};
> > 
> >  };
> >  
> >  &ecspi3 {
> 
> [snip]
> 
> Regards,
> Andreas
Andreas Färber June 2, 2017, 7:54 p.m. UTC | #3
Am 02.06.2017 um 21:39 schrieb stillcompiling@gmail.com:
> On Friday, June 2, 2017 6:30:12 PM PDT Andreas Färber wrote:
>> Am 25.05.2017 um 19:29 schrieb Joshua Clayton:
>>> diff --git a/arch/arm/boot/dts/imx6q-evi.dts
>>> b/arch/arm/boot/dts/imx6q-evi.dts index 24fe093a66db..a0cbb2d84803 100644
>>> --- a/arch/arm/boot/dts/imx6q-evi.dts
>>> +++ b/arch/arm/boot/dts/imx6q-evi.dts
>>> @@ -82,6 +82,15 @@
>>>
>>>  	pinctrl-names = "default";
>>>  	pinctrl-0 = <&pinctrl_ecspi1 &pinctrl_ecspi1cs>;
>>>  	status = "okay";
>>>
>>> +
>>> +	fpga_spi: cyclonespi@0 {
>>
>> "cyclonespi" does not strike me as the best node name.
>>
>> I am guessing this is a sub-node of a SPI controller node, so no need to
>> repeat "spi", and Cyclone seems more or less implied by "altr,fpga-".
> True.
>>
>> Note that the example in the bindings doc uses "evi-fpga-spi". Nodes
>> don't need to be (shouldn't be?) prefixed with the board. Note that
>> bindings examples tend to get copied a lot.
>>
>> Any reason not to just use "fpga@0" in both places for simplicity?
> Sure. fpga: fpga@0 is probably better.

Note that I was only commenting on the node name, the latter part.

I'm not aware of any rules for the label, so that could remain unchanged
or adopt cyclone_spi from the old node name or whatever is unique and
syntactically valid.

> I'll change it in both the dts and the binding doc.

Thanks. Maybe double-check if there's any conventions Xilinx/Lattice DTs
are using.

Cheers,
Andreas
Joshua Clayton June 2, 2017, 9:10 p.m. UTC | #4
On Friday, June 2, 2017 9:54:22 PM PDT Andreas Färber wrote:
> Am 02.06.2017 um 21:39 schrieb stillcompiling@gmail.com:
> > On Friday, June 2, 2017 6:30:12 PM PDT Andreas Färber wrote:
> >> Am 25.05.2017 um 19:29 schrieb Joshua Clayton:
> >>> diff --git a/arch/arm/boot/dts/imx6q-evi.dts
> >>> b/arch/arm/boot/dts/imx6q-evi.dts index 24fe093a66db..a0cbb2d84803
> >>> 100644
> >>> --- a/arch/arm/boot/dts/imx6q-evi.dts
> >>> +++ b/arch/arm/boot/dts/imx6q-evi.dts
> >>> @@ -82,6 +82,15 @@
> >>> 
> >>>  	pinctrl-names = "default";
> >>>  	pinctrl-0 = <&pinctrl_ecspi1 &pinctrl_ecspi1cs>;
> >>>  	status = "okay";
> >>> 
> >>> +
> >>> +	fpga_spi: cyclonespi@0 {
> >> 
> >> "cyclonespi" does not strike me as the best node name.
> >> 
> >> I am guessing this is a sub-node of a SPI controller node, so no need to
> >> repeat "spi", and Cyclone seems more or less implied by "altr,fpga-".
> > 
> > True.
> > 
> >> Note that the example in the bindings doc uses "evi-fpga-spi". Nodes
> >> don't need to be (shouldn't be?) prefixed with the board. Note that
> >> bindings examples tend to get copied a lot.
> >> 
> >> Any reason not to just use "fpga@0" in both places for simplicity?
> > 
> > Sure. fpga: fpga@0 is probably better.
> 
> Note that I was only commenting on the node name, the latter part.
> 
> I'm not aware of any rules for the label, so that could remain unchanged
> or adopt cyclone_spi from the old node name or whatever is unique and
> syntactically valid.
Too late! Patches posted.
Oh, well, I'm not changing it back.
> 
> > I'll change it in both the dts and the binding doc.
> 
> Thanks. Maybe double-check if there's any conventions Xilinx/Lattice DTs
> are using.
> 
Of the conventions I found, fpga seemed the most "hardware descriptive"
for a plain FPGA.
The other one several binding doc examples are using is "fpga-mgr".

> Cheers,
> Andreas
Alan Tull June 5, 2017, 3:10 p.m. UTC | #5
On Fri, Jun 2, 2017 at 4:10 PM,  <stillcompiling@gmail.com> wrote:
> On Friday, June 2, 2017 9:54:22 PM PDT Andreas Färber wrote:
>> Am 02.06.2017 um 21:39 schrieb stillcompiling@gmail.com:
>> > On Friday, June 2, 2017 6:30:12 PM PDT Andreas Färber wrote:
>> >> Am 25.05.2017 um 19:29 schrieb Joshua Clayton:
>> >>> diff --git a/arch/arm/boot/dts/imx6q-evi.dts
>> >>> b/arch/arm/boot/dts/imx6q-evi.dts index 24fe093a66db..a0cbb2d84803
>> >>> 100644
>> >>> --- a/arch/arm/boot/dts/imx6q-evi.dts
>> >>> +++ b/arch/arm/boot/dts/imx6q-evi.dts
>> >>> @@ -82,6 +82,15 @@
>> >>>
>> >>>   pinctrl-names = "default";
>> >>>   pinctrl-0 = <&pinctrl_ecspi1 &pinctrl_ecspi1cs>;
>> >>>   status = "okay";
>> >>>
>> >>> +
>> >>> + fpga_spi: cyclonespi@0 {
>> >>
>> >> "cyclonespi" does not strike me as the best node name.
>> >>
>> >> I am guessing this is a sub-node of a SPI controller node, so no need to
>> >> repeat "spi", and Cyclone seems more or less implied by "altr,fpga-".
>> >
>> > True.
>> >
>> >> Note that the example in the bindings doc uses "evi-fpga-spi". Nodes
>> >> don't need to be (shouldn't be?) prefixed with the board. Note that
>> >> bindings examples tend to get copied a lot.
>> >>
>> >> Any reason not to just use "fpga@0" in both places for simplicity?
>> >
>> > Sure. fpga: fpga@0 is probably better.
>>
>> Note that I was only commenting on the node name, the latter part.
>>
>> I'm not aware of any rules for the label, so that could remain unchanged
>> or adopt cyclone_spi from the old node name or whatever is unique and
>> syntactically valid.
> Too late! Patches posted.
> Oh, well, I'm not changing it back.

It's fine as it is in v12.

fpga_mgr: fpga-mgr@0 is what I've been using most recently.  It
distinguishes the one block that is used to program the fpga from the
fpga and hardware in the fpga.  But no need to respin this.

Alan

>>
>> > I'll change it in both the dts and the binding doc.
>>
>> Thanks. Maybe double-check if there's any conventions Xilinx/Lattice DTs
>> are using.
>>
> Of the conventions I found, fpga seemed the most "hardware descriptive"
> for a plain FPGA.
> The other one several binding doc examples are using is "fpga-mgr".
>
>> Cheers,
>> Andreas
>
>
> --
> ~Joshua A Clayton
diff mbox

Patch

diff --git a/arch/arm/boot/dts/imx6q-evi.dts b/arch/arm/boot/dts/imx6q-evi.dts
index 24fe093a66db..a0cbb2d84803 100644
--- a/arch/arm/boot/dts/imx6q-evi.dts
+++ b/arch/arm/boot/dts/imx6q-evi.dts
@@ -82,6 +82,15 @@ 
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_ecspi1 &pinctrl_ecspi1cs>;
 	status = "okay";
+
+	fpga_spi: cyclonespi@0 {
+		compatible = "altr,fpga-passive-serial";
+		spi-max-frequency = <20000000>;
+		reg = <0>;
+		pinctrl-0 = <&pinctrl_fpgaspi>;
+		nconfig-gpios = <&gpio4 9 GPIO_ACTIVE_LOW>;
+		nstat-gpios = <&gpio4 11 GPIO_ACTIVE_LOW>;
+	};
 };
 
 &ecspi3 {
@@ -313,6 +322,13 @@ 
 		>;
 	};
 
+	pinctrl_fpgaspi: fpgaspigrp {
+		fsl,pins = <
+			MX6QDL_PAD_KEY_ROW1__GPIO4_IO09 0x1b0b0
+			MX6QDL_PAD_KEY_ROW2__GPIO4_IO11 0x1b0b0
+		>;
+	};
+
 	pinctrl_gpminand: gpminandgrp {
 		fsl,pins = <
 			MX6QDL_PAD_NANDF_CLE__NAND_CLE 0xb0b1