[PATCH 1/2] dt-bindings: msm/dsi: Add ref clock for 10nm PHY

[PATCH 1/2] dt-bindings: msm/dsi: Add ref clock for 10nm PHY

[Matthias Kaehlcke]

Allow the 10nm PHY driver to get the ref clock from the DT.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 Documentation/devicetree/bindings/display/msm/dsi.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
index dfc743219bd88..d0d2046ceff69 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi.txt
+++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
@@ -105,6 +105,10 @@ Required properties:
 - power-domains: Should be <&mmcc MDSS_GDSC>.
 - clocks: Phandles to device clocks. See [1] for details on clock bindings.
 - clock-names: the following clocks are required:
+  For 10nm PHY:
+  * "iface"
+  * "ref"
+  For other PHYs:
   * "iface"
   For 28nm HPM/LP, 28nm 8960 PHYs:
 - vddio-supply: phandle to vdd-io regulator device node
-- 
2.19.1.930.g4563a0d9d0-goog

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

[Matthias Kaehlcke]

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;
+	}
+
 	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

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

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

[Matthias Kaehlcke]

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

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

[Stephen Boyd]

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

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

[Matthias Kaehlcke]

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

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

[Stephen Boyd]

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.

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

[Doug Anderson]

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

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

[Matthias Kaehlcke]

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

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

[Doug Anderson]

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

Re: [PATCH 1/2] dt-bindings: msm/dsi: Add ref clock for 10nm PHY

[Sean Paul]

On Fri, Nov 02, 2018 at 02:45:33PM -0700, Matthias Kaehlcke wrote:
> Allow the 10nm PHY driver to get the ref clock from the DT.
> 

Might be nice to state that there are no current users of the 10nm phy in your
commit message.

Sean

> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  Documentation/devicetree/bindings/display/msm/dsi.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
> index dfc743219bd88..d0d2046ceff69 100644
> --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
> +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
> @@ -105,6 +105,10 @@ Required properties:
>  - power-domains: Should be <&mmcc MDSS_GDSC>.
>  - clocks: Phandles to device clocks. See [1] for details on clock bindings.
>  - clock-names: the following clocks are required:
> +  For 10nm PHY:
> +  * "iface"
> +  * "ref"
> +  For other PHYs:
>    * "iface"
>    For 28nm HPM/LP, 28nm 8960 PHYs:
>  - vddio-supply: phandle to vdd-io regulator device node
> -- 
> 2.19.1.930.g4563a0d9d0-goog
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [PATCH 1/2] dt-bindings: msm/dsi: Add ref clock for 10nm PHY

[Stephen Boyd]

Quoting Matthias Kaehlcke (2018-11-02 14:45:33)
> Allow the 10nm PHY driver to get the ref clock from the DT.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  Documentation/devicetree/bindings/display/msm/dsi.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
> index dfc743219bd88..d0d2046ceff69 100644
> --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
> +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
> @@ -105,6 +105,10 @@ Required properties:
>  - power-domains: Should be <&mmcc MDSS_GDSC>.
>  - clocks: Phandles to device clocks. See [1] for details on clock bindings.
>  - clock-names: the following clocks are required:
> +  For 10nm PHY:
> +  * "iface"
> +  * "ref"
> +  For other PHYs:
>    * "iface"

Any reason we can't go back and do this for other phys too? They're all
the same with regards to this reference clk input.

>    For 28nm HPM/LP, 28nm 8960 PHYs:
>  - vddio-supply: phandle to vdd-io regulator device node

Re: [PATCH 1/2] dt-bindings: msm/dsi: Add ref clock for 10nm PHY

[Matthias Kaehlcke]

On Tue, Nov 06, 2018 at 03:09:40PM -0800, Stephen Boyd wrote:
> Quoting Matthias Kaehlcke (2018-11-02 14:45:33)
> > Allow the 10nm PHY driver to get the ref clock from the DT.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> >  Documentation/devicetree/bindings/display/msm/dsi.txt | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
> > index dfc743219bd88..d0d2046ceff69 100644
> > --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
> > +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
> > @@ -105,6 +105,10 @@ Required properties:
> >  - power-domains: Should be <&mmcc MDSS_GDSC>.
> >  - clocks: Phandles to device clocks. See [1] for details on clock bindings.
> >  - clock-names: the following clocks are required:
> > +  For 10nm PHY:
> > +  * "iface"
> > +  * "ref"
> > +  For other PHYs:
> >    * "iface"
> 
> Any reason we can't go back and do this for other phys too? They're all
> the same with regards to this reference clk input.

To avoid breaking existing users, at least the 28nm PHY is used by
msm8916. Also I don't have the hardware to test changes for other
PHYs.

Cheers

Matthias

Re: [PATCH 1/2] dt-bindings: msm/dsi: Add ref clock for 10nm PHY

[Stephen Boyd]

Quoting Matthias Kaehlcke (2018-11-14 14:42:52)
> On Tue, Nov 06, 2018 at 03:09:40PM -0800, Stephen Boyd wrote:
> > Quoting Matthias Kaehlcke (2018-11-02 14:45:33)
> > > Allow the 10nm PHY driver to get the ref clock from the DT.
> > > 
> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > ---
> > >  Documentation/devicetree/bindings/display/msm/dsi.txt | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
> > > index dfc743219bd88..d0d2046ceff69 100644
> > > --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
> > > +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
> > > @@ -105,6 +105,10 @@ Required properties:
> > >  - power-domains: Should be <&mmcc MDSS_GDSC>.
> > >  - clocks: Phandles to device clocks. See [1] for details on clock bindings.
> > >  - clock-names: the following clocks are required:
> > > +  For 10nm PHY:
> > > +  * "iface"
> > > +  * "ref"
> > > +  For other PHYs:
> > >    * "iface"
> > 
> > Any reason we can't go back and do this for other phys too? They're all
> > the same with regards to this reference clk input.
> 
> To avoid breaking existing users, at least the 28nm PHY is used by
> msm8916. Also I don't have the hardware to test changes for other
> PHYs.
> 

Hmmm I don't see why we need to worry about breaking "legacy" users. If
they're using an undocumented but still working method of doing
something that should be OK, but I'd like to see us update the DTS and
binding to be more forward looking so that future designs don't carry
forward badness. And not having hardware hasn't really been a problem in
the past either. We can probably have someone from Linaro test on any
affected platforms.