Re: [PATCH v2 03/11] doc/devicetree: Add Aspeed clock bindings

Re: [PATCH v2 03/11] doc/devicetree: Add Aspeed clock bindings

[Heiko Stübner]

[+clock-related patches should include clock-maintainers and lists]

Am Donnerstag, 21. April 2016, 17:34:01 schrieb Joel Stanley:
> +For example:
> +
> +	clk_clkin: clk_clkin {
> +		#clock-cells = <0>;
> +		compatible = "fixed-clock";
> +		clock-frequency = <48000000>;
> +	};
> +
> +	clk_hpll: clk_hpll {
> +		compatible = "aspeed,g4-hpll-clock";
> +		#clock-cells = <0>;
> +		reg = <0x1e6e2008 0x4>;
> +	};
> +
> +	clk_apb: clk_apb@1e6e2008 {
> +		#clock-cells = <0>;
> +		compatible = "aspeed,g4-apb-clock";
> +		reg = <0x1e6e2008 0x4>;
> +		clocks = <&clk_hpll>;
> +	};

You have both the hpll and apb_clk in the same register (probably even more 
clocks?) and separate clock instances where each instance will of_iomap the 
register itself (and thus multiple times in general).

>From what I remember exposing the clock controller as one block (instead of 
declaring each clock individually in the dts) is still the preferred way but I 
don't think I can find Mike's mail from back then easily.

And I think most clock-controller implementations actually use that paradigm.


Heiko

Re: [PATCH v2 03/11] doc/devicetree: Add Aspeed clock bindings

[Joel Stanley]

On Thu, Apr 21, 2016 at 8:50 PM, Heiko St=C3=BCbner <heiko@sntech.de> wrote=
:
> [+clock-related patches should include clock-maintainers and lists]

Thanks.

> Am Donnerstag, 21. April 2016, 17:34:01 schrieb Joel Stanley:
>> +For example:
>> +
>> +     clk_clkin: clk_clkin {
>> +             #clock-cells =3D <0>;
>> +             compatible =3D "fixed-clock";
>> +             clock-frequency =3D <48000000>;
>> +     };
>> +
>> +     clk_hpll: clk_hpll {
>> +             compatible =3D "aspeed,g4-hpll-clock";
>> +             #clock-cells =3D <0>;
>> +             reg =3D <0x1e6e2008 0x4>;
>> +     };
>> +
>> +     clk_apb: clk_apb@1e6e2008 {
>> +             #clock-cells =3D <0>;
>> +             compatible =3D "aspeed,g4-apb-clock";
>> +             reg =3D <0x1e6e2008 0x4>;
>> +             clocks =3D <&clk_hpll>;
>> +     };
>
> You have both the hpll and apb_clk in the same register (probably even mo=
re
> clocks?) and separate clock instances where each instance will of_iomap t=
he
> register itself (and thus multiple times in general).

Yep. I agree that's not ideal.

>
> From what I remember exposing the clock controller as one block (instead =
of
> declaring each clock individually in the dts) is still the preferred way =
but I
> don't think I can find Mike's mail from back then easily.

I can't picture how that would look. I took my lead from the moxart
clock driver; is there a better example that I should follow?

Cheers,

Joel

Re: [PATCH v2 03/11] doc/devicetree: Add Aspeed clock bindings

[Heiko Stübner]

Hi Joel,

Am Mittwoch, 27. April 2016, 18:01:00 schrieb Joel Stanley:
> On Thu, Apr 21, 2016 at 8:50 PM, Heiko St=FCbner <heiko@sntech.de> wr=
ote:
> > [+clock-related patches should include clock-maintainers and lists]=

>=20
> Thanks.
>=20
> > Am Donnerstag, 21. April 2016, 17:34:01 schrieb Joel Stanley:
> >> +For example:
> >> +
> >> +     clk_clkin: clk_clkin {
> >> +             #clock-cells =3D <0>;
> >> +             compatible =3D "fixed-clock";
> >> +             clock-frequency =3D <48000000>;
> >> +     };
> >> +
> >> +     clk_hpll: clk_hpll {
> >> +             compatible =3D "aspeed,g4-hpll-clock";
> >> +             #clock-cells =3D <0>;
> >> +             reg =3D <0x1e6e2008 0x4>;
> >> +     };
> >> +
> >> +     clk_apb: clk_apb@1e6e2008 {
> >> +             #clock-cells =3D <0>;
> >> +             compatible =3D "aspeed,g4-apb-clock";
> >> +             reg =3D <0x1e6e2008 0x4>;
> >> +             clocks =3D <&clk_hpll>;
> >> +     };
> >=20
> > You have both the hpll and apb_clk in the same register (probably e=
ven
> > more
> > clocks?) and separate clock instances where each instance will of_i=
omap
> > the
> > register itself (and thus multiple times in general).
>=20
> Yep. I agree that's not ideal.
>=20
> > From what I remember exposing the clock controller as one block (in=
stead
> > of
> > declaring each clock individually in the dts) is still the preferre=
d way
> > but I don't think I can find Mike's mail from back then easily.
>=20
> I can't picture how that would look. I took my lead from the moxart
> clock driver; is there a better example that I should follow?

qcom, samsung, rockchip, hisilicon, imx, ...

I guess the design would depend on the actual layout of your clock- / s=
ystem-
controller - aka what else is contained there.


Heiko

Re: [PATCH v2 03/11] doc/devicetree: Add Aspeed clock bindings

[Joel Stanley]

On Wed, Apr 27, 2016 at 6:42 PM, Heiko St=C3=BCbner <heiko@sntech.de> wrote=
:
> Am Mittwoch, 27. April 2016, 18:01:00 schrieb Joel Stanley:
>> > From what I remember exposing the clock controller as one block (inste=
ad
>> > of
>> > declaring each clock individually in the dts) is still the preferred w=
ay
>> > but I don't think I can find Mike's mail from back then easily.
>>
>> I can't picture how that would look. I took my lead from the moxart
>> clock driver; is there a better example that I should follow?
>
> qcom, samsung, rockchip, hisilicon, imx, ...

I had a look here, and they appear to be much more complex than I
need. The qcom directory is 41000 lines of code! The moxart driver is
similar to what we do, but as you mentioned it is not arranged how you
want it.

> I guess the design would depend on the actual layout of your clock- / sys=
tem-
> controller - aka what else is contained there.

In the fourth generation parts, such as the ast2400, we have this layout:

    clock                rate
 -----------------------------
  clk_clkin          48000000
     clk_hpll       384000000
        clk_apb      48000000

clkin is the oscillator that may be running at 24, 25 or 48MHz. We can
determine this from the strapping register.

The hpll divisor is controlled by strapping resistors, and indicated
in the strapping register.

The apb is controlled by a register in the SCU, the Aspeed's
bucket-of-bits for controlling various parts of the soc.

In our case we want don't need to adjust any clocks. We do want struct
clk's so attached device drivers to know how fast they are being
clocked. How do you see this laid out?

Cheers,

Joel

Re: [PATCH v2 03/11] doc/devicetree: Add Aspeed clock bindings

[Heiko Stübner]

Am Donnerstag, 28. April 2016, 16:20:04 schrieb Joel Stanley:
> On Wed, Apr 27, 2016 at 6:42 PM, Heiko St=FCbner <heiko@sntech.de> wr=
ote:
> > Am Mittwoch, 27. April 2016, 18:01:00 schrieb Joel Stanley:
> >> > From what I remember exposing the clock controller as one block
> >> > (instead
> >> > of
> >> > declaring each clock individually in the dts) is still the prefe=
rred
> >> > way
> >> > but I don't think I can find Mike's mail from back then easily.
> >>=20
> >> I can't picture how that would look. I took my lead from the moxar=
t
> >> clock driver; is there a better example that I should follow?
> >=20
> > qcom, samsung, rockchip, hisilicon, imx, ...
>=20
> I had a look here, and they appear to be much more complex than I
> need. The qcom directory is 41000 lines of code! The moxart driver is=

> similar to what we do, but as you mentioned it is not arranged how yo=
u
> want it.

I'm by no means authoritative ;-), but from what you describe below, cl=
k-
asm9260.c or clk-efm32gg.c might be going in that direction of very sim=
ple=20
clock-controllers.

Sorry about pointing to more complex drivers for bigger socs at first :=
-)


> > I guess the design would depend on the actual layout of your clock-=
 /
> > system- controller - aka what else is contained there.
>=20
> In the fourth generation parts, such as the ast2400, we have this lay=
out:
>=20
>     clock                rate
>  -----------------------------
>   clk_clkin          48000000
>      clk_hpll       384000000
>         clk_apb      48000000
>=20
> clkin is the oscillator that may be running at 24, 25 or 48MHz. We ca=
n
> determine this from the strapping register.
>=20
> The hpll divisor is controlled by strapping resistors, and indicated
> in the strapping register.
>=20
> The apb is controlled by a register in the SCU, the Aspeed's
> bucket-of-bits for controlling various parts of the soc.

I remember that from working on Samsung s3c24xx socs, the system-contro=
ller=20
area also worked as sort of catch-all :-) .

>=20
> In our case we want don't need to adjust any clocks. We do want struc=
t
> clk's so attached device drivers to know how fast they are being
> clocked. How do you see this laid out?

see drivers referenced above.

Re: [PATCH v2 03/11] doc/devicetree: Add Aspeed clock bindings

[Benjamin Herrenschmidt]

On Wed, 2016-04-27 at 11:12 +0200, Heiko Stübner wrote:
> I guess the design would depend on the actual layout of your clock- /
> system-
> controller - aka what else is contained there.

One register controls most clocks so it makes sense to have one driver.

Cheers,
Ben.