RFC: Zynq Clock Controller

RFC: Zynq Clock Controller

[Sören Brinkmann]

Hi,

As you know, I'm reviewing Zynq's clock implementation and try to find a way to go forward. It looks like most archs contain all clocks within a clock controller block which provides all the output clocks which are then used by device drivers etc. E.g. the Tegra CAR.

I do also see some benefits of this approach:
 - The DT description should never change even when the clock tree and/or functionality within the controller changes (assuming all required outputs are present)
 - the controller can handle more sophisticated clock operations, which go beyond the simple enable/disable the device drivers are doing

For this reasons, I'd like to propose moving Zynq into the same direction. I.e. adding a clock controller with the following DT description (details may change but the general idea should become clear):
	clkc: clkc {
                #clock-cells = <1>;
                compatible = "xlnx,ps7-clkc";
                ps_clk_frequency = <33333333>;	# board x-tal
                # optional props
                gem0_emio_clk_freq = <125000000>;
                gem1_emio_clk_freq = <50000000>;
                can_mio_clk_freq_xx = <1234>; # this is possible 54 times with xx = 00..53
        };

For the PLLs we can use Josh's implementation and extend it where needed. For everything else we can then use the clock primitives instead of using custom implementations, I think. The node should be a subnode to Zynq's SLCR.

Currently I have identified the following output clocks:
        armpll, ddrpll, iopll,
        cpu_6or4x, cpu_3or2x, cpu_2x, cpu_1x,
        ddr2x, ddr3x, dci,
        lqspi, smc, pcap, gem0, gem1, fclk0, fclk1, fclk2, fclk3, can0, can1,
        sdio0, sdio1, uart0, uart1, spi0, spi1,
        dma_aper, usb0_aper, usb1_aper, gem0_aper, gem1_aper,
        sdio0_aper, sdio1_aper, spi0_aper, spi1_aper, can0_aper, can1_aper,
        i2c0_aper, i2c1_aper, uart0_aper, uart1_aper, gpio_aper, lqspi_aper,
        smc_aper


Clock consumers would work as described in the clock bindings:
	consumer: zynq_dummy_peripheral {
	        compatible = "xlnx,foobar";
	        clocks = <&clkc 69>;
	};


Is this an acceptable solution? Does anybody see issues or has other feedback?

	Thanks,
	S?ren

RFC: Zynq Clock Controller

[Jan Lübbe]

Hi S?ren,

On Tue, 2013-03-05 at 12:04 -0800, S?ren Brinkmann wrote:
> For this reasons, I'd like to propose moving Zynq into the same
> direction. I.e. adding a clock controller with the following DT
> description (details may change but the general idea should become
> clear):
> 	clkc: clkc {
>                 #clock-cells = <1>;
>                 compatible = "xlnx,ps7-clkc";
>                 ps_clk_frequency = <33333333>;	# board x-tal
>                 # optional props
>                 gem0_emio_clk_freq = <125000000>;
>                 gem1_emio_clk_freq = <50000000>;
>                 can_mio_clk_freq_xx = <1234>; # this is possible 54 times with xx = 00..53
>         };

The clock controller should only contain properties for input frequency
(which can obviously not be calculated at run-time).

Are the gem*, can* properties inputs? If they are actually outputs, the
corresponding frequencies should be requested by the clock consumers and
not hard-coded in DT.

Please keep in mind that DT properties use dashes instead of
underscores.

Best regards,
Jan
-- 
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 |

RFC: Zynq Clock Controller

[Sören Brinkmann]

Hi Jan,
 
what a small world. Good to hear from you.

On Wed, Mar 06, 2013 at 12:51:21PM +0100, Jan L?bbe wrote:
> Hi S?ren,
> 
> On Tue, 2013-03-05 at 12:04 -0800, S?ren Brinkmann wrote:
> > For this reasons, I'd like to propose moving Zynq into the same
> > direction. I.e. adding a clock controller with the following DT
> > description (details may change but the general idea should become
> > clear):
> > 	clkc: clkc {
> >                 #clock-cells = <1>;
> >                 compatible = "xlnx,ps7-clkc";
> >                 ps_clk_frequency = <33333333>;	# board x-tal
> >                 # optional props
> >                 gem0_emio_clk_freq = <125000000>;
> >                 gem1_emio_clk_freq = <50000000>;
> >                 can_mio_clk_freq_xx = <1234>; # this is possible 54 times with xx = 00..53
> >         };
> 
> The clock controller should only contain properties for input frequency
> (which can obviously not be calculated at run-time).
> 
> Are the gem*, can* properties inputs? If they are actually outputs, the
> corresponding frequencies should be requested by the clock consumers and
> not hard-coded in DT.
They are inputs. GEM and CAN have the option to be clocked through (E)MIO pins, i.e. some external clock input which cannot be derived from ps_clk like all other clocks. I plan to register a fixed rate, root clock for each of those properties, if present.

> 
> Please keep in mind that DT properties use dashes instead of
> underscores.
Okay, I'll make that substitution.

	S?ren

RFC: Zynq Clock Controller

[Lars-Peter Clausen]

On 03/06/2013 06:27 PM, S?ren Brinkmann wrote:
> Hi Jan,
>  
> what a small world. Good to hear from you.
> 
> On Wed, Mar 06, 2013 at 12:51:21PM +0100, Jan L?bbe wrote:
>> Hi S?ren,
>>
>> On Tue, 2013-03-05 at 12:04 -0800, S?ren Brinkmann wrote:
>>> For this reasons, I'd like to propose moving Zynq into the same
>>> direction. I.e. adding a clock controller with the following DT
>>> description (details may change but the general idea should become
>>> clear):
>>> 	clkc: clkc {
>>>                 #clock-cells = <1>;
>>>                 compatible = "xlnx,ps7-clkc";
>>>                 ps_clk_frequency = <33333333>;	# board x-tal
>>>                 # optional props
>>>                 gem0_emio_clk_freq = <125000000>;
>>>                 gem1_emio_clk_freq = <50000000>;
>>>                 can_mio_clk_freq_xx = <1234>; # this is possible 54 times with xx = 00..53
>>>         };

I definitely prefer the way it is right now in upstream, where we have one
dt node per clock. It is more descriptive and also more extensible. And you
also don't have to remember the clock index, and can use the phandle
directly instead.

>>
>> The clock controller should only contain properties for input frequency
>> (which can obviously not be calculated at run-time).
>>
>> Are the gem*, can* properties inputs? If they are actually outputs, the
>> corresponding frequencies should be requested by the clock consumers and
>> not hard-coded in DT.
> They are inputs. GEM and CAN have the option to be clocked through (E)MIO pins, i.e. some external clock input which cannot be derived from ps_clk like all other clocks. I plan to register a fixed rate, root clock for each of those properties, if present.

If it is a static external input clock use a "fixed-clock" dt node to
describe it. This also has the advantage that is is also possible to use a
non-static clock.

- Lars

RFC: Zynq Clock Controller

[Sören Brinkmann]

On Thu, Mar 07, 2013 at 10:36:35AM +0100, Lars-Peter Clausen wrote:
> On 03/06/2013 06:27 PM, S?ren Brinkmann wrote:
> > Hi Jan,
> >  
> > what a small world. Good to hear from you.
> > 
> > On Wed, Mar 06, 2013 at 12:51:21PM +0100, Jan L?bbe wrote:
> >> Hi S?ren,
> >>
> >> On Tue, 2013-03-05 at 12:04 -0800, S?ren Brinkmann wrote:
> >>> For this reasons, I'd like to propose moving Zynq into the same
> >>> direction. I.e. adding a clock controller with the following DT
> >>> description (details may change but the general idea should become
> >>> clear):
> >>> 	clkc: clkc {
> >>>                 #clock-cells = <1>;
> >>>                 compatible = "xlnx,ps7-clkc";
> >>>                 ps_clk_frequency = <33333333>;	# board x-tal
> >>>                 # optional props
> >>>                 gem0_emio_clk_freq = <125000000>;
> >>>                 gem1_emio_clk_freq = <50000000>;
> >>>                 can_mio_clk_freq_xx = <1234>; # this is possible 54 times with xx = 00..53
> >>>         };
> 
> I definitely prefer the way it is right now in upstream, where we have one
> dt node per clock. It is more descriptive and also more extensible. And you
> also don't have to remember the clock index, and can use the phandle
> directly instead.
The problems I see with that are:
 - with the clock controller we can model the clock tree as we need without changing the DT all the time
 - w/o a clock controller there is no block which has knowledge of the whole clock tree (unless we parse the DT), and could conduct reparent operations and similar
 - once the clock controller is properly defined the clock connection should be contained in a dtsi which never changes. So, regarding remembering outputs, I don't see a problem. I rather see issues with having a pile of clocks described in the DT. I have currently 44 outputs proposed, and to model the whole tree a lot of more clocks are used (I started modeling everything with the clock primitives and removed custom clock implementations, except for the PLLs). Having all those in the DT does not really help, IMHO.

> 
> >>
> >> The clock controller should only contain properties for input frequency
> >> (which can obviously not be calculated at run-time).
> >>
> >> Are the gem*, can* properties inputs? If they are actually outputs, the
> >> corresponding frequencies should be requested by the clock consumers and
> >> not hard-coded in DT.
> > They are inputs. GEM and CAN have the option to be clocked through (E)MIO pins, i.e. some external clock input which cannot be derived from ps_clk like all other clocks. I plan to register a fixed rate, root clock for each of those properties, if present.
> 
> If it is a static external input clock use a "fixed-clock" dt node to
> describe it. This also has the advantage that is is also possible to use a
> non-static clock.
I thought about this too, and it's probably a valid use-case.

What comes to my mind to support this would be:
 - clocks and clock-names properties to the controller
 - those two properties hold a list of available external clock sources

 e.g:
 	clocks = <&foo>, <&bar 2>, <&foobar 0>;
	clock-names = "can_emio_clk_44", "gem1_emio_clk", "can_emio_clk_00";

 Then the clock controller would go through the possible clock names trying a 'clk_get(NULL, "gem0_emio_clk"', etc. making available connections.

 	S?ren

RFC: Zynq Clock Controller

[Lars-Peter Clausen]

On 03/07/2013 08:11 PM, S?ren Brinkmann wrote:
> On Thu, Mar 07, 2013 at 10:36:35AM +0100, Lars-Peter Clausen wrote:
>> On 03/06/2013 06:27 PM, S?ren Brinkmann wrote:
>>> Hi Jan,
>>>  
>>> what a small world. Good to hear from you.
>>>
>>> On Wed, Mar 06, 2013 at 12:51:21PM +0100, Jan L?bbe wrote:
>>>> Hi S?ren,
>>>>
>>>> On Tue, 2013-03-05 at 12:04 -0800, S?ren Brinkmann wrote:
>>>>> For this reasons, I'd like to propose moving Zynq into the same
>>>>> direction. I.e. adding a clock controller with the following DT
>>>>> description (details may change but the general idea should become
>>>>> clear):
>>>>> 	clkc: clkc {
>>>>>                 #clock-cells = <1>;
>>>>>                 compatible = "xlnx,ps7-clkc";
>>>>>                 ps_clk_frequency = <33333333>;	# board x-tal
>>>>>                 # optional props
>>>>>                 gem0_emio_clk_freq = <125000000>;
>>>>>                 gem1_emio_clk_freq = <50000000>;
>>>>>                 can_mio_clk_freq_xx = <1234>; # this is possible 54 times with xx = 00..53
>>>>>         };
>>
>> I definitely prefer the way it is right now in upstream, where we have one
>> dt node per clock. It is more descriptive and also more extensible. And you
>> also don't have to remember the clock index, and can use the phandle
>> directly instead.
> The problems I see with that are:
>  - with the clock controller we can model the clock tree as we need without changing the DT all the time

When do we need to change the clock tree? The clock tree is pretty much fixed
in hardware. And the DT describes the hardware, so I don't so a problem there.

>  - w/o a clock controller there is no block which has knowledge of the whole clock tree (unless we parse the DT), and could conduct reparent operations and similar

The clk framework builds a representation of the clock tree. Each clock should
be able to handle re-parenting on it's own, without knowing about the other
clocks in the tree, the parent is selected by a field in the clocks register.
It doesn't even have to know who the parents are, the clk framework will take
care of all of this and just say, ok, switch your input clock to X, where X is
a simple integer number The clk framework will also take care of re-calculating
all the updates frequencies after re-parenting.

>  - once the clock controller is properly defined the clock connection should be contained in a dtsi which never changes. So, regarding remembering outputs, I don't see a problem. I rather see issues with having a pile of clocks described in the DT. I have currently 44 outputs proposed, and to model the whole tree a lot of more clocks are used (I started modeling everything with the clock primitives and removed custom clock implementations, except for the PLLs). Having all those in the DT does not really help, IMHO.

Well, except if you want to use external clocks for ethernet or CAN. Also you
don't need to change the dtsi either if you have a separate node for each clock.

To avoid misunderstandings let me clarify that I don't want to have one node
per clock, but rather one node per clock block (or whatever they are called).
The combination of input select + divider + output gate. E.g. the UART clock
block with it's 3 inputs and two outputs.

Also the APER clocks should probably be one node with 24 outputs.

- Lars

RFC: Zynq Clock Controller

[Sören Brinkmann]

On Thu, Mar 07, 2013 at 11:02:58PM +0100, Lars-Peter Clausen wrote:
> On 03/07/2013 08:11 PM, S?ren Brinkmann wrote:
> > On Thu, Mar 07, 2013 at 10:36:35AM +0100, Lars-Peter Clausen wrote:
> >> On 03/06/2013 06:27 PM, S?ren Brinkmann wrote:
> >>> Hi Jan,
> >>>  
> >>> what a small world. Good to hear from you.
> >>>
> >>> On Wed, Mar 06, 2013 at 12:51:21PM +0100, Jan L?bbe wrote:
> >>>> Hi S?ren,
> >>>>
> >>>> On Tue, 2013-03-05 at 12:04 -0800, S?ren Brinkmann wrote:
> >>>>> For this reasons, I'd like to propose moving Zynq into the same
> >>>>> direction. I.e. adding a clock controller with the following DT
> >>>>> description (details may change but the general idea should become
> >>>>> clear):
> >>>>> 	clkc: clkc {
> >>>>>                 #clock-cells = <1>;
> >>>>>                 compatible = "xlnx,ps7-clkc";
> >>>>>                 ps_clk_frequency = <33333333>;	# board x-tal
> >>>>>                 # optional props
> >>>>>                 gem0_emio_clk_freq = <125000000>;
> >>>>>                 gem1_emio_clk_freq = <50000000>;
> >>>>>                 can_mio_clk_freq_xx = <1234>; # this is possible 54 times with xx = 00..53
> >>>>>         };
> >>
> >> I definitely prefer the way it is right now in upstream, where we have one
> >> dt node per clock. It is more descriptive and also more extensible. And you
> >> also don't have to remember the clock index, and can use the phandle
> >> directly instead.
> > The problems I see with that are:
> >  - with the clock controller we can model the clock tree as we need without changing the DT all the time
> 
> When do we need to change the clock tree? The clock tree is pretty much fixed
> in hardware. And the DT describes the hardware, so I don't so a problem there.
> 
> >  - w/o a clock controller there is no block which has knowledge of the whole clock tree (unless we parse the DT), and could conduct reparent operations and similar
> 
> The clk framework builds a representation of the clock tree. Each clock should
> be able to handle re-parenting on it's own, without knowing about the other
> clocks in the tree, the parent is selected by a field in the clocks register.
> It doesn't even have to know who the parents are, the clk framework will take
> care of all of this and just say, ok, switch your input clock to X, where X is
> a simple integer number The clk framework will also take care of re-calculating
> all the updates frequencies after re-parenting.
Nope, clk_set_parent() takes two 'struct *clk' as argument. So, you have to have those of the clock to change and all its parents. A device driver for example, which gets a clock through clk_get() does not have that information and should not, since it should not have to be aware of the SOC's clock hierarchy, IMHO.

> 
> >  - once the clock controller is properly defined the clock connection should be contained in a dtsi which never changes. So, regarding remembering outputs, I don't see a problem. I rather see issues with having a pile of clocks described in the DT. I have currently 44 outputs proposed, and to model the whole tree a lot of more clocks are used (I started modeling everything with the clock primitives and removed custom clock implementations, except for the PLLs). Having all those in the DT does not really help, IMHO.
> 
> Well, except if you want to use external clocks for ethernet or CAN. Also you
> don't need to change the dtsi either if you have a separate node for each clock.
> 
> To avoid misunderstandings let me clarify that I don't want to have one node
> per clock, but rather one node per clock block (or whatever they are called).
> The combination of input select + divider + output gate. E.g. the UART clock
> block with it's 3 inputs and two outputs.
> 
> Also the APER clocks should probably be one node with 24 outputs.
Okay, so instead of having one block encapsulating all clocks, you want it on a finer granularity. I don't see huge differences why that should be advantageous? It would just mean to create several blocks with their custom DT bindings instead of a single one. Just the abstraction level would be a little different.

	S?ren

RFC: Zynq Clock Controller

[Lars-Peter Clausen]

On 03/08/2013 12:25 AM, S?ren Brinkmann wrote:
> On Thu, Mar 07, 2013 at 11:02:58PM +0100, Lars-Peter Clausen wrote:
>> On 03/07/2013 08:11 PM, S?ren Brinkmann wrote:
>>> On Thu, Mar 07, 2013 at 10:36:35AM +0100, Lars-Peter Clausen wrote:
>>>> On 03/06/2013 06:27 PM, S?ren Brinkmann wrote:
>>>>> Hi Jan,
>>>>>  
>>>>> what a small world. Good to hear from you.
>>>>>
>>>>> On Wed, Mar 06, 2013 at 12:51:21PM +0100, Jan L?bbe wrote:
>>>>>> Hi S?ren,
>>>>>>
>>>>>> On Tue, 2013-03-05 at 12:04 -0800, S?ren Brinkmann wrote:
>>>>>>> For this reasons, I'd like to propose moving Zynq into the same
>>>>>>> direction. I.e. adding a clock controller with the following DT
>>>>>>> description (details may change but the general idea should become
>>>>>>> clear):
>>>>>>> 	clkc: clkc {
>>>>>>>                 #clock-cells = <1>;
>>>>>>>                 compatible = "xlnx,ps7-clkc";
>>>>>>>                 ps_clk_frequency = <33333333>;	# board x-tal
>>>>>>>                 # optional props
>>>>>>>                 gem0_emio_clk_freq = <125000000>;
>>>>>>>                 gem1_emio_clk_freq = <50000000>;
>>>>>>>                 can_mio_clk_freq_xx = <1234>; # this is possible 54 times with xx = 00..53
>>>>>>>         };
>>>>
>>>> I definitely prefer the way it is right now in upstream, where we have one
>>>> dt node per clock. It is more descriptive and also more extensible. And you
>>>> also don't have to remember the clock index, and can use the phandle
>>>> directly instead.
>>> The problems I see with that are:
>>>  - with the clock controller we can model the clock tree as we need without changing the DT all the time
>>
>> When do we need to change the clock tree? The clock tree is pretty much fixed
>> in hardware. And the DT describes the hardware, so I don't so a problem there.
>>
>>>  - w/o a clock controller there is no block which has knowledge of the whole clock tree (unless we parse the DT), and could conduct reparent operations and similar
>>
>> The clk framework builds a representation of the clock tree. Each clock should
>> be able to handle re-parenting on it's own, without knowing about the other
>> clocks in the tree, the parent is selected by a field in the clocks register.
>> It doesn't even have to know who the parents are, the clk framework will take
>> care of all of this and just say, ok, switch your input clock to X, where X is
>> a simple integer number The clk framework will also take care of re-calculating
>> all the updates frequencies after re-parenting.
> Nope, clk_set_parent() takes two 'struct *clk' as argument. So, you have to have those of the clock to change and all its parents. A device driver for example, which gets a clock through clk_get() does not have that information and should not, since it should not have to be aware of the SOC's clock hierarchy, IMHO.
> 

Yes, you global clk_set_parent() method takes two clk structs. But the clock
framework takes care of all the magic of mapping that second clk struct to a
integer number, based on the clks parent list. So your set_parent callback in
your clk_ops takes as parameters your clk and the index of the parent.

>>
>>>  - once the clock controller is properly defined the clock connection should be contained in a dtsi which never changes. So, regarding remembering outputs, I don't see a problem. I rather see issues with having a pile of clocks described in the DT. I have currently 44 outputs proposed, and to model the whole tree a lot of more clocks are used (I started modeling everything with the clock primitives and removed custom clock implementations, except for the PLLs). Having all those in the DT does not really help, IMHO.
>>
>> Well, except if you want to use external clocks for ethernet or CAN. Also you
>> don't need to change the dtsi either if you have a separate node for each clock.
>>
>> To avoid misunderstandings let me clarify that I don't want to have one node
>> per clock, but rather one node per clock block (or whatever they are called).
>> The combination of input select + divider + output gate. E.g. the UART clock
>> block with it's 3 inputs and two outputs.
>>
>> Also the APER clocks should probably be one node with 24 outputs.
> Okay, so instead of having one block encapsulating all clocks, you want it on a finer granularity. I don't see huge differences why that should be advantageous? It would just mean to create several blocks with their custom DT bindings instead of a single one. Just the abstraction level would be a little different.

The DT is supposed to describe the hardware, not the software. These are the
basic blocks. The are independent of each other and mostly orthogonal. It's the
same IP block instantiated a couple of times next to each other (sometimes with
slightly different parameters). They just happen to have their register mapped
in the same IO region. The SLCR is just a container which contains all these
blocks. Very much the same way the whole SoC is a container which contains the
different peripheral blocks, etc. By you rationale we could argue, why do we
need a dt node for each peripheral and not just simply one ZYNQ node?

- Lars

RFC: Zynq Clock Controller

[Sören Brinkmann]

On Fri, Mar 08, 2013 at 08:12:20AM +0100, Lars-Peter Clausen wrote:
> On 03/08/2013 12:25 AM, S?ren Brinkmann wrote:
> > On Thu, Mar 07, 2013 at 11:02:58PM +0100, Lars-Peter Clausen wrote:
> >> On 03/07/2013 08:11 PM, S?ren Brinkmann wrote:
> >>> On Thu, Mar 07, 2013 at 10:36:35AM +0100, Lars-Peter Clausen wrote:
> >>>> On 03/06/2013 06:27 PM, S?ren Brinkmann wrote:
> >>>>> Hi Jan,
> >>>>>  
> >>>>> what a small world. Good to hear from you.
> >>>>>
> >>>>> On Wed, Mar 06, 2013 at 12:51:21PM +0100, Jan L?bbe wrote:
> >>>>>> Hi S?ren,
> >>>>>>
> >>>>>> On Tue, 2013-03-05 at 12:04 -0800, S?ren Brinkmann wrote:
> >>>>>>> For this reasons, I'd like to propose moving Zynq into the same
> >>>>>>> direction. I.e. adding a clock controller with the following DT
> >>>>>>> description (details may change but the general idea should become
> >>>>>>> clear):
> >>>>>>> 	clkc: clkc {
> >>>>>>>                 #clock-cells = <1>;
> >>>>>>>                 compatible = "xlnx,ps7-clkc";
> >>>>>>>                 ps_clk_frequency = <33333333>;	# board x-tal
> >>>>>>>                 # optional props
> >>>>>>>                 gem0_emio_clk_freq = <125000000>;
> >>>>>>>                 gem1_emio_clk_freq = <50000000>;
> >>>>>>>                 can_mio_clk_freq_xx = <1234>; # this is possible 54 times with xx = 00..53
> >>>>>>>         };
> >>>>
> >>>> I definitely prefer the way it is right now in upstream, where we have one
> >>>> dt node per clock. It is more descriptive and also more extensible. And you
> >>>> also don't have to remember the clock index, and can use the phandle
> >>>> directly instead.
> >>> The problems I see with that are:
> >>>  - with the clock controller we can model the clock tree as we need without changing the DT all the time
> >>
> >> When do we need to change the clock tree? The clock tree is pretty much fixed
> >> in hardware. And the DT describes the hardware, so I don't so a problem there.
> >>
> >>>  - w/o a clock controller there is no block which has knowledge of the whole clock tree (unless we parse the DT), and could conduct reparent operations and similar
> >>
> >> The clk framework builds a representation of the clock tree. Each clock should
> >> be able to handle re-parenting on it's own, without knowing about the other
> >> clocks in the tree, the parent is selected by a field in the clocks register.
> >> It doesn't even have to know who the parents are, the clk framework will take
> >> care of all of this and just say, ok, switch your input clock to X, where X is
> >> a simple integer number The clk framework will also take care of re-calculating
> >> all the updates frequencies after re-parenting.
> > Nope, clk_set_parent() takes two 'struct *clk' as argument. So, you have to have those of the clock to change and all its parents. A device driver for example, which gets a clock through clk_get() does not have that information and should not, since it should not have to be aware of the SOC's clock hierarchy, IMHO.
> > 
> 
> Yes, you global clk_set_parent() method takes two clk structs. But the clock
> framework takes care of all the magic of mapping that second clk struct to a
> integer number, based on the clks parent list. So your set_parent callback in
> your clk_ops takes as parameters your clk and the index of the parent.
Right, but who/what in your scheme is supposed to call the high level API function requiring knowledge about all those 'struct *clk' and their hierarchy? This is where I think a big block has the advantage since it has a holistic view on the clock tree.

> 
> >>
> >>>  - once the clock controller is properly defined the clock connection should be contained in a dtsi which never changes. So, regarding remembering outputs, I don't see a problem. I rather see issues with having a pile of clocks described in the DT. I have currently 44 outputs proposed, and to model the whole tree a lot of more clocks are used (I started modeling everything with the clock primitives and removed custom clock implementations, except for the PLLs). Having all those in the DT does not really help, IMHO.
> >>
> >> Well, except if you want to use external clocks for ethernet or CAN. Also you
> >> don't need to change the dtsi either if you have a separate node for each clock.
> >>
> >> To avoid misunderstandings let me clarify that I don't want to have one node
> >> per clock, but rather one node per clock block (or whatever they are called).
> >> The combination of input select + divider + output gate. E.g. the UART clock
> >> block with it's 3 inputs and two outputs.
> >>
> >> Also the APER clocks should probably be one node with 24 outputs.
> > Okay, so instead of having one block encapsulating all clocks, you want it on a finer granularity. I don't see huge differences why that should be advantageous? It would just mean to create several blocks with their custom DT bindings instead of a single one. Just the abstraction level would be a little different.
> 
> The DT is supposed to describe the hardware, not the software. These are the
> basic blocks. The are independent of each other and mostly orthogonal. It's the
> same IP block instantiated a couple of times next to each other (sometimes with
> slightly different parameters). They just happen to have their register mapped
> in the same IO region. The SLCR is just a container which contains all these
> blocks. Very much the same way the whole SoC is a container which contains the
> different peripheral blocks, etc. By you rationale we could argue, why do we
> need a dt node for each peripheral and not just simply one ZYNQ node?
I see us ending up here with custom clock blocks for each and every peripheral. They all look similar on first look but have all their slight differences. So, it's either different implementations or having a lot of DT properties to make a single implementation work for all the variations. That's why I prefer the single controller solution.
The DT description will always be an abstraction. You prefer grouping clocks for functional units, whereas I put them all into the same drawer labeled 'clocks'.

	S?ren

RFC: Zynq Clock Controller

[Lars-Peter Clausen]

On 03/08/2013 06:38 PM, S?ren Brinkmann wrote:
> On Fri, Mar 08, 2013 at 08:12:20AM +0100, Lars-Peter Clausen wrote:
>> On 03/08/2013 12:25 AM, S?ren Brinkmann wrote:
>>> On Thu, Mar 07, 2013 at 11:02:58PM +0100, Lars-Peter Clausen wrote:
>>>> On 03/07/2013 08:11 PM, S?ren Brinkmann wrote:
>>>>> On Thu, Mar 07, 2013 at 10:36:35AM +0100, Lars-Peter Clausen wrote:
>>>>>> On 03/06/2013 06:27 PM, S?ren Brinkmann wrote:
>>>>>>> Hi Jan,
>>>>>>>  
>>>>>>> what a small world. Good to hear from you.
>>>>>>>
>>>>>>> On Wed, Mar 06, 2013 at 12:51:21PM +0100, Jan L?bbe wrote:
>>>>>>>> Hi S?ren,
>>>>>>>>
>>>>>>>> On Tue, 2013-03-05 at 12:04 -0800, S?ren Brinkmann wrote:
>>>>>>>>> For this reasons, I'd like to propose moving Zynq into the same
>>>>>>>>> direction. I.e. adding a clock controller with the following DT
>>>>>>>>> description (details may change but the general idea should become
>>>>>>>>> clear):
>>>>>>>>> 	clkc: clkc {
>>>>>>>>>                 #clock-cells = <1>;
>>>>>>>>>                 compatible = "xlnx,ps7-clkc";
>>>>>>>>>                 ps_clk_frequency = <33333333>;	# board x-tal
>>>>>>>>>                 # optional props
>>>>>>>>>                 gem0_emio_clk_freq = <125000000>;
>>>>>>>>>                 gem1_emio_clk_freq = <50000000>;
>>>>>>>>>                 can_mio_clk_freq_xx = <1234>; # this is possible 54 times with xx = 00..53
>>>>>>>>>         };
>>>>>>
>>>>>> I definitely prefer the way it is right now in upstream, where we have one
>>>>>> dt node per clock. It is more descriptive and also more extensible. And you
>>>>>> also don't have to remember the clock index, and can use the phandle
>>>>>> directly instead.
>>>>> The problems I see with that are:
>>>>>  - with the clock controller we can model the clock tree as we need without changing the DT all the time
>>>>
>>>> When do we need to change the clock tree? The clock tree is pretty much fixed
>>>> in hardware. And the DT describes the hardware, so I don't so a problem there.
>>>>
>>>>>  - w/o a clock controller there is no block which has knowledge of the whole clock tree (unless we parse the DT), and could conduct reparent operations and similar
>>>>
>>>> The clk framework builds a representation of the clock tree. Each clock should
>>>> be able to handle re-parenting on it's own, without knowing about the other
>>>> clocks in the tree, the parent is selected by a field in the clocks register.
>>>> It doesn't even have to know who the parents are, the clk framework will take
>>>> care of all of this and just say, ok, switch your input clock to X, where X is
>>>> a simple integer number The clk framework will also take care of re-calculating
>>>> all the updates frequencies after re-parenting.
>>> Nope, clk_set_parent() takes two 'struct *clk' as argument. So, you have to have those of the clock to change and all its parents. A device driver for example, which gets a clock through clk_get() does not have that information and should not, since it should not have to be aware of the SOC's clock hierarchy, IMHO.
>>>
>>
>> Yes, you global clk_set_parent() method takes two clk structs. But the clock
>> framework takes care of all the magic of mapping that second clk struct to a
>> integer number, based on the clks parent list. So your set_parent callback in
>> your clk_ops takes as parameters your clk and the index of the parent.
> Right, but who/what in your scheme is supposed to call the high level API function requiring knowledge about all those 'struct *clk' and their hierarchy? This is where I think a big block has the advantage since it has a holistic view on the clock tree.
> 

How would you use the clock controller driver to do re-parent operations? I
really can't see how it will help to implement re-parent operations.

>>
>>>>
>>>>>  - once the clock controller is properly defined the clock connection should be contained in a dtsi which never changes. So, regarding remembering outputs, I don't see a problem. I rather see issues with having a pile of clocks described in the DT. I have currently 44 outputs proposed, and to model the whole tree a lot of more clocks are used (I started modeling everything with the clock primitives and removed custom clock implementations, except for the PLLs). Having all those in the DT does not really help, IMHO.
>>>>
>>>> Well, except if you want to use external clocks for ethernet or CAN. Also you
>>>> don't need to change the dtsi either if you have a separate node for each clock.
>>>>
>>>> To avoid misunderstandings let me clarify that I don't want to have one node
>>>> per clock, but rather one node per clock block (or whatever they are called).
>>>> The combination of input select + divider + output gate. E.g. the UART clock
>>>> block with it's 3 inputs and two outputs.
>>>>
>>>> Also the APER clocks should probably be one node with 24 outputs.
>>> Okay, so instead of having one block encapsulating all clocks, you want it on a finer granularity. I don't see huge differences why that should be advantageous? It would just mean to create several blocks with their custom DT bindings instead of a single one. Just the abstraction level would be a little different.
>>
>> The DT is supposed to describe the hardware, not the software. These are the
>> basic blocks. The are independent of each other and mostly orthogonal. It's the
>> same IP block instantiated a couple of times next to each other (sometimes with
>> slightly different parameters). They just happen to have their register mapped
>> in the same IO region. The SLCR is just a container which contains all these
>> blocks. Very much the same way the whole SoC is a container which contains the
>> different peripheral blocks, etc. By you rationale we could argue, why do we
>> need a dt node for each peripheral and not just simply one ZYNQ node?
> I see us ending up here with custom clock blocks for each and every peripheral. They all look similar on first look but have all their slight differences. So, it's either different implementations or having a lot of DT properties to make a single implementation work for all the variations. That's why I prefer the single controller solution.
> The DT description will always be an abstraction. You prefer grouping clocks for functional units, whereas I put them all into the same drawer labeled 'clocks'.

So how many parameters are there?
- Number of input clocks: We get these by listing the parent clocks in the
clocks property.
- Number of output clocks: We get this through the output-clock-names
  property.
- Whether the output clocks can be gated, this requires a custom property.
- Number of 6bit dividers in the chain, can be 0, 1 or 2. Needs a custom
  property as well

So with these 4 properties you can describe ~90% of the clocks found on the
Zynq.

And then there are the CAN and GEM clocks, which probably require custom
clock implementation since they are kind of special.

Your approach ends up with hundreds of lines of open-coding the
instantiation of all the clock gates and dividers found in the slcr. That's
just madness.

- Lars