| Message ID | 20230221141147.303642-1-xingyu.wu@starfivetech.com |
|---|---|
| Headers | show |
| Series | Add PLL clocks driver for StarFive JH7110 | expand |
On 2023/2/22 17:09, Krzysztof Kozlowski wrote: > On 21/02/2023 15:11, Xingyu Wu wrote: >> Add the PLL clock node for the Starfive JH7110 SoC and >> modify the SYSCRG node to add PLL clocks. >> >> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com> >> --- >> arch/riscv/boot/dts/starfive/jh7110.dtsi | 15 +++++++++++++-- >> 1 file changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi >> index b6612c53d0d2..0cb8d86ebce5 100644 >> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi >> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi >> @@ -461,12 +461,16 @@ syscrg: clock-controller@13020000 { >> <&gmac1_rgmii_rxin>, >> <&i2stx_bclk_ext>, <&i2stx_lrck_ext>, >> <&i2srx_bclk_ext>, <&i2srx_lrck_ext>, >> - <&tdm_ext>, <&mclk_ext>; >> + <&tdm_ext>, <&mclk_ext>, >> + <&pllclk JH7110_CLK_PLL0_OUT>, >> + <&pllclk JH7110_CLK_PLL1_OUT>, >> + <&pllclk JH7110_CLK_PLL2_OUT>; >> clock-names = "osc", "gmac1_rmii_refin", >> "gmac1_rgmii_rxin", >> "i2stx_bclk_ext", "i2stx_lrck_ext", >> "i2srx_bclk_ext", "i2srx_lrck_ext", >> - "tdm_ext", "mclk_ext"; >> + "tdm_ext", "mclk_ext", >> + "pll0_out", "pll1_out", "pll2_out"; >> #clock-cells = <1>; >> #reset-cells = <1>; >> }; >> @@ -476,6 +480,13 @@ sys_syscon: syscon@13030000 { >> reg = <0x0 0x13030000 0x0 0x1000>; >> }; >> >> + pllclk: pll-clock-controller { > > Does not look like you tested the DTS against bindings. Please run `make > dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst > for instructions). You should see here warnings of mixing non-MMIO nodes > in MMIO-bus. > Oh I cherry-pick the commit of syscon node and it also include the MMC node. I will remove the MMC node. I used dtbs_check and get the error 'should not be valid under {'type': 'object'}', If I move this node out of the 'soc' node, the dtbs_check will be pass. Is it OK to move the PLL node out of the 'soc' node? Thanks. Best regards, Xingyu Wu
On 23/02/2023 09:47, Xingyu Wu wrote: > On 2023/2/22 17:09, Krzysztof Kozlowski wrote: >> On 21/02/2023 15:11, Xingyu Wu wrote: >>> Add the PLL clock node for the Starfive JH7110 SoC and >>> modify the SYSCRG node to add PLL clocks. >>> >>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com> >>> --- >>> arch/riscv/boot/dts/starfive/jh7110.dtsi | 15 +++++++++++++-- >>> 1 file changed, 13 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi >>> index b6612c53d0d2..0cb8d86ebce5 100644 >>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi >>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi >>> @@ -461,12 +461,16 @@ syscrg: clock-controller@13020000 { >>> <&gmac1_rgmii_rxin>, >>> <&i2stx_bclk_ext>, <&i2stx_lrck_ext>, >>> <&i2srx_bclk_ext>, <&i2srx_lrck_ext>, >>> - <&tdm_ext>, <&mclk_ext>; >>> + <&tdm_ext>, <&mclk_ext>, >>> + <&pllclk JH7110_CLK_PLL0_OUT>, >>> + <&pllclk JH7110_CLK_PLL1_OUT>, >>> + <&pllclk JH7110_CLK_PLL2_OUT>; >>> clock-names = "osc", "gmac1_rmii_refin", >>> "gmac1_rgmii_rxin", >>> "i2stx_bclk_ext", "i2stx_lrck_ext", >>> "i2srx_bclk_ext", "i2srx_lrck_ext", >>> - "tdm_ext", "mclk_ext"; >>> + "tdm_ext", "mclk_ext", >>> + "pll0_out", "pll1_out", "pll2_out"; >>> #clock-cells = <1>; >>> #reset-cells = <1>; >>> }; >>> @@ -476,6 +480,13 @@ sys_syscon: syscon@13030000 { >>> reg = <0x0 0x13030000 0x0 0x1000>; >>> }; >>> >>> + pllclk: pll-clock-controller { >> >> Does not look like you tested the DTS against bindings. Please run `make >> dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst >> for instructions). You should see here warnings of mixing non-MMIO nodes >> in MMIO-bus. >> > > Oh I cherry-pick the commit of syscon node and it also include the MMC node. > I will remove the MMC node. > I used dtbs_check and get the error 'should not be valid under {'type': 'object'}', > If I move this node out of the 'soc' node, the dtbs_check will be pass. > Is it OK to move the PLL node out of the 'soc' node? Thanks. Shall it be out side of soc? How it can then do anything with registers? This does not look like correct representation of hardware. Best regards, Krzysztof
On 2023/2/23 17:35, Krzysztof Kozlowski wrote: > On 23/02/2023 10:32, Xingyu Wu wrote: >> On 2023/2/23 16:56, Krzysztof Kozlowski wrote: >>> On 21/02/2023 15:11, Xingyu Wu wrote: >>>> Add driver for the StarFive JH7110 PLL clock controller and >>>> modify the JH7110 system clock driver to rely on this PLL clocks. >>>> >>>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com> >>>> --- >>> >>> >>>> + >>>> +static int jh7110_pll_clk_probe(struct platform_device *pdev) >>>> +{ >>>> + int ret; >>>> + struct of_phandle_args args; >>>> + struct regmap *pll_syscon_regmap; >>>> + unsigned int idx; >>>> + struct jh7110_clk_pll_priv *priv; >>>> + struct jh7110_clk_pll_data *data; >>>> + char *pll_name[JH7110_PLLCLK_END] = { >>>> + "pll0_out", >>>> + "pll1_out", >>>> + "pll2_out" >>>> + }; >>>> + >>>> + priv = devm_kzalloc(&pdev->dev, >>>> + struct_size(priv, data, JH7110_PLLCLK_END), >>>> + GFP_KERNEL); >>>> + if (!priv) >>>> + return -ENOMEM; >>>> + >>>> + priv->dev = &pdev->dev; >>>> + ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node, "starfive,sysreg", 0, 0, &args); >>> >>> 1. Wrong wrapping. Wrap code at 80 as coding style asks. >>> >>> 2. Why you are using syscon for normal, device MMIO operation? Your DTS >>> also points that this is incorrect, hacky representation of hardware. >>> Don't add devices to DT to fake places and then overuse syscon to fix >>> that fake placement. The clock is in system registers, thus it must be >>> there. >>> >>> 3. Even if this stays, why so complicated code instead of >>> syscon_regmap_lookup_by_phandle()? >>> >> >> Thanks for your advice. Will use syscon_regmap_lookup_by_phandle instead it >> and remove useless part. > > So you ignored entirely part 2? This was the main comment... I am going > to keep NAK-ing it then. What I understand to mean is that I cannot use a fake node to operate syscon registers. So I should move the PLL node under syscon node directly. Is it ok? Best regards, Xingyu Wu
On 23/02/2023 11:03, Xingyu Wu wrote: > On 2023/2/23 17:35, Krzysztof Kozlowski wrote: >> On 23/02/2023 10:32, Xingyu Wu wrote: >>> On 2023/2/23 16:56, Krzysztof Kozlowski wrote: >>>> On 21/02/2023 15:11, Xingyu Wu wrote: >>>>> Add driver for the StarFive JH7110 PLL clock controller and >>>>> modify the JH7110 system clock driver to rely on this PLL clocks. >>>>> >>>>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com> >>>>> --- >>>> >>>> >>>>> + >>>>> +static int jh7110_pll_clk_probe(struct platform_device *pdev) >>>>> +{ >>>>> + int ret; >>>>> + struct of_phandle_args args; >>>>> + struct regmap *pll_syscon_regmap; >>>>> + unsigned int idx; >>>>> + struct jh7110_clk_pll_priv *priv; >>>>> + struct jh7110_clk_pll_data *data; >>>>> + char *pll_name[JH7110_PLLCLK_END] = { >>>>> + "pll0_out", >>>>> + "pll1_out", >>>>> + "pll2_out" >>>>> + }; >>>>> + >>>>> + priv = devm_kzalloc(&pdev->dev, >>>>> + struct_size(priv, data, JH7110_PLLCLK_END), >>>>> + GFP_KERNEL); >>>>> + if (!priv) >>>>> + return -ENOMEM; >>>>> + >>>>> + priv->dev = &pdev->dev; >>>>> + ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node, "starfive,sysreg", 0, 0, &args); >>>> >>>> 1. Wrong wrapping. Wrap code at 80 as coding style asks. >>>> >>>> 2. Why you are using syscon for normal, device MMIO operation? Your DTS >>>> also points that this is incorrect, hacky representation of hardware. >>>> Don't add devices to DT to fake places and then overuse syscon to fix >>>> that fake placement. The clock is in system registers, thus it must be >>>> there. >>>> >>>> 3. Even if this stays, why so complicated code instead of >>>> syscon_regmap_lookup_by_phandle()? >>>> >>> >>> Thanks for your advice. Will use syscon_regmap_lookup_by_phandle instead it >>> and remove useless part. >> >> So you ignored entirely part 2? This was the main comment... I am going >> to keep NAK-ing it then. > > What I understand to mean is that I cannot use a fake node to operate syscon > registers. So I should move the PLL node under syscon node directly. Is it ok? Yes, because it looks like entire PLL clock control is from the syscon node, thus the clocks are there. Best regards, Krzysztof
On 2023/2/23 18:10, Krzysztof Kozlowski wrote: > On 23/02/2023 11:03, Xingyu Wu wrote: >> On 2023/2/23 17:35, Krzysztof Kozlowski wrote: >>> On 23/02/2023 10:32, Xingyu Wu wrote: >>>> On 2023/2/23 16:56, Krzysztof Kozlowski wrote: >>>>> On 21/02/2023 15:11, Xingyu Wu wrote: >>>>>> Add driver for the StarFive JH7110 PLL clock controller and >>>>>> modify the JH7110 system clock driver to rely on this PLL clocks. >>>>>> >>>>>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com> >>>>>> --- >>>>> >>>>> >>>>>> + >>>>>> +static int jh7110_pll_clk_probe(struct platform_device *pdev) >>>>>> +{ >>>>>> + int ret; >>>>>> + struct of_phandle_args args; >>>>>> + struct regmap *pll_syscon_regmap; >>>>>> + unsigned int idx; >>>>>> + struct jh7110_clk_pll_priv *priv; >>>>>> + struct jh7110_clk_pll_data *data; >>>>>> + char *pll_name[JH7110_PLLCLK_END] = { >>>>>> + "pll0_out", >>>>>> + "pll1_out", >>>>>> + "pll2_out" >>>>>> + }; >>>>>> + >>>>>> + priv = devm_kzalloc(&pdev->dev, >>>>>> + struct_size(priv, data, JH7110_PLLCLK_END), >>>>>> + GFP_KERNEL); >>>>>> + if (!priv) >>>>>> + return -ENOMEM; >>>>>> + >>>>>> + priv->dev = &pdev->dev; >>>>>> + ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node, "starfive,sysreg", 0, 0, &args); >>>>> >>>>> 1. Wrong wrapping. Wrap code at 80 as coding style asks. >>>>> >>>>> 2. Why you are using syscon for normal, device MMIO operation? Your DTS >>>>> also points that this is incorrect, hacky representation of hardware. >>>>> Don't add devices to DT to fake places and then overuse syscon to fix >>>>> that fake placement. The clock is in system registers, thus it must be >>>>> there. >>>>> >>>>> 3. Even if this stays, why so complicated code instead of >>>>> syscon_regmap_lookup_by_phandle()? >>>>> >>>> >>>> Thanks for your advice. Will use syscon_regmap_lookup_by_phandle instead it >>>> and remove useless part. >>> >>> So you ignored entirely part 2? This was the main comment... I am going >>> to keep NAK-ing it then. >> >> What I understand to mean is that I cannot use a fake node to operate syscon >> registers. So I should move the PLL node under syscon node directly. Is it ok? > > Yes, because it looks like entire PLL clock control is from the syscon > node, thus the clocks are there. Thanks for the guidance, I will modify it in the next patch. Best regards, Xingyu Wu