diff mbox

[v2,2/3] ARM: dts: socfpga: fpga bridges bindings docs

Message ID 1414108267-22058-3-git-send-email-atull@opensource.altera.com
State New, archived
Headers show

Commit Message

atull Oct. 23, 2014, 11:51 p.m. UTC
From: Alan Tull <atull@opensource.altera.com>

Add DTS binding documentation for the Altera FPGA bridges.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
 .../bindings/fpga/altera-fpga2sdram-bridge.txt     |   57 ++++++++++++++++++++
 .../bindings/fpga/altera-hps2fpga-bridge.txt       |   53 ++++++++++++++++++
 2 files changed, 110 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
 create mode 100644 Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt

Comments

Steffen Trumtrar Oct. 24, 2014, 7 a.m. UTC | #1
Hi!

On Thu, Oct 23, 2014 at 06:51:06PM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>

(...)

> diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> new file mode 100644
> index 0000000..bc24a2e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> @@ -0,0 +1,53 @@
> +Altera FPGA/HPS Bridge Driver
> +
> +This driver manages a bridge between a FPGA and a host processor system (HPS).
> +User space can enable or disable the bridge by writing a "1" or a "0",
> +respectively, to its enable file under bridge's entry in
> +/sys/class/fpga-bridge.  Typically, one disables the bridges before
> +reprogramming the FPGA.  Once the FPGA is reprogrammed, the bridges are
> +reenabled.
> +

NAK.

This is all linux specific and doesn't belong here.

> +Required properties:
> +
> + - compatible     : should contain one of:
> +                     "altr,socfpga-hps2fpga-bridge"
> +                     "altr,socfpga-lwhps2fpga-bridge"
> +                     "altr,socfpga-fpga2hps-bridge"
> +
> + - clocks         : clocks used by this module
> +
> + - altr,l3-syscon : phandle of the l3 interconnect module
> +

L3 shouldn't be a syscon. Have you tried dumping the regmap in the debugfs if L3
is a syscon? Doesn't work.

> +Optional properties:
> + - label          : name that you want this bridge to show up as under
> +                    /sys/class/fpga-bridge.  Default is br<device#> if this is
> +                    not specified.
> +

Why? Linux-specific.

> + - init-val       : 0 if driver should disable bridge at startup
> +                    1 if driver should enable bridge at startup
> +                    driver leaves bridge in current state if property not
> +                    specified.
> +

Configuration in the DT? Really?

> +Example:
> +	hps_fpgabridge0: fpgabridge@0 {
> +		compatible = "altr,socfpga-hps2fpga-bridge";
> +		label = "hps2fpga";
> +		altr,l3-syscon = <&l3regs>;
> +		clocks = <&l4_main_clk>;
> +		init-val = <1>;
> +	};
> +
> +	hps_fpgabridge1: fpgabridge@1 {
> +		compatible = "altr,socfpga-lwhps2fpga-bridge";
> +		label = "lwhps2fpga";
> +		altr,l3-syscon = <&l3regs>;
> +		clocks = <&l4_main_clk>;
> +		init-val = <0>;
> +	};
> +
> +	hps_fpgabridge2: fpgabridge@2 {
> +		compatible = "altr,socfpga-fpga2hps-bridge";
> +		label = "fpga2hps";
> +		altr,l3-syscon = <&l3regs>;
> +		clocks = <&l4_main_clk>;
> +	};

The bridges are the buses into the FPGA. This has to be accomodated.
The bridges have two specified memory ranges: one the address space
of the bus, the second the register space for configuration.

This binding does NOT correctly describe the hardware. Sorry.

Regards,
Steffen
Pantelis Antoniou Oct. 24, 2014, 9:20 a.m. UTC | #2
Hi Stefan, Allan,

Sorry, haven’t had a chance to review all this yet; will do so in the weekend.
Just wanted to pop in and comment on a few things.

> On Oct 24, 2014, at 10:00 AM, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:
> 
> Hi!
> 
> On Thu, Oct 23, 2014 at 06:51:06PM -0500, atull@opensource.altera.com wrote:
>> From: Alan Tull <atull@opensource.altera.com>
> 
> (...)
> 
>> diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
>> new file mode 100644
>> index 0000000..bc24a2e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
>> @@ -0,0 +1,53 @@
>> +Altera FPGA/HPS Bridge Driver
>> +
>> +This driver manages a bridge between a FPGA and a host processor system (HPS).
>> +User space can enable or disable the bridge by writing a "1" or a "0",
>> +respectively, to its enable file under bridge's entry in
>> +/sys/class/fpga-bridge.  Typically, one disables the bridges before
>> +reprogramming the FPGA.  Once the FPGA is reprogrammed, the bridges are
>> +reenabled.
>> +
> 
> NAK.
> 
> This is all linux specific and doesn't belong here.
> 

We know. The DT spec hasn’t been updated for a while, and this is going to be
part of what we want to do with the restarting of the ePAPR spec process.

So absolutes like “DT is a hardware description only" might be too strong statements, that
do not work in practice anymore.

There’s no such thing as pure hardware devices lately - there is always a configuration
component; some of it OS specific, some of it not.

We have to be engaged in the process and figure out how to update the spec for what is
now the state of the art in the industry.

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pavel Machek Oct. 24, 2014, 10:57 a.m. UTC | #3
On Thu 2014-10-23 18:51:06, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> Add DTS binding documentation for the Altera FPGA bridges.
> 
> Signed-off-by: Alan Tull <atull@opensource.altera.com>

Acked-by: Pavel Machek <pavel@ucw.cz>
Steffen Trumtrar Oct. 25, 2014, 2:42 p.m. UTC | #4
Hi Pantelis!

On Fri, Oct 24, 2014 at 12:20:53PM +0300, Pantelis Antoniou wrote:
> Hi Stefan, Allan,
> 
> Sorry, haven’t had a chance to review all this yet; will do so in the weekend.
> Just wanted to pop in and comment on a few things.
> 
> > On Oct 24, 2014, at 10:00 AM, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:
> > 
> > Hi!
> > 
> > On Thu, Oct 23, 2014 at 06:51:06PM -0500, atull@opensource.altera.com wrote:
> >> From: Alan Tull <atull@opensource.altera.com>
> > 
> > (...)
> > 
> >> diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> >> new file mode 100644
> >> index 0000000..bc24a2e
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> >> @@ -0,0 +1,53 @@
> >> +Altera FPGA/HPS Bridge Driver
> >> +
> >> +This driver manages a bridge between a FPGA and a host processor system (HPS).
> >> +User space can enable or disable the bridge by writing a "1" or a "0",
> >> +respectively, to its enable file under bridge's entry in
> >> +/sys/class/fpga-bridge.  Typically, one disables the bridges before
> >> +reprogramming the FPGA.  Once the FPGA is reprogrammed, the bridges are
> >> +reenabled.
> >> +
> > 
> > NAK.
> > 
> > This is all linux specific and doesn't belong here.
> > 
> 
> We know. The DT spec hasn’t been updated for a while, and this is going to be
> part of what we want to do with the restarting of the ePAPR spec process.
>

As we don't have a new spec yet, I go with the current state of the art.
And I don't see how "file under ... /sys/class/fpga-bridge" fits the
current spec.

> So absolutes like “DT is a hardware description only" might be too strong statements, that
> do not work in practice anymore.
> 
> There’s no such thing as pure hardware devices lately - there is always a configuration
> component; some of it OS specific, some of it not.
> 

If it really is necessary, I'm not against it, don't get me wrong.
But in the grand scheme I wouldn't say that this fits my experience.
There are some devices that need configuration and often when it is
done in the DT, it is just a shortcut or not thought through.
And can be also introspected dynamically. With some discussion on the
list these bindings often change.

Regarding OS specifics: already there, e.g. console setup in the chosen node.
And other things I saw are ethernet aliases just for u-boot. Not a problem
of the spec, just a problem of u-boot and unneccessary "configuration".
Just to fix a bad bootloader. barebox can handle this without any
such stuff. Maybe we should integrate the DT "overlays" barebox uses into
the in-kernel DTs as well...but does it really help/interest someone who
decides to use u-boot where barebox stores its environment? I guess not.

> We have to be engaged in the process and figure out how to update the spec for what is
> now the state of the art in the industry.
> 

Again, not against that if it is necessary. For example your overlay stuff might
be a good update to the spec. Would be better if it is official instead of a "hack".
I want that for SoCFPGA.

But having looked at one or two vendor kernels+DTs, the state of the art in the
industry is: using DT wrong. (And doing HW wrong, which makes some updates to the
ePAPR necessary ;-))

Regards,
Steffen
Pantelis Antoniou Oct. 27, 2014, 11:48 a.m. UTC | #5
Hi Alan,

> On Oct 24, 2014, at 02:51 , atull@opensource.altera.com wrote:
> 
> From: Alan Tull <atull@opensource.altera.com>
> 
> Add DTS binding documentation for the Altera FPGA bridges.
> 
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> ---
> .../bindings/fpga/altera-fpga2sdram-bridge.txt     |   57 ++++++++++++++++++++
> .../bindings/fpga/altera-hps2fpga-bridge.txt       |   53 ++++++++++++++++++
> 2 files changed, 110 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
> create mode 100644 Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> 
> diff --git a/Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
> new file mode 100644
> index 0000000..cc8f522
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
> @@ -0,0 +1,57 @@
> +Altera FPGA To SDRAM Bridge Driver
> +
> +This driver manages a bridge between an FPGA and the SDRAM used by an host
> +processor system (HPS). The bridge contains a number read ports, write ports,
> +and command ports.  Reconfiguring these ports requires that no SDRAM
> +transactions occur during reconfiguration.  In other words, the code
> +reconfiguring the ports cannot be run out of SDRAM nor can the FPGA access the
> +SDRAM during the reconfiguration.  This driver does not support reconfiguring
> +the ports.  Typcially, the ports are configured by code running out of onchip
> +ram before Linux is started.
> +
> +This driver supports enabling and disabling of the configured ports all at
> +once, which allows for safe reprogramming of the FPGA from user space, provided
> +the new FPGA image uses the same port configuration. User space can enable or
> +disable the bridge by writing a "1" or a "0", respectively, to its enable file
> +under bridge's entry in /sys/class/fpga-bridge. Typically, one disables the
> +bridges before reprogramming the FPGA.  Once the FPGA is reprogrammed, the
> +bridges are reenabled.
> +
> +Required properties:
> +
> + - compatible       : "altr,socfpga-fpga2sdram-bridge"
> +
> + - read-ports-mask  : Bits 0 to 3 corresponds read ports 0 to 3. A bit set to 1
> +                      indicates the corresponding read port should be enabled.
> +
> + - write-ports-mask : Bits 0 to 3 corresponds write ports 0 to 3. A bit set
> +                      to 1 indicates the corresponding write port should be
> +                      enabled.
> +
> + - cmd-ports-mask   : Bits 0 to 5 correspond to command ports 0 to 5. A bit set
> +                      to 1 indicates the corresponding command port should be
> +                      enabled.
> +
> + - altr,sdr-syscon  : phandle of the sdr module
> +
> +Optional properties:
> +
> + - label            : name that you want this bridge to show up as under
> +                      /sys/class/fpga-bridge. Default is br<device#> if this is
> +                      not specified.
> +
> + - init-val         : 0 if driver should disable bridge at startup
> +                      1 if driver should enable bridge at startup
> +                      driver leaves bridge in current state if property not
> +		      specified.

Isn’t init-val a boolean property? It’s not named very well.

Along with the label, is kinda hard to defend as configuration in DT.

We need to start thinking about configuration, so let me throw that in
and fan the Monday flames to a nice red-hot color.

/ {
	chosen {
		linux {
			fpga-bridge {
				node = <&FPGA_BRIDGE>;
				label = “foo”;
				enable-bridge-on-startup;
			}
		};
	};
};

We will need accessors for drivers that iterate over the chosen/linux node children
and pick up the properties meant for the given driver node.

We also need to define behaviour in case those configuration properties are absent. 

Let the flames begin…

> +
> +Example:
> +	fpga2sdram_br: fpgabridge@3 {
> +		compatible = "altr,socfpga-fpga2sdram-bridge";
> +		label = "fpga2sdram";
> +		altr,sdr-syscon = <&sdr>;
> +		read-ports-mask = <3>;
> +		write-ports-mask = <3>;
> +		cmd-ports-mask = <0xd>;
> +		init-val = <0>;
> +	};
> diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> new file mode 100644
> index 0000000..bc24a2e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> @@ -0,0 +1,53 @@
> +Altera FPGA/HPS Bridge Driver
> +
> +This driver manages a bridge between a FPGA and a host processor system (HPS).
> +User space can enable or disable the bridge by writing a "1" or a "0",
> +respectively, to its enable file under bridge's entry in
> +/sys/class/fpga-bridge.  Typically, one disables the bridges before
> +reprogramming the FPGA.  Once the FPGA is reprogrammed, the bridges are
> +reenabled.
> +
> +Required properties:
> +
> + - compatible     : should contain one of:
> +                     "altr,socfpga-hps2fpga-bridge"
> +                     "altr,socfpga-lwhps2fpga-bridge"
> +                     "altr,socfpga-fpga2hps-bridge"
> +
> + - clocks         : clocks used by this module
> +
> + - altr,l3-syscon : phandle of the l3 interconnect module
> +
> +Optional properties:
> + - label          : name that you want this bridge to show up as under
> +                    /sys/class/fpga-bridge.  Default is br<device#> if this is
> +                    not specified.
> +
> + - init-val       : 0 if driver should disable bridge at startup
> +                    1 if driver should enable bridge at startup
> +                    driver leaves bridge in current state if property not
> +                    specified.
> +
> +Example:
> +	hps_fpgabridge0: fpgabridge@0 {
> +		compatible = "altr,socfpga-hps2fpga-bridge";
> +		label = "hps2fpga";
> +		altr,l3-syscon = <&l3regs>;
> +		clocks = <&l4_main_clk>;
> +		init-val = <1>;
> +	};
> +
> +	hps_fpgabridge1: fpgabridge@1 {
> +		compatible = "altr,socfpga-lwhps2fpga-bridge";
> +		label = "lwhps2fpga";
> +		altr,l3-syscon = <&l3regs>;
> +		clocks = <&l4_main_clk>;
> +		init-val = <0>;
> +	};
> +
> +	hps_fpgabridge2: fpgabridge@2 {
> +		compatible = "altr,socfpga-fpga2hps-bridge";
> +		label = "fpga2hps";
> +		altr,l3-syscon = <&l3regs>;
> +		clocks = <&l4_main_clk>;
> +	};
> -- 
> 1.7.9.5
> 

Regards

— Pantelis


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pantelis Antoniou Oct. 27, 2014, 11:54 a.m. UTC | #6
Hi Stefen,

> On Oct 25, 2014, at 17:42 , Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:
> 
> Hi Pantelis!
> 
> On Fri, Oct 24, 2014 at 12:20:53PM +0300, Pantelis Antoniou wrote:
>> Hi Stefan, Allan,
>> 
>> Sorry, haven’t had a chance to review all this yet; will do so in the weekend.
>> Just wanted to pop in and comment on a few things.
>> 
>>> On Oct 24, 2014, at 10:00 AM, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:
>>> 
>>> Hi!
>>> 
>>> On Thu, Oct 23, 2014 at 06:51:06PM -0500, atull@opensource.altera.com wrote:
>>>> From: Alan Tull <atull@opensource.altera.com>
>>> 
>>> (...)
>>> 
>>>> diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
>>>> new file mode 100644
>>>> index 0000000..bc24a2e
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
>>>> @@ -0,0 +1,53 @@
>>>> +Altera FPGA/HPS Bridge Driver
>>>> +
>>>> +This driver manages a bridge between a FPGA and a host processor system (HPS).
>>>> +User space can enable or disable the bridge by writing a "1" or a "0",
>>>> +respectively, to its enable file under bridge's entry in
>>>> +/sys/class/fpga-bridge.  Typically, one disables the bridges before
>>>> +reprogramming the FPGA.  Once the FPGA is reprogrammed, the bridges are
>>>> +reenabled.
>>>> +
>>> 
>>> NAK.
>>> 
>>> This is all linux specific and doesn't belong here.
>>> 
>> 
>> We know. The DT spec hasn’t been updated for a while, and this is going to be
>> part of what we want to do with the restarting of the ePAPR spec process.
>> 
> 
> As we don't have a new spec yet, I go with the current state of the art.
> And I don't see how "file under ... /sys/class/fpga-bridge" fits the
> current spec.
> 
>> So absolutes like “DT is a hardware description only" might be too strong statements, that
>> do not work in practice anymore.
>> 
>> There’s no such thing as pure hardware devices lately - there is always a configuration
>> component; some of it OS specific, some of it not.
>> 
> 
> If it really is necessary, I'm not against it, don't get me wrong.
> But in the grand scheme I wouldn't say that this fits my experience.
> There are some devices that need configuration and often when it is
> done in the DT, it is just a shortcut or not thought through.
> And can be also introspected dynamically. With some discussion on the
> list these bindings often change.
> 

Already answered and given a possible way this might work; admittedly this
specific example is not a good fit for what I was trying to say, but whatever.
Let’s get the ball rolling.

> Regarding OS specifics: already there, e.g. console setup in the chosen node.
> And other things I saw are ethernet aliases just for u-boot. Not a problem
> of the spec, just a problem of u-boot and unneccessary "configuration".
> Just to fix a bad bootloader. barebox can handle this without any
> such stuff. Maybe we should integrate the DT "overlays" barebox uses into
> the in-kernel DTs as well...but does it really help/interest someone who
> decides to use u-boot where barebox stores its environment? I guess not.
> 

Although I’m not against having DT overlays in the bootloader, I only see them
as a method that helps a platform developer express things easier. I am completely
against having the kernel tied to any particular bootloader.

We’ve all been through the hell where a kernel only boots if the bootloader has 
setup the platform “correctly”. This “correctly” has a very loose definition and
99/100 times is extremely badly documented or not at all.

IMHO the bootloader should (as part of the normal boot sequence) only setup the
absolutely bare minimum and then pass control to the kernel as soon as possible.

The full featured bootloader environment should only be presented when the user
requests so.

>> We have to be engaged in the process and figure out how to update the spec for what is
>> now the state of the art in the industry.
>> 
> 
> Again, not against that if it is necessary. For example your overlay stuff might
> be a good update to the spec. Would be better if it is official instead of a "hack".
> I want that for SoCFPGA.
> 
> But having looked at one or two vendor kernels+DTs, the state of the art in the
> industry is: using DT wrong. (And doing HW wrong, which makes some updates to the
> ePAPR necessary ;-))
> 

Vendor H/W, vendor DT and a crack pipe. Or as I call it Monday.

> Regards,
> Steffen

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Oct. 27, 2014, 3:01 p.m. UTC | #7
On Mon, Oct 27, 2014 at 01:48:02PM +0200, Pantelis Antoniou wrote:
> > On Oct 24, 2014, at 02:51 , atull@opensource.altera.com wrote:

> > + - init-val         : 0 if driver should disable bridge at startup
> > +                      1 if driver should enable bridge at startup
> > +                      driver leaves bridge in current state if property not
> > +		      specified.

> Isn’t init-val a boolean property? It’s not named very well.

It's not boolean, it's tristate - turn on, turn off or don't touch.

> Along with the label, is kinda hard to defend as configuration in DT.

Yeah...  presumably this decision would fall out of the users?
Pantelis Antoniou Oct. 27, 2014, 3:05 p.m. UTC | #8
Hi Mark,

> On Oct 27, 2014, at 17:01 , Mark Brown <broonie@kernel.org> wrote:
> 
> On Mon, Oct 27, 2014 at 01:48:02PM +0200, Pantelis Antoniou wrote:
>>> On Oct 24, 2014, at 02:51 , atull@opensource.altera.com wrote:
> 
>>> + - init-val         : 0 if driver should disable bridge at startup
>>> +                      1 if driver should enable bridge at startup
>>> +                      driver leaves bridge in current state if property not
>>> +		      specified.
> 
>> Isn’t init-val a boolean property? It’s not named very well.
> 
> It's not boolean, it's tristate - turn on, turn off or don't touch.
> 

I see. Even then ‘init-val’ is cryptic. I’d prefer two booleans,
enable-at-startup; disable-at-startup.

>> Along with the label, is kinda hard to defend as configuration in DT.
> 
> Yeah...  presumably this decision would fall out of the users?

Well, it’s the user that should make the decision, but the driver should
pick it up. This works but it’s not very nice.

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steffen Trumtrar Oct. 27, 2014, 3:23 p.m. UTC | #9
Hi!

On Mon, Oct 27, 2014 at 01:54:20PM +0200, Pantelis Antoniou wrote:
> Hi Stefen,
> 
> > On Oct 25, 2014, at 17:42 , Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:
> > 
> > Hi Pantelis!
> > 
> > On Fri, Oct 24, 2014 at 12:20:53PM +0300, Pantelis Antoniou wrote:
> >> Hi Stefan, Allan,
> >> 
> >> Sorry, haven’t had a chance to review all this yet; will do so in the weekend.
> >> Just wanted to pop in and comment on a few things.
> >> 
> >>> On Oct 24, 2014, at 10:00 AM, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:
> >>> 
> >>> Hi!
> >>> 
> >>> On Thu, Oct 23, 2014 at 06:51:06PM -0500, atull@opensource.altera.com wrote:
> >>>> From: Alan Tull <atull@opensource.altera.com>
> >>> 
> >>> (...)
> >>> 
> >>>> diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> >>>> new file mode 100644
> >>>> index 0000000..bc24a2e
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> >>>> @@ -0,0 +1,53 @@
> >>>> +Altera FPGA/HPS Bridge Driver
> >>>> +
> >>>> +This driver manages a bridge between a FPGA and a host processor system (HPS).
> >>>> +User space can enable or disable the bridge by writing a "1" or a "0",
> >>>> +respectively, to its enable file under bridge's entry in
> >>>> +/sys/class/fpga-bridge.  Typically, one disables the bridges before
> >>>> +reprogramming the FPGA.  Once the FPGA is reprogrammed, the bridges are
> >>>> +reenabled.
> >>>> +
> >>> 
> >>> NAK.
> >>> 
> >>> This is all linux specific and doesn't belong here.
> >>> 
> >> 
> >> We know. The DT spec hasn’t been updated for a while, and this is going to be
> >> part of what we want to do with the restarting of the ePAPR spec process.
> >> 
> > 
> > As we don't have a new spec yet, I go with the current state of the art.
> > And I don't see how "file under ... /sys/class/fpga-bridge" fits the
> > current spec.
> > 
> >> So absolutes like “DT is a hardware description only" might be too strong statements, that
> >> do not work in practice anymore.
> >> 
> >> There’s no such thing as pure hardware devices lately - there is always a configuration
> >> component; some of it OS specific, some of it not.
> >> 
> > 
> > If it really is necessary, I'm not against it, don't get me wrong.
> > But in the grand scheme I wouldn't say that this fits my experience.
> > There are some devices that need configuration and often when it is
> > done in the DT, it is just a shortcut or not thought through.
> > And can be also introspected dynamically. With some discussion on the
> > list these bindings often change.
> > 
> 
> Already answered and given a possible way this might work; admittedly this
> specific example is not a good fit for what I was trying to say, but whatever.
> Let’s get the ball rolling.
> 

I would be okay with chosen-node. Not for this driver; but in general.

> > Regarding OS specifics: already there, e.g. console setup in the chosen node.
> > And other things I saw are ethernet aliases just for u-boot. Not a problem
> > of the spec, just a problem of u-boot and unneccessary "configuration".
> > Just to fix a bad bootloader. barebox can handle this without any
> > such stuff. Maybe we should integrate the DT "overlays" barebox uses into
> > the in-kernel DTs as well...but does it really help/interest someone who
> > decides to use u-boot where barebox stores its environment? I guess not.
> > 
> 
> Although I’m not against having DT overlays in the bootloader, I only see them
> as a method that helps a platform developer express things easier. I am completely
> against having the kernel tied to any particular bootloader.
> 
> We’ve all been through the hell where a kernel only boots if the bootloader has 
> setup the platform “correctly”. This “correctly” has a very loose definition and
> 99/100 times is extremely badly documented or not at all.
> 
> IMHO the bootloader should (as part of the normal boot sequence) only setup the
> absolutely bare minimum and then pass control to the kernel as soon as possible.
> 
> The full featured bootloader environment should only be presented when the user
> requests so.
> 

Totally agree. The kernel shouldn't be tied to any bootloader if at all possible
(the SoCFPGA is an especially bad example here again, as the pinmuxing can only
happen in the bootloader).
But should the DT include stuff than, that is only interesting for linux?

> >> We have to be engaged in the process and figure out how to update the spec for what is
> >> now the state of the art in the industry.
> >> 
> > 
> > Again, not against that if it is necessary. For example your overlay stuff might
> > be a good update to the spec. Would be better if it is official instead of a "hack".
> > I want that for SoCFPGA.
> > 
> > But having looked at one or two vendor kernels+DTs, the state of the art in the
> > industry is: using DT wrong. (And doing HW wrong, which makes some updates to the
> > ePAPR necessary ;-))
> > 
> 
> Vendor H/W, vendor DT and a crack pipe. Or as I call it Monday.
> 

;-)

Regards,
Steffen
Steffen Trumtrar Oct. 27, 2014, 3:32 p.m. UTC | #10
On Mon, Oct 27, 2014 at 05:05:29PM +0200, Pantelis Antoniou wrote:
> Hi Mark,
> 
> > On Oct 27, 2014, at 17:01 , Mark Brown <broonie@kernel.org> wrote:
> > 
> > On Mon, Oct 27, 2014 at 01:48:02PM +0200, Pantelis Antoniou wrote:
> >>> On Oct 24, 2014, at 02:51 , atull@opensource.altera.com wrote:
> > 
> >>> + - init-val         : 0 if driver should disable bridge at startup
> >>> +                      1 if driver should enable bridge at startup
> >>> +                      driver leaves bridge in current state if property not
> >>> +		      specified.
> > 
> >> Isn’t init-val a boolean property? It’s not named very well.
> > 
> > It's not boolean, it's tristate - turn on, turn off or don't touch.
> > 
> 
> I see. Even then ‘init-val’ is cryptic. I’d prefer two booleans,
> enable-at-startup; disable-at-startup.
> 
> >> Along with the label, is kinda hard to defend as configuration in DT.
> > 
> > Yeah...  presumably this decision would fall out of the users?
> 
> Well, it’s the user that should make the decision, but the driver should
> pick it up. This works but it’s not very nice.
> 

Hm, convince me why this AXI bus is so special, that I even need an
"init-val" property? Other buses don't have that.
Why don't I add a property "init-val" to my SPI buses, so I can enable
it in the DT and still have it in reset, just because....

The bridges on the SoCFPGA are buses, from the HPS to the FPGA. If I have
written firmware to the FPGA and I have subnodes on that bus, I have to
get it out of reset and probe everything. Normal procedure, no ?!


Regards,
Steffen
Pantelis Antoniou Oct. 27, 2014, 3:45 p.m. UTC | #11
Hi Stefan,

> On Oct 27, 2014, at 17:32 , Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:
> 
> On Mon, Oct 27, 2014 at 05:05:29PM +0200, Pantelis Antoniou wrote:
>> Hi Mark,
>> 
>>> On Oct 27, 2014, at 17:01 , Mark Brown <broonie@kernel.org> wrote:
>>> 
>>> On Mon, Oct 27, 2014 at 01:48:02PM +0200, Pantelis Antoniou wrote:
>>>>> On Oct 24, 2014, at 02:51 , atull@opensource.altera.com wrote:
>>> 
>>>>> + - init-val         : 0 if driver should disable bridge at startup
>>>>> +                      1 if driver should enable bridge at startup
>>>>> +                      driver leaves bridge in current state if property not
>>>>> +		      specified.
>>> 
>>>> Isn’t init-val a boolean property? It’s not named very well.
>>> 
>>> It's not boolean, it's tristate - turn on, turn off or don't touch.
>>> 
>> 
>> I see. Even then ‘init-val’ is cryptic. I’d prefer two booleans,
>> enable-at-startup; disable-at-startup.
>> 
>>>> Along with the label, is kinda hard to defend as configuration in DT.
>>> 
>>> Yeah...  presumably this decision would fall out of the users?
>> 
>> Well, it’s the user that should make the decision, but the driver should
>> pick it up. This works but it’s not very nice.
>> 
> 
> Hm, convince me why this AXI bus is so special, that I even need an
> "init-val" property? Other buses don't have that.
> Why don't I add a property "init-val" to my SPI buses, so I can enable
> it in the DT and still have it in reset, just because....
> 
> The bridges on the SoCFPGA are buses, from the HPS to the FPGA. If I have
> written firmware to the FPGA and I have subnodes on that bus, I have to
> get it out of reset and probe everything. Normal procedure, no ?!
> 

Well, it’s not my speciality, but my understanding is that FPGAs take (considerable)
time to be programmed. If someone has already configured the ‘bus’ it is considered
a win to not reload the bitstream. I.e. if you boot with the bootloader having loaded
the bitstream already, you don’t want to do it again.

I’m afraid there’s no such analogue with standard hardware busses like SPI, where
the bus setup time is instantaneous.

> 
> Regards,
> Steffen
> 

Regards

— Pantelis

> -- 
> Pengutronix e.K.                           |                             |
> 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 |

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pantelis Antoniou Oct. 27, 2014, 3:52 p.m. UTC | #12
Hi Steffen,

> On Oct 27, 2014, at 17:23 , Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:
> 
> Hi!
> 
> On Mon, Oct 27, 2014 at 01:54:20PM +0200, Pantelis Antoniou wrote:
>> Hi Stefen,
>> 
>>> On Oct 25, 2014, at 17:42 , Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:
>>> 
>>> Hi Pantelis!
>>> 
>>> On Fri, Oct 24, 2014 at 12:20:53PM +0300, Pantelis Antoniou wrote:
>>>> Hi Stefan, Allan,
>>>> 
>>>> Sorry, haven’t had a chance to review all this yet; will do so in the weekend.
>>>> Just wanted to pop in and comment on a few things.
>>>> 
>>>>> On Oct 24, 2014, at 10:00 AM, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:
>>>>> 
>>>>> Hi!
>>>>> 
>>>>> On Thu, Oct 23, 2014 at 06:51:06PM -0500, atull@opensource.altera.com wrote:
>>>>>> From: Alan Tull <atull@opensource.altera.com>
>>>>> 
>>>>> (...)
>>>>> 
>>>>>> diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..bc24a2e
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
>>>>>> @@ -0,0 +1,53 @@
>>>>>> +Altera FPGA/HPS Bridge Driver
>>>>>> +
>>>>>> +This driver manages a bridge between a FPGA and a host processor system (HPS).
>>>>>> +User space can enable or disable the bridge by writing a "1" or a "0",
>>>>>> +respectively, to its enable file under bridge's entry in
>>>>>> +/sys/class/fpga-bridge.  Typically, one disables the bridges before
>>>>>> +reprogramming the FPGA.  Once the FPGA is reprogrammed, the bridges are
>>>>>> +reenabled.
>>>>>> +
>>>>> 
>>>>> NAK.
>>>>> 
>>>>> This is all linux specific and doesn't belong here.
>>>>> 
>>>> 
>>>> We know. The DT spec hasn’t been updated for a while, and this is going to be
>>>> part of what we want to do with the restarting of the ePAPR spec process.
>>>> 
>>> 
>>> As we don't have a new spec yet, I go with the current state of the art.
>>> And I don't see how "file under ... /sys/class/fpga-bridge" fits the
>>> current spec.
>>> 
>>>> So absolutes like “DT is a hardware description only" might be too strong statements, that
>>>> do not work in practice anymore.
>>>> 
>>>> There’s no such thing as pure hardware devices lately - there is always a configuration
>>>> component; some of it OS specific, some of it not.
>>>> 
>>> 
>>> If it really is necessary, I'm not against it, don't get me wrong.
>>> But in the grand scheme I wouldn't say that this fits my experience.
>>> There are some devices that need configuration and often when it is
>>> done in the DT, it is just a shortcut or not thought through.
>>> And can be also introspected dynamically. With some discussion on the
>>> list these bindings often change.
>>> 
>> 
>> Already answered and given a possible way this might work; admittedly this
>> specific example is not a good fit for what I was trying to say, but whatever.
>> Let’s get the ball rolling.
>> 
> 
> I would be okay with chosen-node. Not for this driver; but in general.
> 

Let’s figure out how to do it then…

>>> Regarding OS specifics: already there, e.g. console setup in the chosen node.
>>> And other things I saw are ethernet aliases just for u-boot. Not a problem
>>> of the spec, just a problem of u-boot and unneccessary "configuration".
>>> Just to fix a bad bootloader. barebox can handle this without any
>>> such stuff. Maybe we should integrate the DT "overlays" barebox uses into
>>> the in-kernel DTs as well...but does it really help/interest someone who
>>> decides to use u-boot where barebox stores its environment? I guess not.
>>> 
>> 
>> Although I’m not against having DT overlays in the bootloader, I only see them
>> as a method that helps a platform developer express things easier. I am completely
>> against having the kernel tied to any particular bootloader.
>> 
>> We’ve all been through the hell where a kernel only boots if the bootloader has 
>> setup the platform “correctly”. This “correctly” has a very loose definition and
>> 99/100 times is extremely badly documented or not at all.
>> 
>> IMHO the bootloader should (as part of the normal boot sequence) only setup the
>> absolutely bare minimum and then pass control to the kernel as soon as possible.
>> 
>> The full featured bootloader environment should only be presented when the user
>> requests so.
>> 
> 
> Totally agree. The kernel shouldn't be tied to any bootloader if at all possible
> (the SoCFPGA is an especially bad example here again, as the pinmuxing can only
> happen in the bootloader).

I’m not familiar with SoCFPGA. Why pinmuxing is only possible in the bootloader?
Can’t the bootloader configure the minimum pinmux config and then have Linux setup
the rest?

If we’re feeling particularly nasty, we might just require bootloaders to clean the
hardware state before passing control to Linux (besides memory controller setup) and
see what’s broken.

> But should the DT include stuff than, that is only interesting for linux?
> 

Like it or not it’s Linux that’s in the forefront of many hardware designs.

There are configuration settings in DT that are part of the way hardware is presented to
Linux and user-space, and have been for quite some time. We’ve been pretending they don’t
exist, but they are there…

We have a chance to try to get it right, so why not do so?

>>>> We have to be engaged in the process and figure out how to update the spec for what is
>>>> now the state of the art in the industry.
>>>> 
>>> 
>>> Again, not against that if it is necessary. For example your overlay stuff might
>>> be a good update to the spec. Would be better if it is official instead of a "hack".
>>> I want that for SoCFPGA.
>>> 
>>> But having looked at one or two vendor kernels+DTs, the state of the art in the
>>> industry is: using DT wrong. (And doing HW wrong, which makes some updates to the
>>> ePAPR necessary ;-))
>>> 
>> 
>> Vendor H/W, vendor DT and a crack pipe. Or as I call it Monday.
>> 
> 
> ;-)
> 
> Regards,
> Steffen
> 

Regards

— Pantelis

> -- 
> Pengutronix e.K.                           |                             |
> 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 |

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Oct. 27, 2014, 5:17 p.m. UTC | #13
On Mon, Oct 27, 2014 at 05:45:03PM +0200, Pantelis Antoniou wrote:

Please fix your mail client to word wrap at less than 80 columns.

> Well, it’s not my speciality, but my understanding is that FPGAs take (considerable)
> time to be programmed. If someone has already configured the ‘bus’ it is considered
> a win to not reload the bitstream. I.e. if you boot with the bootloader having loaded
> the bitstream already, you don’t want to do it again.

That's not what your property is saying, though - if it were just about
handover between the bootloader and the kernel that'd be one thing but
it's also got this additional possibility to instruct the kernel that
the device must be either disabled or explicitly programmed.  Having a
property that just says "this FPGA is programmed" covers the handover
case but this property does more than that.
Pantelis Antoniou Oct. 27, 2014, 5:21 p.m. UTC | #14
Hi Mark,

> On Oct 27, 2014, at 19:17 , Mark Brown <broonie@kernel.org> wrote:
> 
> On Mon, Oct 27, 2014 at 05:45:03PM +0200, Pantelis Antoniou wrote:
> 
> Please fix your mail client to word wrap at less than 80 columns.
> 
>> Well, it’s not my speciality, but my understanding is that FPGAs take (considerable)
>> time to be programmed. If someone has already configured the ‘bus’ it is considered
>> a win to not reload the bitstream. I.e. if you boot with the bootloader having loaded
>> the bitstream already, you don’t want to do it again.
> 
> That's not what your property is saying, though - if it were just about
> handover between the bootloader and the kernel that'd be one thing but
> it's also got this additional possibility to instruct the kernel that
> the device must be either disabled or explicitly programmed.  Having a
> property that just says "this FPGA is programmed" covers the handover
> case but this property does more than that.

It’s not my property. This is not my driver. The property is badly-named
as it is IMHO.

Why don’t we let Alan chime in?

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steffen Trumtrar Oct. 27, 2014, 5:47 p.m. UTC | #15
On Mon, Oct 27, 2014 at 05:52:02PM +0200, Pantelis Antoniou wrote:
> Hi Steffen,
> 
> > On Oct 27, 2014, at 17:23 , Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:
> > 
> > Hi!
> > 
> > On Mon, Oct 27, 2014 at 01:54:20PM +0200, Pantelis Antoniou wrote:
> >> Hi Stefen,
> >> 
> >>> On Oct 25, 2014, at 17:42 , Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:
> >>> 
> >>> Hi Pantelis!
> >>> 
> >>> On Fri, Oct 24, 2014 at 12:20:53PM +0300, Pantelis Antoniou wrote:
> >>>> Hi Stefan, Allan,
> >>>> 
> >>>> Sorry, haven’t had a chance to review all this yet; will do so in the weekend.
> >>>> Just wanted to pop in and comment on a few things.
> >>>> 
> >>>>> On Oct 24, 2014, at 10:00 AM, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:
> >>>>> 
> >>>>> Hi!
> >>>>> 
> >>>>> On Thu, Oct 23, 2014 at 06:51:06PM -0500, atull@opensource.altera.com wrote:
> >>>>>> From: Alan Tull <atull@opensource.altera.com>
> >>>>> 
> >>>>> (...)
> >>>>> 
> >>>>>> diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> >>>>>> new file mode 100644
> >>>>>> index 0000000..bc24a2e
> >>>>>> --- /dev/null
> >>>>>> +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> >>>>>> @@ -0,0 +1,53 @@
> >>>>>> +Altera FPGA/HPS Bridge Driver
> >>>>>> +
> >>>>>> +This driver manages a bridge between a FPGA and a host processor system (HPS).
> >>>>>> +User space can enable or disable the bridge by writing a "1" or a "0",
> >>>>>> +respectively, to its enable file under bridge's entry in
> >>>>>> +/sys/class/fpga-bridge.  Typically, one disables the bridges before
> >>>>>> +reprogramming the FPGA.  Once the FPGA is reprogrammed, the bridges are
> >>>>>> +reenabled.
> >>>>>> +
> >>>>> 
> >>>>> NAK.
> >>>>> 
> >>>>> This is all linux specific and doesn't belong here.
> >>>>> 
> >>>> 
> >>>> We know. The DT spec hasn’t been updated for a while, and this is going to be
> >>>> part of what we want to do with the restarting of the ePAPR spec process.
> >>>> 
> >>> 
> >>> As we don't have a new spec yet, I go with the current state of the art.
> >>> And I don't see how "file under ... /sys/class/fpga-bridge" fits the
> >>> current spec.
> >>> 
> >>>> So absolutes like “DT is a hardware description only" might be too strong statements, that
> >>>> do not work in practice anymore.
> >>>> 
> >>>> There’s no such thing as pure hardware devices lately - there is always a configuration
> >>>> component; some of it OS specific, some of it not.
> >>>> 
> >>> 
> >>> If it really is necessary, I'm not against it, don't get me wrong.
> >>> But in the grand scheme I wouldn't say that this fits my experience.
> >>> There are some devices that need configuration and often when it is
> >>> done in the DT, it is just a shortcut or not thought through.
> >>> And can be also introspected dynamically. With some discussion on the
> >>> list these bindings often change.
> >>> 
> >> 
> >> Already answered and given a possible way this might work; admittedly this
> >> specific example is not a good fit for what I was trying to say, but whatever.
> >> Let’s get the ball rolling.
> >> 
> > 
> > I would be okay with chosen-node. Not for this driver; but in general.
> > 
> 
> Let’s figure out how to do it then…
> 
> >>> Regarding OS specifics: already there, e.g. console setup in the chosen node.
> >>> And other things I saw are ethernet aliases just for u-boot. Not a problem
> >>> of the spec, just a problem of u-boot and unneccessary "configuration".
> >>> Just to fix a bad bootloader. barebox can handle this without any
> >>> such stuff. Maybe we should integrate the DT "overlays" barebox uses into
> >>> the in-kernel DTs as well...but does it really help/interest someone who
> >>> decides to use u-boot where barebox stores its environment? I guess not.
> >>> 
> >> 
> >> Although I’m not against having DT overlays in the bootloader, I only see them
> >> as a method that helps a platform developer express things easier. I am completely
> >> against having the kernel tied to any particular bootloader.
> >> 
> >> We’ve all been through the hell where a kernel only boots if the bootloader has 
> >> setup the platform “correctly”. This “correctly” has a very loose definition and
> >> 99/100 times is extremely badly documented or not at all.
> >> 
> >> IMHO the bootloader should (as part of the normal boot sequence) only setup the
> >> absolutely bare minimum and then pass control to the kernel as soon as possible.
> >> 
> >> The full featured bootloader environment should only be presented when the user
> >> requests so.
> >> 
> > 
> > Totally agree. The kernel shouldn't be tied to any bootloader if at all possible
> > (the SoCFPGA is an especially bad example here again, as the pinmuxing can only
> > happen in the bootloader).
> 
> I’m not familiar with SoCFPGA. Why pinmuxing is only possible in the bootloader?
> Can’t the bootloader configure the minimum pinmux config and then have Linux setup
> the rest?
> 
> If we’re feeling particularly nasty, we might just require bootloaders to clean the
> hardware state before passing control to Linux (besides memory controller setup) and
> see what’s broken.
> 

Okay, offtopic, but:

a) datasheet explicitely states that dynamic pinmuxing is not supported.
b) last time I checked, no documentation at all. You get some magic values spit
   out by Altera's tools.
c) you don't have registers. Pinmuxing is done via JTAG scan chain; which you
   feed the magic values.

Apart from that, it seems to be the most sane idea, to at least write your
driver in a way, that doesn't make any assumptions on HW state.
But I think this is already common practice in mainline.

> > But should the DT include stuff than, that is only interesting for linux?
> > 
> 
> Like it or not it’s Linux that’s in the forefront of many hardware designs.
> 

I like it; pays my rent ;-)
But I also like freedom to choose...

> There are configuration settings in DT that are part of the way hardware is presented to
> Linux and user-space, and have been for quite some time. We’ve been pretending they don’t
> exist, but they are there…
> 

As far as I know, a lot of that are older bindings, where more
things just got applied, because it wasn't clear where DT will go.


Regards,
Steffen
Steffen Trumtrar Oct. 27, 2014, 6 p.m. UTC | #16
On Mon, Oct 27, 2014 at 05:45:03PM +0200, Pantelis Antoniou wrote:
> Hi Stefan,
> 
> > On Oct 27, 2014, at 17:32 , Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:
> > 
> > On Mon, Oct 27, 2014 at 05:05:29PM +0200, Pantelis Antoniou wrote:
> >> Hi Mark,
> >> 
> >>> On Oct 27, 2014, at 17:01 , Mark Brown <broonie@kernel.org> wrote:
> >>> 
> >>> On Mon, Oct 27, 2014 at 01:48:02PM +0200, Pantelis Antoniou wrote:
> >>>>> On Oct 24, 2014, at 02:51 , atull@opensource.altera.com wrote:
> >>> 
> >>>>> + - init-val         : 0 if driver should disable bridge at startup
> >>>>> +                      1 if driver should enable bridge at startup
> >>>>> +                      driver leaves bridge in current state if property not
> >>>>> +		      specified.
> >>> 
> >>>> Isn’t init-val a boolean property? It’s not named very well.
> >>> 
> >>> It's not boolean, it's tristate - turn on, turn off or don't touch.
> >>> 
> >> 
> >> I see. Even then ‘init-val’ is cryptic. I’d prefer two booleans,
> >> enable-at-startup; disable-at-startup.
> >> 
> >>>> Along with the label, is kinda hard to defend as configuration in DT.
> >>> 
> >>> Yeah...  presumably this decision would fall out of the users?
> >> 
> >> Well, it’s the user that should make the decision, but the driver should
> >> pick it up. This works but it’s not very nice.
> >> 
> > 
> > Hm, convince me why this AXI bus is so special, that I even need an
> > "init-val" property? Other buses don't have that.
> > Why don't I add a property "init-val" to my SPI buses, so I can enable
> > it in the DT and still have it in reset, just because....
> > 
> > The bridges on the SoCFPGA are buses, from the HPS to the FPGA. If I have
> > written firmware to the FPGA and I have subnodes on that bus, I have to
> > get it out of reset and probe everything. Normal procedure, no ?!
> > 
> 
> Well, it’s not my speciality, but my understanding is that FPGAs take (considerable)
> time to be programmed. If someone has already configured the ‘bus’ it is considered
> a win to not reload the bitstream. I.e. if you boot with the bootloader having loaded
> the bitstream already, you don’t want to do it again.
> 
> I’m afraid there’s no such analogue with standard hardware busses like SPI, where
> the bus setup time is instantaneous.

Ah, okay. I see why you got confused.
The bridges are not in any way responsible for loading the FPGA nor will
resetting them reset the bitstream.

The FPGA Manager, a different IP core, is responsible for that. And AFAIK
it has a status bit, that tells you if the FPGA is programmed or not.
Loading the bitstream is in the order of milliseconds.

So from the bridges point of view, when you probe it, you can ask the
FPGA manager if is done, otherwise -EPROBE_DEFER and than go on and
probe the subdevices.


If you are bored, take a look at
http://www.altera.com/literature/hb/cyclone-v/cv_54004.pdf
page 7-2. There you can see the hardware setup.

Regards,
Steffen
Pantelis Antoniou Oct. 27, 2014, 6:03 p.m. UTC | #17
Hi Steffen,

> On Oct 27, 2014, at 20:00 , Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:
> 
> On Mon, Oct 27, 2014 at 05:45:03PM +0200, Pantelis Antoniou wrote:
>> Hi Stefan,
>> 
>>> On Oct 27, 2014, at 17:32 , Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:
>>> 
>>> On Mon, Oct 27, 2014 at 05:05:29PM +0200, Pantelis Antoniou wrote:
>>>> Hi Mark,
>>>> 
>>>>> On Oct 27, 2014, at 17:01 , Mark Brown <broonie@kernel.org> wrote:
>>>>> 
>>>>> On Mon, Oct 27, 2014 at 01:48:02PM +0200, Pantelis Antoniou wrote:
>>>>>>> On Oct 24, 2014, at 02:51 , atull@opensource.altera.com wrote:
>>>>> 
>>>>>>> + - init-val         : 0 if driver should disable bridge at startup
>>>>>>> +                      1 if driver should enable bridge at startup
>>>>>>> +                      driver leaves bridge in current state if property not
>>>>>>> +		      specified.
>>>>> 
>>>>>> Isn’t init-val a boolean property? It’s not named very well.
>>>>> 
>>>>> It's not boolean, it's tristate - turn on, turn off or don't touch.
>>>>> 
>>>> 
>>>> I see. Even then ‘init-val’ is cryptic. I’d prefer two booleans,
>>>> enable-at-startup; disable-at-startup.
>>>> 
>>>>>> Along with the label, is kinda hard to defend as configuration in DT.
>>>>> 
>>>>> Yeah...  presumably this decision would fall out of the users?
>>>> 
>>>> Well, it’s the user that should make the decision, but the driver should
>>>> pick it up. This works but it’s not very nice.
>>>> 
>>> 
>>> Hm, convince me why this AXI bus is so special, that I even need an
>>> "init-val" property? Other buses don't have that.
>>> Why don't I add a property "init-val" to my SPI buses, so I can enable
>>> it in the DT and still have it in reset, just because....
>>> 
>>> The bridges on the SoCFPGA are buses, from the HPS to the FPGA. If I have
>>> written firmware to the FPGA and I have subnodes on that bus, I have to
>>> get it out of reset and probe everything. Normal procedure, no ?!
>>> 
>> 
>> Well, it’s not my speciality, but my understanding is that FPGAs take (considerable)
>> time to be programmed. If someone has already configured the ‘bus’ it is considered
>> a win to not reload the bitstream. I.e. if you boot with the bootloader having loaded
>> the bitstream already, you don’t want to do it again.
>> 
>> I’m afraid there’s no such analogue with standard hardware busses like SPI, where
>> the bus setup time is instantaneous.
> 
> Ah, okay. I see why you got confused.
> The bridges are not in any way responsible for loading the FPGA nor will
> resetting them reset the bitstream.
> 
> The FPGA Manager, a different IP core, is responsible for that. And AFAIK
> it has a status bit, that tells you if the FPGA is programmed or not.
> Loading the bitstream is in the order of milliseconds.
> 
> So from the bridges point of view, when you probe it, you can ask the
> FPGA manager if is done, otherwise -EPROBE_DEFER and than go on and
> probe the subdevices.
> 
> 

Ohh, I see. Sorry for the confusion.

> If you are bored, take a look at
> http://www.altera.com/literature/hb/cyclone-v/cv_54004.pdf
> page 7-2. There you can see the hardware setup.
> 

Thanks. /me throws it at the pile of things to read…


> Regards,
> Steffen
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> 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 |

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
atull Oct. 28, 2014, 9:19 p.m. UTC | #18
On Fri, 24 Oct 2014, Steffen Trumtrar wrote:

> Hi!
> 

Hi,

I see that my documentation sucks and needs cleanup.  I'll try to
answer some of the flames and get a more coherent version out soon.

> On Thu, Oct 23, 2014 at 06:51:06PM -0500, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> 
> (...)
> 
> > diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> > new file mode 100644
> > index 0000000..bc24a2e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> > @@ -0,0 +1,53 @@
> > +Altera FPGA/HPS Bridge Driver
> > +
> > +This driver manages a bridge between a FPGA and a host processor system (HPS).
> > +User space can enable or disable the bridge by writing a "1" or a "0",
> > +respectively, to its enable file under bridge's entry in
> > +/sys/class/fpga-bridge.  Typically, one disables the bridges before
> > +reprogramming the FPGA.  Once the FPGA is reprogrammed, the bridges are
> > +reenabled.
> > +
> 
> NAK.
> 
> This is all linux specific and doesn't belong here.

Right.  This stuff shouldn't be in this document.  While I was squashing
patches and cleaning up for posting on the mailing list, I added a sysfs
document and forgot to clean up my DT bindings documents.

> 
> > +Required properties:
> > +
> > + - compatible     : should contain one of:
> > +                     "altr,socfpga-hps2fpga-bridge"
> > +                     "altr,socfpga-lwhps2fpga-bridge"
> > +                     "altr,socfpga-fpga2hps-bridge"
> > +
> > + - clocks         : clocks used by this module
> > +
> > + - altr,l3-syscon : phandle of the l3 interconnect module
> > +
> 
> L3 shouldn't be a syscon.

L3 is actually a good candidate for syscon.  Lots of registers, each one
affects a different hardware block.

> Have you tried dumping the regmap in the debugfs if L3
> is a syscon? Doesn't work.

Is that a bug in regmap or is that because the register in L3 that
I am actully interested in here is write-only (ugh)?

> 
> > +Optional properties:
> > + - label          : name that you want this bridge to show up as under
> > +                    /sys/class/fpga-bridge.  Default is br<device#> if this is
> > +                    not specified.
> > +
> 
> Why? Linux-specific.

That was a convience for the user.  I can take that out and won't miss it.

> 
> > + - init-val       : 0 if driver should disable bridge at startup
> > +                    1 if driver should enable bridge at startup
> > +                    driver leaves bridge in current state if property not
> > +                    specified.
> > +
> 
> Configuration in the DT? Really?
> 
> > +Example:
> > +	hps_fpgabridge0: fpgabridge@0 {
> > +		compatible = "altr,socfpga-hps2fpga-bridge";
> > +		label = "hps2fpga";
> > +		altr,l3-syscon = <&l3regs>;
> > +		clocks = <&l4_main_clk>;
> > +		init-val = <1>;
> > +	};
> > +
> > +	hps_fpgabridge1: fpgabridge@1 {
> > +		compatible = "altr,socfpga-lwhps2fpga-bridge";
> > +		label = "lwhps2fpga";
> > +		altr,l3-syscon = <&l3regs>;
> > +		clocks = <&l4_main_clk>;
> > +		init-val = <0>;
> > +	};
> > +
> > +	hps_fpgabridge2: fpgabridge@2 {
> > +		compatible = "altr,socfpga-fpga2hps-bridge";
> > +		label = "fpga2hps";
> > +		altr,l3-syscon = <&l3regs>;
> > +		clocks = <&l4_main_clk>;
> > +	};
> 
> The bridges are the buses into the FPGA. This has to be accomodated.
> The bridges have two specified memory ranges: one the address space
> of the bus, the second the register space for configuration.
> 
> This binding does NOT correctly describe the hardware. Sorry.
> 

OK that was outdated.  More cleanup needed.  How about this type of
binding?  Here's an actual use case for something that has multiple
pieces of soft IP in the FPGA (sysid, gpio).  I eliminated a few.

*snippet of DT*
sopc@0 {
	device_type = "soc";
	ranges;
	#address-cells = <0x1>;
	#size-cells = <0x1>;
	compatible = "ALTR,avalon", "simple-bus";
	bus-frequency = <0x0>;

	bridge@0xc0000000 {
		compatible = "altr,bridge-14.0", "simple-bus";
		reg = <0xc0000000 0x20000000 0xff200000 0x200000>;
		reg-names = "axi_h2f", "axi_h2f_lw";
		clocks = <0x2 0x2 0x2>;
		clock-names = "h2f_axi_clock", "h2f_lw_axi_clock",
		              "f2h_sdram0_clock";
		#address-cells = <0x2>;
		#size-cells = <0x1>;
		ranges = <0x0 0x0 0xc0000000 0x10000
		          0x1 0x10000 0xff210000 0x8
			  0x1 0x10040 0xff210040 0x20 
			  0x1 0x10080 0xff210080 0x10 
			  0x1 0x100c0 0xff2100c0 0x10 
			  0x1 0x20000 0xff220000 0x8>;

		sysid@0x100010000 {
			compatible = "altr,sysid-14.0", "altr,sysid-1.0";
			reg = <0x1 0x10000 0x8>;
			clocks = <0x2>;
			id = <0xacd51402>;
			timestamp = <0x540a048e>;
		};

		gpio@0x100010040 {
			compatible = "altr,pio-14.0", "altr,pio-1.0";
			reg = <0x1 0x10040 0x20>;
			clocks = <0x2>;
			altr,gpio-bank-width = <0x4>;
			resetvalue = <0x0>;
			#gpio-cells = <0x2>;
			gpio-controller;
			linux,phandle = <0x2a>;
		};

		/* other hardware that exists on FPGA here */

	};
};

Alan

> Regards,
> Steffen
> 
> -- 
> Pengutronix e.K.                           |                             |
> 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 |
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
atull Oct. 28, 2014, 9:53 p.m. UTC | #19
On Tue, 28 Oct 2014, atull wrote:

> On Fri, 24 Oct 2014, Steffen Trumtrar wrote:

> > 
> > > + - init-val       : 0 if driver should disable bridge at startup
> > > +                    1 if driver should enable bridge at startup
> > > +                    driver leaves bridge in current state if property not
> > > +                    specified.
> > > +
> > 
> > Configuration in the DT? Really?

The FPGA can be programmed by several different methods (eeprom, jtag,
preloader, Linux).  The guiding principle is: whoever programmed the
FPGA should release the bridge.  At one point, we thought 'init-val'
would be needed, but I think it can be taken out and we won't miss it.

Alan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steffen Trumtrar Oct. 29, 2014, 7:57 a.m. UTC | #20
Hi!

On Tue, Oct 28, 2014 at 04:19:03PM -0500, atull wrote:
> On Fri, 24 Oct 2014, Steffen Trumtrar wrote:
> 
> > Hi!
> > 
> 
> Hi,
> 
> I see that my documentation sucks and needs cleanup.  I'll try to
> answer some of the flames and get a more coherent version out soon.
> 
> > On Thu, Oct 23, 2014 at 06:51:06PM -0500, atull@opensource.altera.com wrote:
> > > From: Alan Tull <atull@opensource.altera.com>
> > 
> > (...)
> > 
> > > diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> > > new file mode 100644
> > > index 0000000..bc24a2e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> > > @@ -0,0 +1,53 @@
> > > +Altera FPGA/HPS Bridge Driver
> > > +
> > > +This driver manages a bridge between a FPGA and a host processor system (HPS).
> > > +User space can enable or disable the bridge by writing a "1" or a "0",
> > > +respectively, to its enable file under bridge's entry in
> > > +/sys/class/fpga-bridge.  Typically, one disables the bridges before
> > > +reprogramming the FPGA.  Once the FPGA is reprogrammed, the bridges are
> > > +reenabled.
> > > +
> > 
> > NAK.
> > 
> > This is all linux specific and doesn't belong here.
> 
> Right.  This stuff shouldn't be in this document.  While I was squashing
> patches and cleaning up for posting on the mailing list, I added a sysfs
> document and forgot to clean up my DT bindings documents.
> 
> > 
> > > +Required properties:
> > > +
> > > + - compatible     : should contain one of:
> > > +                     "altr,socfpga-hps2fpga-bridge"
> > > +                     "altr,socfpga-lwhps2fpga-bridge"
> > > +                     "altr,socfpga-fpga2hps-bridge"
> > > +
> > > + - clocks         : clocks used by this module
> > > +
> > > + - altr,l3-syscon : phandle of the l3 interconnect module
> > > +
> > 
> > L3 shouldn't be a syscon.
> 
> L3 is actually a good candidate for syscon.  Lots of registers, each one
> affects a different hardware block.
> 
> > Have you tried dumping the regmap in the debugfs if L3
> > is a syscon? Doesn't work.
> 
> Is that a bug in regmap or is that because the register in L3 that
> I am actully interested in here is write-only (ugh)?
>

That is not a bug in regmap. It is because you only have a register at 0x4008,
than one at 0x4104, that one at 0x5008, etc.... (values from memory, so might
be wrong)

Of course regmap tries to read from the whole register range if specified as
syscon. Which it can't.

So, that shouldn't be a problem though, as I already cooked up a driver for
the L3 with all the ranges specified. The only thing I need to figure out
before I will post it, is how to nicely handle the WO remap register.
I think regmap might be able to handle this itself, but am not sure yet.

> > 
> > > +Optional properties:
> > > + - label          : name that you want this bridge to show up as under
> > > +                    /sys/class/fpga-bridge.  Default is br<device#> if this is
> > > +                    not specified.
> > > +
> > 
> > Why? Linux-specific.
> 
> That was a convience for the user.  I can take that out and won't miss it.
> 
> > 
> > > + - init-val       : 0 if driver should disable bridge at startup
> > > +                    1 if driver should enable bridge at startup
> > > +                    driver leaves bridge in current state if property not
> > > +                    specified.
> > > +
> > 
> > Configuration in the DT? Really?
> > 
> > > +Example:
> > > +	hps_fpgabridge0: fpgabridge@0 {
> > > +		compatible = "altr,socfpga-hps2fpga-bridge";
> > > +		label = "hps2fpga";
> > > +		altr,l3-syscon = <&l3regs>;
> > > +		clocks = <&l4_main_clk>;
> > > +		init-val = <1>;
> > > +	};
> > > +
> > > +	hps_fpgabridge1: fpgabridge@1 {
> > > +		compatible = "altr,socfpga-lwhps2fpga-bridge";
> > > +		label = "lwhps2fpga";
> > > +		altr,l3-syscon = <&l3regs>;
> > > +		clocks = <&l4_main_clk>;
> > > +		init-val = <0>;
> > > +	};
> > > +
> > > +	hps_fpgabridge2: fpgabridge@2 {
> > > +		compatible = "altr,socfpga-fpga2hps-bridge";
> > > +		label = "fpga2hps";
> > > +		altr,l3-syscon = <&l3regs>;
> > > +		clocks = <&l4_main_clk>;
> > > +	};
> > 
> > The bridges are the buses into the FPGA. This has to be accomodated.
> > The bridges have two specified memory ranges: one the address space
> > of the bus, the second the register space for configuration.
> > 
> > This binding does NOT correctly describe the hardware. Sorry.
> > 
> 
> OK that was outdated.  More cleanup needed.  How about this type of
> binding?  Here's an actual use case for something that has multiple
> pieces of soft IP in the FPGA (sysid, gpio).  I eliminated a few.
> 
> *snippet of DT*
> sopc@0 {
> 	device_type = "soc";
> 	ranges;
> 	#address-cells = <0x1>;
> 	#size-cells = <0x1>;
> 	compatible = "ALTR,avalon", "simple-bus";
> 	bus-frequency = <0x0>;
> 
> 	bridge@0xc0000000 {
> 		compatible = "altr,bridge-14.0", "simple-bus";
> 		reg = <0xc0000000 0x20000000 0xff200000 0x200000>;
> 		reg-names = "axi_h2f", "axi_h2f_lw";
> 		clocks = <0x2 0x2 0x2>;
> 		clock-names = "h2f_axi_clock", "h2f_lw_axi_clock",
> 		              "f2h_sdram0_clock";
> 		#address-cells = <0x2>;
> 		#size-cells = <0x1>;
> 		ranges = <0x0 0x0 0xc0000000 0x10000
> 		          0x1 0x10000 0xff210000 0x8
> 			  0x1 0x10040 0xff210040 0x20 
> 			  0x1 0x10080 0xff210080 0x10 
> 			  0x1 0x100c0 0xff2100c0 0x10 
> 			  0x1 0x20000 0xff220000 0x8>;
> 
> 		sysid@0x100010000 {
> 			compatible = "altr,sysid-14.0", "altr,sysid-1.0";
> 			reg = <0x1 0x10000 0x8>;
> 			clocks = <0x2>;
> 			id = <0xacd51402>;
> 			timestamp = <0x540a048e>;
> 		};
> 
> 		gpio@0x100010040 {
> 			compatible = "altr,pio-14.0", "altr,pio-1.0";
> 			reg = <0x1 0x10040 0x20>;
> 			clocks = <0x2>;
> 			altr,gpio-bank-width = <0x4>;
> 			resetvalue = <0x0>;
> 			#gpio-cells = <0x2>;
> 			gpio-controller;
> 			linux,phandle = <0x2a>;
> 		};
> 
> 		/* other hardware that exists on FPGA here */
> 
> 	};
> };
> 

Yes, something like this seems appropriate. Again, I also cooked up a driver
for this. I need to figure out why the GPV register writing sometimes doesn't
work as expected and will try to marry it to your FPGA manager framework
before I consider sending it to the list.

My proposed binding at the moment looks like:

Example:

	hps2fpga: axibridge@ff500000 {
		#address-cells = <1>;
		#size-cells = <1>;
		compatible = "altr,hps2fpga-axi-bridge";
		reg = <0xff500000 0x100000>,
		      <0xc0000000 0x3c000000>;
		clocks = <&l4_mp_clk>, <&l3_main_clk>;
		clock-names = "gpv_clk", "data_clk";
		resets = <&rst HPS2FPGA_RESET>;
		reset-names = "hps2fpga";
		altr,gpv-master = <&lwhps2fpga>;
		altr,l3-gpv = <&l3regs>;
		bus-width = <64>;
		status = "disabled";
		ranges;
	};

Board file example:

	&hps2fpga {
		bus-width = <32>;
		status = "okay";

		axi-ip: axi-ip@c0000000 {
			compatible = "axi-ip";
			reg = <0xc0000000 0x10000>;
			clocks = <&h2f_usr2_clk>;
			status = "okay";
		};
	};

So we seem to be almost on the same page. I first had the simple-bus in
there, too. But this will lead to all sorts of problems regarding driver
probing/removing on that bus.

Regards,
Steffen
Pavel Machek Oct. 29, 2014, 8:24 a.m. UTC | #21
Hi!

> > > +Optional properties:
> > > + - label          : name that you want this bridge to show up as under
> > > +                    /sys/class/fpga-bridge.  Default is br<device#> if this is
> > > +                    not specified.
> > > +
> > 
> > Why? Linux-specific.
> 
> That was a convience for the user.  I can take that out and won't
> miss it.

Actually make it

- label : optional user-readable name for this bridge

...and it is no longer Linux-specific, and you can keep it if it is
otherwise useful...
									Pavel
Mark Brown Oct. 29, 2014, 10:16 a.m. UTC | #22
On Wed, Oct 29, 2014 at 08:57:01AM +0100, Steffen Trumtrar wrote:

> So, that shouldn't be a problem though, as I already cooked up a driver for
> the L3 with all the ranges specified. The only thing I need to figure out
> before I will post it, is how to nicely handle the WO remap register.
> I think regmap might be able to handle this itself, but am not sure yet.

It's supposed to be able to, not sure if anyone really tests that or not
though so it's possible bugs crept in.  Most write only registers can
physically be read so actually come out as volatile rather than write
only even if functionally there is no reason to read.
Steffen Trumtrar Oct. 29, 2014, 10:31 a.m. UTC | #23
On Wed, Oct 29, 2014 at 10:16:32AM +0000, Mark Brown wrote:
> On Wed, Oct 29, 2014 at 08:57:01AM +0100, Steffen Trumtrar wrote:
> 
> > So, that shouldn't be a problem though, as I already cooked up a driver for
> > the L3 with all the ranges specified. The only thing I need to figure out
> > before I will post it, is how to nicely handle the WO remap register.
> > I think regmap might be able to handle this itself, but am not sure yet.
> 
> It's supposed to be able to, not sure if anyone really tests that or not
> though so it's possible bugs crept in.  Most write only registers can
> physically be read so actually come out as volatile rather than write
> only even if functionally there is no reason to read.

Ah, I think you might be actually right there. IIRC it IS possible to read
from that register, but you only get 0s.
So, I shot myself in the knee with specifying that register as write-only
in my driver.

Thanks,
Steffen
atull Oct. 29, 2014, 2:30 p.m. UTC | #24
On Wed, 29 Oct 2014, Steffen Trumtrar wrote:

> On Wed, Oct 29, 2014 at 10:16:32AM +0000, Mark Brown wrote:
> > On Wed, Oct 29, 2014 at 08:57:01AM +0100, Steffen Trumtrar wrote:
> > 
> > > So, that shouldn't be a problem though, as I already cooked up a driver for
> > > the L3 with all the ranges specified. The only thing I need to figure out
> > > before I will post it, is how to nicely handle the WO remap register.
> > > I think regmap might be able to handle this itself, but am not sure yet.
> > 
> > It's supposed to be able to, not sure if anyone really tests that or not
> > though so it's possible bugs crept in.  Most write only registers can
> > physically be read so actually come out as volatile rather than write
> > only even if functionally there is no reason to read.
> 
> Ah, I think you might be actually right there. IIRC it IS possible to read
> from that register, but you only get 0s.

Yes, you can read and it will be all 0s.  So read-modify-write is not
something you would want to do for instance.

Alan

> So, I shot myself in the knee with specifying that register as write-only
> in my driver.
> 
> Thanks,
> Steffen
> 
> -- 
> Pengutronix e.K.                           |                             |
> 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 |
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
atull Oct. 29, 2014, 8:39 p.m. UTC | #25
On Wed, 29 Oct 2014, Pavel Machek wrote:

> Hi!
> 
> > > > +Optional properties:
> > > > + - label          : name that you want this bridge to show up as under
> > > > +                    /sys/class/fpga-bridge.  Default is br<device#> if this is
> > > > +                    not specified.
> > > > +
> > > 
> > > Why? Linux-specific.
> > 
> > That was a convience for the user.  I can take that out and won't
> > miss it.
> 
> Actually make it
> 
> - label : optional user-readable name for this bridge
> 
> ...and it is no longer Linux-specific, and you can keep it if it is
> otherwise useful...
> 									Pavel

Great!  Thanks for the review.  In the future I'll keep my sysfs 
documentation separate from my DT bindings!

Alan

> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
atull Oct. 29, 2014, 8:49 p.m. UTC | #26
On Wed, 29 Oct 2014, Steffen Trumtrar wrote:

> Hi!
> 
> On Tue, Oct 28, 2014 at 04:19:03PM -0500, atull wrote:
> > On Fri, 24 Oct 2014, Steffen Trumtrar wrote:
> > 
> > > Hi!
> > > 
> > 
> > Hi,
> > 
> > I see that my documentation sucks and needs cleanup.  I'll try to
> > answer some of the flames and get a more coherent version out soon.
> > 
> > > On Thu, Oct 23, 2014 at 06:51:06PM -0500, atull@opensource.altera.com wrote:
> > > > From: Alan Tull <atull@opensource.altera.com>
> > > 
> > > (...)
> > > 
> > > > diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> > > > new file mode 100644
> > > > index 0000000..bc24a2e
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
> > > > @@ -0,0 +1,53 @@
> > > > +Altera FPGA/HPS Bridge Driver
> > > > +
> > > > +This driver manages a bridge between a FPGA and a host processor system (HPS).
> > > > +User space can enable or disable the bridge by writing a "1" or a "0",
> > > > +respectively, to its enable file under bridge's entry in
> > > > +/sys/class/fpga-bridge.  Typically, one disables the bridges before
> > > > +reprogramming the FPGA.  Once the FPGA is reprogrammed, the bridges are
> > > > +reenabled.
> > > > +
> > > 
> > > NAK.
> > > 
> > > This is all linux specific and doesn't belong here.
> > 
> > Right.  This stuff shouldn't be in this document.  While I was squashing
> > patches and cleaning up for posting on the mailing list, I added a sysfs
> > document and forgot to clean up my DT bindings documents.
> > 
> > > 
> > > > +Required properties:
> > > > +
> > > > + - compatible     : should contain one of:
> > > > +                     "altr,socfpga-hps2fpga-bridge"
> > > > +                     "altr,socfpga-lwhps2fpga-bridge"
> > > > +                     "altr,socfpga-fpga2hps-bridge"
> > > > +
> > > > + - clocks         : clocks used by this module
> > > > +
> > > > + - altr,l3-syscon : phandle of the l3 interconnect module
> > > > +
> > > 
> > > L3 shouldn't be a syscon.
> > 
> > L3 is actually a good candidate for syscon.  Lots of registers, each one
> > affects a different hardware block.
> > 
> > > Have you tried dumping the regmap in the debugfs if L3
> > > is a syscon? Doesn't work.
> > 
> > Is that a bug in regmap or is that because the register in L3 that
> > I am actully interested in here is write-only (ugh)?
> >
> 
> That is not a bug in regmap. It is because you only have a register at 0x4008,
> than one at 0x4104, that one at 0x5008, etc.... (values from memory, so might
> be wrong)
> 
> Of course regmap tries to read from the whole register range if specified as
> syscon. Which it can't.
> 
> So, that shouldn't be a problem though, as I already cooked up a driver for
> the L3 with all the ranges specified. The only thing I need to figure out
> before I will post it, is how to nicely handle the WO remap register.
> I think regmap might be able to handle this itself, but am not sure yet.
> 
> > > 
> > > > +Optional properties:
> > > > + - label          : name that you want this bridge to show up as under
> > > > +                    /sys/class/fpga-bridge.  Default is br<device#> if this is
> > > > +                    not specified.
> > > > +
> > > 
> > > Why? Linux-specific.
> > 
> > That was a convience for the user.  I can take that out and won't miss it.
> > 
> > > 
> > > > + - init-val       : 0 if driver should disable bridge at startup
> > > > +                    1 if driver should enable bridge at startup
> > > > +                    driver leaves bridge in current state if property not
> > > > +                    specified.
> > > > +
> > > 
> > > Configuration in the DT? Really?
> > > 
> > > > +Example:
> > > > +	hps_fpgabridge0: fpgabridge@0 {
> > > > +		compatible = "altr,socfpga-hps2fpga-bridge";
> > > > +		label = "hps2fpga";
> > > > +		altr,l3-syscon = <&l3regs>;
> > > > +		clocks = <&l4_main_clk>;
> > > > +		init-val = <1>;
> > > > +	};
> > > > +
> > > > +	hps_fpgabridge1: fpgabridge@1 {
> > > > +		compatible = "altr,socfpga-lwhps2fpga-bridge";
> > > > +		label = "lwhps2fpga";
> > > > +		altr,l3-syscon = <&l3regs>;
> > > > +		clocks = <&l4_main_clk>;
> > > > +		init-val = <0>;
> > > > +	};
> > > > +
> > > > +	hps_fpgabridge2: fpgabridge@2 {
> > > > +		compatible = "altr,socfpga-fpga2hps-bridge";
> > > > +		label = "fpga2hps";
> > > > +		altr,l3-syscon = <&l3regs>;
> > > > +		clocks = <&l4_main_clk>;
> > > > +	};
> > > 
> > > The bridges are the buses into the FPGA. This has to be accomodated.
> > > The bridges have two specified memory ranges: one the address space
> > > of the bus, the second the register space for configuration.
> > > 
> > > This binding does NOT correctly describe the hardware. Sorry.
> > > 
> > 
> > OK that was outdated.  More cleanup needed.  How about this type of
> > binding?  Here's an actual use case for something that has multiple
> > pieces of soft IP in the FPGA (sysid, gpio).  I eliminated a few.
> > 
> > *snippet of DT*
> > sopc@0 {
> > 	device_type = "soc";
> > 	ranges;
> > 	#address-cells = <0x1>;
> > 	#size-cells = <0x1>;
> > 	compatible = "ALTR,avalon", "simple-bus";
> > 	bus-frequency = <0x0>;
> > 
> > 	bridge@0xc0000000 {
> > 		compatible = "altr,bridge-14.0", "simple-bus";
> > 		reg = <0xc0000000 0x20000000 0xff200000 0x200000>;
> > 		reg-names = "axi_h2f", "axi_h2f_lw";
> > 		clocks = <0x2 0x2 0x2>;
> > 		clock-names = "h2f_axi_clock", "h2f_lw_axi_clock",
> > 		              "f2h_sdram0_clock";
> > 		#address-cells = <0x2>;
> > 		#size-cells = <0x1>;
> > 		ranges = <0x0 0x0 0xc0000000 0x10000
> > 		          0x1 0x10000 0xff210000 0x8
> > 			  0x1 0x10040 0xff210040 0x20 
> > 			  0x1 0x10080 0xff210080 0x10 
> > 			  0x1 0x100c0 0xff2100c0 0x10 
> > 			  0x1 0x20000 0xff220000 0x8>;
> > 
> > 		sysid@0x100010000 {
> > 			compatible = "altr,sysid-14.0", "altr,sysid-1.0";
> > 			reg = <0x1 0x10000 0x8>;
> > 			clocks = <0x2>;
> > 			id = <0xacd51402>;
> > 			timestamp = <0x540a048e>;
> > 		};
> > 
> > 		gpio@0x100010040 {
> > 			compatible = "altr,pio-14.0", "altr,pio-1.0";
> > 			reg = <0x1 0x10040 0x20>;
> > 			clocks = <0x2>;
> > 			altr,gpio-bank-width = <0x4>;
> > 			resetvalue = <0x0>;
> > 			#gpio-cells = <0x2>;
> > 			gpio-controller;
> > 			linux,phandle = <0x2a>;
> > 		};
> > 
> > 		/* other hardware that exists on FPGA here */
> > 
> > 	};
> > };
> > 
> 
> Yes, something like this seems appropriate. Again, I also cooked up a driver
> for this. I need to figure out why the GPV register writing sometimes doesn't
> work as expected and will try to marry it to your FPGA manager framework
> before I consider sending it to the list.

Hi Steffen,

I look forward to seeing it.

> 
> My proposed binding at the moment looks like:
> 
> Example:
> 
> 	hps2fpga: axibridge@ff500000 {
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 		compatible = "altr,hps2fpga-axi-bridge";
> 		reg = <0xff500000 0x100000>,
> 		      <0xc0000000 0x3c000000>;
> 		clocks = <&l4_mp_clk>, <&l3_main_clk>;
> 		clock-names = "gpv_clk", "data_clk";
> 		resets = <&rst HPS2FPGA_RESET>;
> 		reset-names = "hps2fpga";
> 		altr,gpv-master = <&lwhps2fpga>;
> 		altr,l3-gpv = <&l3regs>;
> 		bus-width = <64>;
> 		status = "disabled";
> 		ranges;
> 	};
> 
> Board file example:
> 
> 	&hps2fpga {
> 		bus-width = <32>;
> 		status = "okay";
> 
> 		axi-ip: axi-ip@c0000000 {
> 			compatible = "axi-ip";
> 			reg = <0xc0000000 0x10000>;
> 			clocks = <&h2f_usr2_clk>;
> 			status = "okay";
> 		};
> 	};
> 
> So we seem to be almost on the same page. I first had the simple-bus in
> there, too. But this will lead to all sorts of problems regarding driver
> probing/removing on that bus.
> 

I think the problems we have had was that the bus had to be arch_initcall so
that it would be there before the drivers were.  Are there other problems
you are thinking of?  It would be good if we could get around that problem
as otherwise it will be a problem with the l3-nic not being early enough.

Alan


> Regards,
> Steffen
> 
> -- 
> Pengutronix e.K.                           |                             |
> 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 |
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
new file mode 100644
index 0000000..cc8f522
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/altera-fpga2sdram-bridge.txt
@@ -0,0 +1,57 @@ 
+Altera FPGA To SDRAM Bridge Driver
+
+This driver manages a bridge between an FPGA and the SDRAM used by an host
+processor system (HPS). The bridge contains a number read ports, write ports,
+and command ports.  Reconfiguring these ports requires that no SDRAM
+transactions occur during reconfiguration.  In other words, the code
+reconfiguring the ports cannot be run out of SDRAM nor can the FPGA access the
+SDRAM during the reconfiguration.  This driver does not support reconfiguring
+the ports.  Typcially, the ports are configured by code running out of onchip
+ram before Linux is started.
+
+This driver supports enabling and disabling of the configured ports all at
+once, which allows for safe reprogramming of the FPGA from user space, provided
+the new FPGA image uses the same port configuration. User space can enable or
+disable the bridge by writing a "1" or a "0", respectively, to its enable file
+under bridge's entry in /sys/class/fpga-bridge. Typically, one disables the
+bridges before reprogramming the FPGA.  Once the FPGA is reprogrammed, the
+bridges are reenabled.
+
+Required properties:
+
+ - compatible       : "altr,socfpga-fpga2sdram-bridge"
+
+ - read-ports-mask  : Bits 0 to 3 corresponds read ports 0 to 3. A bit set to 1
+                      indicates the corresponding read port should be enabled.
+
+ - write-ports-mask : Bits 0 to 3 corresponds write ports 0 to 3. A bit set
+                      to 1 indicates the corresponding write port should be
+                      enabled.
+
+ - cmd-ports-mask   : Bits 0 to 5 correspond to command ports 0 to 5. A bit set
+                      to 1 indicates the corresponding command port should be
+                      enabled.
+
+ - altr,sdr-syscon  : phandle of the sdr module
+
+Optional properties:
+
+ - label            : name that you want this bridge to show up as under
+                      /sys/class/fpga-bridge. Default is br<device#> if this is
+                      not specified.
+
+ - init-val         : 0 if driver should disable bridge at startup
+                      1 if driver should enable bridge at startup
+                      driver leaves bridge in current state if property not
+		      specified.
+
+Example:
+	fpga2sdram_br: fpgabridge@3 {
+		compatible = "altr,socfpga-fpga2sdram-bridge";
+		label = "fpga2sdram";
+		altr,sdr-syscon = <&sdr>;
+		read-ports-mask = <3>;
+		write-ports-mask = <3>;
+		cmd-ports-mask = <0xd>;
+		init-val = <0>;
+	};
diff --git a/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
new file mode 100644
index 0000000..bc24a2e
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/altera-hps2fpga-bridge.txt
@@ -0,0 +1,53 @@ 
+Altera FPGA/HPS Bridge Driver
+
+This driver manages a bridge between a FPGA and a host processor system (HPS).
+User space can enable or disable the bridge by writing a "1" or a "0",
+respectively, to its enable file under bridge's entry in
+/sys/class/fpga-bridge.  Typically, one disables the bridges before
+reprogramming the FPGA.  Once the FPGA is reprogrammed, the bridges are
+reenabled.
+
+Required properties:
+
+ - compatible     : should contain one of:
+                     "altr,socfpga-hps2fpga-bridge"
+                     "altr,socfpga-lwhps2fpga-bridge"
+                     "altr,socfpga-fpga2hps-bridge"
+
+ - clocks         : clocks used by this module
+
+ - altr,l3-syscon : phandle of the l3 interconnect module
+
+Optional properties:
+ - label          : name that you want this bridge to show up as under
+                    /sys/class/fpga-bridge.  Default is br<device#> if this is
+                    not specified.
+
+ - init-val       : 0 if driver should disable bridge at startup
+                    1 if driver should enable bridge at startup
+                    driver leaves bridge in current state if property not
+                    specified.
+
+Example:
+	hps_fpgabridge0: fpgabridge@0 {
+		compatible = "altr,socfpga-hps2fpga-bridge";
+		label = "hps2fpga";
+		altr,l3-syscon = <&l3regs>;
+		clocks = <&l4_main_clk>;
+		init-val = <1>;
+	};
+
+	hps_fpgabridge1: fpgabridge@1 {
+		compatible = "altr,socfpga-lwhps2fpga-bridge";
+		label = "lwhps2fpga";
+		altr,l3-syscon = <&l3regs>;
+		clocks = <&l4_main_clk>;
+		init-val = <0>;
+	};
+
+	hps_fpgabridge2: fpgabridge@2 {
+		compatible = "altr,socfpga-fpga2hps-bridge";
+		label = "fpga2hps";
+		altr,l3-syscon = <&l3regs>;
+		clocks = <&l4_main_clk>;
+	};