[2/2] drm/msm/dsi: Get PHY ref clock from the DT

Submitted by Matthias Kaehlcke on Nov. 2, 2018, 9:45 p.m.

Details

Message ID 20181102214534.184593-2-mka@chromium.org
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Freedreno

Not browsing as part of any series.

Commit Message

Matthias Kaehlcke Nov. 2, 2018, 9:45 p.m.
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(-)

Patch hide | download patch | download mbox

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");

Comments

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