Quoting Sascha Hauer (2013-08-23 07:01:28)

On Fri, Aug 23, 2013 at 01:58:15PM +0100, Mark Rutland wrote:

quoted

On Fri, Aug 23, 2013 at 07:34:21AM +0100, Sascha Hauer wrote:

quoted

On Fri, Aug 23, 2013 at 12:49:19AM +0200, Tomasz Figa wrote:

quoted

On Thursday 22 of August 2013 15:43:31 Mike Turquette wrote:

quoted

Quoting Sascha Hauer (2013-08-22 14:00:35)
=

quoted

quoted

quoted

quoted

quoted

On Thu, Aug 22, 2013 at 01:09:31PM +0100, Mark Rutland wrote:

quoted

On Thu, Aug 22, 2013 at 08:19:10AM +0100, Mike Turquette wrot=

e:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Quoting Tomasz Figa (2013-08-21 14:34:55)
=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

On Wednesday 21 of August 2013 09:50:15 Mark Rutland wrot=

e:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

On Tue, Aug 20, 2013 at 01:06:25AM +0100, Mike Turquett=

e =

quoted

quoted

quoted

wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Quoting Mark Rutland (2013-08-19 02:35:43)
=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

On Sat, Aug 17, 2013 at 04:17:18PM +0100, Tomasz Fi=

ga =

quoted

quoted

quoted

wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

On Saturday 17 of August 2013 16:53:16 Sascha Hau=

er =

quoted

quoted

quoted

wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomas=

z Figa =

quoted

quoted

quoted

wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Also I would make this option required.=

 Use a

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

dummy
clock for
mux
inputs that are grounded for a specific=

 SoC.

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Some clocks are not from CCM and we haven=

't

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

defined in
imx6q-clk.txt,
so in most cases we can't provide a phand=

le for

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

them, eg:
spdif_ext.
I think it's a bit hard to force it to be
'required'. An
'optional'
looks more flexible to me and a default o=

ne is

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

ensured
even if
it's
missing.

=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

<&clks 0> is the dummy clock. This can be u=

sed for

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

all input
clocks
not
defined by the SoC.

=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Where does this assumption come from? Is it
documented
anywhere?

=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

This is how all i.MX clock bindings currently a=

re. See

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Documentation/devicetree/bindings/clock/imx*-cl=

ock.txt

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

OK, thanks.
=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

I guess we need some discussion on dummy clocks vs
skipped clocks.
I think we want some consistency on this, don't w=

e?

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

If we really need a dummy clock, then we might al=

so want

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

a generic
way to specify it.

=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

What do we actually mean by a "dummy clock"? We alr=

eady

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

have
bindings
for "fixed-clock" and co friends describe relatively
simple
preconfigured clocks.

=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Some platforms have a fake clock which defines noops
callbacks and
basically doesn't do anything. This is analogous to t=

he

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

dummy
regulator
implementation. A central one could be registered by =

the

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

clock core,
as
is done by the regulator core.

=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

When you say some platforms, you presumably mean the pl=

atform

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

code in
Linux? A dummy clock sounds like a completely Linux-spe=

cific

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

abstraction rather than a description of the hardware, =

and I

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

don't see why we need that in the DT:
=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

* If a clock is wired up and running (as presumably the=

 dummy

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

clock is), then surely it's a fixed-clock (it's running=

, we

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

and we have no control over it, but we presumably know =

its

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

rate) and can be described as such?
=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

* If no clock is wired up, then we should be able to de=

scribe

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

that. If a driver believes that a clock is required whe=

n it

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

isn't (for some level of functionality), then that driv=

er

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

should be fixed up to support the clock as being option=

al.

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Am I missing something?

=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

I second that.
=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Moreover, I don't think that device tree should deal with=

 dummy

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

anything. It should be able to describe hardware that is
available on given system, not list what hardware is not
available.

=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

I wasn't clear. The dummy clock IS a completely Linux-speci=

fic

quoted

quoted

quoted

quoted

quoted

quoted

quoted

abstraction.
=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

I'm not advocating a dummy clock in DT. I am advocating
consolidation of the implementation of a clock that does no=

thing

quoted

quoted

quoted

quoted

quoted

quoted

quoted

into the clock core. This code could easily live in
drivers/clk/clk.c instead of having everyone open-code it.
=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

As far as specifying a dummy clock in DT? I dunno. DT should
describe
real hardware so there isn't much use for a dummy clock.

=

quoted

quoted

quoted

quoted

quoted

quoted

Sorry, I misunderstood. Good to hear we're on the same page :)
=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

I'm guessing one of the reasons for such a clock are driver=

s do

quoted

quoted

quoted

quoted

quoted

quoted

quoted

not
honor the clk.h api and they freak out when clk_get gives t=

hem a

quoted

quoted

quoted

quoted

quoted

quoted

quoted

NULL
pointer?

=

quoted

quoted

quoted

quoted

quoted

quoted

I'm not sure. Sascha, could you shed some light on the matter?

=

quoted

quoted

quoted

quoted

quoted

The original reason introducing the dummy clocks in the i.MX dt=

bs

quoted

quoted

quoted

quoted

quoted

was to provide devices a clock which the driver requests but is
not software controllable. We often have the case where the same
devices are on several SoCs, but not on all of them all clocks =

have

quoted

quoted

quoted

quoted

quoted

a bit to en/disable them.
=

quoted

quoted

quoted

quoted

quoted

Anyway, to accomplish this we don't need dummy clocks. We can j=

ust

quoted

quoted

quoted

quoted

quoted

describe the real clocks.

=

quoted

quoted

quoted

quoted

You could use a dummy clk for the Linux implementation, but the d=

ownside

quoted

quoted

quoted

quoted

is that a dummy clock has a rate of 0 always and a your clocks li=

kely

quoted

quoted

quoted

quoted

have non-zero rates.
=

quoted

quoted

quoted

quoted

It is probably better for you define a clock which only implement=

s the

quoted

quoted

quoted

quoted

.recalc_rate callback. If the rate of this clock changes without =

Linux

quoted

quoted

quoted

quoted

having knowledge of it you can use the CLK_GET_RATE_NOCACHE flag.

=

quoted

quoted

quoted

I doubt that rate of a dummy clock could ever change... unless it i=

s a =

quoted

quoted

quoted

rather smart dummy.
=

quoted

quoted

quoted

quoted

quoted

BTW with the S/PDIF core on which not all mux inputs are connec=

ted

quoted

quoted

quoted

quoted

quoted

to actual clocks we could also describe the unconnected inputs =

as

quoted

quoted

quoted

quoted

quoted

ground clocks with rate 0. This way we describe something which
is really there instead of dummy clocks ;)

=

quoted

quoted

quoted

quoted

Again you could use a dummy clock for this OR a fixed-rate clock =

with a

quoted

quoted

quoted

quoted

rate of zero from the perspective of the Linux implementation.
=

quoted

quoted

quoted

quoted

Do you think it worthwhile to have a DT binding for a grounded cl=

ock?

quoted

quoted

quoted

quoted

That is not an entirely uncommon case.

=

quoted

quoted

quoted

Well, how would that differ from skipping a clock from clocks list,=

 i.e. =

quoted

quoted

quoted

not specifying it in clock-names and clocks properties?

=

quoted

quoted

The difference is that you can successfully grab it in your driver.

=

quoted

That's a driver-specific issue. The driver knows best which clocks it
can live without (if it's poking only a subset of the hardware, it may
not need some just yet, but could for extended functionality in future
when support is extended), and could assign a dummy to those clocks it
knows it doesn't need that aren't described. That doesn't need to be in
the dt, and shouldn't be, because it's OS and driver specific.
=

quoted

quoted

=

quoted

quoted

quoted

=

quoted

quoted

quoted

quoted

quoted

Background to why it might be a good idea to connect a ground c=

lock

quoted

quoted

quoted

quoted

quoted

to the unconnected input pins is that a driver has a chance to
successfully grab all clocks. Otherwise how does the driver
distinguish
between an unconnected and an erroneous clock?

=

quoted

quoted

quoted

quoted

Sorry, I don't follow this last question. Do you mean how to dist=

inguish

quoted

quoted

quoted

quoted

based on the value returned from clk_get?

=

quoted

quoted

quoted

Hmm, in theory, a driver could want to distinguish an error case (e=

.g. =

quoted

quoted

quoted

clock specified, but there is a problem with it) from no clock (e.g=

. clock =

quoted

quoted

quoted

not specified in DT, because it is not available on particular boar=

d).

quoted

quoted

=

quoted

quoted

Yes, that's what I meant. To illustrate the problem for this driver:
=

quoted

quoted

    for (i =3D 0; i < STC_TXCLK_SRC_MAX; i++) {
            sprintf(tmp, "rxtx%d", i);
            clk =3D devm_clk_get(&pdev->dev, tmp);
            if (IS_ERR(clk)) {

=

quoted

              [...]
=

quoted

                      /*
                       * ERR_PTR(-ENOENT) returned when clock not
                       * present in the dt (i.e. not wired up). We can
                       * live without this clock, so assign a dummy
                       * (NULL) to simplify the rest of the code. If
                       * the clock is present but something else went
                       * wrong, we'll get a different ERR_PTR value
                       * and actually fail.
                       */
                      if (clk =3D=3D ERR_PTR(-ENOENT)
                              clk =3D NULL;
=

quoted

quoted

            }
    }
=

quoted

quoted

This could be solved by always specifying all input clocks in the
devicetree.

=

quoted

As far as I can see, the above is sufficient, and leaves the knowledge
of skippable clocks in the driver, where I believe it should be.

=

You miss my point. Once a clock is specified in the devicetree it is no
longer optional. The driver currently can't find out whether a clock
hasn't been specified (and it's ok that it's not there) or a clock has
been specified, but some error prevents actually grabbing the clock (in
which case the driver should bail out)

Seems like the regulator framework is solving this with the new
regulator_get_optional() call. This leaves the
optional-versus-not-optional logic up to the driver.

Regards,
Mike

=

Sascha
=

=

-- =

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |