Re: [PATCH v5] drm/pl111: Initial drm/kms driver for pl111

Re: [PATCH v5] drm/pl111: Initial drm/kms driver for pl111

[Linus Walleij]

On Wed, Apr 12, 2017 at 12:13 AM, Eric Anholt <eric@anholt.net> wrote:

> Oh, one last thing I think we need to figure out: I'm using TIM2_CLKSEL,
> which seems to be necessary on this platform.  My understanding is that
> this means that the pixel clock is divided from clcdclk instead of
> apb_pclk.  Do you agree?

Yes the pixed clock is always derived from clcdclk.

In most older ARM reference designs this is a VCO so that
is why there is a clk_set_rate() on this in the fbdev code.
(On some platforms that even has no effect I guess.)

> The fbdev driver is using
> clk_get(&fb->dev->dev, NULL) and not TIM2_CLKSEL, which I'm surprised by
> because I would have thought that would give us the first clock from the
> DT node (also clcdclk).

So that thing is a 1-bit line that can select one of two clocks
to be muxed into the PL111/CLCD.

I guess that up until now all platforms just left that line dangling in
the silicon. Congratulations, you came here first ;)

Though when I look at the Nomadik it seems that it might be muxing
the clock between 48 and 72 MHz, and I've been using 48MHz
all along ooopsie.

The current assumption in the bindings is that we have only
one clock and TIM2_CLKSEL is N/A.

If we want proper clcdclk handling with CLKSEL you should
probably add some code to implement a real mux clock for
this using <linux/clock-provider.h> and drivers/clk/clk-mux.c
with select COMMON_CLK
so that the driver still only sees clcdclk but that in turn is a
mux that can select one of two sources and will react to
the clk_set_rate() call by selecting the clock which is
closest in frequency to what you want.

This needs a small patch to alter the bindings too I guess.
A small clock node inside the CLCD, just like PCI bridges have
irqchips inside them etc:

clcd@10120000 {
        compatible = "arm,pl110", "arm,primecell";
        reg = <0x10120000 0x1000>;
        (...)
        clocks = <&clcdclk>, <&foo>;
        clock-names = "clcdclk", "apb_pclk";

        clcdclk: clock-controller@0 {
                compatible = "arm,pl11x-clock-mux";
                clocks = <&source_a>, <&source_b>;
        };
};

This can be set up easily in the OF probe path since that
is what we're doing: just look for this subnode, if it is there
create the clock controller.

I do not think the clk maintainers would mind a small mux
clock controller inside the CLCD driver to handle this mux
if we need it.

It would *maybe* also be possible to add a second "clcdclk2"
to the block and make an educated decision on which clock
to use in the driver but that is not as elegant as using the
clock framework mux clock I think.

Yours,
Linus Walleij

Re: [PATCH v5] drm/pl111: Initial drm/kms driver for pl111

[Neil Armstrong]

On 04/12/2017 09:40 AM, Linus Walleij wrote:
> On Wed, Apr 12, 2017 at 12:13 AM, Eric Anholt <eric@anholt.net> wrote:
> 
>> Oh, one last thing I think we need to figure out: I'm using TIM2_CLKSEL,
>> which seems to be necessary on this platform.  My understanding is that
>> this means that the pixel clock is divided from clcdclk instead of
>> apb_pclk.  Do you agree?
> 
> Yes the pixed clock is always derived from clcdclk.
> 
> In most older ARM reference designs this is a VCO so that
> is why there is a clk_set_rate() on this in the fbdev code.
> (On some platforms that even has no effect I guess.)
> 
>> The fbdev driver is using
>> clk_get(&fb->dev->dev, NULL) and not TIM2_CLKSEL, which I'm surprised by
>> because I would have thought that would give us the first clock from the
>> DT node (also clcdclk).
> 
> So that thing is a 1-bit line that can select one of two clocks
> to be muxed into the PL111/CLCD.
> 
> I guess that up until now all platforms just left that line dangling in
> the silicon. Congratulations, you came here first ;)
> 
> Though when I look at the Nomadik it seems that it might be muxing
> the clock between 48 and 72 MHz, and I've been using 48MHz
> all along ooopsie.
> 
> The current assumption in the bindings is that we have only
> one clock and TIM2_CLKSEL is N/A.
> 
> If we want proper clcdclk handling with CLKSEL you should
> probably add some code to implement a real mux clock for
> this using <linux/clock-provider.h> and drivers/clk/clk-mux.c
> with select COMMON_CLK
> so that the driver still only sees clcdclk but that in turn is a
> mux that can select one of two sources and will react to
> the clk_set_rate() call by selecting the clock which is
> closest in frequency to what you want.
> 
> This needs a small patch to alter the bindings too I guess.
> A small clock node inside the CLCD, just like PCI bridges have
> irqchips inside them etc:
> 
> clcd@10120000 {
>         compatible = "arm,pl110", "arm,primecell";
>         reg = <0x10120000 0x1000>;
>         (...)
>         clocks = <&clcdclk>, <&foo>;
>         clock-names = "clcdclk", "apb_pclk";
> 
>         clcdclk: clock-controller@0 {
>                 compatible = "arm,pl11x-clock-mux";
>                 clocks = <&source_a>, <&source_b>;
>         };
> };
> 
> This can be set up easily in the OF probe path since that
> is what we're doing: just look for this subnode, if it is there
> create the clock controller.
> 
> I do not think the clk maintainers would mind a small mux
> clock controller inside the CLCD driver to handle this mux
> if we need it.

Hi Linus,

Indeed it's the way of handling this use case, but no need to add
a clk node here, you can copy what we did in pwm-meson, meson-gx-mmc.c,
or dwmac-meson8b.c (we went further by also adding clk dividers).

In the probe code, simply add a clock mux provider with the two parents
clock names (these can and should be platform specific) and only call
a clk_set_parent() with the clock specified in the node clocks cell.

> 
> It would *maybe* also be possible to add a second "clcdclk2"
> to the block and make an educated decision on which clock
> to use in the driver but that is not as elegant as using the
> clock framework mux clock I think.
> 
> Yours,
> Linus Walleij

Neil

Re: [PATCH v5] drm/pl111: Initial drm/kms driver for pl111

[Stephen Boyd]

On 04/12, Neil Armstrong wrote:
> On 04/12/2017 09:40 AM, Linus Walleij wrote:
> > On Wed, Apr 12, 2017 at 12:13 AM, Eric Anholt <eric@anholt.net> wrote:
> > 
> >> Oh, one last thing I think we need to figure out: I'm using TIM2_CLKSEL,
> >> which seems to be necessary on this platform.  My understanding is that
> >> this means that the pixel clock is divided from clcdclk instead of
> >> apb_pclk.  Do you agree?
> > 
> > Yes the pixed clock is always derived from clcdclk.
> > 
> > In most older ARM reference designs this is a VCO so that
> > is why there is a clk_set_rate() on this in the fbdev code.
> > (On some platforms that even has no effect I guess.)
> > 
> >> The fbdev driver is using
> >> clk_get(&fb->dev->dev, NULL) and not TIM2_CLKSEL, which I'm surprised by
> >> because I would have thought that would give us the first clock from the
> >> DT node (also clcdclk).
> > 
> > So that thing is a 1-bit line that can select one of two clocks
> > to be muxed into the PL111/CLCD.
> > 
> > I guess that up until now all platforms just left that line dangling in
> > the silicon. Congratulations, you came here first ;)
> > 
> > Though when I look at the Nomadik it seems that it might be muxing
> > the clock between 48 and 72 MHz, and I've been using 48MHz
> > all along ooopsie.
> > 
> > The current assumption in the bindings is that we have only
> > one clock and TIM2_CLKSEL is N/A.
> > 
> > If we want proper clcdclk handling with CLKSEL you should
> > probably add some code to implement a real mux clock for
> > this using <linux/clock-provider.h> and drivers/clk/clk-mux.c
> > with select COMMON_CLK
> > so that the driver still only sees clcdclk but that in turn is a
> > mux that can select one of two sources and will react to
> > the clk_set_rate() call by selecting the clock which is
> > closest in frequency to what you want.
> > 
> > This needs a small patch to alter the bindings too I guess.
> > A small clock node inside the CLCD, just like PCI bridges have
> > irqchips inside them etc:
> > 
> > clcd@10120000 {
> >         compatible = "arm,pl110", "arm,primecell";
> >         reg = <0x10120000 0x1000>;
> >         (...)
> >         clocks = <&clcdclk>, <&foo>;
> >         clock-names = "clcdclk", "apb_pclk";
> > 
> >         clcdclk: clock-controller@0 {
> >                 compatible = "arm,pl11x-clock-mux";
> >                 clocks = <&source_a>, <&source_b>;
> >         };
> > };
> > 
> > This can be set up easily in the OF probe path since that
> > is what we're doing: just look for this subnode, if it is there
> > create the clock controller.
> > 
> > I do not think the clk maintainers would mind a small mux
> > clock controller inside the CLCD driver to handle this mux
> > if we need it.
> 
> Hi Linus,
> 
> Indeed it's the way of handling this use case, but no need to add
> a clk node here, you can copy what we did in pwm-meson, meson-gx-mmc.c,
> or dwmac-meson8b.c (we went further by also adding clk dividers).
> 
> In the probe code, simply add a clock mux provider with the two parents
> clock names (these can and should be platform specific) and only call
> a clk_set_parent() with the clock specified in the node clocks cell.
> 

+1

Avoiding a binding update is nice.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v5] drm/pl111: Initial drm/kms driver for pl111

[Russell King - ARM Linux]

On Wed, Apr 12, 2017 at 09:40:38AM +0200, Linus Walleij wrote:
> On Wed, Apr 12, 2017 at 12:13 AM, Eric Anholt <eric@anholt.net> wrote:
> > Oh, one last thing I think we need to figure out: I'm using TIM2_CLKSEL,
> > which seems to be necessary on this platform.  My understanding is that
> > this means that the pixel clock is divided from clcdclk instead of
> > apb_pclk.  Do you agree?
> 
> Yes the pixed clock is always derived from clcdclk.
> 
> In most older ARM reference designs this is a VCO so that
> is why there is a clk_set_rate() on this in the fbdev code.
> (On some platforms that even has no effect I guess.)
> 
> > The fbdev driver is using
> > clk_get(&fb->dev->dev, NULL) and not TIM2_CLKSEL, which I'm surprised by
> > because I would have thought that would give us the first clock from the
> > DT node (also clcdclk).
> 
> So that thing is a 1-bit line that can select one of two clocks
> to be muxed into the PL111/CLCD.
> 
> I guess that up until now all platforms just left that line dangling in
> the silicon. Congratulations, you came here first ;)
> 
> Though when I look at the Nomadik it seems that it might be muxing
> the clock between 48 and 72 MHz, and I've been using 48MHz
> all along ooopsie.
> 
> The current assumption in the bindings is that we have only
> one clock and TIM2_CLKSEL is N/A.
> 
> If we want proper clcdclk handling with CLKSEL you should
> probably add some code to implement a real mux clock for
> this using <linux/clock-provider.h> and drivers/clk/clk-mux.c
> with select COMMON_CLK
> so that the driver still only sees clcdclk but that in turn is a
> mux that can select one of two sources and will react to
> the clk_set_rate() call by selecting the clock which is
> closest in frequency to what you want.
> 
> This needs a small patch to alter the bindings too I guess.
> A small clock node inside the CLCD, just like PCI bridges have
> irqchips inside them etc:
> 
> clcd@10120000 {
>         compatible = "arm,pl110", "arm,primecell";
>         reg = <0x10120000 0x1000>;
>         (...)
>         clocks = <&clcdclk>, <&foo>;
>         clock-names = "clcdclk", "apb_pclk";
> 
>         clcdclk: clock-controller@0 {
>                 compatible = "arm,pl11x-clock-mux";
>                 clocks = <&source_a>, <&source_b>;
>         };
> };
> 
> This can be set up easily in the OF probe path since that
> is what we're doing: just look for this subnode, if it is there
> create the clock controller.
> 
> I do not think the clk maintainers would mind a small mux
> clock controller inside the CLCD driver to handle this mux
> if we need it.
> 
> It would *maybe* also be possible to add a second "clcdclk2"
> to the block and make an educated decision on which clock
> to use in the driver but that is not as elegant as using the
> clock framework mux clock I think.

We've had drivers (like imx-drm) embedding clk stuff within itself
in a similar manner to what you're suggesting, and it ended up
being a problem when it came to working out which is the correct
clock to use.  That stuff got ripped out of imx-drm and replaced
with a saner solution (that doesn't use CCF) before imx-drm moved
out of staging.  The reason for this was to get a saner solution to
"I want a clock running at X MHz, which clock gives me the closest
to the requested rate" without using the problematical fractional
divider^w^wfrequency modulator that severely upsets HDMI.

What I think you need to ask here is not how you should be modelling
it in DT, but how you're going to control it in the driver.  Under
what circumstance will you select one CLKSEL state or the other?

There's also consideration of the effect CLKSEL has - it's effectively
a GPIO output from the module that is available for the system
integrator to do whatever they choose with.  It just happens to have
the name "CLKSEL" - but it doesn't have to select a clock.

So, I don't think it's clear cut whether it should be exposed as a
GPIO and the GPIO based clk-mux used with it, or whether we should
update the binding to allow a second clock to be specified, giving
the driver the ability to make its own choice about which clock
should be selected.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.