| Message ID | 20181121150709.6030-7-TheSven73@googlemail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add Fieldbus subsystem + support HMS Profinet card | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | "total: 0 errors, 1 warnings, 66 lines checked" |
On Wed, Nov 21, 2018 at 10:07:08AM -0500, thesven73@gmail.com wrote: > This patch adds devicetree binding documentation for the > Arcx Anybus-S host. > > Signed-off-by: Sven Van Asbroeck <svendev@arcx.com> > --- > .../bindings/fieldbus/arcx,anybuss-host.txt | 66 +++++++++++++++++++ > 1 file changed, 66 insertions(+) > create mode 100644 Documentation/devicetree/bindings/fieldbus/arcx,anybuss-host.txt > > diff --git a/Documentation/devicetree/bindings/fieldbus/arcx,anybuss-host.txt b/Documentation/devicetree/bindings/fieldbus/arcx,anybuss-host.txt > new file mode 100644 > index 000000000000..456587f288c1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/fieldbus/arcx,anybuss-host.txt > @@ -0,0 +1,66 @@ > +* Arcx Anybus-S host > + > +This host communicates with the SoC over a parallel bus. It is > +expected that its Device Tree node is specified as the child of a node > +corresponding to the parallel bus used for communication. > + > +Required properties: > +-------------------- > + > + - compatible : The following string: > + "arcx,anybuss-host" > + > + - reg : bus memory area where the Anybus-S host dpram is located. > + > + - interrupts : interrupt connected to the Anybus-S host interrupt line. > + Generic interrupt client node bindings are described in > + interrupt-controller/interrupts.txt > + > + - resets : the reset line associated with the Anybus-S host. > + > +Optional: use of subnodes > +------------------------- > + > +The card connected to a host may need additional properties. These can be > +specified in subnodes to the host node. > + > +The subnodes are identified by the standard 'reg' property. Which information > +exactly can be specified depends on the bindings for the function driver > +for the subnode. > + > +Required host node properties when using subnodes: > +- #address-cells: should be one. > +- #size-cells: should be zero. > + > +Required subnode properties: > +- reg: Must contain the fieldbus type of the card this subnode describes. > + > +Example of usage: > +----------------- > + > +This example places the Anybus-S host on top of the i.MX WEIM parallel bus, see: > +Documentation/devicetree/bindings/bus/imx-weim.txt > + > +&weim { > + anybus-host@0 { > + compatible = "arcx,anybuss-host"; > + reg = <0 0x400000 0x800>; > + interrupt-parent = <&gpio1>; > + interrupts = <1 IRQ_TYPE_LEVEL_LOW>; > + resets = <&anybus_bridge 0>; I'm still not clear what a bridge vs. host is. Both examples are on WEIM bus CS0? > + /* fsl,weim-cs-timing is a i.MX WEIM bus specific property */ > + fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100 > + 0x00000000 0xa0000240 0x00000000>; > + /* > + * optional subnode for a profinet card > + * (fieldbus type = 0x0089) > + */ > + #address-cells = <1>; > + #size-cells = <0>; > + profinet@0 { Unit-addresses are based on 'reg'. So this should either be '89' or based on a definition for the bus (e.g. PCI uses dev and func). > + reg = <0x0089>; Is fieldbus type how one addresses a device on the bus? > + /* profinet specific properties go here */ > + }; > + }; > + > +}; > -- > 2.17.1 >
Rob, thank you so much for the review ! On Mon, Nov 26, 2018 at 5:08 PM Rob Herring <robh@kernel.org> wrote: > Unit-addresses are based on 'reg'. So this should either be '89' or > based on a definition for the bus (e.g. PCI uses dev and func). > >> + reg = <0x0089>; > > Is fieldbus type how one addresses a device on the bus? I'm afraid not. Anybus cards don't have an address, because only a single card can be connected to an anybus at a time. Fieldbus type defines the type/functionality of the connected card. Like pci device ids. The current patchset allows devicetree nodes to be provided depending on the type of card connected to the bus. It identifies the card by type, not location. Is this not desired / allowed ? > I'm still not clear what a bridge vs. host is. Both examples are on WEIM > bus CS0? I agree that the terminology is too confusing. I'm looking to change it. Let's compare current anybus with a well-established bus architecture like pci: pci anybus -------------------------------------------------------------------------------- Common bus code (spec), pci/pci_driver.c fieldbus/anybuss-host.c calls bus_register() must be parallel bus subnode no of compatible has of compatible -------------------------------------------------------------------------------- Device on bus, calls XXX_client_driver_register() hwmon/k8temp.c fieldbus/hms-profinet.c -------------------------------------------------------------------------------- Controller, silicon driver pci/pcie-tango.c fieldbus/anybus-bridge.c platform_driver calls platform_driver does not pci_host_probe() register with anybus I should probably rework this so it conforms more closely to the way it's done on pci. Would the following look any better? pci anybus -------------------------------------------------------------------------------- Common bus code (spec), pci/pci_driver.c anybus/anybus_driver.c calls bus_register() no of compatible no of compatible -------------------------------------------------------------------------------- Device on bus, calls XXX_client_driver_register() hwmon/k8temp.c fieldbus/hms-profinet.c -------------------------------------------------------------------------------- Controller, silicon driver pci/pcie-tango.c anybus/arcx-controller.c platform_driver calls platform_driver calls pci_host_probe() anybus_host_probe() Of course, the analogy is not perfect, as there can only be a single card (device) connected to an anybus at a time.
On Wed, Nov 28, 2018 at 9:38 AM Sven Van Asbroeck <thesven73@gmail.com> wrote: > > Rob, thank you so much for the review ! > > On Mon, Nov 26, 2018 at 5:08 PM Rob Herring <robh@kernel.org> wrote: > > > Unit-addresses are based on 'reg'. So this should either be '89' or > > based on a definition for the bus (e.g. PCI uses dev and func). > > > >> + reg = <0x0089>; > > > > Is fieldbus type how one addresses a device on the bus? > > I'm afraid not. Anybus cards don't have an address, because only a single card > can be connected to an anybus at a time. Then you don't need reg as you should only have 1 child node. > Fieldbus type defines the type/functionality of the connected card. Like pci > device ids. Which for PCI can be overridden in DT, but is part of reg. > The current patchset allows devicetree nodes to be provided depending on > the type of card connected to the bus. It identifies the card by type, > not location. > > Is this not desired / allowed ? No, 'reg' is how you address a device. You could use 'type' in the compatible string if type implies some sort of standard class interface. > > I'm still not clear what a bridge vs. host is. Both examples are on WEIM > > bus CS0? > > I agree that the terminology is too confusing. I'm looking to change it. > > Let's compare current anybus with a well-established bus architecture like pci: > > pci anybus > -------------------------------------------------------------------------------- > Common bus code (spec), pci/pci_driver.c fieldbus/anybuss-host.c > calls bus_register() must be parallel bus > subnode > no of compatible has of compatible > -------------------------------------------------------------------------------- > Device on bus, calls > XXX_client_driver_register() hwmon/k8temp.c fieldbus/hms-profinet.c > -------------------------------------------------------------------------------- > Controller, silicon driver pci/pcie-tango.c fieldbus/anybus-bridge.c > platform_driver calls platform_driver does not > pci_host_probe() register with anybus > > I should probably rework this so it conforms more closely to the way it's done > on pci. Would the following look any better? Yes, this looks better. > pci anybus > -------------------------------------------------------------------------------- > Common bus code (spec), pci/pci_driver.c anybus/anybus_driver.c Maybe just anybus/core.c or anybus/bus.c. I'm guessing your implementation will be *much* more simple than PCI. > calls bus_register() no of compatible no of compatible > -------------------------------------------------------------------------------- > Device on bus, calls > XXX_client_driver_register() hwmon/k8temp.c fieldbus/hms-profinet.c > -------------------------------------------------------------------------------- > Controller, silicon driver pci/pcie-tango.c anybus/arcx-controller.c > platform_driver calls platform_driver calls > pci_host_probe() anybus_host_probe() > > Of course, the analogy is not perfect, as there can only be a single card > (device) connected to an anybus at a time. Right, that should simplify the bus code a lot. Rob
Rob,
Eliminating the 'compatible' property for the anybus gives me an interesting
devicetree problem.
The imx-weim code naively loops through all subnodes, applying timing settings
to the CS in the first reg entry.
In the example below, WEIM CS0 is programmed with the settings in
fsl,weim-cs-timing, because the CS part of reg is 0:
weim: weim@21b8000 {
compatible = "fsl,imx6q-weim";
reg = <0x021b8000 0x4000>;
clocks = <&clks 196>;
#address-cells = <2>;
#size-cells = <1>;
ranges = <0 0 0x08000000 0x08000000>;
fsl,weim-cs-gpr = <&gpr>;
nor@0,0 {
compatible = "cfi-flash";
reg = <0 0 0x02000000>;
#address-cells = <1>;
#size-cells = <1>;
bank-width = <2>;
fsl,weim-cs-timing = <0x00620081 0x00000001 0x1c022000
0x0000c000 0x1404a38e 0x00000000>;
};
};
The problem is that my 'anybus controller' hardware spans chip selects.
It requires access to various memory areas. In the example below, CS1
will not be programmed with timing settings:
weim {
controller@0 {
compatible = "arcx,anybus-controller";
reg = <0 0 0x100>, <0 0x400000 0x800>, <1 0x400000 0x800>;
fsl,weim-cs-timing = ...;
};
};
I've coped with this in the past by creating a 'dummy' subnode, without a
compatible property. But it's ugly:
weim {
controller@0 {
compatible = "arcx,anybus-controller";
reg = <0 0 0x100>, <0 0x400000 0x800>, <1 0x400000 0x800>;
fsl,weim-cs-timing = ...;
};
dummy@1 { /* this works ! */
reg = <1 0 0>;
fsl,weim-cs-timing = ...;
};
};
Is there a better way? I've looked and looked, but can't find anything that's
similar.
Or should I extend the imx-weim driver to cope?
On Thu, Nov 29, 2018 at 2:59 PM Sven Van Asbroeck <thesven73@gmail.com> wrote: > > Rob, > > Eliminating the 'compatible' property for the anybus gives me an interesting > devicetree problem. > > The imx-weim code naively loops through all subnodes, applying timing settings > to the CS in the first reg entry. > In the example below, WEIM CS0 is programmed with the settings in > fsl,weim-cs-timing, because the CS part of reg is 0: > > > weim: weim@21b8000 { > compatible = "fsl,imx6q-weim"; > reg = <0x021b8000 0x4000>; > clocks = <&clks 196>; > #address-cells = <2>; > #size-cells = <1>; > ranges = <0 0 0x08000000 0x08000000>; > fsl,weim-cs-gpr = <&gpr>; > > nor@0,0 { > compatible = "cfi-flash"; > reg = <0 0 0x02000000>; > #address-cells = <1>; > #size-cells = <1>; > bank-width = <2>; > fsl,weim-cs-timing = <0x00620081 0x00000001 0x1c022000 > 0x0000c000 0x1404a38e 0x00000000>; > }; > }; > > The problem is that my 'anybus controller' hardware spans chip selects. > It requires access to various memory areas. In the example below, CS1 > will not be programmed with timing settings: > > weim { > controller@0 { > compatible = "arcx,anybus-controller"; > reg = <0 0 0x100>, <0 0x400000 0x800>, <1 0x400000 0x800>; > fsl,weim-cs-timing = ...; > }; > }; > > I've coped with this in the past by creating a 'dummy' subnode, without a > compatible property. But it's ugly: > > weim { > controller@0 { > compatible = "arcx,anybus-controller"; > reg = <0 0 0x100>, <0 0x400000 0x800>, <1 0x400000 0x800>; > fsl,weim-cs-timing = ...; > }; > dummy@1 { /* this works ! */ > reg = <1 0 0>; > fsl,weim-cs-timing = ...; > }; > }; > > Is there a better way? I've looked and looked, but can't find anything that's > similar. > > Or should I extend the imx-weim driver to cope? Yes, I think it should iterate on child reg properties not just nodes. Hopefully, you don't need different timing for each CS though. Rob
diff --git a/Documentation/devicetree/bindings/fieldbus/arcx,anybuss-host.txt b/Documentation/devicetree/bindings/fieldbus/arcx,anybuss-host.txt new file mode 100644 index 000000000000..456587f288c1 --- /dev/null +++ b/Documentation/devicetree/bindings/fieldbus/arcx,anybuss-host.txt @@ -0,0 +1,66 @@ +* Arcx Anybus-S host + +This host communicates with the SoC over a parallel bus. It is +expected that its Device Tree node is specified as the child of a node +corresponding to the parallel bus used for communication. + +Required properties: +-------------------- + + - compatible : The following string: + "arcx,anybuss-host" + + - reg : bus memory area where the Anybus-S host dpram is located. + + - interrupts : interrupt connected to the Anybus-S host interrupt line. + Generic interrupt client node bindings are described in + interrupt-controller/interrupts.txt + + - resets : the reset line associated with the Anybus-S host. + +Optional: use of subnodes +------------------------- + +The card connected to a host may need additional properties. These can be +specified in subnodes to the host node. + +The subnodes are identified by the standard 'reg' property. Which information +exactly can be specified depends on the bindings for the function driver +for the subnode. + +Required host node properties when using subnodes: +- #address-cells: should be one. +- #size-cells: should be zero. + +Required subnode properties: +- reg: Must contain the fieldbus type of the card this subnode describes. + +Example of usage: +----------------- + +This example places the Anybus-S host on top of the i.MX WEIM parallel bus, see: +Documentation/devicetree/bindings/bus/imx-weim.txt + +&weim { + anybus-host@0 { + compatible = "arcx,anybuss-host"; + reg = <0 0x400000 0x800>; + interrupt-parent = <&gpio1>; + interrupts = <1 IRQ_TYPE_LEVEL_LOW>; + resets = <&anybus_bridge 0>; + /* fsl,weim-cs-timing is a i.MX WEIM bus specific property */ + fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100 + 0x00000000 0xa0000240 0x00000000>; + /* + * optional subnode for a profinet card + * (fieldbus type = 0x0089) + */ + #address-cells = <1>; + #size-cells = <0>; + profinet@0 { + reg = <0x0089>; + /* profinet specific properties go here */ + }; + }; + +};
This patch adds devicetree binding documentation for the Arcx Anybus-S host. Signed-off-by: Sven Van Asbroeck <svendev@arcx.com> --- .../bindings/fieldbus/arcx,anybuss-host.txt | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 Documentation/devicetree/bindings/fieldbus/arcx,anybuss-host.txt