Re: [PATCH 1/2] dt-bindings: devapc: Update bindings

[Nina Wu]

Hi, Rob


On Fri, 2021-03-26 at 13:58 -0600, Rob Herring wrote:
> On Fri, Mar 26, 2021 at 03:31:10PM +0800, Nina Wu wrote:
> > From: Nina Wu <Nina-CM.Wu@xxxxxxxxxxxx>
> > 
> > To support newer hardware architecture of devapc,
> > update device tree bindings.
> > 
> > Signed-off-by: Nina Wu <Nina-CM.Wu@xxxxxxxxxxxx>
> > ---
> >  .../devicetree/bindings/soc/mediatek/devapc.yaml   | 41 ++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/soc/mediatek/devapc.yaml b/Documentation/devicetree/bindings/soc/mediatek/devapc.yaml
> > index 31e4d3c..489f6a9 100644
> > --- a/Documentation/devicetree/bindings/soc/mediatek/devapc.yaml
> > +++ b/Documentation/devicetree/bindings/soc/mediatek/devapc.yaml
> > @@ -20,9 +20,27 @@ properties:
> >    compatible:
> >      enum:
> >        - mediatek,mt6779-devapc
> > +      - mediatek,mt8192-devapc
> > +
> > +  version:
> > +    description: The version of the hardware architecture
> 
> This should be implied by the compatible string.

The version attribute is used to decide how we interpret the debug info
got from registers.
As you mentioned, we can know the version of the architecture from the
compatible, but I think there will be code like this:

if (compatible is mt6779) version = 1
else if (compatible is mt8192) version = 2

And once we have more chips to support, the code will be quite long.
So I prefer to add a 'version' here.


> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [1, 2]
> > +    maxItems: 1
> > +
> > +  slave_type_num:
> 
> vendor prefix needed and s/_/-/

I will fixed in next version.

> 
> > +    description: The number of the devapc set
> 
> What?

For mt8192, there are multiple pieces of devapc HW for different subsys.
EX: infra devapc, peri devapc, etc.
'slave_type_num' is the total number of the devapc HW.
I cannot come up with an accurate description, though.


> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [1, 4]
> > +    maxItems: 1
> >  
> >    reg:
> >      description: The base address of devapc register bank
> > +    maxItems: 4
> 
> Need to define what each region is.

I will fix it in the next version.

> 
> > +
> > +  vio_idx_num:
> 
> vendor prefix needed and s/_/-/

OK, will be fixed in the next version.

> 
> > +    description: The number of the devices controlled by devapc
> 
> No need to know which devices?

yes, the current driver does not care each of them.

> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> >      maxItems: 1
> 
> uint32-array with 'maxItems: 1' is just 'uint32'
> 

got it, so it should be 'maxItems: 4'

> >  
> >    interrupts:
> > @@ -39,7 +57,10 @@ properties:
> >  
> >  required:
> >    - compatible
> > +  - version
> > +  - slave_type_num
> >    - reg
> > +  - vio_idx_num
> >    - interrupts
> >    - clocks
> >    - clock-names
> > @@ -53,8 +74,28 @@ examples:
> >  
> >      devapc: devapc@10207000 {
> >        compatible = "mediatek,mt6779-devapc";
> > +      version = <1>;
> > +      slave_type_num = <1>;
> >        reg = <0x10207000 0x1000>;
> > +      vio_idx_num = <511>;
> >        interrupts = <GIC_SPI 168 IRQ_TYPE_LEVEL_LOW>;
> >        clocks = <&infracfg_ao CLK_INFRA_DEVICE_APC>;
> >        clock-names = "devapc-infra-clock";
> >      };
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/clock/mt8192-clk.h>
> > +
> > +    devapc: devapc@10207000 {
> > +        compatible = "mediatek,mt8192-devapc";
> > +        version = <2>;
> > +        slave_type_num = <4>;
> > +        reg = <0 0x10207000 0 0x1000>,
> > +            <0 0x10274000 0 0x1000>,
> > +            <0 0x10275000 0 0x1000>,
> > +            <0 0x11020000 0 0x1000>;
> > +        vio_idx_num = <367 292 242 58>;
> 
> Is the length of this the same as the value of slave_type_num? If so, 
> don't need both.
> 

yes, the length is equal to slave_type_num.
I will try to remove it in the next version.

> > +        interrupts = <GIC_SPI 187 IRQ_TYPE_LEVEL_HIGH>;
> > +        clocks = <&infracfg_ao CLK_INFRA_DEVICE_APC>;
> > +        clock-names = "devapc-infra-clock";
> > +    };
> > -- 
> > 2.6.4
> >