Re: [PATCH 2/3] drm/tegra: Support setting the EMC clock

Re: [PATCH 2/3] drm/tegra: Support setting the EMC clock

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6247 bytes --]

On Mon, May 26, 2014 at 11:28:42AM +0200, Stéphane Marchesin wrote:
> 
> 
> 
> On Mon, May 26, 2014 at 2:07 AM, Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org<mailto:l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>> wrote:
> > Am Freitag, den 23.05.2014, 18:58 -0700 schrieb Stéphane Marchesin:
> > > The current code doesn't enable the EMC clock, without which the
> > > display cannot function, so let's enable this clock. We also need a
> > > bit of code to pick the right frequency for the EMC clock depending
> > > on the current video mode settings.
> > >
> > That's not the right way to do it. The DRM driver has no business
> > controlling the EMC clock directly. This should be done through a real
> > EMC driver plus some kind of bus QoS, where DC is just one client.
> 
> I thought about it but didn't see another consumer in upstream
> kernels. Who are the other consumers of EMC?

There are no other EMC consumers upstream at the moment. Some recent
discussions also indicate that it's unlikely that EMC scaling will be
implemented using shared clocks upstream.

See here for the full description:

	https://lkml.org/lkml/2014/5/13/469

Also adding linux-tegra to Cc. I like to keep that list in the loop for
patches that touch the Tegra DRM driver. That's especially useful if
other APIs are involved (such as clocks here).

Thierry

> 
> Stéphane
> 
> 
> Regards,
> Lucas
> 
> > Signed-off-by: Stéphane Marchesin <marcheu-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org<mailto:marcheu-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>>
> > ---
> >  drivers/gpu/drm/tegra/dc.c  | 61 ++++++++++++++++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/tegra/drm.h |  1 +
> >  2 files changed, 61 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> > index edb871d..f398dfb 100644
> > --- a/drivers/gpu/drm/tegra/dc.c
> > +++ b/drivers/gpu/drm/tegra/dc.c
> > @@ -325,6 +325,9 @@ static void tegra_crtc_disable(struct drm_crtc *crtc)
> >       }
> >
> >       drm_vblank_off(drm, dc->pipe);
> > +
> > +     if (dc->emc_clk)
> > +             clk_set_rate(dc->emc_clk, 0);
> >  }
> >
> >  static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc,
> > @@ -640,6 +643,50 @@ unsigned int tegra_dc_format(uint32_t format)
> >       return WIN_COLOR_DEPTH_B8G8R8A8;
> >  }
> >
> > +static unsigned long tegra_emc_bw_to_freq_req(unsigned long bw)
> > +{
> > +     int bytes_per_emc_clock;
> > +
> > +     if (of_machine_is_compatible("nvidia,tegra124"))
> > +             bytes_per_emc_clock = 16;
> > +     else
> > +             bytes_per_emc_clock = 8;
> > +
> > +     return (bw + bytes_per_emc_clock - 1) / bytes_per_emc_clock;
> > +}
> > +
> > +#define EMC_FREQ_CUTOFF_USE_130_PERCENT 100000000UL
> > +#define EMC_FREQ_CUTOFF_USE_140_PERCENT 50000000UL
> > +
> > +static int tegra_dc_program_bandwidth(struct tegra_dc *dc,
> > +                                   struct drm_display_mode *mode,
> > +                                   struct tegra_dc_window *window)
> > +{
> > +     unsigned long bandwidth = mode->clock * window->bits_per_pixel / 8;
> > +     unsigned long freq;
> > +     struct clk *emc_master;
> > +
> > +     if (!dc->emc_clk)
> > +             return 0;
> > +
> > +     emc_master = clk_get_parent(dc->emc_clk);
> > +     freq = tegra_emc_bw_to_freq_req(bandwidth) * 1000;
> > +     freq = clk_round_rate(emc_master, freq);
> > +
> > +     /* XXX: Add safety margins for DVFS */
> > +
> > +     if (freq < EMC_FREQ_CUTOFF_USE_140_PERCENT)
> > +             bandwidth += 4 * bandwidth / 10;
> > +     else if (freq < EMC_FREQ_CUTOFF_USE_130_PERCENT)
> > +             bandwidth += 3 * bandwidth / 10;
> > +     else
> > +             bandwidth += bandwidth / 10;
> > +
> > +     freq = tegra_emc_bw_to_freq_req(bandwidth) * 1000;
> > +
> > +     return clk_set_rate(dc->emc_clk, freq);
> > +}
> > +
> >  static int tegra_crtc_mode_set(struct drm_crtc *crtc,
> >                              struct drm_display_mode *mode,
> >                              struct drm_display_mode *adjusted,
> > @@ -691,7 +738,11 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc,
> >       if (err < 0)
> >               dev_err(dc->dev, "failed to enable root plane\n");
> >
> > -     return 0;
> > +     err = tegra_dc_program_bandwidth(dc, mode, &window);
> > +     if (err)
> > +             dev_err(dc->dev, "failed to program the EMC clock\n");
> > +
> > +     return err;
> >  }
> >
> >  static int tegra_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
> > @@ -1260,6 +1311,12 @@ static int tegra_dc_probe(struct platform_device *pdev)
> >       if (err < 0)
> >               return err;
> >
> > +     dc->emc_clk = devm_clk_get(&pdev->dev, "emc");
> > +     if (IS_ERR(dc->emc_clk))
> > +             dc->emc_clk = NULL;
> > +     else
> > +             clk_prepare_enable(dc->emc_clk);
> > +
> >       regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >       dc->regs = devm_ioremap_resource(&pdev->dev, regs);
> >       if (IS_ERR(dc->regs))
> > @@ -1312,6 +1369,8 @@ static int tegra_dc_remove(struct platform_device *pdev)
> >       }
> >
> >       clk_disable_unprepare(dc->clk);
> > +     if (dc->emc_clk)
> > +             clk_disable_unprepare(dc->emc_clk);
> >
> >       return 0;
> >  }
> > diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> > index 6753598..30d91c0 100644
> > --- a/drivers/gpu/drm/tegra/drm.h
> > +++ b/drivers/gpu/drm/tegra/drm.h
> > @@ -101,6 +101,7 @@ struct tegra_dc {
> >
> >       struct clk *clk;
> >       struct reset_control *rst;
> > +     struct clk *emc_clk;
> >       void __iomem *regs;
> >       int irq;
> >
> 
> --
> Pengutronix e.K.             | Lucas Stach                 |
> Industrial Linux Solutions   | http://www.pengutronix.de/  |
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org<mailto:dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] drm/tegra: Support setting the EMC clock

[Stéphane Marchesin]

[-- Attachment #1.1: Type: text/plain, Size: 6711 bytes --]

On Mon, May 26, 2014 at 2:35 AM, Thierry Reding <treding@nvidia.com> wrote:

> On Mon, May 26, 2014 at 11:28:42AM +0200, Stéphane Marchesin wrote:
> >
> >
> >
> > On Mon, May 26, 2014 at 2:07 AM, Lucas Stach <l.stach@pengutronix.de
> <mailto:l.stach@pengutronix.de>> wrote:
> > > Am Freitag, den 23.05.2014, 18:58 -0700 schrieb Stéphane Marchesin:
> > > > The current code doesn't enable the EMC clock, without which the
> > > > display cannot function, so let's enable this clock. We also need a
> > > > bit of code to pick the right frequency for the EMC clock depending
> > > > on the current video mode settings.
> > > >
> > > That's not the right way to do it. The DRM driver has no business
> > > controlling the EMC clock directly. This should be done through a real
> > > EMC driver plus some kind of bus QoS, where DC is just one client.
> >
> > I thought about it but didn't see another consumer in upstream
> > kernels. Who are the other consumers of EMC?
>
> There are no other EMC consumers upstream at the moment. Some recent
> discussions also indicate that it's unlikely that EMC scaling will be
> implemented using shared clocks upstream.
>
> See here for the full description:
>
>         https://lkml.org/lkml/2014/5/13/469


So if keeping the EMC clock private is a no-go, and shared clocks are also
a no-go, should I just make a separate one-off driver just for EMC and call
into that?

Stéphane


>
>
> Also adding linux-tegra to Cc. I like to keep that list in the loop for
> patches that touch the Tegra DRM driver. That's especially useful if
> other APIs are involved (such as clocks here).
>
> Thierry
>
> >
> > Stéphane
> >
> >
> > Regards,
> > Lucas
> >
> > > Signed-off-by: Stéphane Marchesin <marcheu@chromium.org<mailto:
> marcheu@chromium.org>>
> > > ---
> > >  drivers/gpu/drm/tegra/dc.c  | 61
> ++++++++++++++++++++++++++++++++++++++++++++-
> > >  drivers/gpu/drm/tegra/drm.h |  1 +
> > >  2 files changed, 61 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> > > index edb871d..f398dfb 100644
> > > --- a/drivers/gpu/drm/tegra/dc.c
> > > +++ b/drivers/gpu/drm/tegra/dc.c
> > > @@ -325,6 +325,9 @@ static void tegra_crtc_disable(struct drm_crtc
> *crtc)
> > >       }
> > >
> > >       drm_vblank_off(drm, dc->pipe);
> > > +
> > > +     if (dc->emc_clk)
> > > +             clk_set_rate(dc->emc_clk, 0);
> > >  }
> > >
> > >  static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc,
> > > @@ -640,6 +643,50 @@ unsigned int tegra_dc_format(uint32_t format)
> > >       return WIN_COLOR_DEPTH_B8G8R8A8;
> > >  }
> > >
> > > +static unsigned long tegra_emc_bw_to_freq_req(unsigned long bw)
> > > +{
> > > +     int bytes_per_emc_clock;
> > > +
> > > +     if (of_machine_is_compatible("nvidia,tegra124"))
> > > +             bytes_per_emc_clock = 16;
> > > +     else
> > > +             bytes_per_emc_clock = 8;
> > > +
> > > +     return (bw + bytes_per_emc_clock - 1) / bytes_per_emc_clock;
> > > +}
> > > +
> > > +#define EMC_FREQ_CUTOFF_USE_130_PERCENT 100000000UL
> > > +#define EMC_FREQ_CUTOFF_USE_140_PERCENT 50000000UL
> > > +
> > > +static int tegra_dc_program_bandwidth(struct tegra_dc *dc,
> > > +                                   struct drm_display_mode *mode,
> > > +                                   struct tegra_dc_window *window)
> > > +{
> > > +     unsigned long bandwidth = mode->clock * window->bits_per_pixel /
> 8;
> > > +     unsigned long freq;
> > > +     struct clk *emc_master;
> > > +
> > > +     if (!dc->emc_clk)
> > > +             return 0;
> > > +
> > > +     emc_master = clk_get_parent(dc->emc_clk);
> > > +     freq = tegra_emc_bw_to_freq_req(bandwidth) * 1000;
> > > +     freq = clk_round_rate(emc_master, freq);
> > > +
> > > +     /* XXX: Add safety margins for DVFS */
> > > +
> > > +     if (freq < EMC_FREQ_CUTOFF_USE_140_PERCENT)
> > > +             bandwidth += 4 * bandwidth / 10;
> > > +     else if (freq < EMC_FREQ_CUTOFF_USE_130_PERCENT)
> > > +             bandwidth += 3 * bandwidth / 10;
> > > +     else
> > > +             bandwidth += bandwidth / 10;
> > > +
> > > +     freq = tegra_emc_bw_to_freq_req(bandwidth) * 1000;
> > > +
> > > +     return clk_set_rate(dc->emc_clk, freq);
> > > +}
> > > +
> > >  static int tegra_crtc_mode_set(struct drm_crtc *crtc,
> > >                              struct drm_display_mode *mode,
> > >                              struct drm_display_mode *adjusted,
> > > @@ -691,7 +738,11 @@ static int tegra_crtc_mode_set(struct drm_crtc
> *crtc,
> > >       if (err < 0)
> > >               dev_err(dc->dev, "failed to enable root plane\n");
> > >
> > > -     return 0;
> > > +     err = tegra_dc_program_bandwidth(dc, mode, &window);
> > > +     if (err)
> > > +             dev_err(dc->dev, "failed to program the EMC clock\n");
> > > +
> > > +     return err;
> > >  }
> > >
> > >  static int tegra_crtc_mode_set_base(struct drm_crtc *crtc, int x, int
> y,
> > > @@ -1260,6 +1311,12 @@ static int tegra_dc_probe(struct
> platform_device *pdev)
> > >       if (err < 0)
> > >               return err;
> > >
> > > +     dc->emc_clk = devm_clk_get(&pdev->dev, "emc");
> > > +     if (IS_ERR(dc->emc_clk))
> > > +             dc->emc_clk = NULL;
> > > +     else
> > > +             clk_prepare_enable(dc->emc_clk);
> > > +
> > >       regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >       dc->regs = devm_ioremap_resource(&pdev->dev, regs);
> > >       if (IS_ERR(dc->regs))
> > > @@ -1312,6 +1369,8 @@ static int tegra_dc_remove(struct
> platform_device *pdev)
> > >       }
> > >
> > >       clk_disable_unprepare(dc->clk);
> > > +     if (dc->emc_clk)
> > > +             clk_disable_unprepare(dc->emc_clk);
> > >
> > >       return 0;
> > >  }
> > > diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> > > index 6753598..30d91c0 100644
> > > --- a/drivers/gpu/drm/tegra/drm.h
> > > +++ b/drivers/gpu/drm/tegra/drm.h
> > > @@ -101,6 +101,7 @@ struct tegra_dc {
> > >
> > >       struct clk *clk;
> > >       struct reset_control *rst;
> > > +     struct clk *emc_clk;
> > >       void __iomem *regs;
> > >       int irq;
> > >
> >
> > --
> > Pengutronix e.K.             | Lucas Stach                 |
> > Industrial Linux Solutions   | http://www.pengutronix.de/  |
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org<mailto:dri-devel@lists.freedesktop.org>
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
>

[-- Attachment #1.2: Type: text/html, Size: 9338 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel