| Message ID | 1477327596-16060-2-git-send-email-bgolaszewski@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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
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
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
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");
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