[PATCH RFC] of: add a basic memory driver

[PATCH RFC] of: add a basic memory driver

[Emilio López]

This driver's only job is to claim and ensure that the necessary clock
for memory operation on a DT-enabled machine remains enabled.

Signed-off-by: Emilio L?pez <emilio@elopez.com.ar>
---

Hi,

I am currently facing an issue with the clock setup: a critical but
unclaimed clock gets disabled as a side effect of disabling one of its
children. The clock setup looks something like this:

      PLL
       |
 ------------
 |          |
DDR       Others
            |
          periph

The PLL clock is marked with the CLK_IGNORE_UNUSED flag, so on a normal
boot it remains on, even after the unused clocks cleanup code runs. The
problem occurs when someone enables "periph" and then, later on, disables
it: the framework starts disabling clocks upwards on the tree, 
eventually switching the PLL off (and that kills the machine, as the memory
clock is shut down).

There's two possible solutions I can think of:
 1) add some extra checks on the framework to not turn off clocks marked
    with such a flag on the non-explicit case (ie, when I'm disabling
    some other clock)
 2) create an actual user of the DDR clock, that way it won't get
    disabled simply because it's being used.

I considered 1) and implemented it, but the result was not pretty. This 
patch is my take on 2). Please let me know what you think; all feedback
is welcome :)

Cheers,

Emilio

 drivers/of/Kconfig     |  6 ++++++
 drivers/of/Makefile    |  1 +
 drivers/of/of_memory.c | 30 ++++++++++++++++++++++++++++++
 3 files changed, 37 insertions(+)
 create mode 100644 drivers/of/of_memory.c

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 9d2009a..f6c5e20 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -80,4 +80,10 @@ config OF_RESERVED_MEM
 	help
 	  Initialization code for DMA reserved memory
 
+config OF_MEMORY
+	depends on COMMON_CLK
+	def_bool y
+	help
+	  Simple memory initialization
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index ed9660a..15f0167 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_OF_PCI)	+= of_pci.o
 obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
 obj-$(CONFIG_OF_MTD)	+= of_mtd.o
 obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
+obj-$(CONFIG_OF_MEMORY) += of_memory.o
diff --git a/drivers/of/of_memory.c b/drivers/of/of_memory.c
new file mode 100644
index 0000000..a833f7a
--- /dev/null
+++ b/drivers/of/of_memory.c
@@ -0,0 +1,30 @@
+/*
+ * Simple memory driver
+ */
+
+#include <linux/of.h>
+#include <linux/clk.h>
+
+static int __init of_memory_enable(void)
+{
+	struct device_node *np;
+	struct clk *clk;
+
+	np = of_find_node_by_path("/memory");
+	if (!np) {
+		pr_err("no /memory on DT!\n");
+		return 0;
+	}
+
+	clk = of_clk_get(np, 0);
+	if (!IS_ERR(clk)) {
+		clk_prepare_enable(clk);
+		clk_put(clk);
+	}
+
+	of_node_put(np);
+
+	return 0;
+}
+
+device_initcall(of_memory_enable);
-- 
1.8.4

[PATCH RFC] of: add a basic memory driver

[Maxime Ripard]

Hi Emilio,

On Tue, Sep 10, 2013 at 10:43:01PM -0300, Emilio L?pez wrote:
> This driver's only job is to claim and ensure that the necessary clock
> for memory operation on a DT-enabled machine remains enabled.
> 
> Signed-off-by: Emilio L?pez <emilio@elopez.com.ar>
> ---
> 
> Hi,
> 
> I am currently facing an issue with the clock setup: a critical but
> unclaimed clock gets disabled as a side effect of disabling one of its
> children. The clock setup looks something like this:
> 
>       PLL
>        |
>  ------------
>  |          |
> DDR       Others
>             |
>           periph
> 
> The PLL clock is marked with the CLK_IGNORE_UNUSED flag, so on a normal
> boot it remains on, even after the unused clocks cleanup code runs. The
> problem occurs when someone enables "periph" and then, later on, disables
> it: the framework starts disabling clocks upwards on the tree, 
> eventually switching the PLL off (and that kills the machine, as the memory
> clock is shut down).

That looks like a bug in the clock framework. I'd expect it to at least
behave in the same way when disabling the unused clocks at late startup
and when going up disabling some clocks' parent later on.

> There's two possible solutions I can think of:
>  1) add some extra checks on the framework to not turn off clocks marked
>     with such a flag on the non-explicit case (ie, when I'm disabling
>     some other clock)
> 
>  2) create an actual user of the DDR clock, that way it won't get
>     disabled simply because it's being used.
> 
> I considered 1) and implemented it, but the result was not pretty.

What was not pretty about it?

> This patch is my take on 2). Please let me know what you think; all
> feedback is welcome :)
> 
> Cheers,
> 
> Emilio
> 
>  drivers/of/Kconfig     |  6 ++++++
>  drivers/of/Makefile    |  1 +
>  drivers/of/of_memory.c | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 37 insertions(+)
>  create mode 100644 drivers/of/of_memory.c
> 
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 9d2009a..f6c5e20 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -80,4 +80,10 @@ config OF_RESERVED_MEM
>  	help
>  	  Initialization code for DMA reserved memory
>  
> +config OF_MEMORY
> +	depends on COMMON_CLK
> +	def_bool y
> +	help
> +	  Simple memory initialization
> +
>  endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index ed9660a..15f0167 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_OF_PCI)	+= of_pci.o
>  obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
>  obj-$(CONFIG_OF_MTD)	+= of_mtd.o
>  obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
> +obj-$(CONFIG_OF_MEMORY) += of_memory.o
> diff --git a/drivers/of/of_memory.c b/drivers/of/of_memory.c
> new file mode 100644
> index 0000000..a833f7a
> --- /dev/null
> +++ b/drivers/of/of_memory.c
> @@ -0,0 +1,30 @@
> +/*
> + * Simple memory driver
> + */
> +
> +#include <linux/of.h>
> +#include <linux/clk.h>
> +
> +static int __init of_memory_enable(void)
> +{
> +	struct device_node *np;
> +	struct clk *clk;
> +
> +	np = of_find_node_by_path("/memory");
> +	if (!np) {
> +		pr_err("no /memory on DT!\n");
> +		return 0;
> +	}
> +
> +	clk = of_clk_get(np, 0);
> +	if (!IS_ERR(clk)) {
> +		clk_prepare_enable(clk);
> +		clk_put(clk);
> +	}
> +
> +	of_node_put(np);
> +
> +	return 0;
> +}
> +
> +device_initcall(of_memory_enable);

I like this idea as well. But imho, both 1 and 2 should be done. 2) is
only about memory devices, while 1) is much more generic.

And fwiw, the Marvell Armada 370 is also in this case of having a
gatable clock for the DDR that could potentially be disabled (but is
not, since it has no other users than the DDR itself, and as such, no
one ever calls clk_disable on it). 

Thanks!

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130911/bfe85d37/attachment.sig>

[PATCH RFC] of: add a basic memory driver

[Emilio López]

Hi Maxime,

El 11/09/13 04:54, Maxime Ripard escribi?:
> Hi Emilio,
>
> On Tue, Sep 10, 2013 at 10:43:01PM -0300, Emilio L?pez wrote:
>> This driver's only job is to claim and ensure that the necessary clock
>> for memory operation on a DT-enabled machine remains enabled.
>>
>> Signed-off-by: Emilio L?pez <emilio@elopez.com.ar>
>> ---
>>
>> Hi,
>>
>> I am currently facing an issue with the clock setup: a critical but
>> unclaimed clock gets disabled as a side effect of disabling one of its
>> children. The clock setup looks something like this:
>>
>>        PLL
>>         |
>>   ------------
>>   |          |
>> DDR       Others
>>              |
>>            periph
>>
>> The PLL clock is marked with the CLK_IGNORE_UNUSED flag, so on a normal
>> boot it remains on, even after the unused clocks cleanup code runs. The
>> problem occurs when someone enables "periph" and then, later on, disables
>> it: the framework starts disabling clocks upwards on the tree,
>> eventually switching the PLL off (and that kills the machine, as the memory
>> clock is shut down).
>
> That looks like a bug in the clock framework. I'd expect it to at least
> behave in the same way when disabling the unused clocks at late startup
> and when going up disabling some clocks' parent later on.

Yes, I kind of expected the same, and the flag description seems to 
imply so too:

     #define CLK_IGNORE_UNUSED	BIT(3) /* do not gate even if unused */

>> There's two possible solutions I can think of:
>>   1) add some extra checks on the framework to not turn off clocks marked
>>      with such a flag on the non-explicit case (ie, when I'm disabling
>>      some other clock)
>>
>>   2) create an actual user of the DDR clock, that way it won't get
>>      disabled simply because it's being used.
>>
>> I considered 1) and implemented it, but the result was not pretty.
>
> What was not pretty about it?

It required adding an extra parameter to __clk_disable/__clk_unprepare 
to keep track of the call's explicitness, and ignore the 
disable/unprepare callback on the implicit case (when 
__clk_disable/__clk_unprepare is called recursively) if the flag is set. 
This also means adding a wrapping function to at least __clk_unprepare, 
so as to to not break callers outside of the clk framework. Overall it 
felt too hacky for something that could be properly handled by the 
generic code if it had at least 1 user.

I would like to hear Mike's thoughts on this; maybe CLK_IGNORE_UNUSED is 
not what we think it should be.

>> This patch is my take on 2). Please let me know what you think; all
>> feedback is welcome :)
>>
>> Cheers,
>>
>> Emilio
>>
>>   drivers/of/Kconfig     |  6 ++++++
>>   drivers/of/Makefile    |  1 +
>>   drivers/of/of_memory.c | 30 ++++++++++++++++++++++++++++++
>>   3 files changed, 37 insertions(+)
>>   create mode 100644 drivers/of/of_memory.c
>>
>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
>> index 9d2009a..f6c5e20 100644
>> --- a/drivers/of/Kconfig
>> +++ b/drivers/of/Kconfig
>> @@ -80,4 +80,10 @@ config OF_RESERVED_MEM
>>   	help
>>   	  Initialization code for DMA reserved memory
>>
>> +config OF_MEMORY
>> +	depends on COMMON_CLK
>> +	def_bool y
>> +	help
>> +	  Simple memory initialization
>> +
>>   endmenu # OF
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index ed9660a..15f0167 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -10,3 +10,4 @@ obj-$(CONFIG_OF_PCI)	+= of_pci.o
>>   obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
>>   obj-$(CONFIG_OF_MTD)	+= of_mtd.o
>>   obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
>> +obj-$(CONFIG_OF_MEMORY) += of_memory.o
>> diff --git a/drivers/of/of_memory.c b/drivers/of/of_memory.c
>> new file mode 100644
>> index 0000000..a833f7a
>> --- /dev/null
>> +++ b/drivers/of/of_memory.c
>> @@ -0,0 +1,30 @@
>> +/*
>> + * Simple memory driver
>> + */
>> +
>> +#include <linux/of.h>
>> +#include <linux/clk.h>
>> +
>> +static int __init of_memory_enable(void)
>> +{
>> +	struct device_node *np;
>> +	struct clk *clk;
>> +
>> +	np = of_find_node_by_path("/memory");
>> +	if (!np) {
>> +		pr_err("no /memory on DT!\n");
>> +		return 0;
>> +	}
>> +
>> +	clk = of_clk_get(np, 0);
>> +	if (!IS_ERR(clk)) {
>> +		clk_prepare_enable(clk);
>> +		clk_put(clk);
>> +	}
>> +
>> +	of_node_put(np);
>> +
>> +	return 0;
>> +}
>> +
>> +device_initcall(of_memory_enable);
>
> I like this idea as well. But imho, both 1 and 2 should be done. 2) is
> only about memory devices, while 1) is much more generic.
>
> And fwiw, the Marvell Armada 370 is also in this case of having a
> gatable clock for the DDR that could potentially be disabled (but is
> not, since it has no other users than the DDR itself, and as such, no
> one ever calls clk_disable on it).

Nice to know, thanks for the information :)

Cheers,

Emilio

[PATCH RFC] of: add a basic memory driver

[Mike Turquette]

Quoting Emilio L?pez (2013-09-11 02:34:01)
> Hi Maxime,
> 
> El 11/09/13 04:54, Maxime Ripard escribi?:
> > Hi Emilio,
> >
> > On Tue, Sep 10, 2013 at 10:43:01PM -0300, Emilio L?pez wrote:
> >> This driver's only job is to claim and ensure that the necessary clock
> >> for memory operation on a DT-enabled machine remains enabled.
> >>
> >> Signed-off-by: Emilio L?pez <emilio@elopez.com.ar>
> >> ---
> >>
> >> Hi,
> >>
> >> I am currently facing an issue with the clock setup: a critical but
> >> unclaimed clock gets disabled as a side effect of disabling one of its
> >> children. The clock setup looks something like this:
> >>
> >>        PLL
> >>         |
> >>   ------------
> >>   |          |
> >> DDR       Others
> >>              |
> >>            periph
> >>
> >> The PLL clock is marked with the CLK_IGNORE_UNUSED flag, so on a normal
> >> boot it remains on, even after the unused clocks cleanup code runs. The
> >> problem occurs when someone enables "periph" and then, later on, disables
> >> it: the framework starts disabling clocks upwards on the tree,
> >> eventually switching the PLL off (and that kills the machine, as the memory
> >> clock is shut down).
> >
> > That looks like a bug in the clock framework. I'd expect it to at least
> > behave in the same way when disabling the unused clocks at late startup
> > and when going up disabling some clocks' parent later on.
> 
> Yes, I kind of expected the same, and the flag description seems to 
> imply so too:
> 
>      #define CLK_IGNORE_UNUSED  BIT(3) /* do not gate even if unused */
> 
> >> There's two possible solutions I can think of:
> >>   1) add some extra checks on the framework to not turn off clocks marked
> >>      with such a flag on the non-explicit case (ie, when I'm disabling
> >>      some other clock)
> >>
> >>   2) create an actual user of the DDR clock, that way it won't get
> >>      disabled simply because it's being used.
> >>
> >> I considered 1) and implemented it, but the result was not pretty.
> >
> > What was not pretty about it?
> 
> It required adding an extra parameter to __clk_disable/__clk_unprepare 
> to keep track of the call's explicitness, and ignore the 
> disable/unprepare callback on the implicit case (when 
> __clk_disable/__clk_unprepare is called recursively) if the flag is set. 
> This also means adding a wrapping function to at least __clk_unprepare, 
> so as to to not break callers outside of the clk framework. Overall it 
> felt too hacky for something that could be properly handled by the 
> generic code if it had at least 1 user.
> 
> I would like to hear Mike's thoughts on this; maybe CLK_IGNORE_UNUSED is 
> not what we think it should be.

CLK_IGNORE_UNUSED was only meant to solve the issue of gating unclaimed
clocks during clk_disable_unused that we do not want to gate at that
point in time. To that end it is doing the right thing for your platform
and I don't see a bug here.

Your issue is that you have a normal clk_disable chain affecting the
clock. Always the right way to do this is to have a driver claim that
clock with clk_get and call clk_enable on it. That is how the clk API
works. This is what your 2) approach does and is probably The Right
Thing.

Alternatively a new flag could be added, CLK_ALWON. This flag should be
discussed but it could do a number of things:

1) any attempts to disable a clk with this flag set will always fail
silently (since clk_disable has void return type).

2) same as 1) above but the clock framework could additionally call
clk_enable on it after registering the clock with the CLK_ALWON flag set

Anyways I think a new flag like CLK_ALWON should only really exist to
solve this problem for clocks that do not have drivers in Linux. Since
you went to the trouble of writing a small driver then I think just
claiming the clock and enabling it there is the best solution.

Regards,
Mike

> 
> >> This patch is my take on 2). Please let me know what you think; all
> >> feedback is welcome :)
> >>
> >> Cheers,
> >>
> >> Emilio
> >>
> >>   drivers/of/Kconfig     |  6 ++++++
> >>   drivers/of/Makefile    |  1 +
> >>   drivers/of/of_memory.c | 30 ++++++++++++++++++++++++++++++
> >>   3 files changed, 37 insertions(+)
> >>   create mode 100644 drivers/of/of_memory.c
> >>
> >> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> >> index 9d2009a..f6c5e20 100644
> >> --- a/drivers/of/Kconfig
> >> +++ b/drivers/of/Kconfig
> >> @@ -80,4 +80,10 @@ config OF_RESERVED_MEM
> >>      help
> >>        Initialization code for DMA reserved memory
> >>
> >> +config OF_MEMORY
> >> +    depends on COMMON_CLK
> >> +    def_bool y
> >> +    help
> >> +      Simple memory initialization
> >> +
> >>   endmenu # OF
> >> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> >> index ed9660a..15f0167 100644
> >> --- a/drivers/of/Makefile
> >> +++ b/drivers/of/Makefile
> >> @@ -10,3 +10,4 @@ obj-$(CONFIG_OF_PCI)       += of_pci.o
> >>   obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
> >>   obj-$(CONFIG_OF_MTD)       += of_mtd.o
> >>   obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
> >> +obj-$(CONFIG_OF_MEMORY) += of_memory.o
> >> diff --git a/drivers/of/of_memory.c b/drivers/of/of_memory.c
> >> new file mode 100644
> >> index 0000000..a833f7a
> >> --- /dev/null
> >> +++ b/drivers/of/of_memory.c
> >> @@ -0,0 +1,30 @@
> >> +/*
> >> + * Simple memory driver
> >> + */
> >> +
> >> +#include <linux/of.h>
> >> +#include <linux/clk.h>
> >> +
> >> +static int __init of_memory_enable(void)
> >> +{
> >> +    struct device_node *np;
> >> +    struct clk *clk;
> >> +
> >> +    np = of_find_node_by_path("/memory");
> >> +    if (!np) {
> >> +            pr_err("no /memory on DT!\n");
> >> +            return 0;
> >> +    }
> >> +
> >> +    clk = of_clk_get(np, 0);
> >> +    if (!IS_ERR(clk)) {
> >> +            clk_prepare_enable(clk);
> >> +            clk_put(clk);
> >> +    }
> >> +
> >> +    of_node_put(np);
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +device_initcall(of_memory_enable);
> >
> > I like this idea as well. But imho, both 1 and 2 should be done. 2) is
> > only about memory devices, while 1) is much more generic.
> >
> > And fwiw, the Marvell Armada 370 is also in this case of having a
> > gatable clock for the DDR that could potentially be disabled (but is
> > not, since it has no other users than the DDR itself, and as such, no
> > one ever calls clk_disable on it).
> 
> Nice to know, thanks for the information :)
> 
> Cheers,
> 
> Emilio

[PATCH RFC] of: add a basic memory driver

[Rob Herring]

On 09/10/2013 08:43 PM, Emilio L?pez wrote:
> This driver's only job is to claim and ensure that the necessary clock
> for memory operation on a DT-enabled machine remains enabled.
> 
> Signed-off-by: Emilio L?pez <emilio@elopez.com.ar>
> ---
> 
> Hi,
> 
> I am currently facing an issue with the clock setup: a critical but
> unclaimed clock gets disabled as a side effect of disabling one of its
> children. The clock setup looks something like this:
> 
>       PLL
>        |
>  ------------
>  |          |
> DDR       Others
>             |
>           periph

This would be more accurate:

       PLL
        |
 ----------------
 |              |
DDR Ctrlr     Others
 |
DDR

So having a DDR controller node with a clock is the right way to do
this. There are other possible needs for describing the DDR controller
such as low power modes and ECC.

Rob

> 
> The PLL clock is marked with the CLK_IGNORE_UNUSED flag, so on a normal
> boot it remains on, even after the unused clocks cleanup code runs. The
> problem occurs when someone enables "periph" and then, later on, disables
> it: the framework starts disabling clocks upwards on the tree, 
> eventually switching the PLL off (and that kills the machine, as the memory
> clock is shut down).
> 
> There's two possible solutions I can think of:
>  1) add some extra checks on the framework to not turn off clocks marked
>     with such a flag on the non-explicit case (ie, when I'm disabling
>     some other clock)
>  2) create an actual user of the DDR clock, that way it won't get
>     disabled simply because it's being used.
> 
> I considered 1) and implemented it, but the result was not pretty. This 
> patch is my take on 2). Please let me know what you think; all feedback
> is welcome :)
> 
> Cheers,
> 
> Emilio
> 
>  drivers/of/Kconfig     |  6 ++++++
>  drivers/of/Makefile    |  1 +
>  drivers/of/of_memory.c | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 37 insertions(+)
>  create mode 100644 drivers/of/of_memory.c
> 
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 9d2009a..f6c5e20 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -80,4 +80,10 @@ config OF_RESERVED_MEM
>  	help
>  	  Initialization code for DMA reserved memory
>  
> +config OF_MEMORY
> +	depends on COMMON_CLK
> +	def_bool y
> +	help
> +	  Simple memory initialization
> +
>  endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index ed9660a..15f0167 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_OF_PCI)	+= of_pci.o
>  obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
>  obj-$(CONFIG_OF_MTD)	+= of_mtd.o
>  obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
> +obj-$(CONFIG_OF_MEMORY) += of_memory.o
> diff --git a/drivers/of/of_memory.c b/drivers/of/of_memory.c
> new file mode 100644
> index 0000000..a833f7a
> --- /dev/null
> +++ b/drivers/of/of_memory.c
> @@ -0,0 +1,30 @@
> +/*
> + * Simple memory driver
> + */
> +
> +#include <linux/of.h>
> +#include <linux/clk.h>
> +
> +static int __init of_memory_enable(void)
> +{
> +	struct device_node *np;
> +	struct clk *clk;
> +
> +	np = of_find_node_by_path("/memory");
> +	if (!np) {
> +		pr_err("no /memory on DT!\n");
> +		return 0;
> +	}
> +
> +	clk = of_clk_get(np, 0);
> +	if (!IS_ERR(clk)) {
> +		clk_prepare_enable(clk);
> +		clk_put(clk);
> +	}
> +
> +	of_node_put(np);
> +
> +	return 0;
> +}
> +
> +device_initcall(of_memory_enable);
> 

[PATCH] memory: add a basic OF-based memory driver

[Emilio López]

This driver's only job is to claim and ensure the necessary clock
for memory operation on a DT-powered machine remains enabled.

Signed-off-by: Emilio L?pez <emilio@elopez.com.ar>
---

I believe this new patch should resolve all the concerns raised; as
always, all feedback is welcome :)

Changes from RFC:
- Move from drivers/of to drivers/memory
- Make a proper driver instead of using an initcall
- Binding document for the new "simple-memory-controller"

 .../simple-memory-controller.txt                   | 19 ++++++++
 drivers/memory/Kconfig                             | 11 +++++
 drivers/memory/Makefile                            |  1 +
 drivers/memory/simple-mc.c                         | 57 ++++++++++++++++++++++
 4 files changed, 88 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/simple-memory-controller.txt
 create mode 100644 drivers/memory/simple-mc.c

diff --git a/Documentation/devicetree/bindings/memory-controllers/simple-memory-controller.txt b/Documentation/devicetree/bindings/memory-controllers/simple-memory-controller.txt
new file mode 100644
index 0000000..d37683b
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/simple-memory-controller.txt
@@ -0,0 +1,19 @@
+Device Tree Clock binding for a simple memory controller.
+
+Required properties:
+- compatible : shall be "simple-memory-controller"
+
+Optional properties:
+- reg        : may contain the register space for the controller. This
+               property is currently ignored by the driver
+- clocks     : may contain a phandle to the clock that is currently being
+               used on the controller. This clock shall remain enabled
+               during system operation.
+
+Example:
+
+mc: mc at 0123000 {
+	compatible = "simple-memory-controller";
+	reg = <0x0123000 0x400>;
+	clocks = <&pll5 1>;
+};
diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index 29a11db..4a6df65 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -50,4 +50,15 @@ config TEGRA30_MC
 	  analysis, especially for IOMMU/SMMU(System Memory Management
 	  Unit) module.
 
+config SIMPLE_MC
+	bool "Simple memory controller"
+	default y
+	depends on OF && COMMON_CLK
+	help
+	  This driver is able to manage a simple memory controller whose
+	  only needs consist of keeping one clock enabled. The
+	  controller must be defined on the device tree as compatible
+	  with "simple-memory-controller"; see the corresponding binding
+	  document for more details.
+
 endif
diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index 969d923..e0953e5 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_TI_EMIF)		+= emif.o
 obj-$(CONFIG_MVEBU_DEVBUS)	+= mvebu-devbus.o
 obj-$(CONFIG_TEGRA20_MC)	+= tegra20-mc.o
 obj-$(CONFIG_TEGRA30_MC)	+= tegra30-mc.o
+obj-$(CONFIG_SIMPLE_MC)		+= simple-mc.o
diff --git a/drivers/memory/simple-mc.c b/drivers/memory/simple-mc.c
new file mode 100644
index 0000000..e58371d
--- /dev/null
+++ b/drivers/memory/simple-mc.c
@@ -0,0 +1,57 @@
+/*
+ * Simple memory controller driver
+ *
+ * Copyright 2013 Emilio L?pez <emilio@elopez.com.ar>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/of.h>
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+static int simple_mc_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct clk *clk;
+
+	if (!np)
+		return -ENODEV;
+
+	clk = of_clk_get(np, 0);
+	if (!IS_ERR(clk)) {
+		clk_prepare_enable(clk);
+		clk_put(clk);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id simple_mc_of_match[] = {
+	{ .compatible = "simple-memory-controller", },
+	{ /* sentinel */ },
+};
+
+static struct platform_driver simple_mc_driver = {
+	.probe = simple_mc_probe,
+	.driver = {
+		.name = "simple-mc",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(simple_mc_of_match),
+	},
+};
+
+module_platform_driver(simple_mc_driver);
+
+MODULE_AUTHOR("Emilio L?pez <emilio@elopez.com.ar>");
+MODULE_DESCRIPTION("Simple memory controller driver");
+MODULE_LICENSE("GPL");
-- 
1.8.4

[PATCH] memory: add a basic OF-based memory driver

[Olof Johansson]

On Thu, Sep 12, 2013 at 5:30 PM, Emilio L?pez <emilio@elopez.com.ar> wrote:
> This driver's only job is to claim and ensure the necessary clock
> for memory operation on a DT-powered machine remains enabled.
>
> Signed-off-by: Emilio L?pez <emilio@elopez.com.ar>
> ---
>
> I believe this new patch should resolve all the concerns raised; as
> always, all feedback is welcome :)

I think you're going about this the wrong way.

If you have a problem with a clock not staying on, shouldn't you just
marking it appropriately in the clock table instead, making sure it's
initialized with at least one reference to it? I believe that is how
some of the other platforms handle this, and it's a lot cleaner than
adding a fake binding and a fake driver just to grab a single clock.


-Olof

[PATCH] memory: add a basic OF-based memory driver

[Emilio López]

Hi Olof,

El 12/09/13 21:57, Olof Johansson escribi?:
> On Thu, Sep 12, 2013 at 5:30 PM, Emilio L?pez <emilio@elopez.com.ar> wrote:
>> This driver's only job is to claim and ensure the necessary clock
>> for memory operation on a DT-powered machine remains enabled.
>>
>> Signed-off-by: Emilio L?pez <emilio@elopez.com.ar>
>> ---
>>
>> I believe this new patch should resolve all the concerns raised; as
>> always, all feedback is welcome :)
>
> I think you're going about this the wrong way.
>
> If you have a problem with a clock not staying on, shouldn't you just
> marking it appropriately in the clock table instead, making sure it's
> initialized with at least one reference to it?

If by "the clock table" you mean the tree as handled by the common clock 
framework, there is no such flag available as of today; see Mike's reply 
for more information.

Personally I feel that if the general case can solve our problems (in 
this case, having a consumer who prepares and enables the clock), we 
should avoid adding special cases to the framework.

> I believe that is how
> some of the other platforms handle this, and it's a lot cleaner than
> adding a fake binding and a fake driver just to grab a single clock.

The binding doesn't have to be fake; it is actually describing the 
memory controller hardware:

mc: mc at 0123000 {
	compatible = "simple-memory-controller";
	reg = <0x0123000 0x400>;
	clocks = <&pll5 1>;
};

If one day we get docs and/or have any special features we may need from 
the controller, we can use something like

mc: mc at 0123000 {
	compatible = "vendor,awesome-mc", "simple-memory-controller";
	reg = <0x0123000 0x400>;
	clocks = <&pll5 1>;
};

Cheers,

Emilio

[PATCH] memory: add a basic OF-based memory driver

[Rob Herring]

On Thu, Sep 12, 2013 at 8:31 PM, Emilio L?pez <emilio@elopez.com.ar> wrote:
> Hi Olof,
>
> El 12/09/13 21:57, Olof Johansson escribi?:
>
>> On Thu, Sep 12, 2013 at 5:30 PM, Emilio L?pez <emilio@elopez.com.ar>
>> wrote:
>>>
>>> This driver's only job is to claim and ensure the necessary clock
>>> for memory operation on a DT-powered machine remains enabled.
>>>
>>> Signed-off-by: Emilio L?pez <emilio@elopez.com.ar>
>>> ---
>>>
>>> I believe this new patch should resolve all the concerns raised; as
>>> always, all feedback is welcome :)
>>
>>
>> I think you're going about this the wrong way.
>>
>> If you have a problem with a clock not staying on, shouldn't you just
>> marking it appropriately in the clock table instead, making sure it's
>> initialized with at least one reference to it?
>
>
> If by "the clock table" you mean the tree as handled by the common clock
> framework, there is no such flag available as of today; see Mike's reply for
> more information.
>
> Personally I feel that if the general case can solve our problems (in this
> case, having a consumer who prepares and enables the clock), we should avoid
> adding special cases to the framework.
>
>
>> I believe that is how
>> some of the other platforms handle this, and it's a lot cleaner than
>> adding a fake binding and a fake driver just to grab a single clock.
>
>
> The binding doesn't have to be fake; it is actually describing the memory
> controller hardware:
>
> mc: mc at 0123000 {
>         compatible = "simple-memory-controller";
>         reg = <0x0123000 0x400>;
>         clocks = <&pll5 1>;
> };
>
> If one day we get docs and/or have any special features we may need from the
> controller, we can use something like
>
> mc: mc at 0123000 {
>         compatible = "vendor,awesome-mc", "simple-memory-controller";
>         reg = <0x0123000 0x400>;
>         clocks = <&pll5 1>;
> };

Better, but this is still wrong. DT describes the hardware. There is
no such h/w as a simple-memory-controller. The fact that you have a
simple-memory-ctrlr kernel driver is a kernel
feature/artifact/limitation. Describe the h/w with a meaningful
compatible string and put that string in the simple memory controller
driver match table. If someday we have a real driver for said memory
controller, then it is only a kernel change to use a different driver.

Rob

[PATCH] memory: add a basic OF-based memory driver

[Olof Johansson]

On Fri, Sep 13, 2013 at 7:00 AM, Rob Herring <robherring2@gmail.com> wrote:
> On Thu, Sep 12, 2013 at 8:31 PM, Emilio L?pez <emilio@elopez.com.ar> wrote:
>> Hi Olof,
>>
>> El 12/09/13 21:57, Olof Johansson escribi?:
>>
>>> On Thu, Sep 12, 2013 at 5:30 PM, Emilio L?pez <emilio@elopez.com.ar>
>>> wrote:
>>>>
>>>> This driver's only job is to claim and ensure the necessary clock
>>>> for memory operation on a DT-powered machine remains enabled.
>>>>
>>>> Signed-off-by: Emilio L?pez <emilio@elopez.com.ar>
>>>> ---
>>>>
>>>> I believe this new patch should resolve all the concerns raised; as
>>>> always, all feedback is welcome :)
>>>
>>>
>>> I think you're going about this the wrong way.
>>>
>>> If you have a problem with a clock not staying on, shouldn't you just
>>> marking it appropriately in the clock table instead, making sure it's
>>> initialized with at least one reference to it?
>>
>>
>> If by "the clock table" you mean the tree as handled by the common clock
>> framework, there is no such flag available as of today; see Mike's reply for
>> more information.
>>
>> Personally I feel that if the general case can solve our problems (in this
>> case, having a consumer who prepares and enables the clock), we should avoid
>> adding special cases to the framework.
>>
>>
>>> I believe that is how
>>> some of the other platforms handle this, and it's a lot cleaner than
>>> adding a fake binding and a fake driver just to grab a single clock.
>>
>>
>> The binding doesn't have to be fake; it is actually describing the memory
>> controller hardware:
>>
>> mc: mc at 0123000 {
>>         compatible = "simple-memory-controller";
>>         reg = <0x0123000 0x400>;
>>         clocks = <&pll5 1>;
>> };
>>
>> If one day we get docs and/or have any special features we may need from the
>> controller, we can use something like
>>
>> mc: mc at 0123000 {
>>         compatible = "vendor,awesome-mc", "simple-memory-controller";
>>         reg = <0x0123000 0x400>;
>>         clocks = <&pll5 1>;
>> };
>
> Better, but this is still wrong. DT describes the hardware. There is
> no such h/w as a simple-memory-controller. The fact that you have a
> simple-memory-ctrlr kernel driver is a kernel
> feature/artifact/limitation. Describe the h/w with a meaningful
> compatible string and put that string in the simple memory controller
> driver match table. If someday we have a real driver for said memory
> controller, then it is only a kernel change to use a different driver.


We discussed this over IRC last night -- I still think it makes more
sense to make the clock driver for sunxi aware of this and just add a
reference to the clock at init time.

This is never going to differ from board to board (today the clock
name is the same on all sunxi platforms -- pll5_ddr. And the need will
likewise be there for all platforms at this time.

If and when it changes in the future, we can reevaluate. But this
doesn't have to be driven by device tree at this time, it seems to
just make things overly complicated and contrived.


-Olof

[PATCH] memory: add a basic OF-based memory driver

[Grant Likely]

On Fri, 13 Sep 2013 08:49:06 -0700, Olof Johansson <olof@lixom.net> wrote:
> On Fri, Sep 13, 2013 at 7:00 AM, Rob Herring <robherring2@gmail.com> wrote:
> > On Thu, Sep 12, 2013 at 8:31 PM, Emilio L??pez <emilio@elopez.com.ar> wrote:
> >> Hi Olof,
> >>
> >> El 12/09/13 21:57, Olof Johansson escribi??:
> >>
> >>> On Thu, Sep 12, 2013 at 5:30 PM, Emilio L??pez <emilio@elopez.com.ar>
> >>> wrote:
> >>>>
> >>>> This driver's only job is to claim and ensure the necessary clock
> >>>> for memory operation on a DT-powered machine remains enabled.
> >>>>
> >>>> Signed-off-by: Emilio L??pez <emilio@elopez.com.ar>
> >>>> ---
> >>>>
> >>>> I believe this new patch should resolve all the concerns raised; as
> >>>> always, all feedback is welcome :)
> >>>
> >>>
> >>> I think you're going about this the wrong way.
> >>>
> >>> If you have a problem with a clock not staying on, shouldn't you just
> >>> marking it appropriately in the clock table instead, making sure it's
> >>> initialized with at least one reference to it?
> >>
> >>
> >> If by "the clock table" you mean the tree as handled by the common clock
> >> framework, there is no such flag available as of today; see Mike's reply for
> >> more information.
> >>
> >> Personally I feel that if the general case can solve our problems (in this
> >> case, having a consumer who prepares and enables the clock), we should avoid
> >> adding special cases to the framework.
> >>
> >>
> >>> I believe that is how
> >>> some of the other platforms handle this, and it's a lot cleaner than
> >>> adding a fake binding and a fake driver just to grab a single clock.
> >>
> >>
> >> The binding doesn't have to be fake; it is actually describing the memory
> >> controller hardware:
> >>
> >> mc: mc at 0123000 {
> >>         compatible = "simple-memory-controller";
> >>         reg = <0x0123000 0x400>;
> >>         clocks = <&pll5 1>;
> >> };
> >>
> >> If one day we get docs and/or have any special features we may need from the
> >> controller, we can use something like
> >>
> >> mc: mc at 0123000 {
> >>         compatible = "vendor,awesome-mc", "simple-memory-controller";
> >>         reg = <0x0123000 0x400>;
> >>         clocks = <&pll5 1>;
> >> };
> >
> > Better, but this is still wrong. DT describes the hardware. There is
> > no such h/w as a simple-memory-controller. The fact that you have a
> > simple-memory-ctrlr kernel driver is a kernel
> > feature/artifact/limitation. Describe the h/w with a meaningful
> > compatible string and put that string in the simple memory controller
> > driver match table. If someday we have a real driver for said memory
> > controller, then it is only a kernel change to use a different driver.
> 
> 
> We discussed this over IRC last night -- I still think it makes more
> sense to make the clock driver for sunxi aware of this and just add a
> reference to the clock at init time.
> 
> This is never going to differ from board to board (today the clock
> name is the same on all sunxi platforms -- pll5_ddr. And the need will
> likewise be there for all platforms at this time.
> 
> If and when it changes in the future, we can reevaluate. But this
> doesn't have to be driven by device tree at this time, it seems to
> just make things overly complicated and contrived.

I agree. Creating a new platform driver + device tree binding just to
claim a clock that must not be disables does not look like the right
approach to me either.

g.

[PATCH] memory: add a basic OF-based memory driver

[Rob Herring]

On Sun, Sep 15, 2013 at 7:43 AM, Grant Likely <grant.likely@linaro.org> wrote:
> On Fri, 13 Sep 2013 08:49:06 -0700, Olof Johansson <olof@lixom.net> wrote:
>> On Fri, Sep 13, 2013 at 7:00 AM, Rob Herring <robherring2@gmail.com> wrote:
>> > On Thu, Sep 12, 2013 at 8:31 PM, Emilio L?pez <emilio@elopez.com.ar> wrote:

[snip]

>> > Better, but this is still wrong. DT describes the hardware. There is
>> > no such h/w as a simple-memory-controller. The fact that you have a
>> > simple-memory-ctrlr kernel driver is a kernel
>> > feature/artifact/limitation. Describe the h/w with a meaningful
>> > compatible string and put that string in the simple memory controller
>> > driver match table. If someday we have a real driver for said memory
>> > controller, then it is only a kernel change to use a different driver.
>>
>>
>> We discussed this over IRC last night -- I still think it makes more
>> sense to make the clock driver for sunxi aware of this and just add a
>> reference to the clock at init time.
>>
>> This is never going to differ from board to board (today the clock
>> name is the same on all sunxi platforms -- pll5_ddr. And the need will
>> likewise be there for all platforms at this time.
>>
>> If and when it changes in the future, we can reevaluate. But this
>> doesn't have to be driven by device tree at this time, it seems to
>> just make things overly complicated and contrived.
>
> I agree. Creating a new platform driver + device tree binding just to
> claim a clock that must not be disables does not look like the right
> approach to me either.

Maybe a driver is overkill, but fully describing the h/w would be a
good thing. Only defining the h/w that Linux currently uses is not a
good practice (although admittedly hard to avoid).

Rob