Re: [PATCH v4 2/2] dt-bindings: nand: meson: refine Amlogic NAND controller driver

[Liang Yang]

Hi Miquel,

On 2022/4/20 20:16, Miquel Raynal wrote:
> [ EXTERNAL EMAIL ]
>
> Hi Liang,
>
> > > > +maintainers:
> > > > +  - liang.yang@xxxxxxxxxxx
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - "amlogic,meson-gxl-nfc"
> > > > +      - "amlogic,meson-axg-nfc"
> > > > +
> > > > +  reg:
> > > > +    maxItems: 2
> > > > +
> > > > +  '#address-cells':
> > > > +    const: 1
> > >
> > > Not sure this property is needed.
> > this is for the subnode, such as nand@0.
>
> Yes but if you refer to nand-controller.yaml you no longer need these.
ok, i will try it.
> > >    
> > > > +
> > > > +  '#size-cells':
> > > > +    const: 0
> > >
> > > Ditto. Plus, this one looks wrong anyway.
> > this is for the subnode, such as nand@0. do you mean s/''/""/?
>
> Sorry, this is not "wrong anyway", my fault. But still, you don't need
> this property for the same reason as above.
ok.
> > >    
> > > > +
> > > > +  reg-names:
> > > > +    items:
> > > > +      - const: nfc
> > > > +      - const: emmc
> > >
> > > Why do you need the emmc register map? Do you really need to perform a
> > > register access there?
> > yes, we have to access the emmc register map. because the NFC clock comes from SDEMMC_CLOCK register.
>
> But if it's a clock you should get the clock and call
> clk_prepare_enable(), you don't need to poke directly in the registers.
> Do you?

no, it doesn't. it is special and the reason why need to implement a MMC 
sub clock driver previously. also we don't implement it as a clock 
provider in drivers/clk/meson/, because the SDEMMC_CLOCK register is 
internal in MCI controller.
> > > > +examples:
> > > > +  - |
> > > > +    #include <dt-bindings/clock/axg-clkc.h>
> > > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > > +    apb {
> > > > +      #address-cells = <2>;
> > > > +      #size-cells = <2>;
> > >
> > > Not sure you need this upper node in the example.
> > use the upper node to indicate the "#address-cells" and "#size-cells". if i do not do that, dt_binding_check will report:
> >    ".....reg:0: [0, 30720, 0, 256] is too long" and
> >    ".....reg:1: [0, 28672, 0, 2048] is too long".
>
> ok, maybe, I'll let bindings maintainer review that.

ok. thanks.

> > >    
> > > > +      nand-controller@7800 {
> > > > +        #address-cells = <1>;
> > > > +        #size-cells = <0>;
> > > > +        compatible = "amlogic,meson-axg-nfc";
> > > > +        reg = <0x0 0x7800 0x0 0x100>,
> > > > +              <0x0 0x7000 0x0 0x800>;
> > > > +        reg-names = "nfc", "emmc";
> > > > +
> > > > +        interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>;
> > > > +        clocks = <&clkc CLKID_SD_EMMC_C>,
> > > > +                 <&clkc CLKID_FCLK_DIV2>;
> > > > +        clock-names = "core", "device";
> > > > +
> > > > +      };
> > > > +    };
> > > > +...
>
>
> Thanks,
> Miquèl
>
> .