Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941716AbcJYR5m (ORCPT ); Tue, 25 Oct 2016 13:57:42 -0400 Received: from mail-pf0-f173.google.com ([209.85.192.173]:35725 "EHLO mail-pf0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752236AbcJYR5j (ORCPT ); Tue, 25 Oct 2016 13:57:39 -0400 From: Kevin Hilman To: Bartosz Golaszewski Cc: Michael Turquette , Sekhar Nori , Rob Herring , Frank Rowand , Mark Rutland , Peter Ujfalusi , Russell King , LKML , arm-soc , linux-drm , linux-devicetree , Jyri Sarha , Tomi Valkeinen , David Airlie , Laurent Pinchart Subject: Re: [RFC v2] ARM: memory: da8xx-ddrctl: new driver Organization: BayLibre References: <1477402876-22472-1-git-send-email-bgolaszewski@baylibre.com> <1477402876-22472-2-git-send-email-bgolaszewski@baylibre.com> Date: Tue, 25 Oct 2016 10:57:37 -0700 In-Reply-To: <1477402876-22472-2-git-send-email-bgolaszewski@baylibre.com> (Bartosz Golaszewski's message of "Tue, 25 Oct 2016 15:41:16 +0200") Message-ID: <7hy41ctipa.fsf@baylibre.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8682 Lines: 278 Bartosz Golaszewski 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 > --- > .../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..7e271dd > --- /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 0x100>; > +}; Axel's series for the USB PHY reminded me that the PHY also has some config registers in this same area, and his series creates a syscon for a similar range of registers. Could you create a syscon for the SYSCFG0 registers, which would then be used by ths driver and your other drivers/bus driver? Then the binding would just reference the sysconf via phandle, and your driver can use syscon_regmap_lookup_by_phandle() > 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..66022df > --- /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 > + * > + * 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 > +#include > +#include > +#include > +#include > +#include > + > +/* > + * 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 offset; nit: call this shift instead, which will also map well onto the regmap accessors (which you'll use when switching to syscon.) > +}; > + > +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_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[0]; Looks like that '0' should be '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 > (res->end - res->start - 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); nit: I don't think you need the __raw accessors here. But in any case, moving to syscon you'll be using regmap accessors, and this can just be converted to a single regmap_update_bits. Kevin > + } > + > + 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 "); > +MODULE_DESCRIPTION("TI da8xx DDR2/mDDR controller driver"); > +MODULE_LICENSE("GPL v2");