[PATCH] ARM: mach-imx6q: Remove code for setting up cko clock

[PATCH] ARM: mach-imx6q: Remove code for setting up cko clock

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

Let the bootloader setup the cko clock that drives the audio codec.

This simplifies a lot the code and when a new board needs to add audio support,
it will not have to add all this amount of code again. 

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 arch/arm/mach-imx/mach-imx6q.c |   25 -------------------------
 1 file changed, 25 deletions(-)

diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index 5536fd8..fe896a8 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -113,36 +113,11 @@ static int ksz9021rn_phy_fixup(struct phy_device *phydev)
 	return 0;
 }
 
-static void __init imx6q_sabrelite_cko1_setup(void)
-{
-	struct clk *cko1_sel, *ahb, *cko1;
-	unsigned long rate;
-
-	cko1_sel = clk_get_sys(NULL, "cko1_sel");
-	ahb = clk_get_sys(NULL, "ahb");
-	cko1 = clk_get_sys(NULL, "cko1");
-	if (IS_ERR(cko1_sel) || IS_ERR(ahb) || IS_ERR(cko1)) {
-		pr_err("cko1 setup failed!\n");
-		goto put_clk;
-	}
-	clk_set_parent(cko1_sel, ahb);
-	rate = clk_round_rate(cko1, 16000000);
-	clk_set_rate(cko1, rate);
-put_clk:
-	if (!IS_ERR(cko1_sel))
-		clk_put(cko1_sel);
-	if (!IS_ERR(ahb))
-		clk_put(ahb);
-	if (!IS_ERR(cko1))
-		clk_put(cko1);
-}
-
 static void __init imx6q_sabrelite_init(void)
 {
 	if (IS_BUILTIN(CONFIG_PHYLIB))
 		phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK,
 				ksz9021rn_phy_fixup);
-	imx6q_sabrelite_cko1_setup();
 }
 
 static void __init imx6q_1588_init(void)
-- 
1.7.9.5

[PATCH] ARM: mach-imx6q: Remove code for setting up cko clock

[Sascha Hauer]

On Wed, Apr 17, 2013 at 03:43:02PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Let the bootloader setup the cko clock that drives the audio codec.
> 

I really don't like such stuff. Where is documented what's expected from
the bootloader? Which versions of which bootloader do implement this?

The code you remove is not particularly nice, but just removing it is no
solution to the problem.


> This simplifies a lot the code and when a new board needs to add audio support,
> it will not have to add all this amount of code again. 

It shouldn't be necessary to duplicate the code for each board.

Sascha

> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  arch/arm/mach-imx/mach-imx6q.c |   25 -------------------------
>  1 file changed, 25 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index 5536fd8..fe896a8 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -113,36 +113,11 @@ static int ksz9021rn_phy_fixup(struct phy_device *phydev)
>  	return 0;
>  }
>  
> -static void __init imx6q_sabrelite_cko1_setup(void)
> -{
> -	struct clk *cko1_sel, *ahb, *cko1;
> -	unsigned long rate;
> -
> -	cko1_sel = clk_get_sys(NULL, "cko1_sel");
> -	ahb = clk_get_sys(NULL, "ahb");
> -	cko1 = clk_get_sys(NULL, "cko1");
> -	if (IS_ERR(cko1_sel) || IS_ERR(ahb) || IS_ERR(cko1)) {
> -		pr_err("cko1 setup failed!\n");
> -		goto put_clk;
> -	}
> -	clk_set_parent(cko1_sel, ahb);
> -	rate = clk_round_rate(cko1, 16000000);
> -	clk_set_rate(cko1, rate);
> -put_clk:
> -	if (!IS_ERR(cko1_sel))
> -		clk_put(cko1_sel);
> -	if (!IS_ERR(ahb))
> -		clk_put(ahb);
> -	if (!IS_ERR(cko1))
> -		clk_put(cko1);
> -}
> -
>  static void __init imx6q_sabrelite_init(void)
>  {
>  	if (IS_BUILTIN(CONFIG_PHYLIB))
>  		phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK,
>  				ksz9021rn_phy_fixup);
> -	imx6q_sabrelite_cko1_setup();
>  }
>  
>  static void __init imx6q_1588_init(void)
> -- 
> 1.7.9.5
> 
> 

-- 
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 |

[PATCH] ARM: mach-imx6q: Remove code for setting up cko clock

[Fabio Estevam]

Hi Sascha,

On Wed, Apr 17, 2013 at 4:57 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Wed, Apr 17, 2013 at 03:43:02PM -0300, Fabio Estevam wrote:
>> From: Fabio Estevam <fabio.estevam@freescale.com>
>>
>> Let the bootloader setup the cko clock that drives the audio codec.
>>
>
> I really don't like such stuff. Where is documented what's expected from
> the bootloader? Which versions of which bootloader do implement this?

As far as I know such documentation does not exist. It would be nice
to have it, especially now that
we are recently moving some ARM errata stuff out of the multi plaform kernel.

I sent patches today for enabling CLKO for sabrelite in Barebox and U-boot.

> The code you remove is not particularly nice, but just removing it is no
> solution to the problem.

Please let me know what is your preferred solution then.

There was some discussion here:
https://patchwork.kernel.org/patch/2303011/

,but I didn't see this come into any conclusion of what is the
preferred way to handle this.

>
>> This simplifies a lot the code and when a new board needs to add audio support,
>> it will not have to add all this amount of code again.
>
> It shouldn't be necessary to duplicate the code for each board.

Yes, some code could be factored out, but every time we need to add
audio support for a new board we would need to keep polluting
arch/arm/mach-imx/mach-imx6q.c with things like:

if (of_machine_is_compatible("fsl,imx6q-xxx"))
    cko_setup()

Regards,

Fabio Estevam

[PATCH] ARM: mach-imx6q: Remove code for setting up cko clock

[Sascha Hauer]

On Wed, Apr 17, 2013 at 05:21:25PM -0300, Fabio Estevam wrote:
> Hi Sascha,
> 
> On Wed, Apr 17, 2013 at 4:57 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Wed, Apr 17, 2013 at 03:43:02PM -0300, Fabio Estevam wrote:
> >> From: Fabio Estevam <fabio.estevam@freescale.com>
> >>
> >> Let the bootloader setup the cko clock that drives the audio codec.
> >>
> >
> > I really don't like such stuff. Where is documented what's expected from
> > the bootloader? Which versions of which bootloader do implement this?
> 
> As far as I know such documentation does not exist. It would be nice
> to have it, especially now that
> we are recently moving some ARM errata stuff out of the multi plaform kernel.

Yes, that's another thing that should be documented. Without even
documenting it we go into an area where things happen to work in certain
combinations and we won't have good answers when somebody asks on the
list why something doesn't work for him (other than: update your
bootloader and see if that helps)

> 
> I sent patches today for enabling CLKO for sabrelite in Barebox and U-boot.
> 
> > The code you remove is not particularly nice, but just removing it is no
> > solution to the problem.
> 
> Please let me know what is your preferred solution then.
> 
> There was some discussion here:
> https://patchwork.kernel.org/patch/2303011/
> 
> ,but I didn't see this come into any conclusion of what is the
> preferred way to handle this.

It may seemed like I bashed upon putting configuration data in the
devicetree. This wasn't really my intention, I more hoped that there
would be some more discussion about this topic.

I still think that it's a bad idea to just mix the configuration data
with the hardware description, because the hardware doesn't change, but
the confguration data is a matter of usecase and sometimes even personal
taste.
I also think that the devicetree format as such is a good format to
describe such (hardware specific) configuration data.

I'm perfectly fine with having configuration data in the devicetree, but
there should be some strict rules:

- configuration data should be in a separate source dts
- configuration data should be in a separate subtree (/chosen)
- configuration data should *not* be in the board dts files in the
  kernel.
- ideally it should be possible to distribute configuration data and
  hardware description separately. hardware description doesn't change
  (as long as we don't change the bindings of course), but configuration
  data often changes with the usecase or kernel version.

Particularly currently the dts files in the Kernel often enforce a
partition layout for mtd devices. Such things shouldn't happen.

> 
> >
> >> This simplifies a lot the code and when a new board needs to add audio support,
> >> it will not have to add all this amount of code again.
> >
> > It shouldn't be necessary to duplicate the code for each board.
> 
> Yes, some code could be factored out, but every time we need to add
> audio support for a new board we would need to keep polluting
> arch/arm/mach-imx/mach-imx6q.c with things like:
> 
> if (of_machine_is_compatible("fsl,imx6q-xxx"))
>     cko_setup()

We already have the necessary bindings to encode that the SGTL5000 clock
parent is the CLKO pin. That's hardware description and nobody would
object to that. That would only leave the problem of finding the correct
parent.

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 |

[PATCH] ARM: mach-imx6q: Remove code for setting up cko clock

[Matt Sealey]

Resending to LAKML in plaintext (Google ARGH..), apologies for anyone
receiving it twice..

--
Matt Sealey <matt@genesi-usa.com>
Product Development Analyst, Genesi USA, Inc.

On Wed, Apr 17, 2013 at 5:29 PM, Matt Sealey <matt@genesi-usa.com> wrote:
> On Wed, Apr 17, 2013 at 2:57 PM, Sascha Hauer <s.hauer@pengutronix.de>
> wrote:
>>
>> On Wed, Apr 17, 2013 at 03:43:02PM -0300, Fabio Estevam wrote:
>> > From: Fabio Estevam <fabio.estevam@freescale.com>
>> >
>> > Let the bootloader setup the cko clock that drives the audio codec.
>> >
>>
>> I really don't like such stuff. Where is documented what's expected from
>> the bootloader?
>
>
> Schematics for the board - and basically a flat-out recommendation that all
> clocks that need to be set to something other than power-on defaults need to
> be set to their expected parents and rates at bootloader time even if
> they're not used by the bootloader, unless they can be confirmed totally
> unused (in which case, they needn't even be in the device tree and Linux
> will just never know they even exist).
>
> This is a fact of life.. I don't see why Linux should end up with a bunch of
> board-specific hacks to work around broken bootloaders when we're talking
> about DT-supported boards. In this case, probably everyone has to go get a
> new U-Boot anyway to even boot a kernel that would show this problem (no
> Freescale board ever shipped before 2012 with a dt-capable U-Boot, so unless
> you're using DTB_APPEND..).
>
>> Which versions of which bootloader do implement this?
>>
>> The code you remove is not particularly nice, but just removing it is no
>> solution to the problem.
>>
>>
>> > This simplifies a lot the code and when a new board needs to add audio
>> > support,
>> > it will not have to add all this amount of code again.
>>
>> It shouldn't be necessary to duplicate the code for each board.
>
>
>
> But it usually is - even if there is a generic "set up clko" routine where
> you pass a parent and a desired rate, a bunch of boards will end up calling
> it, and in the end it's basically a workaround for the fact that the
> bootloader did NOT set this up (when it should!)
>
> For an audio codec, this rate is basically nearly always fixed (and where it
> isn't.. it's up to the codec driver to set_rate on it's specified clock when
> it needs to). Since it isn't possible to dictate clock frequency in the
> device tree right now, and I at least would prefer that we didn't store "yet
> to be configured, please assure this" kind of directives in device tree, the
> bootloader is the best place.
>
> There's similar code in MX5 platforms right now which reparents the USB PHY
> clock to one that can actually can generate desired rates for USB PHYs
> (which is coordinated with some other code which sets the USB PHY rate
> inside the PHY). In any case here, all MX5 chips boot with an "impossible"
> USB PHY configuration (there is no clock parent that can give it that rate,
> it must be changed), and it is not possible to use the default parent to
> generate two of the other rates either. This is a bootloader thing for sure.
> Linux should not be working around this. There's also code that sets parents
> for eSDHC controllers; this is global code run on every board, and only
> works because every board designer - so far - didn't bother to do something
> different (as it stands, the eSDHC gets a reaaaally weird rate because of
> it, and it is surprising nobody noticed..)
>
> Even if we had such directives in DT, 99.9% of it would be reproduction of
> clock setting code in bootloader being dumped into the device tree and
> re-set by Linux yet again. That can cause glitches which can cause devices
> like codecs to basically go into indeterminate states (and some of these
> devices simply don't have resets, so they stay in those states unless
> powered off and on again, and sometimes the regulators aren't configurable -
> all this is in the schematics for the board if they're available).
>
> When your Linux kernel warns (and it should warn in each driver that detects
> a weirdo clock scenario that can detect it) that you may need a new
> bootloader, then updating the bootloader is necessary. For boards where the
> developers don't like you to update the bootloader, well.. ours are a couple
> of those, and to be honest, I will happily ship firmware updates when
> there's some platform support for our board, which I've still not gotten
> round to doing yet (busy...)
>
> The usual scenario here is that there was a lazy port of BSP code to
> mainline U-Boot and you're running that instead of the BSP version
> (*ahemlinarocough*), someone coded bootloader support but assumed Linux will
> just set it up (*ahemfreescalecough*) or someone hacked up support for a
> board which has no public schematics (GK802/Richtechie MT-500A comes to
> mind) and no open-sourced board support (which is illegal by any means in
> the aforementioned case) - basically fixable (Linaro usually recommends
> using a Linaro U-Boot for example), fixable (someone may need to align board
> support mainline U-Boot and mainline Linux at some point), and no reason to
> ever bother with criminals - respectively.
>
> What we're discussing here is basically "it used to work and now Fabio broke
> Linux" - actually, Fabio is removing an egregious hack that should never
> have been there in the first place, Linux works fine ("as always"), and the
> bootloader is broken.. it would never have passed QA for consumer
> electronics. Since these are developer boards, we can be forgiving up to a
> point...
>
> --
> Matt Sealey <matt@genesi-usa.com>
> Product Development Analyst, Genesi USA, Inc.
>