| Message ID | 20240319-topic-sm8x50-upstream-pcie-1-phy-aux-clk-v1-3-926d7a4ccd80@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: qcom-sm8[456]50: properly describe the PCIe Gen4x2 PHY AUX clock | expand |
On Tue, 19 Mar 2024 at 12:45, 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. > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++ > 1 file changed, 70 insertions(+) > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > index 079b3e306489..2d05226ae200 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) > @@ -3681,6 +3687,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); > +} Can we use of_clk_hw_onecell_get() instead? I think it even should be possible to use onecell for both cases, it will look at the first arg, which will be 0 in case of #clock-cells equal to 0. > + > static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np) > { > int ret; > @@ -3689,6 +3751,14 @@ static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np > if (ret) > return ret; > > + if (qmp->cfg->aux_clock_rate) { > + ret = phy_aux_clk_register(qmp, np); > + if (ret) > + return ret; > + > + return devm_of_clk_add_hw_provider(qmp->dev, qmp_pcie_clk_hw_get, qmp); > + } > + > return devm_of_clk_add_hw_provider(qmp->dev, of_clk_hw_simple_get, > &qmp->pipe_clk_fixed.hw); > } > > -- > 2.34.1 > >
On 19/03/2024 11:55, Dmitry Baryshkov wrote: > On Tue, 19 Mar 2024 at 12:45, 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. >> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++ >> 1 file changed, 70 insertions(+) >> >> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c >> index 079b3e306489..2d05226ae200 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) >> @@ -3681,6 +3687,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); >> +} > > Can we use of_clk_hw_onecell_get() instead? I think it even should be > possible to use onecell for both cases, it will look at the first arg, > which will be 0 in case of #clock-cells equal to 0. Let me investigate if it's possible > >> + >> static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np) >> { >> int ret; >> @@ -3689,6 +3751,14 @@ static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np >> if (ret) >> return ret; >> >> + if (qmp->cfg->aux_clock_rate) { >> + ret = phy_aux_clk_register(qmp, np); >> + if (ret) >> + return ret; >> + >> + return devm_of_clk_add_hw_provider(qmp->dev, qmp_pcie_clk_hw_get, qmp); >> + } >> + >> return devm_of_clk_add_hw_provider(qmp->dev, of_clk_hw_simple_get, >> &qmp->pipe_clk_fixed.hw); >> } >> >> -- >> 2.34.1 >> >> > >
On 19/03/2024 11:59, Neil Armstrong wrote: > On 19/03/2024 11:55, Dmitry Baryshkov wrote: >> On Tue, 19 Mar 2024 at 12:45, 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. >>> >>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>> --- >>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++ >>> 1 file changed, 70 insertions(+) >>> >>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c >>> index 079b3e306489..2d05226ae200 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) >>> @@ -3681,6 +3687,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); >>> +} >> >> Can we use of_clk_hw_onecell_get() instead? I think it even should be >> possible to use onecell for both cases, it will look at the first arg, >> which will be 0 in case of #clock-cells equal to 0. > > Let me investigate if it's possible Ok, it would work but it would require building a clk_hw_onecell_data a runtime, while we could simply provide this qmp_pcie_clk_hw_get() and avoid runtime 2 allocations. I'm not sure it's worth it. Neil > >> >>> + >>> static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np) >>> { >>> int ret; >>> @@ -3689,6 +3751,14 @@ static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np >>> if (ret) >>> return ret; >>> >>> + if (qmp->cfg->aux_clock_rate) { >>> + ret = phy_aux_clk_register(qmp, np); >>> + if (ret) >>> + return ret; >>> + >>> + return devm_of_clk_add_hw_provider(qmp->dev, qmp_pcie_clk_hw_get, qmp); >>> + } >>> + >>> return devm_of_clk_add_hw_provider(qmp->dev, of_clk_hw_simple_get, >>> &qmp->pipe_clk_fixed.hw); >>> } >>> >>> -- >>> 2.34.1 >>> >>> >> >> >
On Tue, 19 Mar 2024 at 16:35, Neil Armstrong <neil.armstrong@linaro.org> wrote: > > On 19/03/2024 11:59, Neil Armstrong wrote: > > On 19/03/2024 11:55, Dmitry Baryshkov wrote: > >> On Tue, 19 Mar 2024 at 12:45, 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. > >>> > >>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > >>> --- > >>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++ > >>> 1 file changed, 70 insertions(+) > >>> > >>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > >>> index 079b3e306489..2d05226ae200 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) > >>> @@ -3681,6 +3687,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); > >>> +} > >> > >> Can we use of_clk_hw_onecell_get() instead? I think it even should be > >> possible to use onecell for both cases, it will look at the first arg, > >> which will be 0 in case of #clock-cells equal to 0. > > > > Let me investigate if it's possible > > Ok, it would work but it would require building a clk_hw_onecell_data a runtime, > while we could simply provide this qmp_pcie_clk_hw_get() and avoid runtime 2 allocations. > > I'm not sure it's worth it. Single allocation (or even 0 allocations if you embed it into struct qmp_pcie) for the sake of using standard helpers.
On 19/03/2024 15:46, Dmitry Baryshkov wrote: > On Tue, 19 Mar 2024 at 16:35, Neil Armstrong <neil.armstrong@linaro.org> wrote: >> >> On 19/03/2024 11:59, Neil Armstrong wrote: >>> On 19/03/2024 11:55, Dmitry Baryshkov wrote: >>>> On Tue, 19 Mar 2024 at 12:45, 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. >>>>> >>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>>> --- >>>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 70 insertions(+) >>>>> >>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c >>>>> index 079b3e306489..2d05226ae200 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) >>>>> @@ -3681,6 +3687,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); >>>>> +} >>>> >>>> Can we use of_clk_hw_onecell_get() instead? I think it even should be >>>> possible to use onecell for both cases, it will look at the first arg, >>>> which will be 0 in case of #clock-cells equal to 0. I didn't find evidence this is the case, while following of_parse_clkspec() called from of_clk_get_hw() or clk_core_get(), where clkspec is not initialized, the __of_parse_phandle_with_args() and of_phandle_iterator_args() don't touch clkspec->args if it->cur_count is 0. So clkspec->args[0] may be left initialized and it would be abusingthe API to use it with #clocks-cells = 0. >>> >>> Let me investigate if it's possible >> >> Ok, it would work but it would require building a clk_hw_onecell_data a runtime, >> while we could simply provide this qmp_pcie_clk_hw_get() and avoid runtime 2 allocations. >> >> I'm not sure it's worth it. > > Single allocation (or even 0 allocations if you embed it into struct > qmp_pcie) for the sake of using standard helpers. > Well, sure Thanks, Neil
On Tue, 19 Mar 2024 at 17:10, <neil.armstrong@linaro.org> wrote: > > On 19/03/2024 15:46, Dmitry Baryshkov wrote: > > On Tue, 19 Mar 2024 at 16:35, Neil Armstrong <neil.armstrong@linaro.org> wrote: > >> > >> On 19/03/2024 11:59, Neil Armstrong wrote: > >>> On 19/03/2024 11:55, Dmitry Baryshkov wrote: > >>>> On Tue, 19 Mar 2024 at 12:45, 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. > >>>>> > >>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > >>>>> --- > >>>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++ > >>>>> 1 file changed, 70 insertions(+) > >>>>> > >>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > >>>>> index 079b3e306489..2d05226ae200 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) > >>>>> @@ -3681,6 +3687,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); > >>>>> +} > >>>> > >>>> Can we use of_clk_hw_onecell_get() instead? I think it even should be > >>>> possible to use onecell for both cases, it will look at the first arg, > >>>> which will be 0 in case of #clock-cells equal to 0. > > I didn't find evidence this is the case, while following of_parse_clkspec() called > from of_clk_get_hw() or clk_core_get(), where clkspec is not initialized, the > __of_parse_phandle_with_args() and of_phandle_iterator_args() don't touch > clkspec->args if it->cur_count is 0. So clkspec->args[0] may be left initialized > and it would be abusingthe API to use it with #clocks-cells = 0. Ack, true. > > >>> > >>> Let me investigate if it's possible > >> > >> Ok, it would work but it would require building a clk_hw_onecell_data a runtime, > >> while we could simply provide this qmp_pcie_clk_hw_get() and avoid runtime 2 allocations. > >> > >> I'm not sure it's worth it. > > > > Single allocation (or even 0 allocations if you embed it into struct > > qmp_pcie) for the sake of using standard helpers. > > > Well, sure > > Thanks, > Neil >
On 19/03/2024 15:46, Dmitry Baryshkov wrote: > On Tue, 19 Mar 2024 at 16:35, Neil Armstrong <neil.armstrong@linaro.org> wrote: >> >> On 19/03/2024 11:59, Neil Armstrong wrote: >>> On 19/03/2024 11:55, Dmitry Baryshkov wrote: >>>> On Tue, 19 Mar 2024 at 12:45, 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. >>>>> >>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>>> --- >>>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 70 insertions(+) >>>>> >>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c >>>>> index 079b3e306489..2d05226ae200 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) >>>>> @@ -3681,6 +3687,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); >>>>> +} >>>> >>>> Can we use of_clk_hw_onecell_get() instead? I think it even should be >>>> possible to use onecell for both cases, it will look at the first arg, >>>> which will be 0 in case of #clock-cells equal to 0. >>> >>> Let me investigate if it's possible >> >> Ok, it would work but it would require building a clk_hw_onecell_data a runtime, >> while we could simply provide this qmp_pcie_clk_hw_get() and avoid runtime 2 allocations. >> >> I'm not sure it's worth it. > > Single allocation (or even 0 allocations if you embed it into struct > qmp_pcie) for the sake of using standard helpers. And I just recall I tried the same for Amlogic clocks, but the clk_hw_onecell_data hws field is a flexible array member you can't set at runtime, if you try you'll get: drivers/phy/qualcomm/phy-qcom-qmp-pcie.c:3753:38: error: invalid use of flexible array member 3753 | qmp->clk_hw_data.hws = qmp->clk_hws; Neil > > >
On Tue, 19 Mar 2024 at 17:15, <neil.armstrong@linaro.org> wrote: > > On 19/03/2024 15:46, Dmitry Baryshkov wrote: > > On Tue, 19 Mar 2024 at 16:35, Neil Armstrong <neil.armstrong@linaro.org> wrote: > >> > >> On 19/03/2024 11:59, Neil Armstrong wrote: > >>> On 19/03/2024 11:55, Dmitry Baryshkov wrote: > >>>> On Tue, 19 Mar 2024 at 12:45, 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. > >>>>> > >>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > >>>>> --- > >>>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++ > >>>>> 1 file changed, 70 insertions(+) > >>>>> > >>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > >>>>> index 079b3e306489..2d05226ae200 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) > >>>>> @@ -3681,6 +3687,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); > >>>>> +} > >>>> > >>>> Can we use of_clk_hw_onecell_get() instead? I think it even should be > >>>> possible to use onecell for both cases, it will look at the first arg, > >>>> which will be 0 in case of #clock-cells equal to 0. > >>> > >>> Let me investigate if it's possible > >> > >> Ok, it would work but it would require building a clk_hw_onecell_data a runtime, > >> while we could simply provide this qmp_pcie_clk_hw_get() and avoid runtime 2 allocations. > >> > >> I'm not sure it's worth it. > > > > Single allocation (or even 0 allocations if you embed it into struct > > qmp_pcie) for the sake of using standard helpers. > > And I just recall I tried the same for Amlogic clocks, but the clk_hw_onecell_data hws > field is a flexible array member you can't set at runtime, if you try you'll get: > drivers/phy/qualcomm/phy-qcom-qmp-pcie.c:3753:38: error: invalid use of flexible array member > 3753 | qmp->clk_hw_data.hws = qmp->clk_hws; Yes, so it's either devm_kzalloc(dev, struct_size(data, hws, 2), GFP_KERNEL); or struct qmp_pcie { ... struct { struct clk_hw_onecell_data clk_data; struct clk_hw clocks[2]; }; }; > > Neil > > > > > > > >
On 19/03/2024 17:05, Dmitry Baryshkov wrote: > On Tue, 19 Mar 2024 at 17:15, <neil.armstrong@linaro.org> wrote: >> >> On 19/03/2024 15:46, Dmitry Baryshkov wrote: >>> On Tue, 19 Mar 2024 at 16:35, Neil Armstrong <neil.armstrong@linaro.org> wrote: >>>> >>>> On 19/03/2024 11:59, Neil Armstrong wrote: >>>>> On 19/03/2024 11:55, Dmitry Baryshkov wrote: >>>>>> On Tue, 19 Mar 2024 at 12:45, 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. >>>>>>> >>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>>>>> --- >>>>>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++ >>>>>>> 1 file changed, 70 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c >>>>>>> index 079b3e306489..2d05226ae200 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) >>>>>>> @@ -3681,6 +3687,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); >>>>>>> +} >>>>>> >>>>>> Can we use of_clk_hw_onecell_get() instead? I think it even should be >>>>>> possible to use onecell for both cases, it will look at the first arg, >>>>>> which will be 0 in case of #clock-cells equal to 0. >>>>> >>>>> Let me investigate if it's possible >>>> >>>> Ok, it would work but it would require building a clk_hw_onecell_data a runtime, >>>> while we could simply provide this qmp_pcie_clk_hw_get() and avoid runtime 2 allocations. >>>> >>>> I'm not sure it's worth it. >>> >>> Single allocation (or even 0 allocations if you embed it into struct >>> qmp_pcie) for the sake of using standard helpers. >> >> And I just recall I tried the same for Amlogic clocks, but the clk_hw_onecell_data hws >> field is a flexible array member you can't set at runtime, if you try you'll get: >> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c:3753:38: error: invalid use of flexible array member >> 3753 | qmp->clk_hw_data.hws = qmp->clk_hws; > > Yes, so it's either > devm_kzalloc(dev, struct_size(data, hws, 2), GFP_KERNEL); > or > struct qmp_pcie { > ... > struct { > struct clk_hw_onecell_data clk_data; > struct clk_hw clocks[2]; > }; > }; I won't go down this path because of that mess and using of_clk_hw_onecell_get() would need to still add a custom getter before calling of_clk_hw_onecell_get() to handle the #clock-cells=0 legacy case. Neil > >> >> Neil >> >>> >>> >>> >> > >
On Tue, 19 Mar 2024 at 18:46, <neil.armstrong@linaro.org> wrote: > > On 19/03/2024 17:05, Dmitry Baryshkov wrote: > > On Tue, 19 Mar 2024 at 17:15, <neil.armstrong@linaro.org> wrote: > >> > >> On 19/03/2024 15:46, Dmitry Baryshkov wrote: > >>> On Tue, 19 Mar 2024 at 16:35, Neil Armstrong <neil.armstrong@linaro.org> wrote: > >>>> > >>>> On 19/03/2024 11:59, Neil Armstrong wrote: > >>>>> On 19/03/2024 11:55, Dmitry Baryshkov wrote: > >>>>>> On Tue, 19 Mar 2024 at 12:45, 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. > >>>>>>> > >>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > >>>>>>> --- > >>>>>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++ > >>>>>>> 1 file changed, 70 insertions(+) > >>>>>>> > >>>>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > >>>>>>> index 079b3e306489..2d05226ae200 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) > >>>>>>> @@ -3681,6 +3687,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); > >>>>>>> +} > >>>>>> > >>>>>> Can we use of_clk_hw_onecell_get() instead? I think it even should be > >>>>>> possible to use onecell for both cases, it will look at the first arg, > >>>>>> which will be 0 in case of #clock-cells equal to 0. > >>>>> > >>>>> Let me investigate if it's possible > >>>> > >>>> Ok, it would work but it would require building a clk_hw_onecell_data a runtime, > >>>> while we could simply provide this qmp_pcie_clk_hw_get() and avoid runtime 2 allocations. > >>>> > >>>> I'm not sure it's worth it. > >>> > >>> Single allocation (or even 0 allocations if you embed it into struct > >>> qmp_pcie) for the sake of using standard helpers. > >> > >> And I just recall I tried the same for Amlogic clocks, but the clk_hw_onecell_data hws > >> field is a flexible array member you can't set at runtime, if you try you'll get: > >> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c:3753:38: error: invalid use of flexible array member > >> 3753 | qmp->clk_hw_data.hws = qmp->clk_hws; > > > > Yes, so it's either > > devm_kzalloc(dev, struct_size(data, hws, 2), GFP_KERNEL); > > or > > struct qmp_pcie { > > ... > > struct { > > struct clk_hw_onecell_data clk_data; > > struct clk_hw clocks[2]; > > }; > > }; > > I won't go down this path because of that mess and using of_clk_hw_onecell_get() would need > to still add a custom getter before calling of_clk_hw_onecell_get() to handle the > #clock-cells=0 legacy case. No need for custom getters, if you select the getter during DT probe. But yes, choose whatever seems better. > > Neil > > > > >> > >> Neil > >> > >>> > >>> > >>> > >> > > > > >
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c index 079b3e306489..2d05226ae200 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) @@ -3681,6 +3687,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; @@ -3689,6 +3751,14 @@ static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np if (ret) return ret; + if (qmp->cfg->aux_clock_rate) { + ret = phy_aux_clk_register(qmp, np); + if (ret) + return ret; + + return devm_of_clk_add_hw_provider(qmp->dev, qmp_pcie_clk_hw_get, qmp); + } + return devm_of_clk_add_hw_provider(qmp->dev, of_clk_hw_simple_get, &qmp->pipe_clk_fixed.hw); }
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. Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> --- drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 70 ++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+)