2014-07-07 15:19:32

by Alexandre Belloni

[permalink] [raw]
Subject: [PATCH 0/5] Add a driver for the atmel ram controller

The atmel ram controller needs one or more clocks to work. For now the
CLK_IGNORE_UNUSED flag is used on those clocks. This patch set introduce a
driver that will take care of taking those clocks.

The final goal is also to move the ioremap done in mach-at91 to that driver with
a proper power management driver.

Alexandre Belloni (5):
memory: add a driver for atmel ram controllers
ARM: at91: select ATMEL_RAMC when using OF
ARM: at91/dt: sama5d3: define mpddr clock and ramc clocks
ARM: at91/dt: at91sam9: use ddrck in ramc
clk: at91: remove the useless CLK_IGNORE_UNUSED flag

.../devicetree/bindings/arm/atmel-at91.txt | 1 +
arch/arm/boot/dts/at91sam9n12.dtsi | 2 +
arch/arm/boot/dts/at91sam9x5.dtsi | 2 +
arch/arm/boot/dts/sama5d3.dtsi | 9 +-
arch/arm/mach-at91/Kconfig | 4 +
drivers/clk/at91/clk-system.c | 8 +-
drivers/memory/Kconfig | 8 ++
drivers/memory/Makefile | 1 +
drivers/memory/atmel-ramc.c | 96 ++++++++++++++++++++++
9 files changed, 123 insertions(+), 8 deletions(-)
create mode 100644 drivers/memory/atmel-ramc.c

--
1.9.1


2014-07-07 15:19:35

by Alexandre Belloni

[permalink] [raw]
Subject: [PATCH 2/5] ARM: at91: select ATMEL_RAMC when using OF

When using device tree, select the Atmel RAM controller driver to handle its
clocks.

Signed-off-by: Alexandre Belloni <[email protected]>
---
arch/arm/mach-at91/Kconfig | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
index 45b55e0f0db6..ad6b2b8b6afd 100644
--- a/arch/arm/mach-at91/Kconfig
+++ b/arch/arm/mach-at91/Kconfig
@@ -49,6 +49,8 @@ config SOC_AT91SAM9
select GENERIC_CLOCKEVENTS
select MULTI_IRQ_HANDLER
select SPARSE_IRQ
+ select MEMORY if USE_OF
+ select ATMEL_RAMC if USE_OF

config SOC_SAMA5
bool
@@ -58,6 +60,8 @@ config SOC_SAMA5
select MULTI_IRQ_HANDLER
select SPARSE_IRQ
select USE_OF
+ select MEMORY
+ select ATMEL_RAMC

menu "Atmel AT91 System-on-Chip"

--
1.9.1

2014-07-07 15:19:49

by Alexandre Belloni

[permalink] [raw]
Subject: [PATCH 5/5] clk: at91: remove the useless CLK_IGNORE_UNUSED flag

The CLK_IGNORE_UNUSED flag was added on all the system clocks because of the
ddrck. Now that it is handled by the ram controller driver, we can drop it.

Signed-off-by: Alexandre Belloni <[email protected]>
---
drivers/clk/at91/clk-system.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/clk/at91/clk-system.c b/drivers/clk/at91/clk-system.c
index 8c96307d7363..a76d03fd577b 100644
--- a/drivers/clk/at91/clk-system.c
+++ b/drivers/clk/at91/clk-system.c
@@ -119,13 +119,7 @@ at91_clk_register_system(struct at91_pmc *pmc, const char *name,
init.ops = &system_ops;
init.parent_names = &parent_name;
init.num_parents = 1;
- /*
- * CLK_IGNORE_UNUSED is used to avoid ddrck switch off.
- * TODO : we should implement a driver supporting at91 ddr controller
- * (see drivers/memory) which would request and enable the ddrck clock.
- * When this is done we will be able to remove CLK_IGNORE_UNUSED flag.
- */
- init.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED;
+ init.flags = CLK_SET_RATE_PARENT;

sys->id = id;
sys->hw.init = &init;
--
1.9.1

2014-07-07 15:19:33

by Alexandre Belloni

[permalink] [raw]
Subject: [PATCH 1/5] memory: add a driver for atmel ram controllers

Atmel SoCs have one or multiple RAM controllers that need one or multiple clocks
to run.
This driver handle those clocks.

Signed-off-by: Alexandre Belloni <[email protected]>
---
.../devicetree/bindings/arm/atmel-at91.txt | 1 +
drivers/memory/Kconfig | 8 ++
drivers/memory/Makefile | 1 +
drivers/memory/atmel-ramc.c | 96 ++++++++++++++++++++++
4 files changed, 106 insertions(+)
create mode 100644 drivers/memory/atmel-ramc.c

diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt
index 16f60b41c147..54dc3aefb12a 100644
--- a/Documentation/devicetree/bindings/arm/atmel-at91.txt
+++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt
@@ -61,6 +61,7 @@ RAMC SDRAM/DDR Controller required properties:
- compatible: Should be "atmel,at91rm9200-sdramc",
"atmel,at91sam9260-sdramc",
"atmel,at91sam9g45-ddramc",
+ "atmel,sama5d3-mpddramc",
- reg: Should contain registers location and length
For at91sam9263 and at91sam9g45 you must specify 2 entries.

diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index c59e9c96e86d..e49b9caa1b30 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -7,6 +7,14 @@ menuconfig MEMORY

if MEMORY

+config ATMEL_RAMC
+ bool "Atmel (Multi-port DDR-)SDRAM Controller"
+ default y
+ depends on ARCH_AT91 && OF
+ help
+ This driver is for Atmel SDRAM Controller or Atmel Multi-port
+ DDR-SDRAM Controller available on Atmel AT91SAM9 and SAMA5 SoCs
+
config TI_AEMIF
tristate "Texas Instruments AEMIF driver"
depends on (ARCH_DAVINCI || ARCH_KEYSTONE) && OF
diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index 71160a2b7313..eb5d7716efa4 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -5,6 +5,7 @@
ifeq ($(CONFIG_DDR),y)
obj-$(CONFIG_OF) += of_memory.o
endif
+obj-$(CONFIG_ATMEL_RAMC) += atmel-ramc.o
obj-$(CONFIG_TI_AEMIF) += ti-aemif.o
obj-$(CONFIG_TI_EMIF) += emif.o
obj-$(CONFIG_FSL_IFC) += fsl_ifc.o
diff --git a/drivers/memory/atmel-ramc.c b/drivers/memory/atmel-ramc.c
new file mode 100644
index 000000000000..1d12d3d01cbd
--- /dev/null
+++ b/drivers/memory/atmel-ramc.c
@@ -0,0 +1,96 @@
+/*
+ * Atmel (Multi-port DDR-)SDRAM Controller driver
+ *
+ * Copyright (C) 2014 Atmel
+ *
+ * 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 version 2 of the License.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
+struct at91_ramc_caps {
+ bool has_ddrck;
+ bool has_mpddr_clk;
+};
+
+static const struct at91_ramc_caps at91rm9200_caps = { };
+
+static const struct at91_ramc_caps at91sam9g45_caps = {
+ .has_ddrck = 1,
+ .has_mpddr_clk = 0,
+};
+
+static const struct at91_ramc_caps sama5d3_caps = {
+ .has_ddrck = 1,
+ .has_mpddr_clk = 1,
+};
+
+static const struct of_device_id atmel_ramc_of_match[] = {
+ { .compatible = "atmel,at91rm9200-sdramc", .data = &at91rm9200_caps, },
+ { .compatible = "atmel,at91sam9260-sdramc", .data = &at91rm9200_caps, },
+ { .compatible = "atmel,at91sam9g45-ddramc", .data = &at91sam9g45_caps, },
+ { .compatible = "atmel,sama5d3-mpddramc", .data = &sama5d3_caps, },
+ {},
+};
+MODULE_DEVICE_TABLE(of, atmel_ramc_of_match);
+
+static int atmel_ramc_probe(struct platform_device *pdev)
+{
+ const struct of_device_id *match;
+ const struct at91_ramc_caps *caps;
+ struct clk *clk;
+
+ match = of_match_device(atmel_ramc_of_match, &pdev->dev);
+ caps = match->data;
+
+ if (caps->has_ddrck) {
+ clk = devm_clk_get(&pdev->dev, "ddrck");
+ if (IS_ERR(clk))
+ return PTR_ERR(clk);
+ clk_prepare_enable(clk);
+ }
+
+ if (caps->has_mpddr_clk) {
+ clk = devm_clk_get(&pdev->dev, "mpddr");
+ if (WARN_ON(IS_ERR(clk)))
+ return 0;
+ clk_prepare_enable(clk);
+ }
+
+ return 0;
+}
+
+static struct platform_driver atmel_ramc_driver = {
+ .probe = atmel_ramc_probe,
+ .driver = {
+ .name = "atmel-ramc",
+ .owner = THIS_MODULE,
+ .of_match_table = atmel_ramc_of_match,
+ },
+};
+
+static int __init atmel_ramc_init(void)
+{
+ return platform_driver_register(&atmel_ramc_driver);
+}
+module_init(atmel_ramc_init);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Alexandre Belloni <[email protected]>");
+MODULE_DESCRIPTION("Atmel (Multi-port DDR-)SDRAM Controller");
--
1.9.1

2014-07-07 15:26:40

by Alexandre Belloni

[permalink] [raw]
Subject: [PATCH 4/5] ARM: at91/dt: at91sam9: use ddrck in ramc

Make the ram controller driver take the ddrck clock for at91sam9n12 and
at91sam9x5. For at91sam9g45, use mck instead.

Signed-off-by: Alexandre Belloni <[email protected]>
---
arch/arm/boot/dts/at91sam9n12.dtsi | 2 ++
arch/arm/boot/dts/at91sam9x5.dtsi | 2 ++
2 files changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/at91sam9n12.dtsi b/arch/arm/boot/dts/at91sam9n12.dtsi
index 287795985e32..3d48255c4ed1 100644
--- a/arch/arm/boot/dts/at91sam9n12.dtsi
+++ b/arch/arm/boot/dts/at91sam9n12.dtsi
@@ -85,6 +85,8 @@
ramc0: ramc@ffffe800 {
compatible = "atmel,at91sam9g45-ddramc";
reg = <0xffffe800 0x200>;
+ clocks = <&ddrck>;
+ clock-names = "ddrck";
};

pmc: pmc@fffffc00 {
diff --git a/arch/arm/boot/dts/at91sam9x5.dtsi b/arch/arm/boot/dts/at91sam9x5.dtsi
index d6133f497207..74722853e667 100644
--- a/arch/arm/boot/dts/at91sam9x5.dtsi
+++ b/arch/arm/boot/dts/at91sam9x5.dtsi
@@ -93,6 +93,8 @@
ramc0: ramc@ffffe800 {
compatible = "atmel,at91sam9g45-ddramc";
reg = <0xffffe800 0x200>;
+ clocks = <&ddrck>;
+ clock-names = "ddrck";
};

pmc: pmc@fffffc00 {
--
1.9.1

2014-07-07 15:27:06

by Alexandre Belloni

[permalink] [raw]
Subject: [PATCH 3/5] ARM: at91/dt: sama5d3: define mpddr clock and ramc clocks

Define the available clock for mprddr and take both mpddr_clk and ddrck in the
ram controller driver.

Signed-off-by: Alexandre Belloni <[email protected]>
---
arch/arm/boot/dts/sama5d3.dtsi | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/sama5d3.dtsi b/arch/arm/boot/dts/sama5d3.dtsi
index e0b15a6e8897..3bd8db9e069b 100644
--- a/arch/arm/boot/dts/sama5d3.dtsi
+++ b/arch/arm/boot/dts/sama5d3.dtsi
@@ -402,8 +402,10 @@
};

ramc0: ramc@ffffea00 {
- compatible = "atmel,at91sam9g45-ddramc";
+ compatible = "atmel,sama5d3-mpddramc", "atmel,at91sam9g45-ddramc";
reg = <0xffffea00 0x200>;
+ clocks = <&ddrck>, <&mpddr_clk>;
+ clock-names = "ddrck", "mpddr";
};

dbgu: serial@ffffee00 {
@@ -1170,6 +1172,11 @@
#clock-cells = <0>;
reg = <48>;
};
+
+ mpddr_clk: mpddr_clk {
+ #clock-cells = <0>;
+ reg = <49>;
+ };
};
};

--
1.9.1

2014-07-07 15:46:49

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH 1/5] memory: add a driver for atmel ram controllers

On Mon, 7 Jul 2014 17:19:11 +0200
Alexandre Belloni <[email protected]> wrote:

> Atmel SoCs have one or multiple RAM controllers that need one or multiple clocks
> to run.
> This driver handle those clocks.
>

Actually this controller is an SDRAM controller which, depending on the
SoC, might support SDR SDRAMs, DDR SDRAMs or both.

This is just a nitpick, but if you don't mind I'd rather replace
references to RAMC by SDRAMC (ATMEL_RAMC -> ATMEL_SDRAMC) and just state
that in some cases (at least this is the case for the DDRSDRC available
in at91sam9g45 SoC) it supports both type of SDRAM (DDR and SDR).

The same goes for the source file name (atmel-ramc.c -> atmel-sdramc.c).

> Signed-off-by: Alexandre Belloni <[email protected]>
> ---
> .../devicetree/bindings/arm/atmel-at91.txt | 1 +
> drivers/memory/Kconfig | 8 ++
> drivers/memory/Makefile | 1 +
> drivers/memory/atmel-ramc.c | 96 ++++++++++++++++++++++
> 4 files changed, 106 insertions(+)
> create mode 100644 drivers/memory/atmel-ramc.c
>
> diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt
> index 16f60b41c147..54dc3aefb12a 100644
> --- a/Documentation/devicetree/bindings/arm/atmel-at91.txt
> +++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt
> @@ -61,6 +61,7 @@ RAMC SDRAM/DDR Controller required properties:
> - compatible: Should be "atmel,at91rm9200-sdramc",
> "atmel,at91sam9260-sdramc",
> "atmel,at91sam9g45-ddramc",
> + "atmel,sama5d3-mpddramc",
> - reg: Should contain registers location and length
> For at91sam9263 and at91sam9g45 you must specify 2 entries.

Shouldn't we move the documentation in
Documentation/devicetree/bindings/memory-controllers/ (though this
should be done in different patch).

>
> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> index c59e9c96e86d..e49b9caa1b30 100644
> --- a/drivers/memory/Kconfig
> +++ b/drivers/memory/Kconfig
> @@ -7,6 +7,14 @@ menuconfig MEMORY
>
> if MEMORY
>
> +config ATMEL_RAMC
> + bool "Atmel (Multi-port DDR-)SDRAM Controller"
> + default y
> + depends on ARCH_AT91 && OF
> + help
> + This driver is for Atmel SDRAM Controller or Atmel Multi-port
> + DDR-SDRAM Controller available on Atmel AT91SAM9 and SAMA5 SoCs
> +
> config TI_AEMIF
> tristate "Texas Instruments AEMIF driver"
> depends on (ARCH_DAVINCI || ARCH_KEYSTONE) && OF
> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> index 71160a2b7313..eb5d7716efa4 100644
> --- a/drivers/memory/Makefile
> +++ b/drivers/memory/Makefile
> @@ -5,6 +5,7 @@
> ifeq ($(CONFIG_DDR),y)
> obj-$(CONFIG_OF) += of_memory.o
> endif
> +obj-$(CONFIG_ATMEL_RAMC) += atmel-ramc.o
> obj-$(CONFIG_TI_AEMIF) += ti-aemif.o
> obj-$(CONFIG_TI_EMIF) += emif.o
> obj-$(CONFIG_FSL_IFC) += fsl_ifc.o
> diff --git a/drivers/memory/atmel-ramc.c b/drivers/memory/atmel-ramc.c
> new file mode 100644
> index 000000000000..1d12d3d01cbd
> --- /dev/null
> +++ b/drivers/memory/atmel-ramc.c
> @@ -0,0 +1,96 @@
> +/*
> + * Atmel (Multi-port DDR-)SDRAM Controller driver
> + *
> + * Copyright (C) 2014 Atmel
> + *
> + * 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 version 2 of the License.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
> +struct at91_ramc_caps {
> + bool has_ddrck;
> + bool has_mpddr_clk;
> +};
> +
> +static const struct at91_ramc_caps at91rm9200_caps = { };
> +
> +static const struct at91_ramc_caps at91sam9g45_caps = {
> + .has_ddrck = 1,
> + .has_mpddr_clk = 0,
> +};
> +
> +static const struct at91_ramc_caps sama5d3_caps = {
> + .has_ddrck = 1,
> + .has_mpddr_clk = 1,
> +};
> +
> +static const struct of_device_id atmel_ramc_of_match[] = {
> + { .compatible = "atmel,at91rm9200-sdramc", .data = &at91rm9200_caps, },
> + { .compatible = "atmel,at91sam9260-sdramc", .data = &at91rm9200_caps, },
> + { .compatible = "atmel,at91sam9g45-ddramc", .data = &at91sam9g45_caps, },
> + { .compatible = "atmel,sama5d3-mpddramc", .data = &sama5d3_caps, },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, atmel_ramc_of_match);
> +
> +static int atmel_ramc_probe(struct platform_device *pdev)
> +{
> + const struct of_device_id *match;
> + const struct at91_ramc_caps *caps;
> + struct clk *clk;
> +
> + match = of_match_device(atmel_ramc_of_match, &pdev->dev);
> + caps = match->data;
> +
> + if (caps->has_ddrck) {
> + clk = devm_clk_get(&pdev->dev, "ddrck");
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> + clk_prepare_enable(clk);
> + }
> +
> + if (caps->has_mpddr_clk) {
> + clk = devm_clk_get(&pdev->dev, "mpddr");
> + if (WARN_ON(IS_ERR(clk)))
> + return 0;
> + clk_prepare_enable(clk);
> + }
> +
> + return 0;
> +}
> +
> +static struct platform_driver atmel_ramc_driver = {
> + .probe = atmel_ramc_probe,
> + .driver = {
> + .name = "atmel-ramc",
> + .owner = THIS_MODULE,
> + .of_match_table = atmel_ramc_of_match,
> + },
> +};
> +
> +static int __init atmel_ramc_init(void)
> +{
> + return platform_driver_register(&atmel_ramc_driver);
> +}
> +module_init(atmel_ramc_init);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Alexandre Belloni <[email protected]>");
> +MODULE_DESCRIPTION("Atmel (Multi-port DDR-)SDRAM Controller");



--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

2014-07-07 17:03:24

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH 0/5] Add a driver for the atmel ram controller

Hello Alexandre,

On Mon, 7 Jul 2014 17:19:10 +0200
Alexandre Belloni <[email protected]> wrote:

> The atmel ram controller needs one or more clocks to work. For now the
> CLK_IGNORE_UNUSED flag is used on those clocks. This patch set introduce a
> driver that will take care of taking those clocks.
>
> The final goal is also to move the ioremap done in mach-at91 to that driver with
> a proper power management driver.
>
>Apart from the minor comments I made on patch 1, you have my

Acked-by: Boris BREZILLON <[email protected]>

That's great to see the dirty CLK_IGNORE_UNSUSED hack removed in favor
of this SDRAM controller driver approach.

Thanks,

Boris

--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

2014-07-07 17:56:33

by Alexandre Belloni

[permalink] [raw]
Subject: Re: [PATCH 1/5] memory: add a driver for atmel ram controllers

Hi,

On 07/07/2014 at 17:46:42 +0200, Boris Brezillon wrote :
> On Mon, 7 Jul 2014 17:19:11 +0200
> Alexandre Belloni <[email protected]> wrote:
>
> > Atmel SoCs have one or multiple RAM controllers that need one or multiple clocks
> > to run.
> > This driver handle those clocks.
> >
>
> Actually this controller is an SDRAM controller which, depending on the
> SoC, might support SDR SDRAMs, DDR SDRAMs or both.
>
> This is just a nitpick, but if you don't mind I'd rather replace
> references to RAMC by SDRAMC (ATMEL_RAMC -> ATMEL_SDRAMC) and just state
> that in some cases (at least this is the case for the DDRSDRC available
> in at91sam9g45 SoC) it supports both type of SDRAM (DDR and SDR).
>
> The same goes for the source file name (atmel-ramc.c -> atmel-sdramc.c).
>

will do.


> > diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt
> > index 16f60b41c147..54dc3aefb12a 100644
> > --- a/Documentation/devicetree/bindings/arm/atmel-at91.txt
> > +++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt
> > @@ -61,6 +61,7 @@ RAMC SDRAM/DDR Controller required properties:
> > - compatible: Should be "atmel,at91rm9200-sdramc",
> > "atmel,at91sam9260-sdramc",
> > "atmel,at91sam9g45-ddramc",
> > + "atmel,sama5d3-mpddramc",
> > - reg: Should contain registers location and length
> > For at91sam9263 and at91sam9g45 you must specify 2 entries.
>
> Shouldn't we move the documentation in
> Documentation/devicetree/bindings/memory-controllers/ (though this
> should be done in different patch).
>

I guess we should move it later, to avoid merge conflicts


--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Subject: Re: [PATCH 3/5] ARM: at91/dt: sama5d3: define mpddr clock and ramc clocks

On 17:19 Mon 07 Jul , Alexandre Belloni wrote:
>
> Define the available clock for mprddr and take both mpddr_clk and ddrck in the
> ram controller driver.
>
> Signed-off-by: Alexandre Belloni <[email protected]>
> ---
> arch/arm/boot/dts/sama5d3.dtsi | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/sama5d3.dtsi b/arch/arm/boot/dts/sama5d3.dtsi
> index e0b15a6e8897..3bd8db9e069b 100644
> --- a/arch/arm/boot/dts/sama5d3.dtsi
> +++ b/arch/arm/boot/dts/sama5d3.dtsi
> @@ -402,8 +402,10 @@
> };
>
> ramc0: ramc@ffffea00 {
> - compatible = "atmel,at91sam9g45-ddramc";
> + compatible = "atmel,sama5d3-mpddramc", "atmel,at91sam9g45-ddramc";
the sama5 ddr controler is not back compitble with 9g45 one the compatible is
wrong
> reg = <0xffffea00 0x200>;
> + clocks = <&ddrck>, <&mpddr_clk>;
> + clock-names = "ddrck", "mpddr";
> };
>
> dbgu: serial@ffffee00 {
> @@ -1170,6 +1172,11 @@
> #clock-cells = <0>;
> reg = <48>;
> };
> +
> + mpddr_clk: mpddr_clk {
> + #clock-cells = <0>;
> + reg = <49>;
> + };
> };
> };
>
> --
> 1.9.1
>

Subject: Re: [PATCH 1/5] memory: add a driver for atmel ram controllers

On 17:19 Mon 07 Jul , Alexandre Belloni wrote:
>
> Atmel SoCs have one or multiple RAM controllers that need one or multiple clocks
> to run.
> This driver handle those clocks.
>
> Signed-off-by: Alexandre Belloni <[email protected]>
> ---
> .../devicetree/bindings/arm/atmel-at91.txt | 1 +
> drivers/memory/Kconfig | 8 ++
> drivers/memory/Makefile | 1 +
> drivers/memory/atmel-ramc.c | 96 ++++++++++++++++++++++
> 4 files changed, 106 insertions(+)
> create mode 100644 drivers/memory/atmel-ramc.c
>
> diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt
> index 16f60b41c147..54dc3aefb12a 100644
> --- a/Documentation/devicetree/bindings/arm/atmel-at91.txt
> +++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt
> @@ -61,6 +61,7 @@ RAMC SDRAM/DDR Controller required properties:
> - compatible: Should be "atmel,at91rm9200-sdramc",
> "atmel,at91sam9260-sdramc",
> "atmel,at91sam9g45-ddramc",
> + "atmel,sama5d3-mpddramc",
> - reg: Should contain registers location and length
> For at91sam9263 and at91sam9g45 you must specify 2 entries.
>
> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> index c59e9c96e86d..e49b9caa1b30 100644
> --- a/drivers/memory/Kconfig
> +++ b/drivers/memory/Kconfig
> @@ -7,6 +7,14 @@ menuconfig MEMORY
>
> if MEMORY
>
> +config ATMEL_RAMC
> + bool "Atmel (Multi-port DDR-)SDRAM Controller"
> + default y
> + depends on ARCH_AT91 && OF
> + help
> + This driver is for Atmel SDRAM Controller or Atmel Multi-port
> + DDR-SDRAM Controller available on Atmel AT91SAM9 and SAMA5 SoCs
> +
> config TI_AEMIF
> tristate "Texas Instruments AEMIF driver"
> depends on (ARCH_DAVINCI || ARCH_KEYSTONE) && OF
> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> index 71160a2b7313..eb5d7716efa4 100644
> --- a/drivers/memory/Makefile
> +++ b/drivers/memory/Makefile
> @@ -5,6 +5,7 @@
> ifeq ($(CONFIG_DDR),y)
> obj-$(CONFIG_OF) += of_memory.o
> endif
> +obj-$(CONFIG_ATMEL_RAMC) += atmel-ramc.o
> obj-$(CONFIG_TI_AEMIF) += ti-aemif.o
> obj-$(CONFIG_TI_EMIF) += emif.o
> obj-$(CONFIG_FSL_IFC) += fsl_ifc.o
> diff --git a/drivers/memory/atmel-ramc.c b/drivers/memory/atmel-ramc.c
> new file mode 100644
> index 000000000000..1d12d3d01cbd
> --- /dev/null
> +++ b/drivers/memory/atmel-ramc.c
> @@ -0,0 +1,96 @@
> +/*
> + * Atmel (Multi-port DDR-)SDRAM Controller driver
> + *
> + * Copyright (C) 2014 Atmel
> + *
> + * 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 version 2 of the License.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
> +struct at91_ramc_caps {
> + bool has_ddrck;
> + bool has_mpddr_clk;
> +};
> +
> +static const struct at91_ramc_caps at91rm9200_caps = { };
> +
> +static const struct at91_ramc_caps at91sam9g45_caps = {
> + .has_ddrck = 1,
> + .has_mpddr_clk = 0,
> +};
> +
> +static const struct at91_ramc_caps sama5d3_caps = {
> + .has_ddrck = 1,
> + .has_mpddr_clk = 1,
> +};
> +
> +static const struct of_device_id atmel_ramc_of_match[] = {
> + { .compatible = "atmel,at91rm9200-sdramc", .data = &at91rm9200_caps, },
> + { .compatible = "atmel,at91sam9260-sdramc", .data = &at91rm9200_caps, },
> + { .compatible = "atmel,at91sam9g45-ddramc", .data = &at91sam9g45_caps, },
> + { .compatible = "atmel,sama5d3-mpddramc", .data = &sama5d3_caps, },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, atmel_ramc_of_match);
> +
> +static int atmel_ramc_probe(struct platform_device *pdev)
> +{
> + const struct of_device_id *match;
> + const struct at91_ramc_caps *caps;
> + struct clk *clk;
> +
> + match = of_match_device(atmel_ramc_of_match, &pdev->dev);
> + caps = match->data;
> +
> + if (caps->has_ddrck) {
> + clk = devm_clk_get(&pdev->dev, "ddrck");
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> + clk_prepare_enable(clk);
> + }
> +
> + if (caps->has_mpddr_clk) {
> + clk = devm_clk_get(&pdev->dev, "mpddr");
> + if (WARN_ON(IS_ERR(clk)))
> + return 0;
I don't like this warn_on this need to be an error
> + clk_prepare_enable(clk);
> + }
> +
> + return 0;
> +}
> +
> +static struct platform_driver atmel_ramc_driver = {
> + .probe = atmel_ramc_probe,
> + .driver = {
> + .name = "atmel-ramc",
> + .owner = THIS_MODULE,
> + .of_match_table = atmel_ramc_of_match,
> + },
> +};
> +
> +static int __init atmel_ramc_init(void)
> +{
> + return platform_driver_register(&atmel_ramc_driver);
> +}
> +module_init(atmel_ramc_init);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Alexandre Belloni <[email protected]>");
> +MODULE_DESCRIPTION("Atmel (Multi-port DDR-)SDRAM Controller");
> --
> 1.9.1
>

2014-07-07 19:44:10

by Alexandre Belloni

[permalink] [raw]
Subject: Re: [PATCH 1/5] memory: add a driver for atmel ram controllers

On 07/07/2014 at 20:33:40 +0200, Jean-Christophe PLAGNIOL-VILLARD wrote :
> > + if (caps->has_mpddr_clk) {
> > + clk = devm_clk_get(&pdev->dev, "mpddr");
> > + if (WARN_ON(IS_ERR(clk)))
> > + return 0;
> I don't like this warn_on this need to be an error

What would you prefer ? pr_err() and panic or BUG_ON ?. We can also
probably simply put a single pr_err(), anyway, the platform will stop
just before switching to userspace.


--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

2014-07-07 19:56:01

by Alexandre Belloni

[permalink] [raw]
Subject: Re: [PATCH 3/5] ARM: at91/dt: sama5d3: define mpddr clock and ramc clocks

On 07/07/2014 at 20:32:36 +0200, Jean-Christophe PLAGNIOL-VILLARD wrote :
> > - compatible = "atmel,at91sam9g45-ddramc";
> > + compatible = "atmel,sama5d3-mpddramc", "atmel,at91sam9g45-ddramc";
> the sama5 ddr controler is not back compitble with 9g45 one the compatible is
> wrong

Keeping atmel,at91sam9g45-ddramc allows to reuse the old code for the
iomap needed for the PM code without adding a new compatible in the
ramc_ids[] array.

--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

2014-07-08 07:55:34

by Maxime Ripard

[permalink] [raw]
Subject: Re: [PATCH 3/5] ARM: at91/dt: sama5d3: define mpddr clock and ramc clocks

Hi,

On Mon, Jul 07, 2014 at 09:55:56PM +0200, Alexandre Belloni wrote:
> On 07/07/2014 at 20:32:36 +0200, Jean-Christophe PLAGNIOL-VILLARD wrote :
> > > - compatible = "atmel,at91sam9g45-ddramc";
> > > + compatible = "atmel,sama5d3-mpddramc", "atmel,at91sam9g45-ddramc";
> > the sama5 ddr controler is not back compitble with 9g45 one the compatible is
> > wrong
>
> Keeping atmel,at91sam9g45-ddramc allows to reuse the old code for the
> iomap needed for the PM code without adding a new compatible in the
> ramc_ids[] array.

That looks like a pretty bad argument :)

If the two devices are not alike, they should have a different
compatible, it's as simple as that, and Linux should just deal with
it.

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


Attachments:
(No filename) (829.00 B)
signature.asc (819.00 B)
Digital signature
Download all attachments