* [RFC] da850: DDR2/mDDR memory controller driver @ 2016-10-24 16:46 Bartosz Golaszewski 2016-10-24 16:46 ` [RFC] ARM: memory: da8xx-ddrctl: new driver Bartosz Golaszewski 0 siblings, 1 reply; 8+ messages in thread From: Bartosz Golaszewski @ 2016-10-24 16:46 UTC (permalink / raw) To: Kevin Hilman, Michael Turquette, Sekhar Nori, Rob Herring, Frank Rowand, Mark Rutland, Peter Ujfalusi, Russell King Cc: linux-devicetree, LKML, linux-drm, Bartosz Golaszewski, Tomi Valkeinen, Jyri Sarha, arm-soc, Laurent Pinchart 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC] ARM: memory: da8xx-ddrctl: new driver 2016-10-24 16:46 [RFC] da850: DDR2/mDDR memory controller driver Bartosz Golaszewski @ 2016-10-24 16:46 ` Bartosz Golaszewski [not found] ` <1477327596-16060-2-git-send-email-bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> 0 siblings, 1 reply; 8+ messages in thread From: Bartosz Golaszewski @ 2016-10-24 16:46 UTC (permalink / raw) To: Kevin Hilman, Michael Turquette, Sekhar Nori, Rob Herring, Frank Rowand, Mark Rutland, Peter Ujfalusi, Russell King Cc: linux-devicetree, LKML, linux-drm, Bartosz Golaszewski, Tomi Valkeinen, Jyri Sarha, arm-soc, Laurent Pinchart 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
[parent not found: <1477327596-16060-2-git-send-email-bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>]
* Re: [RFC] ARM: memory: da8xx-ddrctl: new driver [not found] ` <1477327596-16060-2-git-send-email-bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> @ 2016-10-24 17:00 ` Mark Rutland 2016-10-24 17:35 ` Kevin Hilman 2016-10-25 10:17 ` Bartosz Golaszewski 0 siblings, 2 replies; 8+ messages in thread From: Mark Rutland @ 2016-10-24 17:00 UTC (permalink / raw) To: Bartosz Golaszewski Cc: Kevin Hilman, Michael Turquette, Sekhar Nori, Rob Herring, Frank Rowand, Peter Ujfalusi, Russell King, LKML, arm-soc, linux-drm, linux-devicetree, Jyri Sarha, Tomi Valkeinen, David Airlie, Laurent Pinchart 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] ARM: memory: da8xx-ddrctl: new driver 2016-10-24 17:00 ` Mark Rutland @ 2016-10-24 17:35 ` Kevin Hilman 2016-10-24 17:42 ` Mark Rutland 2016-10-25 10:17 ` Bartosz Golaszewski 1 sibling, 1 reply; 8+ messages in thread From: Kevin Hilman @ 2016-10-24 17:35 UTC (permalink / raw) To: Mark Rutland Cc: Bartosz Golaszewski, Michael Turquette, Sekhar Nori, Rob Herring, Frank Rowand, Peter Ujfalusi, Russell King, LKML, arm-soc, linux-drm, linux-devicetree, Jyri Sarha, Tomi Valkeinen, David Airlie, Laurent Pinchart 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] ARM: memory: da8xx-ddrctl: new driver 2016-10-24 17:35 ` Kevin Hilman @ 2016-10-24 17:42 ` Mark Rutland 2016-10-24 18:41 ` Kevin Hilman 0 siblings, 1 reply; 8+ messages in thread From: Mark Rutland @ 2016-10-24 17:42 UTC (permalink / raw) To: Kevin Hilman Cc: linux-devicetree, David Airlie, Tomi Valkeinen, Michael Turquette, Sekhar Nori, Russell King, linux-drm, LKML, Bartosz Golaszewski, Rob Herring, Jyri Sarha, Frank Rowand, arm-soc, Laurent Pinchart 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] ARM: memory: da8xx-ddrctl: new driver 2016-10-24 17:42 ` Mark Rutland @ 2016-10-24 18:41 ` Kevin Hilman 2016-10-24 18:52 ` Kevin Hilman 0 siblings, 1 reply; 8+ messages in thread From: Kevin Hilman @ 2016-10-24 18:41 UTC (permalink / raw) To: Mark Rutland Cc: linux-devicetree, Tomi Valkeinen, Michael Turquette, Sekhar Nori, Russell King, linux-drm, LKML, Peter Ujfalusi, Bartosz Golaszewski, Rob Herring, Jyri Sarha, Frank Rowand, arm-soc, Laurent Pinchart 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] ARM: memory: da8xx-ddrctl: new driver 2016-10-24 18:41 ` Kevin Hilman @ 2016-10-24 18:52 ` Kevin Hilman 0 siblings, 0 replies; 8+ messages in thread From: Kevin Hilman @ 2016-10-24 18:52 UTC (permalink / raw) To: Mark Rutland Cc: linux-devicetree, Tomi Valkeinen, Michael Turquette, Sekhar Nori, Russell King, linux-drm, LKML, Peter Ujfalusi, Bartosz Golaszewski, Rob Herring, Jyri Sarha, Frank Rowand, arm-soc, Laurent Pinchart 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] ARM: memory: da8xx-ddrctl: new driver 2016-10-24 17:00 ` Mark Rutland 2016-10-24 17:35 ` Kevin Hilman @ 2016-10-25 10:17 ` Bartosz Golaszewski 1 sibling, 0 replies; 8+ messages in thread From: Bartosz Golaszewski @ 2016-10-25 10:17 UTC (permalink / raw) To: Mark Rutland Cc: Kevin Hilman, Michael Turquette, Sekhar Nori, Rob Herring, Frank Rowand, Peter Ujfalusi, Russell King, LKML, arm-soc, linux-drm, linux-devicetree, Jyri Sarha, Tomi Valkeinen, David Airlie, Laurent Pinchart 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-10-25 10:17 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-24 16:46 [RFC] da850: DDR2/mDDR memory controller driver Bartosz Golaszewski
2016-10-24 16:46 ` [RFC] ARM: memory: da8xx-ddrctl: new driver Bartosz Golaszewski
[not found] ` <1477327596-16060-2-git-send-email-bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
2016-10-24 17:00 ` Mark Rutland
2016-10-24 17:35 ` Kevin Hilman
2016-10-24 17:42 ` Mark Rutland
2016-10-24 18:41 ` Kevin Hilman
2016-10-24 18:52 ` Kevin Hilman
2016-10-25 10:17 ` Bartosz Golaszewski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).