Re: [PATCH v1 1/2] dt-binding: soc: nuvoton: Add NPCM BPC LPC documentation

From: Tomer Maimon
Date: Thu Nov 24 2022 - 10:38:55 EST


Hi Krzysztof,

Thanks a lot for your comments.

On Wed, 23 Nov 2022 at 12:03, Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 22/11/2022 21:12, Tomer Maimon wrote:
>
> 1. Subject: drop second, redundant "documentation" (dt-bindings are
> documentation).
O.K.
>
> 2. Use subject prefixes matching the subsystem (git log --oneline -- ...).
this is what I did dt-binding: soc: nuvoton... do you mean dt-binding: nuvoton.
>
> > Added device tree binding documentation for Nuvoton BMC NPCM BIOS Post
> > Code (BPC).
> >
> > The NPCM BPC monitoring two configurable I/O addresses written by the
> > host on Low Pin Count (LPC) bus.
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@xxxxxxxxx>
> > ---
> > .../bindings/soc/nuvoton/npcm-lpc-bpc.yaml | 112 ++++++++++++++++++
> > 1 file changed, 112 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/soc/nuvoton/npcm-lpc-bpc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/soc/nuvoton/npcm-lpc-bpc.yaml b/Documentation/devicetree/bindings/soc/nuvoton/npcm-lpc-bpc.yaml
> > new file mode 100644
> > index 000000000000..2c8e66546891
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/nuvoton/npcm-lpc-bpc.yaml
>
> Filename should match compatibles, at least in the "vendor,device"
> style, so for example: nuvoton,lpc.yaml
>
> > @@ -0,0 +1,112 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/soc/nuvoton/npcm-lpc-bpc.yaml#
>
> LPC is a generic bus, so this should not be in "soc" directory. Where?
> Depends what is this... Generic bus bindings could be in "bus" directory
> or dedicated "lpc", if we have more of these.
The BPC can run also on the eSPI bus therefore I think it better to
remove the LPC and describe only the BPC module it self.
>
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Nuvoton Low Pin Count (LPC) Bus Controller
> > +
> > +maintainers:
> > + - Tomer Maimon <tmaimon77@xxxxxxxxx>
> > +
> > +description:
> > + The Low Pin Count (LPC) is a low bandwidth bus that is used to connect
> > + peripherals around the CPU and to replace the Industry Standard Architecture
> > + (ISA) bus.
>
> You need to decide whether you describe here bus, bus controller or
> device on the bus.
>
> > +
> > + The Nuvoton NPCM LPC bus is a bridge of host CPU to a several of peripheral
> > + devices.
>
> LPC bus is a bridge? It's either incorrect or so generic that every bus
> is a "bridge"?
>
> > +
> > +properties:
> > + compatible:
> > + items:
> > + - enum:
> > + - nuvoton,npcm750-lpc
> > + - nuvoton,npcm845-lpc
> > + - const: simple-mfd
> > + - const: syscon
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + "#address-cells":
> > + const: 1
> > +
> > + "#size-cells":
> > + const: 1
> > +
> > + ranges: true
> > +
> > +patternProperties:
> > + "^lpc_bpc@[0-9a-f]+$":
>
> No underscores in node names. Generic node names, so maybe "bpc"
>
> This also does not match your example at all.
>
>
> > + type: object
> > + additionalProperties: false
> > +
> > + description:
> > + Nuvoton BMC NPCM BIOS Post Code (BPC) monitoring two configurable I/O
> > + addresses written by the host on the Low Pin Count (LPC) bus, the capure
>
> typo: capture
>
> > + data stored in 128-word FIFO.
> > +
> > + NPCM BPC supports capture double words, when using capture
> > + double word only I/O address 1 is monitored.
>
> This sentence is not grammatically correct. BPC supports capturing
> double words when using double word capturing? Aren't these two sentences?
>
> > +
> > + properties:
> > + compatible:
> > + items:
>
> No items here.
>
> > + - enum:
> > + - nuvoton,npcm750-lpc-bpc
> > + - nuvoton,npcm845-lpc-bpc
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + nuvoton,monitor-ports:
> > + $ref: /schemas/types.yaml#/definitions/uint32-array
> > + description:
> > + Contain monitor I/O addresses, at least one monitor I/O address
>
> Contains
>
> But you need to explain what are these... I/O addresses on the bus?
>
> > + required.
> > +
> > + nuvoton,bpc-en-dwcapture:
> > + description: If present, Enable capture double words support.
>
> Is it the same as reg-io-width?
>
> > + type: boolean
> > +
> > + required:
> > + - compatible
> > + - reg
> > + - interrupts
> > + - nuvoton,monitor-ports
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - "#address-cells"
> > + - "#size-cells"
> > + - ranges
> > +
> > +additionalProperties:
> > + type: object
>
> No, only bus schemas could have it. Here additionalProperties: false.
>
> It seems there are already few LPC controllers and all are put in
> different places:
> Documentation/devicetree/bindings/mfd/aspeed-lpc.yaml
> Documentation/devicetree/bindings/arm/hisilicon/low-pin-count.yaml
>
> Maybe Rob why this was made not really as two bindings - for bus
> controller and devices?
As mention above, next patch I will describe only the BPC device.
>
> Best regards,
> Krzysztof
>

In general, I waiting for Arnd approval for adding the NPCM BPC driver to SoC.
After Arnd approval, I will send a new patch revision.

Best regards,

Tomer