Hi Krzysztof, Thank you for your review! On Thu, Nov 10, 2022 at 10:28 PM Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote: > > On 10/11/2022 14:36, Zhe Wang wrote: > > From: Zhe Wang <zhe.wang1@xxxxxxxxxx> > > > > Add Unisoc ums9620 ufs host controller devicetree document. > > > > Signed-off-by: Zhe Wang <zhe.wang1@xxxxxxxxxx> > > --- > > .../devicetree/bindings/ufs/sprd,ufs.yaml | 72 +++++++++++++++++++ > > 1 file changed, 72 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/ufs/sprd,ufs.yaml > > > > diff --git a/Documentation/devicetree/bindings/ufs/sprd,ufs.yaml b/Documentation/devicetree/bindings/ufs/sprd,ufs.yaml > > new file mode 100644 > > index 000000000000..88f2c670b0a4 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/ufs/sprd,ufs.yaml > > > Filename matching the compatible, so sprd,ums9620-ufs.yaml, unless you > expect this to grow already? If so, can you post the rest? > Currently only ums9620 will be uploaded, I'll fix it. > > @@ -0,0 +1,72 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/ufs/sprd,ufs.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Unisoc Universal Flash Storage (UFS) Controller > > + > > +maintainers: > > + - Zhe Wang <zhe.wang1@xxxxxxxxxx> > > + > > +allOf: > > + - $ref: ufs-common.yaml > > + > > +properties: > > + compatible: > > + enum: > > + - sprd,ums9620-ufs > > + > > + clocks: > > + maxItems: 2 > > + > > + clock-names: > > + items: > > + - const: hclk > > + - const: hclk_source > > Can you make these descriptive? "clk" is redundant, so basically you are > saying name is "h" and "h_source"? > I'll fix it. > > + > > + resets: > > + maxItems: 2 > > + > > + reset-names: > > + items: > > + - const: ufs_soft_rst > > + - const: ufsdev_soft_rst > > Drop "_rst" from both. > Will remove this. > > + > > + sprd,ufs-anly-reg-syscon: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: phandle of syscon used to control ufs analog reg. > > It's a reg? Then such syntax is expected: > https://elixir.bootlin.com/linux/v5.18-rc1/source/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml#L42 > > I will modify it based on this example. > > + sprd,aon-apb-syscon: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: phandle of syscon used to control always-on reg. > > It's a reg? Then such syntax is expected: > https://elixir.bootlin.com/linux/v5.18-rc1/source/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml#L42 > I will modify it based on this example. > > + > > +required: > > + - compatible > > + - reg > > + - clocks > > + - clock-names > > + - resets > > + > > +unevaluatedProperties: false > > + > > +examples: > > + - | > > + ufs: ufs@22000000 { > > + compatible = "sprd,ums9620-ufs"; > > + reg = <0x22000000 0x3000>; > > + interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>; > > + vcc-supply = <&vddemmcore>; > > + vdd-mphy-supply = <&vddufs1v2>; > > + clocks-name = "ufs_eb", "ufs_cfg_eb", > > + "ufs_hclk", "ufs_hclk_source"; > > Align the lines. > I'll fix it. > > + clocks = <&apahb_gate CLK_UFS_EB>, <&apahb_gate CLK_UFS_CFG_EB>, > > + <&onapb_clk CLK_UFS_AON>, <&g51_pll CLK_TGPLL_256M>; > > + freq-table-hz = <0 0>, <0 0>, <0 0>, <0 0>; > > Why this is empty? What's the use of empty table? > freq-table-hz is used to configure the maximum frequency and minimum frequency of clk, and an empty table means that no scaling up\down operation is requiredfor the frequency of these clks. > > + reset-names = "ufs_soft_rst", "ufsdev_soft_rst"; > > + resets = <&apahb_gate RESET_AP_AHB_UFS_SOFT_RST>, > > + <&aonapb_gate RESET_AON_APB_UFSDEV_SOFT_RST>; > > + sprd,ufs-anly-reg-syscon = <&anly_phy_g12_regs>; > > + sprd,aon-apb-syscon = <&aon_apb_regs>; > > + status = "disable"; > > Drop status. > Will remove this. Best regards, Zhe Wang > > + }; > > Best regards, > Krzysztof >
