* [PATCH] memory: add a basic OF-based memory driver
[not found] <1378863781-4235-1-git-send-email-emilio@elopez.com.ar>
@ 2013-09-13 0:30 ` Emilio López
2013-09-13 0:57 ` Olof Johansson
0 siblings, 1 reply; 6+ messages in thread
From: Emilio López @ 2013-09-13 0:30 UTC (permalink / raw
To: Mike Turquette, Maxime Ripard, grant.likely, rob.herring, gregkh
Cc: devicetree, linux-arm-kernel, david.lanzendoerfer, linux-kernel,
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@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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] memory: add a basic OF-based memory driver
2013-09-13 0:30 ` [PATCH] memory: add a basic OF-based memory driver Emilio López
@ 2013-09-13 0:57 ` Olof Johansson
2013-09-13 1:31 ` Emilio López
0 siblings, 1 reply; 6+ messages in thread
From: Olof Johansson @ 2013-09-13 0:57 UTC (permalink / raw
To: Emilio López
Cc: Mike Turquette, Maxime Ripard, Grant Likely, Rob Herring,
Greg Kroah-Hartman, devicetree@vger.kernel.org,
David Lanzendörfer, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] memory: add a basic OF-based memory driver
2013-09-13 0:57 ` Olof Johansson
@ 2013-09-13 1:31 ` Emilio López
2013-09-13 14:00 ` Rob Herring
0 siblings, 1 reply; 6+ messages in thread
From: Emilio López @ 2013-09-13 1:31 UTC (permalink / raw
To: Olof Johansson
Cc: Mike Turquette, Maxime Ripard, Grant Likely, Rob Herring,
Greg Kroah-Hartman, devicetree@vger.kernel.org,
David Lanzendörfer, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
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@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@0123000 {
compatible = "vendor,awesome-mc", "simple-memory-controller";
reg = <0x0123000 0x400>;
clocks = <&pll5 1>;
};
Cheers,
Emilio
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] memory: add a basic OF-based memory driver
2013-09-13 1:31 ` Emilio López
@ 2013-09-13 14:00 ` Rob Herring
2013-09-13 15:49 ` Olof Johansson
0 siblings, 1 reply; 6+ messages in thread
From: Rob Herring @ 2013-09-13 14:00 UTC (permalink / raw
To: Emilio López
Cc: Olof Johansson, Mike Turquette, Maxime Ripard, Grant Likely,
Rob Herring, Greg Kroah-Hartman, devicetree@vger.kernel.org,
David Lanzendörfer, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
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@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@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] memory: add a basic OF-based memory driver
2013-09-13 14:00 ` Rob Herring
@ 2013-09-13 15:49 ` Olof Johansson
[not found] ` <20130915124325.B1DF2C42C5C@trevor.secretlab.ca>
0 siblings, 1 reply; 6+ messages in thread
From: Olof Johansson @ 2013-09-13 15:49 UTC (permalink / raw
To: Rob Herring
Cc: Emilio López, Mike Turquette, Maxime Ripard, Grant Likely,
Rob Herring, Greg Kroah-Hartman, devicetree@vger.kernel.org,
David Lanzendörfer, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
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@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@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] memory: add a basic OF-based memory driver
[not found] ` <20130915124325.B1DF2C42C5C@trevor.secretlab.ca>
@ 2013-09-16 12:55 ` Rob Herring
0 siblings, 0 replies; 6+ messages in thread
From: Rob Herring @ 2013-09-16 12:55 UTC (permalink / raw
To: Grant Likely
Cc: Olof Johansson, Emilio López, Mike Turquette, Maxime Ripard,
Rob Herring, Greg Kroah-Hartman, devicetree@vger.kernel.org,
David Lanzendörfer, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-09-16 12:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1378863781-4235-1-git-send-email-emilio@elopez.com.ar>
2013-09-13 0:30 ` [PATCH] memory: add a basic OF-based memory driver Emilio López
2013-09-13 0:57 ` Olof Johansson
2013-09-13 1:31 ` Emilio López
2013-09-13 14:00 ` Rob Herring
2013-09-13 15:49 ` Olof Johansson
[not found] ` <20130915124325.B1DF2C42C5C@trevor.secretlab.ca>
2013-09-16 12:55 ` Rob Herring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).