| Message ID | 20181102214534.184593-2-mka@chromium.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/2] dt-bindings: msm/dsi: Add ref clock for 10nm PHY | expand |
On Fri, Nov 02, 2018 at 02:45:34PM -0700, Matthias Kaehlcke wrote: > Get the PHY ref clock from the device tree instead of hardcoding > its name and rate. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c > index 4c03f0b7343ed..1016eb50df8f5 100644 > --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c > +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c > @@ -91,6 +91,8 @@ struct dsi_pll_10nm { > void __iomem *phy_cmn_mmio; > void __iomem *mmio; > > + struct clk *vco_ref_clk; > + > u64 vco_ref_clk_rate; > u64 vco_current_rate; > > @@ -630,7 +632,8 @@ static int pll_10nm_register(struct dsi_pll_10nm *pll_10nm) > char clk_name[32], parent[32], vco_name[32]; > char parent2[32], parent3[32], parent4[32]; > struct clk_init_data vco_init = { > - .parent_names = (const char *[]){ "xo" }, > + .parent_names = (const char *[]){ > + __clk_get_name(pll_10nm->vco_ref_clk) }, > .num_parents = 1, You should check the return of __clk_get_name() since you're setting num_parents to 1. Also, you should revert the patch that hardcodes 19.2MHz as part of this set. > .name = vco_name, > .flags = CLK_IGNORE_UNUSED, > @@ -786,6 +789,12 @@ struct msm_dsi_pll *msm_dsi_pll_10nm_init(struct platform_device *pdev, int id) > pll_10nm->id = id; > pll_10nm_list[id] = pll_10nm; > > + pll_10nm->vco_ref_clk = devm_clk_get(&pdev->dev, "ref"); > + if (IS_ERR(pll_10nm->vco_ref_clk)) { > + dev_err(&pdev->dev, "couldn't get 'ref' clock\n"); Please print the error message > + return (void *)pll_10nm->vco_ref_clk; Use ERR_CAST here > + } > + > pll_10nm->phy_cmn_mmio = msm_ioremap(pdev, "dsi_phy", "DSI_PHY"); > if (IS_ERR_OR_NULL(pll_10nm->phy_cmn_mmio)) { > dev_err(&pdev->dev, "failed to map CMN PHY base\n"); > -- > 2.19.1.930.g4563a0d9d0-goog > > _______________________________________________ > Freedreno mailing list > Freedreno@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/freedreno
Quoting Matthias Kaehlcke (2018-11-02 14:45:34) > @@ -630,7 +632,8 @@ static int pll_10nm_register(struct dsi_pll_10nm *pll_10nm) > char clk_name[32], parent[32], vco_name[32]; > char parent2[32], parent3[32], parent4[32]; > struct clk_init_data vco_init = { > - .parent_names = (const char *[]){ "xo" }, > + .parent_names = (const char *[]){ > + __clk_get_name(pll_10nm->vco_ref_clk) }, I find this syntax odd, in addition to needing to check for NULL here as Sean pointed out. Preferably just have it be the address of the character pointer instead of making an anonymous array and then casting that inline, i.e .parent_names = &ref_clk_name, > .num_parents = 1, > .name = vco_name, > .flags = CLK_IGNORE_UNUSED, > @@ -786,6 +789,12 @@ struct msm_dsi_pll *msm_dsi_pll_10nm_init(struct platform_device *pdev, int id) > pll_10nm->id = id; > pll_10nm_list[id] = pll_10nm; > > + pll_10nm->vco_ref_clk = devm_clk_get(&pdev->dev, "ref"); > + if (IS_ERR(pll_10nm->vco_ref_clk)) { > + dev_err(&pdev->dev, "couldn't get 'ref' clock\n"); This might be because of probe defer, which may be annoying to see this failure many times. > + return (void *)pll_10nm->vco_ref_clk; > + } > + > pll_10nm->phy_cmn_mmio = msm_ioremap(pdev, "dsi_phy", "DSI_PHY"); > if (IS_ERR_OR_NULL(pll_10nm->phy_cmn_mmio)) { > dev_err(&pdev->dev, "failed to map CMN PHY base\n");
Hi, On Fri, Nov 2, 2018 at 2:45 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > Get the PHY ref clock from the device tree instead of hardcoding > its name and rate. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c > index 4c03f0b7343ed..1016eb50df8f5 100644 > --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c > +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c > @@ -91,6 +91,8 @@ struct dsi_pll_10nm { > void __iomem *phy_cmn_mmio; > void __iomem *mmio; > > + struct clk *vco_ref_clk; > + > u64 vco_ref_clk_rate; > u64 vco_current_rate; > > @@ -630,7 +632,8 @@ static int pll_10nm_register(struct dsi_pll_10nm *pll_10nm) > char clk_name[32], parent[32], vco_name[32]; > char parent2[32], parent3[32], parent4[32]; > struct clk_init_data vco_init = { > - .parent_names = (const char *[]){ "xo" }, > + .parent_names = (const char *[]){ > + __clk_get_name(pll_10nm->vco_ref_clk) }, > .num_parents = 1, > .name = vco_name, > .flags = CLK_IGNORE_UNUSED, > @@ -786,6 +789,12 @@ struct msm_dsi_pll *msm_dsi_pll_10nm_init(struct platform_device *pdev, int id) > pll_10nm->id = id; > pll_10nm_list[id] = pll_10nm; > > + pll_10nm->vco_ref_clk = devm_clk_get(&pdev->dev, "ref"); > + if (IS_ERR(pll_10nm->vco_ref_clk)) { > + dev_err(&pdev->dev, "couldn't get 'ref' clock\n"); > + return (void *)pll_10nm->vco_ref_clk; > + } So, ummmm. Can you follow the same pattern for all the other clocks in this file too? All parents should get their name based on references in the device tree. It turns out that right now we have a mismatch because "drivers/clk/qcom/dispcc-sdm845.c" calls "dsi0pllbyte" "dsi0_phy_pll_out_byteclk" and calls "dsi0pll" "dsi0_phy_pll_out_dsiclk". We might want to change the names in dispcc-sdm845.c, but it wouldn't matter if we simply didn't hardcode them here. -Doug
On Mon, Nov 05, 2018 at 12:33:04PM -0500, Sean Paul wrote: > On Fri, Nov 02, 2018 at 02:45:34PM -0700, Matthias Kaehlcke wrote: > > Get the PHY ref clock from the device tree instead of hardcoding > > its name and rate. > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c > > index 4c03f0b7343ed..1016eb50df8f5 100644 > > --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c > > +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c > > @@ -91,6 +91,8 @@ struct dsi_pll_10nm { > > void __iomem *phy_cmn_mmio; > > void __iomem *mmio; > > > > + struct clk *vco_ref_clk; > > + > > u64 vco_ref_clk_rate; > > u64 vco_current_rate; > > > > @@ -630,7 +632,8 @@ static int pll_10nm_register(struct dsi_pll_10nm *pll_10nm) > > char clk_name[32], parent[32], vco_name[32]; > > char parent2[32], parent3[32], parent4[32]; > > struct clk_init_data vco_init = { > > - .parent_names = (const char *[]){ "xo" }, > > + .parent_names = (const char *[]){ > > + __clk_get_name(pll_10nm->vco_ref_clk) }, > > .num_parents = 1, > > You should check the return of __clk_get_name() since you're setting num_parents > to 1. Is that actually needed? __clk_get_name() only returns NULL if the passed clock is NULL, and this can't happen here since _init() fails if the clock can't be obtained, or am I missing something here? > Also, you should revert the patch that hardcodes 19.2MHz as part of this > set. Ooops, this somehow got dropped when moving the patch from my working tree to the repo I use for upstreaming. > > .name = vco_name, > > .flags = CLK_IGNORE_UNUSED, > > @@ -786,6 +789,12 @@ struct msm_dsi_pll *msm_dsi_pll_10nm_init(struct platform_device *pdev, int id) > > pll_10nm->id = id; > > pll_10nm_list[id] = pll_10nm; > > > > + pll_10nm->vco_ref_clk = devm_clk_get(&pdev->dev, "ref"); > > + if (IS_ERR(pll_10nm->vco_ref_clk)) { > > + dev_err(&pdev->dev, "couldn't get 'ref' clock\n"); > > Please print the error message Ok, except for -EPROBE_DEFER as per Stephen's comment. > > + return (void *)pll_10nm->vco_ref_clk; > > Use ERR_CAST here Will do Cheers Matthias
On Tue, Nov 06, 2018 at 03:11:48PM -0800, Stephen Boyd wrote: > Quoting Matthias Kaehlcke (2018-11-02 14:45:34) > > @@ -630,7 +632,8 @@ static int pll_10nm_register(struct dsi_pll_10nm *pll_10nm) > > char clk_name[32], parent[32], vco_name[32]; > > char parent2[32], parent3[32], parent4[32]; > > struct clk_init_data vco_init = { > > - .parent_names = (const char *[]){ "xo" }, > > + .parent_names = (const char *[]){ > > + __clk_get_name(pll_10nm->vco_ref_clk) }, > > I find this syntax odd, in addition to needing to check for NULL here as > Sean pointed out. Preferably just have it be the address of the > character pointer instead of making an anonymous array and then casting > that inline, i.e > > .parent_names = &ref_clk_name, Ok I'm not convinced the check for NULL is needed though, see my reply to Sean. > > .num_parents = 1, > > .name = vco_name, > > .flags = CLK_IGNORE_UNUSED, > > @@ -786,6 +789,12 @@ struct msm_dsi_pll *msm_dsi_pll_10nm_init(struct platform_device *pdev, int id) > > pll_10nm->id = id; > > pll_10nm_list[id] = pll_10nm; > > > > + pll_10nm->vco_ref_clk = devm_clk_get(&pdev->dev, "ref"); > > + if (IS_ERR(pll_10nm->vco_ref_clk)) { > > + dev_err(&pdev->dev, "couldn't get 'ref' clock\n"); > > This might be because of probe defer, which may be annoying to see this > failure many times. Ok, will skip the logging for -EPROBE_DEFER Cheers Matthias
Quoting Matthias Kaehlcke (2018-11-14 14:24:43) > On Tue, Nov 06, 2018 at 03:11:48PM -0800, Stephen Boyd wrote: > > Quoting Matthias Kaehlcke (2018-11-02 14:45:34) > > > @@ -630,7 +632,8 @@ static int pll_10nm_register(struct dsi_pll_10nm *pll_10nm) > > > char clk_name[32], parent[32], vco_name[32]; > > > char parent2[32], parent3[32], parent4[32]; > > > struct clk_init_data vco_init = { > > > - .parent_names = (const char *[]){ "xo" }, > > > + .parent_names = (const char *[]){ > > > + __clk_get_name(pll_10nm->vco_ref_clk) }, > > > > I find this syntax odd, in addition to needing to check for NULL here as > > Sean pointed out. Preferably just have it be the address of the > > character pointer instead of making an anonymous array and then casting > > that inline, i.e > > > > .parent_names = &ref_clk_name, > > Ok > > I'm not convinced the check for NULL is needed though, see my reply to Sean. Ok! I'm fine either way on the NULL stuff.
On Thu, Nov 08, 2018 at 02:04:31PM -0800, Doug Anderson wrote: > Hi, > > On Fri, Nov 2, 2018 at 2:45 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > > > Get the PHY ref clock from the device tree instead of hardcoding > > its name and rate. > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c > > index 4c03f0b7343ed..1016eb50df8f5 100644 > > --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c > > +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c > > @@ -91,6 +91,8 @@ struct dsi_pll_10nm { > > void __iomem *phy_cmn_mmio; > > void __iomem *mmio; > > > > + struct clk *vco_ref_clk; > > + > > u64 vco_ref_clk_rate; > > u64 vco_current_rate; > > > > @@ -630,7 +632,8 @@ static int pll_10nm_register(struct dsi_pll_10nm *pll_10nm) > > char clk_name[32], parent[32], vco_name[32]; > > char parent2[32], parent3[32], parent4[32]; > > struct clk_init_data vco_init = { > > - .parent_names = (const char *[]){ "xo" }, > > + .parent_names = (const char *[]){ > > + __clk_get_name(pll_10nm->vco_ref_clk) }, > > .num_parents = 1, > > .name = vco_name, > > .flags = CLK_IGNORE_UNUSED, > > @@ -786,6 +789,12 @@ struct msm_dsi_pll *msm_dsi_pll_10nm_init(struct platform_device *pdev, int id) > > pll_10nm->id = id; > > pll_10nm_list[id] = pll_10nm; > > > > + pll_10nm->vco_ref_clk = devm_clk_get(&pdev->dev, "ref"); > > + if (IS_ERR(pll_10nm->vco_ref_clk)) { > > + dev_err(&pdev->dev, "couldn't get 'ref' clock\n"); > > + return (void *)pll_10nm->vco_ref_clk; > > + } > > So, ummmm. Can you follow the same pattern for all the other clocks > in this file too? All parents should get their name based on > references in the device tree. > > It turns out that right now we have a mismatch because > "drivers/clk/qcom/dispcc-sdm845.c" calls "dsi0pllbyte" > "dsi0_phy_pll_out_byteclk" and calls "dsi0pll" > "dsi0_phy_pll_out_dsiclk". We might want to change the names in > dispcc-sdm845.c, but it wouldn't matter if we simply didn't hardcode > them here. Hm, I understand the problem, but not quite what you mean with 'follow the same pattern'. The VCO ref clock is an 'external'/existing clock, hence it can be specificed in the DT and obtained with _clk_get(). However the clocks you mention above are 'created' by the PHY driver, so we could only specify their names in the DT, not sure if that's what you are suggesting. I guess 'clock-output-names' could be used, though it isn't really useful to describe the names in a clock tree. If you still think this should be done please share how you envision the DT entries to look. Cheers Matthias
Hi, On Wed, Nov 14, 2018 at 3:56 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > On Thu, Nov 08, 2018 at 02:04:31PM -0800, Doug Anderson wrote: > > Hi, > > > > On Fri, Nov 2, 2018 at 2:45 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > > Get the PHY ref clock from the device tree instead of hardcoding > > > its name and rate. > > > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > > --- > > > drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 11 ++++++++++- > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c > > > index 4c03f0b7343ed..1016eb50df8f5 100644 > > > --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c > > > +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c > > > @@ -91,6 +91,8 @@ struct dsi_pll_10nm { > > > void __iomem *phy_cmn_mmio; > > > void __iomem *mmio; > > > > > > + struct clk *vco_ref_clk; > > > + > > > u64 vco_ref_clk_rate; > > > u64 vco_current_rate; > > > > > > @@ -630,7 +632,8 @@ static int pll_10nm_register(struct dsi_pll_10nm *pll_10nm) > > > char clk_name[32], parent[32], vco_name[32]; > > > char parent2[32], parent3[32], parent4[32]; > > > struct clk_init_data vco_init = { > > > - .parent_names = (const char *[]){ "xo" }, > > > + .parent_names = (const char *[]){ > > > + __clk_get_name(pll_10nm->vco_ref_clk) }, > > > .num_parents = 1, > > > .name = vco_name, > > > .flags = CLK_IGNORE_UNUSED, > > > @@ -786,6 +789,12 @@ struct msm_dsi_pll *msm_dsi_pll_10nm_init(struct platform_device *pdev, int id) > > > pll_10nm->id = id; > > > pll_10nm_list[id] = pll_10nm; > > > > > > + pll_10nm->vco_ref_clk = devm_clk_get(&pdev->dev, "ref"); > > > + if (IS_ERR(pll_10nm->vco_ref_clk)) { > > > + dev_err(&pdev->dev, "couldn't get 'ref' clock\n"); > > > + return (void *)pll_10nm->vco_ref_clk; > > > + } > > > > So, ummmm. Can you follow the same pattern for all the other clocks > > in this file too? All parents should get their name based on > > references in the device tree. > > > > It turns out that right now we have a mismatch because > > "drivers/clk/qcom/dispcc-sdm845.c" calls "dsi0pllbyte" > > "dsi0_phy_pll_out_byteclk" and calls "dsi0pll" > > "dsi0_phy_pll_out_dsiclk". We might want to change the names in > > dispcc-sdm845.c, but it wouldn't matter if we simply didn't hardcode > > them here. > > Hm, I understand the problem, but not quite what you mean with 'follow > the same pattern'. The VCO ref clock is an 'external'/existing clock, > hence it can be specificed in the DT and obtained with > _clk_get(). However the clocks you mention above are 'created' by the > PHY driver, so we could only specify their names in the DT, not sure > if that's what you are suggesting. I guess 'clock-output-names' could > be used, though it isn't really useful to describe the names in a > clock tree. If you still think this should be done please share how > you envision the DT entries to look. Ah. Right. This is me being dumb. As you said the "dsi0_phy_pll_out_byteclk" and "dsi0_phy_pll_out_dsiclk" clocks are backwards of the one you're dealing with here. No no change needed in your patch for this. In theory we could do a follow-up patch where the dispcc-sdm845 bindings take phandle references to the PHY clock for the clocks it consumes. That would future proof the bindings but I believe it wouldn't really be possible to use them right now in code since the clock framework doesn't really handle cases where two drivers mutually produce / consume clocks from each other. -Doug
diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c index 4c03f0b7343ed..1016eb50df8f5 100644 --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c @@ -91,6 +91,8 @@ struct dsi_pll_10nm { void __iomem *phy_cmn_mmio; void __iomem *mmio; + struct clk *vco_ref_clk; + u64 vco_ref_clk_rate; u64 vco_current_rate; @@ -630,7 +632,8 @@ static int pll_10nm_register(struct dsi_pll_10nm *pll_10nm) char clk_name[32], parent[32], vco_name[32]; char parent2[32], parent3[32], parent4[32]; struct clk_init_data vco_init = { - .parent_names = (const char *[]){ "xo" }, + .parent_names = (const char *[]){ + __clk_get_name(pll_10nm->vco_ref_clk) }, .num_parents = 1, .name = vco_name, .flags = CLK_IGNORE_UNUSED, @@ -786,6 +789,12 @@ struct msm_dsi_pll *msm_dsi_pll_10nm_init(struct platform_device *pdev, int id) pll_10nm->id = id; pll_10nm_list[id] = pll_10nm; + pll_10nm->vco_ref_clk = devm_clk_get(&pdev->dev, "ref"); + if (IS_ERR(pll_10nm->vco_ref_clk)) { + dev_err(&pdev->dev, "couldn't get 'ref' clock\n"); + return (void *)pll_10nm->vco_ref_clk; + } + pll_10nm->phy_cmn_mmio = msm_ioremap(pdev, "dsi_phy", "DSI_PHY"); if (IS_ERR_OR_NULL(pll_10nm->phy_cmn_mmio)) { dev_err(&pdev->dev, "failed to map CMN PHY base\n");
Get the PHY ref clock from the device tree instead of hardcoding its name and rate. Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)