On Fri, Aug 16, 2013 at 12:43:31PM +0800, Nicolin Chen wrote:

Hi Tomasz,

Thank you for the comments. I'll revise them in v6. And below is my reply for you comments.

On Thu, Aug 15, 2013 at 02:18:22PM +0200, Tomasz Figa wrote:

...

...

• • clock-names : Includes the following entries:
• name type comments
• "core" Required The core clock of spdif controller
• "rx" Optional Rx clock source for spdif record.

• 			If absent, will use core clock.
  

• "tx" Optional Tx clock source for spdif playback.

• 			If absent, will use core clock.
  

• "tx-32000" Optional Tx clock source for 32000Hz sample rate

• 			playback. If absent, will use tx clock.
  

• "tx-44100" Optional Tx clock source for 44100Hz sample rate

• 			playback. If absent, will use tx clock.
  

• "tx-48000" Optional Tx clock source for 48000Hz sample rate

• 			playback. If absent, will use tx clock.
  

• "src<0-7>" Optional Clock source list for tx and rx clock

• 			to look up their clock source indexes.
  

• 			This clock list should be identical to
  

• 			the list of TxClk_Source bit value of
  

• 			register SPDIF_STC. If absent or failed
  

• 			to look up, tx and rx clock would then
  

• 			ignore the "rx", "tx" "tx-32000",
  

• 			"tx-44100", "tx-48000" clock phandles
  

• 			and select the core clock as default
  

• 			tx and rx clock.
  

I suspect a little abuse of clocks property here. From the description of "core" and "src<0-7>" clocks I assume that the IP can have up to 9 clock inputs - core clock and up to 8 extra source clocks. Is it correct?

If yes, this makes the "tx", "rx" and "tx-*" clocks describe configuration, not hardware. IMHO it should be up to the driver which source clocks to use for tx and rx channels and for each sampling rate.

First, you are right that all the properties you just commented are software configurations. And I got the point that device tree now can't allow any software configuration even if the actual hardware connection will depend on it.

If so, I would like to remove those abused clocks and also drop the unused clocks in src<0-7>, then just remain those needed clocks src. I think that can be plausible because there'll be no more clock abuse and the driver will be able to get the source index from the name 'src<num>'.

And you are right about the 9 clock inputs, just there're not only 9 inputs but also an extra external clock from S/PDIF transmitter via coaxial cable or optical fiber -- RxCLK. Please check the following list:

0000 if (DPLL Locked) SPDIF_RxClk else extal 0001 if (DPLL Locked) SPDIF_RxClk else spdif_clk 0010 if (DPLL Locked) SPDIF_RxClk else asrc_clk 0011 if (DPLL Locked) SPDIF_RxClk else spdif_extclk 0100 if (DPLL Locked) SPDIF_Rxclk else esai_hckt 0101 extal_clk 0110 spdif_clk 0111 asrc_clk 1000 spdif_extclk 1001 esai_hckt 1010 if (DPLL Locked) SPDIF_RxClk else mlb_clk 1011 if (DPLL Locked) SPDIF_RxClk else mlb_phy_clk 1100 mkb_clk 1101 mlb_phy_clk

When (DPLL Locked) condition matches, the rx clock can ignore the 8 input clocks from clock mux then use the external one from a S/PDIF transmitter.

So extal, spdif_clk, asrc_clk, spdif_extclk, esai_hckt, mlb_clk and mlb_phy_clk are clocks provided to the S/PDIF core and thus should be described in the devicetree.

Which of them the driver should use is configuration and thus normally should *not* be described in the devicetree. However, there may be no good way for the driver to know which clock to use in which case. There may be additional board requirements which are unknown to the driver. So in this case it might be valid to put the information which clock to use into the devicetree. But be aware that from the moment you put this information into the devicetree the driver is no longer free to chose the best clock, even if in future we find a good way to automatically guess the best clock. Do you have some insights in which case I would use which input clock? Is this only about which clock has the best suitable input frequency or is this also about synchronization of the audio signal with some other unit?

Likewise with the rx-clksrc-lock property. what are the reasons to enable/disable this property? Is this an option you want to change during runtime? Is it an option you always want to use when it's available?

If you don't know the answer then it might be a good option to just let the driver pick a sane default. If someone feels the need to change the default he probably comes up with a good reason why this is necessary. And then we can discuss again whether we want to have the option in the devicetree or some sysfs entry or whatever else.

Sascha

On Fri, Aug 16, 2013 at 04:01:25PM +0800, Nicolin Chen wrote:

Hi Sascha,

Thank you for the detailed comments.

On Fri, Aug 16, 2013 at 09:08:18AM +0200, Sascha Hauer wrote:

...

Which of them the driver should use is configuration and thus normally should *not* be described in the devicetree. However, there may be no good way for the driver to know which clock to use in which case. There may be additional board requirements which are unknown to the driver. So in this case it might be valid to put the information which clock to use into the devicetree. But be aware that from the moment you put this information into the devicetree the driver is no longer free to chose the best clock, even if in future we find a good way to automatically guess the best clock. Do you have some insights in which case I would use which input clock? Is this only about which clock has the best suitable input frequency or is this also about synchronization of the audio signal with some other unit?

I understand. What I'm thinking now is to let the driver find the best clock source for tx clock and a correspond divisor like this:

"tx<0-8>" Optional Tx clock source for spdif playback. If absent, will use core clock. The index from 0 to 8 is identical to the clock source list described in TxClk_Source bit of register STC. Multiple clock source are allowed for this tx clock source. The driver will select one source from them for each supported sample rate according to the clock rates of these provided clock sources.

You mean tx<0-7>.

Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC.

Please review this idea.

And likewise for rx:

"rx<0-16>" Optional Rx clock source for spdif record. If absent, will use core clock. The index from 0 to 16 is identical to the clock source list described in ClkSrc_Sel bit of register SRPC. If the index provided contains an "if (DPLL Locked)" condition in its source, the correspond clock phandle should be the one in "else" path. Only one rx clock source should be defined here.

Again, describe the input clocks *to* *the* *S/PDIF* *core* in the devicetree. Nothing more, nothing less. We've already been at the point where we realized that half of the above clocks only describe the 'PDLL locked' condition. Also the tx clocks are from what I see identical to the rx clocks. The following are the clocks:

clock-names: "core", "rxtx<0-7>" Required. The S/PDIF core has a core clock and 8 clocks which are muxed internally to provide input/output sample clocks.

This is all binding that is needed for now.

Sascha

On Fri, Aug 16, 2013 at 05:53:58PM +0800, Nicolin Chen wrote:

On Fri, Aug 16, 2013 at 10:56:32AM +0200, Sascha Hauer wrote:

...

...

"tx<0-8>" Optional Tx clock source for spdif playback. If absent, will use core clock. The index from 0 to 8 is identical to the clock source list described in TxClk_Source bit of register STC. Multiple clock source are allowed for this tx clock source. The driver will select one source from them for each supported sample rate according to the clock rates of these provided clock sources.

You mean tx<0-7>

Yes. Thank you.

...

Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC.

Some clocks are not from CCM and we haven't defined in imx6q-clk.txt, so in most cases we can't provide a phandle for them, eg: spdif_ext. I think it's a bit hard to force it to be 'required'. An 'optional' looks more flexible to me and a default one is ensured even if it's missing.

<&clks 0> is the dummy clock. This can be used for all input clocks not defined by the SoC.

spdif_ext would be a fixed clock on boards which provide it, but wiring this up would be the job of the board maintainer.

...

Again, describe the input clocks *to* *the* *S/PDIF* *core* in the devicetree. Nothing more, nothing less. We've already been at the point where we realized that half of the above clocks only describe the 'PDLL locked' condition. Also the tx clocks are from what I see identical to the rx clocks. The following are the clocks:

clock-names: "core", "rxtx<0-7>" Required. The S/PDIF core has a core clock and 8 clocks which are muxed internally to provide input/output sample clocks.

I know the reason why you suggest to combine two into 'rxtx<0-7>' is because the clock mux is defined so. And the previous suggestion 'the option required' is also because of it. But actually the rxclk itself, can be not only routed from the clock mux but also derived from DPLL of SPDIF Rx BLOCK as well. So, IMHO, it's more likely to be a fact that rxclk actually has 9 clock source, 8 from mux and 1 from DPLL. But why we here have to exclude it?

Because this is configuration, not hardware description.

WELL anyway, I know my opinion might not be concerned so much. So I would like to follow the suggestion as an expediency because I still wish this patch could be finally applied and merged into mainline :(

But I still don't get why we need to be so obsessed to make this impenetrable rule of devicetree that we here have to sacrifice something we could have reasonably done.

Look, it's really simple. Define the binding in a way that describes the hardware. Then use some sensible default in the driver for which clock to use. This doesn't have to cover all possible usecases, it only has to work. This is all that is necessary to get this driver mainline and will make most users happy. Then, later, someone might come along who needs more fine grained control over the clocks, but this guy is then able to justify *why* more control is needed.

This is not about getting a full featured driver into mainline. Get a basic driver into mainline and improve it later. You'll make it easier for us all.

Sascha

On Friday 16 of August 2013 12:11:51 Sascha Hauer wrote:

On Fri, Aug 16, 2013 at 05:53:58PM +0800, Nicolin Chen wrote:

...

On Fri, Aug 16, 2013 at 10:56:32AM +0200, Sascha Hauer wrote:

...

...

"tx<0-8>" Optional Tx clock source for spdif

playback.

...

...

...

		If absent, will use core clock.
		The index from 0 to 8 is identical
		to the clock source list described
		in TxClk_Source bit of register 

STC.

...

...

...

		Multiple clock source are allowed
		for this tx clock source. The 

driver

...

...

...

		will select one source from them 

for

...

...

...

		each supported sample rate 

according

...

...

...

		to the clock rates of these 

provided

...

...

...

		clock sources.

You mean tx<0-7>

Yes. Thank you.

...

Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC.

Some clocks are not from CCM and we haven't defined in imx6q-clk.txt, so in most cases we can't provide a phandle for them, eg: spdif_ext. I think it's a bit hard to force it to be 'required'. An 'optional' looks more flexible to me and a default one is ensured even if it's missing.

<&clks 0> is the dummy clock. This can be used for all input clocks not defined by the SoC.

Where does this assumption come from? Is it documented anywhere? What about cases when you have full description of clocks in device tree, with one node per clock and #clock-cells = <0>?

Best regards, Tomasz

On Saturday 17 of August 2013 16:53:16 Sascha Hauer wrote:

On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa wrote:

...

...

...

...

Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC.

Some clocks are not from CCM and we haven't defined in imx6q-clk.txt, so in most cases we can't provide a phandle for them, eg: spdif_ext. I think it's a bit hard to force it to be 'required'. An 'optional' looks more flexible to me and a default one is ensured even if it's missing.

<&clks 0> is the dummy clock. This can be used for all input clocks not defined by the SoC.

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

This is how all i.MX clock bindings currently are. See Documentation/devicetree/bindings/clock/imx*-clock.txt

OK, thanks.

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

If we really need a dummy clock, then we might also want a generic way to specify it.

Best regards, Tomasz

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

On Sat, Aug 17, 2013 at 04:17:18PM +0100, Tomasz Figa wrote:

...

On Saturday 17 of August 2013 16:53:16 Sascha Hauer wrote:

...

On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa wrote:

...

...

...

> Also I would make this option required. Use a dummy clock for > mux > inputs that are grounded for a specific SoC.

Some clocks are not from CCM and we haven't defined in imx6q-clk.txt, so in most cases we can't provide a phandle for them, eg: spdif_ext. I think it's a bit hard to force it to be 'required'. An 'optional' looks more flexible to me and a default one is ensured even if it's missing.

<&clks 0> is the dummy clock. This can be used for all input clocks not defined by the SoC.

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

This is how all i.MX clock bindings currently are. See Documentation/devicetree/bindings/clock/imx*-clock.txt

OK, thanks.

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

If we really need a dummy clock, then we might also want a generic way to specify it.

What do we actually mean by a "dummy clock"? We already have bindings for "fixed-clock" and co friends describe relatively simple preconfigured clocks.

Some platforms have a fake clock which defines noops callbacks and basically doesn't do anything. This is analogous to the dummy regulator implementation. A central one could be registered by the clock core, as is done by the regulator core.

I'll add this to the todo list.

Regards, Mike

If a clock isn't actually wired, we shouldn't describe it at all, or we're describing what Linux wants to think rather than what the hardware actually is. That can easily be handled with clock-names.

Thanks, Mark.

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

On Tue, Aug 20, 2013 at 01:06:25AM +0100, Mike Turquette wrote:

...

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

...

On Sat, Aug 17, 2013 at 04:17:18PM +0100, Tomasz Figa wrote:

...

On Saturday 17 of August 2013 16:53:16 Sascha Hauer wrote:

...

On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa wrote:

...

> > > Also I would make this option required. Use a dummy > > > clock for > > > mux > > > inputs that are grounded for a specific SoC. > > > > Some clocks are not from CCM and we haven't defined in > > imx6q-clk.txt, > > so in most cases we can't provide a phandle for them, eg: > > spdif_ext. > > I think it's a bit hard to force it to be 'required'. An > > 'optional' > > looks more flexible to me and a default one is ensured > > even if > > it's > > missing. > > <&clks 0> is the dummy clock. This can be used for all input > clocks > not > defined by the SoC.

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

This is how all i.MX clock bindings currently are. See Documentation/devicetree/bindings/clock/imx*-clock.txt

OK, thanks.

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

If we really need a dummy clock, then we might also want a generic way to specify it.

What do we actually mean by a "dummy clock"? We already have bindings for "fixed-clock" and co friends describe relatively simple preconfigured clocks.

Some platforms have a fake clock which defines noops callbacks and basically doesn't do anything. This is analogous to the dummy regulator implementation. A central one could be registered by the clock core, as is done by the regulator core.

When you say some platforms, you presumably mean the platform code in Linux? A dummy clock sounds like a completely Linux-specific abstraction rather than a description of the hardware, and I don't see why we need that in the DT:

• If a clock is wired up and running (as presumably the dummy clock is),

then surely it's a fixed-clock (it's running, we and we have no control over it, but we presumably know its rate) and can be described as such?

• If no clock is wired up, then we should be able to describe that. If a

driver believes that a clock is required when it isn't (for some level of functionality), then that driver should be fixed up to support the clock as being optional.

Am I missing something?

I second that.

Moreover, I don't think that device tree should deal with dummy anything. It should be able to describe hardware that is available on given system, not list what hardware is not available.

Best regards, Tomasz

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

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

...

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

...

On Tue, Aug 20, 2013 at 01:06:25AM +0100, Mike Turquette wrote:

...

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

...

On Sat, Aug 17, 2013 at 04:17:18PM +0100, Tomasz Figa wrote:

...

On Saturday 17 of August 2013 16:53:16 Sascha Hauer wrote: > On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa wrote: > > > > > Also I would make this option required. Use a dummy > > > > > clock for > > > > > mux > > > > > inputs that are grounded for a specific SoC. > > > > > > > > Some clocks are not from CCM and we haven't defined in > > > > imx6q-clk.txt, > > > > so in most cases we can't provide a phandle for them, eg: > > > > spdif_ext. > > > > I think it's a bit hard to force it to be 'required'. An > > > > 'optional' > > > > looks more flexible to me and a default one is ensured > > > > even if > > > > it's > > > > missing. > > > > > > <&clks 0> is the dummy clock. This can be used for all input > > > clocks > > > not > > > defined by the SoC. > > > > Where does this assumption come from? Is it documented > > anywhere? > > This is how all i.MX clock bindings currently are. See > Documentation/devicetree/bindings/clock/imx*-clock.txt

OK, thanks.

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

If we really need a dummy clock, then we might also want a generic way to specify it.

What do we actually mean by a "dummy clock"? We already have bindings for "fixed-clock" and co friends describe relatively simple preconfigured clocks.

Some platforms have a fake clock which defines noops callbacks and basically doesn't do anything. This is analogous to the dummy regulator implementation. A central one could be registered by the clock core, as is done by the regulator core.

When you say some platforms, you presumably mean the platform code in Linux? A dummy clock sounds like a completely Linux-specific abstraction rather than a description of the hardware, and I don't see why we need that in the DT:

• If a clock is wired up and running (as presumably the dummy clock is),

then surely it's a fixed-clock (it's running, we and we have no control over it, but we presumably know its rate) and can be described as such?

• If no clock is wired up, then we should be able to describe that. If a

driver believes that a clock is required when it isn't (for some level of functionality), then that driver should be fixed up to support the clock as being optional.

Am I missing something?

I second that.

Moreover, I don't think that device tree should deal with dummy anything. It should be able to describe hardware that is available on given system, not list what hardware is not available.

I wasn't clear. The dummy clock IS a completely Linux-specific abstraction.

I'm not advocating a dummy clock in DT. I am advocating consolidation of the implementation of a clock that does nothing into the clock core. This code could easily live in drivers/clk/clk.c instead of having everyone open-code it.

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

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

I'm guessing one of the reasons for such a clock are drivers do not honor the clk.h api and they freak out when clk_get gives them a NULL pointer?

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

Thanks, Mark.

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

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

...

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

...

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

...

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

...

On Tue, Aug 20, 2013 at 01:06:25AM +0100, Mike Turquette wrote:

...

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

> On Sat, Aug 17, 2013 at 04:17:18PM +0100, Tomasz Figa wrote: > > On Saturday 17 of August 2013 16:53:16 Sascha Hauer wrote: > > > On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa wrote: > > > > > > > Also I would make this option required. Use a dummy > > > > > > > clock for > > > > > > > mux > > > > > > > inputs that are grounded for a specific SoC. > > > > > > > > > > > > Some clocks are not from CCM and we haven't defined in > > > > > > imx6q-clk.txt, > > > > > > so in most cases we can't provide a phandle for them, eg: > > > > > > spdif_ext. > > > > > > I think it's a bit hard to force it to be 'required'. An > > > > > > 'optional' > > > > > > looks more flexible to me and a default one is ensured > > > > > > even if > > > > > > it's > > > > > > missing. > > > > > > > > > > <&clks 0> is the dummy clock. This can be used for all input > > > > > clocks > > > > > not > > > > > defined by the SoC. > > > > > > > > Where does this assumption come from? Is it documented > > > > anywhere? > > > > > > This is how all i.MX clock bindings currently are. See > > > Documentation/devicetree/bindings/clock/imx*-clock.txt > > > > OK, thanks. > > > > I guess we need some discussion on dummy clocks vs skipped clocks. > > I think we want some consistency on this, don't we? > > > > If we really need a dummy clock, then we might also want a generic > > way to specify it. > > What do we actually mean by a "dummy clock"? We already have > bindings > for "fixed-clock" and co friends describe relatively simple > preconfigured clocks.

Some platforms have a fake clock which defines noops callbacks and basically doesn't do anything. This is analogous to the dummy regulator implementation. A central one could be registered by the clock core, as is done by the regulator core.

When you say some platforms, you presumably mean the platform code in Linux? A dummy clock sounds like a completely Linux-specific abstraction rather than a description of the hardware, and I don't see why we need that in the DT:

• If a clock is wired up and running (as presumably the dummy clock is),

then surely it's a fixed-clock (it's running, we and we have no control over it, but we presumably know its rate) and can be described as such?

• If no clock is wired up, then we should be able to describe that. If a

driver believes that a clock is required when it isn't (for some level of functionality), then that driver should be fixed up to support the clock as being optional.

Am I missing something?

I second that.

Moreover, I don't think that device tree should deal with dummy anything. It should be able to describe hardware that is available on given system, not list what hardware is not available.

I wasn't clear. The dummy clock IS a completely Linux-specific abstraction.

I'm not advocating a dummy clock in DT. I am advocating consolidation of the implementation of a clock that does nothing into the clock core. This code could easily live in drivers/clk/clk.c instead of having everyone open-code it.

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

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

...

I'm guessing one of the reasons for such a clock are drivers do not honor the clk.h api and they freak out when clk_get gives them a NULL pointer?

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

The original reason introducing the dummy clocks in the i.MX dtbs was to provide devices a clock which the driver requests but is not software controllable. We often have the case where the same devices are on several SoCs, but not on all of them all clocks have a bit to en/disable them.

Anyway, to accomplish this we don't need dummy clocks. We can just describe the real clocks.

You could use a dummy clk for the Linux implementation, but the downside is that a dummy clock has a rate of 0 always and a your clocks likely have non-zero rates.

It is probably better for you define a clock which only implements the .recalc_rate callback. If the rate of this clock changes without Linux having knowledge of it you can use the CLK_GET_RATE_NOCACHE flag.

BTW with the S/PDIF core on which not all mux inputs are connected to actual clocks we could also describe the unconnected inputs as ground clocks with rate 0. This way we describe something which is really there instead of dummy clocks ;)

Again you could use a dummy clock for this OR a fixed-rate clock with a rate of zero from the perspective of the Linux implementation.

Do you think it worthwhile to have a DT binding for a grounded clock? That is not an entirely uncommon case.

Background to why it might be a good idea to connect a ground clock to the unconnected input pins is that a driver has a chance to successfully grab all clocks. Otherwise how does the driver distinguish between an unconnected and an erroneous clock?

Sorry, I don't follow this last question. Do you mean how to distinguish based on the value returned from clk_get?

Regards, Mike

Sascha

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

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

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

...

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

...

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

...

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

...

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

...

On Wednesday 21 of August 2013 09:50:15 Mark Rutland wrote: > On Tue, Aug 20, 2013 at 01:06:25AM +0100, Mike Turquette

wrote:

...

...

...

...

...

> > Quoting Mark Rutland (2013-08-19 02:35:43) > > > > > On Sat, Aug 17, 2013 at 04:17:18PM +0100, Tomasz Figa

wrote:

...

...

...

...

...

> > > > On Saturday 17 of August 2013 16:53:16 Sascha Hauer

wrote:

...

...

...

...

...

> > > > > On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa

wrote:

...

...

...

...

...

> > > > > > > > > Also I would make this option required. Use a > > > > > > > > > dummy > > > > > > > > > clock for > > > > > > > > > mux > > > > > > > > > inputs that are grounded for a specific SoC. > > > > > > > > > > > > > > > > Some clocks are not from CCM and we haven't > > > > > > > > defined in > > > > > > > > imx6q-clk.txt, > > > > > > > > so in most cases we can't provide a phandle for > > > > > > > > them, eg: > > > > > > > > spdif_ext. > > > > > > > > I think it's a bit hard to force it to be > > > > > > > > 'required'. An > > > > > > > > 'optional' > > > > > > > > looks more flexible to me and a default one is > > > > > > > > ensured > > > > > > > > even if > > > > > > > > it's > > > > > > > > missing. > > > > > > > > > > > > > > <&clks 0> is the dummy clock. This can be used for > > > > > > > all input > > > > > > > clocks > > > > > > > not > > > > > > > defined by the SoC. > > > > > > > > > > > > Where does this assumption come from? Is it > > > > > > documented > > > > > > anywhere? > > > > > > > > > > This is how all i.MX clock bindings currently are. See > > > > > Documentation/devicetree/bindings/clock/imx*-clock.txt > > > > > > > > OK, thanks. > > > > > > > > I guess we need some discussion on dummy clocks vs > > > > skipped clocks. > > > > I think we want some consistency on this, don't we? > > > > > > > > If we really need a dummy clock, then we might also want > > > > a generic > > > > way to specify it. > > > > > > What do we actually mean by a "dummy clock"? We already > > > have > > > bindings > > > for "fixed-clock" and co friends describe relatively > > > simple > > > preconfigured clocks. > > > > Some platforms have a fake clock which defines noops > > callbacks and > > basically doesn't do anything. This is analogous to the > > dummy > > regulator > > implementation. A central one could be registered by the > > clock core, > > as > > is done by the regulator core. > > When you say some platforms, you presumably mean the platform > code in > Linux? A dummy clock sounds like a completely Linux-specific > abstraction rather than a description of the hardware, and I > don't see why we need that in the DT: > > * If a clock is wired up and running (as presumably the dummy > clock is), then surely it's a fixed-clock (it's running, we > and we have no control over it, but we presumably know its > rate) and can be described as such? > > * If no clock is wired up, then we should be able to describe > that. If a driver believes that a clock is required when it > isn't (for some level of functionality), then that driver > should be fixed up to support the clock as being optional. > > Am I missing something?

I second that.

Moreover, I don't think that device tree should deal with dummy anything. It should be able to describe hardware that is available on given system, not list what hardware is not available.

I wasn't clear. The dummy clock IS a completely Linux-specific abstraction.

I'm not advocating a dummy clock in DT. I am advocating consolidation of the implementation of a clock that does nothing into the clock core. This code could easily live in drivers/clk/clk.c instead of having everyone open-code it.

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

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

...

I'm guessing one of the reasons for such a clock are drivers do not honor the clk.h api and they freak out when clk_get gives them a NULL pointer?

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

The original reason introducing the dummy clocks in the i.MX dtbs was to provide devices a clock which the driver requests but is not software controllable. We often have the case where the same devices are on several SoCs, but not on all of them all clocks have a bit to en/disable them.

Anyway, to accomplish this we don't need dummy clocks. We can just describe the real clocks.

You could use a dummy clk for the Linux implementation, but the downside is that a dummy clock has a rate of 0 always and a your clocks likely have non-zero rates.

It is probably better for you define a clock which only implements the .recalc_rate callback. If the rate of this clock changes without Linux having knowledge of it you can use the CLK_GET_RATE_NOCACHE flag.

I doubt that rate of a dummy clock could ever change... unless it is a rather smart dummy.

...

...

BTW with the S/PDIF core on which not all mux inputs are connected to actual clocks we could also describe the unconnected inputs as ground clocks with rate 0. This way we describe something which is really there instead of dummy clocks ;)

Again you could use a dummy clock for this OR a fixed-rate clock with a rate of zero from the perspective of the Linux implementation.

Do you think it worthwhile to have a DT binding for a grounded clock? That is not an entirely uncommon case.

Well, how would that differ from skipping a clock from clocks list, i.e. not specifying it in clock-names and clocks properties?

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

...

...

Background to why it might be a good idea to connect a ground clock to the unconnected input pins is that a driver has a chance to successfully grab all clocks. Otherwise how does the driver distinguish between an unconnected and an erroneous clock?

Sorry, I don't follow this last question. Do you mean how to distinguish based on the value returned from clk_get?

Hmm, in theory, a driver could want to distinguish an error case (e.g. clock specified, but there is a problem with it) from no clock (e.g. clock not specified in DT, because it is not available on particular board).

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

for (i = 0; i < STC_TXCLK_SRC_MAX; i++) { sprintf(tmp, "rxtx%d", i); clk = devm_clk_get(&pdev->dev, tmp); if (IS_ERR(clk)) { /* * This can either mean this clock is not wired * up (skipped in the devicetree, *or* it is wired * up, but there went something wrong requesting it. * We can only ignore this error. */ } }

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

Sascha

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

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

...

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

...

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

...

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

...

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

...

On Thu, Aug 22, 2013 at 08:19:10AM +0100, Mike Turquette wrote: > Quoting Tomasz Figa (2013-08-21 14:34:55) > > > On Wednesday 21 of August 2013 09:50:15 Mark Rutland wrote: > > > On Tue, Aug 20, 2013 at 01:06:25AM +0100, Mike Turquette

wrote:

...

...

...

> > > > Quoting Mark Rutland (2013-08-19 02:35:43) > > > > > > > > > On Sat, Aug 17, 2013 at 04:17:18PM +0100, Tomasz Figa

wrote:

...

...

...

> > > > > > On Saturday 17 of August 2013 16:53:16 Sascha Hauer

wrote:

...

...

...

> > > > > > > On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa

wrote:

...

...

...

> > > > > > > > > > > Also I would make this option required. Use a > > > > > > > > > > > dummy > > > > > > > > > > > clock for > > > > > > > > > > > mux > > > > > > > > > > > inputs that are grounded for a specific SoC. > > > > > > > > > > > > > > > > > > > > Some clocks are not from CCM and we haven't > > > > > > > > > > defined in > > > > > > > > > > imx6q-clk.txt, > > > > > > > > > > so in most cases we can't provide a phandle for > > > > > > > > > > them, eg: > > > > > > > > > > spdif_ext. > > > > > > > > > > I think it's a bit hard to force it to be > > > > > > > > > > 'required'. An > > > > > > > > > > 'optional' > > > > > > > > > > looks more flexible to me and a default one is > > > > > > > > > > ensured > > > > > > > > > > even if > > > > > > > > > > it's > > > > > > > > > > missing. > > > > > > > > > > > > > > > > > > <&clks 0> is the dummy clock. This can be used for > > > > > > > > > all input > > > > > > > > > clocks > > > > > > > > > not > > > > > > > > > defined by the SoC. > > > > > > > > > > > > > > > > Where does this assumption come from? Is it > > > > > > > > documented > > > > > > > > anywhere? > > > > > > > > > > > > > > This is how all i.MX clock bindings currently are. See > > > > > > > Documentation/devicetree/bindings/clock/imx*-clock.txt > > > > > > > > > > > > OK, thanks. > > > > > > > > > > > > I guess we need some discussion on dummy clocks vs > > > > > > skipped clocks. > > > > > > I think we want some consistency on this, don't we? > > > > > > > > > > > > If we really need a dummy clock, then we might also want > > > > > > a generic > > > > > > way to specify it. > > > > > > > > > > What do we actually mean by a "dummy clock"? We already > > > > > have > > > > > bindings > > > > > for "fixed-clock" and co friends describe relatively > > > > > simple > > > > > preconfigured clocks. > > > > > > > > Some platforms have a fake clock which defines noops > > > > callbacks and > > > > basically doesn't do anything. This is analogous to the > > > > dummy > > > > regulator > > > > implementation. A central one could be registered by the > > > > clock core, > > > > as > > > > is done by the regulator core. > > > > > > When you say some platforms, you presumably mean the platform > > > code in > > > Linux? A dummy clock sounds like a completely Linux-specific > > > abstraction rather than a description of the hardware, and I > > > don't see why we need that in the DT: > > > > > > * If a clock is wired up and running (as presumably the dummy > > > clock is), then surely it's a fixed-clock (it's running, we > > > and we have no control over it, but we presumably know its > > > rate) and can be described as such? > > > > > > * If no clock is wired up, then we should be able to describe > > > that. If a driver believes that a clock is required when it > > > isn't (for some level of functionality), then that driver > > > should be fixed up to support the clock as being optional. > > > > > > Am I missing something? > > > > I second that. > > > > Moreover, I don't think that device tree should deal with dummy > > anything. It should be able to describe hardware that is > > available on given system, not list what hardware is not > > available. > > I wasn't clear. The dummy clock IS a completely Linux-specific > abstraction. > > I'm not advocating a dummy clock in DT. I am advocating > consolidation of the implementation of a clock that does nothing > into the clock core. This code could easily live in > drivers/clk/clk.c instead of having everyone open-code it. > > As far as specifying a dummy clock in DT? I dunno. DT should > describe > real hardware so there isn't much use for a dummy clock.

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

> I'm guessing one of the reasons for such a clock are drivers do > not > honor the clk.h api and they freak out when clk_get gives them a > NULL > pointer?

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

The original reason introducing the dummy clocks in the i.MX dtbs was to provide devices a clock which the driver requests but is not software controllable. We often have the case where the same devices are on several SoCs, but not on all of them all clocks have a bit to en/disable them.

Anyway, to accomplish this we don't need dummy clocks. We can just describe the real clocks.

You could use a dummy clk for the Linux implementation, but the downside is that a dummy clock has a rate of 0 always and a your clocks likely have non-zero rates.

It is probably better for you define a clock which only implements the .recalc_rate callback. If the rate of this clock changes without Linux having knowledge of it you can use the CLK_GET_RATE_NOCACHE flag.

I doubt that rate of a dummy clock could ever change... unless it is a rather smart dummy.

...

...

BTW with the S/PDIF core on which not all mux inputs are connected to actual clocks we could also describe the unconnected inputs as ground clocks with rate 0. This way we describe something which is really there instead of dummy clocks ;)

Again you could use a dummy clock for this OR a fixed-rate clock with a rate of zero from the perspective of the Linux implementation.

Do you think it worthwhile to have a DT binding for a grounded clock? That is not an entirely uncommon case.

Well, how would that differ from skipping a clock from clocks list, i.e. not specifying it in clock-names and clocks properties?

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

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

...

...

...

...

Background to why it might be a good idea to connect a ground clock to the unconnected input pins is that a driver has a chance to successfully grab all clocks. Otherwise how does the driver distinguish between an unconnected and an erroneous clock?

Sorry, I don't follow this last question. Do you mean how to distinguish based on the value returned from clk_get?

Hmm, in theory, a driver could want to distinguish an error case (e.g. clock specified, but there is a problem with it) from no clock (e.g. clock not specified in DT, because it is not available on particular board).

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

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

[...]

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

...

}

}

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

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

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

Sascha

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

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

...

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

...

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

...

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

...

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

...

On Thu, Aug 22, 2013 at 01:09:31PM +0100, Mark Rutland wrote: > On Thu, Aug 22, 2013 at 08:19:10AM +0100, Mike Turquette wrote: > > Quoting Tomasz Figa (2013-08-21 14:34:55) > > > > > On Wednesday 21 of August 2013 09:50:15 Mark Rutland wrote: > > > > On Tue, Aug 20, 2013 at 01:06:25AM +0100, Mike Turquette

wrote:

...

...

> > > > > Quoting Mark Rutland (2013-08-19 02:35:43) > > > > > > > > > > > On Sat, Aug 17, 2013 at 04:17:18PM +0100, Tomasz Figa

wrote:

...

...

> > > > > > > On Saturday 17 of August 2013 16:53:16 Sascha Hauer

wrote:

...

...

> > > > > > > > On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa

wrote:

...

...

> > > > > > > > > > > > Also I would make this option required. Use a > > > > > > > > > > > > dummy > > > > > > > > > > > > clock for > > > > > > > > > > > > mux > > > > > > > > > > > > inputs that are grounded for a specific SoC. > > > > > > > > > > > > > > > > > > > > > > Some clocks are not from CCM and we haven't > > > > > > > > > > > defined in > > > > > > > > > > > imx6q-clk.txt, > > > > > > > > > > > so in most cases we can't provide a phandle for > > > > > > > > > > > them, eg: > > > > > > > > > > > spdif_ext. > > > > > > > > > > > I think it's a bit hard to force it to be > > > > > > > > > > > 'required'. An > > > > > > > > > > > 'optional' > > > > > > > > > > > looks more flexible to me and a default one is > > > > > > > > > > > ensured > > > > > > > > > > > even if > > > > > > > > > > > it's > > > > > > > > > > > missing. > > > > > > > > > > > > > > > > > > > > <&clks 0> is the dummy clock. This can be used for > > > > > > > > > > all input > > > > > > > > > > clocks > > > > > > > > > > not > > > > > > > > > > defined by the SoC. > > > > > > > > > > > > > > > > > > Where does this assumption come from? Is it > > > > > > > > > documented > > > > > > > > > anywhere? > > > > > > > > > > > > > > > > This is how all i.MX clock bindings currently are. See > > > > > > > > Documentation/devicetree/bindings/clock/imx*-clock.txt > > > > > > > > > > > > > > OK, thanks. > > > > > > > > > > > > > > I guess we need some discussion on dummy clocks vs > > > > > > > skipped clocks. > > > > > > > I think we want some consistency on this, don't we? > > > > > > > > > > > > > > If we really need a dummy clock, then we might also want > > > > > > > a generic > > > > > > > way to specify it. > > > > > > > > > > > > What do we actually mean by a "dummy clock"? We already > > > > > > have > > > > > > bindings > > > > > > for "fixed-clock" and co friends describe relatively > > > > > > simple > > > > > > preconfigured clocks. > > > > > > > > > > Some platforms have a fake clock which defines noops > > > > > callbacks and > > > > > basically doesn't do anything. This is analogous to the > > > > > dummy > > > > > regulator > > > > > implementation. A central one could be registered by the > > > > > clock core, > > > > > as > > > > > is done by the regulator core. > > > > > > > > When you say some platforms, you presumably mean the platform > > > > code in > > > > Linux? A dummy clock sounds like a completely Linux-specific > > > > abstraction rather than a description of the hardware, and I > > > > don't see why we need that in the DT: > > > > > > > > * If a clock is wired up and running (as presumably the dummy > > > > clock is), then surely it's a fixed-clock (it's running, we > > > > and we have no control over it, but we presumably know its > > > > rate) and can be described as such? > > > > > > > > * If no clock is wired up, then we should be able to describe > > > > that. If a driver believes that a clock is required when it > > > > isn't (for some level of functionality), then that driver > > > > should be fixed up to support the clock as being optional. > > > > > > > > Am I missing something? > > > > > > I second that. > > > > > > Moreover, I don't think that device tree should deal with dummy > > > anything. It should be able to describe hardware that is > > > available on given system, not list what hardware is not > > > available. > > > > I wasn't clear. The dummy clock IS a completely Linux-specific > > abstraction. > > > > I'm not advocating a dummy clock in DT. I am advocating > > consolidation of the implementation of a clock that does nothing > > into the clock core. This code could easily live in > > drivers/clk/clk.c instead of having everyone open-code it. > > > > As far as specifying a dummy clock in DT? I dunno. DT should > > describe > > real hardware so there isn't much use for a dummy clock. > > Sorry, I misunderstood. Good to hear we're on the same page :) > > > I'm guessing one of the reasons for such a clock are drivers do > > not > > honor the clk.h api and they freak out when clk_get gives them a > > NULL > > pointer? > > I'm not sure. Sascha, could you shed some light on the matter?

The original reason introducing the dummy clocks in the i.MX dtbs was to provide devices a clock which the driver requests but is not software controllable. We often have the case where the same devices are on several SoCs, but not on all of them all clocks have a bit to en/disable them.

Anyway, to accomplish this we don't need dummy clocks. We can just describe the real clocks.

You could use a dummy clk for the Linux implementation, but the downside is that a dummy clock has a rate of 0 always and a your clocks likely have non-zero rates.

It is probably better for you define a clock which only implements the .recalc_rate callback. If the rate of this clock changes without Linux having knowledge of it you can use the CLK_GET_RATE_NOCACHE flag.

I doubt that rate of a dummy clock could ever change... unless it is a rather smart dummy.

...

...

BTW with the S/PDIF core on which not all mux inputs are connected to actual clocks we could also describe the unconnected inputs as ground clocks with rate 0. This way we describe something which is really there instead of dummy clocks ;)

Again you could use a dummy clock for this OR a fixed-rate clock with a rate of zero from the perspective of the Linux implementation.

Do you think it worthwhile to have a DT binding for a grounded clock? That is not an entirely uncommon case.

Well, how would that differ from skipping a clock from clocks list, i.e. not specifying it in clock-names and clocks properties?

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

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

...

...

...

...

Background to why it might be a good idea to connect a ground clock to the unconnected input pins is that a driver has a chance to successfully grab all clocks. Otherwise how does the driver distinguish between an unconnected and an erroneous clock?

Sorry, I don't follow this last question. Do you mean how to distinguish based on the value returned from clk_get?

Hmm, in theory, a driver could want to distinguish an error case (e.g. clock specified, but there is a problem with it) from no clock (e.g. clock not specified in DT, because it is not available on particular board).

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

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

          [...]

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

...

        }
}

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

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

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

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

Regards, Mike

Sascha

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

On Thu, Aug 22, 2013 at 10:00:35PM +0100, Sascha Hauer wrote:

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

...

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

...

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

...

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

...

On Tue, Aug 20, 2013 at 01:06:25AM +0100, Mike Turquette wrote:

...

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

> On Sat, Aug 17, 2013 at 04:17:18PM +0100, Tomasz Figa wrote: > > On Saturday 17 of August 2013 16:53:16 Sascha Hauer wrote: > > > On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa wrote: > > > > > > > Also I would make this option required. Use a dummy > > > > > > > clock for > > > > > > > mux > > > > > > > inputs that are grounded for a specific SoC. > > > > > > > > > > > > Some clocks are not from CCM and we haven't defined in > > > > > > imx6q-clk.txt, > > > > > > so in most cases we can't provide a phandle for them, eg: > > > > > > spdif_ext. > > > > > > I think it's a bit hard to force it to be 'required'. An > > > > > > 'optional' > > > > > > looks more flexible to me and a default one is ensured > > > > > > even if > > > > > > it's > > > > > > missing. > > > > > > > > > > <&clks 0> is the dummy clock. This can be used for all input > > > > > clocks > > > > > not > > > > > defined by the SoC. > > > > > > > > Where does this assumption come from? Is it documented > > > > anywhere? > > > > > > This is how all i.MX clock bindings currently are. See > > > Documentation/devicetree/bindings/clock/imx*-clock.txt > > > > OK, thanks. > > > > I guess we need some discussion on dummy clocks vs skipped clocks. > > I think we want some consistency on this, don't we? > > > > If we really need a dummy clock, then we might also want a generic > > way to specify it. > > What do we actually mean by a "dummy clock"? We already have > bindings > for "fixed-clock" and co friends describe relatively simple > preconfigured clocks.

Some platforms have a fake clock which defines noops callbacks and basically doesn't do anything. This is analogous to the dummy regulator implementation. A central one could be registered by the clock core, as is done by the regulator core.

When you say some platforms, you presumably mean the platform code in Linux? A dummy clock sounds like a completely Linux-specific abstraction rather than a description of the hardware, and I don't see why we need that in the DT:

• If a clock is wired up and running (as presumably the dummy clock is),

then surely it's a fixed-clock (it's running, we and we have no control over it, but we presumably know its rate) and can be described as such?

• If no clock is wired up, then we should be able to describe that. If a

driver believes that a clock is required when it isn't (for some level of functionality), then that driver should be fixed up to support the clock as being optional.

Am I missing something?

I second that.

Moreover, I don't think that device tree should deal with dummy anything. It should be able to describe hardware that is available on given system, not list what hardware is not available.

I wasn't clear. The dummy clock IS a completely Linux-specific abstraction.

I'm not advocating a dummy clock in DT. I am advocating consolidation of the implementation of a clock that does nothing into the clock core. This code could easily live in drivers/clk/clk.c instead of having everyone open-code it.

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

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

...

I'm guessing one of the reasons for such a clock are drivers do not honor the clk.h api and they freak out when clk_get gives them a NULL pointer?

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

The original reason introducing the dummy clocks in the i.MX dtbs was to provide devices a clock which the driver requests but is not software controllable. We often have the case where the same devices are on several SoCs, but not on all of them all clocks have a bit to en/disable them.

So those clocks are always on at some fixed rate?

Anyway, to accomplish this we don't need dummy clocks. We can just describe the real clocks.

Ok.

BTW with the S/PDIF core on which not all mux inputs are connected to actual clocks we could also describe the unconnected inputs as ground clocks with rate 0. This way we describe something which is really there instead of dummy clocks ;)

Background to why it might be a good idea to connect a ground clock to the unconnected input pins is that a driver has a chance to successfully grab all clocks. Otherwise how does the driver distinguish between an unconnected and an erroneous clock?

Surely the presence of a name in "clock-names" tells you that a clock is connected, and if not listed it's not connected? If we don't have the infrastructure for telling apart unconnected clocks from inability to enable/configure clocks, that's a Linux issue rather than something that should be hacked up in the dt.

Thanks, Mark.

On Sat, Aug 17, 2013 at 02:26:40PM +0200, Tomasz Figa wrote:

...

You mean tx<0-7>.

Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC.

Why do you need a dummy clock?

The driver can simply try to grab all the possible clocks and discard those that failed, so you can just keep those grounded clocks unspecified.

We don't need dummy clocks. My motivation saying this that I was afraid people try to configure the driver by skipping the clocks they don't want from the devicetree.

Sascha

On Sat, Aug 17, 2013 at 05:13:20PM +0200, Tomasz Figa wrote:

On Saturday 17 of August 2013 17:00:02 Sascha Hauer wrote:

...

On Sat, Aug 17, 2013 at 02:26:40PM +0200, Tomasz Figa wrote:

...

...

You mean tx<0-7>.

Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC.

Why do you need a dummy clock?

The driver can simply try to grab all the possible clocks and discard those that failed, so you can just keep those grounded clocks unspecified.

We don't need dummy clocks. My motivation saying this that I was afraid people try to configure the driver by skipping the clocks they don't want from the devicetree.

I'm not really sure if the same abuse couldn't be easily achieved by putting dummy clocks in place of those skipped clocks.

That's right, yes.

Adding a note in binding documentation that says that all clocks that are fed to the IP shall be specified should be fine IMHO.

ok.

Sascha

On Sat, Aug 17, 2013 at 02:56:11PM +0200, Tomasz Figa wrote:

Hi Nicolin,

...

First, you are right that all the properties you just commented are software configurations. And I got the point that device tree now can't allow any software configuration even if the actual hardware connection will depend on it.

If so, I would like to remove those abused clocks and also drop the unused clocks in src<0-7>, then just remain those needed clocks src. I think that can be plausible because there'll be no more clock abuse and the driver will be able to get the source index from the name 'src<num>'.

OK.

...

And you are right about the 9 clock inputs, just there're not only 9 inputs but also an extra external clock from S/PDIF transmitter via coaxial cable or optical fiber -- RxCLK. Please check the following list:

0000 if (DPLL Locked) SPDIF_RxClk else extal 0001 if (DPLL Locked) SPDIF_RxClk else spdif_clk 0010 if (DPLL Locked) SPDIF_RxClk else asrc_clk 0011 if (DPLL Locked) SPDIF_RxClk else spdif_extclk 0100 if (DPLL Locked) SPDIF_Rxclk else esai_hckt 0101 extal_clk 0110 spdif_clk 0111 asrc_clk 1000 spdif_extclk 1001 esai_hckt 1010 if (DPLL Locked) SPDIF_RxClk else mlb_clk 1011 if (DPLL Locked) SPDIF_RxClk else mlb_phy_clk 1100 mkb_clk 1101 mlb_phy_clk

Could you explain what the above values are? If they are values written to a 4-bit mux that selects RX clock source, then all the 16 clocks should be specified from device tree, even if they are duplicated.

The S/PDIF core can recover the clock for the tx signal from the rx signal. So if you have an S/PDIF input signal, then the DPLL will be locked and the SPDIF_RxClk can be used for tx. So the above are really 8 clocks and one "If DPLL locked, use it" bit.

Sascha