| Message ID | 20240322-topic-sm8x50-upstream-pcie-1-phy-aux-clk-v2-3-3ec0a966d52f@linaro.org |
|---|---|
| State | Accepted |
| Commit | 583ca9ccfa806605ae1391aafa3f78a8a2cc0b48 |
| Headers | show |
| Series | arm64: qcom-sm8[456]50: properly describe the PCIe Gen4x2 PHY AUX clock | expand |
On Fri, 22 Mar 2024 at 11:43, Neil Armstrong <neil.armstrong@linaro.org> wrote: > > The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock, > add the code to register it for PHYs configs that sets a aux_clock_rate. > > In order to get the right clock, add qmp_pcie_clk_hw_get() which uses > the newly introduced QMP_PCIE_PIPE_CLK & QMP_PCIE_PHY_AUX_CLK clock > IDs and also supports the legacy bindings by returning the PIPE clock > when #clock-cells=0. > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Small question below. > --- > drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 78 ++++++++++++++++++++++++++++++-- > 1 file changed, 75 insertions(+), 3 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > index e8da2e9146dc..6c9a95e62429 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > @@ -22,6 +22,8 @@ > #include <linux/reset.h> > #include <linux/slab.h> > > +#include <dt-bindings/phy/phy-qcom-qmp.h> > + > #include "phy-qcom-qmp-common.h" > > #include "phy-qcom-qmp.h" > @@ -2389,6 +2391,9 @@ struct qmp_phy_cfg { > > /* QMP PHY pipe clock interface rate */ > unsigned long pipe_clock_rate; > + > + /* QMP PHY AUX clock interface rate */ > + unsigned long aux_clock_rate; > }; > > struct qmp_pcie { > @@ -2420,6 +2425,7 @@ struct qmp_pcie { > int mode; > > struct clk_fixed_rate pipe_clk_fixed; > + struct clk_fixed_rate aux_clk_fixed; > }; > > static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val) > @@ -3686,6 +3692,62 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np) > return devm_clk_hw_register(qmp->dev, &fixed->hw); > } > > +/* > + * Register a fixed rate PHY aux clock. > + * > + * The <s>_phy_aux_clksrc generated by PHY goes to the GCC that gate > + * controls it. The <s>_phy_aux_clk coming out of the GCC is requested > + * by the PHY driver for its operations. > + * We register the <s>_phy_aux_clksrc here. The gcc driver takes care > + * of assigning this <s>_phy_aux_clksrc as parent to <s>_phy_aux_clk. > + * Below picture shows this relationship. > + * > + * +---------------+ > + * | PHY block |<<---------------------------------------------+ > + * | | | > + * | +-------+ | +-----+ | > + * I/P---^-->| PLL |---^--->phy_aux_clksrc--->| GCC |--->phy_aux_clk---+ > + * clk | +-------+ | +-----+ > + * +---------------+ > + */ > +static int phy_aux_clk_register(struct qmp_pcie *qmp, struct device_node *np) > +{ > + struct clk_fixed_rate *fixed = &qmp->aux_clk_fixed; > + struct clk_init_data init = { }; > + int ret; > + > + ret = of_property_read_string_index(np, "clock-output-names", 1, &init.name); > + if (ret) { > + dev_err(qmp->dev, "%pOFn: No clock-output-names index 1\n", np); > + return ret; > + } > + > + init.ops = &clk_fixed_rate_ops; > + > + fixed->fixed_rate = qmp->cfg->aux_clock_rate; > + fixed->hw.init = &init; > + > + return devm_clk_hw_register(qmp->dev, &fixed->hw); > +} > + > +static struct clk_hw *qmp_pcie_clk_hw_get(struct of_phandle_args *clkspec, void *data) > +{ > + struct qmp_pcie *qmp = data; > + > + /* Support legacy bindings */ > + if (!clkspec->args_count) > + return &qmp->pipe_clk_fixed.hw; > + > + switch (clkspec->args[0]) { > + case QMP_PCIE_PIPE_CLK: > + return &qmp->pipe_clk_fixed.hw; > + case QMP_PCIE_PHY_AUX_CLK: > + return &qmp->aux_clk_fixed.hw; Does the absence of the default case trigger a warning if compiled with W=1? > + } > + > + return ERR_PTR(-EINVAL); > +} > + > static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np) > { > int ret; > @@ -3694,9 +3756,19 @@ static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np > if (ret) > return ret; > > - ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &qmp->pipe_clk_fixed.hw); > - if (ret) > - return ret; > + if (qmp->cfg->aux_clock_rate) { > + ret = phy_aux_clk_register(qmp, np); > + if (ret) > + return ret; > + > + ret = of_clk_add_hw_provider(np, qmp_pcie_clk_hw_get, qmp); > + if (ret) > + return ret; > + } else { > + ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &qmp->pipe_clk_fixed.hw); > + if (ret) > + return ret; > + } > > /* > * Roll a devm action because the clock provider is the child node, but > > -- > 2.34.1 > >
On 22/03/2024 11:41, Dmitry Baryshkov wrote: > On Fri, 22 Mar 2024 at 11:43, Neil Armstrong <neil.armstrong@linaro.org> wrote: >> >> The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock, >> add the code to register it for PHYs configs that sets a aux_clock_rate. >> >> In order to get the right clock, add qmp_pcie_clk_hw_get() which uses >> the newly introduced QMP_PCIE_PIPE_CLK & QMP_PCIE_PHY_AUX_CLK clock >> IDs and also supports the legacy bindings by returning the PIPE clock >> when #clock-cells=0. >> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > Small question below. > >> --- >> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 78 ++++++++++++++++++++++++++++++-- >> 1 file changed, 75 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c >> index e8da2e9146dc..6c9a95e62429 100644 >> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c >> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c >> @@ -22,6 +22,8 @@ >> #include <linux/reset.h> >> #include <linux/slab.h> >> >> +#include <dt-bindings/phy/phy-qcom-qmp.h> >> + >> #include "phy-qcom-qmp-common.h" >> >> #include "phy-qcom-qmp.h" >> @@ -2389,6 +2391,9 @@ struct qmp_phy_cfg { >> >> /* QMP PHY pipe clock interface rate */ >> unsigned long pipe_clock_rate; >> + >> + /* QMP PHY AUX clock interface rate */ >> + unsigned long aux_clock_rate; >> }; >> >> struct qmp_pcie { >> @@ -2420,6 +2425,7 @@ struct qmp_pcie { >> int mode; >> >> struct clk_fixed_rate pipe_clk_fixed; >> + struct clk_fixed_rate aux_clk_fixed; >> }; >> >> static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val) >> @@ -3686,6 +3692,62 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np) >> return devm_clk_hw_register(qmp->dev, &fixed->hw); >> } >> >> +/* >> + * Register a fixed rate PHY aux clock. >> + * >> + * The <s>_phy_aux_clksrc generated by PHY goes to the GCC that gate >> + * controls it. The <s>_phy_aux_clk coming out of the GCC is requested >> + * by the PHY driver for its operations. >> + * We register the <s>_phy_aux_clksrc here. The gcc driver takes care >> + * of assigning this <s>_phy_aux_clksrc as parent to <s>_phy_aux_clk. >> + * Below picture shows this relationship. >> + * >> + * +---------------+ >> + * | PHY block |<<---------------------------------------------+ >> + * | | | >> + * | +-------+ | +-----+ | >> + * I/P---^-->| PLL |---^--->phy_aux_clksrc--->| GCC |--->phy_aux_clk---+ >> + * clk | +-------+ | +-----+ >> + * +---------------+ >> + */ >> +static int phy_aux_clk_register(struct qmp_pcie *qmp, struct device_node *np) >> +{ >> + struct clk_fixed_rate *fixed = &qmp->aux_clk_fixed; >> + struct clk_init_data init = { }; >> + int ret; >> + >> + ret = of_property_read_string_index(np, "clock-output-names", 1, &init.name); >> + if (ret) { >> + dev_err(qmp->dev, "%pOFn: No clock-output-names index 1\n", np); >> + return ret; >> + } >> + >> + init.ops = &clk_fixed_rate_ops; >> + >> + fixed->fixed_rate = qmp->cfg->aux_clock_rate; >> + fixed->hw.init = &init; >> + >> + return devm_clk_hw_register(qmp->dev, &fixed->hw); >> +} >> + >> +static struct clk_hw *qmp_pcie_clk_hw_get(struct of_phandle_args *clkspec, void *data) >> +{ >> + struct qmp_pcie *qmp = data; >> + >> + /* Support legacy bindings */ >> + if (!clkspec->args_count) >> + return &qmp->pipe_clk_fixed.hw; >> + >> + switch (clkspec->args[0]) { >> + case QMP_PCIE_PIPE_CLK: >> + return &qmp->pipe_clk_fixed.hw; >> + case QMP_PCIE_PHY_AUX_CLK: >> + return &qmp->aux_clk_fixed.hw; > > Does the absence of the default case trigger a warning if compiled with W=1? Nop it doesn't with GCC arm-gnu-toolchain-13.2.Rel1-x86_64-aarch64-none-linux-gnu + W=1 and with smatch and C=1 Neil > >> + } >> + >> + return ERR_PTR(-EINVAL); >> +} >> + >> static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np) >> { >> int ret; >> @@ -3694,9 +3756,19 @@ static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np >> if (ret) >> return ret; >> >> - ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &qmp->pipe_clk_fixed.hw); >> - if (ret) >> - return ret; >> + if (qmp->cfg->aux_clock_rate) { >> + ret = phy_aux_clk_register(qmp, np); >> + if (ret) >> + return ret; >> + >> + ret = of_clk_add_hw_provider(np, qmp_pcie_clk_hw_get, qmp); >> + if (ret) >> + return ret; >> + } else { >> + ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &qmp->pipe_clk_fixed.hw); >> + if (ret) >> + return ret; >> + } >> >> /* >> * Roll a devm action because the clock provider is the child node, but >> >> -- >> 2.34.1 >> >> > >
On Fri, 22 Mar 2024 at 12:45, Neil Armstrong <neil.armstrong@linaro.org> wrote: > > On 22/03/2024 11:41, Dmitry Baryshkov wrote: > > On Fri, 22 Mar 2024 at 11:43, Neil Armstrong <neil.armstrong@linaro.org> wrote: > >> > >> The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock, > >> add the code to register it for PHYs configs that sets a aux_clock_rate. > >> > >> In order to get the right clock, add qmp_pcie_clk_hw_get() which uses > >> the newly introduced QMP_PCIE_PIPE_CLK & QMP_PCIE_PHY_AUX_CLK clock > >> IDs and also supports the legacy bindings by returning the PIPE clock > >> when #clock-cells=0. > >> > >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > Small question below. > > > >> --- > >> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 78 ++++++++++++++++++++++++++++++-- > >> 1 file changed, 75 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > >> index e8da2e9146dc..6c9a95e62429 100644 > >> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > >> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > >> @@ -22,6 +22,8 @@ > >> #include <linux/reset.h> > >> #include <linux/slab.h> > >> > >> +#include <dt-bindings/phy/phy-qcom-qmp.h> > >> + > >> #include "phy-qcom-qmp-common.h" > >> > >> #include "phy-qcom-qmp.h" > >> @@ -2389,6 +2391,9 @@ struct qmp_phy_cfg { > >> > >> /* QMP PHY pipe clock interface rate */ > >> unsigned long pipe_clock_rate; > >> + > >> + /* QMP PHY AUX clock interface rate */ > >> + unsigned long aux_clock_rate; > >> }; > >> > >> struct qmp_pcie { > >> @@ -2420,6 +2425,7 @@ struct qmp_pcie { > >> int mode; > >> > >> struct clk_fixed_rate pipe_clk_fixed; > >> + struct clk_fixed_rate aux_clk_fixed; > >> }; > >> > >> static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val) > >> @@ -3686,6 +3692,62 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np) > >> return devm_clk_hw_register(qmp->dev, &fixed->hw); > >> } > >> > >> +/* > >> + * Register a fixed rate PHY aux clock. > >> + * > >> + * The <s>_phy_aux_clksrc generated by PHY goes to the GCC that gate > >> + * controls it. The <s>_phy_aux_clk coming out of the GCC is requested > >> + * by the PHY driver for its operations. > >> + * We register the <s>_phy_aux_clksrc here. The gcc driver takes care > >> + * of assigning this <s>_phy_aux_clksrc as parent to <s>_phy_aux_clk. > >> + * Below picture shows this relationship. > >> + * > >> + * +---------------+ > >> + * | PHY block |<<---------------------------------------------+ > >> + * | | | > >> + * | +-------+ | +-----+ | > >> + * I/P---^-->| PLL |---^--->phy_aux_clksrc--->| GCC |--->phy_aux_clk---+ > >> + * clk | +-------+ | +-----+ > >> + * +---------------+ > >> + */ > >> +static int phy_aux_clk_register(struct qmp_pcie *qmp, struct device_node *np) > >> +{ > >> + struct clk_fixed_rate *fixed = &qmp->aux_clk_fixed; > >> + struct clk_init_data init = { }; > >> + int ret; > >> + > >> + ret = of_property_read_string_index(np, "clock-output-names", 1, &init.name); > >> + if (ret) { > >> + dev_err(qmp->dev, "%pOFn: No clock-output-names index 1\n", np); > >> + return ret; > >> + } > >> + > >> + init.ops = &clk_fixed_rate_ops; > >> + > >> + fixed->fixed_rate = qmp->cfg->aux_clock_rate; > >> + fixed->hw.init = &init; > >> + > >> + return devm_clk_hw_register(qmp->dev, &fixed->hw); > >> +} > >> + > >> +static struct clk_hw *qmp_pcie_clk_hw_get(struct of_phandle_args *clkspec, void *data) > >> +{ > >> + struct qmp_pcie *qmp = data; > >> + > >> + /* Support legacy bindings */ > >> + if (!clkspec->args_count) > >> + return &qmp->pipe_clk_fixed.hw; > >> + > >> + switch (clkspec->args[0]) { > >> + case QMP_PCIE_PIPE_CLK: > >> + return &qmp->pipe_clk_fixed.hw; > >> + case QMP_PCIE_PHY_AUX_CLK: > >> + return &qmp->aux_clk_fixed.hw; > > > > Does the absence of the default case trigger a warning if compiled with W=1? > > Nop it doesn't with GCC arm-gnu-toolchain-13.2.Rel1-x86_64-aarch64-none-linux-gnu + W=1 and with smatch and C=1 Ok, great! > > Neil > > > > >> + } > >> + > >> + return ERR_PTR(-EINVAL); > >> +} > >> + > >> static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np) > >> { > >> int ret; > >> @@ -3694,9 +3756,19 @@ static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np > >> if (ret) > >> return ret; > >> > >> - ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &qmp->pipe_clk_fixed.hw); > >> - if (ret) > >> - return ret; > >> + if (qmp->cfg->aux_clock_rate) { > >> + ret = phy_aux_clk_register(qmp, np); > >> + if (ret) > >> + return ret; > >> + > >> + ret = of_clk_add_hw_provider(np, qmp_pcie_clk_hw_get, qmp); > >> + if (ret) > >> + return ret; > >> + } else { > >> + ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &qmp->pipe_clk_fixed.hw); > >> + if (ret) > >> + return ret; > >> + } > >> > >> /* > >> * Roll a devm action because the clock provider is the child node, but > >> > >> -- > >> 2.34.1 > >> > >> > > > > >
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c index e8da2e9146dc..6c9a95e62429 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c @@ -22,6 +22,8 @@ #include <linux/reset.h> #include <linux/slab.h> +#include <dt-bindings/phy/phy-qcom-qmp.h> + #include "phy-qcom-qmp-common.h" #include "phy-qcom-qmp.h" @@ -2389,6 +2391,9 @@ struct qmp_phy_cfg { /* QMP PHY pipe clock interface rate */ unsigned long pipe_clock_rate; + + /* QMP PHY AUX clock interface rate */ + unsigned long aux_clock_rate; }; struct qmp_pcie { @@ -2420,6 +2425,7 @@ struct qmp_pcie { int mode; struct clk_fixed_rate pipe_clk_fixed; + struct clk_fixed_rate aux_clk_fixed; }; static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val) @@ -3686,6 +3692,62 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np) return devm_clk_hw_register(qmp->dev, &fixed->hw); } +/* + * Register a fixed rate PHY aux clock. + * + * The <s>_phy_aux_clksrc generated by PHY goes to the GCC that gate + * controls it. The <s>_phy_aux_clk coming out of the GCC is requested + * by the PHY driver for its operations. + * We register the <s>_phy_aux_clksrc here. The gcc driver takes care + * of assigning this <s>_phy_aux_clksrc as parent to <s>_phy_aux_clk. + * Below picture shows this relationship. + * + * +---------------+ + * | PHY block |<<---------------------------------------------+ + * | | | + * | +-------+ | +-----+ | + * I/P---^-->| PLL |---^--->phy_aux_clksrc--->| GCC |--->phy_aux_clk---+ + * clk | +-------+ | +-----+ + * +---------------+ + */ +static int phy_aux_clk_register(struct qmp_pcie *qmp, struct device_node *np) +{ + struct clk_fixed_rate *fixed = &qmp->aux_clk_fixed; + struct clk_init_data init = { }; + int ret; + + ret = of_property_read_string_index(np, "clock-output-names", 1, &init.name); + if (ret) { + dev_err(qmp->dev, "%pOFn: No clock-output-names index 1\n", np); + return ret; + } + + init.ops = &clk_fixed_rate_ops; + + fixed->fixed_rate = qmp->cfg->aux_clock_rate; + fixed->hw.init = &init; + + return devm_clk_hw_register(qmp->dev, &fixed->hw); +} + +static struct clk_hw *qmp_pcie_clk_hw_get(struct of_phandle_args *clkspec, void *data) +{ + struct qmp_pcie *qmp = data; + + /* Support legacy bindings */ + if (!clkspec->args_count) + return &qmp->pipe_clk_fixed.hw; + + switch (clkspec->args[0]) { + case QMP_PCIE_PIPE_CLK: + return &qmp->pipe_clk_fixed.hw; + case QMP_PCIE_PHY_AUX_CLK: + return &qmp->aux_clk_fixed.hw; + } + + return ERR_PTR(-EINVAL); +} + static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np) { int ret; @@ -3694,9 +3756,19 @@ static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np if (ret) return ret; - ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &qmp->pipe_clk_fixed.hw); - if (ret) - return ret; + if (qmp->cfg->aux_clock_rate) { + ret = phy_aux_clk_register(qmp, np); + if (ret) + return ret; + + ret = of_clk_add_hw_provider(np, qmp_pcie_clk_hw_get, qmp); + if (ret) + return ret; + } else { + ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &qmp->pipe_clk_fixed.hw); + if (ret) + return ret; + } /* * Roll a devm action because the clock provider is the child node, but
The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock, add the code to register it for PHYs configs that sets a aux_clock_rate. In order to get the right clock, add qmp_pcie_clk_hw_get() which uses the newly introduced QMP_PCIE_PIPE_CLK & QMP_PCIE_PHY_AUX_CLK clock IDs and also supports the legacy bindings by returning the PIPE clock when #clock-cells=0. Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> --- drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 78 ++++++++++++++++++++++++++++++-- 1 file changed, 75 insertions(+), 3 deletions(-)