| Message ID | 20220621161126.15883-1-quic_srichara@quicinc.com |
|---|---|
| Headers | show |
| Series | Add minimal boot support for IPQ5018 | expand |
On 21/06/2022 18:11, Sricharan R wrote: > From: Varadarajan Narayanan <quic_varada@quicinc.com> > > Document the new ipq5018 SOC/board device tree bindings. > > Co-developed-by: Sricharan R <quic_srichara@quicinc.com> > Signed-off-by: Sricharan R <quic_srichara@quicinc.com> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> Your chain is wrong. Since you send it, your SoB is the last. We have nice example for this: https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L473 Best regards, Krzysztof
On 6/22/2022 8:38 PM, Krzysztof Kozlowski wrote: > On 21/06/2022 18:11, Sricharan R wrote: >> From: Varadarajan Narayanan <quic_varada@quicinc.com> >> >> Add support for the global clock controller found on IPQ5018 >> based devices. >> >> Co-developed-by: Sricharan R <quic_srichara@quicinc.com> >> Signed-off-by: Sricharan R <quic_srichara@quicinc.com> >> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > Thank you for your patch. There is something to discuss/improve. > >> --- >> drivers/clk/qcom/Kconfig | 7 + >> drivers/clk/qcom/Makefile | 1 + >> drivers/clk/qcom/gcc-ipq5018.c | 3995 ++++++++++++++++++++++++++++++++ >> 3 files changed, 4003 insertions(+) >> create mode 100644 drivers/clk/qcom/gcc-ipq5018.c >> >> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig >> index d01436be6d7a..294fb975db85 100644 >> --- a/drivers/clk/qcom/Kconfig >> +++ b/drivers/clk/qcom/Kconfig >> @@ -172,6 +172,13 @@ config IPQ_GCC_8074 >> i2c, USB, SD/eMMC, etc. Select this for the root clock >> of ipq8074. >> >> +config IPQ_GCC_5018 >> + tristate "IPQ5018 Global Clock Controller" >> + help >> + Support for global clock controller on ipq5018 devices. >> + Say Y if you want to use peripheral devices such as UART, SPI, >> + i2c, USB, SD/eMMC, etc. >> + >> config MSM_GCC_8660 >> tristate "MSM8660 Global Clock Controller" >> help >> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile >> index 671cf5821af1..33ab4ce9b863 100644 >> --- a/drivers/clk/qcom/Makefile >> +++ b/drivers/clk/qcom/Makefile >> @@ -26,6 +26,7 @@ obj-$(CONFIG_IPQ_GCC_4019) += gcc-ipq4019.o >> obj-$(CONFIG_IPQ_GCC_6018) += gcc-ipq6018.o >> obj-$(CONFIG_IPQ_GCC_806X) += gcc-ipq806x.o >> obj-$(CONFIG_IPQ_GCC_8074) += gcc-ipq8074.o >> +obj-$(CONFIG_IPQ_GCC_5018) += gcc-ipq5018.o >> obj-$(CONFIG_IPQ_LCC_806X) += lcc-ipq806x.o >> obj-$(CONFIG_MDM_GCC_9607) += gcc-mdm9607.o >> obj-$(CONFIG_MDM_GCC_9615) += gcc-mdm9615.o > (...) > >> + >> +static int gcc_ipq5018_probe(struct platform_device *pdev) >> +{ >> + int ret; >> + struct regmap *regmap; >> + struct qcom_cc_desc ipq5018_desc = gcc_ipq5018_desc; >> + >> + regmap = qcom_cc_map(pdev, &ipq5018_desc); >> + if (IS_ERR(regmap)) >> + return PTR_ERR(regmap); >> + >> + clk_alpha_pll_configure(&ubi32_pll_main, regmap, &ubi32_pll_config); >> + >> + ret = qcom_cc_really_probe(pdev, &ipq5018_desc, regmap); > return qcom_cc_really_probe(....) ok. >> + if (ret) { >> + dev_err(&pdev->dev, "Failed to register ipq5018 GCC clocks\n"); >> + return ret; >> + } >> + >> + dev_info(&pdev->dev, "Registered ipq5018 GCC clocks provider"); > No probe success messages. This pollutes the log and there is other > infrastructure to check for successful probe. ok. >> + >> + return ret; >> +} >> + >> +static struct platform_driver gcc_ipq5018_driver = { >> + .probe = gcc_ipq5018_probe, >> + .driver = { >> + .name = "qcom,gcc-ipq5018", >> + .owner = THIS_MODULE, > No need for owner. ok. Regards, Sricharan
On 6/22/2022 8:44 PM, Krzysztof Kozlowski wrote: > On 21/06/2022 18:11, Sricharan R wrote: >> From: Varadarajan Narayanan <quic_varada@quicinc.com> >> >> Document the new ipq5018 SOC/board device tree bindings. >> >> Co-developed-by: Sricharan R <quic_srichara@quicinc.com> >> Signed-off-by: Sricharan R <quic_srichara@quicinc.com> >> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > Your chain is wrong. Since you send it, your SoB is the last. We have > nice example for this: > > https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L473 > sure, will fix it. Regards, Sricharan
On Tue 21 Jun 11:11 CDT 2022, Sricharan R wrote: > diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c [..] > +static const struct clk_parent_data gcc_xo_gpll0_gpll0_out_main_div2[] = { > + { .fw_name = "xo", .name = "xo", }, Please replace .fw_name with .index based lookup, in line with what was done in gcc-sc8280xp.c recently. There's no reason to include global name lookup (.name) in new drivers, so please omit this part. > + { .fw_name = "gpll0", .name = "gpll0", }, > + { .fw_name = "gpll0_out_main_div2", .name = "gpll0_out_main_div2", }, > +}; > + [..] > +static struct clk_alpha_pll gpll0_main = { > + .offset = 0x21000, > + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT], > + .clkr = { > + .enable_reg = 0x0b000, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gpll0_main", > + .parent_data = &(const struct clk_parent_data){ > + .fw_name = "xo", > + .name = "xo", Are you referring to the board XO here, or the CXO pin on the SoC? On many platforms these are not the same... Please omit the .name here as well and as this is used a few times, please create a struct clk_parent_data for this parent. > + }, > + .num_parents = 1, > + .ops = &clk_alpha_pll_stromer_ops, > + .flags = CLK_IS_CRITICAL, > + }, > + }, > +}; > + > +static struct clk_fixed_factor gpll0_out_main_div2 = { > + .mult = 1, > + .div = 2, > + .hw.init = &(struct clk_init_data){ > + .name = "gpll0_out_main_div2", > + .parent_data = &(const struct clk_parent_data){ It would be nice to have a space inbetween ) and { in all these. > + .fw_name = "gpll0_main", > + .name = "gpll0_main", > + }, > + .num_parents = 1, > + .ops = &clk_fixed_factor_ops, > + .flags = CLK_SET_RATE_PARENT, > + }, > +}; [..] > +static struct clk_branch gcc_gephy_tx_clk = { > + .halt_reg = 0x56014, > + .halt_check = BRANCH_HALT_DELAY, > + .clkr = { > + .enable_reg = 0x56014, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gcc_gephy_tx_clk", > + .parent_data = &(const struct clk_parent_data){ > + .fw_name = "gmac0_tx_div_clk_src", > + .name = "gmac0_tx_div_clk_src", > + }, This parent_data is repeated multiple times, but more importantly it's not an external clock, so you should use .parent_hw instead of .parent_data. Please review the parent for all your clocks. Regards, Bjorn
On 6/24/2022 9:35 AM, Bjorn Andersson wrote: > On Tue 21 Jun 11:11 CDT 2022, Sricharan R wrote: >> diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c > [..] >> +static const struct clk_parent_data gcc_xo_gpll0_gpll0_out_main_div2[] = { >> + { .fw_name = "xo", .name = "xo", }, > Please replace .fw_name with .index based lookup, in line with what was > done in gcc-sc8280xp.c recently. Sure, understand will fix it. > > There's no reason to include global name lookup (.name) in new drivers, > so please omit this part. ok. >> + { .fw_name = "gpll0", .name = "gpll0", }, >> + { .fw_name = "gpll0_out_main_div2", .name = "gpll0_out_main_div2", }, >> +}; >> + > [..] >> +static struct clk_alpha_pll gpll0_main = { >> + .offset = 0x21000, >> + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT], >> + .clkr = { >> + .enable_reg = 0x0b000, >> + .enable_mask = BIT(0), >> + .hw.init = &(struct clk_init_data){ >> + .name = "gpll0_main", >> + .parent_data = &(const struct clk_parent_data){ >> + .fw_name = "xo", >> + .name = "xo", > Are you referring to the board XO here, or the CXO pin on the SoC? On > many platforms these are not the same... board XO, will refer your above example and fix it here as well > Please omit the .name here as well and as this is used a few times, > please create a struct clk_parent_data for this parent. ok. >> + }, >> + .num_parents = 1, >> + .ops = &clk_alpha_pll_stromer_ops, >> + .flags = CLK_IS_CRITICAL, >> + }, >> + }, >> +}; >> + >> +static struct clk_fixed_factor gpll0_out_main_div2 = { >> + .mult = 1, >> + .div = 2, >> + .hw.init = &(struct clk_init_data){ >> + .name = "gpll0_out_main_div2", >> + .parent_data = &(const struct clk_parent_data){ > It would be nice to have a space inbetween ) and { in all these. ok. >> + .fw_name = "gpll0_main", >> + .name = "gpll0_main", >> + }, >> + .num_parents = 1, >> + .ops = &clk_fixed_factor_ops, >> + .flags = CLK_SET_RATE_PARENT, >> + }, >> +}; > [..] >> +static struct clk_branch gcc_gephy_tx_clk = { >> + .halt_reg = 0x56014, >> + .halt_check = BRANCH_HALT_DELAY, >> + .clkr = { >> + .enable_reg = 0x56014, >> + .enable_mask = BIT(0), >> + .hw.init = &(struct clk_init_data){ >> + .name = "gcc_gephy_tx_clk", >> + .parent_data = &(const struct clk_parent_data){ >> + .fw_name = "gmac0_tx_div_clk_src", >> + .name = "gmac0_tx_div_clk_src", >> + }, > This parent_data is repeated multiple times, but more importantly it's > not an external clock, so you should use .parent_hw instead of > .parent_data. > > Please review the parent for all your clocks. ok, will do. Regards, Sricharan
On Tue, Jun 21, 2022 at 6:11 PM Sricharan R <quic_srichara@quicinc.com> wrote: > The IPQ5018 is Qualcomm's 802.11ax SoC for Routers, > Gateways and Access Points. > > This series adds minimal board boot support for ipq5018-mp03.1-c2 board. Pretty cool! > dt-bindings: pinctrl: qcom: Add ipq5018 pinctrl bindings > pinctrl: qcom: Add IPQ5018 pinctrl driver I'm happy to merge the two pinctrl patches separately to the pinctrl tree if I can get a review from Bjorn or Krzysztof. Yours, Linus Walleij
Hi Linus, On 6/28/2022 6:25 PM, Linus Walleij wrote: > On Tue, Jun 21, 2022 at 6:11 PM Sricharan R <quic_srichara@quicinc.com> wrote: > >> The IPQ5018 is Qualcomm's 802.11ax SoC for Routers, >> Gateways and Access Points. >> >> This series adds minimal board boot support for ipq5018-mp03.1-c2 board. > Pretty cool! > >> dt-bindings: pinctrl: qcom: Add ipq5018 pinctrl bindings >> pinctrl: qcom: Add IPQ5018 pinctrl driver > I'm happy to merge the two pinctrl patches separately to the pinctrl > tree if I can get a review from Bjorn or Krzysztof. Thanks. Have few review comments from Bjorn and Krzysztof on the bindings. Will post V3 and hopefully that should have their acks on that. Regards, Sricharan
The IPQ5018 is Qualcomm's 802.11ax SoC for Routers, Gateways and Access Points. This series adds minimal board boot support for ipq5018-mp03.1-c2 board. [v2] Fixed all comments and rebased for TOT. Varadarajan Narayanan (8): clk: qcom: clk-alpha-pll: Add support for Stromer PLLs dt-bindings: arm64: ipq5018: Add binding descriptions for clock and reset clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018 dt-bindings: pinctrl: qcom: Add ipq5018 pinctrl bindings pinctrl: qcom: Add IPQ5018 pinctrl driver dt-bindings: qcom: Add ipq5018 bindings arm64: dts: Add ipq5018 SoC and MP03 board support arm64: defconfig: Enable IPQ5018 SoC base configs .../devicetree/bindings/arm/qcom.yaml | 7 + .../bindings/clock/qcom,gcc-other.yaml | 3 + .../pinctrl/qcom,ipq5018-pinctrl.yaml | 145 + arch/arm64/boot/dts/qcom/Makefile | 1 + .../arm64/boot/dts/qcom/ipq5018-mp03.1-c2.dts | 29 + arch/arm64/boot/dts/qcom/ipq5018.dtsi | 221 + arch/arm64/configs/defconfig | 3 + drivers/clk/qcom/Kconfig | 7 + drivers/clk/qcom/Makefile | 1 + drivers/clk/qcom/clk-alpha-pll.c | 100 +- drivers/clk/qcom/clk-alpha-pll.h | 7 +- drivers/clk/qcom/gcc-ipq5018.c | 3995 +++++++++++++++++ drivers/pinctrl/qcom/Kconfig | 10 + drivers/pinctrl/qcom/Makefile | 1 + drivers/pinctrl/qcom/pinctrl-ipq5018.c | 791 ++++ include/dt-bindings/clock/qcom,gcc-ipq5018.h | 188 + include/dt-bindings/reset/qcom,gcc-ipq5018.h | 122 + 17 files changed, 5629 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,ipq5018-pinctrl.yaml create mode 100644 arch/arm64/boot/dts/qcom/ipq5018-mp03.1-c2.dts create mode 100644 arch/arm64/boot/dts/qcom/ipq5018.dtsi create mode 100644 drivers/clk/qcom/gcc-ipq5018.c create mode 100644 drivers/pinctrl/qcom/pinctrl-ipq5018.c create mode 100644 include/dt-bindings/clock/qcom,gcc-ipq5018.h create mode 100644 include/dt-bindings/reset/qcom,gcc-ipq5018.h