[RFC] da850: DDR2/mDDR memory controller driver

[RFC] da850: DDR2/mDDR memory controller driver

[Bartosz Golaszewski]

This is a follow-up for the series[1] adding new bus and memory drivers
to better support the TI LCD controller on the da850-lcdk board.

The general consensus of the discussion that followed was that DT is
not the right tool for this kind of SoC performance tweaks.

In order to avoid committing to stable DT bindings, we only introduce
two common properties (compatible and reg) while the configuration
register values are hard-coded for each board (currently only lcdk).

I'm sending a single patch this time as RFC to get some reviews and
see it the approach is viewed as correct.

[1] https://lkml.org/lkml/2016/10/17/613

Bartosz Golaszewski (1):
  ARM: memory: da8xx-ddrctl: new driver

 .../memory-controllers/ti-da8xx-ddrctl.txt         |  20 +++
 drivers/memory/Kconfig                             |   8 +
 drivers/memory/Makefile                            |   1 +
 drivers/memory/da8xx-ddrctl.c                      | 187 +++++++++++++++++++++
 4 files changed, 216 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
 create mode 100644 drivers/memory/da8xx-ddrctl.c

-- 
2.9.3

[RFC] da850: DDR2/mDDR memory controller driver

[Bartosz Golaszewski]

This is a follow-up for the series[1] adding new bus and memory drivers
to better support the TI LCD controller on the da850-lcdk board.

The general consensus of the discussion that followed was that DT is
not the right tool for this kind of SoC performance tweaks.

In order to avoid committing to stable DT bindings, we only introduce
two common properties (compatible and reg) while the configuration
register values are hard-coded for each board (currently only lcdk).

I'm sending a single patch this time as RFC to get some reviews and
see it the approach is viewed as correct.

[1] https://lkml.org/lkml/2016/10/17/613

Bartosz Golaszewski (1):
  ARM: memory: da8xx-ddrctl: new driver

 .../memory-controllers/ti-da8xx-ddrctl.txt         |  20 +++
 drivers/memory/Kconfig                             |   8 +
 drivers/memory/Makefile                            |   1 +
 drivers/memory/da8xx-ddrctl.c                      | 187 +++++++++++++++++++++
 4 files changed, 216 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
 create mode 100644 drivers/memory/da8xx-ddrctl.c

-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC] da850: DDR2/mDDR memory controller driver

[Bartosz Golaszewski]

This is a follow-up for the series[1] adding new bus and memory drivers
to better support the TI LCD controller on the da850-lcdk board.

The general consensus of the discussion that followed was that DT is
not the right tool for this kind of SoC performance tweaks.

In order to avoid committing to stable DT bindings, we only introduce
two common properties (compatible and reg) while the configuration
register values are hard-coded for each board (currently only lcdk).

I'm sending a single patch this time as RFC to get some reviews and
see it the approach is viewed as correct.

[1] https://lkml.org/lkml/2016/10/17/613

Bartosz Golaszewski (1):
  ARM: memory: da8xx-ddrctl: new driver

 .../memory-controllers/ti-da8xx-ddrctl.txt         |  20 +++
 drivers/memory/Kconfig                             |   8 +
 drivers/memory/Makefile                            |   1 +
 drivers/memory/da8xx-ddrctl.c                      | 187 +++++++++++++++++++++
 4 files changed, 216 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
 create mode 100644 drivers/memory/da8xx-ddrctl.c

-- 
2.9.3

[RFC] ARM: memory: da8xx-ddrctl: new driver

[Bartosz Golaszewski]

Create a new driver for the da8xx DDR2/mDDR controller and implement
support for writing to the Peripheral Bus Burst Priority Register.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 .../memory-controllers/ti-da8xx-ddrctl.txt         |  20 +++
 drivers/memory/Kconfig                             |   8 +
 drivers/memory/Makefile                            |   1 +
 drivers/memory/da8xx-ddrctl.c                      | 187 +++++++++++++++++++++
 4 files changed, 216 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
 create mode 100644 drivers/memory/da8xx-ddrctl.c

diff --git a/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
new file mode 100644
index 0000000..f0eda59
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
@@ -0,0 +1,20 @@
+* Device tree bindings for Texas Instruments da8xx DDR2/mDDR memory controller
+
+The DDR2/mDDR memory controller present on Texas Instruments da8xx SoCs memory
+maps a set of registers which allow to tweak the controller's behavior.
+
+Documentation:
+OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf
+
+Required properties:
+
+- compatible:		"ti,da850-ddrctl" - for da850 SoC based boards
+- reg:			a tuple containing the base address of the memory
+			controller and the size of the memory area to map
+
+Example for da850 shown below.
+
+ddrctl {
+	compatible = "ti,da850-ddrctl";
+	reg = <0xB0000000 0x100>;
+};
diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index 4b4c0c3..ec80e35 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -134,6 +134,14 @@ config MTK_SMI
 	  mainly help enable/disable iommu and control the power domain and
 	  clocks for each local arbiter.
 
+config DA8XX_DDRCTL
+	bool "Texas Instruments da8xx DDR2/mDDR driver"
+	depends on ARCH_DAVINCI_DA8XX
+	help
+	  This driver is for the DDR2/mDDR Memory Controller present on
+	  Texas Instruments da8xx SoCs. It's used to tweak various memory
+	  controller configuration options.
+
 source "drivers/memory/samsung/Kconfig"
 source "drivers/memory/tegra/Kconfig"
 
diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index b20ae38..e88097fb 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_MVEBU_DEVBUS)	+= mvebu-devbus.o
 obj-$(CONFIG_TEGRA20_MC)	+= tegra20-mc.o
 obj-$(CONFIG_JZ4780_NEMC)	+= jz4780-nemc.o
 obj-$(CONFIG_MTK_SMI)		+= mtk-smi.o
+obj-$(CONFIG_DA8XX_DDRCTL)	+= da8xx-ddrctl.o
 
 obj-$(CONFIG_SAMSUNG_MC)	+= samsung/
 obj-$(CONFIG_TEGRA_MC)		+= tegra/
diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
new file mode 100644
index 0000000..756a6f3
--- /dev/null
+++ b/drivers/memory/da8xx-ddrctl.c
@@ -0,0 +1,187 @@
+/*
+ * TI da8xx DDR2/mDDR controller driver
+ *
+ * Copyright (C) 2016 BayLibre SAS
+ *
+ * Author:
+ *   Bartosz Golaszewski <bgolaszewski@baylibre.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+
+struct da8xx_ddrctl_config_knob {
+	const char *name;
+	u32 reg;
+	u32 mask;
+	u32 offset;
+};
+
+static const struct da8xx_ddrctl_config_knob da8xx_ddrctl_knobs[] = {
+	{
+		.name = "da850-pbbpr",
+		.reg = 0x20,
+		.mask = 0xffffff00,
+		.offset = 0,
+	},
+};
+
+struct da8xx_ddrctl_setting {
+	const char *name;
+	u32 val;
+};
+
+struct da8xx_ddrctl_board_settings {
+	const char *board;
+	const struct da8xx_ddrctl_setting *settings;
+};
+
+static const struct da8xx_ddrctl_setting da850_lcdk_ddrctl_settings[] = {
+	{
+		.name = "da850-pbbpr",
+		.val = 0x20,
+	},
+	{ }
+};
+
+static const struct da8xx_ddrctl_board_settings da8xx_ddrctl_board_confs[] = {
+	{
+		.board = "ti,da850-lcdk",
+		.settings = da850_lcdk_ddrctl_settings,
+	},
+};
+
+static const struct da8xx_ddrctl_config_knob *
+da8xx_ddrctl_match_knob(const struct da8xx_ddrctl_setting *setting)
+{
+	const struct da8xx_ddrctl_config_knob *knob;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(da8xx_ddrctl_knobs); i++) {
+		knob = &da8xx_ddrctl_knobs[i];
+
+		if (strcmp(knob->name, setting->name) == 0) {
+			return knob;
+		}
+	}
+
+	return NULL;
+}
+
+static const struct da8xx_ddrctl_setting *
+da8xx_ddrctl_match_board(const char *board)
+{
+	const struct da8xx_ddrctl_board_settings *board_settings;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(da8xx_ddrctl_board_confs); i++) {
+		board_settings = &da8xx_ddrctl_board_confs[0];
+
+		if (strcmp(board, board_settings->board) == 0)
+			return board_settings->settings;
+	}
+
+	return NULL;
+}
+
+static int da8xx_ddrctl_probe(struct platform_device *pdev)
+{
+	const struct da8xx_ddrctl_config_knob *knob;
+	const struct da8xx_ddrctl_setting *setting;
+	u32 regprop[2], base, memsize, reg;
+	struct device_node *node, *parent;
+	void __iomem *ddrctl;
+	const char *board;
+	struct device *dev;
+	int ret;
+
+	dev = &pdev->dev;
+	node = dev->of_node;
+
+	/* Find the board name. */
+	for (parent = node;
+	     !of_node_is_root(parent);
+	     parent = of_get_parent(parent));
+
+	ret = of_property_read_string(parent, "compatible", &board);
+	if (ret) {
+		dev_err(dev, "unable to read the soc model\n");
+		return ret;
+	}
+
+	/* Check if we have settings for this board. */
+	setting = da8xx_ddrctl_match_board(board);
+	if (!setting) {
+		dev_err(dev, "no settings for board '%s'\n", board);
+		return -EINVAL;
+	}
+
+	/* Figure out how to map the memory for the controller. */
+	ret = of_property_read_u32_array(node, "reg", regprop, 2);
+	if (ret) {
+		dev_err(dev, "unable to parse 'reg' property\n");
+		return ret;
+	}
+
+	base = regprop[0];
+	memsize = regprop[1];
+
+	ddrctl = ioremap(base, memsize);
+	if (!ddrctl) {
+		dev_err(dev, "unable to map memory controller registers\n");
+		return -EIO;
+	}
+
+	for (; setting->name; setting++) {
+		knob = da8xx_ddrctl_match_knob(setting);
+		if (!knob) {
+			dev_warn(dev,
+				 "no such config option: %s\n", setting->name);
+			continue;
+		}
+
+		if (knob->reg > (memsize - sizeof(u32))) {
+			dev_warn(dev,
+				 "register offset of '%s' exceeds mapped memory size\n",
+				 knob->name);
+			continue;
+		}
+
+		reg = __raw_readl(ddrctl + knob->reg);
+		reg &= knob->mask;
+		reg |= setting->val << knob->offset;
+
+		dev_dbg(dev, "writing 0x%08x to %s\n", reg, setting->name);
+
+		__raw_writel(reg, ddrctl + knob->reg);
+	}
+
+	iounmap(ddrctl);
+
+	return 0;
+}
+
+static const struct of_device_id da8xx_ddrctl_of_match[] = {
+	{ .compatible = "ti,da850-ddrctl", },
+	{ },
+};
+
+static struct platform_driver da8xx_ddrctl_driver = {
+	.probe = da8xx_ddrctl_probe,
+	.driver = {
+		.name = "da8xx-ddrctl",
+		.of_match_table = da8xx_ddrctl_of_match,
+	},
+};
+module_platform_driver(da8xx_ddrctl_driver);
+
+MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
+MODULE_DESCRIPTION("TI da8xx DDR2/mDDR controller driver");
+MODULE_LICENSE("GPL v2");
-- 
2.9.3

[RFC] ARM: memory: da8xx-ddrctl: new driver

[Bartosz Golaszewski]

Create a new driver for the da8xx DDR2/mDDR controller and implement
support for writing to the Peripheral Bus Burst Priority Register.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 .../memory-controllers/ti-da8xx-ddrctl.txt         |  20 +++
 drivers/memory/Kconfig                             |   8 +
 drivers/memory/Makefile                            |   1 +
 drivers/memory/da8xx-ddrctl.c                      | 187 +++++++++++++++++++++
 4 files changed, 216 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
 create mode 100644 drivers/memory/da8xx-ddrctl.c

diff --git a/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
new file mode 100644
index 0000000..f0eda59
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
@@ -0,0 +1,20 @@
+* Device tree bindings for Texas Instruments da8xx DDR2/mDDR memory controller
+
+The DDR2/mDDR memory controller present on Texas Instruments da8xx SoCs memory
+maps a set of registers which allow to tweak the controller's behavior.
+
+Documentation:
+OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf
+
+Required properties:
+
+- compatible:		"ti,da850-ddrctl" - for da850 SoC based boards
+- reg:			a tuple containing the base address of the memory
+			controller and the size of the memory area to map
+
+Example for da850 shown below.
+
+ddrctl {
+	compatible = "ti,da850-ddrctl";
+	reg = <0xB0000000 0x100>;
+};
diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index 4b4c0c3..ec80e35 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -134,6 +134,14 @@ config MTK_SMI
 	  mainly help enable/disable iommu and control the power domain and
 	  clocks for each local arbiter.
 
+config DA8XX_DDRCTL
+	bool "Texas Instruments da8xx DDR2/mDDR driver"
+	depends on ARCH_DAVINCI_DA8XX
+	help
+	  This driver is for the DDR2/mDDR Memory Controller present on
+	  Texas Instruments da8xx SoCs. It's used to tweak various memory
+	  controller configuration options.
+
 source "drivers/memory/samsung/Kconfig"
 source "drivers/memory/tegra/Kconfig"
 
diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index b20ae38..e88097fb 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_MVEBU_DEVBUS)	+= mvebu-devbus.o
 obj-$(CONFIG_TEGRA20_MC)	+= tegra20-mc.o
 obj-$(CONFIG_JZ4780_NEMC)	+= jz4780-nemc.o
 obj-$(CONFIG_MTK_SMI)		+= mtk-smi.o
+obj-$(CONFIG_DA8XX_DDRCTL)	+= da8xx-ddrctl.o
 
 obj-$(CONFIG_SAMSUNG_MC)	+= samsung/
 obj-$(CONFIG_TEGRA_MC)		+= tegra/
diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
new file mode 100644
index 0000000..756a6f3
--- /dev/null
+++ b/drivers/memory/da8xx-ddrctl.c
@@ -0,0 +1,187 @@
+/*
+ * TI da8xx DDR2/mDDR controller driver
+ *
+ * Copyright (C) 2016 BayLibre SAS
+ *
+ * Author:
+ *   Bartosz Golaszewski <bgolaszewski@baylibre.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+
+struct da8xx_ddrctl_config_knob {
+	const char *name;
+	u32 reg;
+	u32 mask;
+	u32 offset;
+};
+
+static const struct da8xx_ddrctl_config_knob da8xx_ddrctl_knobs[] = {
+	{
+		.name = "da850-pbbpr",
+		.reg = 0x20,
+		.mask = 0xffffff00,
+		.offset = 0,
+	},
+};
+
+struct da8xx_ddrctl_setting {
+	const char *name;
+	u32 val;
+};
+
+struct da8xx_ddrctl_board_settings {
+	const char *board;
+	const struct da8xx_ddrctl_setting *settings;
+};
+
+static const struct da8xx_ddrctl_setting da850_lcdk_ddrctl_settings[] = {
+	{
+		.name = "da850-pbbpr",
+		.val = 0x20,
+	},
+	{ }
+};
+
+static const struct da8xx_ddrctl_board_settings da8xx_ddrctl_board_confs[] = {
+	{
+		.board = "ti,da850-lcdk",
+		.settings = da850_lcdk_ddrctl_settings,
+	},
+};
+
+static const struct da8xx_ddrctl_config_knob *
+da8xx_ddrctl_match_knob(const struct da8xx_ddrctl_setting *setting)
+{
+	const struct da8xx_ddrctl_config_knob *knob;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(da8xx_ddrctl_knobs); i++) {
+		knob = &da8xx_ddrctl_knobs[i];
+
+		if (strcmp(knob->name, setting->name) == 0) {
+			return knob;
+		}
+	}
+
+	return NULL;
+}
+
+static const struct da8xx_ddrctl_setting *
+da8xx_ddrctl_match_board(const char *board)
+{
+	const struct da8xx_ddrctl_board_settings *board_settings;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(da8xx_ddrctl_board_confs); i++) {
+		board_settings = &da8xx_ddrctl_board_confs[0];
+
+		if (strcmp(board, board_settings->board) == 0)
+			return board_settings->settings;
+	}
+
+	return NULL;
+}
+
+static int da8xx_ddrctl_probe(struct platform_device *pdev)
+{
+	const struct da8xx_ddrctl_config_knob *knob;
+	const struct da8xx_ddrctl_setting *setting;
+	u32 regprop[2], base, memsize, reg;
+	struct device_node *node, *parent;
+	void __iomem *ddrctl;
+	const char *board;
+	struct device *dev;
+	int ret;
+
+	dev = &pdev->dev;
+	node = dev->of_node;
+
+	/* Find the board name. */
+	for (parent = node;
+	     !of_node_is_root(parent);
+	     parent = of_get_parent(parent));
+
+	ret = of_property_read_string(parent, "compatible", &board);
+	if (ret) {
+		dev_err(dev, "unable to read the soc model\n");
+		return ret;
+	}
+
+	/* Check if we have settings for this board. */
+	setting = da8xx_ddrctl_match_board(board);
+	if (!setting) {
+		dev_err(dev, "no settings for board '%s'\n", board);
+		return -EINVAL;
+	}
+
+	/* Figure out how to map the memory for the controller. */
+	ret = of_property_read_u32_array(node, "reg", regprop, 2);
+	if (ret) {
+		dev_err(dev, "unable to parse 'reg' property\n");
+		return ret;
+	}
+
+	base = regprop[0];
+	memsize = regprop[1];
+
+	ddrctl = ioremap(base, memsize);
+	if (!ddrctl) {
+		dev_err(dev, "unable to map memory controller registers\n");
+		return -EIO;
+	}
+
+	for (; setting->name; setting++) {
+		knob = da8xx_ddrctl_match_knob(setting);
+		if (!knob) {
+			dev_warn(dev,
+				 "no such config option: %s\n", setting->name);
+			continue;
+		}
+
+		if (knob->reg > (memsize - sizeof(u32))) {
+			dev_warn(dev,
+				 "register offset of '%s' exceeds mapped memory size\n",
+				 knob->name);
+			continue;
+		}
+
+		reg = __raw_readl(ddrctl + knob->reg);
+		reg &= knob->mask;
+		reg |= setting->val << knob->offset;
+
+		dev_dbg(dev, "writing 0x%08x to %s\n", reg, setting->name);
+
+		__raw_writel(reg, ddrctl + knob->reg);
+	}
+
+	iounmap(ddrctl);
+
+	return 0;
+}
+
+static const struct of_device_id da8xx_ddrctl_of_match[] = {
+	{ .compatible = "ti,da850-ddrctl", },
+	{ },
+};
+
+static struct platform_driver da8xx_ddrctl_driver = {
+	.probe = da8xx_ddrctl_probe,
+	.driver = {
+		.name = "da8xx-ddrctl",
+		.of_match_table = da8xx_ddrctl_of_match,
+	},
+};
+module_platform_driver(da8xx_ddrctl_driver);
+
+MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
+MODULE_DESCRIPTION("TI da8xx DDR2/mDDR controller driver");
+MODULE_LICENSE("GPL v2");
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC] ARM: memory: da8xx-ddrctl: new driver

[Bartosz Golaszewski]

Create a new driver for the da8xx DDR2/mDDR controller and implement
support for writing to the Peripheral Bus Burst Priority Register.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 .../memory-controllers/ti-da8xx-ddrctl.txt         |  20 +++
 drivers/memory/Kconfig                             |   8 +
 drivers/memory/Makefile                            |   1 +
 drivers/memory/da8xx-ddrctl.c                      | 187 +++++++++++++++++++++
 4 files changed, 216 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
 create mode 100644 drivers/memory/da8xx-ddrctl.c

diff --git a/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
new file mode 100644
index 0000000..f0eda59
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
@@ -0,0 +1,20 @@
+* Device tree bindings for Texas Instruments da8xx DDR2/mDDR memory controller
+
+The DDR2/mDDR memory controller present on Texas Instruments da8xx SoCs memory
+maps a set of registers which allow to tweak the controller's behavior.
+
+Documentation:
+OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf
+
+Required properties:
+
+- compatible:		"ti,da850-ddrctl" - for da850 SoC based boards
+- reg:			a tuple containing the base address of the memory
+			controller and the size of the memory area to map
+
+Example for da850 shown below.
+
+ddrctl {
+	compatible = "ti,da850-ddrctl";
+	reg = <0xB0000000 0x100>;
+};
diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index 4b4c0c3..ec80e35 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -134,6 +134,14 @@ config MTK_SMI
 	  mainly help enable/disable iommu and control the power domain and
 	  clocks for each local arbiter.
 
+config DA8XX_DDRCTL
+	bool "Texas Instruments da8xx DDR2/mDDR driver"
+	depends on ARCH_DAVINCI_DA8XX
+	help
+	  This driver is for the DDR2/mDDR Memory Controller present on
+	  Texas Instruments da8xx SoCs. It's used to tweak various memory
+	  controller configuration options.
+
 source "drivers/memory/samsung/Kconfig"
 source "drivers/memory/tegra/Kconfig"
 
diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index b20ae38..e88097fb 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_MVEBU_DEVBUS)	+= mvebu-devbus.o
 obj-$(CONFIG_TEGRA20_MC)	+= tegra20-mc.o
 obj-$(CONFIG_JZ4780_NEMC)	+= jz4780-nemc.o
 obj-$(CONFIG_MTK_SMI)		+= mtk-smi.o
+obj-$(CONFIG_DA8XX_DDRCTL)	+= da8xx-ddrctl.o
 
 obj-$(CONFIG_SAMSUNG_MC)	+= samsung/
 obj-$(CONFIG_TEGRA_MC)		+= tegra/
diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
new file mode 100644
index 0000000..756a6f3
--- /dev/null
+++ b/drivers/memory/da8xx-ddrctl.c
@@ -0,0 +1,187 @@
+/*
+ * TI da8xx DDR2/mDDR controller driver
+ *
+ * Copyright (C) 2016 BayLibre SAS
+ *
+ * Author:
+ *   Bartosz Golaszewski <bgolaszewski@baylibre.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+
+struct da8xx_ddrctl_config_knob {
+	const char *name;
+	u32 reg;
+	u32 mask;
+	u32 offset;
+};
+
+static const struct da8xx_ddrctl_config_knob da8xx_ddrctl_knobs[] = {
+	{
+		.name = "da850-pbbpr",
+		.reg = 0x20,
+		.mask = 0xffffff00,
+		.offset = 0,
+	},
+};
+
+struct da8xx_ddrctl_setting {
+	const char *name;
+	u32 val;
+};
+
+struct da8xx_ddrctl_board_settings {
+	const char *board;
+	const struct da8xx_ddrctl_setting *settings;
+};
+
+static const struct da8xx_ddrctl_setting da850_lcdk_ddrctl_settings[] = {
+	{
+		.name = "da850-pbbpr",
+		.val = 0x20,
+	},
+	{ }
+};
+
+static const struct da8xx_ddrctl_board_settings da8xx_ddrctl_board_confs[] = {
+	{
+		.board = "ti,da850-lcdk",
+		.settings = da850_lcdk_ddrctl_settings,
+	},
+};
+
+static const struct da8xx_ddrctl_config_knob *
+da8xx_ddrctl_match_knob(const struct da8xx_ddrctl_setting *setting)
+{
+	const struct da8xx_ddrctl_config_knob *knob;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(da8xx_ddrctl_knobs); i++) {
+		knob = &da8xx_ddrctl_knobs[i];
+
+		if (strcmp(knob->name, setting->name) == 0) {
+			return knob;
+		}
+	}
+
+	return NULL;
+}
+
+static const struct da8xx_ddrctl_setting *
+da8xx_ddrctl_match_board(const char *board)
+{
+	const struct da8xx_ddrctl_board_settings *board_settings;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(da8xx_ddrctl_board_confs); i++) {
+		board_settings = &da8xx_ddrctl_board_confs[0];
+
+		if (strcmp(board, board_settings->board) == 0)
+			return board_settings->settings;
+	}
+
+	return NULL;
+}
+
+static int da8xx_ddrctl_probe(struct platform_device *pdev)
+{
+	const struct da8xx_ddrctl_config_knob *knob;
+	const struct da8xx_ddrctl_setting *setting;
+	u32 regprop[2], base, memsize, reg;
+	struct device_node *node, *parent;
+	void __iomem *ddrctl;
+	const char *board;
+	struct device *dev;
+	int ret;
+
+	dev = &pdev->dev;
+	node = dev->of_node;
+
+	/* Find the board name. */
+	for (parent = node;
+	     !of_node_is_root(parent);
+	     parent = of_get_parent(parent));
+
+	ret = of_property_read_string(parent, "compatible", &board);
+	if (ret) {
+		dev_err(dev, "unable to read the soc model\n");
+		return ret;
+	}
+
+	/* Check if we have settings for this board. */
+	setting = da8xx_ddrctl_match_board(board);
+	if (!setting) {
+		dev_err(dev, "no settings for board '%s'\n", board);
+		return -EINVAL;
+	}
+
+	/* Figure out how to map the memory for the controller. */
+	ret = of_property_read_u32_array(node, "reg", regprop, 2);
+	if (ret) {
+		dev_err(dev, "unable to parse 'reg' property\n");
+		return ret;
+	}
+
+	base = regprop[0];
+	memsize = regprop[1];
+
+	ddrctl = ioremap(base, memsize);
+	if (!ddrctl) {
+		dev_err(dev, "unable to map memory controller registers\n");
+		return -EIO;
+	}
+
+	for (; setting->name; setting++) {
+		knob = da8xx_ddrctl_match_knob(setting);
+		if (!knob) {
+			dev_warn(dev,
+				 "no such config option: %s\n", setting->name);
+			continue;
+		}
+
+		if (knob->reg > (memsize - sizeof(u32))) {
+			dev_warn(dev,
+				 "register offset of '%s' exceeds mapped memory size\n",
+				 knob->name);
+			continue;
+		}
+
+		reg = __raw_readl(ddrctl + knob->reg);
+		reg &= knob->mask;
+		reg |= setting->val << knob->offset;
+
+		dev_dbg(dev, "writing 0x%08x to %s\n", reg, setting->name);
+
+		__raw_writel(reg, ddrctl + knob->reg);
+	}
+
+	iounmap(ddrctl);
+
+	return 0;
+}
+
+static const struct of_device_id da8xx_ddrctl_of_match[] = {
+	{ .compatible = "ti,da850-ddrctl", },
+	{ },
+};
+
+static struct platform_driver da8xx_ddrctl_driver = {
+	.probe = da8xx_ddrctl_probe,
+	.driver = {
+		.name = "da8xx-ddrctl",
+		.of_match_table = da8xx_ddrctl_of_match,
+	},
+};
+module_platform_driver(da8xx_ddrctl_driver);
+
+MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
+MODULE_DESCRIPTION("TI da8xx DDR2/mDDR controller driver");
+MODULE_LICENSE("GPL v2");
-- 
2.9.3

Re: [RFC] ARM: memory: da8xx-ddrctl: new driver

[Mark Rutland]

On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote:
> Create a new driver for the da8xx DDR2/mDDR controller and implement
> support for writing to the Peripheral Bus Burst Priority Register.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  .../memory-controllers/ti-da8xx-ddrctl.txt         |  20 +++
>  drivers/memory/Kconfig                             |   8 +
>  drivers/memory/Makefile                            |   1 +
>  drivers/memory/da8xx-ddrctl.c                      | 187 +++++++++++++++++++++
>  4 files changed, 216 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>  create mode 100644 drivers/memory/da8xx-ddrctl.c
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
> new file mode 100644
> index 0000000..f0eda59
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
> @@ -0,0 +1,20 @@
> +* Device tree bindings for Texas Instruments da8xx DDR2/mDDR memory controller
> +
> +The DDR2/mDDR memory controller present on Texas Instruments da8xx SoCs memory
> +maps a set of registers which allow to tweak the controller's behavior.

This is a description of the *driver*. The device itself doesn't map
some registers, it features them. Please descrive the *device*.

> +
> +Documentation:
> +OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf
> +
> +Required properties:
> +
> +- compatible:		"ti,da850-ddrctl" - for da850 SoC based boards

Perhaps:

	"ti,da850-ddr-controller"

> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
> +{
> +	const struct da8xx_ddrctl_config_knob *knob;
> +	const struct da8xx_ddrctl_setting *setting;
> +	u32 regprop[2], base, memsize, reg;
> +	struct device_node *node, *parent;
> +	void __iomem *ddrctl;
> +	const char *board;
> +	struct device *dev;
> +	int ret;
> +
> +	dev = &pdev->dev;
> +	node = dev->of_node;
> +
> +	/* Find the board name. */
> +	for (parent = node;
> +	     !of_node_is_root(parent);
> +	     parent = of_get_parent(parent));
> +
> +	ret = of_property_read_string(parent, "compatible", &board);
> +	if (ret) {
> +		dev_err(dev, "unable to read the soc model\n");
> +		return ret;
> +	}

I can see that you want to expose sysfs knobs for this, but is it really
necessary to match boards like this? It's very fragile, and commits us
to maintaining a database of board data (i.e. a board file).

I am very much not keen on that.

> +
> +	/* Check if we have settings for this board. */
> +	setting = da8xx_ddrctl_match_board(board);
> +	if (!setting) {
> +		dev_err(dev, "no settings for board '%s'\n", board);
> +		return -EINVAL;
> +	}

What's wrong with of_machine_is_compatible?

> +
> +	/* Figure out how to map the memory for the controller. */
> +	ret = of_property_read_u32_array(node, "reg", regprop, 2);
> +	if (ret) {
> +		dev_err(dev, "unable to parse 'reg' property\n");
> +		return ret;
> +	}
> +
> +	base = regprop[0];
> +	memsize = regprop[1];
> +
> +	ddrctl = ioremap(base, memsize);

NAK. Use the proper accessors for handling reg entries.

Thanks,
Mark.

Re: [RFC] ARM: memory: da8xx-ddrctl: new driver

[Mark Rutland]

On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote:
> Create a new driver for the da8xx DDR2/mDDR controller and implement
> support for writing to the Peripheral Bus Burst Priority Register.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> ---
>  .../memory-controllers/ti-da8xx-ddrctl.txt         |  20 +++
>  drivers/memory/Kconfig                             |   8 +
>  drivers/memory/Makefile                            |   1 +
>  drivers/memory/da8xx-ddrctl.c                      | 187 +++++++++++++++++++++
>  4 files changed, 216 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>  create mode 100644 drivers/memory/da8xx-ddrctl.c
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
> new file mode 100644
> index 0000000..f0eda59
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
> @@ -0,0 +1,20 @@
> +* Device tree bindings for Texas Instruments da8xx DDR2/mDDR memory controller
> +
> +The DDR2/mDDR memory controller present on Texas Instruments da8xx SoCs memory
> +maps a set of registers which allow to tweak the controller's behavior.

This is a description of the *driver*. The device itself doesn't map
some registers, it features them. Please descrive the *device*.

> +
> +Documentation:
> +OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf
> +
> +Required properties:
> +
> +- compatible:		"ti,da850-ddrctl" - for da850 SoC based boards

Perhaps:

	"ti,da850-ddr-controller"

> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
> +{
> +	const struct da8xx_ddrctl_config_knob *knob;
> +	const struct da8xx_ddrctl_setting *setting;
> +	u32 regprop[2], base, memsize, reg;
> +	struct device_node *node, *parent;
> +	void __iomem *ddrctl;
> +	const char *board;
> +	struct device *dev;
> +	int ret;
> +
> +	dev = &pdev->dev;
> +	node = dev->of_node;
> +
> +	/* Find the board name. */
> +	for (parent = node;
> +	     !of_node_is_root(parent);
> +	     parent = of_get_parent(parent));
> +
> +	ret = of_property_read_string(parent, "compatible", &board);
> +	if (ret) {
> +		dev_err(dev, "unable to read the soc model\n");
> +		return ret;
> +	}

I can see that you want to expose sysfs knobs for this, but is it really
necessary to match boards like this? It's very fragile, and commits us
to maintaining a database of board data (i.e. a board file).

I am very much not keen on that.

> +
> +	/* Check if we have settings for this board. */
> +	setting = da8xx_ddrctl_match_board(board);
> +	if (!setting) {
> +		dev_err(dev, "no settings for board '%s'\n", board);
> +		return -EINVAL;
> +	}

What's wrong with of_machine_is_compatible?

> +
> +	/* Figure out how to map the memory for the controller. */
> +	ret = of_property_read_u32_array(node, "reg", regprop, 2);
> +	if (ret) {
> +		dev_err(dev, "unable to parse 'reg' property\n");
> +		return ret;
> +	}
> +
> +	base = regprop[0];
> +	memsize = regprop[1];
> +
> +	ddrctl = ioremap(base, memsize);

NAK. Use the proper accessors for handling reg entries.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] ARM: memory: da8xx-ddrctl: new driver

[Mark Rutland]

On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote:
> Create a new driver for the da8xx DDR2/mDDR controller and implement
> support for writing to the Peripheral Bus Burst Priority Register.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  .../memory-controllers/ti-da8xx-ddrctl.txt         |  20 +++
>  drivers/memory/Kconfig                             |   8 +
>  drivers/memory/Makefile                            |   1 +
>  drivers/memory/da8xx-ddrctl.c                      | 187 +++++++++++++++++++++
>  4 files changed, 216 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>  create mode 100644 drivers/memory/da8xx-ddrctl.c
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
> new file mode 100644
> index 0000000..f0eda59
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
> @@ -0,0 +1,20 @@
> +* Device tree bindings for Texas Instruments da8xx DDR2/mDDR memory controller
> +
> +The DDR2/mDDR memory controller present on Texas Instruments da8xx SoCs memory
> +maps a set of registers which allow to tweak the controller's behavior.

This is a description of the *driver*. The device itself doesn't map
some registers, it features them. Please descrive the *device*.

> +
> +Documentation:
> +OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf
> +
> +Required properties:
> +
> +- compatible:		"ti,da850-ddrctl" - for da850 SoC based boards

Perhaps:

	"ti,da850-ddr-controller"

> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
> +{
> +	const struct da8xx_ddrctl_config_knob *knob;
> +	const struct da8xx_ddrctl_setting *setting;
> +	u32 regprop[2], base, memsize, reg;
> +	struct device_node *node, *parent;
> +	void __iomem *ddrctl;
> +	const char *board;
> +	struct device *dev;
> +	int ret;
> +
> +	dev = &pdev->dev;
> +	node = dev->of_node;
> +
> +	/* Find the board name. */
> +	for (parent = node;
> +	     !of_node_is_root(parent);
> +	     parent = of_get_parent(parent));
> +
> +	ret = of_property_read_string(parent, "compatible", &board);
> +	if (ret) {
> +		dev_err(dev, "unable to read the soc model\n");
> +		return ret;
> +	}

I can see that you want to expose sysfs knobs for this, but is it really
necessary to match boards like this? It's very fragile, and commits us
to maintaining a database of board data (i.e. a board file).

I am very much not keen on that.

> +
> +	/* Check if we have settings for this board. */
> +	setting = da8xx_ddrctl_match_board(board);
> +	if (!setting) {
> +		dev_err(dev, "no settings for board '%s'\n", board);
> +		return -EINVAL;
> +	}

What's wrong with of_machine_is_compatible?

> +
> +	/* Figure out how to map the memory for the controller. */
> +	ret = of_property_read_u32_array(node, "reg", regprop, 2);
> +	if (ret) {
> +		dev_err(dev, "unable to parse 'reg' property\n");
> +		return ret;
> +	}
> +
> +	base = regprop[0];
> +	memsize = regprop[1];
> +
> +	ddrctl = ioremap(base, memsize);

NAK. Use the proper accessors for handling reg entries.

Thanks,
Mark.

Re: [RFC] ARM: memory: da8xx-ddrctl: new driver

[Kevin Hilman]

Hi Mark,

Mark Rutland <mark.rutland@arm.com> writes:

> On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote:
>> Create a new driver for the da8xx DDR2/mDDR controller and implement
>> support for writing to the Peripheral Bus Burst Priority Register.
>> 
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>>  .../memory-controllers/ti-da8xx-ddrctl.txt         |  20 +++
>>  drivers/memory/Kconfig                             |   8 +
>>  drivers/memory/Makefile                            |   1 +
>>  drivers/memory/da8xx-ddrctl.c                      | 187 +++++++++++++++++++++
>>  4 files changed, 216 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>>  create mode 100644 drivers/memory/da8xx-ddrctl.c
>> 
>> diff --git
>> a/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>> b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>> new file mode 100644
>> index 0000000..f0eda59
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>> @@ -0,0 +1,20 @@
>> +* Device tree bindings for Texas Instruments da8xx DDR2/mDDR memory controller
>> +
>> +The DDR2/mDDR memory controller present on Texas Instruments da8xx SoCs memory
>> +maps a set of registers which allow to tweak the controller's behavior.
>
> This is a description of the *driver*. The device itself doesn't map
> some registers, it features them. Please descrive the *device*.
>
>> +
>> +Documentation:
>> +OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf
>> +
>> +Required properties:
>> +
>> +- compatible:		"ti,da850-ddrctl" - for da850 SoC based boards
>
> Perhaps:
>
> 	"ti,da850-ddr-controller"
>
>> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
>> +{
>> +	const struct da8xx_ddrctl_config_knob *knob;
>> +	const struct da8xx_ddrctl_setting *setting;
>> +	u32 regprop[2], base, memsize, reg;
>> +	struct device_node *node, *parent;
>> +	void __iomem *ddrctl;
>> +	const char *board;
>> +	struct device *dev;
>> +	int ret;
>> +
>> +	dev = &pdev->dev;
>> +	node = dev->of_node;
>> +
>> +	/* Find the board name. */
>> +	for (parent = node;
>> +	     !of_node_is_root(parent);
>> +	     parent = of_get_parent(parent));
>> +
>> +	ret = of_property_read_string(parent, "compatible", &board);
>> +	if (ret) {
>> +		dev_err(dev, "unable to read the soc model\n");
>> +		return ret;
>> +	}
>
> I can see that you want to expose sysfs knobs for this, but is it really
> necessary to match boards like this? It's very fragile, and commits us
> to maintaining a database of board data (i.e. a board file).
>
> I am very much not keen on that.

The original proposal[1] was to create DT properties reflecting the
various knobs in the DDR Controller, but that was frowned upon since
that was more HW configuration than hardware description.

That resulted in this approach which keeps the HW configuration values
in the driver, and selectable based on DT compatible.

IMO, neither aproach is pretty.  From a DT maintainer perspective, can
you comment on your preference?

Thanks,

Kevin

[1] https://marc.info/?l=linux-arm-kernel&m=147672200715076&w=2

Re: [RFC] ARM: memory: da8xx-ddrctl: new driver

[Kevin Hilman]

Hi Mark,

Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> writes:

> On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote:
>> Create a new driver for the da8xx DDR2/mDDR controller and implement
>> support for writing to the Peripheral Bus Burst Priority Register.
>> 
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>> ---
>>  .../memory-controllers/ti-da8xx-ddrctl.txt         |  20 +++
>>  drivers/memory/Kconfig                             |   8 +
>>  drivers/memory/Makefile                            |   1 +
>>  drivers/memory/da8xx-ddrctl.c                      | 187 +++++++++++++++++++++
>>  4 files changed, 216 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>>  create mode 100644 drivers/memory/da8xx-ddrctl.c
>> 
>> diff --git
>> a/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>> b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>> new file mode 100644
>> index 0000000..f0eda59
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>> @@ -0,0 +1,20 @@
>> +* Device tree bindings for Texas Instruments da8xx DDR2/mDDR memory controller
>> +
>> +The DDR2/mDDR memory controller present on Texas Instruments da8xx SoCs memory
>> +maps a set of registers which allow to tweak the controller's behavior.
>
> This is a description of the *driver*. The device itself doesn't map
> some registers, it features them. Please descrive the *device*.
>
>> +
>> +Documentation:
>> +OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf
>> +
>> +Required properties:
>> +
>> +- compatible:		"ti,da850-ddrctl" - for da850 SoC based boards
>
> Perhaps:
>
> 	"ti,da850-ddr-controller"
>
>> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
>> +{
>> +	const struct da8xx_ddrctl_config_knob *knob;
>> +	const struct da8xx_ddrctl_setting *setting;
>> +	u32 regprop[2], base, memsize, reg;
>> +	struct device_node *node, *parent;
>> +	void __iomem *ddrctl;
>> +	const char *board;
>> +	struct device *dev;
>> +	int ret;
>> +
>> +	dev = &pdev->dev;
>> +	node = dev->of_node;
>> +
>> +	/* Find the board name. */
>> +	for (parent = node;
>> +	     !of_node_is_root(parent);
>> +	     parent = of_get_parent(parent));
>> +
>> +	ret = of_property_read_string(parent, "compatible", &board);
>> +	if (ret) {
>> +		dev_err(dev, "unable to read the soc model\n");
>> +		return ret;
>> +	}
>
> I can see that you want to expose sysfs knobs for this, but is it really
> necessary to match boards like this? It's very fragile, and commits us
> to maintaining a database of board data (i.e. a board file).
>
> I am very much not keen on that.

The original proposal[1] was to create DT properties reflecting the
various knobs in the DDR Controller, but that was frowned upon since
that was more HW configuration than hardware description.

That resulted in this approach which keeps the HW configuration values
in the driver, and selectable based on DT compatible.

IMO, neither aproach is pretty.  From a DT maintainer perspective, can
you comment on your preference?

Thanks,

Kevin

[1] https://marc.info/?l=linux-arm-kernel&m=147672200715076&w=2
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] ARM: memory: da8xx-ddrctl: new driver

[Kevin Hilman]

Hi Mark,

Mark Rutland <mark.rutland@arm.com> writes:

> On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote:
>> Create a new driver for the da8xx DDR2/mDDR controller and implement
>> support for writing to the Peripheral Bus Burst Priority Register.
>> 
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>>  .../memory-controllers/ti-da8xx-ddrctl.txt         |  20 +++
>>  drivers/memory/Kconfig                             |   8 +
>>  drivers/memory/Makefile                            |   1 +
>>  drivers/memory/da8xx-ddrctl.c                      | 187 +++++++++++++++++++++
>>  4 files changed, 216 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>>  create mode 100644 drivers/memory/da8xx-ddrctl.c
>> 
>> diff --git
>> a/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>> b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>> new file mode 100644
>> index 0000000..f0eda59
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>> @@ -0,0 +1,20 @@
>> +* Device tree bindings for Texas Instruments da8xx DDR2/mDDR memory controller
>> +
>> +The DDR2/mDDR memory controller present on Texas Instruments da8xx SoCs memory
>> +maps a set of registers which allow to tweak the controller's behavior.
>
> This is a description of the *driver*. The device itself doesn't map
> some registers, it features them. Please descrive the *device*.
>
>> +
>> +Documentation:
>> +OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf
>> +
>> +Required properties:
>> +
>> +- compatible:		"ti,da850-ddrctl" - for da850 SoC based boards
>
> Perhaps:
>
> 	"ti,da850-ddr-controller"
>
>> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
>> +{
>> +	const struct da8xx_ddrctl_config_knob *knob;
>> +	const struct da8xx_ddrctl_setting *setting;
>> +	u32 regprop[2], base, memsize, reg;
>> +	struct device_node *node, *parent;
>> +	void __iomem *ddrctl;
>> +	const char *board;
>> +	struct device *dev;
>> +	int ret;
>> +
>> +	dev = &pdev->dev;
>> +	node = dev->of_node;
>> +
>> +	/* Find the board name. */
>> +	for (parent = node;
>> +	     !of_node_is_root(parent);
>> +	     parent = of_get_parent(parent));
>> +
>> +	ret = of_property_read_string(parent, "compatible", &board);
>> +	if (ret) {
>> +		dev_err(dev, "unable to read the soc model\n");
>> +		return ret;
>> +	}
>
> I can see that you want to expose sysfs knobs for this, but is it really
> necessary to match boards like this? It's very fragile, and commits us
> to maintaining a database of board data (i.e. a board file).
>
> I am very much not keen on that.

The original proposal[1] was to create DT properties reflecting the
various knobs in the DDR Controller, but that was frowned upon since
that was more HW configuration than hardware description.

That resulted in this approach which keeps the HW configuration values
in the driver, and selectable based on DT compatible.

IMO, neither aproach is pretty.  From a DT maintainer perspective, can
you comment on your preference?

Thanks,

Kevin

[1] https://marc.info/?l=linux-arm-kernel&m=147672200715076&w=2

Re: [RFC] ARM: memory: da8xx-ddrctl: new driver

[Mark Rutland]

On Mon, Oct 24, 2016 at 10:35:30AM -0700, Kevin Hilman wrote:
> Hi Mark,
> 
> Mark Rutland <mark.rutland@arm.com> writes:
> > On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote:
> >> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
> >> +{
> >> +	const struct da8xx_ddrctl_config_knob *knob;
> >> +	const struct da8xx_ddrctl_setting *setting;
> >> +	u32 regprop[2], base, memsize, reg;
> >> +	struct device_node *node, *parent;
> >> +	void __iomem *ddrctl;
> >> +	const char *board;
> >> +	struct device *dev;
> >> +	int ret;
> >> +
> >> +	dev = &pdev->dev;
> >> +	node = dev->of_node;
> >> +
> >> +	/* Find the board name. */
> >> +	for (parent = node;
> >> +	     !of_node_is_root(parent);
> >> +	     parent = of_get_parent(parent));
> >> +
> >> +	ret = of_property_read_string(parent, "compatible", &board);
> >> +	if (ret) {
> >> +		dev_err(dev, "unable to read the soc model\n");
> >> +		return ret;
> >> +	}
> >
> > I can see that you want to expose sysfs knobs for this, but is it really
> > necessary to match boards like this? It's very fragile, and commits us
> > to maintaining a database of board data (i.e. a board file).
> >
> > I am very much not keen on that.
> 
> The original proposal[1] was to create DT properties reflecting the
> various knobs in the DDR Controller, but that was frowned upon since
> that was more HW configuration than hardware description.
> 
> That resulted in this approach which keeps the HW configuration values
> in the driver, and selectable based on DT compatible.
> 
> IMO, neither aproach is pretty.  From a DT maintainer perspective, can
> you comment on your preference?

>From my PoV, it really depends on *why* we need this. What does the
tuning gain us, and is it specific to a given use-case?

Thanks,
Mark.

Re: [RFC] ARM: memory: da8xx-ddrctl: new driver

[Mark Rutland]

On Mon, Oct 24, 2016 at 10:35:30AM -0700, Kevin Hilman wrote:
> Hi Mark,
> 
> Mark Rutland <mark.rutland@arm.com> writes:
> > On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote:
> >> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
> >> +{
> >> +	const struct da8xx_ddrctl_config_knob *knob;
> >> +	const struct da8xx_ddrctl_setting *setting;
> >> +	u32 regprop[2], base, memsize, reg;
> >> +	struct device_node *node, *parent;
> >> +	void __iomem *ddrctl;
> >> +	const char *board;
> >> +	struct device *dev;
> >> +	int ret;
> >> +
> >> +	dev = &pdev->dev;
> >> +	node = dev->of_node;
> >> +
> >> +	/* Find the board name. */
> >> +	for (parent = node;
> >> +	     !of_node_is_root(parent);
> >> +	     parent = of_get_parent(parent));
> >> +
> >> +	ret = of_property_read_string(parent, "compatible", &board);
> >> +	if (ret) {
> >> +		dev_err(dev, "unable to read the soc model\n");
> >> +		return ret;
> >> +	}
> >
> > I can see that you want to expose sysfs knobs for this, but is it really
> > necessary to match boards like this? It's very fragile, and commits us
> > to maintaining a database of board data (i.e. a board file).
> >
> > I am very much not keen on that.
> 
> The original proposal[1] was to create DT properties reflecting the
> various knobs in the DDR Controller, but that was frowned upon since
> that was more HW configuration than hardware description.
> 
> That resulted in this approach which keeps the HW configuration values
> in the driver, and selectable based on DT compatible.
> 
> IMO, neither aproach is pretty.  From a DT maintainer perspective, can
> you comment on your preference?

>From my PoV, it really depends on *why* we need this. What does the
tuning gain us, and is it specific to a given use-case?

Thanks,
Mark.

[RFC] ARM: memory: da8xx-ddrctl: new driver

[Mark Rutland]

On Mon, Oct 24, 2016 at 10:35:30AM -0700, Kevin Hilman wrote:
> Hi Mark,
> 
> Mark Rutland <mark.rutland@arm.com> writes:
> > On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote:
> >> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
> >> +{
> >> +	const struct da8xx_ddrctl_config_knob *knob;
> >> +	const struct da8xx_ddrctl_setting *setting;
> >> +	u32 regprop[2], base, memsize, reg;
> >> +	struct device_node *node, *parent;
> >> +	void __iomem *ddrctl;
> >> +	const char *board;
> >> +	struct device *dev;
> >> +	int ret;
> >> +
> >> +	dev = &pdev->dev;
> >> +	node = dev->of_node;
> >> +
> >> +	/* Find the board name. */
> >> +	for (parent = node;
> >> +	     !of_node_is_root(parent);
> >> +	     parent = of_get_parent(parent));
> >> +
> >> +	ret = of_property_read_string(parent, "compatible", &board);
> >> +	if (ret) {
> >> +		dev_err(dev, "unable to read the soc model\n");
> >> +		return ret;
> >> +	}
> >
> > I can see that you want to expose sysfs knobs for this, but is it really
> > necessary to match boards like this? It's very fragile, and commits us
> > to maintaining a database of board data (i.e. a board file).
> >
> > I am very much not keen on that.
> 
> The original proposal[1] was to create DT properties reflecting the
> various knobs in the DDR Controller, but that was frowned upon since
> that was more HW configuration than hardware description.
> 
> That resulted in this approach which keeps the HW configuration values
> in the driver, and selectable based on DT compatible.
> 
> IMO, neither aproach is pretty.  From a DT maintainer perspective, can
> you comment on your preference?

>From my PoV, it really depends on *why* we need this. What does the
tuning gain us, and is it specific to a given use-case?

Thanks,
Mark.

Re: [RFC] ARM: memory: da8xx-ddrctl: new driver

[Kevin Hilman]

Mark Rutland <mark.rutland@arm.com> writes:

> On Mon, Oct 24, 2016 at 10:35:30AM -0700, Kevin Hilman wrote:
>> Hi Mark,
>> 
>> Mark Rutland <mark.rutland@arm.com> writes:
>> > On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote:
>> >> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
>> >> +{
>> >> +	const struct da8xx_ddrctl_config_knob *knob;
>> >> +	const struct da8xx_ddrctl_setting *setting;
>> >> +	u32 regprop[2], base, memsize, reg;
>> >> +	struct device_node *node, *parent;
>> >> +	void __iomem *ddrctl;
>> >> +	const char *board;
>> >> +	struct device *dev;
>> >> +	int ret;
>> >> +
>> >> +	dev = &pdev->dev;
>> >> +	node = dev->of_node;
>> >> +
>> >> +	/* Find the board name. */
>> >> +	for (parent = node;
>> >> +	     !of_node_is_root(parent);
>> >> +	     parent = of_get_parent(parent));
>> >> +
>> >> +	ret = of_property_read_string(parent, "compatible", &board);
>> >> +	if (ret) {
>> >> +		dev_err(dev, "unable to read the soc model\n");
>> >> +		return ret;
>> >> +	}
>> >
>> > I can see that you want to expose sysfs knobs for this, but is it really
>> > necessary to match boards like this? It's very fragile, and commits us
>> > to maintaining a database of board data (i.e. a board file).
>> >
>> > I am very much not keen on that.
>> 
>> The original proposal[1] was to create DT properties reflecting the
>> various knobs in the DDR Controller, but that was frowned upon since
>> that was more HW configuration than hardware description.
>> 
>> That resulted in this approach which keeps the HW configuration values
>> in the driver, and selectable based on DT compatible.
>> 
>> IMO, neither aproach is pretty.  From a DT maintainer perspective, can
>> you comment on your preference?
>
> From my PoV, it really depends on *why* we need this. What does the
> tuning gain us, and is it specific to a given use-case?

This is essentially a bus controller which allows you to set relative
priorities of the various bus masters (CPU data, CPU instructions, DMA
channels, ethernet MAC, SATA, display controller, etc. etc.)

The reason behind this work in the first place is a specific
use-case. Namely, a display controller on this SoC needs its bus
priority to be adjusted in order to work reliably at certain
resolutions

The vendor BSPs for this chip just setup hard-coded values in the board
files (davinci, still hasn't fully migrated to DT) but we're trying to
figure out a better way.

The first approach was exposing DT knobs for all the priorities.  The
second approach was the other extermem allowing SoC or board-specific
hard-coded values.

Another possible approach would be getting rid of the hard-coded values
and exporting an API from the driver to set the priorities of the
available masters at run-time.  I'm not awarye any current need of
changing things at run-time, but it seems potentially useful as well.

Based on all this discussion, I'm starting to lean towards the runtime
API approach, but am hoping for some suggestions.

Kevin

Re: [RFC] ARM: memory: da8xx-ddrctl: new driver

[Kevin Hilman]

Mark Rutland <mark.rutland@arm.com> writes:

> On Mon, Oct 24, 2016 at 10:35:30AM -0700, Kevin Hilman wrote:
>> Hi Mark,
>> 
>> Mark Rutland <mark.rutland@arm.com> writes:
>> > On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote:
>> >> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
>> >> +{
>> >> +	const struct da8xx_ddrctl_config_knob *knob;
>> >> +	const struct da8xx_ddrctl_setting *setting;
>> >> +	u32 regprop[2], base, memsize, reg;
>> >> +	struct device_node *node, *parent;
>> >> +	void __iomem *ddrctl;
>> >> +	const char *board;
>> >> +	struct device *dev;
>> >> +	int ret;
>> >> +
>> >> +	dev = &pdev->dev;
>> >> +	node = dev->of_node;
>> >> +
>> >> +	/* Find the board name. */
>> >> +	for (parent = node;
>> >> +	     !of_node_is_root(parent);
>> >> +	     parent = of_get_parent(parent));
>> >> +
>> >> +	ret = of_property_read_string(parent, "compatible", &board);
>> >> +	if (ret) {
>> >> +		dev_err(dev, "unable to read the soc model\n");
>> >> +		return ret;
>> >> +	}
>> >
>> > I can see that you want to expose sysfs knobs for this, but is it really
>> > necessary to match boards like this? It's very fragile, and commits us
>> > to maintaining a database of board data (i.e. a board file).
>> >
>> > I am very much not keen on that.
>> 
>> The original proposal[1] was to create DT properties reflecting the
>> various knobs in the DDR Controller, but that was frowned upon since
>> that was more HW configuration than hardware description.
>> 
>> That resulted in this approach which keeps the HW configuration values
>> in the driver, and selectable based on DT compatible.
>> 
>> IMO, neither aproach is pretty.  From a DT maintainer perspective, can
>> you comment on your preference?
>
> From my PoV, it really depends on *why* we need this. What does the
> tuning gain us, and is it specific to a given use-case?

This is essentially a bus controller which allows you to set relative
priorities of the various bus masters (CPU data, CPU instructions, DMA
channels, ethernet MAC, SATA, display controller, etc. etc.)

The reason behind this work in the first place is a specific
use-case. Namely, a display controller on this SoC needs its bus
priority to be adjusted in order to work reliably at certain
resolutions

The vendor BSPs for this chip just setup hard-coded values in the board
files (davinci, still hasn't fully migrated to DT) but we're trying to
figure out a better way.

The first approach was exposing DT knobs for all the priorities.  The
second approach was the other extermem allowing SoC or board-specific
hard-coded values.

Another possible approach would be getting rid of the hard-coded values
and exporting an API from the driver to set the priorities of the
available masters at run-time.  I'm not awarye any current need of
changing things at run-time, but it seems potentially useful as well.

Based on all this discussion, I'm starting to lean towards the runtime
API approach, but am hoping for some suggestions.

Kevin
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC] ARM: memory: da8xx-ddrctl: new driver

[Kevin Hilman]

Mark Rutland <mark.rutland@arm.com> writes:

> On Mon, Oct 24, 2016 at 10:35:30AM -0700, Kevin Hilman wrote:
>> Hi Mark,
>> 
>> Mark Rutland <mark.rutland@arm.com> writes:
>> > On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote:
>> >> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
>> >> +{
>> >> +	const struct da8xx_ddrctl_config_knob *knob;
>> >> +	const struct da8xx_ddrctl_setting *setting;
>> >> +	u32 regprop[2], base, memsize, reg;
>> >> +	struct device_node *node, *parent;
>> >> +	void __iomem *ddrctl;
>> >> +	const char *board;
>> >> +	struct device *dev;
>> >> +	int ret;
>> >> +
>> >> +	dev = &pdev->dev;
>> >> +	node = dev->of_node;
>> >> +
>> >> +	/* Find the board name. */
>> >> +	for (parent = node;
>> >> +	     !of_node_is_root(parent);
>> >> +	     parent = of_get_parent(parent));
>> >> +
>> >> +	ret = of_property_read_string(parent, "compatible", &board);
>> >> +	if (ret) {
>> >> +		dev_err(dev, "unable to read the soc model\n");
>> >> +		return ret;
>> >> +	}
>> >
>> > I can see that you want to expose sysfs knobs for this, but is it really
>> > necessary to match boards like this? It's very fragile, and commits us
>> > to maintaining a database of board data (i.e. a board file).
>> >
>> > I am very much not keen on that.
>> 
>> The original proposal[1] was to create DT properties reflecting the
>> various knobs in the DDR Controller, but that was frowned upon since
>> that was more HW configuration than hardware description.
>> 
>> That resulted in this approach which keeps the HW configuration values
>> in the driver, and selectable based on DT compatible.
>> 
>> IMO, neither aproach is pretty.  From a DT maintainer perspective, can
>> you comment on your preference?
>
> From my PoV, it really depends on *why* we need this. What does the
> tuning gain us, and is it specific to a given use-case?

This is essentially a bus controller which allows you to set relative
priorities of the various bus masters (CPU data, CPU instructions, DMA
channels, ethernet MAC, SATA, display controller, etc. etc.)

The reason behind this work in the first place is a specific
use-case. Namely, a display controller on this SoC needs its bus
priority to be adjusted in order to work reliably at certain
resolutions

The vendor BSPs for this chip just setup hard-coded values in the board
files (davinci, still hasn't fully migrated to DT) but we're trying to
figure out a better way.

The first approach was exposing DT knobs for all the priorities.  The
second approach was the other extermem allowing SoC or board-specific
hard-coded values.

Another possible approach would be getting rid of the hard-coded values
and exporting an API from the driver to set the priorities of the
available masters at run-time.  I'm not awarye any current need of
changing things at run-time, but it seems potentially useful as well.

Based on all this discussion, I'm starting to lean towards the runtime
API approach, but am hoping for some suggestions.

Kevin

Re: [RFC] ARM: memory: da8xx-ddrctl: new driver

[Kevin Hilman]

On Mon, Oct 24, 2016 at 11:41 AM, Kevin Hilman <khilman@baylibre.com> wrote:
> Mark Rutland <mark.rutland@arm.com> writes:
>
>> On Mon, Oct 24, 2016 at 10:35:30AM -0700, Kevin Hilman wrote:
>>> Hi Mark,
>>>
>>> Mark Rutland <mark.rutland@arm.com> writes:
>>> > On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote:
>>> >> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
>>> >> +{
>>> >> + const struct da8xx_ddrctl_config_knob *knob;
>>> >> + const struct da8xx_ddrctl_setting *setting;
>>> >> + u32 regprop[2], base, memsize, reg;
>>> >> + struct device_node *node, *parent;
>>> >> + void __iomem *ddrctl;
>>> >> + const char *board;
>>> >> + struct device *dev;
>>> >> + int ret;
>>> >> +
>>> >> + dev = &pdev->dev;
>>> >> + node = dev->of_node;
>>> >> +
>>> >> + /* Find the board name. */
>>> >> + for (parent = node;
>>> >> +      !of_node_is_root(parent);
>>> >> +      parent = of_get_parent(parent));
>>> >> +
>>> >> + ret = of_property_read_string(parent, "compatible", &board);
>>> >> + if (ret) {
>>> >> +         dev_err(dev, "unable to read the soc model\n");
>>> >> +         return ret;
>>> >> + }
>>> >
>>> > I can see that you want to expose sysfs knobs for this, but is it really
>>> > necessary to match boards like this? It's very fragile, and commits us
>>> > to maintaining a database of board data (i.e. a board file).
>>> >
>>> > I am very much not keen on that.
>>>
>>> The original proposal[1] was to create DT properties reflecting the
>>> various knobs in the DDR Controller, but that was frowned upon since
>>> that was more HW configuration than hardware description.
>>>
>>> That resulted in this approach which keeps the HW configuration values
>>> in the driver, and selectable based on DT compatible.
>>>
>>> IMO, neither aproach is pretty.  From a DT maintainer perspective, can
>>> you comment on your preference?
>>
>> From my PoV, it really depends on *why* we need this. What does the
>> tuning gain us, and is it specific to a given use-case?
>
> This is essentially a bus controller which allows you to set relative
> priorities of the various bus masters (CPU data, CPU instructions, DMA
> channels, ethernet MAC, SATA, display controller, etc. etc.)

Scratch that... I got this one confused with a different drivers/bus
driver Bartosz is also working on. :(

This one is just for the mechanism that controls how long old
(low-priority) xfers in the DDR command FIFO are allowed to sit around
before they will be flushed.

The use-case is the same though.  The display controller doesn't work
at higher resolutions without tweaking this setting.

The question remains though: as a system-wide setting, should this be
configured via DT (either by a DT property, or based on a compatible
string in the driver) or should the driver provide an API to tweak it.

Kevin

Re: [RFC] ARM: memory: da8xx-ddrctl: new driver

[Kevin Hilman]

On Mon, Oct 24, 2016 at 11:41 AM, Kevin Hilman <khilman@baylibre.com> wrote:
> Mark Rutland <mark.rutland@arm.com> writes:
>
>> On Mon, Oct 24, 2016 at 10:35:30AM -0700, Kevin Hilman wrote:
>>> Hi Mark,
>>>
>>> Mark Rutland <mark.rutland@arm.com> writes:
>>> > On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote:
>>> >> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
>>> >> +{
>>> >> + const struct da8xx_ddrctl_config_knob *knob;
>>> >> + const struct da8xx_ddrctl_setting *setting;
>>> >> + u32 regprop[2], base, memsize, reg;
>>> >> + struct device_node *node, *parent;
>>> >> + void __iomem *ddrctl;
>>> >> + const char *board;
>>> >> + struct device *dev;
>>> >> + int ret;
>>> >> +
>>> >> + dev = &pdev->dev;
>>> >> + node = dev->of_node;
>>> >> +
>>> >> + /* Find the board name. */
>>> >> + for (parent = node;
>>> >> +      !of_node_is_root(parent);
>>> >> +      parent = of_get_parent(parent));
>>> >> +
>>> >> + ret = of_property_read_string(parent, "compatible", &board);
>>> >> + if (ret) {
>>> >> +         dev_err(dev, "unable to read the soc model\n");
>>> >> +         return ret;
>>> >> + }
>>> >
>>> > I can see that you want to expose sysfs knobs for this, but is it really
>>> > necessary to match boards like this? It's very fragile, and commits us
>>> > to maintaining a database of board data (i.e. a board file).
>>> >
>>> > I am very much not keen on that.
>>>
>>> The original proposal[1] was to create DT properties reflecting the
>>> various knobs in the DDR Controller, but that was frowned upon since
>>> that was more HW configuration than hardware description.
>>>
>>> That resulted in this approach which keeps the HW configuration values
>>> in the driver, and selectable based on DT compatible.
>>>
>>> IMO, neither aproach is pretty.  From a DT maintainer perspective, can
>>> you comment on your preference?
>>
>> From my PoV, it really depends on *why* we need this. What does the
>> tuning gain us, and is it specific to a given use-case?
>
> This is essentially a bus controller which allows you to set relative
> priorities of the various bus masters (CPU data, CPU instructions, DMA
> channels, ethernet MAC, SATA, display controller, etc. etc.)

Scratch that... I got this one confused with a different drivers/bus
driver Bartosz is also working on. :(

This one is just for the mechanism that controls how long old
(low-priority) xfers in the DDR command FIFO are allowed to sit around
before they will be flushed.

The use-case is the same though.  The display controller doesn't work
at higher resolutions without tweaking this setting.

The question remains though: as a system-wide setting, should this be
configured via DT (either by a DT property, or based on a compatible
string in the driver) or should the driver provide an API to tweak it.

Kevin
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC] ARM: memory: da8xx-ddrctl: new driver

[Kevin Hilman]

On Mon, Oct 24, 2016 at 11:41 AM, Kevin Hilman <khilman@baylibre.com> wrote:
> Mark Rutland <mark.rutland@arm.com> writes:
>
>> On Mon, Oct 24, 2016 at 10:35:30AM -0700, Kevin Hilman wrote:
>>> Hi Mark,
>>>
>>> Mark Rutland <mark.rutland@arm.com> writes:
>>> > On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote:
>>> >> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
>>> >> +{
>>> >> + const struct da8xx_ddrctl_config_knob *knob;
>>> >> + const struct da8xx_ddrctl_setting *setting;
>>> >> + u32 regprop[2], base, memsize, reg;
>>> >> + struct device_node *node, *parent;
>>> >> + void __iomem *ddrctl;
>>> >> + const char *board;
>>> >> + struct device *dev;
>>> >> + int ret;
>>> >> +
>>> >> + dev = &pdev->dev;
>>> >> + node = dev->of_node;
>>> >> +
>>> >> + /* Find the board name. */
>>> >> + for (parent = node;
>>> >> +      !of_node_is_root(parent);
>>> >> +      parent = of_get_parent(parent));
>>> >> +
>>> >> + ret = of_property_read_string(parent, "compatible", &board);
>>> >> + if (ret) {
>>> >> +         dev_err(dev, "unable to read the soc model\n");
>>> >> +         return ret;
>>> >> + }
>>> >
>>> > I can see that you want to expose sysfs knobs for this, but is it really
>>> > necessary to match boards like this? It's very fragile, and commits us
>>> > to maintaining a database of board data (i.e. a board file).
>>> >
>>> > I am very much not keen on that.
>>>
>>> The original proposal[1] was to create DT properties reflecting the
>>> various knobs in the DDR Controller, but that was frowned upon since
>>> that was more HW configuration than hardware description.
>>>
>>> That resulted in this approach which keeps the HW configuration values
>>> in the driver, and selectable based on DT compatible.
>>>
>>> IMO, neither aproach is pretty.  From a DT maintainer perspective, can
>>> you comment on your preference?
>>
>> From my PoV, it really depends on *why* we need this. What does the
>> tuning gain us, and is it specific to a given use-case?
>
> This is essentially a bus controller which allows you to set relative
> priorities of the various bus masters (CPU data, CPU instructions, DMA
> channels, ethernet MAC, SATA, display controller, etc. etc.)

Scratch that... I got this one confused with a different drivers/bus
driver Bartosz is also working on. :(

This one is just for the mechanism that controls how long old
(low-priority) xfers in the DDR command FIFO are allowed to sit around
before they will be flushed.

The use-case is the same though.  The display controller doesn't work
at higher resolutions without tweaking this setting.

The question remains though: as a system-wide setting, should this be
configured via DT (either by a DT property, or based on a compatible
string in the driver) or should the driver provide an API to tweak it.

Kevin

Re: [RFC] ARM: memory: da8xx-ddrctl: new driver

[Bartosz Golaszewski]

2016-10-24 19:00 GMT+02:00 Mark Rutland <mark.rutland@arm.com>:
> On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote:
>> +
>> +     dev = &pdev->dev;
>> +     node = dev->of_node;
>> +
>> +     /* Find the board name. */
>> +     for (parent = node;
>> +          !of_node_is_root(parent);
>> +          parent = of_get_parent(parent));
>> +
>> +     ret = of_property_read_string(parent, "compatible", &board);
>> +     if (ret) {
>> +             dev_err(dev, "unable to read the soc model\n");
>> +             return ret;
>> +     }
>
> I can see that you want to expose sysfs knobs for this, but is it really
> necessary to match boards like this? It's very fragile, and commits us
> to maintaining a database of board data (i.e. a board file).
>
> I am very much not keen on that.
>

I Mark,

I don't want to expose any new sysfs interface.

The initial idea was to follow the way the ti-aemif driver is
implemented and expose DT bindings for the memory controller knobs
(initially only the Peripheral Bus Burst Priority Register, tweaking
which is required to make LCDC work correctly on da850 based boards as
mentioned by Kevin). This was rejected as it's not hardware
description but configuration, so it should not be controlled by DT
properties.

The correct approach for this kind of performance knobs doesn't exist
yet. There was a BoF during this year's ELCE in Berlin during which
several ideas were discussed, but no code has been written so far.

Using sysfs would have exactly the same disadvantage - committing to a
stable interface that would have to be maintained indefinitely.

In the end it was decided that a fairly good, temporary solution would
be to create a driver for the da850 DDR controller which would
hardcode the required tweaks for several boards (as the LCDC issue is
known to affect more TI SoCs and boards). Once a framework for
performance knobs is implemented and merged, it would be easy to port
the driver to it as we would not have implemented any stable
interface.

The same solution would be used for the SYSCFG registers on the da8xx SoCs.

Hope that clarifies the need for this patch a bit. I will address all
other issues in v2.

Best regards,
Bartosz Golaszewski

Re: [RFC] ARM: memory: da8xx-ddrctl: new driver

[Bartosz Golaszewski]

2016-10-24 19:00 GMT+02:00 Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>:
> On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote:
>> +
>> +     dev = &pdev->dev;
>> +     node = dev->of_node;
>> +
>> +     /* Find the board name. */
>> +     for (parent = node;
>> +          !of_node_is_root(parent);
>> +          parent = of_get_parent(parent));
>> +
>> +     ret = of_property_read_string(parent, "compatible", &board);
>> +     if (ret) {
>> +             dev_err(dev, "unable to read the soc model\n");
>> +             return ret;
>> +     }
>
> I can see that you want to expose sysfs knobs for this, but is it really
> necessary to match boards like this? It's very fragile, and commits us
> to maintaining a database of board data (i.e. a board file).
>
> I am very much not keen on that.
>

I Mark,

I don't want to expose any new sysfs interface.

The initial idea was to follow the way the ti-aemif driver is
implemented and expose DT bindings for the memory controller knobs
(initially only the Peripheral Bus Burst Priority Register, tweaking
which is required to make LCDC work correctly on da850 based boards as
mentioned by Kevin). This was rejected as it's not hardware
description but configuration, so it should not be controlled by DT
properties.

The correct approach for this kind of performance knobs doesn't exist
yet. There was a BoF during this year's ELCE in Berlin during which
several ideas were discussed, but no code has been written so far.

Using sysfs would have exactly the same disadvantage - committing to a
stable interface that would have to be maintained indefinitely.

In the end it was decided that a fairly good, temporary solution would
be to create a driver for the da850 DDR controller which would
hardcode the required tweaks for several boards (as the LCDC issue is
known to affect more TI SoCs and boards). Once a framework for
performance knobs is implemented and merged, it would be easy to port
the driver to it as we would not have implemented any stable
interface.

The same solution would be used for the SYSCFG registers on the da8xx SoCs.

Hope that clarifies the need for this patch a bit. I will address all
other issues in v2.

Best regards,
Bartosz Golaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] ARM: memory: da8xx-ddrctl: new driver

[Bartosz Golaszewski]

2016-10-24 19:00 GMT+02:00 Mark Rutland <mark.rutland@arm.com>:
> On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote:
>> +
>> +     dev = &pdev->dev;
>> +     node = dev->of_node;
>> +
>> +     /* Find the board name. */
>> +     for (parent = node;
>> +          !of_node_is_root(parent);
>> +          parent = of_get_parent(parent));
>> +
>> +     ret = of_property_read_string(parent, "compatible", &board);
>> +     if (ret) {
>> +             dev_err(dev, "unable to read the soc model\n");
>> +             return ret;
>> +     }
>
> I can see that you want to expose sysfs knobs for this, but is it really
> necessary to match boards like this? It's very fragile, and commits us
> to maintaining a database of board data (i.e. a board file).
>
> I am very much not keen on that.
>

I Mark,

I don't want to expose any new sysfs interface.

The initial idea was to follow the way the ti-aemif driver is
implemented and expose DT bindings for the memory controller knobs
(initially only the Peripheral Bus Burst Priority Register, tweaking
which is required to make LCDC work correctly on da850 based boards as
mentioned by Kevin). This was rejected as it's not hardware
description but configuration, so it should not be controlled by DT
properties.

The correct approach for this kind of performance knobs doesn't exist
yet. There was a BoF during this year's ELCE in Berlin during which
several ideas were discussed, but no code has been written so far.

Using sysfs would have exactly the same disadvantage - committing to a
stable interface that would have to be maintained indefinitely.

In the end it was decided that a fairly good, temporary solution would
be to create a driver for the da850 DDR controller which would
hardcode the required tweaks for several boards (as the LCDC issue is
known to affect more TI SoCs and boards). Once a framework for
performance knobs is implemented and merged, it would be easy to port
the driver to it as we would not have implemented any stable
interface.

The same solution would be used for the SYSCFG registers on the da8xx SoCs.

Hope that clarifies the need for this patch a bit. I will address all
other issues in v2.

Best regards,
Bartosz Golaszewski