| Message ID | 20221110133640.30522-2-zhewang116@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Unisoc UFS host controller | expand |
On 10/11/2022 14:36, Zhe Wang wrote: > From: Zhe Wang <zhe.wang1@unisoc.com> > > Add Unisoc ums9620 ufs host controller devicetree document. > > Signed-off-by: Zhe Wang <zhe.wang1@unisoc.com> > --- > .../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? > @@ -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@unisoc.com> > + > +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"? > + > + resets: > + maxItems: 2 > + > + reset-names: > + items: > + - const: ufs_soft_rst > + - const: ufsdev_soft_rst Drop "_rst" from both. > + > + 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 > + 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 > + > +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. > + 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? > + 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. > + }; Best regards, Krzysztof
On Thu, 10 Nov 2022 21:36:39 +0800, Zhe Wang wrote: > From: Zhe Wang <zhe.wang1@unisoc.com> > > Add Unisoc ums9620 ufs host controller devicetree document. > > Signed-off-by: Zhe Wang <zhe.wang1@unisoc.com> > --- > .../devicetree/bindings/ufs/sprd,ufs.yaml | 72 +++++++++++++++++++ > 1 file changed, 72 insertions(+) > create mode 100644 Documentation/devicetree/bindings/ufs/sprd,ufs.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: Error: Documentation/devicetree/bindings/ufs/sprd,ufs.example.dts:24.27-28 syntax error FATAL ERROR: Unable to parse input tree make[1]: *** [scripts/Makefile.lib:406: Documentation/devicetree/bindings/ufs/sprd,ufs.example.dtb] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1492: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
Hi Krzysztof, Thank you for your review! On Thu, Nov 10, 2022 at 10:28 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 10/11/2022 14:36, Zhe Wang wrote: > > From: Zhe Wang <zhe.wang1@unisoc.com> > > > > Add Unisoc ums9620 ufs host controller devicetree document. > > > > Signed-off-by: Zhe Wang <zhe.wang1@unisoc.com> > > --- > > .../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@unisoc.com> > > + > > +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 >
On 11/11/2022 06:34, Zhe Wang wrote: >> > > 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. No, to indicate lack of scaling you skip freq-table-hz entirely, not provide empty one. Best regards, Krzysztof
Hi Krzysztof, On Fri, Nov 11, 2022 at 3:48 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 11/11/2022 06:34, Zhe Wang wrote: > >> > > > > 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. > > No, to indicate lack of scaling you skip freq-table-hz entirely, not > provide empty one. > > In the ufshcd-pltfrm.c file, the clock information is parsed by executing the function ufshcd_parse_clock_info, if the number of "freq-table-hz" is zero or if the number of "clock-names" and "freq-table-hz" does not match, the UFS CLK information in dts will not be obtained. Although we don't need to scaling freq, we also need the CLK information for the CLK GATE operations. So we cannot delete this freq-table here. > Best regards, > Krzysztof > According to the local test results just now, I would like to ask a question about the previous revisions. > > + > > + 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 > In the syntax of this example, reg is represented by phandle and offset, but I only need the information of phandle in this place，So in this scenario, whether my original syntax is fine？ just describe the pandlle. Best regards, Zhe Wang
On 11/11/2022 10:34, Zhe Wang wrote: > Hi Krzysztof, > > On Fri, Nov 11, 2022 at 3:48 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 11/11/2022 06:34, Zhe Wang wrote: >>>> >>> >>> 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. >> >> No, to indicate lack of scaling you skip freq-table-hz entirely, not >> provide empty one. >> >> > > In the ufshcd-pltfrm.c file, the clock information is parsed by > executing the function ufshcd_parse_clock_info, if the number of > "freq-table-hz" is zero or if the number of "clock-names" and > "freq-table-hz" does not match, the UFS CLK information in dts will > not be obtained. Although we don't need to scaling freq, we also need > the CLK information for the CLK GATE operations. So we cannot delete > this freq-table here. I did not check the driver implementation, but if that's the case it does not match bindings. Before adding empty useless tables, please either fix bindings or driver. I think the latter - the driver - becasue clocks are not depending logically on freq-table-hz. > >> Best regards, >> Krzysztof >> > > According to the local test results just now, I would like to ask a > question about the previous revisions. >>> + >>> + 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 >> > > In the syntax of this example, reg is represented by phandle and > offset, but I only need the information of phandle in this place， No. You wrote "analog reg". So one reg. Not regs. > So in > this scenario, whether my original syntax is fine？ just describe the > pandlle. No, because your description said one reg. Best regards, Krzysztof
Hi Krzysztof, On Fri, Nov 11, 2022 at 5:51 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 11/11/2022 10:34, Zhe Wang wrote: > > Hi Krzysztof, > > > > On Fri, Nov 11, 2022 at 3:48 PM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 11/11/2022 06:34, Zhe Wang wrote: > >>>> > >>> > >>> 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. > >> > >> No, to indicate lack of scaling you skip freq-table-hz entirely, not > >> provide empty one. > >> > >> > > > > In the ufshcd-pltfrm.c file, the clock information is parsed by > > executing the function ufshcd_parse_clock_info, if the number of > > "freq-table-hz" is zero or if the number of "clock-names" and > > "freq-table-hz" does not match, the UFS CLK information in dts will > > not be obtained. Although we don't need to scaling freq, we also need > > the CLK information for the CLK GATE operations. So we cannot delete > > this freq-table here. > > I did not check the driver implementation, but if that's the case it > does not match bindings. Before adding empty useless tables, please > either fix bindings or driver. I think the latter - the driver - becasue > clocks are not depending logically on freq-table-hz. > OK, I will think about how to solve this problem. > > > >> Best regards, > >> Krzysztof > >> > > > > According to the local test results just now, I would like to ask a > > question about the previous revisions. > >>> + > >>> + 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 > >> > > > > In the syntax of this example, reg is represented by phandle and > > offset, but I only need the information of phandle in this place， > > No. You wrote "analog reg". So one reg. Not regs. > > > So in > > this scenario, whether my original syntax is fine？ just describe the > > pandlle. > > No, because your description said one reg. > This is my mistake, the description is wrong, it's regs. Thank you for your review! Best regards, Zhe Wang > Best regards, > Krzysztof >
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 @@ -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@unisoc.com> + +allOf: + - $ref: ufs-common.yaml + +properties: + compatible: + enum: + - sprd,ums9620-ufs + + clocks: + maxItems: 2 + + clock-names: + items: + - const: hclk + - const: hclk_source + + resets: + maxItems: 2 + + reset-names: + items: + - const: ufs_soft_rst + - const: ufsdev_soft_rst + + sprd,ufs-anly-reg-syscon: + $ref: /schemas/types.yaml#/definitions/phandle + description: phandle of syscon used to control ufs analog reg. + + sprd,aon-apb-syscon: + $ref: /schemas/types.yaml#/definitions/phandle + description: phandle of syscon used to control always-on reg. + +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"; + 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>; + 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"; + };