Re: [PATCH 2/5] dt-bindings: soc: hpe: hpe,gxp-srom.yaml

[Clay Chang]

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@xxxxxxx 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@xxxxxxx>
> >> > +
> >> > +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