[RFC 00/14] Generic clk for Orion platforms.

[RFC 00/14] Generic clk for Orion platforms.

[Andrew Lunn]

This patch set starts the process of changing Orion based platforms to
use the generic clk framework. The first patch fixes compile problems
with the framework, and is expected to be dropped once fixed upstream.
Then clks are added. This is a fixed rate clk for tclk and for
kirkwood, most of the gated clocks also get a clk. The following
patches then modified the drivers to make use of these clocks,
getting, prepareing, enabling at probe time, and disableing,
unprepareing, and putting at remove time. Rather than pass the clock
frequency as platform_data, the driver now uses clk_get_rate(). This
results in some platform_data structures becoming redundant, and so
are removed.

TODO:

 0) Cleanup the white space changes, and hunks in the wrong patches.
 1) PCIe needs adopting to use clks.
 2) Strip out all references to kirkwood_clk_ctrl
 3) Find a solution for turning off unused clks - Probably needs
    framework support, which Mike has already suggested.
 4) Find a solution for tuning off unused SATA and PCIe PHYs. Maybe
    use the unprepare() method, but the current basic clk providers 
    don't support this.

If we are willing to accept a regression in power usage, we could skip
3) and 4) for the moment in order to get this code into use. That
would help the device tree work which benefits from not having to deal
with clock frequencies when setting up device driver bindings.

Boot tested on kirkwood. No testing what so ever on other Orion
platforms.

Based on 

git://git.linaro.org/people/mturquette/linux.git v3.3-rc5-clkv5


Andrew Lunn (14):
  [clk] Fix compile errors in DEFINE_CLK_GATE()
  ARM: Orion: Add clocks using the generic clk infrastructure.
  Arm: Orion: spi: Add clk/clkdev support.
  orion: spi: remove enable_clock_fix which is not used
  Arm: Orion: eth: Add clk/clkdev support.
  ARM: Orion: wdt: Add clk/clkdev support
  ARM: Orion: uart: Get the clock rate via clk_get_rate().
  ARM: Kirkwood: Remove tclk from kirkwood_asoc_platform_data.
  MV SATA: Add per channel clk/clkdev support.
  [Orion ehci] Add support for enabling clocks
  [Orion nand] Add support for clk, if there is one.
  [Orion sdio] Add support for clk.
  [Orion crypto] Add support for clk
  [Orion xor] Add support for clk

 arch/arm/Kconfig                                  |    3 +
 arch/arm/mach-dove/common.c                       |   37 ++++---
 arch/arm/mach-dove/dove-db-setup.c                |    1 -
 arch/arm/mach-kirkwood/common.c                   |  119 +++++++++++++++++----
 arch/arm/mach-kirkwood/include/mach/bridge-regs.h |   16 +++
 arch/arm/mach-kirkwood/mv88f6281gtw_ge-setup.c    |    1 -
 arch/arm/mach-kirkwood/rd88f6192-nas-setup.c      |    1 -
 arch/arm/mach-kirkwood/t5325-setup.c              |    1 -
 arch/arm/mach-kirkwood/tsx1x-common.c             |    1 -
 arch/arm/mach-mv78xx0/common.c                    |   44 +++++---
 arch/arm/mach-orion5x/common.c                    |   26 ++++-
 arch/arm/mach-orion5x/rd88f6183ap-ge-setup.c      |    1 -
 arch/arm/plat-orion/common.c                      |   88 ++++++++-------
 arch/arm/plat-orion/include/plat/audio.h          |    1 -
 arch/arm/plat-orion/include/plat/common.h         |   26 +++--
 arch/arm/plat-orion/include/plat/orion_wdt.h      |   18 ---
 drivers/ata/sata_mv.c                             |   43 +++++++-
 drivers/clk/clk.c                                 |    1 +
 drivers/crypto/mv_cesa.c                          |   15 +++
 drivers/dma/mv_xor.c                              |   16 +++
 drivers/dma/mv_xor.h                              |    1 +
 drivers/mmc/host/mvsdio.c                         |   14 +++
 drivers/mtd/nand/orion_nand.c                     |   18 +++
 drivers/net/ethernet/marvell/mv643xx_eth.c        |   46 ++++++--
 drivers/spi/spi-orion.c                           |   37 +++++--
 drivers/usb/host/ehci-orion.c                     |   16 +++
 drivers/watchdog/orion_wdt.c                      |   18 ++-
 include/linux/clk-private.h                       |    4 +-
 include/linux/mv643xx_eth.h                       |    1 -
 include/linux/spi/orion_spi.h                     |   18 ---
 30 files changed, 443 insertions(+), 189 deletions(-)
 delete mode 100644 arch/arm/plat-orion/include/plat/orion_wdt.h
 delete mode 100644 include/linux/spi/orion_spi.h

-- 
1.7.9.1

[RFC 00/14] Generic clk for Orion platforms.

[Jason]

On Tue, Mar 06, 2012 at 07:30:56AM +0100, Andrew Lunn wrote:
> This patch set starts the process of changing Orion based platforms to
> use the generic clk framework. The first patch fixes compile problems
> with the framework, and is expected to be dropped once fixed upstream.
> Then clks are added. This is a fixed rate clk for tclk and for
> kirkwood, most of the gated clocks also get a clk. The following
> patches then modified the drivers to make use of these clocks,
> getting, prepareing, enabling at probe time, and disableing,
> unprepareing, and putting at remove time. Rather than pass the clock
> frequency as platform_data, the driver now uses clk_get_rate(). This
> results in some platform_data structures becoming redundant, and so
> are removed.

Awesome, thanks for posting!

> TODO:
> 
>  0) Cleanup the white space changes, and hunks in the wrong patches.

It would be helpful to break out code cleanup as well.  eg spi_clock_fix
removal, sound platform data removal, and anything else I didn't see on
initial read through.

>  1) PCIe needs adopting to use clks.
>  2) Strip out all references to kirkwood_clk_ctrl
>  3) Find a solution for turning off unused clks - Probably needs
>     framework support, which Mike has already suggested.

I'm assuming the driver(s) will increment a reference, so when it
reaches zero, the framework would call a clk_gate hook.

>  4) Find a solution for tuning off unused SATA and PCIe PHYs. Maybe
>     use the unprepare() method, but the current basic clk providers 
>     don't support this.

It looks like each SATA and PCIe PHY has it's own clock and it's own
shutdown (kirkwood_clock_gate()).  If the return value of unprepare
indicated a clock was gated (all references gone), then the driver could
shut down the PHY.  Or, am I missing something?

> If we are willing to accept a regression in power usage, we could skip
> 3) and 4) for the moment in order to get this code into use. That
> would help the device tree work which benefits from not having to deal
> with clock frequencies when setting up device driver bindings.
> 
> Boot tested on kirkwood. No testing what so ever on other Orion
> platforms.
> 
> Based on 
> 
> git://git.linaro.org/people/mturquette/linux.git v3.3-rc5-clkv5
> 
> 
> Andrew Lunn (14):
>   [clk] Fix compile errors in DEFINE_CLK_GATE()
>   ARM: Orion: Add clocks using the generic clk infrastructure.
>   Arm: Orion: spi: Add clk/clkdev support.
>   orion: spi: remove enable_clock_fix which is not used
>   Arm: Orion: eth: Add clk/clkdev support.
>   ARM: Orion: wdt: Add clk/clkdev support
>   ARM: Orion: uart: Get the clock rate via clk_get_rate().
>   ARM: Kirkwood: Remove tclk from kirkwood_asoc_platform_data.
>   MV SATA: Add per channel clk/clkdev support.
>   [Orion ehci] Add support for enabling clocks
>   [Orion nand] Add support for clk, if there is one.
>   [Orion sdio] Add support for clk.
>   [Orion crypto] Add support for clk
>   [Orion xor] Add support for clk
> 
>  arch/arm/Kconfig                                  |    3 +
>  arch/arm/mach-dove/common.c                       |   37 ++++---
>  arch/arm/mach-dove/dove-db-setup.c                |    1 -
>  arch/arm/mach-kirkwood/common.c                   |  119 +++++++++++++++++----
>  arch/arm/mach-kirkwood/include/mach/bridge-regs.h |   16 +++
>  arch/arm/mach-kirkwood/mv88f6281gtw_ge-setup.c    |    1 -
>  arch/arm/mach-kirkwood/rd88f6192-nas-setup.c      |    1 -
>  arch/arm/mach-kirkwood/t5325-setup.c              |    1 -
>  arch/arm/mach-kirkwood/tsx1x-common.c             |    1 -
>  arch/arm/mach-mv78xx0/common.c                    |   44 +++++---
>  arch/arm/mach-orion5x/common.c                    |   26 ++++-
>  arch/arm/mach-orion5x/rd88f6183ap-ge-setup.c      |    1 -
>  arch/arm/plat-orion/common.c                      |   88 ++++++++-------
>  arch/arm/plat-orion/include/plat/audio.h          |    1 -
>  arch/arm/plat-orion/include/plat/common.h         |   26 +++--
>  arch/arm/plat-orion/include/plat/orion_wdt.h      |   18 ---
>  drivers/ata/sata_mv.c                             |   43 +++++++-
>  drivers/clk/clk.c                                 |    1 +
>  drivers/crypto/mv_cesa.c                          |   15 +++
>  drivers/dma/mv_xor.c                              |   16 +++
>  drivers/dma/mv_xor.h                              |    1 +
>  drivers/mmc/host/mvsdio.c                         |   14 +++
>  drivers/mtd/nand/orion_nand.c                     |   18 +++
>  drivers/net/ethernet/marvell/mv643xx_eth.c        |   46 ++++++--
>  drivers/spi/spi-orion.c                           |   37 +++++--
>  drivers/usb/host/ehci-orion.c                     |   16 +++
>  drivers/watchdog/orion_wdt.c                      |   18 ++-
>  include/linux/clk-private.h                       |    4 +-
>  include/linux/mv643xx_eth.h                       |    1 -
>  include/linux/spi/orion_spi.h                     |   18 ---
>  30 files changed, 443 insertions(+), 189 deletions(-)
>  delete mode 100644 arch/arm/plat-orion/include/plat/orion_wdt.h
>  delete mode 100644 include/linux/spi/orion_spi.h
> 
> -- 
> 1.7.9.1
> 

[RFC 00/14] Generic clk for Orion platforms.

[Andrew Lunn]

On Tue, Mar 06, 2012 at 10:58:40AM -0500, Jason wrote:
> On Tue, Mar 06, 2012 at 07:30:56AM +0100, Andrew Lunn wrote:
> > This patch set starts the process of changing Orion based platforms to
> > use the generic clk framework. The first patch fixes compile problems
> > with the framework, and is expected to be dropped once fixed upstream.
> > Then clks are added. This is a fixed rate clk for tclk and for
> > kirkwood, most of the gated clocks also get a clk. The following
> > patches then modified the drivers to make use of these clocks,
> > getting, prepareing, enabling at probe time, and disableing,
> > unprepareing, and putting at remove time. Rather than pass the clock
> > frequency as platform_data, the driver now uses clk_get_rate(). This
> > results in some platform_data structures becoming redundant, and so
> > are removed.
> 
> Awesome, thanks for posting!
> 
> > TODO:
> > 
> >  0) Cleanup the white space changes, and hunks in the wrong patches.
> 
> It would be helpful to break out code cleanup as well.  eg spi_clock_fix
> removal, sound platform data removal, and anything else I didn't see on
> initial read through.

I could move these out, but they actually help you. It means there is
less for you to add DT support for. If they are left in, you need to
add a DT property for spi_clock_fix, etc.
 
> >  1) PCIe needs adopting to use clks.
> >  2) Strip out all references to kirkwood_clk_ctrl
> >  3) Find a solution for turning off unused clks - Probably needs
> >     framework support, which Mike has already suggested.
> 
> I'm assuming the driver(s) will increment a reference, so when it
> reaches zero, the framework would call a clk_gate hook.

You missed an important point. No driver has claimed these, but u-boot
has turned them on. So we want linux to turn off all clocks for which
there is no driver using them, in order to save power.
kirkwood_clk_ctrl is dual purpose. It turns on needed clocks, but it
also turns off unneeded clocks.
 
> >  4) Find a solution for tuning off unused SATA and PCIe PHYs. Maybe
> >     use the unprepare() method, but the current basic clk providers 
> >     don't support this.
> 
> It looks like each SATA and PCIe PHY has it's own clock and it's own
> shutdown (kirkwood_clock_gate()).  If the return value of unprepare
> indicated a clock was gated (all references gone), then the driver could
> shut down the PHY.  Or, am I missing something?

Yes, same comment as above. We want to turn off PHYs which don't have
a driver.

  Andrew

[RFC 00/14] Generic clk for Orion platforms.

[Jason]

On Tue, Mar 06, 2012 at 05:17:43PM +0100, Andrew Lunn wrote:
> On Tue, Mar 06, 2012 at 10:58:40AM -0500, Jason wrote:
> > On Tue, Mar 06, 2012 at 07:30:56AM +0100, Andrew Lunn wrote:
> > > This patch set starts the process of changing Orion based platforms to
> > > use the generic clk framework. The first patch fixes compile problems
> > > with the framework, and is expected to be dropped once fixed upstream.
> > > Then clks are added. This is a fixed rate clk for tclk and for
> > > kirkwood, most of the gated clocks also get a clk. The following
> > > patches then modified the drivers to make use of these clocks,
> > > getting, prepareing, enabling at probe time, and disableing,
> > > unprepareing, and putting at remove time. Rather than pass the clock
> > > frequency as platform_data, the driver now uses clk_get_rate(). This
> > > results in some platform_data structures becoming redundant, and so
> > > are removed.
> > 
> > Awesome, thanks for posting!
> > 
> > > TODO:
> > > 
> > >  0) Cleanup the white space changes, and hunks in the wrong patches.
> > 
> > It would be helpful to break out code cleanup as well.  eg spi_clock_fix
> > removal, sound platform data removal, and anything else I didn't see on
> > initial read through.
> 
> I could move these out, but they actually help you. It means there is
> less for you to add DT support for. If they are left in, you need to
> add a DT property for spi_clock_fix, etc.

Oops, #4 and #8 are what I wanted to pull in separately.  I could've
sworn I saw something else that went in the same catagory as those
two...

> > >  1) PCIe needs adopting to use clks.
> > >  2) Strip out all references to kirkwood_clk_ctrl
> > >  3) Find a solution for turning off unused clks - Probably needs
> > >     framework support, which Mike has already suggested.
> > 
> > I'm assuming the driver(s) will increment a reference, so when it
> > reaches zero, the framework would call a clk_gate hook.
> 
> You missed an important point. No driver has claimed these, but u-boot
> has turned them on.

This sounds like a bug in u-boot.

> So we want linux to turn off all clocks for which
> there is no driver using them, in order to save power.
> kirkwood_clk_ctrl is dual purpose. It turns on needed clocks, but it
> also turns off unneeded clocks.

This is creating a lot of complexity where the real answer seems to be
to get u-boot to shutoff all peripheral clocks before handing over
control to linux.

Until that happens, could we walk across kirkwood_clks[] in
kirkwood_clock_gate() to determine what is in use?  eg:

#########
curr = readl(CLOCK_GATING_CTRL);
...
used = CGC_DUNIT | CGC_RESERVED;

for (i = 0; i < ARRAY_SIZE(kirkwood_clks); i++) {
	if (kirkwood_clks[i]->enable_count)
		used |= kirkwood_clks[i]->/*PFM*/->mask;
}

/* disable anything in curr, but not in used */
if (CGC_SATA0 & (curr & ~used)) {
	/* disable sata0 logic */
}

...
#########

This would handle both fdt and non-fdt use cases.

thx,

Jason.

[RFC 00/14] Generic clk for Orion platforms.

[Andrew Lunn]

> > You missed an important point. No driver has claimed these, but u-boot
> > has turned them on.
> 
> This sounds like a bug in u-boot.

Is it? Even if it is, we have to live with it. Otherwise we introduce
a regression, which is a big no-no. Also, how many users know how to
upgrade u-boot? I don't have the necessary JTAG setup for my boxes, so
i'm not going to risk bricking them by trying to upgrade u-boot.

> Until that happens, could we walk across kirkwood_clks[] in
> kirkwood_clock_gate() to determine what is in use?  eg:
> 
> #########
> curr = readl(CLOCK_GATING_CTRL);
> ...
> used = CGC_DUNIT | CGC_RESERVED;
> 
> for (i = 0; i < ARRAY_SIZE(kirkwood_clks); i++) {
> 	if (kirkwood_clks[i]->enable_count)
> 		used |= kirkwood_clks[i]->/*PFM*/->mask;
> }
> 
> /* disable anything in curr, but not in used */
> if (CGC_SATA0 & (curr & ~used)) {
> 	/* disable sata0 logic */
> }
> 
> ...
> #########

NO! clk is opaque. You should not be accessing members inside it. The
clk struct members are not even declared when using the normal include
files.

Probably the short term solution is to simply add a deliberate
regression, and not turn them off. Then help Mike implement generic
code to handle this. We probably have three months to do this, since i
doubt the clock code is going to be part of the next merge.

      Andrew

[RFC 00/14] Generic clk for Orion platforms.

[Jason]

On Tue, Mar 06, 2012 at 06:53:33PM +0100, Andrew Lunn wrote:
> > > You missed an important point. No driver has claimed these, but u-boot
> > > has turned them on.
> > 
> > This sounds like a bug in u-boot.
> 
> Is it? Even if it is, we have to live with it. Otherwise we introduce
> a regression, which is a big no-no. Also, how many users know how to
> upgrade u-boot? I don't have the necessary JTAG setup for my boxes, so
> i'm not going to risk bricking them by trying to upgrade u-boot.

Wrong is wrong.  if u-boot is enabling things that aren't needed
immediately after handoff, that needs to be fixed.  If the kernel is
depending on that error and not checking the state of the clocks before
performing actions, that also is broken.

Upgrading u-boot is necessary to support devicetree on kirkwood.  At
least, in any real fashion.  You could append the blob, but that is
Discouraged. ;-)

What kirkwood boards do you have?

> > Until that happens, could we walk across kirkwood_clks[] in
> > kirkwood_clock_gate() to determine what is in use?  eg:
> > 
> > #########
> > curr = readl(CLOCK_GATING_CTRL);
> > ...
> > used = CGC_DUNIT | CGC_RESERVED;
> > 
> > for (i = 0; i < ARRAY_SIZE(kirkwood_clks); i++) {
> > 	if (kirkwood_clks[i]->enable_count)
> > 		used |= kirkwood_clks[i]->/*PFM*/->mask;
> > }
> > 
> > /* disable anything in curr, but not in used */
> > if (CGC_SATA0 & (curr & ~used)) {
> > 	/* disable sata0 logic */
> > }
> > 
> > ...
> > #########
> 
> NO! clk is opaque. You should not be accessing members inside it. The
> clk struct members are not even declared when using the normal include
> files.

Ok, then how about:

	if (clk_is_enabled(kirkwood_clks[i])
		used |= 1 << clk_gate_bit_idx(kirkwood_clks[i]);

> Probably the short term solution is to simply add a deliberate
> regression, and not turn them off. Then help Mike implement generic
> code to handle this. We probably have three months to do this, since i
> doubt the clock code is going to be part of the next merge.

Good point.  I'd like to get it stabilized sooner rather than later,
though.

thx,

Jason.

[RFC 09/14] MV SATA: Add per channel clk/clkdev support.

[Jason]

On Tue, Mar 06, 2012 at 07:31:05AM +0100, Andrew Lunn wrote:
> The Orion kirkwood chips have a gatable clock per SATA channel. Add
> code to get and enable this clk if it exists.
> 
> Signed-of-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  arch/arm/mach-kirkwood/common.c           |    3 ++
>  arch/arm/plat-orion/common.c              |    4 +-
>  arch/arm/plat-orion/include/plat/common.h |    3 ++
>  drivers/ata/sata_mv.c                     |   43 +++++++++++++++++++++++++---
>  4 files changed, 46 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
> index 7588812..855540a 100644
> --- a/arch/arm/mach-kirkwood/common.c
> +++ b/arch/arm/mach-kirkwood/common.c
> @@ -244,6 +244,9 @@ void __init kirkwood_sata_init(struct mv_sata_platform_data *sata_data)
>  	if (sata_data->n_ports > 1)
>  		kirkwood_clk_ctrl |= CGC_SATA1;
>  
> +	orion_clkdev_add("0", "sata_mv.0", &clk_sata0);
> +	orion_clkdev_add("1", "sata_mv.0", &clk_sata1);
> +

This is a problem for devicetree.  With devicetree, neither
kirkwood_sata_init(), nor orion_sata_init() are ever called.  The same
is true for all the other drivers as well.

Could this be moved into the driver's _probe() function?

thx,

Jason.

>  	orion_sata_init(sata_data, SATA_PHYS_BASE, IRQ_KIRKWOOD_SATA);
>  }
>  
> diff --git a/arch/arm/plat-orion/common.c b/arch/arm/plat-orion/common.c
> index 05f078a..06f1e7a 100644
> --- a/arch/arm/plat-orion/common.c
> +++ b/arch/arm/plat-orion/common.c
> @@ -23,8 +23,8 @@
>  #include <plat/ehci-orion.h>
>  
>  /* Create a clkdev entry for a given device/clk */
> -static void orion_clkdev_add(const char *con_id, const char *dev_id,
> -			     struct clk *clk)
> +void orion_clkdev_add(const char *con_id, const char *dev_id,
> +		      struct clk *clk)
>  {
>  	struct clk_lookup *cl;
>  
> diff --git a/arch/arm/plat-orion/include/plat/common.h b/arch/arm/plat-orion/include/plat/common.h
> index a70976e..b739343 100644
> --- a/arch/arm/plat-orion/include/plat/common.h
> +++ b/arch/arm/plat-orion/include/plat/common.h
> @@ -107,4 +107,7 @@ void __init orion_crypto_init(unsigned long mapbase,
>  			      unsigned long srambase,
>  			      unsigned long sram_size,
>  			      unsigned long irq);
> +
> +void orion_clkdev_add(const char *con_id, const char *dev_id,
> +		      struct clk *clk);
>  #endif
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 38950ea..0f4a7e9 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -553,6 +553,7 @@ struct mv_host_priv {
>  
>  #if defined(CONFIG_HAVE_CLK)
>  	struct clk		*clk;
> +	struct clk              **port_clks;
>  #endif
>  	/*
>  	 * These consistent DMA memory pools give us guaranteed
> @@ -4026,6 +4027,9 @@ static int mv_platform_probe(struct platform_device *pdev)
>  	struct mv_host_priv *hpriv;
>  	struct resource *res;
>  	int n_ports, rc;
> +#if defined(CONFIG_HAVE_CLK)
> +	int port;
> +#endif
>  
>  	ata_print_version_once(&pdev->dev, DRV_VERSION);
>  
> @@ -4053,6 +4057,13 @@ static int mv_platform_probe(struct platform_device *pdev)
>  
>  	if (!host || !hpriv)
>  		return -ENOMEM;
> +#if defined(CONFIG_HAVE_CLK)
> +	hpriv->port_clks = devm_kzalloc(&pdev->dev,
> +					sizeof(struct clk *) * n_ports,
> +					GFP_KERNEL);
> +	if (!hpriv->port_clks)
> +		return -ENOMEM;
> +#endif
>  	host->private_data = hpriv;
>  	hpriv->n_ports = n_ports;
>  	hpriv->board_idx = chip_soc;
> @@ -4065,9 +4076,18 @@ static int mv_platform_probe(struct platform_device *pdev)
>  #if defined(CONFIG_HAVE_CLK)
>  	hpriv->clk = clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(hpriv->clk))
> -		dev_notice(&pdev->dev, "cannot get clkdev\n");
> -	else
> -		clk_enable(hpriv->clk);
> +		dev_notice(&pdev->dev, "cannot get optional clkdev\n");
> +	else {
> +		clk_prepare_enable(hpriv->clk);
> +	}
> +	for (port = 0; port < n_ports; port++) {
> +		char port_number[16];
> +		sprintf(port_number, "%d", port);
> +		hpriv->port_clks[port] = clk_get(&pdev->dev, port_number);
> +		if (!IS_ERR(hpriv->port_clks[port])) {
> +			clk_prepare_enable(hpriv->port_clks[port]);
> +		}
> +	}
>  #endif
>  
>  	/*
> @@ -4097,9 +4117,15 @@ static int mv_platform_probe(struct platform_device *pdev)
>  err:
>  #if defined(CONFIG_HAVE_CLK)
>  	if (!IS_ERR(hpriv->clk)) {
> -		clk_disable(hpriv->clk);
> +		clk_disable_unprepare(hpriv->clk);
>  		clk_put(hpriv->clk);
>  	}
> +	for (port = 0; port < n_ports; port++) {
> +		if (!IS_ERR(hpriv->port_clks[port])) {
> +			clk_disable_unprepare(hpriv->port_clks[port]);
> +			clk_put(hpriv->port_clks[port]);
> +		}
> +	}
>  #endif
>  
>  	return rc;
> @@ -4118,14 +4144,21 @@ static int __devexit mv_platform_remove(struct platform_device *pdev)
>  	struct ata_host *host = platform_get_drvdata(pdev);
>  #if defined(CONFIG_HAVE_CLK)
>  	struct mv_host_priv *hpriv = host->private_data;
> +	int port;
>  #endif
>  	ata_host_detach(host);
>  
>  #if defined(CONFIG_HAVE_CLK)
>  	if (!IS_ERR(hpriv->clk)) {
> -		clk_disable(hpriv->clk);
> +		clk_disable_unprepare(hpriv->clk);
>  		clk_put(hpriv->clk);
>  	}
> +	for (port = 0; port < host->n_ports; port++) {
> +		if (!IS_ERR(hpriv->port_clks[port])) {
> +			clk_disable_unprepare(hpriv->port_clks[port]);
> +			clk_put(hpriv->port_clks[port]);
> +		}
> +	}
>  #endif
>  	return 0;
>  }
> -- 
> 1.7.9.1
> 

[RFC 00/14] Generic clk for Orion platforms.

[Nicolas Pitre]

On Tue, 6 Mar 2012, Jason wrote:
> On Tue, Mar 06, 2012 at 05:17:43PM +0100, Andrew Lunn wrote:
> > On Tue, Mar 06, 2012 at 10:58:40AM -0500, Jason wrote:
> > > I'm assuming the driver(s) will increment a reference, so when it
> > > reaches zero, the framework would call a clk_gate hook.
> > 
> > You missed an important point. No driver has claimed these, but u-boot
> > has turned them on.
> 
> This sounds like a bug in u-boot.
> 
> > So we want linux to turn off all clocks for which
> > there is no driver using them, in order to save power.
> > kirkwood_clk_ctrl is dual purpose. It turns on needed clocks, but it
> > also turns off unneeded clocks.
> 
> This is creating a lot of complexity where the real answer seems to be
> to get u-boot to shutoff all peripheral clocks before handing over
> control to linux.

No.

Yes, u-Boot might be wrong.  But the kernel should always distrust as 
much as possible from the bootloader.  Instead of presuming some initial 
state from the bootloader, it is best to simply set that state up front.

The fewer ties you have with any bootloaders, the fewer bugs you'll have 
in the end.  Otherwise you might enter into a game of requiring a 
specific version of a specific bootloader to go with a given kernel, and 
that is far more complex to deal with than the complexity you are 
referring to above.


Nicolas

[RFC 00/14] Generic clk for Orion platforms.

[Turquette, Mike]

On Tue, Mar 6, 2012 at 11:45 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> On Tue, 6 Mar 2012, Jason wrote:
>> On Tue, Mar 06, 2012 at 05:17:43PM +0100, Andrew Lunn wrote:
>> > On Tue, Mar 06, 2012 at 10:58:40AM -0500, Jason wrote:
>> > > I'm assuming the driver(s) will increment a reference, so when it
>> > > reaches zero, the framework would call a clk_gate hook.
>> >
>> > You missed an important point. No driver has claimed these, but u-boot
>> > has turned them on.
>>
>> This sounds like a bug in u-boot.
>>
>> > So we want linux to turn off all clocks for which
>> > there is no driver using them, in order to save power.
>> > kirkwood_clk_ctrl is dual purpose. It turns on needed clocks, but it
>> > also turns off unneeded clocks.
>>
>> This is creating a lot of complexity where the real answer seems to be
>> to get u-boot to shutoff all peripheral clocks before handing over
>> control to linux.
>
> No.
>
> Yes, u-Boot might be wrong. ?But the kernel should always distrust as
> much as possible from the bootloader. ?Instead of presuming some initial
> state from the bootloader, it is best to simply set that state up front.
>
> The fewer ties you have with any bootloaders, the fewer bugs you'll have
> in the end. ?Otherwise you might enter into a game of requiring a
> specific version of a specific bootloader to go with a given kernel, and
> that is far more complex to deal with than the complexity you are
> referring to above.

+1 to the above.

Let's not forget that the type of devices you would buy in a store
typically don't use u-boot, so having a u-boot-centric point of view
is simply too narrowly focused.  The greater the burden we add to
u-boot then the greater the burden we add to every bootloader out
there in the world.  The kernel should manage system resources, simple
as that.

The current clk series does have a flag that can be set per clk,
CLK_IGNORE_UNUSED, which allows platform clock data to NOT
automatically disable unused clocks.  Otherwise in a future revision
of the clock code there will be a late_initcall which walks the tree
disabling clocks that are enabled in hardware, but have prepare_count
== 0.  That didn't go in this time since I'm trying to get the current
stuff merged and adding features tends to slow that process down...

Regards,
Mike

>
>
> Nicolas
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC 00/14] Generic clk for Orion platforms.

[Jason]

On Tue, Mar 06, 2012 at 02:45:23PM -0500, Nicolas Pitre wrote:
> On Tue, 6 Mar 2012, Jason wrote:
> > On Tue, Mar 06, 2012 at 05:17:43PM +0100, Andrew Lunn wrote:
> > > On Tue, Mar 06, 2012 at 10:58:40AM -0500, Jason wrote:
> > > > I'm assuming the driver(s) will increment a reference, so when it
> > > > reaches zero, the framework would call a clk_gate hook.
> > > 
> > > You missed an important point. No driver has claimed these, but u-boot
> > > has turned them on.
> > 
> > This sounds like a bug in u-boot.
> > 
> > > So we want linux to turn off all clocks for which
> > > there is no driver using them, in order to save power.
> > > kirkwood_clk_ctrl is dual purpose. It turns on needed clocks, but it
> > > also turns off unneeded clocks.
> > 
> > This is creating a lot of complexity where the real answer seems to be
> > to get u-boot to shutoff all peripheral clocks before handing over
> > control to linux.
> 
> No.
> 
> Yes, u-Boot might be wrong.  But the kernel should always distrust as 
> much as possible from the bootloader.  Instead of presuming some initial 
> state from the bootloader, it is best to simply set that state up front.

Yes, I clarified this in my response to Andrew:

Wrong is wrong.  if u-boot is enabling things that aren't needed
immediately after handoff, that needs to be fixed.  If the kernel is
depending on that error and not checking the state of the clocks before
performing actions, that also is broken.

> The fewer ties you have with any bootloaders, the fewer bugs you'll have 
> in the end.  Otherwise you might enter into a game of requiring a 
> specific version of a specific bootloader to go with a given kernel, and 
> that is far more complex to deal with than the complexity you are 
> referring to above.

Agreed.

thx,

Jason.