diff mbox series

[v3,2/8] drm/msm/dsi: 28nm 8960 PHY: Get ref clock from the DT

Message ID 20181201005254.139908-3-mka@chromium.org (mailing list archive)
State New, archived
Headers show
Series drm/msm/dsi: Get PHY ref clocks from the DT | expand

Commit Message

Matthias Kaehlcke Dec. 1, 2018, 12:52 a.m. UTC
Get the ref clock of the PHY from the device tree instead of
hardcoding its name and rate. Use default values if the ref
clock is not specified.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v3:
- use default name and rate if the ref clock is not specified
  in the DT
- store vco_ref_clk_name instead of vco_ref_clk
- fixed check for EPROBE_DEFER
- renamed VCO_REF_CLK_RATE to VCO_REF_CLK_DEFAULT_RATE

Changes in v2:
- patch added to the series
---
 .../gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c   | 28 +++++++++++++++----
 1 file changed, 23 insertions(+), 5 deletions(-)

Comments

Stephen Boyd Dec. 4, 2018, 4:44 p.m. UTC | #1
Quoting Matthias Kaehlcke (2018-11-30 16:52:48)
> Get the ref clock of the PHY from the device tree instead of
> hardcoding its name and rate. Use default values if the ref
> clock is not specified.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> Changes in v3:
> - use default name and rate if the ref clock is not specified
>   in the DT
> - store vco_ref_clk_name instead of vco_ref_clk
> - fixed check for EPROBE_DEFER
> - renamed VCO_REF_CLK_RATE to VCO_REF_CLK_DEFAULT_RATE
> 
> Changes in v2:
> - patch added to the series
> ---
>  .../gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c   | 28 +++++++++++++++----
>  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c
> index 49008451085b8..3af678d3317f6 100644
> --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c
> +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c
> @@ -47,9 +47,9 @@
>  
>  #define NUM_PROVIDED_CLKS      2
>  
> -#define VCO_REF_CLK_RATE       27000000
> -#define VCO_MIN_RATE           600000000
> -#define VCO_MAX_RATE           1200000000
> +#define VCO_REF_CLK_DEFAULT_RATE       27000000
> +#define VCO_MIN_RATE                   600000000
> +#define VCO_MAX_RATE                   1200000000
>  
>  #define DSI_BYTE_PLL_CLK       0
>  #define DSI_PIXEL_PLL_CLK      1
> @@ -75,6 +75,8 @@ struct dsi_pll_28nm {
>         struct platform_device *pdev;
>         void __iomem *mmio;
>  
> +       const char *vco_ref_clk_name;

Can this be passed around during clk registration so we don't have to
store it away in the structure?

> +
>         /* custom byte clock divider */
>         struct clk_bytediv *bytediv;
>  
> @@ -125,7 +127,10 @@ static int dsi_pll_28nm_clk_set_rate(struct clk_hw *hw, unsigned long rate,
>         DBG("rate=%lu, parent's=%lu", rate, parent_rate);
>  
>         temp = rate / 10;
> -       val = VCO_REF_CLK_RATE / 10;
> +       if (parent_rate)
> +               val = parent_rate / 10;
> +       else
> +               val = VCO_REF_CLK_DEFAULT_RATE / 10;

Is the clk not properly hooked up to a parent sometimes so parent_rate
is 0? That sounds odd given the fact that it used to be 'pxo' and that
has always existed on the system as 27 MHz. So I'd remove this and just
use parent_rate all the time.
Matthias Kaehlcke Dec. 4, 2018, 5:35 p.m. UTC | #2
On Tue, Dec 04, 2018 at 08:44:00AM -0800, Stephen Boyd wrote:
> Quoting Matthias Kaehlcke (2018-11-30 16:52:48)
> > Get the ref clock of the PHY from the device tree instead of
> > hardcoding its name and rate. Use default values if the ref
> > clock is not specified.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > Changes in v3:
> > - use default name and rate if the ref clock is not specified
> >   in the DT
> > - store vco_ref_clk_name instead of vco_ref_clk
> > - fixed check for EPROBE_DEFER
> > - renamed VCO_REF_CLK_RATE to VCO_REF_CLK_DEFAULT_RATE
> > 
> > Changes in v2:
> > - patch added to the series
> > ---
> >  .../gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c   | 28 +++++++++++++++----
> >  1 file changed, 23 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c
> > index 49008451085b8..3af678d3317f6 100644
> > --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c
> > +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c
> > @@ -47,9 +47,9 @@
> >  
> >  #define NUM_PROVIDED_CLKS      2
> >  
> > -#define VCO_REF_CLK_RATE       27000000
> > -#define VCO_MIN_RATE           600000000
> > -#define VCO_MAX_RATE           1200000000
> > +#define VCO_REF_CLK_DEFAULT_RATE       27000000
> > +#define VCO_MIN_RATE                   600000000
> > +#define VCO_MAX_RATE                   1200000000
> >  
> >  #define DSI_BYTE_PLL_CLK       0
> >  #define DSI_PIXEL_PLL_CLK      1
> > @@ -75,6 +75,8 @@ struct dsi_pll_28nm {
> >         struct platform_device *pdev;
> >         void __iomem *mmio;
> >  
> > +       const char *vco_ref_clk_name;
> 
> Can this be passed around during clk registration so we don't have to
> store it away in the structure?

makes sense, will do

> > +
> >         /* custom byte clock divider */
> >         struct clk_bytediv *bytediv;
> >  
> > @@ -125,7 +127,10 @@ static int dsi_pll_28nm_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> >         DBG("rate=%lu, parent's=%lu", rate, parent_rate);
> >  
> >         temp = rate / 10;
> > -       val = VCO_REF_CLK_RATE / 10;
> > +       if (parent_rate)
> > +               val = parent_rate / 10;
> > +       else
> > +               val = VCO_REF_CLK_DEFAULT_RATE / 10;
> 
> Is the clk not properly hooked up to a parent sometimes so parent_rate
> is 0? That sounds odd given the fact that it used to be 'pxo' and that
> has always existed on the system as 27 MHz. So I'd remove this and just
> use parent_rate all the time.

I wondered about this, but since I don't have hardware for testing I
kept the previous hardcoded rate. If we know for sure that 'pxo'
always exists it should indeed be fine to use the parent rate.
Stephen Boyd Dec. 4, 2018, 6:26 p.m. UTC | #3
Quoting Matthias Kaehlcke (2018-12-04 09:35:49)
> On Tue, Dec 04, 2018 at 08:44:00AM -0800, Stephen Boyd wrote:
> > Quoting Matthias Kaehlcke (2018-11-30 16:52:48)
> > > +
> > >         /* custom byte clock divider */
> > >         struct clk_bytediv *bytediv;
> > >  
> > > @@ -125,7 +127,10 @@ static int dsi_pll_28nm_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> > >         DBG("rate=%lu, parent's=%lu", rate, parent_rate);
> > >  
> > >         temp = rate / 10;
> > > -       val = VCO_REF_CLK_RATE / 10;
> > > +       if (parent_rate)
> > > +               val = parent_rate / 10;
> > > +       else
> > > +               val = VCO_REF_CLK_DEFAULT_RATE / 10;
> > 
> > Is the clk not properly hooked up to a parent sometimes so parent_rate
> > is 0? That sounds odd given the fact that it used to be 'pxo' and that
> > has always existed on the system as 27 MHz. So I'd remove this and just
> > use parent_rate all the time.
> 
> I wondered about this, but since I don't have hardware for testing I
> kept the previous hardcoded rate. If we know for sure that 'pxo'
> always exists it should indeed be fine to use the parent rate.

Yes we know for sure. The 'pxo' board clk is there on apq8064 dtsi file
which seems to be the only place this is used. The pxo_board clk is sent
through a 'pxo' clk that's created in drivers/clk/qcom/common.c
qcom_cc_register_board_clk().
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c
index 49008451085b8..3af678d3317f6 100644
--- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c
+++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c
@@ -47,9 +47,9 @@ 
 
 #define NUM_PROVIDED_CLKS	2
 
-#define VCO_REF_CLK_RATE	27000000
-#define VCO_MIN_RATE		600000000
-#define VCO_MAX_RATE		1200000000
+#define VCO_REF_CLK_DEFAULT_RATE	27000000
+#define VCO_MIN_RATE			600000000
+#define VCO_MAX_RATE			1200000000
 
 #define DSI_BYTE_PLL_CLK	0
 #define DSI_PIXEL_PLL_CLK	1
@@ -75,6 +75,8 @@  struct dsi_pll_28nm {
 	struct platform_device *pdev;
 	void __iomem *mmio;
 
+	const char *vco_ref_clk_name;
+
 	/* custom byte clock divider */
 	struct clk_bytediv *bytediv;
 
@@ -125,7 +127,10 @@  static int dsi_pll_28nm_clk_set_rate(struct clk_hw *hw, unsigned long rate,
 	DBG("rate=%lu, parent's=%lu", rate, parent_rate);
 
 	temp = rate / 10;
-	val = VCO_REF_CLK_RATE / 10;
+	if (parent_rate)
+		val = parent_rate / 10;
+	else
+		val = VCO_REF_CLK_DEFAULT_RATE / 10;
 	fb_divider = (temp * VCO_PREF_DIV_RATIO) / val;
 	fb_divider = fb_divider / 2 - 1;
 	pll_write(base + REG_DSI_28nm_8960_PHY_PLL_CTRL_1,
@@ -410,7 +415,7 @@  static int pll_28nm_register(struct dsi_pll_28nm *pll_28nm)
 {
 	char *clk_name, *parent_name, *vco_name;
 	struct clk_init_data vco_init = {
-		.parent_names = (const char *[]){ "pxo" },
+		.parent_names = &pll_28nm->vco_ref_clk_name,
 		.num_parents = 1,
 		.flags = CLK_IGNORE_UNUSED,
 		.ops = &clk_ops_dsi_pll_28nm_vco,
@@ -494,6 +499,7 @@  struct msm_dsi_pll *msm_dsi_pll_28nm_8960_init(struct platform_device *pdev,
 {
 	struct dsi_pll_28nm *pll_28nm;
 	struct msm_dsi_pll *pll;
+	struct clk *vco_ref_clk;
 	int ret;
 
 	if (!pdev)
@@ -506,6 +512,18 @@  struct msm_dsi_pll *msm_dsi_pll_28nm_8960_init(struct platform_device *pdev,
 	pll_28nm->pdev = pdev;
 	pll_28nm->id = id + 1;
 
+	vco_ref_clk = devm_clk_get(&pdev->dev, "ref");
+	if (!IS_ERR(vco_ref_clk)) {
+		pll_28nm->vco_ref_clk_name = __clk_get_name(vco_ref_clk);
+	} else {
+		ret = PTR_ERR(vco_ref_clk);
+		if (ret == -EPROBE_DEFER)
+			return ERR_PTR(ret);
+
+		dev_warn(&pdev->dev, "'ref' clock is not specified, using default name\n");
+		pll_28nm->vco_ref_clk_name = "pxo";
+	}
+
 	pll_28nm->mmio = msm_ioremap(pdev, "dsi_pll", "DSI_PLL");
 	if (IS_ERR_OR_NULL(pll_28nm->mmio)) {
 		dev_err(&pdev->dev, "%s: failed to map pll base\n", __func__);