| Message ID | 20210519001802.1863-1-jonathan@marek.ca |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] clk: qcom: add support for SM8350 DISPCC | expand |
Quoting Jonathan Marek (2021-05-18 17:18:02) > Add sm8350 DISPCC bindings, which are simply a symlink to the sm8250 > bindings. Update the documentation with the new compatible. > > Signed-off-by: Jonathan Marek <jonathan@marek.ca> > Reviewed-by: Rob Herring <robh@kernel.org> > --- > .../devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml | 6 ++++-- > include/dt-bindings/clock/qcom,dispcc-sm8350.h | 1 + > 2 files changed, 5 insertions(+), 2 deletions(-) > create mode 120000 include/dt-bindings/clock/qcom,dispcc-sm8350.h Why the symlink? Can we have the dt authors use the existing header file instead? > > diff --git a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml > index 0cdf53f41f84..8f414642445e 100644 > --- a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml > +++ b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml > @@ -4,24 +4,26 @@ > $id: http://devicetree.org/schemas/clock/qcom,dispcc-sm8x50.yaml# > $schema: http://devicetree.org/meta-schemas/core.yaml# > > -title: Qualcomm Display Clock & Reset Controller Binding for SM8150/SM8250 > +title: Qualcomm Display Clock & Reset Controller Binding for SM8150/SM8250/SM8350 Maybe just "Binding for SM8x50 SoCs" > > maintainers: > - Jonathan Marek <jonathan@marek.ca> > > description: | > Qualcomm display clock control module which supports the clocks, resets and > - power domains on SM8150 and SM8250. > + power domains on SM8150/SM8250/SM8350. same 8x50 comment. > > See also: > dt-bindings/clock/qcom,dispcc-sm8150.h > dt-bindings/clock/qcom,dispcc-sm8250.h > + dt-bindings/clock/qcom,dispcc-sm8350.h > > properties: > compatible: > enum: > - qcom,sm8150-dispcc > - qcom,sm8250-dispcc > + - qcom,sm8350-dispcc > > clocks: > items: > diff --git a/include/dt-bindings/clock/qcom,dispcc-sm8350.h b/include/dt-bindings/clock/qcom,dispcc-sm8350.h > new file mode 120000 > index 000000000000..0312b4544acb > --- /dev/null > +++ b/include/dt-bindings/clock/qcom,dispcc-sm8350.h > @@ -0,0 +1 @@ > +qcom,dispcc-sm8250.h > \ No newline at end of file
Quoting Jonathan Marek (2021-05-18 17:18:01) > diff --git a/drivers/clk/qcom/dispcc-sm8250.c b/drivers/clk/qcom/dispcc-sm8250.c > index de09cd5c209f..5f22a715e2f0 100644 > --- a/drivers/clk/qcom/dispcc-sm8250.c > +++ b/drivers/clk/qcom/dispcc-sm8250.c > @@ -36,6 +36,10 @@ static struct pll_vco vco_table[] = { > { 249600000, 2000000000, 0 }, > }; > > +static struct pll_vco lucid_5lpe_vco[] = { const > + { 249600000, 1750000000, 0 }, > +}; > + > static struct alpha_pll_config disp_cc_pll0_config = { > .l = 0x47, > .alpha = 0xE000, > @@ -1039,6 +1043,7 @@ static const struct qcom_cc_desc disp_cc_sm8250_desc = { > static const struct of_device_id disp_cc_sm8250_match_table[] = { > { .compatible = "qcom,sm8150-dispcc" }, > { .compatible = "qcom,sm8250-dispcc" }, > + { .compatible = "qcom,sm8350-dispcc" }, > { } > }; > MODULE_DEVICE_TABLE(of, disp_cc_sm8250_match_table); > @@ -1051,7 +1056,7 @@ static int disp_cc_sm8250_probe(struct platform_device *pdev) > if (IS_ERR(regmap)) > return PTR_ERR(regmap); > > - /* note: trion == lucid, except for the prepare() op */ > + /* Apply differences for SM8150 and SM8350 */ > BUILD_BUG_ON(CLK_ALPHA_PLL_TYPE_TRION != CLK_ALPHA_PLL_TYPE_LUCID); > if (of_device_is_compatible(pdev->dev.of_node, "qcom,sm8150-dispcc")) { > disp_cc_pll0_config.config_ctl_hi_val = 0x00002267; > @@ -1062,8 +1067,62 @@ static int disp_cc_sm8250_probe(struct platform_device *pdev) > disp_cc_pll1_config.config_ctl_hi1_val = 0x00000024; > disp_cc_pll1_config.user_ctl_hi1_val = 0x000000D0; > disp_cc_pll1_init.ops = &clk_alpha_pll_trion_ops; > + } else if (of_device_is_compatible(pdev->dev.of_node, "qcom,sm8350-dispcc")) { > + static struct clk_rcg2* const rcgs[] = { Please move this to global scope and give it a name. > + &disp_cc_mdss_byte0_clk_src, > + &disp_cc_mdss_byte1_clk_src, > + &disp_cc_mdss_dp_aux1_clk_src, > + &disp_cc_mdss_dp_aux_clk_src, > + &disp_cc_mdss_dp_link1_clk_src, > + &disp_cc_mdss_dp_link_clk_src, > + &disp_cc_mdss_dp_pixel1_clk_src, > + &disp_cc_mdss_dp_pixel2_clk_src, > + &disp_cc_mdss_dp_pixel_clk_src, > + &disp_cc_mdss_esc0_clk_src, > + &disp_cc_mdss_mdp_clk_src, > + &disp_cc_mdss_pclk0_clk_src, > + &disp_cc_mdss_pclk1_clk_src, > + &disp_cc_mdss_rot_clk_src, > + &disp_cc_mdss_vsync_clk_src, > + }; > + static struct clk_regmap_div* const divs[] = { Move global. > + &disp_cc_mdss_byte0_div_clk_src, > + &disp_cc_mdss_byte1_div_clk_src, > + &disp_cc_mdss_dp_link1_div_clk_src, > + &disp_cc_mdss_dp_link_div_clk_src, > + }; > + unsigned i; int i? I doubt being unsigned helps. > + static bool offset_applied = false; > + > + /* only apply the offsets once (in case of deferred probe) */ > + if (!offset_applied) { Maybe instead of doing this in probe it can be done when the driver is added in module init? It would mean searching the DT for the compatible string and then if it is present running the subtraction code, but at least we would only do it once and the code would be thrown away after init. > + for (i = 0; i < ARRAY_SIZE(rcgs); i++) > + rcgs[i]->cmd_rcgr -= 4; > + > + for (i = 0; i < ARRAY_SIZE(divs); i++) { > + divs[i]->reg -= 4; > + divs[i]->width = 4; > + } > + > + disp_cc_mdss_ahb_clk.halt_reg -= 4; > + disp_cc_mdss_ahb_clk.clkr.enable_reg -= 4; > + > + offset_applied = true; > + } > + > + disp_cc_mdss_ahb_clk_src.cmd_rcgr = 0x22a0; > + > + disp_cc_pll0_config.config_ctl_hi1_val = 0x2A9A699C; > + disp_cc_pll0_config.test_ctl_hi1_val = 0x01800000; > + disp_cc_pll0_init.ops = &clk_alpha_pll_lucid_5lpe_ops; > + disp_cc_pll0.vco_table = lucid_5lpe_vco; > + disp_cc_pll1_config.config_ctl_hi1_val = 0x2A9A699C; Lowercase hex please. > + disp_cc_pll1_config.test_ctl_hi1_val = 0x01800000; > + disp_cc_pll1_init.ops = &clk_alpha_pll_lucid_5lpe_ops; > + disp_cc_pll1.vco_table = lucid_5lpe_vco; > } > > + /* note for SM8350: downstream lucid_5lpe configure differs slightly */ Is this a TODO? > clk_lucid_pll_configure(&disp_cc_pll0, regmap, &disp_cc_pll0_config); > clk_lucid_pll_configure(&disp_cc_pll1, regmap, &disp_cc_pll1_config); >
On 6/2/21 5:27 PM, Stephen Boyd wrote: > Quoting Jonathan Marek (2021-05-18 17:18:02) >> Add sm8350 DISPCC bindings, which are simply a symlink to the sm8250 >> bindings. Update the documentation with the new compatible. >> >> Signed-off-by: Jonathan Marek <jonathan@marek.ca> >> Reviewed-by: Rob Herring <robh@kernel.org> >> --- >> .../devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml | 6 ++++-- >> include/dt-bindings/clock/qcom,dispcc-sm8350.h | 1 + > >> 2 files changed, 5 insertions(+), 2 deletions(-) >> create mode 120000 include/dt-bindings/clock/qcom,dispcc-sm8350.h > > Why the symlink? Can we have the dt authors use the existing header file > instead? > It would be strange to include bindings with the name of a different SoC. I guess it is a matter a preference, is there any good reason to *not* do it like this? >> >> diff --git a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml >> index 0cdf53f41f84..8f414642445e 100644 >> --- a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml >> +++ b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml >> @@ -4,24 +4,26 @@ >> $id: http://devicetree.org/schemas/clock/qcom,dispcc-sm8x50.yaml# >> $schema: http://devicetree.org/meta-schemas/core.yaml# >> >> -title: Qualcomm Display Clock & Reset Controller Binding for SM8150/SM8250 >> +title: Qualcomm Display Clock & Reset Controller Binding for SM8150/SM8250/SM8350 > > Maybe just "Binding for SM8x50 SoCs" > Its likely these bindings won't be compatible with future "SM8x50" SoCs, listing supported SoCs explicitly will avoid confusion in the future. >> >> maintainers: >> - Jonathan Marek <jonathan@marek.ca> >> >> description: | >> Qualcomm display clock control module which supports the clocks, resets and >> - power domains on SM8150 and SM8250. >> + power domains on SM8150/SM8250/SM8350. > > same 8x50 comment. > >> >> See also: >> dt-bindings/clock/qcom,dispcc-sm8150.h >> dt-bindings/clock/qcom,dispcc-sm8250.h >> + dt-bindings/clock/qcom,dispcc-sm8350.h >> >> properties: >> compatible: >> enum: >> - qcom,sm8150-dispcc >> - qcom,sm8250-dispcc >> + - qcom,sm8350-dispcc >> >> clocks: >> items: >> diff --git a/include/dt-bindings/clock/qcom,dispcc-sm8350.h b/include/dt-bindings/clock/qcom,dispcc-sm8350.h >> new file mode 120000 >> index 000000000000..0312b4544acb >> --- /dev/null >> +++ b/include/dt-bindings/clock/qcom,dispcc-sm8350.h >> @@ -0,0 +1 @@ >> +qcom,dispcc-sm8250.h >> \ No newline at end of file
On Wed 02 Jun 16:27 CDT 2021, Stephen Boyd wrote: > Quoting Jonathan Marek (2021-05-18 17:18:02) > > Add sm8350 DISPCC bindings, which are simply a symlink to the sm8250 > > bindings. Update the documentation with the new compatible. > > > > Signed-off-by: Jonathan Marek <jonathan@marek.ca> > > Reviewed-by: Rob Herring <robh@kernel.org> > > --- > > .../devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml | 6 ++++-- > > include/dt-bindings/clock/qcom,dispcc-sm8350.h | 1 + > > > 2 files changed, 5 insertions(+), 2 deletions(-) > > create mode 120000 include/dt-bindings/clock/qcom,dispcc-sm8350.h > > Why the symlink? Can we have the dt authors use the existing header file > instead? > > > > > diff --git a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml > > index 0cdf53f41f84..8f414642445e 100644 > > --- a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml > > +++ b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml > > @@ -4,24 +4,26 @@ > > $id: http://devicetree.org/schemas/clock/qcom,dispcc-sm8x50.yaml# > > $schema: http://devicetree.org/meta-schemas/core.yaml# > > > > -title: Qualcomm Display Clock & Reset Controller Binding for SM8150/SM8250 > > +title: Qualcomm Display Clock & Reset Controller Binding for SM8150/SM8250/SM8350 > > Maybe just "Binding for SM8x50 SoCs" > That seems like a certain way to ensure that SM8450 etc will be different enough to not match this binding :) Regards, Bjorn
Quoting Jonathan Marek (2021-06-04 10:25:41) > On 6/2/21 5:27 PM, Stephen Boyd wrote: > > Quoting Jonathan Marek (2021-05-18 17:18:02) > >> Add sm8350 DISPCC bindings, which are simply a symlink to the sm8250 > >> bindings. Update the documentation with the new compatible. > >> > >> Signed-off-by: Jonathan Marek <jonathan@marek.ca> > >> Reviewed-by: Rob Herring <robh@kernel.org> > >> --- > >> .../devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml | 6 ++++-- > >> include/dt-bindings/clock/qcom,dispcc-sm8350.h | 1 + > > > >> 2 files changed, 5 insertions(+), 2 deletions(-) > >> create mode 120000 include/dt-bindings/clock/qcom,dispcc-sm8350.h > > > > Why the symlink? Can we have the dt authors use the existing header file > > instead? > > > > It would be strange to include bindings with the name of a different > SoC. I guess it is a matter a preference, is there any good reason to > *not* do it like this? $ find include/dt-bindings -type l include/dt-bindings/input/linux-event-codes.h include/dt-bindings/clock/qcom,dispcc-sm8150.h It seems to not be common at all. > > >> > >> diff --git a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml > >> index 0cdf53f41f84..8f414642445e 100644 > >> --- a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml > >> +++ b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml > >> @@ -4,24 +4,26 @@ > >> $id: http://devicetree.org/schemas/clock/qcom,dispcc-sm8x50.yaml# > >> $schema: http://devicetree.org/meta-schemas/core.yaml# > >> > >> -title: Qualcomm Display Clock & Reset Controller Binding for SM8150/SM8250 > >> +title: Qualcomm Display Clock & Reset Controller Binding for SM8150/SM8250/SM8350 > > > > Maybe just "Binding for SM8x50 SoCs" > > > > Its likely these bindings won't be compatible with future "SM8x50" SoCs, > listing supported SoCs explicitly will avoid confusion in the future. The yaml file has sm8x50 in the name. What's the plan there?
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig index 45646b867cdb..cc60e6ee1654 100644 --- a/drivers/clk/qcom/Kconfig +++ b/drivers/clk/qcom/Kconfig @@ -484,11 +484,11 @@ config SDX_GCC_55 SPI, I2C, USB, SD/UFS, PCIe etc. config SM_DISPCC_8250 - tristate "SM8150 and SM8250 Display Clock Controller" + tristate "SM8150/SM8250/SM8350 Display Clock Controller" depends on SM_GCC_8150 || SM_GCC_8250 help Support for the display clock controller on Qualcomm Technologies, Inc - SM8150 and SM8250 devices. + SM8150/SM8250/SM8350 devices. Say Y if you want to support display devices and functionality such as splash screen. diff --git a/drivers/clk/qcom/dispcc-sm8250.c b/drivers/clk/qcom/dispcc-sm8250.c index de09cd5c209f..5f22a715e2f0 100644 --- a/drivers/clk/qcom/dispcc-sm8250.c +++ b/drivers/clk/qcom/dispcc-sm8250.c @@ -36,6 +36,10 @@ static struct pll_vco vco_table[] = { { 249600000, 2000000000, 0 }, }; +static struct pll_vco lucid_5lpe_vco[] = { + { 249600000, 1750000000, 0 }, +}; + static struct alpha_pll_config disp_cc_pll0_config = { .l = 0x47, .alpha = 0xE000, @@ -1039,6 +1043,7 @@ static const struct qcom_cc_desc disp_cc_sm8250_desc = { static const struct of_device_id disp_cc_sm8250_match_table[] = { { .compatible = "qcom,sm8150-dispcc" }, { .compatible = "qcom,sm8250-dispcc" }, + { .compatible = "qcom,sm8350-dispcc" }, { } }; MODULE_DEVICE_TABLE(of, disp_cc_sm8250_match_table); @@ -1051,7 +1056,7 @@ static int disp_cc_sm8250_probe(struct platform_device *pdev) if (IS_ERR(regmap)) return PTR_ERR(regmap); - /* note: trion == lucid, except for the prepare() op */ + /* Apply differences for SM8150 and SM8350 */ BUILD_BUG_ON(CLK_ALPHA_PLL_TYPE_TRION != CLK_ALPHA_PLL_TYPE_LUCID); if (of_device_is_compatible(pdev->dev.of_node, "qcom,sm8150-dispcc")) { disp_cc_pll0_config.config_ctl_hi_val = 0x00002267; @@ -1062,8 +1067,62 @@ static int disp_cc_sm8250_probe(struct platform_device *pdev) disp_cc_pll1_config.config_ctl_hi1_val = 0x00000024; disp_cc_pll1_config.user_ctl_hi1_val = 0x000000D0; disp_cc_pll1_init.ops = &clk_alpha_pll_trion_ops; + } else if (of_device_is_compatible(pdev->dev.of_node, "qcom,sm8350-dispcc")) { + static struct clk_rcg2* const rcgs[] = { + &disp_cc_mdss_byte0_clk_src, + &disp_cc_mdss_byte1_clk_src, + &disp_cc_mdss_dp_aux1_clk_src, + &disp_cc_mdss_dp_aux_clk_src, + &disp_cc_mdss_dp_link1_clk_src, + &disp_cc_mdss_dp_link_clk_src, + &disp_cc_mdss_dp_pixel1_clk_src, + &disp_cc_mdss_dp_pixel2_clk_src, + &disp_cc_mdss_dp_pixel_clk_src, + &disp_cc_mdss_esc0_clk_src, + &disp_cc_mdss_mdp_clk_src, + &disp_cc_mdss_pclk0_clk_src, + &disp_cc_mdss_pclk1_clk_src, + &disp_cc_mdss_rot_clk_src, + &disp_cc_mdss_vsync_clk_src, + }; + static struct clk_regmap_div* const divs[] = { + &disp_cc_mdss_byte0_div_clk_src, + &disp_cc_mdss_byte1_div_clk_src, + &disp_cc_mdss_dp_link1_div_clk_src, + &disp_cc_mdss_dp_link_div_clk_src, + }; + unsigned i; + static bool offset_applied = false; + + /* only apply the offsets once (in case of deferred probe) */ + if (!offset_applied) { + for (i = 0; i < ARRAY_SIZE(rcgs); i++) + rcgs[i]->cmd_rcgr -= 4; + + for (i = 0; i < ARRAY_SIZE(divs); i++) { + divs[i]->reg -= 4; + divs[i]->width = 4; + } + + disp_cc_mdss_ahb_clk.halt_reg -= 4; + disp_cc_mdss_ahb_clk.clkr.enable_reg -= 4; + + offset_applied = true; + } + + disp_cc_mdss_ahb_clk_src.cmd_rcgr = 0x22a0; + + disp_cc_pll0_config.config_ctl_hi1_val = 0x2A9A699C; + disp_cc_pll0_config.test_ctl_hi1_val = 0x01800000; + disp_cc_pll0_init.ops = &clk_alpha_pll_lucid_5lpe_ops; + disp_cc_pll0.vco_table = lucid_5lpe_vco; + disp_cc_pll1_config.config_ctl_hi1_val = 0x2A9A699C; + disp_cc_pll1_config.test_ctl_hi1_val = 0x01800000; + disp_cc_pll1_init.ops = &clk_alpha_pll_lucid_5lpe_ops; + disp_cc_pll1.vco_table = lucid_5lpe_vco; } + /* note for SM8350: downstream lucid_5lpe configure differs slightly */ clk_lucid_pll_configure(&disp_cc_pll0, regmap, &disp_cc_pll0_config); clk_lucid_pll_configure(&disp_cc_pll1, regmap, &disp_cc_pll1_config);
Add support to the SM8350 display clock controller by extending the SM8250 display clock controller, which is almost identical but has some minor differences. Signed-off-by: Jonathan Marek <jonathan@marek.ca> --- v2: improved diff (don't move sm8150 case around), update comment drivers/clk/qcom/Kconfig | 4 +-- drivers/clk/qcom/dispcc-sm8250.c | 61 +++++++++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 3 deletions(-)