| Message ID | 20221011185525.94210-3-nick.hawkins@hpe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add PLREG and SPI Driver GXP Support | expand |
On 11/10/2022 14:55, nick.hawkins@hpe.com wrote: > From: Nick Hawkins <nick.hawkins@hpe.com> > > The hpe,gxp-plreg binding provides access to the board i/o through the > Programmable logic interface. The binding provides information to enable > use of the same driver across the HPE portfolio. > > Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com> > --- > .../bindings/soc/hpe/hpe,gxp-plreg.yaml | 43 +++++++++++++++++++ > 1 file changed, 43 insertions(+) > create mode 100644 Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml > > diff --git a/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml > new file mode 100644 > index 000000000000..cdc54e66d9a9 > --- /dev/null > +++ b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml > @@ -0,0 +1,43 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: "http://devicetree.org/schemas/soc/hpe/hpe,gxp-plreg.yaml#" > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" Drop quotes from both lines. > + > +title: HPE GXP Programmable Logic Registers Controller > + > +maintainers: > + - Nick Hawkins <nick.hawkins@hpe.com> > + > +properties: > + compatible: > + items: > + - const: hpe,gxp-plreg > + - const: simple-mfd > + - const: syscon Blank line > + reg: > + maxItems: 1 > + > +required: > + - compatible > + > +additionalProperties: true This must be false... and then you will see the errors to fix. > + > +examples: > + - | > + cpld@1e789000 { > + compatible = "hpe,gxp-plreg", "simple-mfd", "syscon"; > + reg = <0x1e789000 0x1000>; > + fan1 { fan (or fan-1) > + bit = <0x01>; > + inst = <0x27>; > + id = <0x2B>; > + }; > + iopled1 { led or led-1? > + on = <0x04 0x01>; > + }; > + pwrbtn { power-button? > + latch = <0xFF 0xFF 0x04>; Keep lower case hex everywhere. > + }; > + }; > + Best regards, Krzysztof
On Tue, Oct 11, 2022 at 1:56 PM <nick.hawkins@hpe.com> wrote: > > From: Nick Hawkins <nick.hawkins@hpe.com> > > The hpe,gxp-plreg binding provides access to the board i/o through the > Programmable logic interface. The binding provides information to enable > use of the same driver across the HPE portfolio. > > Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com> > --- > .../bindings/soc/hpe/hpe,gxp-plreg.yaml | 43 +++++++++++++++++++ > 1 file changed, 43 insertions(+) > create mode 100644 Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml > > diff --git a/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml > new file mode 100644 > index 000000000000..cdc54e66d9a9 > --- /dev/null > +++ b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml > @@ -0,0 +1,43 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: "http://devicetree.org/schemas/soc/hpe/hpe,gxp-plreg.yaml#" > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > + > +title: HPE GXP Programmable Logic Registers Controller > + > +maintainers: > + - Nick Hawkins <nick.hawkins@hpe.com> > + > +properties: > + compatible: > + items: > + - const: hpe,gxp-plreg > + - const: simple-mfd > + - const: syscon > + reg: > + maxItems: 1 > + > +required: > + - compatible > + > +additionalProperties: true > + > +examples: > + - | > + cpld@1e789000 { > + compatible = "hpe,gxp-plreg", "simple-mfd", "syscon"; > + reg = <0x1e789000 0x1000>; > + fan1 { > + bit = <0x01>; > + inst = <0x27>; > + id = <0x2B>; These property names are way too generic for device specific properties. There is zero description of what the h/w does and any of these child nodes to give any advice on direction. However, a node per register or register field is generally the wrong direction. Rob
> > +examples: > > + - | > > + cpld@1e789000 { > > + compatible = "hpe,gxp-plreg", "simple-mfd", "syscon"; > > + reg = <0x1e789000 0x1000>; > > + fan1 { > > + bit = <0x01>; > > + inst = <0x27>; > > + id = <0x2B>; > These property names are way too generic for device specific properties. There is zero description of what the h/w does and any of these child nodes to give any advice on direction. However, a node per register or register field is generally the wrong direction. Thank you for your valued feedback. The goal I was trying to achieve here was making our programmable logic register driver interface in a generic way across multiple platforms based on inputs we provide with the .dts file for each platform. For instance if we want to read the fan 1 install status on platform X it would be reading bit 0x01 of byte 0x27 where as on platform Y it could be bit 0x02 of byte 0x16. Is there a format you would recommend that I can adhere too? Thanks! -Nick Hawkins
On 12/10/2022 15:56, Hawkins, Nick wrote: >>> +examples: >>> + - | >>> + cpld@1e789000 { >>> + compatible = "hpe,gxp-plreg", "simple-mfd", "syscon"; >>> + reg = <0x1e789000 0x1000>; >>> + fan1 { >>> + bit = <0x01>; >>> + inst = <0x27>; >>> + id = <0x2B>; > >> These property names are way too generic for device specific properties. There is zero description of what the h/w does and any of these child nodes to give any advice on direction. However, a node per register or register field is generally the wrong direction. > > Thank you for your valued feedback. The goal I was trying to achieve here was making our programmable logic register driver interface in a generic way across multiple platforms based on inputs we provide with the .dts file for each platform. For instance if we want to read the fan 1 install status on platform X it would be reading bit 0x01 of byte 0x27 where as on platform Y it could be bit 0x02 of byte 0x16. Is there a format you would recommend that I can adhere too? I don't think DT place is to describe register or memory layout, with some exceptions like MTD partitions or nvmem cells. Basically you are representing here a device register map inside DT, just because it is a CPLD. Every regular multi-functional device handles its register map in the driver itself and uses Linux framework to expose the internals. CPLD should not be different, except that is programmable. Best regards, Krzysztof
> I don't think DT place is to describe register or memory layout, with some exceptions like MTD partitions or nvmem cells. Basically you are representing here a device register map inside DT, just because it is a CPLD. > Every regular multi-functional device handles its register map in the driver itself and uses Linux framework to expose the internals. CPLD should not be different, except that is programmable. Hi Krzysztof, Thank you for your time and feedback. We are looking for a place to describe differences within our CPLD implementation due to our memory mapping not being consistent. The idea we are pursuing is to use the device tree to serve as an input to Linux to prevent driver code fragmentation from multiple platforms needing their own specific offsets. If this is not acceptable to do through the device tree, should we rely on having an include file for each platform instead? Thanks, -Nick Hawkins
On 24/10/2022 20:03, Hawkins, Nick wrote: >> I don't think DT place is to describe register or memory layout, with some exceptions like MTD partitions or nvmem cells. Basically you are representing here a device register map inside DT, just because it is a CPLD. > >> Every regular multi-functional device handles its register map in the driver itself and uses Linux framework to expose the internals. CPLD should not be different, except that is programmable. > Hi Krzysztof, > > Thank you for your time and feedback. We are looking for a place to describe differences within our CPLD implementation due to our memory mapping not being consistent. The idea we are pursuing is to use the device tree to serve as an input to Linux to prevent driver code fragmentation from multiple platforms needing their own specific offsets. I understand what you want to achieve, but Devicetree does not seem a tool for that. DT describes the hardware characteristics, but not is exact memory map. Although your goal differs than for example goal of some developer of I2C or MMIO device drivers, but essentially devices are similar. We do not describe memory map of MMIO or register map of I2C devices in DT. > If this is not acceptable to do through the device tree, should we rely on having an include file for each platform instead? I would say use rather standard Linux subsystems and problem is gone. You have fan? Sure, we have subsystem for fans. You have power supply or battery - we have stuff for this as well. Best regards, Krzysztof
> I understand what you want to achieve, but Devicetree does not seem a tool for that. DT describes the hardware characteristics, but not is exact memory map. > Although your goal differs than for example goal of some developer of I2C or MMIO device drivers, but essentially devices are similar. We do not describe memory map of MMIO or register map of I2C devices in DT. > > If this is not acceptable to do through the device tree, should we rely on having an include file for each platform instead? > I would say use rather standard Linux subsystems and problem is gone. > You have fan? Sure, we have subsystem for fans. You have power supply or battery - we have stuff for this as well. Greetings Krzysztof, Thanks for your feedback. In this case for something like fans as you suggest above would it be acceptable for the fans to call into plreg(the proposed driver) just to read the fan related registers with the fan driver knowing what offset to use? Multiple drivers will need to access registers in this memory range so I am trying to determine if they all need to map this area or all goto one source to read/write to it. Thanks, -Nick Hawkins
On 25/10/2022 14:44, Hawkins, Nick wrote: >> I understand what you want to achieve, but Devicetree does not seem a tool for that. DT describes the hardware characteristics, but not is exact memory map. > >> Although your goal differs than for example goal of some developer of I2C or MMIO device drivers, but essentially devices are similar. We do not describe memory map of MMIO or register map of I2C devices in DT. > > >>> If this is not acceptable to do through the device tree, should we rely on having an include file for each platform instead? > >> I would say use rather standard Linux subsystems and problem is gone. >> You have fan? Sure, we have subsystem for fans. You have power supply or battery - we have stuff for this as well. > > Greetings Krzysztof, > > Thanks for your feedback. In this case for something like fans as you suggest above would it be acceptable for the fans to call into plreg(the proposed driver) just to read the fan related registers with the fan driver knowing what offset to use? Multiple drivers will need to access registers in this memory range so I am trying to determine if they all need to map this area or all goto one source to read/write to it. I don't know exactly what type of devices you represent in that plreg, but in general the fan device would be the respective plreg. The same with other pieces like hwmon, power supply. Best regards, Krzysztof
> I don't know exactly what type of devices you represent in that plreg, but in general the fan device would be the respective plreg. The same with other pieces like hwmon, power supply.
We were primarily representing the registers that translate to the CPLD input/outputs from our platforms as well as handling the interrupts associated with those inputs/outputs. When you say "would be the respective plreg" do you mean that each device/controller would need to perform the actions plreg does individually? In that case how should we get information for that register/memory region and interrupts from the dts? Could it be something like this:
plreg: plreg@d1000000 {
compatible = "hpe,gxp-plreg";
reg = <0xd1000000 0xFF>;
interrupts = <26>;
interrupt-parent = <&vic0>;
};
fanctrl: fanctrl@c1000c00 {
compatible = "hpe,gxp-fan-ctrl";
reg = <0xc1000c00 0x200>;
plreg_handle = <&plreg>;
};
Thanks for the assistance,
-Nick Hawkins
On 25/10/2022 15:26, Hawkins, Nick wrote: > >> I don't know exactly what type of devices you represent in that plreg, but in general the fan device would be the respective plreg. The same with other pieces like hwmon, power supply. > We were primarily representing the registers that translate to the CPLD input/outputs from our platforms as well as handling the interrupts associated with those inputs/outputs. So basically each register (or set of registers) is a device? How is it different than any other multi-functional device? Why do you want to model it differently? > When you say "would be the respective plreg" do you mean that each device/controller would need to perform the actions plreg does individually? In that case how should we get information for that register/memory region and interrupts from the dts? Could it be something like this: > > plreg: plreg@d1000000 { > compatible = "hpe,gxp-plreg"; > reg = <0xd1000000 0xFF>; > interrupts = <26>; > interrupt-parent = <&vic0>; > }; > > fanctrl: fanctrl@c1000c00 { > compatible = "hpe,gxp-fan-ctrl"; > reg = <0xc1000c00 0x200>; > plreg_handle = <&plreg>; > }; > No, rather these are one node. You insist to represent this all as programmable logic, but why? CPLD, FPGA, ASIC, dedicated IC - all are just implementations and for us what's matter are the interfaces, inputs and outputs. Best regards, Krzysztof
On 25/10/2022 15:39, Verdun, Jean-Marie wrote: > Hi Krzysztof, > Use mailing list style of replying. I can bear the lack of wrapping (one huge sentence), but top-post is a no. > I think what we try to do is to introduce an abstraction layer between the interfaces and the drivers, as our CPLD interfaces are platform dependents. I mean the Power On control could be at address 0x09 on one platform or 0x119 on another one. We would like to find a way to avoid to have to change the driver code, but just feeding the driver with relevant datas, which could be into a platform dependent include file or through the proposed solution that Nick is promoting. I think this was already said. Repeating it, with very similar words will not help us... Let me be then clear: I care little about your goal of abstracting some driver code. I care about proper Devicetree bindings and proper representation of hardware in Devicetree sources. Looks like you want to hack around Devicetree to match your needs. This does not work. I gave you the proposed solution based on my feeling and limited understanding of what you have there. If this does not work for you, then life - you need to find other solution. > > If the CPLD memory address space was consistent between platform and generation that would be great but unfortunately that is not the case that is why we try to break down the dependency into the driver code and retrieve the data from another place. I don't see how this argument is relevant to my questions. I did not propose anything which would require some "memory address space consistency". Best regards, Krzysztof
On 25/10/2022 15:39, Verdun, Jean-Marie wrote: > Hi Krzysztof, > > I think what we try to do is to introduce an abstraction layer between the interfaces and the drivers, as our CPLD interfaces are platform dependents. I mean the Power On control could be at address 0x09 on one platform or 0x119 on another one. We would like to find a way to avoid to have to change the driver code, but just feeding the driver with relevant datas, which could be into a platform dependent include file or through the proposed solution that Nick is promoting. > > If the CPLD memory address space was consistent between platform and generation that would be great but unfortunately that is not the case that is why we try to break down the dependency into the driver code and retrieve the data from another place. > > JM > ________________________________ > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Tuesday, October 25, 2022 12:33 PM > To: Hawkins, Nick <nick.hawkins@hpe.com> > Cc: Verdun, Jean-Marie <verdun@hpe.com>; krzysztof.kozlowski+dt@linaro.org <krzysztof.kozlowski+dt@linaro.org>; linux@armlinux.org.uk <linux@armlinux.org.uk>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linux-arm-kernel@lists.infradead.org <linux-arm-kernel@lists.infradead.org>; Rob Herring <robh+dt@kernel.org> > Subject: Re: [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg > > On 25/10/2022 15:26, Hawkins, Nick wrote: >> >>> I don't know exactly what type of devices you represent in that plreg, but in general the fan device would be the respective plreg. The same with other pieces like hwmon, power supply. >> We were primarily representing the registers that translate to the CPLD input/outputs from our platforms as well as handling the interrupts associated with those inputs/outputs. > > So basically each register (or set of registers) is a device? How is it > different than any other multi-functional device? Why do you want to > model it differently? How is it different, I am asking? > >> When you say "would be the respective plreg" do you mean that each device/controller would need to perform the actions plreg does individually? In that case how should we get information for that register/memory region and interrupts from the dts? Could it be something like this: >> >> plreg: plreg@d1000000 { >> compatible = "hpe,gxp-plreg"; >> reg = <0xd1000000 0xFF>; >> interrupts = <26>; >> interrupt-parent = <&vic0>; >> }; >> >> fanctrl: fanctrl@c1000c00 { >> compatible = "hpe,gxp-fan-ctrl"; >> reg = <0xc1000c00 0x200>; >> plreg_handle = <&plreg>; >> }; >> > > No, rather these are one node. > > You insist to represent this all as programmable logic, but why? CPLD, > FPGA, ASIC, dedicated IC - all are just implementations and for us > what's matter are the interfaces, inputs and outputs. And seriously... this is not a chat. Take a bit of time to answer these questions instead of replying immediately with a same response as yesterday. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml new file mode 100644 index 000000000000..cdc54e66d9a9 --- /dev/null +++ b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml @@ -0,0 +1,43 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/soc/hpe/hpe,gxp-plreg.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#" + +title: HPE GXP Programmable Logic Registers Controller + +maintainers: + - Nick Hawkins <nick.hawkins@hpe.com> + +properties: + compatible: + items: + - const: hpe,gxp-plreg + - const: simple-mfd + - const: syscon + reg: + maxItems: 1 + +required: + - compatible + +additionalProperties: true + +examples: + - | + cpld@1e789000 { + compatible = "hpe,gxp-plreg", "simple-mfd", "syscon"; + reg = <0x1e789000 0x1000>; + fan1 { + bit = <0x01>; + inst = <0x27>; + id = <0x2B>; + }; + iopled1 { + on = <0x04 0x01>; + }; + pwrbtn { + latch = <0xFF 0xFF 0x04>; + }; + }; +