hi Krzysztof
On Thu, Nov 17, 2022 at 1:11 AM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 16/11/2022 14:31, Zhe Wang wrote:
> > Add Unisoc ums9620 ufs host controller devicetree document.
> >
> > Signed-off-by: Zhe Wang <zhe.wang1@xxxxxxxxxx>
>
> Thank you for your patch. There is something to discuss/improve.
>
> > +
> > + clock-names:
> > + items:
> > + - const: ufs_eb
> > + - const: ufs_cfg_eb
> > + - const: ufsh
> > + - const: ufsh_source
> > +
> > + resets:
> > + maxItems: 2
> > +
> > + reset-names:
> > + items:
> > + - const: ufs
> > + - const: ufsdev
>
> Both clock names and resets are still not useful. "ufs" is the name of
> the block, so reset name of "ufs" and "ufsdev" says nothing. This is the
> dev right?
>
Yes, this means reset on the device side. How about the modification below?
+ clock-names:
+ items:
+ - const: controller_eb
+ - const: cfg_eb
+ - const: core
+ - const: core_source
+
+ resets:
+ maxItems: 2
+
+ reset-names:
+ items:
+ - const: controller
+ - const: device
> > +
> > + vdd-mphy-supply:
> > + description:
> > + Phandle to vdd-mphy supply regulator node.
> > +
> > + sprd,ufs-anlg-syscon:
> > + $ref: /schemas/types.yaml#/definitions/phandle
> > + description: phandle of syscon used to control ufs analog regs.
> > +
> > + sprd,aon-apb-syscon:
> > + $ref: /schemas/types.yaml#/definitions/phandle
> > + description: phandle of syscon used to control always-on regs.
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - clocks
> > + - clock-names
> > + - resets
> > + - reset-names
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > + ufs: ufs@22000000 {
> > + compatible = "sprd,ums9620-ufs";
> > + reg = <0x22000000 0x3000>;
> > + interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>;
> > + vcc-supply = <&vddemmccore>;
> > + vdd-mphy-supply = <&vddufs1v2>;
> > + clock-names = "ufs_eb", "ufs_cfg_eb", "ufsh", "ufsh_source";
> > + clocks = <&apahb_gate 5>, <&apahb_gate 22>, <&aonapb_clk 52>, <&g5l_pll 12>;
>
> First clocks, then names.
>
I'll fix it.
> > + reset-names = "ufs", "ufsdev";
> > + resets = <&apahb_gate 4>, <&aonapb_gate 69>;
>
> First resets, then names.
>
I'll fix it.
Best regards,
Zhe
> > + sprd,ufs-anlg-syscon = <&anlg_phy_g12_regs>;
> > + sprd,aon-apb-syscon = <&aon_apb_regs>;
> > + };
>
> Best regards,
> Krzysztof
>
