[PATCH] ARM: dts: socfpga: Add fpga2hps and fpga2sdram bridges

[PATCH] ARM: dts: socfpga: Add fpga2hps and fpga2sdram bridges

[Steffen Trumtrar]

Add the remaining two bridges on the Cyclone-V SoCFPGA SoCs.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 arch/arm/boot/dts/socfpga.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index b38f8c240558..e0fb76107f34 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -543,6 +543,18 @@
 			clocks = <&l4_main_clk>;
 		};
 
+		fpga_bridge2: fpga-bridge at ff600000 {
+			compatible = "altr,socfpga-fpga2hps-bridge";
+			reg = <0xff600000 0x100000>;
+			resets = <&rst FPGA2HPS_RESET>;
+			clocks = <&l4_main_clk>;
+		};
+
+		fpga_bridge3: fpga-bridge at ffc25080 {
+			compatible = "altr,socfpga-fpga2sdram-bridge";
+			reg = <0xffc25080 0x4>;
+		};
+
 		fpgamgr0: fpgamgr at ff706000 {
 			compatible = "altr,socfpga-fpga-mgr";
 			reg = <0xff706000 0x1000
-- 
2.11.0

[PATCH] ARM: dts: socfpga: Add fpga2hps and fpga2sdram bridges

[Dinh Nguyen]

CC'd Alan Tull

Alan, do you have any comments?

Dinh

On 08/15/2018 07:59 AM, Steffen Trumtrar wrote:
> Add the remaining two bridges on the Cyclone-V SoCFPGA SoCs.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  arch/arm/boot/dts/socfpga.dtsi | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index b38f8c240558..e0fb76107f34 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -543,6 +543,18 @@
>  			clocks = <&l4_main_clk>;
>  		};
>  
> +		fpga_bridge2: fpga-bridge at ff600000 {
> +			compatible = "altr,socfpga-fpga2hps-bridge";
> +			reg = <0xff600000 0x100000>;
> +			resets = <&rst FPGA2HPS_RESET>;
> +			clocks = <&l4_main_clk>;
> +		};
> +
> +		fpga_bridge3: fpga-bridge at ffc25080 {
> +			compatible = "altr,socfpga-fpga2sdram-bridge";
> +			reg = <0xffc25080 0x4>;
> +		};
> +
>  		fpgamgr0: fpgamgr at ff706000 {
>  			compatible = "altr,socfpga-fpga-mgr";
>  			reg = <0xff706000 0x1000
> 

[PATCH] ARM: dts: socfpga: Add fpga2hps and fpga2sdram bridges

[Alan Tull]

On Wed, Aug 15, 2018 at 9:48 AM, Dinh Nguyen <dinguyen@kernel.org> wrote:
> CC'd Alan Tull
>
> Alan, do you have any comments?
>
> Dinh
>
> On 08/15/2018 07:59 AM, Steffen Trumtrar wrote:
>> Add the remaining two bridges on the Cyclone-V SoCFPGA SoCs.

I left these out on purpose, see below.

>>
>> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
>> ---
>>  arch/arm/boot/dts/socfpga.dtsi | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>> index b38f8c240558..e0fb76107f34 100644
>> --- a/arch/arm/boot/dts/socfpga.dtsi
>> +++ b/arch/arm/boot/dts/socfpga.dtsi
>> @@ -543,6 +543,18 @@
>>                       clocks = <&l4_main_clk>;
>>               };
>>
>> +             fpga_bridge2: fpga-bridge at ff600000 {
>> +                     compatible = "altr,socfpga-fpga2hps-bridge";
>> +                     reg = <0xff600000 0x100000>;
>> +                     resets = <&rst FPGA2HPS_RESET>;
>> +                     clocks = <&l4_main_clk>;

The FPGA has to be programmed before this bridge can be enabled.
Since we can't guarantee that, we shouldn't add that here.

>> +             };
>> +
>> +             fpga_bridge3: fpga-bridge at ffc25080 {
>> +                     compatible = "altr,socfpga-fpga2sdram-bridge";
>> +                     reg = <0xffc25080 0x4>;

Missing altr,sdr-ctl and altr,sys-mgr which will cause probe failures
with error messages.  I checked the binding document, I see that these
properties weren't added and that should be corrected in the bindings
doc.

But the larger issue is this bridge needs to be configured by the
bootloader before Linux boots.  This requirement is documented at the
top of drivers/fpga/altera-fpga2sdram.c  Also it needs a handoff from
the bootloader that may or may not be happening depending on whether
the bootloader does this or not.  That's why I left this out.

Alan

>> +             };
>> +
>>               fpgamgr0: fpgamgr at ff706000 {
>>                       compatible = "altr,socfpga-fpga-mgr";
>>                       reg = <0xff706000 0x1000
>>

[PATCH] ARM: dts: socfpga: Add fpga2hps and fpga2sdram bridges

[Steffen Trumtrar]

Hi!

Alan Tull <atull@kernel.org> writes:

> On Wed, Aug 15, 2018 at 9:48 AM, Dinh Nguyen 
> <dinguyen@kernel.org> wrote:
>> CC'd Alan Tull
>>
>> Alan, do you have any comments?
>>
>> Dinh
>>
>> On 08/15/2018 07:59 AM, Steffen Trumtrar wrote:
>>> Add the remaining two bridges on the Cyclone-V SoCFPGA SoCs.
>
> I left these out on purpose, see below.
>
>>>
>>> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
>>> ---
>>>  arch/arm/boot/dts/socfpga.dtsi | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/socfpga.dtsi 
>>> b/arch/arm/boot/dts/socfpga.dtsi
>>> index b38f8c240558..e0fb76107f34 100644
>>> --- a/arch/arm/boot/dts/socfpga.dtsi
>>> +++ b/arch/arm/boot/dts/socfpga.dtsi
>>> @@ -543,6 +543,18 @@
>>>                       clocks = <&l4_main_clk>;
>>>               };
>>>
>>> +             fpga_bridge2: fpga-bridge at ff600000 {
>>> +                     compatible = 
>>> "altr,socfpga-fpga2hps-bridge";
>>> +                     reg = <0xff600000 0x100000>;
>>> +                     resets = <&rst FPGA2HPS_RESET>;
>>> +                     clocks = <&l4_main_clk>;
>
> The FPGA has to be programmed before this bridge can be enabled.
> Since we can't guarantee that, we shouldn't add that here.
>

The hardware is there, that is what the binding describes. Maybe a
    status = "disabled";
would be justified here. But the same goes for the other bridges. 
You can't use any of them without a programmed FPGA.

>>> +             };
>>> +
>>> +             fpga_bridge3: fpga-bridge at ffc25080 {
>>> +                     compatible = 
>>> "altr,socfpga-fpga2sdram-bridge";
>>> +                     reg = <0xffc25080 0x4>;
>
> Missing altr,sdr-ctl and altr,sys-mgr which will cause probe 
> failures
> with error messages.  I checked the binding document, I see that 
> these
> properties weren't added and that should be corrected in the 
> bindings
> doc.
>

It doesn't, because the driver uses 
syscon_regmap_lookup_by_compatible and not 
syscon_regmap_lookup_by_phandle.
I don't see any probe failures. Nevertheless, the dependency 
should be documented, yes.

> But the larger issue is this bridge needs to be configured by 
> the
> bootloader before Linux boots.  This requirement is documented 
> at the
> top of drivers/fpga/altera-fpga2sdram.c  Also it needs a handoff 
> from
> the bootloader that may or may not be happening depending on 
> whether
> the bootloader does this or not.  That's why I left this out.
>

Although registration of the driver works fine, you are right, 
that it shouldn't be used without the correct setup.
So, I'd go for status=disabled in the dtsi and would leave 
enabling of the bridges to the board-specific dts file.


Best regards,
Steffen

-- 
Pengutronix e.K.                          | Steffen Trumtrar 
|
Industrial Linux Solutions                | 
http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany| Phone: 
+49-5121-206917-0   |
Amtsgericht Hildesheim, HRA 2686          | Fax: 
+49-5121-206917-5555|