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

[PATCH v3 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.

Changes in v3:

- made 'frequency' property optional to avoid introducing warnings for
  existing BMIPS and BCM7445 DTS files
- updated sysfs document to use a shorter and universal path

Changes in v2:

- merged the v1 first two patches
- added a sysfs document describing attributes exposed
- addressed feedback from Krzysztof regarding style and API usage


Florian Fainelli (3):
  dt-bindings: memory-controller: Document Broadcom STB MEMC
  Documentation: sysfs: Document Broadcom STB memc sysfs knobs
  memory: Add Broadcom STB memory controller driver

 .../ABI/testing/sysfs-platform-brcmstb-memc   |  15 +
 .../bindings/arm/bcm/brcm,brcmstb.txt         |  11 +-
 .../brcm,brcmstb-memc-ddr.yaml                |  52 +++
 drivers/memory/Kconfig                        |   9 +
 drivers/memory/Makefile                       |   1 +
 drivers/memory/brcmstb_memc.c                 | 302 ++++++++++++++++++
 6 files changed, 381 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-brcmstb-memc
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/brcm,brcmstb-memc-ddr.yaml
 create mode 100644 drivers/memory/brcmstb_memc.c

-- 
2.25.1

[PATCH v3 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.

Since we introduce this binding, the section in
Documentation/devicetree/bindings/arm/bcm/brcm,brcmstb.txt is removed
and this binding is referenced instead.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 .../bindings/arm/bcm/brcm,brcmstb.txt         | 11 +---
 .../brcm,brcmstb-memc-ddr.yaml                | 52 +++++++++++++++++++
 2 files changed, 54 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/brcm,brcmstb-memc-ddr.yaml

diff --git a/Documentation/devicetree/bindings/arm/bcm/brcm,brcmstb.txt b/Documentation/devicetree/bindings/arm/bcm/brcm,brcmstb.txt
index 104cc9b41df4..e797d2f69f3b 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-controllers/brcm,memc.yaml for a
+full list of supported compatible strings and properties.
 
 Example:
 
diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,brcmstb-memc-ddr.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,brcmstb-memc-ddr.yaml
new file mode 100644
index 000000000000..4b072c879b02
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/brcm,brcmstb-memc-ddr.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/memory-controllers/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
+
+  clock-frequency:
+    description: DDR PHY frequency in Hz
+
+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>;
+        clock-frequency = <2133000000>;
+    };
-- 
2.25.1

[PATCH v3 2/3] Documentation: sysfs: Document Broadcom STB memc sysfs knobs

[Florian Fainelli]

Document the "srpd" and "frequency" sysfs attributes exposed by
the brcmstb_memc driver.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 .../ABI/testing/sysfs-platform-brcmstb-memc       | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-brcmstb-memc

diff --git a/Documentation/ABI/testing/sysfs-platform-brcmstb-memc b/Documentation/ABI/testing/sysfs-platform-brcmstb-memc
new file mode 100644
index 000000000000..2f2b750ac2fd
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-brcmstb-memc
@@ -0,0 +1,15 @@
+What:		/sys/bus/platform/devices/*/srpd
+Date:		July 2022
+KernelVersion:	5.21
+Contact:	Florian Fainelli <f.fainelli@gmail.com>
+Description:
+		Self Refresh Power Down (SRPD) inactivity timeout counted in
+		internal DDR controller clock cycles. Possible values range
+		from 0 (disable inactivity timeout) to 65535 (0xffff).
+
+What:		/sys/bus/platform/devices/*/frequency
+Date:		July 2022
+KernelVersion:	5.21
+Contact:	Florian Fainelli <f.fainelli@gmail.com>
+Description:
+		DDR PHY frequency in Hz.
-- 
2.25.1

[PATCH v3 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 | 302 ++++++++++++++++++++++++++++++++++
 3 files changed, 312 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..9b993343d447
--- /dev/null
+++ b/drivers/memory/brcmstb_memc.c
@@ -0,0 +1,302 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * DDR Self-Refresh Power Down (SRPD) support for Broadcom STB SoCs
+ *
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#define REG_MEMC_CNTRLR_CONFIG		0x00
+#define  CNTRLR_CONFIG_LPDDR4_SHIFT	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_SHIFT;
+}
+
+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 |= BIT(SRPD_EN_SHIFT);
+
+	writel_relaxed(val, cfg);
+	/* Ensure the write is committed to the controller */
+	(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;
+	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);
+	memc_data = of_id->data;
+	memc->srpd_offset = memc_data->srpd_offset;
+
+	memc->ddr_ctrl = devm_platform_ioremap_resource(pdev, 0);
+	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)
+		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 SRPD prior to suspending the system since that can
+	 * cause issues with other memory clients managed by the ARM
+	 * trusted firmware to access memory.
+	 */
+	val = readl_relaxed(cfg);
+	val &= ~BIT(SRPD_EN_SHIFT);
+	writel_relaxed(val, cfg);
+	/* Ensure the write is committed to the controller */
+	(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	= 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 v3 3/3] memory: Add Broadcom STB memory controller driver

[Krzysztof Kozlowski]

On 02/08/2022 01:09, 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.
> 


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

No need, run coccinelle.

> +		.of_match_table	= brcmstb_memc_of_match,
> +		.pm		= &brcmstb_memc_pm_ops,

Shouldn't this be pm_ptr()? and then no need for __maybe_unused in
brcmstb_memc_resume/suspend.

> +	},
> +};
> +module_platform_driver(brcmstb_memc_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Broadcom");
> +MODULE_DESCRIPTION("DDR SRPD driver for Broadcom STB chips");


Best regards,
Krzysztof

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

[Florian Fainelli]

On 8/9/22 02:58, Krzysztof Kozlowski wrote:
> On 02/08/2022 01:09, 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.
>>
> 
> 
>> +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,
> 
> No need, run coccinelle.
> 
>> +		.of_match_table	= brcmstb_memc_of_match,
>> +		.pm		= &brcmstb_memc_pm_ops,
> 
> Shouldn't this be pm_ptr()? and then no need for __maybe_unused in
> brcmstb_memc_resume/suspend.

How can one can remove __maybe_unused without causing a warning for the 
CONFIG_PM=n case, not that I needed to build to convince myself, but 
still did anyway:

drivers/memory/brcmstb_memc.c:275:12: warning: 'brcmstb_memc_resume' 
defined but not used [-Wunused-function]
  static int brcmstb_memc_resume(struct device *dev)
             ^~~~~~~~~~~~~~~~~~~
drivers/memory/brcmstb_memc.c:252:12: warning: 'brcmstb_memc_suspend' 
defined but not used [-Wunused-function]
  static int brcmstb_memc_suspend(struct device *dev)
             ^~~~~~~~~~~~~~~~~~~~

unless you also implied enclosing those functions under an #if 
IS_ENABLED(CONFIG_PM) or something which is IMHO less preferable.
-- 
Florian

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

[Krzysztof Kozlowski]

On 12/08/2022 20:29, Florian Fainelli wrote:
> On 8/9/22 02:58, Krzysztof Kozlowski wrote:
>> On 02/08/2022 01:09, 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.
>>>
>>
>>
>>> +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,
>>
>> No need, run coccinelle.
>>
>>> +		.of_match_table	= brcmstb_memc_of_match,
>>> +		.pm		= &brcmstb_memc_pm_ops,
>>
>> Shouldn't this be pm_ptr()? and then no need for __maybe_unused in
>> brcmstb_memc_resume/suspend.
> 
> How can one can remove __maybe_unused without causing a warning for the 
> CONFIG_PM=n case, not that I needed to build to convince myself, but 
> still did anyway:
> 
> drivers/memory/brcmstb_memc.c:275:12: warning: 'brcmstb_memc_resume' 
> defined but not used [-Wunused-function]
>   static int brcmstb_memc_resume(struct device *dev)
>              ^~~~~~~~~~~~~~~~~~~
> drivers/memory/brcmstb_memc.c:252:12: warning: 'brcmstb_memc_suspend' 
> defined but not used [-Wunused-function]
>   static int brcmstb_memc_suspend(struct device *dev)
>              ^~~~~~~~~~~~~~~~~~~~
> 
> unless you also implied enclosing those functions under an #if 
> IS_ENABLED(CONFIG_PM) or something which is IMHO less preferable.

Are you sure you added also pm_ptr()? I don't see such warnings with W=1
and final object does not have the functions (for a different driver but
same principle).

Best regards,
Krzysztof

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

[Florian Fainelli]

On 8/12/22 10:36, Krzysztof Kozlowski wrote:
> On 12/08/2022 20:29, Florian Fainelli wrote:
>> On 8/9/22 02:58, Krzysztof Kozlowski wrote:
>>> On 02/08/2022 01:09, 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.
>>>>
>>>
>>>
>>>> +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,
>>>
>>> No need, run coccinelle.
>>>
>>>> +		.of_match_table	= brcmstb_memc_of_match,
>>>> +		.pm		= &brcmstb_memc_pm_ops,
>>>
>>> Shouldn't this be pm_ptr()? and then no need for __maybe_unused in
>>> brcmstb_memc_resume/suspend.
>>
>> How can one can remove __maybe_unused without causing a warning for the
>> CONFIG_PM=n case, not that I needed to build to convince myself, but
>> still did anyway:
>>
>> drivers/memory/brcmstb_memc.c:275:12: warning: 'brcmstb_memc_resume'
>> defined but not used [-Wunused-function]
>>    static int brcmstb_memc_resume(struct device *dev)
>>               ^~~~~~~~~~~~~~~~~~~
>> drivers/memory/brcmstb_memc.c:252:12: warning: 'brcmstb_memc_suspend'
>> defined but not used [-Wunused-function]
>>    static int brcmstb_memc_suspend(struct device *dev)
>>               ^~~~~~~~~~~~~~~~~~~~
>>
>> unless you also implied enclosing those functions under an #if
>> IS_ENABLED(CONFIG_PM) or something which is IMHO less preferable.
> 
> Are you sure you added also pm_ptr()? I don't see such warnings with W=1
> and final object does not have the functions (for a different driver but
> same principle).

Yes I am sure I added pm_ptr() see the v4 I just submitted. I don't see 
how the compiler cannot warn about the functions being unused the day 
they stop being referenced by the pm_ops structure which is eliminated?
-- 
Florian

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

[Krzysztof Kozlowski]

On 12/08/2022 20:52, Florian Fainelli wrote:

>>> unless you also implied enclosing those functions under an #if
>>> IS_ENABLED(CONFIG_PM) or something which is IMHO less preferable.
>>
>> Are you sure you added also pm_ptr()? I don't see such warnings with W=1
>> and final object does not have the functions (for a different driver but
>> same principle).
> 
> Yes I am sure I added pm_ptr() see the v4 I just submitted. I don't see 
> how the compiler cannot warn about the functions being unused the day 
> they stop being referenced by the pm_ops structure which is eliminated?

I don't have the answer how it exactly works (or which gcc version
introduced it), but I tested it and the functions were not present in
the object file, thus of course no warnings.

The driver change I am referring to is:
https://lore.kernel.org/all/20220808174107.38676-15-paul@crapouillou.net/

I think the only difference against your v4 is:
DEFINE_SIMPLE_DEV_PM_OPS
and lack of __maybe_unused on the functions.

The DEFINE_SIMPLE_DEV_PM_OPS itself adds __maybe_unused for !CONFIG_PM
case, but I don't think it is relevant.

Best regards,
Krzysztof

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

[Florian Fainelli]

On 8/12/22 11:41, Krzysztof Kozlowski wrote:
> On 12/08/2022 20:52, Florian Fainelli wrote:
> 
>>>> unless you also implied enclosing those functions under an #if
>>>> IS_ENABLED(CONFIG_PM) or something which is IMHO less preferable.
>>>
>>> Are you sure you added also pm_ptr()? I don't see such warnings with W=1
>>> and final object does not have the functions (for a different driver but
>>> same principle).
>>
>> Yes I am sure I added pm_ptr() see the v4 I just submitted. I don't see
>> how the compiler cannot warn about the functions being unused the day
>> they stop being referenced by the pm_ops structure which is eliminated?
> 
> I don't have the answer how it exactly works (or which gcc version
> introduced it), but I tested it and the functions were not present in
> the object file, thus of course no warnings.
> 
> The driver change I am referring to is:
> https://lore.kernel.org/all/20220808174107.38676-15-paul@crapouillou.net/
> 
> I think the only difference against your v4 is:
> DEFINE_SIMPLE_DEV_PM_OPS
> and lack of __maybe_unused on the functions.
> 
> The DEFINE_SIMPLE_DEV_PM_OPS itself adds __maybe_unused for !CONFIG_PM
> case, but I don't think it is relevant.

It definitively is relevant here. SIMPLE_DEV_PM_OPS without 
__maybe_unused results in the following pre-processed code:

static int brcmstb_memc_suspend(struct device *dev)
{
  struct brcmstb_memc *memc = dev_get_drvdata(dev);
  void *cfg = memc->ddr_ctrl + memc->srpd_offset;
  u32 val;

  if (memc->timeout_cycles == 0)
   return 0;






  val = ({ u32 __r = (( __u32)(__le32)(( __le32) __raw_readl(cfg))); 
__r; });
  val &= ~((((1UL))) << (16));
  __raw_writel(( u32) (( __le32)(__u32)(val)),cfg);

  (void)({ u32 __r = (( __u32)(__le32)(( __le32) __raw_readl(cfg))); 
__r; });

  return 0;
}

static int 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 const struct dev_pm_ops __attribute__((__unused__)) 
brcmstb_memc_pm_ops = { }
                         ;

static struct platform_driver brcmstb_memc_driver = {
  .probe = brcmstb_memc_probe,
  .remove = brcmstb_memc_remove,
  .driver = {
   .name = "brcmstb_memc",
   .of_match_table = brcmstb_memc_of_match,
   .pm = ((1) ? ((&brcmstb_memc_pm_ops)) : ((void *)0)),
  },
};

Now with DEFINE_SIMPLE_PM_OPS, we get the following instead:

static const struct dev_pm_ops brcmstb_memc_pm_ops = { .suspend = ((0) ? 
((brcmstb_memc_suspend)) : ((void *)0)), .resume = ((0) ? 
((brcmstb_memc_resume)) : ((void *)0)), .freeze = ((0) ? 
((brcmstb_memc_suspend)) : ((void *)0)), .thaw = ((0) ? 
((brcmstb_memc_resume)) : ((void *)0)), .poweroff = ((0) ? 
((brcmstb_memc_suspend)) : ((void *)0)), .restore = ((0) ? 
((brcmstb_memc_resume)) : ((void *)0)), .runtime_suspend = ((void *)0), 
.runtime_resume = ((void *)0), .runtime_idle = ((void *)0), }
                         ;

static struct platform_driver brcmstb_memc_driver = {
  .probe = brcmstb_memc_probe,
  .remove = brcmstb_memc_remove,
  .driver = {
   .name = "brcmstb_memc",
   .of_match_table = brcmstb_memc_of_match,
   .pm = ((1) ? ((&brcmstb_memc_pm_ops)) : ((void *)0)),
  },
};

so we will continue to reference the functions, but eventually we will 
eliminate those at the optimization stage by figuring out that this is 
dead code, therefore it can be eliminated. Note that in both cases the 
object does not contain brcmstb_memc_{resume,suspend} which makes sense 
because the warnings are generated at the time the code is processed, 
and the dead code elimination at one of the optimization stages.

I can spin a v5 with DEFINE_SIMPLE_PM_OPS if you prefer to get rid of 
the __maybe_unused.
-- 
Florian

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

[Krzysztof Kozlowski]

On Sat, 13 Aug 2022 at 01:23, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 8/12/22 11:41, Krzysztof Kozlowski wrote:
> > On 12/08/2022 20:52, Florian Fainelli wrote:
> >
> >>>> unless you also implied enclosing those functions under an #if
> >>>> IS_ENABLED(CONFIG_PM) or something which is IMHO less preferable.
> >>>
> >>> Are you sure you added also pm_ptr()? I don't see such warnings with W=1
> >>> and final object does not have the functions (for a different driver but
> >>> same principle).
> >>
> >> Yes I am sure I added pm_ptr() see the v4 I just submitted. I don't see
> >> how the compiler cannot warn about the functions being unused the day
> >> they stop being referenced by the pm_ops structure which is eliminated?
> >
> > I don't have the answer how it exactly works (or which gcc version
> > introduced it), but I tested it and the functions were not present in
> > the object file, thus of course no warnings.
> >
> > The driver change I am referring to is:
> > https://lore.kernel.org/all/20220808174107.38676-15-paul@crapouillou.net/
> >
> > I think the only difference against your v4 is:
> > DEFINE_SIMPLE_DEV_PM_OPS
> > and lack of __maybe_unused on the functions.
> >
> > The DEFINE_SIMPLE_DEV_PM_OPS itself adds __maybe_unused for !CONFIG_PM
> > case, but I don't think it is relevant.
>
> It definitively is relevant here. SIMPLE_DEV_PM_OPS without
> __maybe_unused results in the following pre-processed code:
>

What I was referring to is "__maybe_unused" is not relevant here. The
DEFINE_SIMPLE_DEV_PM_OPS (with pm_ptr()) are the change you need.

Best regards,
Krzysztof