[PATCH v2 0/5] ARM: da850: new drivers for better LCDC support

[PATCH v2 0/5] ARM: da850: new drivers for better LCDC support

[Bartosz Golaszewski]

This series adds two new drivers in order to better support the LCDC
rev1 present on the da850 boards.

The first patch adds a new memory driver which allows to write to the
DDR2/mDDR memory controller present on the da8xx SoCs.

The second patch adds a new bus driver which allows to interact with
the MSTPRI registers of the SYSCFG0 module

As is mentioned in the comments: we don't want to commit to supporting
stable interfaces (DT bindings or sysfs attributes) so we hardcode the
settings required by some boards (for now only da850-lcdk) with the
hope that linux gets an appropriate framework for performance knobs
in the future.

Potential extensions of these drivers should be straightforward in the
future.

Subsequent patches add DT nodes for the new drivers: disabled nodes
in da850.dtsi and enabled in da850-lcdk.dts.

The last patch adds a workaround for current lack of support for drm
bridges in tilcdc.

Tested on a da850-lcdk with a display connected over VGA and two
additional patches for tilcdc (sent to linux-drm): ran simple modetest
for supported resolutions, used X.org and fluxbox as graphical
environment, played video with mplayer.

v1 -> v2:
- used regular readl()/writel() instead of __raw_** versions
- used resource_size() instead of calculating the size by hand
- used ioremap instead of syscon in patch [2/5]
- added the DT nodes in patches [3/5]-[5/5]

Bartosz Golaszewski (5):
  ARM: memory: da8xx-ddrctl: new driver
  ARM: bus: da8xx-mstpri: new driver
  ARM: dts: da850: add the mstpri and ddrctl nodes
  ARM: dts: da850-lcdk: enable mstpri and ddrctl nodes
  ARM: dts: da850-lcdk: add tilcdc panel node

 .../devicetree/bindings/bus/ti,da850-mstpri.txt    |  20 ++
 .../memory-controllers/ti-da8xx-ddrctl.txt         |  20 ++
 arch/arm/boot/dts/da850-lcdk.dts                   |  71 ++++++
 arch/arm/boot/dts/da850.dtsi                       |  11 +
 drivers/bus/Kconfig                                |   9 +
 drivers/bus/Makefile                               |   2 +
 drivers/bus/da8xx-mstpri.c                         | 269 +++++++++++++++++++++
 drivers/memory/Kconfig                             |   8 +
 drivers/memory/Makefile                            |   1 +
 drivers/memory/da8xx-ddrctl.c                      | 175 ++++++++++++++
 10 files changed, 586 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/bus/ti,da850-mstpri.txt
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
 create mode 100644 drivers/bus/da8xx-mstpri.c
 create mode 100644 drivers/memory/da8xx-ddrctl.c

-- 
2.9.3

[PATCH v2 1/5] 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                      | 175 +++++++++++++++++++++
 4 files changed, 204 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..ec1dd40
--- /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 features
+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-ddr-controller" - 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-ddr-controller";
+	reg = <0xb0000000 0xe8>;
+};
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..a20e7bb
--- /dev/null
+++ b/drivers/memory/da8xx-ddrctl.c
@@ -0,0 +1,175 @@
+/*
+ * 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/of_fdt.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+
+/*
+ * REVISIT: Linux doesn't have a good framework for the kind of performance
+ * knobs this driver controls. We can't use device tree properties as it deals
+ * with hardware configuration rather than description. We also don't want to
+ * commit to maintaining some random sysfs attributes.
+ *
+ * For now we just hardcode the register values for the boards that need
+ * some changes (as is the case for the LCD controller on da850-lcdk - the
+ * first board we support here). When linux gets an appropriate framework,
+ * we'll easily convert the driver to it.
+ */
+
+struct da8xx_ddrctl_config_knob {
+	const char *name;
+	u32 reg;
+	u32 mask;
+	u32 shift;
+};
+
+static const struct da8xx_ddrctl_config_knob da8xx_ddrctl_knobs[] = {
+	{
+		.name = "da850-pbbpr",
+		.reg = 0x20,
+		.mask = 0xffffff00,
+		.shift = 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_get_board_settings(void)
+{
+	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[i];
+
+		if (of_machine_is_compatible(board_settings->board))
+			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;
+	struct device_node *node;
+	struct resource *res;
+	void __iomem *ddrctl;
+	struct device *dev;
+	u32 reg;
+
+	dev = &pdev->dev;
+	node = dev->of_node;
+
+	setting = da8xx_ddrctl_get_board_settings();
+	if (!setting) {
+		dev_err(dev, "no settings for board '%s'\n",
+			of_flat_dt_get_machine_name());
+		return -EINVAL;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ddrctl = devm_ioremap_resource(dev, res);
+	if (IS_ERR(ddrctl)) {
+		dev_err(dev, "unable to map memory controller registers\n");
+		return PTR_ERR(ddrctl);
+	}
+
+	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 + sizeof(u32) > resource_size(res)) {
+			dev_warn(dev,
+				 "register offset of '%s' exceeds mapped memory size\n",
+				 knob->name);
+			continue;
+		}
+
+		reg = readl(ddrctl + knob->reg);
+		reg &= knob->mask;
+		reg |= setting->val << knob->shift;
+
+		dev_dbg(dev, "writing 0x%08x to %s\n", reg, setting->name);
+
+		writel(reg, ddrctl + knob->reg);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id da8xx_ddrctl_of_match[] = {
+	{ .compatible = "ti,da850-ddr-controller", },
+	{ },
+};
+
+static struct platform_driver da8xx_ddrctl_driver = {
+	.probe = da8xx_ddrctl_probe,
+	.driver = {
+		.name = "da850-ddr-controller",
+		.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

[PATCH v2 2/5] ARM: bus: da8xx-mstpri: new driver

[Bartosz Golaszewski]

Create the driver for the da8xx master peripheral priority
configuration and implement support for writing to the three
Master Priority registers on da850 SoCs.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 .../devicetree/bindings/bus/ti,da850-mstpri.txt    |  20 ++
 drivers/bus/Kconfig                                |   9 +
 drivers/bus/Makefile                               |   2 +
 drivers/bus/da8xx-mstpri.c                         | 269 +++++++++++++++++++++
 4 files changed, 300 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/bus/ti,da850-mstpri.txt
 create mode 100644 drivers/bus/da8xx-mstpri.c

diff --git a/Documentation/devicetree/bindings/bus/ti,da850-mstpri.txt b/Documentation/devicetree/bindings/bus/ti,da850-mstpri.txt
new file mode 100644
index 0000000..72daefc
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/ti,da850-mstpri.txt
@@ -0,0 +1,20 @@
+* Device tree bindings for Texas Instruments da8xx master peripheral
+  priority driver
+
+DA8XX SoCs feature a set of registers allowing to change the priority of all
+peripherals classified as masters.
+
+Documentation:
+OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf
+
+Required properties:
+
+- compatible:		"ti,da850-mstpri" - for da850 based boards
+- reg:			offset and length of the mstpri registers
+
+Example for da850-lcdk is shown below.
+
+mstpri {
+	compatible = "ti,da850-mstpri";
+	reg = <0x14110 0x0c>;
+};
diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 7875105..f5db3a7 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -167,4 +167,13 @@ config VEXPRESS_CONFIG
 	help
 	  Platform configuration infrastructure for the ARM Ltd.
 	  Versatile Express.
+
+config DA8XX_MSTPRI
+	bool "TI da8xx master peripheral priority driver"
+	depends on ARCH_DAVINCI_DA8XX
+	help
+	  Driver for Texas Instruments da8xx master peripheral priority
+	  configuration. Allows to adjust the priorities of all master
+	  peripherals.
+
 endmenu
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index c6cfa6b..2adb540 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -21,3 +21,5 @@ obj-$(CONFIG_SIMPLE_PM_BUS)	+= simple-pm-bus.o
 obj-$(CONFIG_TEGRA_ACONNECT)	+= tegra-aconnect.o
 obj-$(CONFIG_UNIPHIER_SYSTEM_BUS)	+= uniphier-system-bus.o
 obj-$(CONFIG_VEXPRESS_CONFIG)	+= vexpress-config.o
+
+obj-$(CONFIG_DA8XX_MSTPRI)	+= da8xx-mstpri.o
diff --git a/drivers/bus/da8xx-mstpri.c b/drivers/bus/da8xx-mstpri.c
new file mode 100644
index 0000000..85f0b53
--- /dev/null
+++ b/drivers/bus/da8xx-mstpri.c
@@ -0,0 +1,269 @@
+/*
+ * TI da8xx master peripheral priority driver
+ *
+ * Copyright (C) 2016 BayLibre SAS
+ *
+ * Author:
+ *   Bartosz Golaszewski <bgolaszewski@baylibre.com.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/platform_device.h>
+#include <linux/io.h>
+#include <linux/regmap.h>
+#include <linux/of_fdt.h>
+
+/*
+ * REVISIT: Linux doesn't have a good framework for the kind of performance
+ * knobs this driver controls. We can't use device tree properties as it deals
+ * with hardware configuration rather than description. We also don't want to
+ * commit to maintaining some random sysfs attributes.
+ *
+ * For now we just hardcode the register values for the boards that need
+ * some changes (as is the case for the LCD controller on da850-lcdk - the
+ * first board we support here). When linux gets an appropriate framework,
+ * we'll easily convert the driver to it.
+ */
+
+#define DA8XX_MSTPRI0_OFFSET		0
+#define DA8XX_MSTPRI1_OFFSET		4
+#define DA8XX_MSTPRI2_OFFSET		8
+
+enum {
+	DA8XX_MSTPRI_ARM_I = 0,
+	DA8XX_MSTPRI_ARM_D,
+	DA8XX_MSTPRI_UPP,
+	DA8XX_MSTPRI_SATA,
+	DA8XX_MSTPRI_PRU0,
+	DA8XX_MSTPRI_PRU1,
+	DA8XX_MSTPRI_EDMA30TC0,
+	DA8XX_MSTPRI_EDMA30TC1,
+	DA8XX_MSTPRI_EDMA31TC0,
+	DA8XX_MSTPRI_VPIF_DMA_0,
+	DA8XX_MSTPRI_VPIF_DMA_1,
+	DA8XX_MSTPRI_EMAC,
+	DA8XX_MSTPRI_USB0CFG,
+	DA8XX_MSTPRI_USB0CDMA,
+	DA8XX_MSTPRI_UHPI,
+	DA8XX_MSTPRI_USB1,
+	DA8XX_MSTPRI_LCDC,
+};
+
+struct da8xx_mstpri_descr {
+	int reg;
+	int shift;
+	int mask;
+};
+
+static const struct da8xx_mstpri_descr da8xx_mstpri_priority_list[] = {
+	[DA8XX_MSTPRI_ARM_I] = {
+		.reg = DA8XX_MSTPRI0_OFFSET,
+		.shift = 0,
+		.mask = 0x0000000f,
+	},
+	[DA8XX_MSTPRI_ARM_D] = {
+		.reg = DA8XX_MSTPRI0_OFFSET,
+		.shift = 4,
+		.mask = 0x000000f0,
+	},
+	[DA8XX_MSTPRI_UPP] = {
+		.reg = DA8XX_MSTPRI0_OFFSET,
+		.shift = 16,
+		.mask = 0x000f0000,
+	},
+	[DA8XX_MSTPRI_SATA] = {
+		.reg = DA8XX_MSTPRI0_OFFSET,
+		.shift = 20,
+		.mask = 0x00f00000,
+	},
+	[DA8XX_MSTPRI_PRU0] = {
+		.reg = DA8XX_MSTPRI1_OFFSET,
+		.shift = 0,
+		.mask = 0x0000000f,
+	},
+	[DA8XX_MSTPRI_PRU1] = {
+		.reg = DA8XX_MSTPRI1_OFFSET,
+		.shift = 4,
+		.mask = 0x000000f0,
+	},
+	[DA8XX_MSTPRI_EDMA30TC0] = {
+		.reg = DA8XX_MSTPRI1_OFFSET,
+		.shift = 8,
+		.mask = 0x00000f00,
+	},
+	[DA8XX_MSTPRI_EDMA30TC1] = {
+		.reg = DA8XX_MSTPRI1_OFFSET,
+		.shift = 12,
+		.mask = 0x0000f000,
+	},
+	[DA8XX_MSTPRI_EDMA31TC0] = {
+		.reg = DA8XX_MSTPRI1_OFFSET,
+		.shift = 16,
+		.mask = 0x000f0000,
+	},
+	[DA8XX_MSTPRI_VPIF_DMA_0] = {
+		.reg = DA8XX_MSTPRI1_OFFSET,
+		.shift = 24,
+		.mask = 0x0f000000,
+	},
+	[DA8XX_MSTPRI_VPIF_DMA_1] = {
+		.reg = DA8XX_MSTPRI1_OFFSET,
+		.shift = 28,
+		.mask = 0xf0000000,
+	},
+	[DA8XX_MSTPRI_EMAC] = {
+		.reg = DA8XX_MSTPRI2_OFFSET,
+		.shift = 0,
+		.mask = 0x0000000f,
+	},
+	[DA8XX_MSTPRI_USB0CFG] = {
+		.reg = DA8XX_MSTPRI2_OFFSET,
+		.shift = 8,
+		.mask = 0x00000f00,
+	},
+	[DA8XX_MSTPRI_USB0CDMA] = {
+		.reg = DA8XX_MSTPRI2_OFFSET,
+		.shift = 12,
+		.mask = 0x0000f000,
+	},
+	[DA8XX_MSTPRI_UHPI] = {
+		.reg = DA8XX_MSTPRI2_OFFSET,
+		.shift = 20,
+		.mask = 0x00f00000,
+	},
+	[DA8XX_MSTPRI_USB1] = {
+		.reg = DA8XX_MSTPRI2_OFFSET,
+		.shift = 24,
+		.mask = 0x0f000000,
+	},
+	[DA8XX_MSTPRI_LCDC] = {
+		.reg = DA8XX_MSTPRI2_OFFSET,
+		.shift = 28,
+		.mask = 0xf0000000,
+	},
+};
+
+struct da8xx_mstpri_priority {
+	int which;
+	u32 val;
+};
+
+struct da8xx_mstpri_board_priorities {
+	const char *board;
+	const struct da8xx_mstpri_priority *priorities;
+	size_t numprio;
+};
+
+/*
+ * Default memory settings of da850 do not meet the throughput/latency
+ * requirements of tilcdc. This results in the image displayed being
+ * incorrect and the following warning being displayed by the LCDC
+ * drm driver:
+ *
+ *   tilcdc da8xx_lcdc.0: tilcdc_crtc_irq(0x00000020): FIFO underfow
+ */
+static const struct da8xx_mstpri_priority da850_lcdk_priorities[] = {
+	{
+		.which = DA8XX_MSTPRI_LCDC,
+		.val = 0,
+	},
+	{
+		.which = DA8XX_MSTPRI_EDMA30TC1,
+		.val = 0,
+	},
+	{
+		.which = DA8XX_MSTPRI_EDMA30TC0,
+		.val = 1,
+	},
+};
+
+static const struct da8xx_mstpri_board_priorities da8xx_mstpri_board_confs[] = {
+	{
+		.board = "ti,da850-lcdk",
+		.priorities = da850_lcdk_priorities,
+		.numprio = ARRAY_SIZE(da850_lcdk_priorities),
+	},
+};
+
+static const struct da8xx_mstpri_board_priorities *
+da8xx_mstpri_get_board_prio(void)
+{
+	const struct da8xx_mstpri_board_priorities *board_prio;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(da8xx_mstpri_board_confs); i++) {
+		board_prio = &da8xx_mstpri_board_confs[i];
+
+		if (of_machine_is_compatible(board_prio->board))
+			return board_prio;
+	}
+
+	return NULL;
+}
+
+static int da8xx_mstpri_probe(struct platform_device *pdev)
+{
+	const struct da8xx_mstpri_board_priorities *prio_list;
+	const struct da8xx_mstpri_descr *prio_descr;
+	const struct da8xx_mstpri_priority *prio;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	void __iomem *mstpri;
+	u32 reg;
+	int i;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mstpri = devm_ioremap_resource(dev, res);
+	if (IS_ERR(mstpri)) {
+		dev_err(dev, "unable to map MSTPRI registers\n");
+		return PTR_ERR(mstpri);
+	}
+
+	prio_list = da8xx_mstpri_get_board_prio();
+	if (!prio_list) {
+		dev_err(dev, "no master priotities defined for board '%s'\n",
+			of_flat_dt_get_machine_name());
+		return -EINVAL;
+	}
+
+	for (i = 0; i < prio_list->numprio; i++) {
+		prio = &prio_list->priorities[i];
+		prio_descr = &da8xx_mstpri_priority_list[prio->which];
+
+		if (prio_descr->reg + sizeof(u32) > resource_size(res)) {
+			dev_warn(dev, "register offset out of range\n");
+			continue;
+		}
+
+		reg = readl(mstpri + prio_descr->reg);
+		reg &= ~prio_descr->mask;
+		reg |= prio->val << prio_descr->shift;
+
+		writel(reg, mstpri + prio_descr->reg);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id da8xx_mstpri_of_match[] = {
+	{ .compatible = "ti,da850-mstpri", },
+	{ },
+};
+
+static struct platform_driver da8xx_mstpri_driver = {
+	.probe = da8xx_mstpri_probe,
+	.driver = {
+		.name = "da8xx-mstpri",
+		.of_match_table = da8xx_mstpri_of_match,
+	},
+};
+module_platform_driver(da8xx_mstpri_driver);
+
+MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
+MODULE_DESCRIPTION("TI da8xx master peripheral priority driver");
+MODULE_LICENSE("GPL v2");
-- 
2.9.3

[PATCH v2 3/5] ARM: dts: da850: add the mstpri and ddrctl nodes

[Bartosz Golaszewski]

Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
controller drivers to da850.dtsi.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/boot/dts/da850.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 112c6d7..44bece3 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -454,6 +454,12 @@
 			interrupts = <52>;
 			status = "disabled";
 		};
+
+		mstpri: mstpri@14110 {
+			compatible = "ti,da850-mstpri";
+			reg = <0x14110 0x0c>;
+			status = "disabled";
+		};
 	};
 	aemif: aemif@68000000 {
 		compatible = "ti,da850-aemif";
@@ -465,4 +471,9 @@
 			  1 0 0x68000000 0x00008000>;
 		status = "disabled";
 	};
+	ddrctl: ddrctl@b0000000 {
+		compatible = "ti,da850-ddr-controller";
+		reg = <0xb0000000 0xe8>;
+		status = "disabled";
+	};
 };
-- 
2.9.3

[PATCH v2 4/5] ARM: dts: da850-lcdk: enable mstpri and ddrctl nodes

[Bartosz Golaszewski]

Enable the MSTPRI configuration and DDR2/mDDR memory controller
nodes on da850-lcdk. This is needed in order to adjust the memory
throughput constraints for better tilcdc support.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/boot/dts/da850-lcdk.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/da850-lcdk.dts b/arch/arm/boot/dts/da850-lcdk.dts
index 4747629..b39796e 100644
--- a/arch/arm/boot/dts/da850-lcdk.dts
+++ b/arch/arm/boot/dts/da850-lcdk.dts
@@ -243,3 +243,11 @@
 		};
 	};
 };
+
+&mstpri {
+	status = "okay";
+};
+
+&ddrctl {
+	status = "okay";
+};
-- 
2.9.3

[PATCH v2 5/5] ARM: dts: da850-lcdk: add tilcdc panel node

[Bartosz Golaszewski]

The tilcdc driver is not yet ready for working together with the
dumb-vga-dac drm bridge. While the work on enabling drm_bridge
support in tilcdc continues, enable the VGA connector on da850-lcdk
with the following workaround: use the tilcdc-panel driver with
a set of common (and tested) resolutions.

Once the drm bridge support is complete, we'll remove the node added
by this patch and use the correct solution. This change will be
transparent for the user.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/boot/dts/da850-lcdk.dts | 63 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/arch/arm/boot/dts/da850-lcdk.dts b/arch/arm/boot/dts/da850-lcdk.dts
index b39796e..df582c6 100644
--- a/arch/arm/boot/dts/da850-lcdk.dts
+++ b/arch/arm/boot/dts/da850-lcdk.dts
@@ -62,6 +62,65 @@
 		regulator-max-microvolt = <5000000>;
 	};
 
+	/*
+	 * Remove this node once the tilcdc driver gets support for
+	 * drm bridge modules.
+	 */
+	panel {
+		compatible = "ti,tilcdc,panel";
+		pinctrl-names = "default";
+		pinctrl-0 = <&lcd_pins>;
+		status = "okay";
+
+		panel-info {
+			ac-bias = <0>;
+			ac-bias-intrpt = <0>;
+			dma-burst-sz = <16>;
+			bpp = <16>;
+			fdd = <255>;
+			sync-edge = <0>;
+			sync-ctrl = <0>;
+			raster-order = <0>;
+			fifo-th = <5>;
+		};
+
+		display-timings {
+			native-mode = <&svga_timings>;
+			vga_timings: 640x480@60 {
+				clock-frequency = <27500000>;
+				hactive = <640>;
+				hback-porch = <90>;
+				hfront-porch = <40>;
+				hsync-len = <128>;
+				vactive = <480>;
+				vback-porch = <23>;
+				vfront-porch = <1>;
+				vsync-len = <4>;
+			};
+			vga_timings_hf: 640x480@75 {
+				clock-frequency = <34000000>;
+				hactive = <640>;
+				hback-porch = <90>;
+				hfront-porch = <40>;
+				hsync-len = <128>;
+				vactive = <480>;
+				vback-porch = <23>;
+				vfront-porch = <1>;
+				vsync-len = <4>;
+			};
+			svga_timings: 800x600@56 {
+				clock-frequency = <37500000>;
+				hactive = <800>;
+				hback-porch = <140>;
+				hfront-porch = <40>;
+				hsync-len = <128>;
+				vactive = <600>;
+				vback-porch = <23>;
+				vfront-porch = <1>;
+				vsync-len = <4>;
+			};
+		};
+	};
 };
 
 &pmx_core {
@@ -251,3 +310,7 @@
 &ddrctl {
 	status = "okay";
 };
+
+&display {
+	status = "okay";
+};
-- 
2.9.3

Re: [PATCH v2 4/5] ARM: dts: da850-lcdk: enable mstpri and ddrctl nodes

[Laurent Pinchart]

Hi Bartosz,

Thank you for the patch.

On Monday 31 Oct 2016 15:45:37 Bartosz Golaszewski wrote:
> Enable the MSTPRI configuration and DDR2/mDDR memory controller
> nodes on da850-lcdk. This is needed in order to adjust the memory
> throughput constraints for better tilcdc support.

Is there a reason not to enable these unconditionally in da850.dtsi (or rather 
not disabling them) instead of handling it per board ?

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  arch/arm/boot/dts/da850-lcdk.dts | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/da850-lcdk.dts
> b/arch/arm/boot/dts/da850-lcdk.dts index 4747629..b39796e 100644
> --- a/arch/arm/boot/dts/da850-lcdk.dts
> +++ b/arch/arm/boot/dts/da850-lcdk.dts
> @@ -243,3 +243,11 @@
>  		};
>  	};
>  };
> +
> +&mstpri {
> +	status = "okay";
> +};
> +
> +&ddrctl {
> +	status = "okay";
> +};

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/5] ARM: dts: da850-lcdk: enable mstpri and ddrctl nodes

[Kevin Hilman]

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> Hi Bartosz,
>
> Thank you for the patch.
>
> On Monday 31 Oct 2016 15:45:37 Bartosz Golaszewski wrote:
>> Enable the MSTPRI configuration and DDR2/mDDR memory controller
>> nodes on da850-lcdk. This is needed in order to adjust the memory
>> throughput constraints for better tilcdc support.
>
> Is there a reason not to enable these unconditionally in da850.dtsi (or rather 
> not disabling them) instead of handling it per board ?

Right.  They should be enabled by default in DT.  The drivers already
have board-specific compatible checks.

Kevin

Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Sekhar Nori]

On Monday 31 October 2016 08:15 PM, 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>

Reviewed-by: Sekhar Nori <nsekhar@ti.com>

Thanks,
Sekhar

Re: [PATCH v2 2/5] ARM: bus: da8xx-mstpri: new driver

[Sekhar Nori]

On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote:
> Create the driver for the da8xx master peripheral priority
> configuration and implement support for writing to the three
> Master Priority registers on da850 SoCs.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Reviewed-by: Sekhar Nori <nsekhar@ti.com>

Thanks,
Sekhar

Re: [PATCH v2 5/5] ARM: dts: da850-lcdk: add tilcdc panel node

[Sekhar Nori]

On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote:
> The tilcdc driver is not yet ready for working together with the
> dumb-vga-dac drm bridge. While the work on enabling drm_bridge
> support in tilcdc continues, enable the VGA connector on da850-lcdk
> with the following workaround: use the tilcdc-panel driver with
> a set of common (and tested) resolutions.
> 
> Once the drm bridge support is complete, we'll remove the node added
> by this patch and use the correct solution. This change will be
> transparent for the user.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

I dont think this can be applied. If this goes in v4.10, then the DT
blob becomes an ABI and driver will have to support tilcdc-panel driver
for DA850 forever.

Its probably better to wait for the proper driver support to arrive.

Thanks,
Sekhar

Re: [PATCH v2 5/5] ARM: dts: da850-lcdk: add tilcdc panel node

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 958 bytes --]

On 01/11/16 12:26, Sekhar Nori wrote:
> On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote:
>> The tilcdc driver is not yet ready for working together with the
>> dumb-vga-dac drm bridge. While the work on enabling drm_bridge
>> support in tilcdc continues, enable the VGA connector on da850-lcdk
>> with the following workaround: use the tilcdc-panel driver with
>> a set of common (and tested) resolutions.
>>
>> Once the drm bridge support is complete, we'll remove the node added
>> by this patch and use the correct solution. This change will be
>> transparent for the user.
>>
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> I dont think this can be applied. If this goes in v4.10, then the DT
> blob becomes an ABI and driver will have to support tilcdc-panel driver
> for DA850 forever.
> 
> Its probably better to wait for the proper driver support to arrive.

Agreed, NAK for this.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Kevin Hilman]

Bartosz Golaszewski <bgolaszewski@baylibre.com> writes:

> 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>

Reviewed-by: Kevin Hilman <khilman@baylibre.com>

Re: [PATCH v2 2/5] ARM: bus: da8xx-mstpri: new driver

[Kevin Hilman]

Bartosz Golaszewski <bgolaszewski@baylibre.com> writes:

> Create the driver for the da8xx master peripheral priority
> configuration and implement support for writing to the three
> Master Priority registers on da850 SoCs.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Reviewed-by: Kevin Hilman <khilman@baylibre.com>

Re: [PATCH v2 0/5] ARM: da850: new drivers for better LCDC support

[Sekhar Nori]

+ Arnd, Olof

On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote:
> This series adds two new drivers in order to better support the LCDC
> rev1 present on the da850 boards.
> 
> The first patch adds a new memory driver which allows to write to the
> DDR2/mDDR memory controller present on the da8xx SoCs.
> 
> The second patch adds a new bus driver which allows to interact with
> the MSTPRI registers of the SYSCFG0 module

I think patches 1/5 and 2/5 are ready to be merged. If there are no
objections I would like to send a pull request for them to be merged
through ARM-SoC tree for v4.10 kernel.

Thanks,
Sekhar

> 
> As is mentioned in the comments: we don't want to commit to supporting
> stable interfaces (DT bindings or sysfs attributes) so we hardcode the
> settings required by some boards (for now only da850-lcdk) with the
> hope that linux gets an appropriate framework for performance knobs
> in the future.
> 
> Potential extensions of these drivers should be straightforward in the
> future.
> 
> Subsequent patches add DT nodes for the new drivers: disabled nodes
> in da850.dtsi and enabled in da850-lcdk.dts.
> 
> The last patch adds a workaround for current lack of support for drm
> bridges in tilcdc.
> 
> Tested on a da850-lcdk with a display connected over VGA and two
> additional patches for tilcdc (sent to linux-drm): ran simple modetest
> for supported resolutions, used X.org and fluxbox as graphical
> environment, played video with mplayer.
> 
> v1 -> v2:
> - used regular readl()/writel() instead of __raw_** versions
> - used resource_size() instead of calculating the size by hand
> - used ioremap instead of syscon in patch [2/5]
> - added the DT nodes in patches [3/5]-[5/5]
> 
> Bartosz Golaszewski (5):
>   ARM: memory: da8xx-ddrctl: new driver
>   ARM: bus: da8xx-mstpri: new driver
>   ARM: dts: da850: add the mstpri and ddrctl nodes
>   ARM: dts: da850-lcdk: enable mstpri and ddrctl nodes
>   ARM: dts: da850-lcdk: add tilcdc panel node
> 
>  .../devicetree/bindings/bus/ti,da850-mstpri.txt    |  20 ++
>  .../memory-controllers/ti-da8xx-ddrctl.txt         |  20 ++
>  arch/arm/boot/dts/da850-lcdk.dts                   |  71 ++++++
>  arch/arm/boot/dts/da850.dtsi                       |  11 +
>  drivers/bus/Kconfig                                |   9 +
>  drivers/bus/Makefile                               |   2 +
>  drivers/bus/da8xx-mstpri.c                         | 269 +++++++++++++++++++++
>  drivers/memory/Kconfig                             |   8 +
>  drivers/memory/Makefile                            |   1 +
>  drivers/memory/da8xx-ddrctl.c                      | 175 ++++++++++++++
>  10 files changed, 586 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/bus/ti,da850-mstpri.txt
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>  create mode 100644 drivers/bus/da8xx-mstpri.c
>  create mode 100644 drivers/memory/da8xx-ddrctl.c
> 

Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Rob Herring]

On Mon, Oct 31, 2016 at 03:45:34PM +0100, 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 +++

Acked-by: Rob Herring <robh@kernel.org>

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

Re: [PATCH v2 2/5] ARM: bus: da8xx-mstpri: new driver

[Rob Herring]

On Mon, Oct 31, 2016 at 03:45:35PM +0100, Bartosz Golaszewski wrote:
> Create the driver for the da8xx master peripheral priority
> configuration and implement support for writing to the three
> Master Priority registers on da850 SoCs.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  .../devicetree/bindings/bus/ti,da850-mstpri.txt    |  20 ++

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/bus/Kconfig                                |   9 +
>  drivers/bus/Makefile                               |   2 +
>  drivers/bus/da8xx-mstpri.c                         | 269 +++++++++++++++++++++
>  4 files changed, 300 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/bus/ti,da850-mstpri.txt
>  create mode 100644 drivers/bus/da8xx-mstpri.c

Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Sekhar Nori]

On Wednesday 09 November 2016 11:54 PM, Rob Herring wrote:
> On Mon, Oct 31, 2016 at 03:45:34PM +0100, 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 +++
> 
> Acked-by: Rob Herring <robh@kernel.org>

Applied to v4.10/drivers

Thanks,
Sekhar

Re: [PATCH v2 2/5] ARM: bus: da8xx-mstpri: new driver

[Sekhar Nori]

On Wednesday 09 November 2016 11:54 PM, Rob Herring wrote:
> On Mon, Oct 31, 2016 at 03:45:35PM +0100, Bartosz Golaszewski wrote:
>> Create the driver for the da8xx master peripheral priority
>> configuration and implement support for writing to the three
>> Master Priority registers on da850 SoCs.
>>
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>>  .../devicetree/bindings/bus/ti,da850-mstpri.txt    |  20 ++
> 
> Acked-by: Rob Herring <robh@kernel.org>

Applied to v4.10/drivers

Thanks,
Sekhar

Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Sekhar Nori]

On Monday 31 October 2016 08:15 PM, 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;
> +	struct device_node *node;
> +	struct resource *res;
> +	void __iomem *ddrctl;
> +	struct device *dev;
> +	u32 reg;
> +
> +	dev = &pdev->dev;
> +	node = dev->of_node;
> +
> +	setting = da8xx_ddrctl_get_board_settings();
> +	if (!setting) {
> +		dev_err(dev, "no settings for board '%s'\n",
> +			of_flat_dt_get_machine_name());
> +		return -EINVAL;
> +	}

This causes a section mismatch because of_flat_dt_get_machine_name() 
has an __init annotation. I did not notice that before, sorry.

It can be fixed with a patch like below:

---8<---
diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
index a20e7bbbcbe0..9ca5aab3ac54 100644
--- a/drivers/memory/da8xx-ddrctl.c
+++ b/drivers/memory/da8xx-ddrctl.c
@@ -102,6 +102,18 @@ static const struct da8xx_ddrctl_setting *da8xx_ddrctl_get_board_settings(void)
 	return NULL;
 }
 
+static const char* da8xx_ddrctl_get_machine_name(void)
+{
+	const char *str;
+	int ret;
+
+	ret = of_property_read_string(of_root, "model", &str);
+	if (ret)
+		ret = of_property_read_string(of_root, "compatible", &str);
+
+	return str;
+}
+
 static int da8xx_ddrctl_probe(struct platform_device *pdev)
 {
 	const struct da8xx_ddrctl_config_knob *knob;
@@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device *pdev)
 	setting = da8xx_ddrctl_get_board_settings();
 	if (!setting) {
 		dev_err(dev, "no settings for board '%s'\n",
-			of_flat_dt_get_machine_name());
+			da8xx_ddrctl_get_machine_name());
 		return -EINVAL;
 	}
---8<--- 

A similar fix is required for the other driver in this series (patch 
2/5). I need some advise on whether I should introduce a common 
function to get the machine name post kernel boot-up (I cannot see an 
existing one). If yes, any advise on which file it should go into?

Thanks,
Sekhar

Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Bartosz Golaszewski]

2016-11-21 17:33 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
> On Monday 31 October 2016 08:15 PM, 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;
>> +     struct device_node *node;
>> +     struct resource *res;
>> +     void __iomem *ddrctl;
>> +     struct device *dev;
>> +     u32 reg;
>> +
>> +     dev = &pdev->dev;
>> +     node = dev->of_node;
>> +
>> +     setting = da8xx_ddrctl_get_board_settings();
>> +     if (!setting) {
>> +             dev_err(dev, "no settings for board '%s'\n",
>> +                     of_flat_dt_get_machine_name());
>> +             return -EINVAL;
>> +     }
>
> This causes a section mismatch because of_flat_dt_get_machine_name()
> has an __init annotation. I did not notice that before, sorry.
>
> It can be fixed with a patch like below:
>
> ---8<---
> diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
> index a20e7bbbcbe0..9ca5aab3ac54 100644
> --- a/drivers/memory/da8xx-ddrctl.c
> +++ b/drivers/memory/da8xx-ddrctl.c
> @@ -102,6 +102,18 @@ static const struct da8xx_ddrctl_setting *da8xx_ddrctl_get_board_settings(void)
>         return NULL;
>  }
>
> +static const char* da8xx_ddrctl_get_machine_name(void)
> +{
> +       const char *str;
> +       int ret;
> +
> +       ret = of_property_read_string(of_root, "model", &str);
> +       if (ret)
> +               ret = of_property_read_string(of_root, "compatible", &str);
> +
> +       return str;
> +}
> +
>  static int da8xx_ddrctl_probe(struct platform_device *pdev)
>  {
>         const struct da8xx_ddrctl_config_knob *knob;
> @@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device *pdev)
>         setting = da8xx_ddrctl_get_board_settings();
>         if (!setting) {
>                 dev_err(dev, "no settings for board '%s'\n",
> -                       of_flat_dt_get_machine_name());
> +                       da8xx_ddrctl_get_machine_name());
>                 return -EINVAL;
>         }
> ---8<---
>
> A similar fix is required for the other driver in this series (patch
> 2/5). I need some advise on whether I should introduce a common
> function to get the machine name post kernel boot-up (I cannot see an
> existing one). If yes, any advise on which file it should go into?
>

Hi Sekhar,

thanks for spotting that.

I think we should introduce this function right away, rather than
having two static functions doing the same thing. If you don't mind,
I'll try to find a good spot for it and send a follow-up series fixing
the issue.

Best regards,
Bartosz Golaszewski

Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Robin Murphy]

Hi Bartosz, Sekhar,

On 21/11/16 16:48, Bartosz Golaszewski wrote:
> 2016-11-21 17:33 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
>> On Monday 31 October 2016 08:15 PM, 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;
>>> +     struct device_node *node;
>>> +     struct resource *res;
>>> +     void __iomem *ddrctl;
>>> +     struct device *dev;
>>> +     u32 reg;
>>> +
>>> +     dev = &pdev->dev;
>>> +     node = dev->of_node;
>>> +
>>> +     setting = da8xx_ddrctl_get_board_settings();
>>> +     if (!setting) {
>>> +             dev_err(dev, "no settings for board '%s'\n",
>>> +                     of_flat_dt_get_machine_name());
>>> +             return -EINVAL;
>>> +     }
>>
>> This causes a section mismatch because of_flat_dt_get_machine_name()
>> has an __init annotation. I did not notice that before, sorry.
>>
>> It can be fixed with a patch like below:
>>
>> ---8<---
>> diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
>> index a20e7bbbcbe0..9ca5aab3ac54 100644
>> --- a/drivers/memory/da8xx-ddrctl.c
>> +++ b/drivers/memory/da8xx-ddrctl.c
>> @@ -102,6 +102,18 @@ static const struct da8xx_ddrctl_setting *da8xx_ddrctl_get_board_settings(void)
>>         return NULL;
>>  }
>>
>> +static const char* da8xx_ddrctl_get_machine_name(void)
>> +{
>> +       const char *str;
>> +       int ret;
>> +
>> +       ret = of_property_read_string(of_root, "model", &str);
>> +       if (ret)
>> +               ret = of_property_read_string(of_root, "compatible", &str);
>> +
>> +       return str;
>> +}
>> +
>>  static int da8xx_ddrctl_probe(struct platform_device *pdev)
>>  {
>>         const struct da8xx_ddrctl_config_knob *knob;
>> @@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device *pdev)
>>         setting = da8xx_ddrctl_get_board_settings();
>>         if (!setting) {
>>                 dev_err(dev, "no settings for board '%s'\n",
>> -                       of_flat_dt_get_machine_name());
>> +                       da8xx_ddrctl_get_machine_name());
>>                 return -EINVAL;
>>         }
>> ---8<---
>>
>> A similar fix is required for the other driver in this series (patch
>> 2/5). I need some advise on whether I should introduce a common
>> function to get the machine name post kernel boot-up (I cannot see an
>> existing one). If yes, any advise on which file it should go into?
>>
> 
> Hi Sekhar,
> 
> thanks for spotting that.
> 
> I think we should introduce this function right away, rather than
> having two static functions doing the same thing. If you don't mind,
> I'll try to find a good spot for it and send a follow-up series fixing
> the issue.

As it happens, that was already proposed last week, for much the same
reason:

http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg111395.html

Robin.

> 
> Best regards,
> Bartosz Golaszewski
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Sudeep Holla]

Hi Robin,

On 21/11/16 17:47, Robin Murphy wrote:
> Hi Bartosz, Sekhar,
>
> On 21/11/16 16:48, Bartosz Golaszewski wrote:

[...]

>> Hi Sekhar,
>>
>> thanks for spotting that.
>>
>> I think we should introduce this function right away, rather than
>> having two static functions doing the same thing. If you don't mind,
>> I'll try to find a good spot for it and send a follow-up series fixing
>> the issue.
>
> As it happens, that was already proposed last week, for much the same
> reason:
>
> http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg111395.html
>

Thanks for pointing this out, yet another platform to move to the new
API after v4.10.

Hi Shekar, Bartosz,

For v4.10, please continue with the open coding as proposed in this
thread in order to avoid cross tree dependencies. I will rework on the
above patch once v4.10 merge window closes to include all these
occurrence and replace them.

-- 
Regards,
Sudeep

Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Frank Rowand]

Hi Sekhar,

(And adding Sudeep since he becomes involved in this further
down thread and at that point says he will re-work this
proposed work around in a manner that is incorrect in a
manner that is similar to this proposed work around.)

On 11/21/16 08:33, Sekhar Nori wrote:
> On Monday 31 October 2016 08:15 PM, 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;
>> +	struct device_node *node;
>> +	struct resource *res;
>> +	void __iomem *ddrctl;
>> +	struct device *dev;
>> +	u32 reg;
>> +
>> +	dev = &pdev->dev;
>> +	node = dev->of_node;
>> +
>> +	setting = da8xx_ddrctl_get_board_settings();
>> +	if (!setting) {
>> +		dev_err(dev, "no settings for board '%s'\n",
>> +			of_flat_dt_get_machine_name());
>> +		return -EINVAL;
>> +	}
> 
> This causes a section mismatch because of_flat_dt_get_machine_name() 
> has an __init annotation. I did not notice that before, sorry.
> 
> It can be fixed with a patch like below:
> 
> ---8<---
> diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
> index a20e7bbbcbe0..9ca5aab3ac54 100644
> --- a/drivers/memory/da8xx-ddrctl.c
> +++ b/drivers/memory/da8xx-ddrctl.c
> @@ -102,6 +102,18 @@ static const struct da8xx_ddrctl_setting *da8xx_ddrctl_get_board_settings(void)
>  	return NULL;
>  }
>  
> +static const char* da8xx_ddrctl_get_machine_name(void)
> +{
> +	const char *str;
> +	int ret;
> +
> +	ret = of_property_read_string(of_root, "model", &str);
> +	if (ret)
> +		ret = of_property_read_string(of_root, "compatible", &str);
> +
> +	return str;
> +}
> +
>  static int da8xx_ddrctl_probe(struct platform_device *pdev)
>  {
>  	const struct da8xx_ddrctl_config_knob *knob;
> @@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device *pdev)
>  	setting = da8xx_ddrctl_get_board_settings();
>  	if (!setting) {
>  		dev_err(dev, "no settings for board '%s'\n",
> -			of_flat_dt_get_machine_name());

da8xx_ddrctl_get_board_settings() tries to match based on the "compatible"
property in the root node.  The "model" property in the root node has
nothing to do with the failure to match. So creating and then using
da8xx_ddrctl_get_machine_name() to potentially report model is not useful.

It should be sufficient to simply report that no compatible matched.


> +			da8xx_ddrctl_get_machine_name());
>  		return -EINVAL;
>  	}
> ---8<--- 
> 
> A similar fix is required for the other driver in this series (patch 
> 2/5). I need some advise on whether I should introduce a common 
> function to get the machine name post kernel boot-up (I cannot see an 
> existing one). If yes, any advise on which file it should go into?
> 
> Thanks,
> Sekhar
> 
> 

Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Sekhar Nori]

Hi Frank,

On Tuesday 22 November 2016 07:13 AM, Frank Rowand wrote:
> On 11/21/16 08:33, Sekhar Nori wrote:
>> On Monday 31 October 2016 08:15 PM, 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;
>>> +	struct device_node *node;
>>> +	struct resource *res;
>>> +	void __iomem *ddrctl;
>>> +	struct device *dev;
>>> +	u32 reg;
>>> +
>>> +	dev = &pdev->dev;
>>> +	node = dev->of_node;
>>> +
>>> +	setting = da8xx_ddrctl_get_board_settings();
>>> +	if (!setting) {
>>> +		dev_err(dev, "no settings for board '%s'\n",
>>> +			of_flat_dt_get_machine_name());
>>> +		return -EINVAL;
>>> +	}
>>
>> This causes a section mismatch because of_flat_dt_get_machine_name() 
>> has an __init annotation. I did not notice that before, sorry.
>>
>> It can be fixed with a patch like below:
>>
>> ---8<---
>> diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
>> index a20e7bbbcbe0..9ca5aab3ac54 100644
>> --- a/drivers/memory/da8xx-ddrctl.c
>> +++ b/drivers/memory/da8xx-ddrctl.c
>> @@ -102,6 +102,18 @@ static const struct da8xx_ddrctl_setting *da8xx_ddrctl_get_board_settings(void)
>>  	return NULL;
>>  }
>>  
>> +static const char* da8xx_ddrctl_get_machine_name(void)
>> +{
>> +	const char *str;
>> +	int ret;
>> +
>> +	ret = of_property_read_string(of_root, "model", &str);
>> +	if (ret)
>> +		ret = of_property_read_string(of_root, "compatible", &str);
>> +
>> +	return str;
>> +}
>> +
>>  static int da8xx_ddrctl_probe(struct platform_device *pdev)
>>  {
>>  	const struct da8xx_ddrctl_config_knob *knob;
>> @@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device *pdev)
>>  	setting = da8xx_ddrctl_get_board_settings();
>>  	if (!setting) {
>>  		dev_err(dev, "no settings for board '%s'\n",
>> -			of_flat_dt_get_machine_name());
> 
> da8xx_ddrctl_get_board_settings() tries to match based on the "compatible"
> property in the root node.  The "model" property in the root node has
> nothing to do with the failure to match. So creating and then using
> da8xx_ddrctl_get_machine_name() to potentially report model is not useful.
> 
> It should be sufficient to simply report that no compatible matched.

I agree with you on this. Even if model name is printed, you will have
to go back and check the compatible anyway. But I think it will be
useful to print the compatible instead of just reporting that nothing
matched.

Bartosz, if you agree too, could you send a fix patch just printing the
compatible?

Thanks,
Sekhar

Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Frank Rowand]

On 11/21/16 22:25, Sekhar Nori wrote:
> Hi Frank,
> 
> On Tuesday 22 November 2016 07:13 AM, Frank Rowand wrote:
>> On 11/21/16 08:33, Sekhar Nori wrote:
>>> On Monday 31 October 2016 08:15 PM, 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;
>>>> +	struct device_node *node;
>>>> +	struct resource *res;
>>>> +	void __iomem *ddrctl;
>>>> +	struct device *dev;
>>>> +	u32 reg;
>>>> +
>>>> +	dev = &pdev->dev;
>>>> +	node = dev->of_node;
>>>> +
>>>> +	setting = da8xx_ddrctl_get_board_settings();
>>>> +	if (!setting) {
>>>> +		dev_err(dev, "no settings for board '%s'\n",
>>>> +			of_flat_dt_get_machine_name());
>>>> +		return -EINVAL;
>>>> +	}
>>>
>>> This causes a section mismatch because of_flat_dt_get_machine_name() 
>>> has an __init annotation. I did not notice that before, sorry.
>>>
>>> It can be fixed with a patch like below:
>>>
>>> ---8<---
>>> diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
>>> index a20e7bbbcbe0..9ca5aab3ac54 100644
>>> --- a/drivers/memory/da8xx-ddrctl.c
>>> +++ b/drivers/memory/da8xx-ddrctl.c
>>> @@ -102,6 +102,18 @@ static const struct da8xx_ddrctl_setting *da8xx_ddrctl_get_board_settings(void)
>>>  	return NULL;
>>>  }
>>>  
>>> +static const char* da8xx_ddrctl_get_machine_name(void)
>>> +{
>>> +	const char *str;
>>> +	int ret;
>>> +
>>> +	ret = of_property_read_string(of_root, "model", &str);
>>> +	if (ret)
>>> +		ret = of_property_read_string(of_root, "compatible", &str);
>>> +
>>> +	return str;
>>> +}
>>> +
>>>  static int da8xx_ddrctl_probe(struct platform_device *pdev)
>>>  {
>>>  	const struct da8xx_ddrctl_config_knob *knob;
>>> @@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device *pdev)
>>>  	setting = da8xx_ddrctl_get_board_settings();
>>>  	if (!setting) {
>>>  		dev_err(dev, "no settings for board '%s'\n",
>>> -			of_flat_dt_get_machine_name());
>>
>> da8xx_ddrctl_get_board_settings() tries to match based on the "compatible"
>> property in the root node.  The "model" property in the root node has
>> nothing to do with the failure to match. So creating and then using
>> da8xx_ddrctl_get_machine_name() to potentially report model is not useful.
>>
>> It should be sufficient to simply report that no compatible matched.
> 
> I agree with you on this. Even if model name is printed, you will have
> to go back and check the compatible anyway. But I think it will be
> useful to print the compatible instead of just reporting that nothing
> matched.
> 
> Bartosz, if you agree too, could you send a fix patch just printing the
> compatible?

Please note that the compatible property might contain several strings, not just
a single string.

> 
> Thanks,
> Sekhar
> 

Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Sekhar Nori]

On Tuesday 22 November 2016 11:51 PM, Frank Rowand wrote:
> Please note that the compatible property might contain several strings, not just
> a single string.

So I guess the best thing to do is to use
of_property_read_string_index() and print the sting at index 0.

Thanks,
Sekhar

Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Sudeep Holla]

On 22/11/16 01:43, Frank Rowand wrote:
> Hi Sekhar,
>
> (And adding Sudeep since he becomes involved in this further
> down thread and at that point says he will re-work this
> proposed work around in a manner that is incorrect in a
> manner that is similar to this proposed work around.)
>
> On 11/21/16 08:33, Sekhar Nori wrote:


[...]

>>  static int da8xx_ddrctl_probe(struct platform_device *pdev)
>>  {
>>  	const struct da8xx_ddrctl_config_knob *knob;
>> @@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device *pdev)
>>  	setting = da8xx_ddrctl_get_board_settings();
>>  	if (!setting) {
>>  		dev_err(dev, "no settings for board '%s'\n",
>> -			of_flat_dt_get_machine_name());
>
> da8xx_ddrctl_get_board_settings() tries to match based on the "compatible"
> property in the root node.  The "model" property in the root node has
> nothing to do with the failure to match. So creating and then using
> da8xx_ddrctl_get_machine_name() to potentially report model is not useful.
>
> It should be sufficient to simply report that no compatible matched.
>

Agreed.

-- 
Regards,
Sudeep

Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Frank Rowand]

On 11/22/16 21:55, Sekhar Nori wrote:
> On Tuesday 22 November 2016 11:51 PM, Frank Rowand wrote:
>> Please note that the compatible property might contain several strings, not just
>> a single string.
> 
> So I guess the best thing to do is to use
> of_property_read_string_index() and print the sting at index 0.
> 
> Thanks,
> Sekhar

If you want to print just one compatible value, you could use that method.

To give all of the information needed to understand the problem, the error
message would need to include all of the strings contained in the compatible
property and all of the .board values in the da8xx_ddrctl_board_confs[] array
(currently only one entry, but coded to allow additional entries in the
future).

It is hard to justify an error message that complex.

I would just print an error that no match was found.

-Frank

Re: [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Frank Rowand]

On 11/23/16 10:13, Frank Rowand wrote:
> On 11/22/16 21:55, Sekhar Nori wrote:
>> On Tuesday 22 November 2016 11:51 PM, Frank Rowand wrote:
>>> Please note that the compatible property might contain several strings, not just
>>> a single string.
>>
>> So I guess the best thing to do is to use
>> of_property_read_string_index() and print the sting at index 0.
>>
>> Thanks,
>> Sekhar
> 
> If you want to print just one compatible value, you could use that method.
> 
> To give all of the information needed to understand the problem, the error
> message would need to include all of the strings contained in the compatible
> property and all of the .board values in the da8xx_ddrctl_board_confs[] array
> (currently only one entry, but coded to allow additional entries in the
> future).
> 
> It is hard to justify an error message that complex.
> 
> I would just print an error that no match was found.
> 
> -Frank

I just needed to read some more emails.  I see this approach was taken
in the "[PATCH v4 0/2] da8xx: fix section mismatch in new drivers"
series.

-Frank