[PATCH 0/3] Add Broadcom STB memory controller driver

[PATCH 0/3] Add Broadcom STB memory controller driver

[Florian Fainelli]

Hi Krzysztof,

This small patch series adds basic support for controlling self-refresh
power down on Broadcom STB memory controllers. We might be able to
contribute more features to the memory controller driver in the future
like accurate reporting of the memory type, timings, and possibly some
performance counters.

Thans!

Florian Fainelli (3):
  dt-bindings: memory-controller: Document Broadcom STB MEMC
  dt-bindings: arm: bcm: Refer to the YAML binding for MEMC
  memory: Add Broadcom STB memory controller driver

 .../bindings/arm/bcm/brcm,brcmstb.txt         |  11 +-
 .../memory-controllers/brcm,memc.yaml         |  49 +++
 drivers/memory/Kconfig                        |   9 +
 drivers/memory/Makefile                       |   1 +
 drivers/memory/brcmstb_memc.c                 | 304 ++++++++++++++++++
 5 files changed, 365 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/brcm,memc.yaml
 create mode 100644 drivers/memory/brcmstb_memc.c

-- 
2.25.1

[PATCH 1/3] dt-bindings: memory-controller: Document Broadcom STB MEMC

[Florian Fainelli]

Document the Broadcom STB memory controller which is a trivial binding
for now with a set of compatible strings and single register.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 .../memory-controllers/brcm,memc.yaml         | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/brcm,memc.yaml

diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,memc.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,memc.yaml
new file mode 100644
index 000000000000..a629734f0cd0
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/brcm,memc.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/memory-controllers/brcm,memc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Memory controller (MEMC) for Broadcom STB
+
+maintainers:
+  - Florian Fainelli <f.fainelli@gmail.com>
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - brcm,brcmstb-memc-ddr-rev-b.1.x
+          - brcm,brcmstb-memc-ddr-rev-b.2.0
+          - brcm,brcmstb-memc-ddr-rev-b.2.1
+          - brcm,brcmstb-memc-ddr-rev-b.2.2
+          - brcm,brcmstb-memc-ddr-rev-b.2.3
+          - brcm,brcmstb-memc-ddr-rev-b.2.5
+          - brcm,brcmstb-memc-ddr-rev-b.2.6
+          - brcm,brcmstb-memc-ddr-rev-b.2.7
+          - brcm,brcmstb-memc-ddr-rev-b.2.8
+          - brcm,brcmstb-memc-ddr-rev-b.3.0
+          - brcm,brcmstb-memc-ddr-rev-b.3.1
+          - brcm,brcmstb-memc-ddr-rev-c.1.0
+          - brcm,brcmstb-memc-ddr-rev-c.1.1
+          - brcm,brcmstb-memc-ddr-rev-c.1.2
+          - brcm,brcmstb-memc-ddr-rev-c.1.3
+          - brcm,brcmstb-memc-ddr-rev-c.1.4
+      - const: brcm,brcmstb-memc-ddr
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    memory-controller@9902000 {
+        compatible = "brcm,brcmstb-memc-ddr-rev-c.1.1",
+                        "brcm,brcmstb-memc-ddr";
+        reg = <0x9902000 0x600>;
+    };
-- 
2.25.1

Re: [PATCH 1/3] dt-bindings: memory-controller: Document Broadcom STB MEMC

[Krzysztof Kozlowski]

On 22/07/2022 20:41, Florian Fainelli wrote:
> Document the Broadcom STB memory controller which is a trivial binding
> for now with a set of compatible strings and single register.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  .../memory-controllers/brcm,memc.yaml         | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/brcm,memc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,memc.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,memc.yaml
> new file mode 100644
> index 000000000000..a629734f0cd0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/brcm,memc.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/memory-controllers/brcm,memc.yaml#

Filename like first compatible, so: brcm,brcmstb-memc-ddr.yaml

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Memory controller (MEMC) for Broadcom STB
> +
> +maintainers:
> +  - Florian Fainelli <f.fainelli@gmail.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - brcm,brcmstb-memc-ddr-rev-b.1.x
> +          - brcm,brcmstb-memc-ddr-rev-b.2.0
> +          - brcm,brcmstb-memc-ddr-rev-b.2.1
> +          - brcm,brcmstb-memc-ddr-rev-b.2.2
> +          - brcm,brcmstb-memc-ddr-rev-b.2.3
> +          - brcm,brcmstb-memc-ddr-rev-b.2.5
> +          - brcm,brcmstb-memc-ddr-rev-b.2.6
> +          - brcm,brcmstb-memc-ddr-rev-b.2.7
> +          - brcm,brcmstb-memc-ddr-rev-b.2.8
> +          - brcm,brcmstb-memc-ddr-rev-b.3.0
> +          - brcm,brcmstb-memc-ddr-rev-b.3.1
> +          - brcm,brcmstb-memc-ddr-rev-c.1.0
> +          - brcm,brcmstb-memc-ddr-rev-c.1.1
> +          - brcm,brcmstb-memc-ddr-rev-c.1.2
> +          - brcm,brcmstb-memc-ddr-rev-c.1.3
> +          - brcm,brcmstb-memc-ddr-rev-c.1.4
> +      - const: brcm,brcmstb-memc-ddr
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    memory-controller@9902000 {
> +        compatible = "brcm,brcmstb-memc-ddr-rev-c.1.1",
> +                        "brcm,brcmstb-memc-ddr";

Could you align it with previous quote "?

Other than that, looks ok, but anyway this will have to wait for merge
window to finish.

> +        reg = <0x9902000 0x600>;
> +    };


Best regards,
Krzysztof

[PATCH 2/3] dt-bindings: arm: bcm: Refer to the YAML binding for MEMC

[Florian Fainelli]

Remove the list of existing compatible strings and prefer to use the
YAML binding which more complete and up to date.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 .../devicetree/bindings/arm/bcm/brcm,brcmstb.txt      | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/bcm/brcm,brcmstb.txt b/Documentation/devicetree/bindings/arm/bcm/brcm,brcmstb.txt
index 104cc9b41df4..4db7209d5be3 100644
--- a/Documentation/devicetree/bindings/arm/bcm/brcm,brcmstb.txt
+++ b/Documentation/devicetree/bindings/arm/bcm/brcm,brcmstb.txt
@@ -187,15 +187,8 @@ Required properties:
 Sequencer DRAM parameters and control registers. Used for Self-Refresh
 Power-Down (SRPD), among other things.
 
-Required properties:
-- compatible     : should contain one of these
-	"brcm,brcmstb-memc-ddr-rev-b.2.1"
-	"brcm,brcmstb-memc-ddr-rev-b.2.2"
-	"brcm,brcmstb-memc-ddr-rev-b.2.3"
-	"brcm,brcmstb-memc-ddr-rev-b.3.0"
-	"brcm,brcmstb-memc-ddr-rev-b.3.1"
-	"brcm,brcmstb-memc-ddr"
-- reg            : the MEMC DDR register range
+See Documentation/devicetree/bindings/memory-controller/brcm,memc.yaml for
+a full list of supported compatible strings.
 
 Example:
 
-- 
2.25.1

Re: [PATCH 2/3] dt-bindings: arm: bcm: Refer to the YAML binding for MEMC

[Krzysztof Kozlowski]

On 22/07/2022 20:41, Florian Fainelli wrote:
> Remove the list of existing compatible strings and prefer to use the
> YAML binding which more complete and up to date.

Ah, so this is partial conversion from TXT? Then please squash it with
previous commit and extend the commit msg.

> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  .../devicetree/bindings/arm/bcm/brcm,brcmstb.txt      | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 


Best regards,
Krzysztof

[PATCH 3/3] memory: Add Broadcom STB memory controller driver

[Florian Fainelli]

Add support for configuring the Self Refresh Power Down (SRPD)
inactivity timeout on Broadcom STB chips. This is used to conserve power
when the DRAM activity is reduced.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/memory/Kconfig        |   9 +
 drivers/memory/Makefile       |   1 +
 drivers/memory/brcmstb_memc.c | 304 ++++++++++++++++++++++++++++++++++
 3 files changed, 314 insertions(+)
 create mode 100644 drivers/memory/brcmstb_memc.c

diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index ac1a411648d8..fac290e48e0b 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -66,6 +66,15 @@ config BRCMSTB_DPFE
 	  for the DRAM's temperature. Slower refresh rate means cooler RAM,
 	  higher refresh rate means hotter RAM.
 
+config BRCMSTB_MEMC
+	tristate "Broadcom STB MEMC driver"
+	default ARCH_BRCMSTB
+	depends on ARCH_BRCMSTB || COMPILE_TEST
+	help
+	  This driver provides a way to configure the Broadcom STB memory
+	  controller and specifically control the Self Refresh Power Down
+	  (SRPD) inactivity timeout.
+
 config BT1_L2_CTL
 	bool "Baikal-T1 CM2 L2-RAM Cache Control Block"
 	depends on MIPS_BAIKAL_T1 || COMPILE_TEST
diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index bc7663ed1c25..e148f636c082 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_ARM_PL172_MPMC)	+= pl172.o
 obj-$(CONFIG_ATMEL_SDRAMC)	+= atmel-sdramc.o
 obj-$(CONFIG_ATMEL_EBI)		+= atmel-ebi.o
 obj-$(CONFIG_BRCMSTB_DPFE)	+= brcmstb_dpfe.o
+obj-$(CONFIG_BRCMSTB_MEMC)	+= brcmstb_memc.o
 obj-$(CONFIG_BT1_L2_CTL)	+= bt1-l2-ctl.o
 obj-$(CONFIG_TI_AEMIF)		+= ti-aemif.o
 obj-$(CONFIG_TI_EMIF)		+= emif.o
diff --git a/drivers/memory/brcmstb_memc.c b/drivers/memory/brcmstb_memc.c
new file mode 100644
index 000000000000..881da958c542
--- /dev/null
+++ b/drivers/memory/brcmstb_memc.c
@@ -0,0 +1,304 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * DDR Self-Refresh Power Down (SRPD) support for Broadcom STB SoCs
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/of_device.h>
+#include <linux/io.h>
+
+#define REG_MEMC_CNTRLR_CONFIG		0x00
+#define  CNTRLR_CONFIG_LPDDR4		5
+#define  CNTRLR_CONFIG_MASK		0xf
+#define REG_MEMC_SRPD_CFG_21		0x20
+#define REG_MEMC_SRPD_CFG_20		0x34
+#define REG_MEMC_SRPD_CFG_1x		0x3c
+#define INACT_COUNT_SHIFT		0
+#define INACT_COUNT_MASK		0xffff
+#define SRPD_EN_SHIFT			16
+
+struct brcmstb_memc_data {
+	u32 srpd_offset;
+};
+
+struct brcmstb_memc {
+	struct device *dev;
+	void __iomem *ddr_ctrl;
+	unsigned int timeout_cycles;
+	u32 frequency;
+	u32 srpd_offset;
+};
+
+static int brcmstb_memc_uses_lpddr4(struct brcmstb_memc *memc)
+{
+	void __iomem *config = memc->ddr_ctrl + REG_MEMC_CNTRLR_CONFIG;
+	u32 reg;
+
+	reg = readl_relaxed(config) & CNTRLR_CONFIG_MASK;
+
+	return reg == CNTRLR_CONFIG_LPDDR4;
+}
+
+static int brcmstb_memc_srpd_config(struct brcmstb_memc *memc,
+				    unsigned int cycles)
+{
+	void __iomem *cfg = memc->ddr_ctrl + memc->srpd_offset;
+	u32 val;
+
+	/* Max timeout supported in HW */
+	if (cycles > INACT_COUNT_MASK)
+		return -EINVAL;
+
+	memc->timeout_cycles = cycles;
+
+	val = (cycles << INACT_COUNT_SHIFT) & INACT_COUNT_MASK;
+	if (cycles)
+		val |= (1 << SRPD_EN_SHIFT);
+
+	writel_relaxed(val, cfg);
+	(void)readl_relaxed(cfg);
+
+	return 0;
+}
+
+static ssize_t frequency_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct brcmstb_memc *memc = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", memc->frequency);
+}
+
+static ssize_t srpd_show(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	struct brcmstb_memc *memc = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", memc->timeout_cycles);
+}
+
+static ssize_t srpd_store(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t count)
+{
+	struct brcmstb_memc *memc = dev_get_drvdata(dev);
+	unsigned int val;
+	int ret;
+
+	/* Cannot change the inactivity timeout on LPDDR4 chips because the
+	 * dynamic tuning process will also get affected by the inactivity
+	 * timeout, thus making it non functional.
+	 */
+	if (brcmstb_memc_uses_lpddr4(memc))
+		return -EOPNOTSUPP;
+
+	ret = kstrtouint(buf, 10, &val);
+	if (ret < 0)
+		return ret;
+
+	ret = brcmstb_memc_srpd_config(memc, val);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static DEVICE_ATTR_RO(frequency);
+static DEVICE_ATTR_RW(srpd);
+
+static struct attribute *dev_attrs[] = {
+	&dev_attr_frequency.attr,
+	&dev_attr_srpd.attr,
+	NULL,
+};
+
+static struct attribute_group dev_attr_group = {
+	.attrs = dev_attrs,
+};
+
+static const struct of_device_id brcmstb_memc_of_match[];
+
+static int brcmstb_memc_probe(struct platform_device *pdev)
+{
+	const struct brcmstb_memc_data *memc_data;
+	const struct of_device_id *of_id;
+	struct device *dev = &pdev->dev;
+	struct brcmstb_memc *memc;
+	struct resource *res;
+	int ret;
+
+	memc = devm_kzalloc(dev, sizeof(*memc), GFP_KERNEL);
+	if (!memc)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, memc);
+
+	of_id = of_match_device(brcmstb_memc_of_match, dev);
+	if (!of_id || !of_id->data)
+		return -EINVAL;
+
+	memc_data = of_id->data;
+	memc->srpd_offset = memc_data->srpd_offset;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	memc->ddr_ctrl = devm_ioremap_resource(dev, res);
+	if (IS_ERR(memc->ddr_ctrl))
+		return PTR_ERR(memc->ddr_ctrl);
+
+	of_property_read_u32(pdev->dev.of_node, "clock-frequency",
+			     &memc->frequency);
+
+	ret = sysfs_create_group(&dev->kobj, &dev_attr_group);
+	if (ret) {
+		dev_err(dev, "failed to create attribute group (%d)\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int brcmstb_memc_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	sysfs_remove_group(&dev->kobj, &dev_attr_group);
+
+	return 0;
+}
+
+enum brcmstb_memc_hwtype {
+	BRCMSTB_MEMC_V21,
+	BRCMSTB_MEMC_V20,
+	BRCMSTB_MEMC_V1X,
+};
+
+static const struct brcmstb_memc_data brcmstb_memc_versions[] = {
+	{ .srpd_offset = REG_MEMC_SRPD_CFG_21 },
+	{ .srpd_offset = REG_MEMC_SRPD_CFG_20 },
+	{ .srpd_offset = REG_MEMC_SRPD_CFG_1x },
+};
+
+static const struct of_device_id brcmstb_memc_of_match[] = {
+	{
+		.compatible = "brcm,brcmstb-memc-ddr-rev-b.1.x",
+		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V1X]
+	},
+	{
+		.compatible = "brcm,brcmstb-memc-ddr-rev-b.2.0",
+		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V20]
+	},
+	{
+		.compatible = "brcm,brcmstb-memc-ddr-rev-b.2.1",
+		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
+	},
+	{
+		.compatible = "brcm,brcmstb-memc-ddr-rev-b.2.2",
+		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
+	},
+	{
+		.compatible = "brcm,brcmstb-memc-ddr-rev-b.2.3",
+		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
+	},
+	{
+		.compatible = "brcm,brcmstb-memc-ddr-rev-b.2.5",
+		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
+	},
+	{
+		.compatible = "brcm,brcmstb-memc-ddr-rev-b.2.6",
+		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
+	},
+	{
+		.compatible = "brcm,brcmstb-memc-ddr-rev-b.2.7",
+		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
+	},
+	{
+		.compatible = "brcm,brcmstb-memc-ddr-rev-b.2.8",
+		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
+	},
+	{
+		.compatible = "brcm,brcmstb-memc-ddr-rev-b.3.0",
+		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
+	},
+	{
+		.compatible = "brcm,brcmstb-memc-ddr-rev-b.3.1",
+		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
+	},
+	{
+		.compatible = "brcm,brcmstb-memc-ddr-rev-c.1.0",
+		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
+	},
+	{
+		.compatible = "brcm,brcmstb-memc-ddr-rev-c.1.1",
+		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
+	},
+	{
+		.compatible = "brcm,brcmstb-memc-ddr-rev-c.1.2",
+		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
+	},
+	{
+		.compatible = "brcm,brcmstb-memc-ddr-rev-c.1.3",
+		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
+	},
+	{
+		.compatible = "brcm,brcmstb-memc-ddr-rev-c.1.4",
+		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
+	},
+	/* default to the original offset */
+	{
+		.compatible = "brcm,brcmstb-memc-ddr",
+		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V1X]
+	},
+	{}
+};
+
+static int __maybe_unused brcmstb_memc_suspend(struct device *dev)
+{
+	struct brcmstb_memc *memc = dev_get_drvdata(dev);
+	void __iomem *cfg = memc->ddr_ctrl + memc->srpd_offset;
+	u32 val;
+
+	if (memc->timeout_cycles == 0)
+		return 0;
+
+	/* Disable SPRD prior to suspending the system since that can
+	 * cause issues with e.g: XPT_DMA trying to hash memory
+	 */
+	val = readl_relaxed(cfg);
+	val &= ~(1 << SRPD_EN_SHIFT);
+	writel_relaxed(val, cfg);
+	(void)readl_relaxed(cfg);
+
+	return 0;
+}
+
+static int __maybe_unused brcmstb_memc_resume(struct device *dev)
+{
+	struct brcmstb_memc *memc = dev_get_drvdata(dev);
+
+	if (memc->timeout_cycles == 0)
+		return 0;
+
+	return brcmstb_memc_srpd_config(memc, memc->timeout_cycles);
+}
+
+static SIMPLE_DEV_PM_OPS(brcmstb_memc_pm_ops, brcmstb_memc_suspend,
+			 brcmstb_memc_resume);
+
+static struct platform_driver brcmstb_memc_driver = {
+	.probe = brcmstb_memc_probe,
+	.remove = brcmstb_memc_remove,
+	.driver = {
+		.name		= "brcmstb_memc",
+		.owner		= THIS_MODULE,
+		.of_match_table	= of_match_ptr(brcmstb_memc_of_match),
+		.pm		= &brcmstb_memc_pm_ops,
+	},
+};
+module_platform_driver(brcmstb_memc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Broadcom");
+MODULE_DESCRIPTION("DDR SRPD driver for Broadcom STB chips");
-- 
2.25.1

Re: [PATCH 3/3] memory: Add Broadcom STB memory controller driver

[Krzysztof Kozlowski]

On 22/07/2022 20:41, Florian Fainelli wrote:
> Add support for configuring the Self Refresh Power Down (SRPD)
> inactivity timeout on Broadcom STB chips. This is used to conserve power
> when the DRAM activity is reduced.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/memory/Kconfig        |   9 +
>  drivers/memory/Makefile       |   1 +
>  drivers/memory/brcmstb_memc.c | 304 ++++++++++++++++++++++++++++++++++
>  3 files changed, 314 insertions(+)
>  create mode 100644 drivers/memory/brcmstb_memc.c
> 
> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> index ac1a411648d8..fac290e48e0b 100644
> --- a/drivers/memory/Kconfig
> +++ b/drivers/memory/Kconfig
> @@ -66,6 +66,15 @@ config BRCMSTB_DPFE
>  	  for the DRAM's temperature. Slower refresh rate means cooler RAM,
>  	  higher refresh rate means hotter RAM.
>  
> +config BRCMSTB_MEMC
> +	tristate "Broadcom STB MEMC driver"
> +	default ARCH_BRCMSTB
> +	depends on ARCH_BRCMSTB || COMPILE_TEST
> +	help
> +	  This driver provides a way to configure the Broadcom STB memory
> +	  controller and specifically control the Self Refresh Power Down
> +	  (SRPD) inactivity timeout.
> +
>  config BT1_L2_CTL
>  	bool "Baikal-T1 CM2 L2-RAM Cache Control Block"
>  	depends on MIPS_BAIKAL_T1 || COMPILE_TEST
> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> index bc7663ed1c25..e148f636c082 100644
> --- a/drivers/memory/Makefile
> +++ b/drivers/memory/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_ARM_PL172_MPMC)	+= pl172.o
>  obj-$(CONFIG_ATMEL_SDRAMC)	+= atmel-sdramc.o
>  obj-$(CONFIG_ATMEL_EBI)		+= atmel-ebi.o
>  obj-$(CONFIG_BRCMSTB_DPFE)	+= brcmstb_dpfe.o
> +obj-$(CONFIG_BRCMSTB_MEMC)	+= brcmstb_memc.o
>  obj-$(CONFIG_BT1_L2_CTL)	+= bt1-l2-ctl.o
>  obj-$(CONFIG_TI_AEMIF)		+= ti-aemif.o
>  obj-$(CONFIG_TI_EMIF)		+= emif.o
> diff --git a/drivers/memory/brcmstb_memc.c b/drivers/memory/brcmstb_memc.c
> new file mode 100644
> index 000000000000..881da958c542
> --- /dev/null
> +++ b/drivers/memory/brcmstb_memc.c
> @@ -0,0 +1,304 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * DDR Self-Refresh Power Down (SRPD) support for Broadcom STB SoCs
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_device.h>
> +#include <linux/io.h>

Let's order the includes by name. It reduces the chances of conflicts later.

> +
> +#define REG_MEMC_CNTRLR_CONFIG		0x00
> +#define  CNTRLR_CONFIG_LPDDR4		5

This is also a SHIFT? Later you use such suffix.

> +#define  CNTRLR_CONFIG_MASK		0xf
> +#define REG_MEMC_SRPD_CFG_21		0x20
> +#define REG_MEMC_SRPD_CFG_20		0x34
> +#define REG_MEMC_SRPD_CFG_1x		0x3c
> +#define INACT_COUNT_SHIFT		0
> +#define INACT_COUNT_MASK		0xffff
> +#define SRPD_EN_SHIFT			16
> +
> +struct brcmstb_memc_data {
> +	u32 srpd_offset;
> +};
> +
> +struct brcmstb_memc {
> +	struct device *dev;
> +	void __iomem *ddr_ctrl;
> +	unsigned int timeout_cycles;
> +	u32 frequency;
> +	u32 srpd_offset;
> +};
> +
> +static int brcmstb_memc_uses_lpddr4(struct brcmstb_memc *memc)
> +{
> +	void __iomem *config = memc->ddr_ctrl + REG_MEMC_CNTRLR_CONFIG;
> +	u32 reg;
> +
> +	reg = readl_relaxed(config) & CNTRLR_CONFIG_MASK;
> +
> +	return reg == CNTRLR_CONFIG_LPDDR4;
> +}
> +
> +static int brcmstb_memc_srpd_config(struct brcmstb_memc *memc,
> +				    unsigned int cycles)
> +{
> +	void __iomem *cfg = memc->ddr_ctrl + memc->srpd_offset;
> +	u32 val;
> +
> +	/* Max timeout supported in HW */
> +	if (cycles > INACT_COUNT_MASK)
> +		return -EINVAL;
> +
> +	memc->timeout_cycles = cycles;
> +
> +	val = (cycles << INACT_COUNT_SHIFT) & INACT_COUNT_MASK;
> +	if (cycles)
> +		val |= (1 << SRPD_EN_SHIFT);

BIT(SRPD_EN_SHIFT)

> +
> +	writel_relaxed(val, cfg);
> +	(void)readl_relaxed(cfg);

Leave a comment why do you need such read.

> +
> +	return 0;
> +}
> +
> +static ssize_t frequency_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	struct brcmstb_memc *memc = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", memc->frequency);
> +}
> +
> +static ssize_t srpd_show(struct device *dev,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	struct brcmstb_memc *memc = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", memc->timeout_cycles);
> +}
> +
> +static ssize_t srpd_store(struct device *dev, struct device_attribute *attr,
> +			  const char *buf, size_t count)

You need to document the sysfs ABI.

> +{
> +	struct brcmstb_memc *memc = dev_get_drvdata(dev);
> +	unsigned int val;
> +	int ret;
> +

Start with /*

> +	/* Cannot change the inactivity timeout on LPDDR4 chips because the

...for this comment.

> +	 * dynamic tuning process will also get affected by the inactivity
> +	 * timeout, thus making it non functional.
> +	 */
> +	if (brcmstb_memc_uses_lpddr4(memc))
> +		return -EOPNOTSUPP;
> +
> +	ret = kstrtouint(buf, 10, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = brcmstb_memc_srpd_config(memc, val);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RO(frequency);
> +static DEVICE_ATTR_RW(srpd);
> +
> +static struct attribute *dev_attrs[] = {
> +	&dev_attr_frequency.attr,
> +	&dev_attr_srpd.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group dev_attr_group = {
> +	.attrs = dev_attrs,
> +};
> +
> +static const struct of_device_id brcmstb_memc_of_match[];
> +
> +static int brcmstb_memc_probe(struct platform_device *pdev)
> +{
> +	const struct brcmstb_memc_data *memc_data;> +	const struct of_device_id *of_id;
> +	struct device *dev = &pdev->dev;
> +	struct brcmstb_memc *memc;
> +	struct resource *res;
> +	int ret;
> +
> +	memc = devm_kzalloc(dev, sizeof(*memc), GFP_KERNEL);
> +	if (!memc)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, memc);
> +
> +	of_id = of_match_device(brcmstb_memc_of_match, dev);
> +	if (!of_id || !of_id->data)

!of_id is not possible
!of_id->data is the same not possible (or even less...)

I would suggest to drop both or at least the latter.

> +		return -EINVAL;
> +
> +	memc_data = of_id->data;
> +	memc->srpd_offset = memc_data->srpd_offset;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	memc->ddr_ctrl = devm_ioremap_resource(dev, res);

devm_platform_ioremap_resource()

> +	if (IS_ERR(memc->ddr_ctrl))
> +		return PTR_ERR(memc->ddr_ctrl);
> +
> +	of_property_read_u32(pdev->dev.of_node, "clock-frequency",
> +			     &memc->frequency);

Undocumented property.

> +
> +	ret = sysfs_create_group(&dev->kobj, &dev_attr_group);
> +	if (ret) {
> +		dev_err(dev, "failed to create attribute group (%d)\n", ret);

I am pretty sure there was a helper to make it simpler, but canno recall
now...

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int brcmstb_memc_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +
> +	sysfs_remove_group(&dev->kobj, &dev_attr_group);
> +
> +	return 0;
> +}
> +
> +enum brcmstb_memc_hwtype {
> +	BRCMSTB_MEMC_V21,
> +	BRCMSTB_MEMC_V20,
> +	BRCMSTB_MEMC_V1X,
> +};
> +
> +static const struct brcmstb_memc_data brcmstb_memc_versions[] = {
> +	{ .srpd_offset = REG_MEMC_SRPD_CFG_21 },
> +	{ .srpd_offset = REG_MEMC_SRPD_CFG_20 },
> +	{ .srpd_offset = REG_MEMC_SRPD_CFG_1x },
> +};
> +
> +static const struct of_device_id brcmstb_memc_of_match[] = {
> +	{
> +		.compatible = "brcm,brcmstb-memc-ddr-rev-b.1.x",
> +		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V1X]
> +	},
> +	{
> +		.compatible = "brcm,brcmstb-memc-ddr-rev-b.2.0",
> +		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V20]
> +	},
> +	{
> +		.compatible = "brcm,brcmstb-memc-ddr-rev-b.2.1",
> +		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
> +	},
> +	{
> +		.compatible = "brcm,brcmstb-memc-ddr-rev-b.2.2",
> +		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
> +	},
> +	{
> +		.compatible = "brcm,brcmstb-memc-ddr-rev-b.2.3",
> +		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
> +	},
> +	{
> +		.compatible = "brcm,brcmstb-memc-ddr-rev-b.2.5",
> +		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
> +	},
> +	{
> +		.compatible = "brcm,brcmstb-memc-ddr-rev-b.2.6",
> +		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
> +	},
> +	{
> +		.compatible = "brcm,brcmstb-memc-ddr-rev-b.2.7",
> +		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
> +	},
> +	{
> +		.compatible = "brcm,brcmstb-memc-ddr-rev-b.2.8",
> +		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
> +	},
> +	{
> +		.compatible = "brcm,brcmstb-memc-ddr-rev-b.3.0",
> +		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
> +	},
> +	{
> +		.compatible = "brcm,brcmstb-memc-ddr-rev-b.3.1",
> +		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
> +	},
> +	{
> +		.compatible = "brcm,brcmstb-memc-ddr-rev-c.1.0",
> +		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
> +	},
> +	{
> +		.compatible = "brcm,brcmstb-memc-ddr-rev-c.1.1",
> +		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
> +	},
> +	{
> +		.compatible = "brcm,brcmstb-memc-ddr-rev-c.1.2",
> +		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
> +	},
> +	{
> +		.compatible = "brcm,brcmstb-memc-ddr-rev-c.1.3",
> +		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
> +	},
> +	{
> +		.compatible = "brcm,brcmstb-memc-ddr-rev-c.1.4",
> +		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V21]
> +	},
> +	/* default to the original offset */
> +	{
> +		.compatible = "brcm,brcmstb-memc-ddr",
> +		.data = &brcmstb_memc_versions[BRCMSTB_MEMC_V1X]
> +	},
> +	{}
> +};
> +
> +static int __maybe_unused brcmstb_memc_suspend(struct device *dev)
> +{
> +	struct brcmstb_memc *memc = dev_get_drvdata(dev);
> +	void __iomem *cfg = memc->ddr_ctrl + memc->srpd_offset;
> +	u32 val;
> +
> +	if (memc->timeout_cycles == 0)
> +		return 0;
> +
> +	/* Disable SPRD prior to suspending the system since that can

SRPD?

Comment starts with /* alone


> +	 * cause issues with e.g: XPT_DMA trying to hash memory
> +	 */
> +	val = readl_relaxed(cfg);
> +	val &= ~(1 << SRPD_EN_SHIFT);

~(BIT(SRPD_EN_SHIFT))

> +	writel_relaxed(val, cfg);
> +	(void)readl_relaxed(cfg);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused brcmstb_memc_resume(struct device *dev)
> +{
> +	struct brcmstb_memc *memc = dev_get_drvdata(dev);
> +
> +	if (memc->timeout_cycles == 0)
> +		return 0;
> +
> +	return brcmstb_memc_srpd_config(memc, memc->timeout_cycles);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(brcmstb_memc_pm_ops, brcmstb_memc_suspend,
> +			 brcmstb_memc_resume);
> +
> +static struct platform_driver brcmstb_memc_driver = {
> +	.probe = brcmstb_memc_probe,
> +	.remove = brcmstb_memc_remove,
> +	.driver = {
> +		.name		= "brcmstb_memc",
> +		.owner		= THIS_MODULE,
> +		.of_match_table	= of_match_ptr(brcmstb_memc_of_match),

This does not match brcmstb_memc_of_match - you will have unused
variable warning. Drop of_match_ptr or add maybe_unused.



Best regards,
Krzysztof