| Message ID | 20230110042533.12894-3-clayc@hpe.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ARM: Add GXP SROM Support | expand |
On 10/01/2023 05:25, clayc@hpe.com wrote: > From: Clay Chang <clayc@hpe.com> > > Document binding to support SROM driver in GXP. > > Signed-off-by: Clay Chang <clayc@hpe.com> > --- > .../bindings/soc/hpe/hpe,gxp-srom.yaml | 36 +++++++++++++++++++ > 1 file changed, 36 insertions(+) > create mode 100644 Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml > > diff --git a/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml > new file mode 100644 > index 000000000000..14ad97d595c8 > --- /dev/null > +++ b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml Don't drop stuff to soc. Put it in respective directories. This also applies to your driver. SROM controllers go to memory-controllers. What is this, I have no clue. "SROM Control Register" is not helping me. > @@ -0,0 +1,36 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/soc/hpe/hpe,gxp-srom.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: HPE GXP SoC SROM Control Register > + > +maintainers: > + - Clay Chang <clayc@hpe.com> > + > +description: |+ > + The SROM control register can be used to configure LPC related legacy > + I/O registers. And why this is a hardware? No, you now add fake devices to be able to write some stuff from user-space... Otherwise this needs proper hardware description. > + > +properties: > + compatible: > + items: Drop items, you have only one item. > + - const: hpe,gxp-srom > + > + reg: > + items: > + - description: SROM LPC Configuration Registers Drop items and description. Just maxItems: 1 > + > +required: > + - compatible > + - reg > + > +unevaluatedProperties: false > + > +examples: > + - | > + srom: srom@80fc0000 { > + compatible = "hpe,gxp-srom"; > + reg = <0x80fc0000 0x100>; > + }; Best regards, Krzysztof
On Tue, Jan 10, 2023 at 10:49:36AM +0100, Krzysztof Kozlowski wrote: > On 10/01/2023 05:25, clayc@hpe.com wrote: > > From: Clay Chang <clayc@hpe.com> > > > > Document binding to support SROM driver in GXP. > > > > Signed-off-by: Clay Chang <clayc@hpe.com> > > --- > > .../bindings/soc/hpe/hpe,gxp-srom.yaml | 36 +++++++++++++++++++ > > 1 file changed, 36 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml > > > > diff --git a/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml > > new file mode 100644 > > index 000000000000..14ad97d595c8 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml > > Don't drop stuff to soc. Put it in respective directories. This also > applies to your driver. > > SROM controllers go to memory-controllers. What is this, I have no clue. > "SROM Control Register" is not helping me. > > > @@ -0,0 +1,36 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/soc/hpe/hpe,gxp-srom.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: HPE GXP SoC SROM Control Register > > + > > +maintainers: > > + - Clay Chang <clayc@hpe.com> > > + > > +description: |+ > > + The SROM control register can be used to configure LPC related legacy > > + I/O registers. > > And why this is a hardware? No, you now add fake devices to be able to > write some stuff from user-space... Otherwise this needs proper hardware > description. Thank you for commenting on this. You are right, this is not a real hardware device, but simply exposes MMIO regions to the user-space. Maybe we should rewrite this as a syscon driver. Is writing a syscon driver a right direction? > > > + > > +properties: > > + compatible: > > + items: > > Drop items, you have only one item. > > > + - const: hpe,gxp-srom > > + > > + reg: > > + items: > > + - description: SROM LPC Configuration Registers > > Drop items and description. Just maxItems: 1 > > > + > > +required: > > + - compatible > > + - reg > > + > > +unevaluatedProperties: false > > + > > +examples: > > + - | > > + srom: srom@80fc0000 { > > + compatible = "hpe,gxp-srom"; > > + reg = <0x80fc0000 0x100>; > > + }; > > Best regards, > Krzysztof > Thanks, Clay
On Thu, Jan 12, 2023, at 14:16, Clay Chang wrote: > On Tue, Jan 10, 2023 at 10:49:36AM +0100, Krzysztof Kozlowski wrote: >> On 10/01/2023 05:25, clayc@hpe.com wrote: >> > @@ -0,0 +1,36 @@ >> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> > +%YAML 1.2 >> > +--- >> > +$id: http://devicetree.org/schemas/soc/hpe/hpe,gxp-srom.yaml# >> > +$schema: http://devicetree.org/meta-schemas/core.yaml# >> > + >> > +title: HPE GXP SoC SROM Control Register >> > + >> > +maintainers: >> > + - Clay Chang <clayc@hpe.com> >> > + >> > +description: |+ >> > + The SROM control register can be used to configure LPC related legacy >> > + I/O registers. >> >> And why this is a hardware? No, you now add fake devices to be able to >> write some stuff from user-space... Otherwise this needs proper hardware >> description. > > Thank you for commenting on this. You are right, this is not a real > hardware device, but simply exposes MMIO regions to the user-space. > Maybe we should rewrite this as a syscon driver. Is writing a syscon > driver a right direction? There are two completely separate questions about the DT binding and about the user-visible interface. The binding needs to properly identify what this device is. I don't think anyone without the datasheet can tell you the right answer here, it really depends what the other registers do. If there are lots of unrelated registers in a small area, a syscon might be the right answer, but if they are all related to an external memory bus, then categorizing it as a memory controller may be more appropriate. For the user interface side, I don't really like the idea of having a hardware register directly exposed as driver in drivers/soc, this generally makes it impossible to have portable userspace that works across implementations of multiple SoC vendors, and it makes it too easy to come up with an ad-hoc interface to make a chip work for a particular use case when a more general solution would be better. Again, it's hard for me to tell why this even needs to be runtime configurable, please try to describe what type of application would access the sysfs interface here, and why this can't just be set to a fixed value by bootloader or kernel without user interaction. Arnd
Hi Arnd, Thank you for taking time to answer my questions. On Thu, Jan 12, 2023 at 02:37:53PM +0100, Arnd Bergmann wrote: > On Thu, Jan 12, 2023, at 14:16, Clay Chang wrote: > > On Tue, Jan 10, 2023 at 10:49:36AM +0100, Krzysztof Kozlowski wrote: > >> On 10/01/2023 05:25, clayc@hpe.com wrote: > > >> > @@ -0,0 +1,36 @@ > >> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >> > +%YAML 1.2 > >> > +--- > >> > +$id: http://devicetree.org/schemas/soc/hpe/hpe,gxp-srom.yaml# > >> > +$schema: http://devicetree.org/meta-schemas/core.yaml# > >> > + > >> > +title: HPE GXP SoC SROM Control Register > >> > + > >> > +maintainers: > >> > + - Clay Chang <clayc@hpe.com> > >> > + > >> > +description: |+ > >> > + The SROM control register can be used to configure LPC related legacy > >> > + I/O registers. > >> > >> And why this is a hardware? No, you now add fake devices to be able to > >> write some stuff from user-space... Otherwise this needs proper hardware > >> description. > > > > Thank you for commenting on this. You are right, this is not a real > > hardware device, but simply exposes MMIO regions to the user-space. > > Maybe we should rewrite this as a syscon driver. Is writing a syscon > > driver a right direction? > > There are two completely separate questions about the DT binding > and about the user-visible interface. > > The binding needs to properly identify what this device is. I don't > think anyone without the datasheet can tell you the right answer > here, it really depends what the other registers do. If there are > lots of unrelated registers in a small area, a syscon might be > the right answer, but if they are all related to an external > memory bus, then categorizing it as a memory controller may > be more appropriate. Our use-cases are more like some register accesses not related to an external memory bus, so syscon might be a better fit. > > For the user interface side, I don't really like the idea of > having a hardware register directly exposed as driver in > drivers/soc, this generally makes it impossible to have portable > userspace that works across implementations of multiple SoC > vendors, and it makes it too easy to come up with an ad-hoc > interface to make a chip work for a particular use case when > a more general solution would be better. > I agree with you. I have one question though: if we create a 'hpe' directory under drivers/soc, and put all HPE BMC specific drivers there, do you think this proper? > Again, it's hard for me to tell why this even needs to be runtime > configurable, please try to describe what type of application > would access the sysfs interface here, and why this can't just > be set to a fixed value by bootloader or kernel without user > interaction. The register is used for communication and synchronization between the BMC and the host. During runtime, user-space daemons configures the value of the register for interactions. > > Arnd Thanks, Clay
On Mon, Jan 16, 2023, at 14:42, Clay Chang wrote: > On Thu, Jan 12, 2023 at 02:37:53PM +0100, Arnd Bergmann wrote: >> On Thu, Jan 12, 2023, at 14:16, Clay Chang wrote: >> For the user interface side, I don't really like the idea of >> having a hardware register directly exposed as driver in >> drivers/soc, this generally makes it impossible to have portable >> userspace that works across implementations of multiple SoC >> vendors, and it makes it too easy to come up with an ad-hoc >> interface to make a chip work for a particular use case when >> a more general solution would be better. >> > > I agree with you. I have one question though: if we create a 'hpe' > directory under drivers/soc, and put all HPE BMC specific drivers there, > do you think this proper? It certainly wouldn't be right to put "all HPE BMC specific drivers" in there. Most drivers will fit into some existing subsystem, and should be moved there instead. drivers/soc is used primarily for drivers using soc_device_register() to provide information about the soc, and we also use it as a place for drivers that just export soc-specific helper functions that can be used by other drivers. >> Again, it's hard for me to tell why this even needs to be runtime >> configurable, please try to describe what type of application >> would access the sysfs interface here, and why this can't just >> be set to a fixed value by bootloader or kernel without user >> interaction. > > The register is used for communication and synchronization between the > BMC and the host. During runtime, user-space daemons configures the > value of the register for interactions. That does not sound very specific. What is the subsystem on the host that this communicates with? Can you put the driver into the same subsystem? Arnd
On Mon, Jan 16, 2023 at 04:18:59PM +0100, Arnd Bergmann wrote: > On Mon, Jan 16, 2023, at 14:42, Clay Chang wrote: > > On Thu, Jan 12, 2023 at 02:37:53PM +0100, Arnd Bergmann wrote: > >> On Thu, Jan 12, 2023, at 14:16, Clay Chang wrote: > >> For the user interface side, I don't really like the idea of > >> having a hardware register directly exposed as driver in > >> drivers/soc, this generally makes it impossible to have portable > >> userspace that works across implementations of multiple SoC > >> vendors, and it makes it too easy to come up with an ad-hoc > >> interface to make a chip work for a particular use case when > >> a more general solution would be better. > >> > > > > I agree with you. I have one question though: if we create a 'hpe' > > directory under drivers/soc, and put all HPE BMC specific drivers there, > > do you think this proper? > > It certainly wouldn't be right to put "all HPE BMC specific drivers" > in there. Most drivers will fit into some existing subsystem, and > should be moved there instead. drivers/soc is used primarily for > drivers using soc_device_register() to provide information about the > soc, and we also use it as a place for drivers that just export > soc-specific helper functions that can be used by other drivers. > Sorry for not saying it clearly. I meant to put those HPE BMC related drivers that are "not specific" to a particular subsystem in drivers/soc/hpe. For those fit into some existing subsystems go to their designated places. > >> Again, it's hard for me to tell why this even needs to be runtime > >> configurable, please try to describe what type of application > >> would access the sysfs interface here, and why this can't just > >> be set to a fixed value by bootloader or kernel without user > >> interaction. > > > > The register is used for communication and synchronization between the > > BMC and the host. During runtime, user-space daemons configures the > > value of the register for interactions. > > That does not sound very specific. What is the subsystem on the > host that this communicates with? Can you put the driver into the > same subsystem? > > Arnd This is a control register in the BMC chip that partially controls host boot behaviors. When writing to the register, privileged mode is required. That's why we rely on a kernel driver for writing to the control register. And, there is no corresponding subsystem in the host OS. For this case, is it acceptable to put this driver under drivers/soc/hpe? Thanks, Clay
On Thu, Jan 19, 2023, at 08:39, Clay Chang wrote: > On Mon, Jan 16, 2023 at 04:18:59PM +0100, Arnd Bergmann wrote: > > Sorry for not saying it clearly. I meant to put those HPE BMC related > drivers that are "not specific" to a particular subsystem in > drivers/soc/hpe. For those fit into some existing subsystems go to their > designated places. Ok, that makes sense. I'm just trying to reduce the number of drivers that fit into this category. >> >> Again, it's hard for me to tell why this even needs to be runtime >> >> configurable, please try to describe what type of application >> >> would access the sysfs interface here, and why this can't just >> >> be set to a fixed value by bootloader or kernel without user >> >> interaction. >> > >> > The register is used for communication and synchronization between the >> > BMC and the host. During runtime, user-space daemons configures the >> > value of the register for interactions. >> >> That does not sound very specific. What is the subsystem on the >> host that this communicates with? Can you put the driver into the >> same subsystem? > > This is a control register in the BMC chip that partially controls host > boot behaviors. When writing to the register, privileged mode is > required. That's why we rely on a kernel driver for writing to the > control register. And, there is no corresponding subsystem in the host > OS. For this case, is it acceptable to put this driver under > drivers/soc/hpe? I see. So this sounds like it might be generic BMC feature, which would be nice to handle consistently for all BMC families. We had some discussions about other BMC features in the past, but don't remember what the overall consensus was, so I'm adding the openbmc list and as well as aspeed and npcm maintainers to Cc. The part I'm still missing is what type of userspace makes this decision on the BMC side, and whether that might already have a generalized interface. If there is, maybe part of that interface can be abstracted in the kernel. Arnd
diff --git a/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml new file mode 100644 index 000000000000..14ad97d595c8 --- /dev/null +++ b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml @@ -0,0 +1,36 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/soc/hpe/hpe,gxp-srom.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: HPE GXP SoC SROM Control Register + +maintainers: + - Clay Chang <clayc@hpe.com> + +description: |+ + The SROM control register can be used to configure LPC related legacy + I/O registers. + +properties: + compatible: + items: + - const: hpe,gxp-srom + + reg: + items: + - description: SROM LPC Configuration Registers + +required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + srom: srom@80fc0000 { + compatible = "hpe,gxp-srom"; + reg = <0x80fc0000 0x100>; + };