RFC v2: Zynq Clock Controller

RFC v2: Zynq Clock Controller

[Sören Brinkmann]

Hi,

I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq:

Required properties:
 - #clock-cells : Must be 1
 - compatible : "xlnx,ps7-clkc"  (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline)
 - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
                     (usually 33 MHz oscillators are used for Zynq platforms)
 - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below.

Optional properties:
 - clocks : as described in the clock bindings
 - clock-names : as described in the clock bindings

Clock inputs:
The following strings are optional parameters to the 'clock-names' property in
order to provide optional (E)MIO clock sources.
 - swdt_ext_clk
 - gem0_emio_clk
 - gem1_emio_clk
 - mio_clk_XX          # with XX = 00..53

Example:
        clkc: clkc {
                #clock-cells = <1>;
                compatible = "xlnx,ps7-clkc";
                ps-clk-frequency = <33333333>;
                clock-output-names = "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", "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", "swdt", "dbg_trc", "dbg_apb";  /* long list... explanation below */
                /* optional props */
                clocks = <&clkc 16>, <&clk_foo>;
                clock-names = "gem1_emio_clk", "can_mio_clk_23";
        };

With this revised bindings arbitrary upstream and downstream clock providers should be supported and it's also possible to loop back an output as input. The downside of supporting this is, that I don't see a way around explicitly listing the clock output names in the DT.
The reason for this is, that a downstream clock provider should use of_clk_get_parent_name() to obtain its parent clock name. For a block with multiple outputs of_clk_get_parent_name() can return a valid clock name only when 'clock-output-names' is present.
Probably the fclks are the only realistic use case to become parent of downstream clock providers, but I could imagine that e.g. a device driver like UART wants to use the CCF to model its internal clocks, hence it would require its parent clock name. Even though a device driver could use clk_get_parent() and __clk_get_name(), of_clk_get_parent_name() should probably work as well. I simply have a bad feeling about breaking of_clk_get_parent_name() for any clock.
But after all, I'm open for finding a better solution for this.


Similar, inputs for optional clock sources through (E)MIO pins can be defined as described in the clock bindings using the 'clocks' and 'clock-names' properties, with 'clock-names' being an arbitrary subset of the documented names. The actual parent clock names are internally resolved using of_clk_get_parent_name().


Regarding this monolithic solution versus a finer granular split:

On cost of more complex probing we would also have:
 - one clock driver covering the peripheral clocks
 - one for the CPU clock domain
 - one for the DDR clock domain
 - one for GEM
 - one for CAN
 - one for the APER clks
 - one for the PLLs
 - one for fclks
 - probably one for the debug clocks (not sure whether we need those)

Except for the peripheral one and probably the fclk, they would all be instantiated just once. So, there is not a lot of reuse going on.
Fclks I would probably also rather put into one driver with four outputs instead of instantiating a single output driver multiple times. Same for PLLs.

And then there is e.g. a mux for the system watchdog input which doesn't really fit anywhere and it would be pretty much ridiculous to have another clock driver just for that one and it would also become "hidden" in one of the others.

In my opinion that's just not necessary. We would create a bunch of clock drivers including DT bindings for them, probing would become more complex and it doesn't really help with the probe/naming issue. So, I don't think it's beneficial to go down that road.

The monolithic solution would need one custom driver for the PLLs, DT bindings wouldn't be required for it though. Everything else should be internally described using the clock-primitives.

Other than having a much simpler probe and init process, I still think it might be beneficial to have this monolithic block with a holistic view of the clock tree. For suspend e.g. I think the clock controller could export functions like zynq_clkc_(suspend|resume) and then the controller handles the PLL bypassing/shutdown.
Regarding full dynamic reparenting, I don't know how exactly that could work, but with the clock controller there is at least a block where that intelligence would be going and which has knowledge of all the 'struct clk *' required to reparent clocks.

Regarding the DT description, it is probably controversial what is considered better. I, like the Tegra folks, think having one clock controller in there is fine and hides irrelevant implementation details.

        Regards,
        Sören


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Re: RFC v2: Zynq Clock Controller

[Lars-Peter Clausen]

On 03/21/2013 12:56 AM, Sören Brinkmann wrote:
> Hi,
> 
> I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq:
> 
> Required properties:
>  - #clock-cells : Must be 1
>  - compatible : "xlnx,ps7-clkc"  (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline)
>  - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
>                      (usually 33 MHz oscillators are used for Zynq platforms)
>  - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below.
> 
> Optional properties:
>  - clocks : as described in the clock bindings
>  - clock-names : as described in the clock bindings
> 
> Clock inputs:
> The following strings are optional parameters to the 'clock-names' property in
> order to provide optional (E)MIO clock sources.
>  - swdt_ext_clk
>  - gem0_emio_clk
>  - gem1_emio_clk
>  - mio_clk_XX          # with XX = 00..53
> 
> Example:
>         clkc: clkc {
>                 #clock-cells = <1>;
>                 compatible = "xlnx,ps7-clkc";
>                 ps-clk-frequency = <33333333>;

The input frequency should be a clock as well.

>                 clock-output-names = "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", "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", "swdt", "dbg_trc", "dbg_apb";  /* long list... explanation below */
>                 /* optional props */
>                 clocks = <&clkc 16>, <&clk_foo>;
>                 clock-names = "gem1_emio_clk", "can_mio_clk_23";
>         };
> 
> With this revised bindings arbitrary upstream and downstream clock providers should be supported and it's also possible to loop back an output as input. The downside of supporting this is, that I don't see a way around explicitly listing the clock output names in the DT.
> The reason for this is, that a downstream clock provider should use of_clk_get_parent_name() to obtain its parent clock name. For a block with multiple outputs of_clk_get_parent_name() can return a valid clock name only when 'clock-output-names' is present.
> Probably the fclks are the only realistic use case to become parent of downstream clock providers, but I could imagine that e.g. a device driver like UART wants to use the CCF to model its internal clocks, hence it would require its parent clock name. Even though a device driver could use clk_get_parent() and __clk_get_name(), of_clk_get_parent_name() should probably work as well. I simply have a bad feeling about breaking of_clk_get_parent_name() for any clock.
> But after all, I'm open for finding a better solution for this.
> 
> 
> Similar, inputs for optional clock sources through (E)MIO pins can be defined as described in the clock bindings using the 'clocks' and 'clock-names' properties, with 'clock-names' being an arbitrary subset of the documented names. The actual parent clock names are internally resolved using of_clk_get_parent_name().
> 
> 
> Regarding this monolithic solution versus a finer granular split:
> 
> On cost of more complex probing we would also have:
>  - one clock driver covering the peripheral clocks
>  - one for the CPU clock domain
>  - one for the DDR clock domain
>  - one for GEM
>  - one for CAN
>  - one for the APER clks
>  - one for the PLLs
>  - one for fclks

fclks are the same as peripheral clocks except for the gate bit, as far as I
can see.

>  - probably one for the debug clocks (not sure whether we need those)
> 
> Except for the peripheral one and probably the fclk, they would all be instantiated just once. So, there is not a lot of reuse going on.

PLLs are going to be instantiated multiple times as well.

> Fclks I would probably also rather put into one driver with four outputs instead of instantiating a single output driver multiple times. Same for PLLs.
> 
> And then there is e.g. a mux for the system watchdog input which doesn't really fit anywhere and it would be pretty much ridiculous to have another clock driver just for that one and it would also become "hidden" in one of the others.
> 
> In my opinion that's just not necessary. We would create a bunch of clock drivers including DT bindings for them, probing would become more complex and it doesn't really help with the probe/naming issue. So, I don't think it's beneficial to go down that road.
> 
> The monolithic solution would need one custom driver for the PLLs, DT bindings wouldn't be required for it though. Everything else should be internally described using the clock-primitives.
> 
> Other than having a much simpler probe and init process, I still think it might be beneficial to have this monolithic block with a holistic view of the clock tree. For suspend e.g. I think the clock controller could export functions like zynq_clkc_(suspend|resume) and then the controller handles the PLL bypassing/shutdown.
> Regarding full dynamic reparenting, I don't know how exactly that could work, but with the clock controller there is at least a block where that intelligence would be going and which has knowledge of all the 'struct clk *' required to reparent clocks.
> 
> Regarding the DT description, it is probably controversial what is considered better. I, like the Tegra folks, think having one clock controller in there is fine and hides irrelevant implementation details.
> 

I still don't like the monolithic solution. From my point of view it is
architecturally inferior, it is a bad abstraction. You argue that for a
non-monolithic version you'd have to implement a clock driver for each
different type of clock. But you still have to do this for the monolithic
version, they'd probably just end up in one huge messy file. Unless you are
going to duplicate huge amounts of lines you'd probably have functions like
add_gem_clk, add_peripheral_clk, add_pll_clk and so on in your monolithic drivers.

The SLCR is a virtual construct, which groups the clock units together. But the
clock units are the actual entities. E.g. imagine a Zync 2.0, it would probably
reuse the same clock units, but there would quite likely be different clocks
mapped at different addresses. If you choose a non-monolithic implementation
you'd just have to update your devicetree to make this work, with a monolithic
version you'd have to add a almost identical driver for the v2.

- Lars

Re: RFC v2: Zynq Clock Controller

[Sören Brinkmann]

Hi Lars,

On Thu, Mar 21, 2013 at 07:32:52PM +0100, Lars-Peter Clausen wrote:
> On 03/21/2013 12:56 AM, Sören Brinkmann wrote:
> > Hi,
> > 
> > I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq:
> > 
> > Required properties:
> >  - #clock-cells : Must be 1
> >  - compatible : "xlnx,ps7-clkc"  (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline)
> >  - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
> >                      (usually 33 MHz oscillators are used for Zynq platforms)
> >  - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below.
> > 
> > Optional properties:
> >  - clocks : as described in the clock bindings
> >  - clock-names : as described in the clock bindings
> > 
> > Clock inputs:
> > The following strings are optional parameters to the 'clock-names' property in
> > order to provide optional (E)MIO clock sources.
> >  - swdt_ext_clk
> >  - gem0_emio_clk
> >  - gem1_emio_clk
> >  - mio_clk_XX          # with XX = 00..53
> > 
> > Example:
> >         clkc: clkc {
> >                 #clock-cells = <1>;
> >                 compatible = "xlnx,ps7-clkc";
> >                 ps-clk-frequency = <33333333>;
> 
> The input frequency should be a clock as well.
Again, monolithic vs split. I don't see a reason not to just internally
call clk_register_fixed_rate(). That way its children do not have to
cope with a variable name for the xtal.
Also, with my proposal 'clocks' and 'clock-names' would be purely
optional properties, only required if optional external inputs are
present. Having the xtal defined externally would add mandatory entries for
those props.

> 
> >                 clock-output-names = "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", "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", "swdt", "dbg_trc", "dbg_apb";  /* long list... explanation below */
> >                 /* optional props */
> >                 clocks = <&clkc 16>, <&clk_foo>;
> >                 clock-names = "gem1_emio_clk", "can_mio_clk_23";
> >         };
> > 
> > With this revised bindings arbitrary upstream and downstream clock providers should be supported and it's also possible to loop back an output as input. The downside of supporting this is, that I don't see a way around explicitly listing the clock output names in the DT.
> > The reason for this is, that a downstream clock provider should use of_clk_get_parent_name() to obtain its parent clock name. For a block with multiple outputs of_clk_get_parent_name() can return a valid clock name only when 'clock-output-names' is present.
> > Probably the fclks are the only realistic use case to become parent of downstream clock providers, but I could imagine that e.g. a device driver like UART wants to use the CCF to model its internal clocks, hence it would require its parent clock name. Even though a device driver could use clk_get_parent() and __clk_get_name(), of_clk_get_parent_name() should probably work as well. I simply have a bad feeling about breaking of_clk_get_parent_name() for any clock.
> > But after all, I'm open for finding a better solution for this.
> > 
> > 
> > Similar, inputs for optional clock sources through (E)MIO pins can be defined as described in the clock bindings using the 'clocks' and 'clock-names' properties, with 'clock-names' being an arbitrary subset of the documented names. The actual parent clock names are internally resolved using of_clk_get_parent_name().
> > 
> > 
> > Regarding this monolithic solution versus a finer granular split:
> > 
> > On cost of more complex probing we would also have:
> >  - one clock driver covering the peripheral clocks
> >  - one for the CPU clock domain
> >  - one for the DDR clock domain
> >  - one for GEM
> >  - one for CAN
> >  - one for the APER clks
> >  - one for the PLLs
> >  - one for fclks
> 
> fclks are the same as peripheral clocks except for the gate bit, as far as I
> can see.
And that makes them quite different, since they have to access multiple
registers instead of a single one. Also, the fclks have two dividers.
If you want to cover all of those with a single driver, you need a
plethora of arguments/properties to catch the small differences.

> 
> >  - probably one for the debug clocks (not sure whether we need those)
> > 
> > Except for the peripheral one and probably the fclk, they would all be instantiated just once. So, there is not a lot of reuse going on.
> 
> PLLs are going to be instantiated multiple times as well.
As mentioned in the very next sentence, I rather see a single driver
with multiple outputs. Take suspend: My plan is to have a few functions
like zynq_clk_(suspend|resume). That should take care of bypassing
shutting down the PLLs (and probably more). Therefore it's easier to
have them all in a single driver.
And if it turned out, that other clocks require special handling for
such system level functions, that could be addressed that way too with
the monolithic approach.

> 
> > Fclks I would probably also rather put into one driver with four outputs instead of instantiating a single output driver multiple times. Same for PLLs.
> > 
> > And then there is e.g. a mux for the system watchdog input which doesn't really fit anywhere and it would be pretty much ridiculous to have another clock driver just for that one and it would also become "hidden" in one of the others.
> > 
> > In my opinion that's just not necessary. We would create a bunch of clock drivers including DT bindings for them, probing would become more complex and it doesn't really help with the probe/naming issue. So, I don't think it's beneficial to go down that road.
> > 
> > The monolithic solution would need one custom driver for the PLLs, DT bindings wouldn't be required for it though. Everything else should be internally described using the clock-primitives.
> > 
> > Other than having a much simpler probe and init process, I still think it might be beneficial to have this monolithic block with a holistic view of the clock tree. For suspend e.g. I think the clock controller could export functions like zynq_clkc_(suspend|resume) and then the controller handles the PLL bypassing/shutdown.
> > Regarding full dynamic reparenting, I don't know how exactly that could work, but with the clock controller there is at least a block where that intelligence would be going and which has knowledge of all the 'struct clk *' required to reparent clocks.
> > 
> > Regarding the DT description, it is probably controversial what is considered better. I, like the Tegra folks, think having one clock controller in there is fine and hides irrelevant implementation details.
> > 
> 
> I still don't like the monolithic solution. From my point of view it is
> architecturally inferior, it is a bad abstraction. You argue that for a
> non-monolithic version you'd have to implement a clock driver for each
> different type of clock. But you still have to do this for the monolithic
> version, they'd probably just end up in one huge messy file. Unless you are
> going to duplicate huge amounts of lines you'd probably have functions like
> add_gem_clk, add_peripheral_clk, add_pll_clk and so on in your monolithic drivers.
It probably makes sense to differ between having custom drivers and
describing the whole clock tree in DT.

From reusability perspective, it may make sense to factor out some code
in drivers. IMHO, this does only apply to the peripheral clocks, since
everything else isn't reused and/or quite Zynq specific.
But a monolithic approach would not even prevent this. You could just
transparently change the implementation: Just add a clock driver,
replace the original code in the controller to use the new driver
instead and you're good. No need to touch DT bindings.

In Zynq a peripheral clock is essentially described by:
        clk = clk_register_mux(NULL, mux_name, parents, 4, 0,                    
                        clk_ctrl, 4, 2, 0, lock);                                
                                                                                 
        clk = clk_register_divider(NULL, div_name, mux_name, 0, clk_ctrl, 8, 6,  
                        CLK_DIVIDER_ONE_BASED, lock);                            
                                                                                 
        clks[clk0] = clk_register_gate(NULL, clk_name0, div_name,                
                        CLK_SET_RATE_PARENT, clk_ctrl, 0, 0, lock);              

If we wrapped this by a driver, any code outside of that driver couldn't
change the mux setting, since its struct clk* is not exposed outside.
To get around this we'd have to reimplement all the clock ops, to create a
clock which supports all possible operations.
In the monolithic approach we could simply remember that struct clk* and
work with it.

Bottom line: Factoring out some parts of the monolithic driver might
make sense for some parts. But it can be done later in a transparent way,
especially w/o touching DT bindings.


The other thing is describing the whole clock tree in DT:
That would force us to not only describe clocks for which it might make
sense, but also all Zynq specific clocks in custom drivers and DT
bindings and we gain nothing from it.

> 
> The SLCR is a virtual construct, which groups the clock units together. But the
> clock units are the actual entities. E.g. imagine a Zync 2.0, it would probably
> reuse the same clock units, but there would quite likely be different clocks
> mapped at different addresses. If you choose a non-monolithic implementation
> you'd just have to update your devicetree to make this work, with a monolithic
> version you'd have to add a almost identical driver for the v2.
Either way you'll end up with a lot of lines describing the hierarchy.
Either in the dts or in the clock driver.


I think, my problem is, that I do not yet know how certain functionality
will be implemented later and what exact use-cases have to be supported.
With the monolithic approach I keep all options open. We hopefully will
never have to touch its DT bindings again. And refactoring the code and
migrating it to use dedicated clock drivers should be transparent
changes, if deemed beneficial.
Furthermore, I have a driver, which has references to all strucht clk*
in the system and can work with the whole clock tree leveraging a system
level view. Separating things in smaller blocks might complicate things
if a use case requires this.
And finally pushing the whole hierarchy description into the DT seems to
be the most restrictive approach, without offering big rewards.

	Regards,
	Sören

Re: RFC v2: Zynq Clock Controller

[Lars-Peter Clausen]

Hi,

On 03/22/2013 11:41 PM, Sören Brinkmann wrote:
> Hi Lars,
> 
> On Thu, Mar 21, 2013 at 07:32:52PM +0100, Lars-Peter Clausen wrote:
>> On 03/21/2013 12:56 AM, Sören Brinkmann wrote:
>>> Hi,
>>>
>>> I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq:
>>>
>>> Required properties:
>>>  - #clock-cells : Must be 1
>>>  - compatible : "xlnx,ps7-clkc"  (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline)
>>>  - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
>>>                      (usually 33 MHz oscillators are used for Zynq platforms)
>>>  - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below.
>>>
>>> Optional properties:
>>>  - clocks : as described in the clock bindings
>>>  - clock-names : as described in the clock bindings
>>>
>>> Clock inputs:
>>> The following strings are optional parameters to the 'clock-names' property in
>>> order to provide optional (E)MIO clock sources.
>>>  - swdt_ext_clk
>>>  - gem0_emio_clk
>>>  - gem1_emio_clk
>>>  - mio_clk_XX          # with XX = 00..53
>>>
>>> Example:
>>>         clkc: clkc {
>>>                 #clock-cells = <1>;
>>>                 compatible = "xlnx,ps7-clkc";
>>>                 ps-clk-frequency = <33333333>;
>>
>> The input frequency should be a clock as well.
> Again, monolithic vs split. I don't see a reason not to just internally
> call clk_register_fixed_rate(). That way its children do not have to
> cope with a variable name for the xtal.
> Also, with my proposal 'clocks' and 'clock-names' would be purely
> optional properties, only required if optional external inputs are
> present. Having the xtal defined externally would add mandatory entries for
> those props.



> 
>>
>>>                 clock-output-names = "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", "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", "swdt", "dbg_trc", "dbg_apb";  /* long list... explanation below */
>>>                 /* optional props */
>>>                 clocks = <&clkc 16>, <&clk_foo>;
>>>                 clock-names = "gem1_emio_clk", "can_mio_clk_23";
>>>         };
>>>
>>> With this revised bindings arbitrary upstream and downstream clock providers should be supported and it's also possible to loop back an output as input. The downside of supporting this is, that I don't see a way around explicitly listing the clock output names in the DT.
>>> The reason for this is, that a downstream clock provider should use of_clk_get_parent_name() to obtain its parent clock name. For a block with multiple outputs of_clk_get_parent_name() can return a valid clock name only when 'clock-output-names' is present.
>>> Probably the fclks are the only realistic use case to become parent of downstream clock providers, but I could imagine that e.g. a device driver like UART wants to use the CCF to model its internal clocks, hence it would require its parent clock name. Even though a device driver could use clk_get_parent() and __clk_get_name(), of_clk_get_parent_name() should probably work as well. I simply have a bad feeling about breaking of_clk_get_parent_name() for any clock.
>>> But after all, I'm open for finding a better solution for this.
>>>
>>>
>>> Similar, inputs for optional clock sources through (E)MIO pins can be defined as described in the clock bindings using the 'clocks' and 'clock-names' properties, with 'clock-names' being an arbitrary subset of the documented names. The actual parent clock names are internally resolved using of_clk_get_parent_name().
>>>
>>>
>>> Regarding this monolithic solution versus a finer granular split:
>>>
>>> On cost of more complex probing we would also have:
>>>  - one clock driver covering the peripheral clocks
>>>  - one for the CPU clock domain
>>>  - one for the DDR clock domain
>>>  - one for GEM
>>>  - one for CAN
>>>  - one for the APER clks
>>>  - one for the PLLs
>>>  - one for fclks
>>
>> fclks are the same as peripheral clocks except for the gate bit, as far as I
>> can see.
> And that makes them quite different, since they have to access multiple
> registers instead of a single one. Also, the fclks have two dividers.
> If you want to cover all of those with a single driver, you need a
> plethora of arguments/properties to catch the small differences.
> 
>>
>>>  - probably one for the debug clocks (not sure whether we need those)
>>>
>>> Except for the peripheral one and probably the fclk, they would all be instantiated just once. So, there is not a lot of reuse going on.
>>
>> PLLs are going to be instantiated multiple times as well.
> As mentioned in the very next sentence, I rather see a single driver
> with multiple outputs. Take suspend: My plan is to have a few functions
> like zynq_clk_(suspend|resume). That should take care of bypassing
> shutting down the PLLs (and probably more). Therefore it's easier to
> have them all in a single driver.
> And if it turned out, that other clocks require special handling for
> such system level functions, that could be addressed that way too with
> the monolithic approach.
> 
>>
>>> Fclks I would probably also rather put into one driver with four outputs instead of instantiating a single output driver multiple times. Same for PLLs.
>>>
>>> And then there is e.g. a mux for the system watchdog input which doesn't really fit anywhere and it would be pretty much ridiculous to have another clock driver just for that one and it would also become "hidden" in one of the others.
>>>
>>> In my opinion that's just not necessary. We would create a bunch of clock drivers including DT bindings for them, probing would become more complex and it doesn't really help with the probe/naming issue. So, I don't think it's beneficial to go down that road.
>>>
>>> The monolithic solution would need one custom driver for the PLLs, DT bindings wouldn't be required for it though. Everything else should be internally described using the clock-primitives.
>>>
>>> Other than having a much simpler probe and init process, I still think it might be beneficial to have this monolithic block with a holistic view of the clock tree. For suspend e.g. I think the clock controller could export functions like zynq_clkc_(suspend|resume) and then the controller handles the PLL bypassing/shutdown.
>>> Regarding full dynamic reparenting, I don't know how exactly that could work, but with the clock controller there is at least a block where that intelligence would be going and which has knowledge of all the 'struct clk *' required to reparent clocks.
>>>
>>> Regarding the DT description, it is probably controversial what is considered better. I, like the Tegra folks, think having one clock controller in there is fine and hides irrelevant implementation details.
>>>
>>
>> I still don't like the monolithic solution. From my point of view it is
>> architecturally inferior, it is a bad abstraction. You argue that for a
>> non-monolithic version you'd have to implement a clock driver for each
>> different type of clock. But you still have to do this for the monolithic
>> version, they'd probably just end up in one huge messy file. Unless you are
>> going to duplicate huge amounts of lines you'd probably have functions like
>> add_gem_clk, add_peripheral_clk, add_pll_clk and so on in your monolithic drivers.
> It probably makes sense to differ between having custom drivers and
> describing the whole clock tree in DT.
> 
> From reusability perspective, it may make sense to factor out some code
> in drivers. IMHO, this does only apply to the peripheral clocks, since
> everything else isn't reused and/or quite Zynq specific.
> But a monolithic approach would not even prevent this. You could just
> transparently change the implementation: Just add a clock driver,
> replace the original code in the controller to use the new driver
> instead and you're good. No need to touch DT bindings.
> 
> In Zynq a peripheral clock is essentially described by:
>         clk = clk_register_mux(NULL, mux_name, parents, 4, 0,                    
>                         clk_ctrl, 4, 2, 0, lock);                                
>                                                                                  
>         clk = clk_register_divider(NULL, div_name, mux_name, 0, clk_ctrl, 8, 6,  
>                         CLK_DIVIDER_ONE_BASED, lock);                            
>                                                                                  
>         clks[clk0] = clk_register_gate(NULL, clk_name0, div_name,                
>                         CLK_SET_RATE_PARENT, clk_ctrl, 0, 0, lock);              
> 
> If we wrapped this by a driver, any code outside of that driver couldn't
> change the mux setting, since its struct clk* is not exposed outside.
> To get around this we'd have to reimplement all the clock ops, to create a
> clock which supports all possible operations.
> In the monolithic approach we could simply remember that struct clk* and
> work with it.
> 
> Bottom line: Factoring out some parts of the monolithic driver might
> make sense for some parts. But it can be done later in a transparent way,
> especially w/o touching DT bindings.
> 
> 
> The other thing is describing the whole clock tree in DT:
> That would force us to not only describe clocks for which it might make
> sense, but also all Zynq specific clocks in custom drivers and DT
> bindings and we gain nothing from it.
> 
>>
>> The SLCR is a virtual construct, which groups the clock units together. But the
>> clock units are the actual entities. E.g. imagine a Zync 2.0, it would probably
>> reuse the same clock units, but there would quite likely be different clocks
>> mapped at different addresses. If you choose a non-monolithic implementation
>> you'd just have to update your devicetree to make this work, with a monolithic
>> version you'd have to add a almost identical driver for the v2.
> Either way you'll end up with a lot of lines describing the hierarchy.
> Either in the dts or in the clock driver.
> 
> 
> I think, my problem is, that I do not yet know how certain functionality
> will be implemented later and what exact use-cases have to be supported.
> With the monolithic approach I keep all options open. We hopefully will
> never have to touch its DT bindings again. And refactoring the code and
> migrating it to use dedicated clock drivers should be transparent
> changes, if deemed beneficial.
> Furthermore, I have a driver, which has references to all strucht clk*
> in the system and can work with the whole clock tree leveraging a system
> level view. Separating things in smaller blocks might complicate things
> if a use case requires this.
> And finally pushing the whole hierarchy description into the DT seems to
> be the most restrictive approach, without offering big rewards.

I don't see those restrictions you see with a dt binding which has nodes for
each clock block. You seem to be under the impression, that each device tree
node requires its own driver or device. This is not the case, have you looked
at how the current upstream zynq clock support is implemented? This is still
one monolithic driver, but there are multiple dt nodes, one for each clock
block. This leads to a very clean and well structured code and also nicely
structured dt, which represents the tree as it is in hardware. If I understand
you correctly what you suggest is that you basically want to copy 'n paste the
same 10 lines to instantiate a peripheral clock, which you mentioned above,
again and again. This will be quite messy.

- Lars

Re: RFC v2: Zynq Clock Controller

[Sören Brinkmann]

Hi Lars,

On Mon, Mar 25, 2013 at 03:46:35PM +0100, Lars-Peter Clausen wrote:
> Hi,
> 
> On 03/22/2013 11:41 PM, Sören Brinkmann wrote:
> > Hi Lars,
> > 
> > On Thu, Mar 21, 2013 at 07:32:52PM +0100, Lars-Peter Clausen wrote:
> >> On 03/21/2013 12:56 AM, Sören Brinkmann wrote:
> >>> Hi,
> >>>
> >>> I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq:
> >>>
> >>> Required properties:
> >>>  - #clock-cells : Must be 1
> >>>  - compatible : "xlnx,ps7-clkc"  (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline)
> >>>  - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
> >>>                      (usually 33 MHz oscillators are used for Zynq platforms)
> >>>  - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below.
> >>>
> >>> Optional properties:
> >>>  - clocks : as described in the clock bindings
> >>>  - clock-names : as described in the clock bindings
> >>>
> >>> Clock inputs:
> >>> The following strings are optional parameters to the 'clock-names' property in
> >>> order to provide optional (E)MIO clock sources.
> >>>  - swdt_ext_clk
> >>>  - gem0_emio_clk
> >>>  - gem1_emio_clk
> >>>  - mio_clk_XX          # with XX = 00..53
> >>>
> >>> Example:
> >>>         clkc: clkc {
> >>>                 #clock-cells = <1>;
> >>>                 compatible = "xlnx,ps7-clkc";
> >>>                 ps-clk-frequency = <33333333>;
> >>
> >> The input frequency should be a clock as well.
> > Again, monolithic vs split. I don't see a reason not to just internally
> > call clk_register_fixed_rate(). That way its children do not have to
> > cope with a variable name for the xtal.
> > Also, with my proposal 'clocks' and 'clock-names' would be purely
> > optional properties, only required if optional external inputs are
> > present. Having the xtal defined externally would add mandatory entries for
> > those props.
> 
> 
> 
> > 
> >>
> >>>                 clock-output-names = "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", "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", "swdt", "dbg_trc", "dbg_apb";  /* long list... explanation below */
> >>>                 /* optional props */
> >>>                 clocks = <&clkc 16>, <&clk_foo>;
> >>>                 clock-names = "gem1_emio_clk", "can_mio_clk_23";
> >>>         };
> >>>
> >>> With this revised bindings arbitrary upstream and downstream clock providers should be supported and it's also possible to loop back an output as input. The downside of supporting this is, that I don't see a way around explicitly listing the clock output names in the DT.
> >>> The reason for this is, that a downstream clock provider should use of_clk_get_parent_name() to obtain its parent clock name. For a block with multiple outputs of_clk_get_parent_name() can return a valid clock name only when 'clock-output-names' is present.
> >>> Probably the fclks are the only realistic use case to become parent of downstream clock providers, but I could imagine that e.g. a device driver like UART wants to use the CCF to model its internal clocks, hence it would require its parent clock name. Even though a device driver could use clk_get_parent() and __clk_get_name(), of_clk_get_parent_name() should probably work as well. I simply have a bad feeling about breaking of_clk_get_parent_name() for any clock.
> >>> But after all, I'm open for finding a better solution for this.
> >>>
> >>>
> >>> Similar, inputs for optional clock sources through (E)MIO pins can be defined as described in the clock bindings using the 'clocks' and 'clock-names' properties, with 'clock-names' being an arbitrary subset of the documented names. The actual parent clock names are internally resolved using of_clk_get_parent_name().
> >>>
> >>>
> >>> Regarding this monolithic solution versus a finer granular split:
> >>>
> >>> On cost of more complex probing we would also have:
> >>>  - one clock driver covering the peripheral clocks
> >>>  - one for the CPU clock domain
> >>>  - one for the DDR clock domain
> >>>  - one for GEM
> >>>  - one for CAN
> >>>  - one for the APER clks
> >>>  - one for the PLLs
> >>>  - one for fclks
> >>
> >> fclks are the same as peripheral clocks except for the gate bit, as far as I
> >> can see.
> > And that makes them quite different, since they have to access multiple
> > registers instead of a single one. Also, the fclks have two dividers.
> > If you want to cover all of those with a single driver, you need a
> > plethora of arguments/properties to catch the small differences.
> > 
> >>
> >>>  - probably one for the debug clocks (not sure whether we need those)
> >>>
> >>> Except for the peripheral one and probably the fclk, they would all be instantiated just once. So, there is not a lot of reuse going on.
> >>
> >> PLLs are going to be instantiated multiple times as well.
> > As mentioned in the very next sentence, I rather see a single driver
> > with multiple outputs. Take suspend: My plan is to have a few functions
> > like zynq_clk_(suspend|resume). That should take care of bypassing
> > shutting down the PLLs (and probably more). Therefore it's easier to
> > have them all in a single driver.
> > And if it turned out, that other clocks require special handling for
> > such system level functions, that could be addressed that way too with
> > the monolithic approach.
> > 
> >>
> >>> Fclks I would probably also rather put into one driver with four outputs instead of instantiating a single output driver multiple times. Same for PLLs.
> >>>
> >>> And then there is e.g. a mux for the system watchdog input which doesn't really fit anywhere and it would be pretty much ridiculous to have another clock driver just for that one and it would also become "hidden" in one of the others.
> >>>
> >>> In my opinion that's just not necessary. We would create a bunch of clock drivers including DT bindings for them, probing would become more complex and it doesn't really help with the probe/naming issue. So, I don't think it's beneficial to go down that road.
> >>>
> >>> The monolithic solution would need one custom driver for the PLLs, DT bindings wouldn't be required for it though. Everything else should be internally described using the clock-primitives.
> >>>
> >>> Other than having a much simpler probe and init process, I still think it might be beneficial to have this monolithic block with a holistic view of the clock tree. For suspend e.g. I think the clock controller could export functions like zynq_clkc_(suspend|resume) and then the controller handles the PLL bypassing/shutdown.
> >>> Regarding full dynamic reparenting, I don't know how exactly that could work, but with the clock controller there is at least a block where that intelligence would be going and which has knowledge of all the 'struct clk *' required to reparent clocks.
> >>>
> >>> Regarding the DT description, it is probably controversial what is considered better. I, like the Tegra folks, think having one clock controller in there is fine and hides irrelevant implementation details.
> >>>
> >>
> >> I still don't like the monolithic solution. From my point of view it is
> >> architecturally inferior, it is a bad abstraction. You argue that for a
> >> non-monolithic version you'd have to implement a clock driver for each
> >> different type of clock. But you still have to do this for the monolithic
> >> version, they'd probably just end up in one huge messy file. Unless you are
> >> going to duplicate huge amounts of lines you'd probably have functions like
> >> add_gem_clk, add_peripheral_clk, add_pll_clk and so on in your monolithic drivers.
> > It probably makes sense to differ between having custom drivers and
> > describing the whole clock tree in DT.
> > 
> > From reusability perspective, it may make sense to factor out some code
> > in drivers. IMHO, this does only apply to the peripheral clocks, since
> > everything else isn't reused and/or quite Zynq specific.
> > But a monolithic approach would not even prevent this. You could just
> > transparently change the implementation: Just add a clock driver,
> > replace the original code in the controller to use the new driver
> > instead and you're good. No need to touch DT bindings.
> > 
> > In Zynq a peripheral clock is essentially described by:
> >         clk = clk_register_mux(NULL, mux_name, parents, 4, 0,                    
> >                         clk_ctrl, 4, 2, 0, lock);                                
> >                                                                                  
> >         clk = clk_register_divider(NULL, div_name, mux_name, 0, clk_ctrl, 8, 6,  
> >                         CLK_DIVIDER_ONE_BASED, lock);                            
> >                                                                                  
> >         clks[clk0] = clk_register_gate(NULL, clk_name0, div_name,                
> >                         CLK_SET_RATE_PARENT, clk_ctrl, 0, 0, lock);              
> > 
> > If we wrapped this by a driver, any code outside of that driver couldn't
> > change the mux setting, since its struct clk* is not exposed outside.
> > To get around this we'd have to reimplement all the clock ops, to create a
> > clock which supports all possible operations.
> > In the monolithic approach we could simply remember that struct clk* and
> > work with it.
> > 
> > Bottom line: Factoring out some parts of the monolithic driver might
> > make sense for some parts. But it can be done later in a transparent way,
> > especially w/o touching DT bindings.
> > 
> > 
> > The other thing is describing the whole clock tree in DT:
> > That would force us to not only describe clocks for which it might make
> > sense, but also all Zynq specific clocks in custom drivers and DT
> > bindings and we gain nothing from it.
> > 
> >>
> >> The SLCR is a virtual construct, which groups the clock units together. But the
> >> clock units are the actual entities. E.g. imagine a Zync 2.0, it would probably
> >> reuse the same clock units, but there would quite likely be different clocks
> >> mapped at different addresses. If you choose a non-monolithic implementation
> >> you'd just have to update your devicetree to make this work, with a monolithic
> >> version you'd have to add a almost identical driver for the v2.
> > Either way you'll end up with a lot of lines describing the hierarchy.
> > Either in the dts or in the clock driver.
> > 
> > 
> > I think, my problem is, that I do not yet know how certain functionality
> > will be implemented later and what exact use-cases have to be supported.
> > With the monolithic approach I keep all options open. We hopefully will
> > never have to touch its DT bindings again. And refactoring the code and
> > migrating it to use dedicated clock drivers should be transparent
> > changes, if deemed beneficial.
> > Furthermore, I have a driver, which has references to all strucht clk*
> > in the system and can work with the whole clock tree leveraging a system
> > level view. Separating things in smaller blocks might complicate things
> > if a use case requires this.
> > And finally pushing the whole hierarchy description into the DT seems to
> > be the most restrictive approach, without offering big rewards.
> 
> I don't see those restrictions you see with a dt binding which has nodes for
> each clock block. You seem to be under the impression, that each device tree
> node requires its own driver or device. This is not the case, have you looked
> at how the current upstream zynq clock support is implemented? This is still
> one monolithic driver, but there are multiple dt nodes, one for each clock
> block.
I thought the one "huge messy" file was one thing you wanted to avoid?
Also, how does it address my concern regarding inaccessible
'struct clk *'? A clk_register_foo() will only return a single struct
clk *. So, wrapping several clk_register() calls within one
clk_register_foo() makes all but the actual output inaccessible.
And if we do not directly call clk_register_foo() but do the probing
through DT the clock init code does not even get hold of that struct clk
*.

> This leads to a very clean and well structured code and also nicely
> structured dt, which represents the tree as it is in hardware. If I understand
> you correctly what you suggest is that you basically want to copy 'n paste the
> same 10 lines to instantiate a peripheral clock, which you mentioned above,
> again and again. This will be quite messy.
For the peripheral clocks you're right, there is some reuse possible.
For those I have a helper, like zynq_clk_register_periph(). But for pretty much
everything else I don't.

In conclusion, I see the DT approach limiting my ability to modify the
clock tree when implementing system level functionality. 
Probably quite a stretch, but imagine some smart power management code,
which might try to reparent all and every clock in the system to certain
PLLs. How would such a power manager get hold of all the struct clk * it
needs. In a system probing fully through DT we cannot get hold of those
and even less if we wrap several clocks in one clock_foo. Unless, we add
a dummy device which has every clock in the system as its input. And
that sounds even wackier than having a clock controller which provides
all the clocks to consumers.

	Sören

Re: RFC v2: Zynq Clock Controller

[Lars-Peter Clausen]

On 03/25/2013 06:08 PM, Sören Brinkmann wrote:
> Hi Lars,
> 
> On Mon, Mar 25, 2013 at 03:46:35PM +0100, Lars-Peter Clausen wrote:
>> Hi,
>>
>> On 03/22/2013 11:41 PM, Sören Brinkmann wrote:
>>> Hi Lars,
>>>
>>> On Thu, Mar 21, 2013 at 07:32:52PM +0100, Lars-Peter Clausen wrote:
>>>> On 03/21/2013 12:56 AM, Sören Brinkmann wrote:
>>>>> Hi,
>>>>>
>>>>> I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq:
>>>>>
>>>>> Required properties:
>>>>>  - #clock-cells : Must be 1
>>>>>  - compatible : "xlnx,ps7-clkc"  (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline)
>>>>>  - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
>>>>>                      (usually 33 MHz oscillators are used for Zynq platforms)
>>>>>  - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below.
>>>>>
>>>>> Optional properties:
>>>>>  - clocks : as described in the clock bindings
>>>>>  - clock-names : as described in the clock bindings
>>>>>
>>>>> Clock inputs:
>>>>> The following strings are optional parameters to the 'clock-names' property in
>>>>> order to provide optional (E)MIO clock sources.
>>>>>  - swdt_ext_clk
>>>>>  - gem0_emio_clk
>>>>>  - gem1_emio_clk
>>>>>  - mio_clk_XX          # with XX = 00..53
>>>>>
>>>>> Example:
>>>>>         clkc: clkc {
>>>>>                 #clock-cells = <1>;
>>>>>                 compatible = "xlnx,ps7-clkc";
>>>>>                 ps-clk-frequency = <33333333>;
>>>>
>>>> The input frequency should be a clock as well.
>>> Again, monolithic vs split. I don't see a reason not to just internally
>>> call clk_register_fixed_rate(). That way its children do not have to
>>> cope with a variable name for the xtal.
>>> Also, with my proposal 'clocks' and 'clock-names' would be purely
>>> optional properties, only required if optional external inputs are
>>> present. Having the xtal defined externally would add mandatory entries for
>>> those props.
>>
>>
>>
>>>
>>>>
>>>>>                 clock-output-names = "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", "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", "swdt", "dbg_trc", "dbg_apb";  /* long list... explanation below */
>>>>>                 /* optional props */
>>>>>                 clocks = <&clkc 16>, <&clk_foo>;
>>>>>                 clock-names = "gem1_emio_clk", "can_mio_clk_23";
>>>>>         };
>>>>>
>>>>> With this revised bindings arbitrary upstream and downstream clock providers should be supported and it's also possible to loop back an output as input. The downside of supporting this is, that I don't see a way around explicitly listing the clock output names in the DT.
>>>>> The reason for this is, that a downstream clock provider should use of_clk_get_parent_name() to obtain its parent clock name. For a block with multiple outputs of_clk_get_parent_name() can return a valid clock name only when 'clock-output-names' is present.
>>>>> Probably the fclks are the only realistic use case to become parent of downstream clock providers, but I could imagine that e.g. a device driver like UART wants to use the CCF to model its internal clocks, hence it would require its parent clock name. Even though a device driver could use clk_get_parent() and __clk_get_name(), of_clk_get_parent_name() should probably work as well. I simply have a bad feeling about breaking of_clk_get_parent_name() for any clock.
>>>>> But after all, I'm open for finding a better solution for this.
>>>>>
>>>>>
>>>>> Similar, inputs for optional clock sources through (E)MIO pins can be defined as described in the clock bindings using the 'clocks' and 'clock-names' properties, with 'clock-names' being an arbitrary subset of the documented names. The actual parent clock names are internally resolved using of_clk_get_parent_name().
>>>>>
>>>>>
>>>>> Regarding this monolithic solution versus a finer granular split:
>>>>>
>>>>> On cost of more complex probing we would also have:
>>>>>  - one clock driver covering the peripheral clocks
>>>>>  - one for the CPU clock domain
>>>>>  - one for the DDR clock domain
>>>>>  - one for GEM
>>>>>  - one for CAN
>>>>>  - one for the APER clks
>>>>>  - one for the PLLs
>>>>>  - one for fclks
>>>>
>>>> fclks are the same as peripheral clocks except for the gate bit, as far as I
>>>> can see.
>>> And that makes them quite different, since they have to access multiple
>>> registers instead of a single one. Also, the fclks have two dividers.
>>> If you want to cover all of those with a single driver, you need a
>>> plethora of arguments/properties to catch the small differences.
>>>
>>>>
>>>>>  - probably one for the debug clocks (not sure whether we need those)
>>>>>
>>>>> Except for the peripheral one and probably the fclk, they would all be instantiated just once. So, there is not a lot of reuse going on.
>>>>
>>>> PLLs are going to be instantiated multiple times as well.
>>> As mentioned in the very next sentence, I rather see a single driver
>>> with multiple outputs. Take suspend: My plan is to have a few functions
>>> like zynq_clk_(suspend|resume). That should take care of bypassing
>>> shutting down the PLLs (and probably more). Therefore it's easier to
>>> have them all in a single driver.
>>> And if it turned out, that other clocks require special handling for
>>> such system level functions, that could be addressed that way too with
>>> the monolithic approach.
>>>
>>>>
>>>>> Fclks I would probably also rather put into one driver with four outputs instead of instantiating a single output driver multiple times. Same for PLLs.
>>>>>
>>>>> And then there is e.g. a mux for the system watchdog input which doesn't really fit anywhere and it would be pretty much ridiculous to have another clock driver just for that one and it would also become "hidden" in one of the others.
>>>>>
>>>>> In my opinion that's just not necessary. We would create a bunch of clock drivers including DT bindings for them, probing would become more complex and it doesn't really help with the probe/naming issue. So, I don't think it's beneficial to go down that road.
>>>>>
>>>>> The monolithic solution would need one custom driver for the PLLs, DT bindings wouldn't be required for it though. Everything else should be internally described using the clock-primitives.
>>>>>
>>>>> Other than having a much simpler probe and init process, I still think it might be beneficial to have this monolithic block with a holistic view of the clock tree. For suspend e.g. I think the clock controller could export functions like zynq_clkc_(suspend|resume) and then the controller handles the PLL bypassing/shutdown.
>>>>> Regarding full dynamic reparenting, I don't know how exactly that could work, but with the clock controller there is at least a block where that intelligence would be going and which has knowledge of all the 'struct clk *' required to reparent clocks.
>>>>>
>>>>> Regarding the DT description, it is probably controversial what is considered better. I, like the Tegra folks, think having one clock controller in there is fine and hides irrelevant implementation details.
>>>>>
>>>>
>>>> I still don't like the monolithic solution. From my point of view it is
>>>> architecturally inferior, it is a bad abstraction. You argue that for a
>>>> non-monolithic version you'd have to implement a clock driver for each
>>>> different type of clock. But you still have to do this for the monolithic
>>>> version, they'd probably just end up in one huge messy file. Unless you are
>>>> going to duplicate huge amounts of lines you'd probably have functions like
>>>> add_gem_clk, add_peripheral_clk, add_pll_clk and so on in your monolithic drivers.
>>> It probably makes sense to differ between having custom drivers and
>>> describing the whole clock tree in DT.
>>>
>>> From reusability perspective, it may make sense to factor out some code
>>> in drivers. IMHO, this does only apply to the peripheral clocks, since
>>> everything else isn't reused and/or quite Zynq specific.
>>> But a monolithic approach would not even prevent this. You could just
>>> transparently change the implementation: Just add a clock driver,
>>> replace the original code in the controller to use the new driver
>>> instead and you're good. No need to touch DT bindings.
>>>
>>> In Zynq a peripheral clock is essentially described by:
>>>         clk = clk_register_mux(NULL, mux_name, parents, 4, 0,                    
>>>                         clk_ctrl, 4, 2, 0, lock);                                
>>>                                                                                  
>>>         clk = clk_register_divider(NULL, div_name, mux_name, 0, clk_ctrl, 8, 6,  
>>>                         CLK_DIVIDER_ONE_BASED, lock);                            
>>>                                                                                  
>>>         clks[clk0] = clk_register_gate(NULL, clk_name0, div_name,                
>>>                         CLK_SET_RATE_PARENT, clk_ctrl, 0, 0, lock);              
>>>
>>> If we wrapped this by a driver, any code outside of that driver couldn't
>>> change the mux setting, since its struct clk* is not exposed outside.
>>> To get around this we'd have to reimplement all the clock ops, to create a
>>> clock which supports all possible operations.
>>> In the monolithic approach we could simply remember that struct clk* and
>>> work with it.
>>>
>>> Bottom line: Factoring out some parts of the monolithic driver might
>>> make sense for some parts. But it can be done later in a transparent way,
>>> especially w/o touching DT bindings.
>>>
>>>
>>> The other thing is describing the whole clock tree in DT:
>>> That would force us to not only describe clocks for which it might make
>>> sense, but also all Zynq specific clocks in custom drivers and DT
>>> bindings and we gain nothing from it.
>>>
>>>>
>>>> The SLCR is a virtual construct, which groups the clock units together. But the
>>>> clock units are the actual entities. E.g. imagine a Zync 2.0, it would probably
>>>> reuse the same clock units, but there would quite likely be different clocks
>>>> mapped at different addresses. If you choose a non-monolithic implementation
>>>> you'd just have to update your devicetree to make this work, with a monolithic
>>>> version you'd have to add a almost identical driver for the v2.
>>> Either way you'll end up with a lot of lines describing the hierarchy.
>>> Either in the dts or in the clock driver.
>>>
>>>
>>> I think, my problem is, that I do not yet know how certain functionality
>>> will be implemented later and what exact use-cases have to be supported.
>>> With the monolithic approach I keep all options open. We hopefully will
>>> never have to touch its DT bindings again. And refactoring the code and
>>> migrating it to use dedicated clock drivers should be transparent
>>> changes, if deemed beneficial.
>>> Furthermore, I have a driver, which has references to all strucht clk*
>>> in the system and can work with the whole clock tree leveraging a system
>>> level view. Separating things in smaller blocks might complicate things
>>> if a use case requires this.
>>> And finally pushing the whole hierarchy description into the DT seems to
>>> be the most restrictive approach, without offering big rewards.
>>
>> I don't see those restrictions you see with a dt binding which has nodes for
>> each clock block. You seem to be under the impression, that each device tree
>> node requires its own driver or device. This is not the case, have you looked
>> at how the current upstream zynq clock support is implemented? This is still
>> one monolithic driver, but there are multiple dt nodes, one for each clock
>> block.
> I thought the one "huge messy" file was one thing you wanted to avoid?

I want to avoid a messy one, yes.

> Also, how does it address my concern regarding inaccessible
> 'struct clk *'? A clk_register_foo() will only return a single struct
> clk *. So, wrapping several clk_register() calls within one
> clk_register_foo() makes all but the actual output inaccessible.
> And if we do not directly call clk_register_foo() but do the probing
> through DT the clock init code does not even get hold of that struct clk
> *.

That's what the composite clk driver is for.
http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg404590.html

> 
>> This leads to a very clean and well structured code and also nicely
>> structured dt, which represents the tree as it is in hardware. If I understand
>> you correctly what you suggest is that you basically want to copy 'n paste the
>> same 10 lines to instantiate a peripheral clock, which you mentioned above,
>> again and again. This will be quite messy.
> For the peripheral clocks you're right, there is some reuse possible.
> For those I have a helper, like zynq_clk_register_periph(). But for pretty much
> everything else I don't.
> 
> In conclusion, I see the DT approach limiting my ability to modify the
> clock tree when implementing system level functionality. 
> Probably quite a stretch, but imagine some smart power management code,
> which might try to reparent all and every clock in the system to certain
> PLLs. How would such a power manager get hold of all the struct clk * it
> needs. In a system probing fully through DT we cannot get hold of those
> and even less if we wrap several clocks in one clock_foo. Unless, we add
> a dummy device which has every clock in the system as its input. And
> that sounds even wackier than having a clock controller which provides
> all the clocks to consumers.

If you really really need a list of all the zynq clocks the easiest way to
implement this is to add a global list and whenever a clock gets probed via
dt add the clock to the list.

- Lars

Re: RFC v2: Zynq Clock Controller

[Sören Brinkmann]

Hi,

On Mon, Mar 25, 2013 at 06:19:11PM +0100, Lars-Peter Clausen wrote:
> On 03/25/2013 06:08 PM, Sören Brinkmann wrote:
> > Hi Lars,
> > 
> > On Mon, Mar 25, 2013 at 03:46:35PM +0100, Lars-Peter Clausen wrote:
> >> Hi,
> >>
> >> On 03/22/2013 11:41 PM, Sören Brinkmann wrote:
> >>> Hi Lars,
> >>>
> >>> On Thu, Mar 21, 2013 at 07:32:52PM +0100, Lars-Peter Clausen wrote:
> >>>> On 03/21/2013 12:56 AM, Sören Brinkmann wrote:
> >>>>> Hi,
> >>>>>
> >>>>> I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq:
> >>>>>
> >>>>> Required properties:
> >>>>>  - #clock-cells : Must be 1
> >>>>>  - compatible : "xlnx,ps7-clkc"  (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline)
> >>>>>  - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
> >>>>>                      (usually 33 MHz oscillators are used for Zynq platforms)
> >>>>>  - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below.
> >>>>>
> >>>>> Optional properties:
> >>>>>  - clocks : as described in the clock bindings
> >>>>>  - clock-names : as described in the clock bindings
> >>>>>
> >>>>> Clock inputs:
> >>>>> The following strings are optional parameters to the 'clock-names' property in
> >>>>> order to provide optional (E)MIO clock sources.
> >>>>>  - swdt_ext_clk
> >>>>>  - gem0_emio_clk
> >>>>>  - gem1_emio_clk
> >>>>>  - mio_clk_XX          # with XX = 00..53
> >>>>>
> >>>>> Example:
> >>>>>         clkc: clkc {
> >>>>>                 #clock-cells = <1>;
> >>>>>                 compatible = "xlnx,ps7-clkc";
> >>>>>                 ps-clk-frequency = <33333333>;
> >>>>
> >>>> The input frequency should be a clock as well.
> >>> Again, monolithic vs split. I don't see a reason not to just internally
> >>> call clk_register_fixed_rate(). That way its children do not have to
> >>> cope with a variable name for the xtal.
> >>> Also, with my proposal 'clocks' and 'clock-names' would be purely
> >>> optional properties, only required if optional external inputs are
> >>> present. Having the xtal defined externally would add mandatory entries for
> >>> those props.
> >>
> >>
> >>
> >>>
> >>>>
> >>>>>                 clock-output-names = "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", "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", "swdt", "dbg_trc", "dbg_apb";  /* long list... explanation below */
> >>>>>                 /* optional props */
> >>>>>                 clocks = <&clkc 16>, <&clk_foo>;
> >>>>>                 clock-names = "gem1_emio_clk", "can_mio_clk_23";
> >>>>>         };
> >>>>>
> >>>>> With this revised bindings arbitrary upstream and downstream clock providers should be supported and it's also possible to loop back an output as input. The downside of supporting this is, that I don't see a way around explicitly listing the clock output names in the DT.
> >>>>> The reason for this is, that a downstream clock provider should use of_clk_get_parent_name() to obtain its parent clock name. For a block with multiple outputs of_clk_get_parent_name() can return a valid clock name only when 'clock-output-names' is present.
> >>>>> Probably the fclks are the only realistic use case to become parent of downstream clock providers, but I could imagine that e.g. a device driver like UART wants to use the CCF to model its internal clocks, hence it would require its parent clock name. Even though a device driver could use clk_get_parent() and __clk_get_name(), of_clk_get_parent_name() should probably work as well. I simply have a bad feeling about breaking of_clk_get_parent_name() for any clock.
> >>>>> But after all, I'm open for finding a better solution for this.
> >>>>>
> >>>>>
> >>>>> Similar, inputs for optional clock sources through (E)MIO pins can be defined as described in the clock bindings using the 'clocks' and 'clock-names' properties, with 'clock-names' being an arbitrary subset of the documented names. The actual parent clock names are internally resolved using of_clk_get_parent_name().
> >>>>>
> >>>>>
> >>>>> Regarding this monolithic solution versus a finer granular split:
> >>>>>
> >>>>> On cost of more complex probing we would also have:
> >>>>>  - one clock driver covering the peripheral clocks
> >>>>>  - one for the CPU clock domain
> >>>>>  - one for the DDR clock domain
> >>>>>  - one for GEM
> >>>>>  - one for CAN
> >>>>>  - one for the APER clks
> >>>>>  - one for the PLLs
> >>>>>  - one for fclks
> >>>>
> >>>> fclks are the same as peripheral clocks except for the gate bit, as far as I
> >>>> can see.
> >>> And that makes them quite different, since they have to access multiple
> >>> registers instead of a single one. Also, the fclks have two dividers.
> >>> If you want to cover all of those with a single driver, you need a
> >>> plethora of arguments/properties to catch the small differences.
> >>>
> >>>>
> >>>>>  - probably one for the debug clocks (not sure whether we need those)
> >>>>>
> >>>>> Except for the peripheral one and probably the fclk, they would all be instantiated just once. So, there is not a lot of reuse going on.
> >>>>
> >>>> PLLs are going to be instantiated multiple times as well.
> >>> As mentioned in the very next sentence, I rather see a single driver
> >>> with multiple outputs. Take suspend: My plan is to have a few functions
> >>> like zynq_clk_(suspend|resume). That should take care of bypassing
> >>> shutting down the PLLs (and probably more). Therefore it's easier to
> >>> have them all in a single driver.
> >>> And if it turned out, that other clocks require special handling for
> >>> such system level functions, that could be addressed that way too with
> >>> the monolithic approach.
> >>>
> >>>>
> >>>>> Fclks I would probably also rather put into one driver with four outputs instead of instantiating a single output driver multiple times. Same for PLLs.
> >>>>>
> >>>>> And then there is e.g. a mux for the system watchdog input which doesn't really fit anywhere and it would be pretty much ridiculous to have another clock driver just for that one and it would also become "hidden" in one of the others.
> >>>>>
> >>>>> In my opinion that's just not necessary. We would create a bunch of clock drivers including DT bindings for them, probing would become more complex and it doesn't really help with the probe/naming issue. So, I don't think it's beneficial to go down that road.
> >>>>>
> >>>>> The monolithic solution would need one custom driver for the PLLs, DT bindings wouldn't be required for it though. Everything else should be internally described using the clock-primitives.
> >>>>>
> >>>>> Other than having a much simpler probe and init process, I still think it might be beneficial to have this monolithic block with a holistic view of the clock tree. For suspend e.g. I think the clock controller could export functions like zynq_clkc_(suspend|resume) and then the controller handles the PLL bypassing/shutdown.
> >>>>> Regarding full dynamic reparenting, I don't know how exactly that could work, but with the clock controller there is at least a block where that intelligence would be going and which has knowledge of all the 'struct clk *' required to reparent clocks.
> >>>>>
> >>>>> Regarding the DT description, it is probably controversial what is considered better. I, like the Tegra folks, think having one clock controller in there is fine and hides irrelevant implementation details.
> >>>>>
> >>>>
> >>>> I still don't like the monolithic solution. From my point of view it is
> >>>> architecturally inferior, it is a bad abstraction. You argue that for a
> >>>> non-monolithic version you'd have to implement a clock driver for each
> >>>> different type of clock. But you still have to do this for the monolithic
> >>>> version, they'd probably just end up in one huge messy file. Unless you are
> >>>> going to duplicate huge amounts of lines you'd probably have functions like
> >>>> add_gem_clk, add_peripheral_clk, add_pll_clk and so on in your monolithic drivers.
> >>> It probably makes sense to differ between having custom drivers and
> >>> describing the whole clock tree in DT.
> >>>
> >>> From reusability perspective, it may make sense to factor out some code
> >>> in drivers. IMHO, this does only apply to the peripheral clocks, since
> >>> everything else isn't reused and/or quite Zynq specific.
> >>> But a monolithic approach would not even prevent this. You could just
> >>> transparently change the implementation: Just add a clock driver,
> >>> replace the original code in the controller to use the new driver
> >>> instead and you're good. No need to touch DT bindings.
> >>>
> >>> In Zynq a peripheral clock is essentially described by:
> >>>         clk = clk_register_mux(NULL, mux_name, parents, 4, 0,                    
> >>>                         clk_ctrl, 4, 2, 0, lock);                                
> >>>                                                                                  
> >>>         clk = clk_register_divider(NULL, div_name, mux_name, 0, clk_ctrl, 8, 6,  
> >>>                         CLK_DIVIDER_ONE_BASED, lock);                            
> >>>                                                                                  
> >>>         clks[clk0] = clk_register_gate(NULL, clk_name0, div_name,                
> >>>                         CLK_SET_RATE_PARENT, clk_ctrl, 0, 0, lock);              
> >>>
> >>> If we wrapped this by a driver, any code outside of that driver couldn't
> >>> change the mux setting, since its struct clk* is not exposed outside.
> >>> To get around this we'd have to reimplement all the clock ops, to create a
> >>> clock which supports all possible operations.
> >>> In the monolithic approach we could simply remember that struct clk* and
> >>> work with it.
> >>>
> >>> Bottom line: Factoring out some parts of the monolithic driver might
> >>> make sense for some parts. But it can be done later in a transparent way,
> >>> especially w/o touching DT bindings.
> >>>
> >>>
> >>> The other thing is describing the whole clock tree in DT:
> >>> That would force us to not only describe clocks for which it might make
> >>> sense, but also all Zynq specific clocks in custom drivers and DT
> >>> bindings and we gain nothing from it.
> >>>
> >>>>
> >>>> The SLCR is a virtual construct, which groups the clock units together. But the
> >>>> clock units are the actual entities. E.g. imagine a Zync 2.0, it would probably
> >>>> reuse the same clock units, but there would quite likely be different clocks
> >>>> mapped at different addresses. If you choose a non-monolithic implementation
> >>>> you'd just have to update your devicetree to make this work, with a monolithic
> >>>> version you'd have to add a almost identical driver for the v2.
> >>> Either way you'll end up with a lot of lines describing the hierarchy.
> >>> Either in the dts or in the clock driver.
> >>>
> >>>
> >>> I think, my problem is, that I do not yet know how certain functionality
> >>> will be implemented later and what exact use-cases have to be supported.
> >>> With the monolithic approach I keep all options open. We hopefully will
> >>> never have to touch its DT bindings again. And refactoring the code and
> >>> migrating it to use dedicated clock drivers should be transparent
> >>> changes, if deemed beneficial.
> >>> Furthermore, I have a driver, which has references to all strucht clk*
> >>> in the system and can work with the whole clock tree leveraging a system
> >>> level view. Separating things in smaller blocks might complicate things
> >>> if a use case requires this.
> >>> And finally pushing the whole hierarchy description into the DT seems to
> >>> be the most restrictive approach, without offering big rewards.
> >>
> >> I don't see those restrictions you see with a dt binding which has nodes for
> >> each clock block. You seem to be under the impression, that each device tree
> >> node requires its own driver or device. This is not the case, have you looked
> >> at how the current upstream zynq clock support is implemented? This is still
> >> one monolithic driver, but there are multiple dt nodes, one for each clock
> >> block.
> > I thought the one "huge messy" file was one thing you wanted to avoid?
> 
> I want to avoid a messy one, yes.
> 
> > Also, how does it address my concern regarding inaccessible
> > 'struct clk *'? A clk_register_foo() will only return a single struct
> > clk *. So, wrapping several clk_register() calls within one
> > clk_register_foo() makes all but the actual output inaccessible.
> > And if we do not directly call clk_register_foo() but do the probing
> > through DT the clock init code does not even get hold of that struct clk
> > *.
> 
> That's what the composite clk driver is for.
> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg4k04590.html
Is it merged by now? Last time I checked it wasn't and I was seeking a
solution w/o this driver. And I think it wouldn't fit all clocks. E.g. the
ones with multiple dividers or gates.

> 
> > 
> >> This leads to a very clean and well structured code and also nicely
> >> structured dt, which represents the tree as it is in hardware. If I understand
> >> you correctly what you suggest is that you basically want to copy 'n paste the
> >> same 10 lines to instantiate a peripheral clock, which you mentioned above,
> >> again and again. This will be quite messy.
> > For the peripheral clocks you're right, there is some reuse possible.
> > For those I have a helper, like zynq_clk_register_periph(). But for pretty much
> > everything else I don't.
> > 
> > In conclusion, I see the DT approach limiting my ability to modify the
> > clock tree when implementing system level functionality. 
> > Probably quite a stretch, but imagine some smart power management code,
> > which might try to reparent all and every clock in the system to certain
> > PLLs. How would such a power manager get hold of all the struct clk * it
> > needs. In a system probing fully through DT we cannot get hold of those
> > and even less if we wrap several clocks in one clock_foo. Unless, we add
> > a dummy device which has every clock in the system as its input. And
> > that sounds even wackier than having a clock controller which provides
> > all the clocks to consumers.
> 
> If you really really need a list of all the zynq clocks the easiest way to
> implement this is to add a global list and whenever a clock gets probed via
> dt add the clock to the list.
A simple list wouldn't do it. I need exact knowledge about which clock
is which. So, I pretty much copied the Tegra approach with a
'struct clk *clks[clk_max]' array. And filling such an array from a generic clock
driver seems wrong.
By separating clocks we take away their awareness of the bigger
hierarchy picture. The clock controller approach maintains this knowledge.

	Sören

Re: RFC v2: Zynq Clock Controller

[Lars-Peter Clausen]

On 03/25/2013 06:59 PM, Sören Brinkmann wrote:
> Hi,
> 
> On Mon, Mar 25, 2013 at 06:19:11PM +0100, Lars-Peter Clausen wrote:
>> On 03/25/2013 06:08 PM, Sören Brinkmann wrote:
>>> Hi Lars,
>>>
>>> On Mon, Mar 25, 2013 at 03:46:35PM +0100, Lars-Peter Clausen wrote:
>>>> Hi,
>>>>
>>>> On 03/22/2013 11:41 PM, Sören Brinkmann wrote:
>>>>> Hi Lars,
>>>>>
>>>>> On Thu, Mar 21, 2013 at 07:32:52PM +0100, Lars-Peter Clausen wrote:
>>>>>> On 03/21/2013 12:56 AM, Sören Brinkmann wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq:
>>>>>>>
>>>>>>> Required properties:
>>>>>>>  - #clock-cells : Must be 1
>>>>>>>  - compatible : "xlnx,ps7-clkc"  (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline)
>>>>>>>  - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
>>>>>>>                      (usually 33 MHz oscillators are used for Zynq platforms)
>>>>>>>  - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below.
>>>>>>>
>>>>>>> Optional properties:
>>>>>>>  - clocks : as described in the clock bindings
>>>>>>>  - clock-names : as described in the clock bindings
>>>>>>>
>>>>>>> Clock inputs:
>>>>>>> The following strings are optional parameters to the 'clock-names' property in
>>>>>>> order to provide optional (E)MIO clock sources.
>>>>>>>  - swdt_ext_clk
>>>>>>>  - gem0_emio_clk
>>>>>>>  - gem1_emio_clk
>>>>>>>  - mio_clk_XX          # with XX = 00..53
>>>>>>>
>>>>>>> Example:
>>>>>>>         clkc: clkc {
>>>>>>>                 #clock-cells = <1>;
>>>>>>>                 compatible = "xlnx,ps7-clkc";
>>>>>>>                 ps-clk-frequency = <33333333>;
>>>>>>
>>>>>> The input frequency should be a clock as well.
>>>>> Again, monolithic vs split. I don't see a reason not to just internally
>>>>> call clk_register_fixed_rate(). That way its children do not have to
>>>>> cope with a variable name for the xtal.
>>>>> Also, with my proposal 'clocks' and 'clock-names' would be purely
>>>>> optional properties, only required if optional external inputs are
>>>>> present. Having the xtal defined externally would add mandatory entries for
>>>>> those props.
>>>>
>>>>
>>>>
>>>>>
>>>>>>
>>>>>>>                 clock-output-names = "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", "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", "swdt", "dbg_trc", "dbg_apb";  /* long list... explanation below */
>>>>>>>                 /* optional props */
>>>>>>>                 clocks = <&clkc 16>, <&clk_foo>;
>>>>>>>                 clock-names = "gem1_emio_clk", "can_mio_clk_23";
>>>>>>>         };
>>>>>>>
>>>>>>> With this revised bindings arbitrary upstream and downstream clock providers should be supported and it's also possible to loop back an output as input. The downside of supporting this is, that I don't see a way around explicitly listing the clock output names in the DT.
>>>>>>> The reason for this is, that a downstream clock provider should use of_clk_get_parent_name() to obtain its parent clock name. For a block with multiple outputs of_clk_get_parent_name() can return a valid clock name only when 'clock-output-names' is present.
>>>>>>> Probably the fclks are the only realistic use case to become parent of downstream clock providers, but I could imagine that e.g. a device driver like UART wants to use the CCF to model its internal clocks, hence it would require its parent clock name. Even though a device driver could use clk_get_parent() and __clk_get_name(), of_clk_get_parent_name() should probably work as well. I simply have a bad feeling about breaking of_clk_get_parent_name() for any clock.
>>>>>>> But after all, I'm open for finding a better solution for this.
>>>>>>>
>>>>>>>
>>>>>>> Similar, inputs for optional clock sources through (E)MIO pins can be defined as described in the clock bindings using the 'clocks' and 'clock-names' properties, with 'clock-names' being an arbitrary subset of the documented names. The actual parent clock names are internally resolved using of_clk_get_parent_name().
>>>>>>>
>>>>>>>
>>>>>>> Regarding this monolithic solution versus a finer granular split:
>>>>>>>
>>>>>>> On cost of more complex probing we would also have:
>>>>>>>  - one clock driver covering the peripheral clocks
>>>>>>>  - one for the CPU clock domain
>>>>>>>  - one for the DDR clock domain
>>>>>>>  - one for GEM
>>>>>>>  - one for CAN
>>>>>>>  - one for the APER clks
>>>>>>>  - one for the PLLs
>>>>>>>  - one for fclks
>>>>>>
>>>>>> fclks are the same as peripheral clocks except for the gate bit, as far as I
>>>>>> can see.
>>>>> And that makes them quite different, since they have to access multiple
>>>>> registers instead of a single one. Also, the fclks have two dividers.
>>>>> If you want to cover all of those with a single driver, you need a
>>>>> plethora of arguments/properties to catch the small differences.
>>>>>
>>>>>>
>>>>>>>  - probably one for the debug clocks (not sure whether we need those)
>>>>>>>
>>>>>>> Except for the peripheral one and probably the fclk, they would all be instantiated just once. So, there is not a lot of reuse going on.
>>>>>>
>>>>>> PLLs are going to be instantiated multiple times as well.
>>>>> As mentioned in the very next sentence, I rather see a single driver
>>>>> with multiple outputs. Take suspend: My plan is to have a few functions
>>>>> like zynq_clk_(suspend|resume). That should take care of bypassing
>>>>> shutting down the PLLs (and probably more). Therefore it's easier to
>>>>> have them all in a single driver.
>>>>> And if it turned out, that other clocks require special handling for
>>>>> such system level functions, that could be addressed that way too with
>>>>> the monolithic approach.
>>>>>
>>>>>>
>>>>>>> Fclks I would probably also rather put into one driver with four outputs instead of instantiating a single output driver multiple times. Same for PLLs.
>>>>>>>
>>>>>>> And then there is e.g. a mux for the system watchdog input which doesn't really fit anywhere and it would be pretty much ridiculous to have another clock driver just for that one and it would also become "hidden" in one of the others.
>>>>>>>
>>>>>>> In my opinion that's just not necessary. We would create a bunch of clock drivers including DT bindings for them, probing would become more complex and it doesn't really help with the probe/naming issue. So, I don't think it's beneficial to go down that road.
>>>>>>>
>>>>>>> The monolithic solution would need one custom driver for the PLLs, DT bindings wouldn't be required for it though. Everything else should be internally described using the clock-primitives.
>>>>>>>
>>>>>>> Other than having a much simpler probe and init process, I still think it might be beneficial to have this monolithic block with a holistic view of the clock tree. For suspend e.g. I think the clock controller could export functions like zynq_clkc_(suspend|resume) and then the controller handles the PLL bypassing/shutdown.
>>>>>>> Regarding full dynamic reparenting, I don't know how exactly that could work, but with the clock controller there is at least a block where that intelligence would be going and which has knowledge of all the 'struct clk *' required to reparent clocks.
>>>>>>>
>>>>>>> Regarding the DT description, it is probably controversial what is considered better. I, like the Tegra folks, think having one clock controller in there is fine and hides irrelevant implementation details.
>>>>>>>
>>>>>>
>>>>>> I still don't like the monolithic solution. From my point of view it is
>>>>>> architecturally inferior, it is a bad abstraction. You argue that for a
>>>>>> non-monolithic version you'd have to implement a clock driver for each
>>>>>> different type of clock. But you still have to do this for the monolithic
>>>>>> version, they'd probably just end up in one huge messy file. Unless you are
>>>>>> going to duplicate huge amounts of lines you'd probably have functions like
>>>>>> add_gem_clk, add_peripheral_clk, add_pll_clk and so on in your monolithic drivers.
>>>>> It probably makes sense to differ between having custom drivers and
>>>>> describing the whole clock tree in DT.
>>>>>
>>>>> From reusability perspective, it may make sense to factor out some code
>>>>> in drivers. IMHO, this does only apply to the peripheral clocks, since
>>>>> everything else isn't reused and/or quite Zynq specific.
>>>>> But a monolithic approach would not even prevent this. You could just
>>>>> transparently change the implementation: Just add a clock driver,
>>>>> replace the original code in the controller to use the new driver
>>>>> instead and you're good. No need to touch DT bindings.
>>>>>
>>>>> In Zynq a peripheral clock is essentially described by:
>>>>>         clk = clk_register_mux(NULL, mux_name, parents, 4, 0,                    
>>>>>                         clk_ctrl, 4, 2, 0, lock);                                
>>>>>                                                                                  
>>>>>         clk = clk_register_divider(NULL, div_name, mux_name, 0, clk_ctrl, 8, 6,  
>>>>>                         CLK_DIVIDER_ONE_BASED, lock);                            
>>>>>                                                                                  
>>>>>         clks[clk0] = clk_register_gate(NULL, clk_name0, div_name,                
>>>>>                         CLK_SET_RATE_PARENT, clk_ctrl, 0, 0, lock);              
>>>>>
>>>>> If we wrapped this by a driver, any code outside of that driver couldn't
>>>>> change the mux setting, since its struct clk* is not exposed outside.
>>>>> To get around this we'd have to reimplement all the clock ops, to create a
>>>>> clock which supports all possible operations.
>>>>> In the monolithic approach we could simply remember that struct clk* and
>>>>> work with it.
>>>>>
>>>>> Bottom line: Factoring out some parts of the monolithic driver might
>>>>> make sense for some parts. But it can be done later in a transparent way,
>>>>> especially w/o touching DT bindings.
>>>>>
>>>>>
>>>>> The other thing is describing the whole clock tree in DT:
>>>>> That would force us to not only describe clocks for which it might make
>>>>> sense, but also all Zynq specific clocks in custom drivers and DT
>>>>> bindings and we gain nothing from it.
>>>>>
>>>>>>
>>>>>> The SLCR is a virtual construct, which groups the clock units together. But the
>>>>>> clock units are the actual entities. E.g. imagine a Zync 2.0, it would probably
>>>>>> reuse the same clock units, but there would quite likely be different clocks
>>>>>> mapped at different addresses. If you choose a non-monolithic implementation
>>>>>> you'd just have to update your devicetree to make this work, with a monolithic
>>>>>> version you'd have to add a almost identical driver for the v2.
>>>>> Either way you'll end up with a lot of lines describing the hierarchy.
>>>>> Either in the dts or in the clock driver.
>>>>>
>>>>>
>>>>> I think, my problem is, that I do not yet know how certain functionality
>>>>> will be implemented later and what exact use-cases have to be supported.
>>>>> With the monolithic approach I keep all options open. We hopefully will
>>>>> never have to touch its DT bindings again. And refactoring the code and
>>>>> migrating it to use dedicated clock drivers should be transparent
>>>>> changes, if deemed beneficial.
>>>>> Furthermore, I have a driver, which has references to all strucht clk*
>>>>> in the system and can work with the whole clock tree leveraging a system
>>>>> level view. Separating things in smaller blocks might complicate things
>>>>> if a use case requires this.
>>>>> And finally pushing the whole hierarchy description into the DT seems to
>>>>> be the most restrictive approach, without offering big rewards.
>>>>
>>>> I don't see those restrictions you see with a dt binding which has nodes for
>>>> each clock block. You seem to be under the impression, that each device tree
>>>> node requires its own driver or device. This is not the case, have you looked
>>>> at how the current upstream zynq clock support is implemented? This is still
>>>> one monolithic driver, but there are multiple dt nodes, one for each clock
>>>> block.
>>> I thought the one "huge messy" file was one thing you wanted to avoid?
>>
>> I want to avoid a messy one, yes.
>>
>>> Also, how does it address my concern regarding inaccessible
>>> 'struct clk *'? A clk_register_foo() will only return a single struct
>>> clk *. So, wrapping several clk_register() calls within one
>>> clk_register_foo() makes all but the actual output inaccessible.
>>> And if we do not directly call clk_register_foo() but do the probing
>>> through DT the clock init code does not even get hold of that struct clk
>>> *.
>>
>> That's what the composite clk driver is for.
>> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg4k04590.html
> Is it merged by now? Last time I checked it wasn't and I was seeking a
> solution w/o this driver. And I think it wouldn't fit all clocks. E.g. the
> ones with multiple dividers or gates.

Not yet, but hopefully soon.

For the clocks with multiple dividers we want a custom clk driver, at least
for the divider part, otherwise implementing clk_set_rate wouldn't be so easy.

>>
>>>
>>>> This leads to a very clean and well structured code and also nicely
>>>> structured dt, which represents the tree as it is in hardware. If I understand
>>>> you correctly what you suggest is that you basically want to copy 'n paste the
>>>> same 10 lines to instantiate a peripheral clock, which you mentioned above,
>>>> again and again. This will be quite messy.
>>> For the peripheral clocks you're right, there is some reuse possible.
>>> For those I have a helper, like zynq_clk_register_periph(). But for pretty much
>>> everything else I don't.
>>>
>>> In conclusion, I see the DT approach limiting my ability to modify the
>>> clock tree when implementing system level functionality. 
>>> Probably quite a stretch, but imagine some smart power management code,
>>> which might try to reparent all and every clock in the system to certain
>>> PLLs. How would such a power manager get hold of all the struct clk * it
>>> needs. In a system probing fully through DT we cannot get hold of those
>>> and even less if we wrap several clocks in one clock_foo. Unless, we add
>>> a dummy device which has every clock in the system as its input. And
>>> that sounds even wackier than having a clock controller which provides
>>> all the clocks to consumers.
>>
>> If you really really need a list of all the zynq clocks the easiest way to
>> implement this is to add a global list and whenever a clock gets probed via
>> dt add the clock to the list.
> A simple list wouldn't do it. I need exact knowledge about which clock
> is which. So, I pretty much copied the Tegra approach with a
> 'struct clk *clks[clk_max]' array. And filling such an array from a generic clock
> driver seems wrong.
> By separating clocks we take away their awareness of the bigger
> hierarchy picture. The clock controller approach maintains this knowledge.

If you already have the code can you upload it somewhere, this might make it
easier for me to understand why the solution I have in mind won't work.

- Lars

Re: RFC v2: Zynq Clock Controller

[Sören Brinkmann]

On Mon, Mar 25, 2013 at 07:10:35PM +0100, Lars-Peter Clausen wrote:
> On 03/25/2013 06:59 PM, Sören Brinkmann wrote:
> > Hi,
> > 
> > On Mon, Mar 25, 2013 at 06:19:11PM +0100, Lars-Peter Clausen wrote:
> >> On 03/25/2013 06:08 PM, Sören Brinkmann wrote:
> >>> Hi Lars,
> >>>
> >>> On Mon, Mar 25, 2013 at 03:46:35PM +0100, Lars-Peter Clausen wrote:
> >>>> Hi,
> >>>>
> >>>> On 03/22/2013 11:41 PM, Sören Brinkmann wrote:
> >>>>> Hi Lars,
> >>>>>
> >>>>> On Thu, Mar 21, 2013 at 07:32:52PM +0100, Lars-Peter Clausen wrote:
> >>>>>> On 03/21/2013 12:56 AM, Sören Brinkmann wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq:
> >>>>>>>
> >>>>>>> Required properties:
> >>>>>>>  - #clock-cells : Must be 1
> >>>>>>>  - compatible : "xlnx,ps7-clkc"  (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline)
> >>>>>>>  - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
> >>>>>>>                      (usually 33 MHz oscillators are used for Zynq platforms)
> >>>>>>>  - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below.
> >>>>>>>
> >>>>>>> Optional properties:
> >>>>>>>  - clocks : as described in the clock bindings
> >>>>>>>  - clock-names : as described in the clock bindings
> >>>>>>>
> >>>>>>> Clock inputs:
> >>>>>>> The following strings are optional parameters to the 'clock-names' property in
> >>>>>>> order to provide optional (E)MIO clock sources.
> >>>>>>>  - swdt_ext_clk
> >>>>>>>  - gem0_emio_clk
> >>>>>>>  - gem1_emio_clk
> >>>>>>>  - mio_clk_XX          # with XX = 00..53
> >>>>>>>
> >>>>>>> Example:
> >>>>>>>         clkc: clkc {
> >>>>>>>                 #clock-cells = <1>;
> >>>>>>>                 compatible = "xlnx,ps7-clkc";
> >>>>>>>                 ps-clk-frequency = <33333333>;
> >>>>>>
> >>>>>> The input frequency should be a clock as well.
> >>>>> Again, monolithic vs split. I don't see a reason not to just internally
> >>>>> call clk_register_fixed_rate(). That way its children do not have to
> >>>>> cope with a variable name for the xtal.
> >>>>> Also, with my proposal 'clocks' and 'clock-names' would be purely
> >>>>> optional properties, only required if optional external inputs are
> >>>>> present. Having the xtal defined externally would add mandatory entries for
> >>>>> those props.
> >>>>
> >>>>
> >>>>
> >>>>>
> >>>>>>
> >>>>>>>                 clock-output-names = "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", "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", "swdt", "dbg_trc", "dbg_apb";  /* long list... explanation below */
> >>>>>>>                 /* optional props */
> >>>>>>>                 clocks = <&clkc 16>, <&clk_foo>;
> >>>>>>>                 clock-names = "gem1_emio_clk", "can_mio_clk_23";
> >>>>>>>         };
> >>>>>>>
> >>>>>>> With this revised bindings arbitrary upstream and downstream clock providers should be supported and it's also possible to loop back an output as input. The downside of supporting this is, that I don't see a way around explicitly listing the clock output names in the DT.
> >>>>>>> The reason for this is, that a downstream clock provider should use of_clk_get_parent_name() to obtain its parent clock name. For a block with multiple outputs of_clk_get_parent_name() can return a valid clock name only when 'clock-output-names' is present.
> >>>>>>> Probably the fclks are the only realistic use case to become parent of downstream clock providers, but I could imagine that e.g. a device driver like UART wants to use the CCF to model its internal clocks, hence it would require its parent clock name. Even though a device driver could use clk_get_parent() and __clk_get_name(), of_clk_get_parent_name() should probably work as well. I simply have a bad feeling about breaking of_clk_get_parent_name() for any clock.
> >>>>>>> But after all, I'm open for finding a better solution for this.
> >>>>>>>
> >>>>>>>
> >>>>>>> Similar, inputs for optional clock sources through (E)MIO pins can be defined as described in the clock bindings using the 'clocks' and 'clock-names' properties, with 'clock-names' being an arbitrary subset of the documented names. The actual parent clock names are internally resolved using of_clk_get_parent_name().
> >>>>>>>
> >>>>>>>
> >>>>>>> Regarding this monolithic solution versus a finer granular split:
> >>>>>>>
> >>>>>>> On cost of more complex probing we would also have:
> >>>>>>>  - one clock driver covering the peripheral clocks
> >>>>>>>  - one for the CPU clock domain
> >>>>>>>  - one for the DDR clock domain
> >>>>>>>  - one for GEM
> >>>>>>>  - one for CAN
> >>>>>>>  - one for the APER clks
> >>>>>>>  - one for the PLLs
> >>>>>>>  - one for fclks
> >>>>>>
> >>>>>> fclks are the same as peripheral clocks except for the gate bit, as far as I
> >>>>>> can see.
> >>>>> And that makes them quite different, since they have to access multiple
> >>>>> registers instead of a single one. Also, the fclks have two dividers.
> >>>>> If you want to cover all of those with a single driver, you need a
> >>>>> plethora of arguments/properties to catch the small differences.
> >>>>>
> >>>>>>
> >>>>>>>  - probably one for the debug clocks (not sure whether we need those)
> >>>>>>>
> >>>>>>> Except for the peripheral one and probably the fclk, they would all be instantiated just once. So, there is not a lot of reuse going on.
> >>>>>>
> >>>>>> PLLs are going to be instantiated multiple times as well.
> >>>>> As mentioned in the very next sentence, I rather see a single driver
> >>>>> with multiple outputs. Take suspend: My plan is to have a few functions
> >>>>> like zynq_clk_(suspend|resume). That should take care of bypassing
> >>>>> shutting down the PLLs (and probably more). Therefore it's easier to
> >>>>> have them all in a single driver.
> >>>>> And if it turned out, that other clocks require special handling for
> >>>>> such system level functions, that could be addressed that way too with
> >>>>> the monolithic approach.
> >>>>>
> >>>>>>
> >>>>>>> Fclks I would probably also rather put into one driver with four outputs instead of instantiating a single output driver multiple times. Same for PLLs.
> >>>>>>>
> >>>>>>> And then there is e.g. a mux for the system watchdog input which doesn't really fit anywhere and it would be pretty much ridiculous to have another clock driver just for that one and it would also become "hidden" in one of the others.
> >>>>>>>
> >>>>>>> In my opinion that's just not necessary. We would create a bunch of clock drivers including DT bindings for them, probing would become more complex and it doesn't really help with the probe/naming issue. So, I don't think it's beneficial to go down that road.
> >>>>>>>
> >>>>>>> The monolithic solution would need one custom driver for the PLLs, DT bindings wouldn't be required for it though. Everything else should be internally described using the clock-primitives.
> >>>>>>>
> >>>>>>> Other than having a much simpler probe and init process, I still think it might be beneficial to have this monolithic block with a holistic view of the clock tree. For suspend e.g. I think the clock controller could export functions like zynq_clkc_(suspend|resume) and then the controller handles the PLL bypassing/shutdown.
> >>>>>>> Regarding full dynamic reparenting, I don't know how exactly that could work, but with the clock controller there is at least a block where that intelligence would be going and which has knowledge of all the 'struct clk *' required to reparent clocks.
> >>>>>>>
> >>>>>>> Regarding the DT description, it is probably controversial what is considered better. I, like the Tegra folks, think having one clock controller in there is fine and hides irrelevant implementation details.
> >>>>>>>
> >>>>>>
> >>>>>> I still don't like the monolithic solution. From my point of view it is
> >>>>>> architecturally inferior, it is a bad abstraction. You argue that for a
> >>>>>> non-monolithic version you'd have to implement a clock driver for each
> >>>>>> different type of clock. But you still have to do this for the monolithic
> >>>>>> version, they'd probably just end up in one huge messy file. Unless you are
> >>>>>> going to duplicate huge amounts of lines you'd probably have functions like
> >>>>>> add_gem_clk, add_peripheral_clk, add_pll_clk and so on in your monolithic drivers.
> >>>>> It probably makes sense to differ between having custom drivers and
> >>>>> describing the whole clock tree in DT.
> >>>>>
> >>>>> From reusability perspective, it may make sense to factor out some code
> >>>>> in drivers. IMHO, this does only apply to the peripheral clocks, since
> >>>>> everything else isn't reused and/or quite Zynq specific.
> >>>>> But a monolithic approach would not even prevent this. You could just
> >>>>> transparently change the implementation: Just add a clock driver,
> >>>>> replace the original code in the controller to use the new driver
> >>>>> instead and you're good. No need to touch DT bindings.
> >>>>>
> >>>>> In Zynq a peripheral clock is essentially described by:
> >>>>>         clk = clk_register_mux(NULL, mux_name, parents, 4, 0,                    
> >>>>>                         clk_ctrl, 4, 2, 0, lock);                                
> >>>>>                                                                                  
> >>>>>         clk = clk_register_divider(NULL, div_name, mux_name, 0, clk_ctrl, 8, 6,  
> >>>>>                         CLK_DIVIDER_ONE_BASED, lock);                            
> >>>>>                                                                                  
> >>>>>         clks[clk0] = clk_register_gate(NULL, clk_name0, div_name,                
> >>>>>                         CLK_SET_RATE_PARENT, clk_ctrl, 0, 0, lock);              
> >>>>>
> >>>>> If we wrapped this by a driver, any code outside of that driver couldn't
> >>>>> change the mux setting, since its struct clk* is not exposed outside.
> >>>>> To get around this we'd have to reimplement all the clock ops, to create a
> >>>>> clock which supports all possible operations.
> >>>>> In the monolithic approach we could simply remember that struct clk* and
> >>>>> work with it.
> >>>>>
> >>>>> Bottom line: Factoring out some parts of the monolithic driver might
> >>>>> make sense for some parts. But it can be done later in a transparent way,
> >>>>> especially w/o touching DT bindings.
> >>>>>
> >>>>>
> >>>>> The other thing is describing the whole clock tree in DT:
> >>>>> That would force us to not only describe clocks for which it might make
> >>>>> sense, but also all Zynq specific clocks in custom drivers and DT
> >>>>> bindings and we gain nothing from it.
> >>>>>
> >>>>>>
> >>>>>> The SLCR is a virtual construct, which groups the clock units together. But the
> >>>>>> clock units are the actual entities. E.g. imagine a Zync 2.0, it would probably
> >>>>>> reuse the same clock units, but there would quite likely be different clocks
> >>>>>> mapped at different addresses. If you choose a non-monolithic implementation
> >>>>>> you'd just have to update your devicetree to make this work, with a monolithic
> >>>>>> version you'd have to add a almost identical driver for the v2.
> >>>>> Either way you'll end up with a lot of lines describing the hierarchy.
> >>>>> Either in the dts or in the clock driver.
> >>>>>
> >>>>>
> >>>>> I think, my problem is, that I do not yet know how certain functionality
> >>>>> will be implemented later and what exact use-cases have to be supported.
> >>>>> With the monolithic approach I keep all options open. We hopefully will
> >>>>> never have to touch its DT bindings again. And refactoring the code and
> >>>>> migrating it to use dedicated clock drivers should be transparent
> >>>>> changes, if deemed beneficial.
> >>>>> Furthermore, I have a driver, which has references to all strucht clk*
> >>>>> in the system and can work with the whole clock tree leveraging a system
> >>>>> level view. Separating things in smaller blocks might complicate things
> >>>>> if a use case requires this.
> >>>>> And finally pushing the whole hierarchy description into the DT seems to
> >>>>> be the most restrictive approach, without offering big rewards.
> >>>>
> >>>> I don't see those restrictions you see with a dt binding which has nodes for
> >>>> each clock block. You seem to be under the impression, that each device tree
> >>>> node requires its own driver or device. This is not the case, have you looked
> >>>> at how the current upstream zynq clock support is implemented? This is still
> >>>> one monolithic driver, but there are multiple dt nodes, one for each clock
> >>>> block.
> >>> I thought the one "huge messy" file was one thing you wanted to avoid?
> >>
> >> I want to avoid a messy one, yes.
> >>
> >>> Also, how does it address my concern regarding inaccessible
> >>> 'struct clk *'? A clk_register_foo() will only return a single struct
> >>> clk *. So, wrapping several clk_register() calls within one
> >>> clk_register_foo() makes all but the actual output inaccessible.
> >>> And if we do not directly call clk_register_foo() but do the probing
> >>> through DT the clock init code does not even get hold of that struct clk
> >>> *.
> >>
> >> That's what the composite clk driver is for.
> >> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg4k04590.html
> > Is it merged by now? Last time I checked it wasn't and I was seeking a
> > solution w/o this driver. And I think it wouldn't fit all clocks. E.g. the
> > ones with multiple dividers or gates.
> 
> Not yet, but hopefully soon.
> 
> For the clocks with multiple dividers we want a custom clk driver, at least
> for the divider part, otherwise implementing clk_set_rate wouldn't be so easy.
IMHO, two cascaded clk-divider should work when CLK_SET_RATE_PARENT is
set for the second one. I hope that I don't need any custom drivers but
describe everything through the kernel provided clock primitives, with
the PLLs as only exception.

> >>
> >>>
> >>>> This leads to a very clean and well structured code and also nicely
> >>>> structured dt, which represents the tree as it is in hardware. If I understand
> >>>> you correctly what you suggest is that you basically want to copy 'n paste the
> >>>> same 10 lines to instantiate a peripheral clock, which you mentioned above,
> >>>> again and again. This will be quite messy.
> >>> For the peripheral clocks you're right, there is some reuse possible.
> >>> For those I have a helper, like zynq_clk_register_periph(). But for pretty much
> >>> everything else I don't.
> >>>
> >>> In conclusion, I see the DT approach limiting my ability to modify the
> >>> clock tree when implementing system level functionality. 
> >>> Probably quite a stretch, but imagine some smart power management code,
> >>> which might try to reparent all and every clock in the system to certain
> >>> PLLs. How would such a power manager get hold of all the struct clk * it
> >>> needs. In a system probing fully through DT we cannot get hold of those
> >>> and even less if we wrap several clocks in one clock_foo. Unless, we add
> >>> a dummy device which has every clock in the system as its input. And
> >>> that sounds even wackier than having a clock controller which provides
> >>> all the clocks to consumers.
> >>
> >> If you really really need a list of all the zynq clocks the easiest way to
> >> implement this is to add a global list and whenever a clock gets probed via
> >> dt add the clock to the list.
> > A simple list wouldn't do it. I need exact knowledge about which clock
> > is which. So, I pretty much copied the Tegra approach with a
> > 'struct clk *clks[clk_max]' array. And filling such an array from a generic clock
> > driver seems wrong.
> > By separating clocks we take away their awareness of the bigger
> > hierarchy picture. The clock controller approach maintains this knowledge.
> 
> If you already have the code can you upload it somewhere, this might make it
> easier for me to understand why the solution I have in mind won't work.
I'll try to make this happen.

	Sören

Re: RFC v2: Zynq Clock Controller

[Sören Brinkmann]

On Mon, Mar 25, 2013 at 07:10:35PM +0100, Lars-Peter Clausen wrote:
> On 03/25/2013 06:59 PM, Sören Brinkmann wrote:
> > Hi,
> > 
> > On Mon, Mar 25, 2013 at 06:19:11PM +0100, Lars-Peter Clausen wrote:
> >> On 03/25/2013 06:08 PM, Sören Brinkmann wrote:
> >>> Hi Lars,
> >>>
> >>> On Mon, Mar 25, 2013 at 03:46:35PM +0100, Lars-Peter Clausen wrote:
> >>>> Hi,
> >>>>
> >>>> On 03/22/2013 11:41 PM, Sören Brinkmann wrote:
> >>>>> Hi Lars,
> >>>>>
> >>>>> On Thu, Mar 21, 2013 at 07:32:52PM +0100, Lars-Peter Clausen wrote:
> >>>>>> On 03/21/2013 12:56 AM, Sören Brinkmann wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq:
> >>>>>>>
> >>>>>>> Required properties:
> >>>>>>>  - #clock-cells : Must be 1
> >>>>>>>  - compatible : "xlnx,ps7-clkc"  (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline)
> >>>>>>>  - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
> >>>>>>>                      (usually 33 MHz oscillators are used for Zynq platforms)
> >>>>>>>  - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below.
> >>>>>>>
> >>>>>>> Optional properties:
> >>>>>>>  - clocks : as described in the clock bindings
> >>>>>>>  - clock-names : as described in the clock bindings
> >>>>>>>
> >>>>>>> Clock inputs:
> >>>>>>> The following strings are optional parameters to the 'clock-names' property in
> >>>>>>> order to provide optional (E)MIO clock sources.
> >>>>>>>  - swdt_ext_clk
> >>>>>>>  - gem0_emio_clk
> >>>>>>>  - gem1_emio_clk
> >>>>>>>  - mio_clk_XX          # with XX = 00..53
> >>>>>>>
> >>>>>>> Example:
> >>>>>>>         clkc: clkc {
> >>>>>>>                 #clock-cells = <1>;
> >>>>>>>                 compatible = "xlnx,ps7-clkc";
> >>>>>>>                 ps-clk-frequency = <33333333>;
> >>>>>>
> >>>>>> The input frequency should be a clock as well.
> >>>>> Again, monolithic vs split. I don't see a reason not to just internally
> >>>>> call clk_register_fixed_rate(). That way its children do not have to
> >>>>> cope with a variable name for the xtal.
> >>>>> Also, with my proposal 'clocks' and 'clock-names' would be purely
> >>>>> optional properties, only required if optional external inputs are
> >>>>> present. Having the xtal defined externally would add mandatory entries for
> >>>>> those props.
> >>>>
> >>>>
> >>>>
> >>>>>
> >>>>>>
> >>>>>>>                 clock-output-names = "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", "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", "swdt", "dbg_trc", "dbg_apb";  /* long list... explanation below */
> >>>>>>>                 /* optional props */
> >>>>>>>                 clocks = <&clkc 16>, <&clk_foo>;
> >>>>>>>                 clock-names = "gem1_emio_clk", "can_mio_clk_23";
> >>>>>>>         };
> >>>>>>>
> >>>>>>> With this revised bindings arbitrary upstream and downstream clock providers should be supported and it's also possible to loop back an output as input. The downside of supporting this is, that I don't see a way around explicitly listing the clock output names in the DT.
> >>>>>>> The reason for this is, that a downstream clock provider should use of_clk_get_parent_name() to obtain its parent clock name. For a block with multiple outputs of_clk_get_parent_name() can return a valid clock name only when 'clock-output-names' is present.
> >>>>>>> Probably the fclks are the only realistic use case to become parent of downstream clock providers, but I could imagine that e.g. a device driver like UART wants to use the CCF to model its internal clocks, hence it would require its parent clock name. Even though a device driver could use clk_get_parent() and __clk_get_name(), of_clk_get_parent_name() should probably work as well. I simply have a bad feeling about breaking of_clk_get_parent_name() for any clock.
> >>>>>>> But after all, I'm open for finding a better solution for this.
> >>>>>>>
> >>>>>>>
> >>>>>>> Similar, inputs for optional clock sources through (E)MIO pins can be defined as described in the clock bindings using the 'clocks' and 'clock-names' properties, with 'clock-names' being an arbitrary subset of the documented names. The actual parent clock names are internally resolved using of_clk_get_parent_name().
> >>>>>>>
> >>>>>>>
> >>>>>>> Regarding this monolithic solution versus a finer granular split:
> >>>>>>>
> >>>>>>> On cost of more complex probing we would also have:
> >>>>>>>  - one clock driver covering the peripheral clocks
> >>>>>>>  - one for the CPU clock domain
> >>>>>>>  - one for the DDR clock domain
> >>>>>>>  - one for GEM
> >>>>>>>  - one for CAN
> >>>>>>>  - one for the APER clks
> >>>>>>>  - one for the PLLs
> >>>>>>>  - one for fclks
> >>>>>>
> >>>>>> fclks are the same as peripheral clocks except for the gate bit, as far as I
> >>>>>> can see.
> >>>>> And that makes them quite different, since they have to access multiple
> >>>>> registers instead of a single one. Also, the fclks have two dividers.
> >>>>> If you want to cover all of those with a single driver, you need a
> >>>>> plethora of arguments/properties to catch the small differences.
> >>>>>
> >>>>>>
> >>>>>>>  - probably one for the debug clocks (not sure whether we need those)
> >>>>>>>
> >>>>>>> Except for the peripheral one and probably the fclk, they would all be instantiated just once. So, there is not a lot of reuse going on.
> >>>>>>
> >>>>>> PLLs are going to be instantiated multiple times as well.
> >>>>> As mentioned in the very next sentence, I rather see a single driver
> >>>>> with multiple outputs. Take suspend: My plan is to have a few functions
> >>>>> like zynq_clk_(suspend|resume). That should take care of bypassing
> >>>>> shutting down the PLLs (and probably more). Therefore it's easier to
> >>>>> have them all in a single driver.
> >>>>> And if it turned out, that other clocks require special handling for
> >>>>> such system level functions, that could be addressed that way too with
> >>>>> the monolithic approach.
> >>>>>
> >>>>>>
> >>>>>>> Fclks I would probably also rather put into one driver with four outputs instead of instantiating a single output driver multiple times. Same for PLLs.
> >>>>>>>
> >>>>>>> And then there is e.g. a mux for the system watchdog input which doesn't really fit anywhere and it would be pretty much ridiculous to have another clock driver just for that one and it would also become "hidden" in one of the others.
> >>>>>>>
> >>>>>>> In my opinion that's just not necessary. We would create a bunch of clock drivers including DT bindings for them, probing would become more complex and it doesn't really help with the probe/naming issue. So, I don't think it's beneficial to go down that road.
> >>>>>>>
> >>>>>>> The monolithic solution would need one custom driver for the PLLs, DT bindings wouldn't be required for it though. Everything else should be internally described using the clock-primitives.
> >>>>>>>
> >>>>>>> Other than having a much simpler probe and init process, I still think it might be beneficial to have this monolithic block with a holistic view of the clock tree. For suspend e.g. I think the clock controller could export functions like zynq_clkc_(suspend|resume) and then the controller handles the PLL bypassing/shutdown.
> >>>>>>> Regarding full dynamic reparenting, I don't know how exactly that could work, but with the clock controller there is at least a block where that intelligence would be going and which has knowledge of all the 'struct clk *' required to reparent clocks.
> >>>>>>>
> >>>>>>> Regarding the DT description, it is probably controversial what is considered better. I, like the Tegra folks, think having one clock controller in there is fine and hides irrelevant implementation details.
> >>>>>>>
> >>>>>>
> >>>>>> I still don't like the monolithic solution. From my point of view it is
> >>>>>> architecturally inferior, it is a bad abstraction. You argue that for a
> >>>>>> non-monolithic version you'd have to implement a clock driver for each
> >>>>>> different type of clock. But you still have to do this for the monolithic
> >>>>>> version, they'd probably just end up in one huge messy file. Unless you are
> >>>>>> going to duplicate huge amounts of lines you'd probably have functions like
> >>>>>> add_gem_clk, add_peripheral_clk, add_pll_clk and so on in your monolithic drivers.
> >>>>> It probably makes sense to differ between having custom drivers and
> >>>>> describing the whole clock tree in DT.
> >>>>>
> >>>>> From reusability perspective, it may make sense to factor out some code
> >>>>> in drivers. IMHO, this does only apply to the peripheral clocks, since
> >>>>> everything else isn't reused and/or quite Zynq specific.
> >>>>> But a monolithic approach would not even prevent this. You could just
> >>>>> transparently change the implementation: Just add a clock driver,
> >>>>> replace the original code in the controller to use the new driver
> >>>>> instead and you're good. No need to touch DT bindings.
> >>>>>
> >>>>> In Zynq a peripheral clock is essentially described by:
> >>>>>         clk = clk_register_mux(NULL, mux_name, parents, 4, 0,                    
> >>>>>                         clk_ctrl, 4, 2, 0, lock);                                
> >>>>>                                                                                  
> >>>>>         clk = clk_register_divider(NULL, div_name, mux_name, 0, clk_ctrl, 8, 6,  
> >>>>>                         CLK_DIVIDER_ONE_BASED, lock);                            
> >>>>>                                                                                  
> >>>>>         clks[clk0] = clk_register_gate(NULL, clk_name0, div_name,                
> >>>>>                         CLK_SET_RATE_PARENT, clk_ctrl, 0, 0, lock);              
> >>>>>
> >>>>> If we wrapped this by a driver, any code outside of that driver couldn't
> >>>>> change the mux setting, since its struct clk* is not exposed outside.
> >>>>> To get around this we'd have to reimplement all the clock ops, to create a
> >>>>> clock which supports all possible operations.
> >>>>> In the monolithic approach we could simply remember that struct clk* and
> >>>>> work with it.
> >>>>>
> >>>>> Bottom line: Factoring out some parts of the monolithic driver might
> >>>>> make sense for some parts. But it can be done later in a transparent way,
> >>>>> especially w/o touching DT bindings.
> >>>>>
> >>>>>
> >>>>> The other thing is describing the whole clock tree in DT:
> >>>>> That would force us to not only describe clocks for which it might make
> >>>>> sense, but also all Zynq specific clocks in custom drivers and DT
> >>>>> bindings and we gain nothing from it.
> >>>>>
> >>>>>>
> >>>>>> The SLCR is a virtual construct, which groups the clock units together. But the
> >>>>>> clock units are the actual entities. E.g. imagine a Zync 2.0, it would probably
> >>>>>> reuse the same clock units, but there would quite likely be different clocks
> >>>>>> mapped at different addresses. If you choose a non-monolithic implementation
> >>>>>> you'd just have to update your devicetree to make this work, with a monolithic
> >>>>>> version you'd have to add a almost identical driver for the v2.
> >>>>> Either way you'll end up with a lot of lines describing the hierarchy.
> >>>>> Either in the dts or in the clock driver.
> >>>>>
> >>>>>
> >>>>> I think, my problem is, that I do not yet know how certain functionality
> >>>>> will be implemented later and what exact use-cases have to be supported.
> >>>>> With the monolithic approach I keep all options open. We hopefully will
> >>>>> never have to touch its DT bindings again. And refactoring the code and
> >>>>> migrating it to use dedicated clock drivers should be transparent
> >>>>> changes, if deemed beneficial.
> >>>>> Furthermore, I have a driver, which has references to all strucht clk*
> >>>>> in the system and can work with the whole clock tree leveraging a system
> >>>>> level view. Separating things in smaller blocks might complicate things
> >>>>> if a use case requires this.
> >>>>> And finally pushing the whole hierarchy description into the DT seems to
> >>>>> be the most restrictive approach, without offering big rewards.
> >>>>
> >>>> I don't see those restrictions you see with a dt binding which has nodes for
> >>>> each clock block. You seem to be under the impression, that each device tree
> >>>> node requires its own driver or device. This is not the case, have you looked
> >>>> at how the current upstream zynq clock support is implemented? This is still
> >>>> one monolithic driver, but there are multiple dt nodes, one for each clock
> >>>> block.
> >>> I thought the one "huge messy" file was one thing you wanted to avoid?
> >>
> >> I want to avoid a messy one, yes.
> >>
> >>> Also, how does it address my concern regarding inaccessible
> >>> 'struct clk *'? A clk_register_foo() will only return a single struct
> >>> clk *. So, wrapping several clk_register() calls within one
> >>> clk_register_foo() makes all but the actual output inaccessible.
> >>> And if we do not directly call clk_register_foo() but do the probing
> >>> through DT the clock init code does not even get hold of that struct clk
> >>> *.
> >>
> >> That's what the composite clk driver is for.
> >> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg4k04590.html
> > Is it merged by now? Last time I checked it wasn't and I was seeking a
> > solution w/o this driver. And I think it wouldn't fit all clocks. E.g. the
> > ones with multiple dividers or gates.
> 
> Not yet, but hopefully soon.
> 
> For the clocks with multiple dividers we want a custom clk driver, at least
> for the divider part, otherwise implementing clk_set_rate wouldn't be so easy.
> 
> >>
> >>>
> >>>> This leads to a very clean and well structured code and also nicely
> >>>> structured dt, which represents the tree as it is in hardware. If I understand
> >>>> you correctly what you suggest is that you basically want to copy 'n paste the
> >>>> same 10 lines to instantiate a peripheral clock, which you mentioned above,
> >>>> again and again. This will be quite messy.
> >>> For the peripheral clocks you're right, there is some reuse possible.
> >>> For those I have a helper, like zynq_clk_register_periph(). But for pretty much
> >>> everything else I don't.
> >>>
> >>> In conclusion, I see the DT approach limiting my ability to modify the
> >>> clock tree when implementing system level functionality. 
> >>> Probably quite a stretch, but imagine some smart power management code,
> >>> which might try to reparent all and every clock in the system to certain
> >>> PLLs. How would such a power manager get hold of all the struct clk * it
> >>> needs. In a system probing fully through DT we cannot get hold of those
> >>> and even less if we wrap several clocks in one clock_foo. Unless, we add
> >>> a dummy device which has every clock in the system as its input. And
> >>> that sounds even wackier than having a clock controller which provides
> >>> all the clocks to consumers.
> >>
> >> If you really really need a list of all the zynq clocks the easiest way to
> >> implement this is to add a global list and whenever a clock gets probed via
> >> dt add the clock to the list.
> > A simple list wouldn't do it. I need exact knowledge about which clock
> > is which. So, I pretty much copied the Tegra approach with a
> > 'struct clk *clks[clk_max]' array. And filling such an array from a generic clock
> > driver seems wrong.
> > By separating clocks we take away their awareness of the bigger
> > hierarchy picture. The clock controller approach maintains this knowledge.
> 
> If you already have the code can you upload it somewhere, this might make it
> easier for me to understand why the solution I have in mind won't work.
Should be accessible here:
https://github.com/sorenb-xlnx/linux-xlnx/tree/zynq-clkc

	Sören

Re: RFC v2: Zynq Clock Controller

[Stephen Warren]

On 03/22/2013 04:41 PM, Sören Brinkmann wrote:
> Hi Lars,
> 
> On Thu, Mar 21, 2013 at 07:32:52PM +0100, Lars-Peter Clausen wrote:
>> On 03/21/2013 12:56 AM, Sören Brinkmann wrote:
>>> Hi,
>>>
>>> I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq:
>>>
>>> Required properties:
>>>  - #clock-cells : Must be 1
>>>  - compatible : "xlnx,ps7-clkc"  (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline)
>>>  - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
>>>                      (usually 33 MHz oscillators are used for Zynq platforms)
>>>  - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below.
>>>
>>> Optional properties:
>>>  - clocks : as described in the clock bindings
>>>  - clock-names : as described in the clock bindings
>>>
>>> Clock inputs:
>>> The following strings are optional parameters to the 'clock-names' property in
>>> order to provide optional (E)MIO clock sources.
>>>  - swdt_ext_clk
>>>  - gem0_emio_clk
>>>  - gem1_emio_clk
>>>  - mio_clk_XX          # with XX = 00..53
>>>
>>> Example:
>>>         clkc: clkc {
>>>                 #clock-cells = <1>;
>>>                 compatible = "xlnx,ps7-clkc";
>>>                 ps-clk-frequency = <33333333>;
>>
>> The input frequency should be a clock as well.
>
> Again, monolithic vs split. I don't see a reason not to just internally
> call clk_register_fixed_rate(). That way its children do not have to
> cope with a variable name for the xtal.
> Also, with my proposal 'clocks' and 'clock-names' would be purely
> optional properties, only required if optional external inputs are
> present. Having the xtal defined externally would add mandatory entries for
> those props.

But isn't the clock source board-specific? It's a completely separate
object from Zynq's own clock controller HW, and as such should be
represented by a separate DT node, right?

The issue with parent clock names is simply a red herring. A solution is
needed to registered clock with a parent clock object, rather than a
parent clock name. Then, the parent names are completely irrelevant.
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: RFC v2: Zynq Clock Controller

[Sören Brinkmann]

On Mon, Mar 25, 2013 at 05:29:33PM -0600, Stephen Warren wrote:
> On 03/22/2013 04:41 PM, Sören Brinkmann wrote:
> > Hi Lars,
> > 
> > On Thu, Mar 21, 2013 at 07:32:52PM +0100, Lars-Peter Clausen wrote:
> >> On 03/21/2013 12:56 AM, Sören Brinkmann wrote:
> >>> Hi,
> >>>
> >>> I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq:
> >>>
> >>> Required properties:
> >>>  - #clock-cells : Must be 1
> >>>  - compatible : "xlnx,ps7-clkc"  (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline)
> >>>  - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
> >>>                      (usually 33 MHz oscillators are used for Zynq platforms)
> >>>  - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below.
> >>>
> >>> Optional properties:
> >>>  - clocks : as described in the clock bindings
> >>>  - clock-names : as described in the clock bindings
> >>>
> >>> Clock inputs:
> >>> The following strings are optional parameters to the 'clock-names' property in
> >>> order to provide optional (E)MIO clock sources.
> >>>  - swdt_ext_clk
> >>>  - gem0_emio_clk
> >>>  - gem1_emio_clk
> >>>  - mio_clk_XX          # with XX = 00..53
> >>>
> >>> Example:
> >>>         clkc: clkc {
> >>>                 #clock-cells = <1>;
> >>>                 compatible = "xlnx,ps7-clkc";
> >>>                 ps-clk-frequency = <33333333>;
> >>
> >> The input frequency should be a clock as well.
> >
> > Again, monolithic vs split. I don't see a reason not to just internally
> > call clk_register_fixed_rate(). That way its children do not have to
> > cope with a variable name for the xtal.
> > Also, with my proposal 'clocks' and 'clock-names' would be purely
> > optional properties, only required if optional external inputs are
> > present. Having the xtal defined externally would add mandatory entries for
> > those props.
> 
> But isn't the clock source board-specific? It's a completely separate
> object from Zynq's own clock controller HW, and as such should be
> represented by a separate DT node, right?
Well, the ps-clk-frequency property would be board specific right. But
the same would apply for a fixed-rate clock. This is how it's currently
done in mainline. The zynq dtsi file defines the fixed-rate clock and
every board dts file overrides the fixed rate clock's frequency
property.
In my approach the fixed rate clock is within the clkc block and every
board dts has to provide this property accordingly.


> 
> The issue with parent clock names is simply a red herring. A solution is
> needed to registered clock with a parent clock object, rather than a
> parent clock name. Then, the parent names are completely irrelevant.
But we'd trade the naming problems with issues regarding the order of
how clocks are probed.
And it may not be as easy as just probe from the root towards the leafs
of the clock tree.
In Zynq I can route clocks generated in the processor part into the FPGA
and back using them as an input for the clock-controller. Ant the IP in
the FPGA could add another layer of clock providers in that loop.
With such circular dependencies a clock object based
registration/probing does not seem too easy to me.

	Sören

Re: RFC v2: Zynq Clock Controller

[Stephen Warren]

On 03/20/2013 05:56 PM, Sören Brinkmann wrote:
> Hi,
> 
> I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq:
> 
> Required properties:
>  - #clock-cells : Must be 1
>  - compatible : "xlnx,ps7-clkc"  (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline)
>  - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
>                      (usually 33 MHz oscillators are used for Zynq platforms)

This may have been mentioned before, but shouldn't the input clock be
represented as an actual clock in DT, and hence as an entry in this
node's clocks property? The crystal/... itself can be represented in DT
as a fixed-clock.

>  - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below.

That shouldn't be required.

> Optional properties:
>  - clocks : as described in the clock bindings
>  - clock-names : as described in the clock bindings

I think clocks should be required, with at least the main crystal clock
input always present, but perhaps having some optional entries for the
(E)MIO feature you mention.

> Example:
>         clkc: clkc {
>                 #clock-cells = <1>;
>                 compatible = "xlnx,ps7-clkc";
>                 ps-clk-frequency = <33333333>;
>                 clock-output-names = "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", "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", "swdt", "dbg_trc", "dbg_apb";  /* long list... explanation below */
>                 /* optional props */
>                 clocks = <&clkc 16>, <&clk_foo>;
>                 clock-names = "gem1_emio_clk", "can_mio_clk_23";
>         };
> 
> The downside of supporting this is, that I don't see a way around explicitly listing the clock output names in the DT.

(Please wrap your emails to ~74 characters or so)

As Mike mentioned off-list, one can create a new clk registration API
that takes a struct clk* as parent rather than a char *clk_name.

> This email and any attachments are intended for the sole use of the named recipient(s)...

It's not good form to include that kind of disclaimer on public mailing
list posts. That's why many people use their personal email when posting
to mailing lists.
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: RFC v2: Zynq Clock Controller

[Sören Brinkmann]

Hi Stephen,

On Mon, Mar 25, 2013 at 12:13:08PM -0600, Stephen Warren wrote:
> On 03/20/2013 05:56 PM, Sören Brinkmann wrote:
> > Hi,
> > 
> > I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq:
> > 
> > Required properties:
> >  - #clock-cells : Must be 1
> >  - compatible : "xlnx,ps7-clkc"  (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline)
> >  - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
> >                      (usually 33 MHz oscillators are used for Zynq platforms)
> 
> This may have been mentioned before, but shouldn't the input clock be
> represented as an actual clock in DT, and hence as an entry in this
> node's clocks property? The crystal/... itself can be represented in DT
> as a fixed-clock.
Lars-Peter brought that up, too. Please refer to my answer to him.

> 
> >  - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below.
> 
> That shouldn't be required.
When I want to support of_clk_get_parent_name() for my clocks, I think
it is. And I'm inclined to not brake this functionality.

> 
> > Optional properties:
> >  - clocks : as described in the clock bindings
> >  - clock-names : as described in the clock bindings
> 
> I think clocks should be required, with at least the main crystal clock
> input always present, but perhaps having some optional entries for the
> (E)MIO feature you mention.
This is why I have the xtal separate. This way these props are purely
optional and the xtal frequency is obtained separately. It also makes it
a little easier internally, because I don't have to cope with a variable
name for the xtal this way.

Describing the xtal as fixed clock in DT means a mandatory entry for
'clocks' and 'clock-names' and a variable name for the xtal clock. I
wanted to avoid this.

> 
> > Example:
> >         clkc: clkc {
> >                 #clock-cells = <1>;
> >                 compatible = "xlnx,ps7-clkc";
> >                 ps-clk-frequency = <33333333>;
> >                 clock-output-names = "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", "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", "swdt", "dbg_trc", "dbg_apb";  /* long list... explanation below */
> >                 /* optional props */
> >                 clocks = <&clkc 16>, <&clk_foo>;
> >                 clock-names = "gem1_emio_clk", "can_mio_clk_23";
> >         };
> > 
> > The downside of supporting this is, that I don't see a way around explicitly listing the clock output names in the DT.
> 
> (Please wrap your emails to ~74 characters or so)
I changed my settings.

> 
> As Mike mentioned off-list, one can create a new clk registration API
> that takes a struct clk* as parent rather than a char *clk_name.
Then we also have to make sure clocks are probed in a specific order. To
obtain a 'struct clk *' through clk_get() the requested clock has to be
already been probed. Currently clock probing relies purely on data present
in DT. This makes this proposal not that trivial, IMHO.

e.g. in this case the clock controller probe would fail if the potential
fixed clock describing the xtal isn't probed first.

> 
> > This email and any attachments are intended for the sole use of the named recipient(s)...
> 
> It's not good form to include that kind of disclaimer on public mailing
> list posts. That's why many people use their personal email when posting
> to mailing lists.
My apologies. IT is currently messing with the email configuration and broke
the flows which used to work. AFAIK, it's currently fixed. And I
sincerely hope it stays this way.

	Sören

Re: RFC v2: Zynq Clock Controller

[Stephen Warren]

On 03/25/2013 12:27 PM, Sören Brinkmann wrote:
> Hi Stephen,
> 
> On Mon, Mar 25, 2013 at 12:13:08PM -0600, Stephen Warren wrote:
>> On 03/20/2013 05:56 PM, Sören Brinkmann wrote:
>>> Hi,
>>>
>>> I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq:
>>>
>>> Required properties:
>>>  - #clock-cells : Must be 1
>>>  - compatible : "xlnx,ps7-clkc"  (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline)
>>>  - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
>>>                      (usually 33 MHz oscillators are used for Zynq platforms)
>>
>> This may have been mentioned before, but shouldn't the input clock be
>> represented as an actual clock in DT, and hence as an entry in this
>> node's clocks property? The crystal/... itself can be represented in DT
>> as a fixed-clock.
> Lars-Peter brought that up, too. Please refer to my answer to him.
> 
>>
>>>  - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below.
>>
>> That shouldn't be required.
>
> When I want to support of_clk_get_parent_name() for my clocks, I think
> it is. And I'm inclined to not brake this functionality.

The solution here is to make clock parent names irrelevant.

Also note that device tree is supposed to describe HW. As such, this
kind of internal implementation detail of the Linux clock driver should
have basically zero effect on the DT binding definition.

>>> Optional properties:
>>>  - clocks : as described in the clock bindings
>>>  - clock-names : as described in the clock bindings
>>
>> I think clocks should be required, with at least the main crystal clock
>> input always present, but perhaps having some optional entries for the
>> (E)MIO feature you mention.
>
> This is why I have the xtal separate. This way these props are purely
> optional and the xtal frequency is obtained separately. It also makes it
> a little easier internally, because I don't have to cope with a variable
> name for the xtal this way.
> 
> Describing the xtal as fixed clock in DT means a mandatory entry for
> 'clocks' and 'clock-names' and a variable name for the xtal clock. I
> wanted to avoid this.

I don't see any benefit with some properties being purely optional.
Having optional entries in a property seems just fine to me.

The name of the crystal clock should be irrelevant; that issue simply
needs to be fixed. It's driving too much of this discussion, and it will
be irrelevant once it's fixed.

>>> Example:
>>>         clkc: clkc {
>>>                 #clock-cells = <1>;
>>>                 compatible = "xlnx,ps7-clkc";
>>>                 ps-clk-frequency = <33333333>;
>>>                 clock-output-names = "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", "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", "swdt", "dbg_trc", "dbg_apb";  /* long list... explanation below */
>>>                 /* optional props */
>>>                 clocks = <&clkc 16>, <&clk_foo>;
>>>                 clock-names = "gem1_emio_clk", "can_mio_clk_23";
>>>         };
>>>
>>> The downside of supporting this is, that I don't see a way around explicitly listing the clock output names in the DT.
>>
>> (Please wrap your emails to ~74 characters or so)
> I changed my settings.
> 
>>
>> As Mike mentioned off-list, one can create a new clk registration API
>> that takes a struct clk* as parent rather than a char *clk_name.
>
> Then we also have to make sure clocks are probed in a specific order. To
> obtain a 'struct clk *' through clk_get() the requested clock has to be
> already been probed. Currently clock probing relies purely on data present
> in DT. This makes this proposal not that trivial, IMHO.

Simply use deferred probe.

Re: RFC v2: Zynq Clock Controller

[Sören Brinkmann]

On Mon, Mar 25, 2013 at 05:33:10PM -0600, Stephen Warren wrote:
> On 03/25/2013 12:27 PM, Sören Brinkmann wrote:
> > Hi Stephen,
> > 
> > On Mon, Mar 25, 2013 at 12:13:08PM -0600, Stephen Warren wrote:
> >> On 03/20/2013 05:56 PM, Sören Brinkmann wrote:
> >>> Hi,
> >>>
> >>> I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq:
> >>>
> >>> Required properties:
> >>>  - #clock-cells : Must be 1
> >>>  - compatible : "xlnx,ps7-clkc"  (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline)
> >>>  - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
> >>>                      (usually 33 MHz oscillators are used for Zynq platforms)
> >>
> >> This may have been mentioned before, but shouldn't the input clock be
> >> represented as an actual clock in DT, and hence as an entry in this
> >> node's clocks property? The crystal/... itself can be represented in DT
> >> as a fixed-clock.
> > Lars-Peter brought that up, too. Please refer to my answer to him.
> > 
> >>
> >>>  - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below.
> >>
> >> That shouldn't be required.
> >
> > When I want to support of_clk_get_parent_name() for my clocks, I think
> > it is. And I'm inclined to not brake this functionality.
> 
> The solution here is to make clock parent names irrelevant.
> 
> Also note that device tree is supposed to describe HW. As such, this
> kind of internal implementation detail of the Linux clock driver should
> have basically zero effect on the DT binding definition.
> 
> >>> Optional properties:
> >>>  - clocks : as described in the clock bindings
> >>>  - clock-names : as described in the clock bindings
> >>
> >> I think clocks should be required, with at least the main crystal clock
> >> input always present, but perhaps having some optional entries for the
> >> (E)MIO feature you mention.
> >
> > This is why I have the xtal separate. This way these props are purely
> > optional and the xtal frequency is obtained separately. It also makes it
> > a little easier internally, because I don't have to cope with a variable
> > name for the xtal this way.
> > 
> > Describing the xtal as fixed clock in DT means a mandatory entry for
> > 'clocks' and 'clock-names' and a variable name for the xtal clock. I
> > wanted to avoid this.
> 
> I don't see any benefit with some properties being purely optional.
> Having optional entries in a property seems just fine to me.
> 
> The name of the crystal clock should be irrelevant; that issue simply
> needs to be fixed. It's driving too much of this discussion, and it will
> be irrelevant once it's fixed.
> 
> >>> Example:
> >>>         clkc: clkc {
> >>>                 #clock-cells = <1>;
> >>>                 compatible = "xlnx,ps7-clkc";
> >>>                 ps-clk-frequency = <33333333>;
> >>>                 clock-output-names = "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", "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", "swdt", "dbg_trc", "dbg_apb";  /* long list... explanation below */
> >>>                 /* optional props */
> >>>                 clocks = <&clkc 16>, <&clk_foo>;
> >>>                 clock-names = "gem1_emio_clk", "can_mio_clk_23";
> >>>         };
> >>>
> >>> The downside of supporting this is, that I don't see a way around explicitly listing the clock output names in the DT.
> >>
> >> (Please wrap your emails to ~74 characters or so)
> > I changed my settings.
> > 
> >>
> >> As Mike mentioned off-list, one can create a new clk registration API
> >> that takes a struct clk* as parent rather than a char *clk_name.
> >
> > Then we also have to make sure clocks are probed in a specific order. To
> > obtain a 'struct clk *' through clk_get() the requested clock has to be
> > already been probed. Currently clock probing relies purely on data present
> > in DT. This makes this proposal not that trivial, IMHO.
> 
> Simply use deferred probe.
This would require major changes to the whole clock probing mechanism.
Currently, clocks can not defer probing. And in case of circular
dependencies in the clock tree, it would rather require a multiple steps
probe. Simply deferring won't be enough.

	Sören

Re: RFC v2: Zynq Clock Controller

[Mike Turquette]

Quoting Sören Brinkmann (2013-03-25 17:03:24)
> On Mon, Mar 25, 2013 at 05:33:10PM -0600, Stephen Warren wrote:
> > On 03/25/2013 12:27 PM, Sören Brinkmann wrote:
> > > Hi Stephen,
> > > 
> > > On Mon, Mar 25, 2013 at 12:13:08PM -0600, Stephen Warren wrote:
> > >> On 03/20/2013 05:56 PM, Sören Brinkmann wrote:
> > >>> Hi,
> > >>>
> > >>> I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq:
> > >>>
> > >>> Required properties:
> > >>>  - #clock-cells : Must be 1
> > >>>  - compatible : "xlnx,ps7-clkc"  (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline)
> > >>>  - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
> > >>>                      (usually 33 MHz oscillators are used for Zynq platforms)
> > >>
> > >> This may have been mentioned before, but shouldn't the input clock be
> > >> represented as an actual clock in DT, and hence as an entry in this
> > >> node's clocks property? The crystal/... itself can be represented in DT
> > >> as a fixed-clock.
> > > Lars-Peter brought that up, too. Please refer to my answer to him.
> > > 
> > >>
> > >>>  - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below.
> > >>
> > >> That shouldn't be required.
> > >
> > > When I want to support of_clk_get_parent_name() for my clocks, I think
> > > it is. And I'm inclined to not brake this functionality.
> > 
> > The solution here is to make clock parent names irrelevant.
> > 
> > Also note that device tree is supposed to describe HW. As such, this
> > kind of internal implementation detail of the Linux clock driver should
> > have basically zero effect on the DT binding definition.
> > 
> > >>> Optional properties:
> > >>>  - clocks : as described in the clock bindings
> > >>>  - clock-names : as described in the clock bindings
> > >>
> > >> I think clocks should be required, with at least the main crystal clock
> > >> input always present, but perhaps having some optional entries for the
> > >> (E)MIO feature you mention.
> > >
> > > This is why I have the xtal separate. This way these props are purely
> > > optional and the xtal frequency is obtained separately. It also makes it
> > > a little easier internally, because I don't have to cope with a variable
> > > name for the xtal this way.
> > > 
> > > Describing the xtal as fixed clock in DT means a mandatory entry for
> > > 'clocks' and 'clock-names' and a variable name for the xtal clock. I
> > > wanted to avoid this.
> > 
> > I don't see any benefit with some properties being purely optional.
> > Having optional entries in a property seems just fine to me.
> > 
> > The name of the crystal clock should be irrelevant; that issue simply
> > needs to be fixed. It's driving too much of this discussion, and it will
> > be irrelevant once it's fixed.
> > 
> > >>> Example:
> > >>>         clkc: clkc {
> > >>>                 #clock-cells = <1>;
> > >>>                 compatible = "xlnx,ps7-clkc";
> > >>>                 ps-clk-frequency = <33333333>;
> > >>>                 clock-output-names = "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", "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", "swdt", "dbg_trc", "dbg_apb";  /* long list... explanation below */
> > >>>                 /* optional props */
> > >>>                 clocks = <&clkc 16>, <&clk_foo>;
> > >>>                 clock-names = "gem1_emio_clk", "can_mio_clk_23";
> > >>>         };
> > >>>
> > >>> The downside of supporting this is, that I don't see a way around explicitly listing the clock output names in the DT.
> > >>
> > >> (Please wrap your emails to ~74 characters or so)
> > > I changed my settings.
> > > 
> > >>
> > >> As Mike mentioned off-list, one can create a new clk registration API
> > >> that takes a struct clk* as parent rather than a char *clk_name.
> > >
> > > Then we also have to make sure clocks are probed in a specific order. To
> > > obtain a 'struct clk *' through clk_get() the requested clock has to be
> > > already been probed. Currently clock probing relies purely on data present
> > > in DT. This makes this proposal not that trivial, IMHO.
> > 
> > Simply use deferred probe.
> This would require major changes to the whole clock probing mechanism.

Which mechanism are you referring to?

> Currently, clocks can not defer probing. And in case of circular
> dependencies in the clock tree, it would rather require a multiple steps
> probe. Simply deferring won't be enough.

Circular dependencies in the clock tree?

Anyways the clock core maintains an orphan list that was originally
designed for this problem: registering a clock before that clock's
parent is registered.  Just for kicks I have actually registered all of
OMAP4's clocks in exactly the opposite order and everything still works.
Of course this makes the clock registration parent-discovery code
O(n^2), but that is the same worst case for deferred probe.

As long as a key exists to link two clocks together then registration
order shouldn't matter.  Originally this key was an immutable clk name
string.

If clock parents are specified in DT then that should also suffice, in
place of doing string comparisons.

If no key to pair up clocks exists then yes we will have to rely purely
on the struct clk *opaque_cookie and register clocks in order.

Regards,
Mike

> 
>         Sören
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: RFC v2: Zynq Clock Controller

[Sören Brinkmann]

Hi Mike,

On Tue, Mar 26, 2013 at 07:18:44PM -0700, Mike Turquette wrote:
> Quoting Sören Brinkmann (2013-03-25 17:03:24)
> > On Mon, Mar 25, 2013 at 05:33:10PM -0600, Stephen Warren wrote:
> > > On 03/25/2013 12:27 PM, Sören Brinkmann wrote:
> > > > Hi Stephen,
> > > > 
> > > > On Mon, Mar 25, 2013 at 12:13:08PM -0600, Stephen Warren wrote:
> > > >> On 03/20/2013 05:56 PM, Sören Brinkmann wrote:
> > > >>> Hi,
> > > >>>
> > > >>> I spent some time working on this and incorporating feedback. Here's an updated proposal for a clock controller for Zynq:
> > > >>>
> > > >>> Required properties:
> > > >>>  - #clock-cells : Must be 1
> > > >>>  - compatible : "xlnx,ps7-clkc"  (this may become 'xlnx,zynq-clkc' terminology differs a bit between Xilinx internal and mainline)
> > > >>>  - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
> > > >>>                      (usually 33 MHz oscillators are used for Zynq platforms)
> > > >>
> > > >> This may have been mentioned before, but shouldn't the input clock be
> > > >> represented as an actual clock in DT, and hence as an entry in this
> > > >> node's clocks property? The crystal/... itself can be represented in DT
> > > >> as a fixed-clock.
> > > > Lars-Peter brought that up, too. Please refer to my answer to him.
> > > > 
> > > >>
> > > >>>  - clock-output-names : List of strings used to name the clock outputs. Shall be a list of the outputs given below.
> > > >>
> > > >> That shouldn't be required.
> > > >
> > > > When I want to support of_clk_get_parent_name() for my clocks, I think
> > > > it is. And I'm inclined to not brake this functionality.
> > > 
> > > The solution here is to make clock parent names irrelevant.
> > > 
> > > Also note that device tree is supposed to describe HW. As such, this
> > > kind of internal implementation detail of the Linux clock driver should
> > > have basically zero effect on the DT binding definition.
> > > 
> > > >>> Optional properties:
> > > >>>  - clocks : as described in the clock bindings
> > > >>>  - clock-names : as described in the clock bindings
> > > >>
> > > >> I think clocks should be required, with at least the main crystal clock
> > > >> input always present, but perhaps having some optional entries for the
> > > >> (E)MIO feature you mention.
> > > >
> > > > This is why I have the xtal separate. This way these props are purely
> > > > optional and the xtal frequency is obtained separately. It also makes it
> > > > a little easier internally, because I don't have to cope with a variable
> > > > name for the xtal this way.
> > > > 
> > > > Describing the xtal as fixed clock in DT means a mandatory entry for
> > > > 'clocks' and 'clock-names' and a variable name for the xtal clock. I
> > > > wanted to avoid this.
> > > 
> > > I don't see any benefit with some properties being purely optional.
> > > Having optional entries in a property seems just fine to me.
> > > 
> > > The name of the crystal clock should be irrelevant; that issue simply
> > > needs to be fixed. It's driving too much of this discussion, and it will
> > > be irrelevant once it's fixed.
> > > 
> > > >>> Example:
> > > >>>         clkc: clkc {
> > > >>>                 #clock-cells = <1>;
> > > >>>                 compatible = "xlnx,ps7-clkc";
> > > >>>                 ps-clk-frequency = <33333333>;
> > > >>>                 clock-output-names = "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", "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", "swdt", "dbg_trc", "dbg_apb";  /* long list... explanation below */
> > > >>>                 /* optional props */
> > > >>>                 clocks = <&clkc 16>, <&clk_foo>;
> > > >>>                 clock-names = "gem1_emio_clk", "can_mio_clk_23";
> > > >>>         };
> > > >>>
> > > >>> The downside of supporting this is, that I don't see a way around explicitly listing the clock output names in the DT.
> > > >>
> > > >> (Please wrap your emails to ~74 characters or so)
> > > > I changed my settings.
> > > > 
> > > >>
> > > >> As Mike mentioned off-list, one can create a new clk registration API
> > > >> that takes a struct clk* as parent rather than a char *clk_name.
> > > >
> > > > Then we also have to make sure clocks are probed in a specific order. To
> > > > obtain a 'struct clk *' through clk_get() the requested clock has to be
> > > > already been probed. Currently clock probing relies purely on data present
> > > > in DT. This makes this proposal not that trivial, IMHO.
> > > 
> > > Simply use deferred probe.
> > This would require major changes to the whole clock probing mechanism.
> 
> Which mechanism are you referring to?
of_clk_init()

> 
> > Currently, clocks can not defer probing. And in case of circular
> > dependencies in the clock tree, it would rather require a multiple steps
> > probe. Simply deferring won't be enough.
> 
> Circular dependencies in the clock tree?
Yes, I can route a clock from the processor part into the FPGA and back
to be used as source for a couple of peripherals. And in the FPGA there
may be all kinds of logic altering that clock.

> 
> Anyways the clock core maintains an orphan list that was originally
> designed for this problem: registering a clock before that clock's
> parent is registered.  Just for kicks I have actually registered all of
> OMAP4's clocks in exactly the opposite order and everything still works.
> Of course this makes the clock registration parent-discovery code
> O(n^2), but that is the same worst case for deferred probe.
> 
> As long as a key exists to link two clocks together then registration
> order shouldn't matter.  Originally this key was an immutable clk name
> string.
> 
> If clock parents are specified in DT then that should also suffice, in
> place of doing string comparisons.
> 
> If no key to pair up clocks exists then yes we will have to rely purely
> on the struct clk *opaque_cookie and register clocks in order.
Okay, so I guess, the fundamental mechanisms should be in place. What is
needed, is changing the key from being the clk name to something else.
And preferably this something else is made up of something available
through DT, so a child can reference its parent w/o the parent to be
probed first. E.g. the provider's node name + the index of the clock
output?
You mentioned, something like this was supposed to happen anyway, right?
Do you already have something specific in mind/are working on a
solution?

	Sören

Re: RFC v2: Zynq Clock Controller

[Mike Turquette]

Quoting Sören Brinkmann (2013-03-29 15:36:18)
> On Tue, Mar 26, 2013 at 07:18:44PM -0700, Mike Turquette wrote:
> > As long as a key exists to link two clocks together then registration
> > order shouldn't matter.  Originally this key was an immutable clk name
> > string.
> > 
> > If clock parents are specified in DT then that should also suffice, in
> > place of doing string comparisons.
> > 
> > If no key to pair up clocks exists then yes we will have to rely purely
> > on the struct clk *opaque_cookie and register clocks in order.
> Okay, so I guess, the fundamental mechanisms should be in place. What is
> needed, is changing the key from being the clk name to something else.
> And preferably this something else is made up of something available
> through DT, so a child can reference its parent w/o the parent to be
> probed first. E.g. the provider's node name + the index of the clock
> output?

Right, order shouldn't matter in the sense that registering a child
before a parent should not constitute an absolute failure from the clock
framework point of view.

Folks using DT today still rely on the parent string name to link things
up.  Grep around for usage of of_clk_get_parent_name to see how several
platforms use that during registration.

> You mentioned, something like this was supposed to happen anyway, right?
> Do you already have something specific in mind/are working on a
> solution?
> 

I'm not looking at it currently.  If no one else gets around to it then
eventually I will.  It would be nice to not have to use
of_clk_get_parent_name and instead have a better way to establish
parent-child relationships at clock registration-time for DT clocks.

Regards,
Mike

>         Sören
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss