[PATCH v3 0/2] Amlogic Meson Always-On ARC remote-processor support

[PATCH v3 0/2] Amlogic Meson Always-On ARC remote-processor support

[Martin Blumenstingl]

Amlogic Meson6/8/8b/8m2 come with an ARC core in the Always-On (AO)
power-domain. This is typically used for waking up the ARM CPU after
powering it down for system suspend.

The exact ARC core used on Meson6 and earlier is not known. I believe
it is an ARC625, but I am not sure about this. Meson8/8b/8m2 uses an
ARC EM4 core.
They all have in common that they use a section of the SoCs SRAM for
running code on the ARC core.

Unfortunately there's no information about the remote-processor control
registers in the public Meson8b (S805) datasheet. All information is
either taken from Amlogic's 3.10 kernel and 2011-03 u-boot or found by
testing (for example the clock input is not mentioned anywhere in the
reference code, but disabling it stops the AO ARC core from working).

This series consists of five patches:
 1: dt-bindings for the SRAM section
 2: dt-bindings for the SECBUS2 syscon region which contains a few
    bits for controlling this remote processor
 3: dt-bindings for the AO ARC remote processor
 4: the driver for booting code on the AO ARC remote processor
 5: (only included for documentation purposes) dts changes (these will
    be re-sent separately)

Patches #3 and #4 should go through the remoteproc tree. Patches #1
and #2 may go through Rob's (devicetree) tree, Kevin's linux-amlogic
tree or through the remoteproc tree. Personally I have no preference
here.

To test this series I ported the Amlogic serial driver and added the
board files for the Amlogic AO ARC EM4 to the Zephyr RTOS. The code can
be found here: [0] (the resulting zephyr.elf can then be loaded as
remote-processor firmware from Linux).


Changes since v1 at [1]:
- fixed yamllint warnings (after installing the package these now also
  show up on my build machine) in patches #2 and #3. Thanks for the
  hint Rob
- dropped the explicit "select" statement from the dt-bindings in patch
  #2 as suggested by Rob (thanks)

Changes since v2 at [2]:
- added Rob's Reviewed-by to the dt-bindings patch (thank you!)
- all other patches (secbus2 dt-bindings, .dtsi, etc.) are already
  applied so they're not part of this series anymore
- remove extra indentation for register fields
- fix maximum (da + len) check in meson_mx_ao_arc_rproc_da_to_va to not
  exceed the allocated SRAM size (thanks for spotting this Bjorn!)
- remove extra usleep_range from meson_mx_ao_arc_rproc_start
- make the code to generate the value for the AO_CPU_CNTL register
  easier to read
- removed unused secbus2_pdev variable from meson_mx_ao_arc_rproc_probe
- add preprocessor defines for the AO_REMAP_REG0 and AO_REMAP_REG1
  fields. Special thanks to Jianxin from Amlogic for providing this
  information
- program AO_REMAP_REG0 correctly so bits 17:14 in the SRAM address can
  be used
- program AO_REMAP_REG1 with 0x0 and add a comment as to why this is
  needed. This comment should also clarify why the da_to_va
  implementation can assume that the memory always starts at 0x0
- allow bits 17:14 to be set in the SRAM address. I tested this with
  physical address 0xd9010000 (SRAM offset 0x10000)
- re-reading the Amlogic u-boot and kernel code again made me understand
  what the bits in AO_SECURE_REG0 are used for: these need to be
  programmed with bits [19:12] of the used SRAM address. The field macro
  and GENMASK is updated accordingly.
- initialize fw_name with NULL so we can simplify the code as we don't
  have to check for the return value of device_property_read_string
  anymore (thanks for the suggestion Bjorn)
- drop of_match_ptr as it's not needed (thanks for the suggestion Bjorn)
- add the new is_iomem parameter to the .da_to_va implementation


[0] https://github.com/xdarklight/zephyr-rtos/commits/amlogic_ao_em4-20201229
[1] https://patchwork.kernel.org/project/linux-amlogic/list/?series=407349
[2] https://patchwork.kernel.org/project/linux-amlogic/cover/20210102205904.2691120-1-martin.blumenstingl@googlemail.com/


Martin Blumenstingl (2):
  dt-bindings: remoteproc: Add the documentation for Meson AO ARC rproc
  remoteproc: meson-mx-ao-arc: Add a driver for the AO ARC remote
    procesor

 .../remoteproc/amlogic,meson-mx-ao-arc.yaml   |  87 ++++++
 drivers/remoteproc/Kconfig                    |  11 +
 drivers/remoteproc/Makefile                   |   1 +
 drivers/remoteproc/meson_mx_ao_arc.c          | 260 ++++++++++++++++++
 4 files changed, 359 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/amlogic,meson-mx-ao-arc.yaml
 create mode 100644 drivers/remoteproc/meson_mx_ao_arc.c

-- 
2.32.0

[PATCH v3 1/2] dt-bindings: remoteproc: Add the documentation for Meson AO ARC rproc

[Martin Blumenstingl]

Amlogic Meson6, Meson8, Meson8b and Meson8m2 SoCs embed an ARC EM4
controller for always-on operations, typically used for managing system
suspend.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../remoteproc/amlogic,meson-mx-ao-arc.yaml   | 87 +++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/amlogic,meson-mx-ao-arc.yaml

diff --git a/Documentation/devicetree/bindings/remoteproc/amlogic,meson-mx-ao-arc.yaml b/Documentation/devicetree/bindings/remoteproc/amlogic,meson-mx-ao-arc.yaml
new file mode 100644
index 000000000000..d892d29a656b
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/amlogic,meson-mx-ao-arc.yaml
@@ -0,0 +1,87 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/remoteproc/amlogic,meson-mx-ao-arc.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Amlogic Meson AO ARC Remote Processor bindings
+
+description:
+  Amlogic Meson6, Meson8, Meson8b and Meson8m2 SoCs embed an ARC core
+  controller for always-on operations, typically used for managing
+  system suspend. Meson6 and older use a ARC core based on the ARCv1
+  ISA, while Meson8, Meson8b and Meson8m2 use an ARC EM4 (ARCv2 ISA)
+  core.
+
+maintainers:
+  - Martin Blumenstingl <martin.blumenstingl@googlemail.com>
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - amlogic,meson8-ao-arc
+          - amlogic,meson8b-ao-arc
+      - const: amlogic,meson-mx-ao-arc
+
+  firmware-name:
+    $ref: /schemas/types.yaml#/definitions/string
+    description:
+      The name of the firmware which should be loaded for this remote
+      processor.
+
+  reg:
+    description:
+      Address ranges of the remap and CPU control addresses for the
+      remote processor.
+    minItems: 2
+
+  reg-names:
+    items:
+      - const: remap
+      - const: cpu
+
+  resets:
+    minItems: 1
+
+  clocks:
+    minItems: 1
+
+  sram:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      phandles to a reserved SRAM region which is used as the memory of
+      the ARC core. The region should be defined as child nodes of the
+      AHB SRAM node as per the generic bindings in
+      Documentation/devicetree/bindings/sram/sram.yaml
+
+  amlogic,secbus2:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      A phandle to the SECBUS2 region which contains some configuration
+      bits of this remote processor
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - resets
+  - clocks
+  - sram
+  - amlogic,secbus2
+
+additionalProperties: false
+
+examples:
+  - |
+    remoteproc@1c {
+      compatible= "amlogic,meson8-ao-arc", "amlogic,meson-mx-ao-arc";
+      reg = <0x1c 0x8>, <0x38 0x8>;
+      reg-names = "remap", "cpu";
+      resets = <&media_cpu_reset>;
+      clocks = <&media_cpu_clock>;
+      sram = <&ahb_sram_ao_arc>;
+      amlogic,secbus2 = <&secbus2>;
+    };
+
+...
-- 
2.32.0

[PATCH v3 2/2] remoteproc: meson-mx-ao-arc: Add a driver for the AO ARC remote procesor

[Martin Blumenstingl]

Amlogic Meson6, Meson8, Meson8b and Meson8m2 embed an ARC core in the
Always-On (AO) power-domain. This is typically used for waking up the
ARM cores after system suspend.

The configuration is spread across three different registers:
- AO_REMAP_REG0 which must be programmed to zero, it's actual purpose
  is unknown. There is a second remap register which is not used in the
  vendor kernel (which served as reference for this driver).
- AO_CPU_CNTL is used to start and stop the ARC core.
- AO_SECURE_REG0 in the SECBUS2 register area with unknown purpose.

To boot the ARC core we also need to enable it's gate clock and trigger
a reset.

The actual code for this ARC core can come from an ELF binary, for
example by building the Zephyr RTOS for an ARC EM4 core and then taking
"zephyr.elf" as firmware. This executable does not have any "rsc table"
so we are skipping rproc_elf_load_rsc_table (rproc_ops.parse_fw) and
rproc_elf_find_loaded_rsc_table (rproc_ops.find_loaded_rsc_table).

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/remoteproc/Kconfig           |  11 ++
 drivers/remoteproc/Makefile          |   1 +
 drivers/remoteproc/meson_mx_ao_arc.c | 260 +++++++++++++++++++++++++++
 3 files changed, 272 insertions(+)
 create mode 100644 drivers/remoteproc/meson_mx_ao_arc.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 9a6eedc3994a..3197bbe38785 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -127,6 +127,17 @@ config KEYSTONE_REMOTEPROC
 	  It's safe to say N here if you're not interested in the Keystone
 	  DSPs or just want to use a bare minimum kernel.
 
+config MESON_MX_AO_ARC_REMOTEPROC
+	tristate "Amlogic Meson6/8/8b/8m2 AO ARC remote processor support"
+	depends on HAS_IOMEM
+	depends on (ARM && ARCH_MESON) || COMPILE_TEST
+	select GENERIC_ALLOCATOR
+	help
+	  Say m or y here to have support for the AO ARC remote processor
+	  on Amlogic Meson6/Meson8/Meson8b/Meson8m2 SoCs. This is
+	  typically used for system suspend.
+	  If unusre say N.
+
 config PRU_REMOTEPROC
 	tristate "TI PRU remoteproc support"
 	depends on TI_PRUSS
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index bb26c9e4ef9c..ce1abeb30907 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
 obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
 obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
 obj-$(CONFIG_KEYSTONE_REMOTEPROC)	+= keystone_remoteproc.o
+obj-$(CONFIG_MESON_MX_AO_ARC_REMOTEPROC)+= meson_mx_ao_arc.o
 obj-$(CONFIG_PRU_REMOTEPROC)		+= pru_rproc.o
 obj-$(CONFIG_QCOM_PIL_INFO)		+= qcom_pil_info.o
 obj-$(CONFIG_QCOM_RPROC_COMMON)		+= qcom_common.o
diff --git a/drivers/remoteproc/meson_mx_ao_arc.c b/drivers/remoteproc/meson_mx_ao_arc.c
new file mode 100644
index 000000000000..86c0f3e48701
--- /dev/null
+++ b/drivers/remoteproc/meson_mx_ao_arc.c
@@ -0,0 +1,260 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2020 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/property.h>
+#include <linux/genalloc.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/remoteproc.h>
+#include <linux/reset.h>
+#include <linux/sizes.h>
+
+#include "remoteproc_internal.h"
+
+#define AO_REMAP_REG0						0x0
+#define AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU	GENMASK(3, 0)
+
+#define AO_REMAP_REG1						0x4
+#define AO_REMAP_REG1_MOVE_AHB_SRAM_TO_0X0_INSTEAD_OF_DDR	BIT(4)
+#define AO_REMAP_REG1_REMAP_AHB_SRAM_BITS_17_14_FOR_MEDIA_CPU	GENMASK(3, 0)
+
+#define AO_CPU_CNTL						0x0
+#define AO_CPU_CNTL_AHB_SRAM_BITS_31_20				GENMASK(28, 16)
+#define AO_CPU_CNTL_HALT					BIT(9)
+#define AO_CPU_CNTL_UNKNONWN					BIT(8)
+#define AO_CPU_CNTL_RUN						BIT(0)
+
+#define AO_CPU_STAT						0x4
+
+#define AO_SECURE_REG0						0x0
+#define AO_SECURE_REG0_AHB_SRAM_BITS_19_12			GENMASK(15, 8)
+
+/* Only bits [31:20] and [17:14] are usable, all other bits must be zero */
+#define MESON_AO_RPROC_SRAM_USABLE_BITS				0xfff3c000
+
+#define MESON_AO_RPROC_MEMORY_OFFSET				0x10000000
+
+struct meson_mx_ao_arc_rproc_priv {
+	void __iomem		*remap_base;
+	void __iomem		*cpu_base;
+	unsigned long		sram_va;
+	phys_addr_t		sram_pa;
+	size_t			sram_size;
+	struct gen_pool		*sram_pool;
+	struct reset_control	*arc_reset;
+	struct clk		*arc_pclk;
+	struct regmap		*secbus2_regmap;
+};
+
+static int meson_mx_ao_arc_rproc_start(struct rproc *rproc)
+{
+	struct meson_mx_ao_arc_rproc_priv *priv = rproc->priv;
+	phys_addr_t translated_sram_addr;
+	int ret;
+
+	ret = clk_prepare_enable(priv->arc_pclk);
+	if (ret)
+		return ret;
+
+	writel(FIELD_PREP(AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU,
+			       priv->sram_pa >> 14),
+	       priv->remap_base + AO_REMAP_REG0);
+
+	/*
+	 * The SRAM content as seen by the ARC core always starts at 0x0
+	 * regardless of the value given here (this was discovered by trial and
+	 * error). For SoCs older than Meson6 we probably have to set
+	 * AO_REMAP_REG1_MOVE_AHB_SRAM_TO_0X0_INSTEAD_OF_DDR to achieve the
+	 * same. (At least) For Meson8 and newer that bit must not be set.
+	 */
+	writel(0x0, priv->remap_base + AO_REMAP_REG1);
+
+	regmap_update_bits(priv->secbus2_regmap, AO_SECURE_REG0,
+			   AO_SECURE_REG0_AHB_SRAM_BITS_19_12,
+			   FIELD_PREP(AO_SECURE_REG0_AHB_SRAM_BITS_19_12,
+				      priv->sram_pa >> 12));
+
+	ret = reset_control_reset(priv->arc_reset);
+	if (ret) {
+		clk_disable_unprepare(priv->arc_pclk);
+		return ret;
+	}
+
+	usleep_range(10, 100);
+
+	/*
+	 * Convert from 0xd9000000 to 0xc9000000 as the vendor driver does.
+	 * This only seems to be relevant for the AO_CPU_CNTL register. It is
+	 * unknown why this is needed.
+	 */
+	translated_sram_addr = priv->sram_pa - MESON_AO_RPROC_MEMORY_OFFSET;
+
+	writel(FIELD_PREP(AO_CPU_CNTL_AHB_SRAM_BITS_31_20,
+			  translated_sram_addr >> 20) |
+	       AO_CPU_CNTL_UNKNONWN |
+	       AO_CPU_CNTL_RUN,
+	       priv->cpu_base + AO_CPU_CNTL);
+	usleep_range(20, 200);
+
+	return 0;
+}
+
+static int meson_mx_ao_arc_rproc_stop(struct rproc *rproc)
+{
+	struct meson_mx_ao_arc_rproc_priv *priv = rproc->priv;
+
+	writel(AO_CPU_CNTL_HALT, priv->cpu_base + AO_CPU_CNTL);
+
+	clk_disable_unprepare(priv->arc_pclk);
+
+	return 0;
+}
+
+static void *meson_mx_ao_arc_rproc_da_to_va(struct rproc *rproc, u64 da,
+					    size_t len, bool *is_iomem)
+{
+	struct meson_mx_ao_arc_rproc_priv *priv = rproc->priv;
+
+	if ((da + len) > priv->sram_size)
+		return NULL;
+
+	return (void *)priv->sram_va + da;
+}
+
+static struct rproc_ops meson_mx_ao_arc_rproc_ops = {
+	.start		= meson_mx_ao_arc_rproc_start,
+	.stop		= meson_mx_ao_arc_rproc_stop,
+	.da_to_va	= meson_mx_ao_arc_rproc_da_to_va,
+	.get_boot_addr	= rproc_elf_get_boot_addr,
+	.load		= rproc_elf_load_segments,
+	.sanity_check	= rproc_elf_sanity_check,
+};
+
+static int meson_mx_ao_arc_rproc_probe(struct platform_device *pdev)
+{
+	struct meson_mx_ao_arc_rproc_priv *priv;
+	struct device *dev = &pdev->dev;
+	const char *fw_name = NULL;
+	struct rproc *rproc;
+	int ret;
+
+	device_property_read_string(dev, "firmware-name", &fw_name);
+
+	rproc = devm_rproc_alloc(dev, "meson-mx-ao-arc",
+				 &meson_mx_ao_arc_rproc_ops, fw_name,
+				 sizeof(*priv));
+	if (!rproc)
+		return -ENOMEM;
+
+	rproc->has_iommu = false;
+	priv = rproc->priv;
+
+	priv->sram_pool = of_gen_pool_get(dev->of_node, "sram", 0);
+	if (!priv->sram_pool) {
+		dev_err(dev, "Could not get SRAM pool\n");
+		return -ENODEV;
+	}
+
+	priv->sram_size = gen_pool_avail(priv->sram_pool);
+
+	priv->sram_va = gen_pool_alloc(priv->sram_pool, priv->sram_size);
+	if (!priv->sram_va) {
+		dev_err(dev, "Could not alloc memory in SRAM pool\n");
+		return -ENOMEM;
+	}
+
+	priv->sram_pa = gen_pool_virt_to_phys(priv->sram_pool, priv->sram_va);
+	if (priv->sram_pa & ~MESON_AO_RPROC_SRAM_USABLE_BITS) {
+		dev_err(dev, "SRAM address contains unusable bits\n");
+		ret = -EINVAL;
+		goto err_free_genpool;
+	}
+
+	priv->secbus2_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
+							       "amlogic,secbus2");
+	if (IS_ERR(priv->secbus2_regmap)) {
+		dev_err(dev, "Failed to find SECBUS2 regmap\n");
+		ret = PTR_ERR(priv->secbus2_regmap);
+		goto err_free_genpool;
+	}
+
+	priv->remap_base = devm_platform_ioremap_resource_byname(pdev, "remap");
+	if (IS_ERR(priv->remap_base)) {
+		ret = PTR_ERR(priv->remap_base);
+		goto err_free_genpool;
+	}
+
+	priv->cpu_base = devm_platform_ioremap_resource_byname(pdev, "cpu");
+	if (IS_ERR(priv->cpu_base)) {
+		ret = PTR_ERR(priv->cpu_base);
+		goto err_free_genpool;
+	}
+
+	priv->arc_reset = devm_reset_control_get_exclusive(dev, NULL);
+	if (IS_ERR(priv->arc_reset)) {
+		dev_err(dev, "Failed to get ARC reset\n");
+		ret = PTR_ERR(priv->arc_reset);
+		goto err_free_genpool;
+	}
+
+	priv->arc_pclk = devm_clk_get(dev, NULL);
+	if (IS_ERR(priv->arc_pclk)) {
+		dev_err(dev, "Failed to get the ARC PCLK\n");
+		ret = PTR_ERR(priv->arc_pclk);
+		goto err_free_genpool;
+	}
+
+	platform_set_drvdata(pdev, rproc);
+
+	ret = rproc_add(rproc);
+	if (ret)
+		goto err_free_genpool;
+
+	return 0;
+
+err_free_genpool:
+	gen_pool_free(priv->sram_pool, priv->sram_va, priv->sram_size);
+	return ret;
+}
+
+static int meson_mx_ao_arc_rproc_remove(struct platform_device *pdev)
+{
+	struct rproc *rproc = platform_get_drvdata(pdev);
+	struct meson_mx_ao_arc_rproc_priv *priv = rproc->priv;
+
+	rproc_del(rproc);
+	gen_pool_free(priv->sram_pool, priv->sram_va, priv->sram_size);
+
+	return 0;
+}
+
+static const struct of_device_id meson_mx_ao_arc_rproc_match[] = {
+	{ .compatible = "amlogic,meson8-ao-arc" },
+	{ .compatible = "amlogic,meson8b-ao-arc" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, meson_mx_ao_arc_rproc_match);
+
+static struct platform_driver meson_mx_ao_arc_rproc_driver = {
+	.probe = meson_mx_ao_arc_rproc_probe,
+	.remove = meson_mx_ao_arc_rproc_remove,
+	.driver = {
+		.name = "meson-mx-ao-arc-rproc",
+		.of_match_table = meson_mx_ao_arc_rproc_match,
+	},
+};
+module_platform_driver(meson_mx_ao_arc_rproc_driver);
+
+MODULE_DESCRIPTION("Amlogic Meson6/8/8b/8m2 AO ARC remote processor driver");
+MODULE_AUTHOR("Martin Blumenstingl <martin.blumenstingl@googlemail.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.32.0

Re: [PATCH v3 2/2] remoteproc: meson-mx-ao-arc: Add a driver for the AO ARC remote procesor

[Mathieu Poirier]

Hi Martin,

On Sun, Jul 18, 2021 at 01:48:59AM +0200, Martin Blumenstingl wrote:
> Amlogic Meson6, Meson8, Meson8b and Meson8m2 embed an ARC core in the
> Always-On (AO) power-domain. This is typically used for waking up the
> ARM cores after system suspend.
> 
> The configuration is spread across three different registers:
> - AO_REMAP_REG0 which must be programmed to zero, it's actual purpose
>   is unknown. There is a second remap register which is not used in the
>   vendor kernel (which served as reference for this driver).
> - AO_CPU_CNTL is used to start and stop the ARC core.
> - AO_SECURE_REG0 in the SECBUS2 register area with unknown purpose.
> 
> To boot the ARC core we also need to enable it's gate clock and trigger
> a reset.
> 
> The actual code for this ARC core can come from an ELF binary, for
> example by building the Zephyr RTOS for an ARC EM4 core and then taking
> "zephyr.elf" as firmware. This executable does not have any "rsc table"
> so we are skipping rproc_elf_load_rsc_table (rproc_ops.parse_fw) and
> rproc_elf_find_loaded_rsc_table (rproc_ops.find_loaded_rsc_table).
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/remoteproc/Kconfig           |  11 ++
>  drivers/remoteproc/Makefile          |   1 +
>  drivers/remoteproc/meson_mx_ao_arc.c | 260 +++++++++++++++++++++++++++
>  3 files changed, 272 insertions(+)
>  create mode 100644 drivers/remoteproc/meson_mx_ao_arc.c
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 9a6eedc3994a..3197bbe38785 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -127,6 +127,17 @@ config KEYSTONE_REMOTEPROC
>  	  It's safe to say N here if you're not interested in the Keystone
>  	  DSPs or just want to use a bare minimum kernel.
>  
> +config MESON_MX_AO_ARC_REMOTEPROC
> +	tristate "Amlogic Meson6/8/8b/8m2 AO ARC remote processor support"
> +	depends on HAS_IOMEM
> +	depends on (ARM && ARCH_MESON) || COMPILE_TEST
> +	select GENERIC_ALLOCATOR
> +	help
> +	  Say m or y here to have support for the AO ARC remote processor
> +	  on Amlogic Meson6/Meson8/Meson8b/Meson8m2 SoCs. This is
> +	  typically used for system suspend.
> +	  If unusre say N.

s/unusre/unsure

> +
>  config PRU_REMOTEPROC
>  	tristate "TI PRU remoteproc support"
>  	depends on TI_PRUSS
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index bb26c9e4ef9c..ce1abeb30907 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>  obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
>  obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
>  obj-$(CONFIG_KEYSTONE_REMOTEPROC)	+= keystone_remoteproc.o
> +obj-$(CONFIG_MESON_MX_AO_ARC_REMOTEPROC)+= meson_mx_ao_arc.o
>  obj-$(CONFIG_PRU_REMOTEPROC)		+= pru_rproc.o
>  obj-$(CONFIG_QCOM_PIL_INFO)		+= qcom_pil_info.o
>  obj-$(CONFIG_QCOM_RPROC_COMMON)		+= qcom_common.o
> diff --git a/drivers/remoteproc/meson_mx_ao_arc.c b/drivers/remoteproc/meson_mx_ao_arc.c
> new file mode 100644
> index 000000000000..86c0f3e48701
> --- /dev/null
> +++ b/drivers/remoteproc/meson_mx_ao_arc.c
> @@ -0,0 +1,260 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2020 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/property.h>
> +#include <linux/genalloc.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>

Not sure this is needed.

> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/remoteproc.h>
> +#include <linux/reset.h>
> +#include <linux/sizes.h>
> +
> +#include "remoteproc_internal.h"
> +
> +#define AO_REMAP_REG0						0x0
> +#define AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU	GENMASK(3, 0)
> +
> +#define AO_REMAP_REG1						0x4
> +#define AO_REMAP_REG1_MOVE_AHB_SRAM_TO_0X0_INSTEAD_OF_DDR	BIT(4)
> +#define AO_REMAP_REG1_REMAP_AHB_SRAM_BITS_17_14_FOR_MEDIA_CPU	GENMASK(3, 0)
> +
> +#define AO_CPU_CNTL						0x0
> +#define AO_CPU_CNTL_AHB_SRAM_BITS_31_20				GENMASK(28, 16)
> +#define AO_CPU_CNTL_HALT					BIT(9)
> +#define AO_CPU_CNTL_UNKNONWN					BIT(8)
> +#define AO_CPU_CNTL_RUN						BIT(0)
> +
> +#define AO_CPU_STAT						0x4
> +
> +#define AO_SECURE_REG0						0x0
> +#define AO_SECURE_REG0_AHB_SRAM_BITS_19_12			GENMASK(15, 8)
> +
> +/* Only bits [31:20] and [17:14] are usable, all other bits must be zero */
> +#define MESON_AO_RPROC_SRAM_USABLE_BITS				0xfff3c000
> +
> +#define MESON_AO_RPROC_MEMORY_OFFSET				0x10000000
> +
> +struct meson_mx_ao_arc_rproc_priv {
> +	void __iomem		*remap_base;
> +	void __iomem		*cpu_base;
> +	unsigned long		sram_va;
> +	phys_addr_t		sram_pa;
> +	size_t			sram_size;
> +	struct gen_pool		*sram_pool;
> +	struct reset_control	*arc_reset;
> +	struct clk		*arc_pclk;
> +	struct regmap		*secbus2_regmap;
> +};
> +
> +static int meson_mx_ao_arc_rproc_start(struct rproc *rproc)
> +{
> +	struct meson_mx_ao_arc_rproc_priv *priv = rproc->priv;
> +	phys_addr_t translated_sram_addr;
> +	int ret;
> +
> +	ret = clk_prepare_enable(priv->arc_pclk);
> +	if (ret)
> +		return ret;
> +
> +	writel(FIELD_PREP(AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU,
> +			       priv->sram_pa >> 14),

Indentation problem

> +	       priv->remap_base + AO_REMAP_REG0);
> +
> +	/*
> +	 * The SRAM content as seen by the ARC core always starts at 0x0
> +	 * regardless of the value given here (this was discovered by trial and
> +	 * error). For SoCs older than Meson6 we probably have to set
> +	 * AO_REMAP_REG1_MOVE_AHB_SRAM_TO_0X0_INSTEAD_OF_DDR to achieve the
> +	 * same. (At least) For Meson8 and newer that bit must not be set.
> +	 */
> +	writel(0x0, priv->remap_base + AO_REMAP_REG1);
> +
> +	regmap_update_bits(priv->secbus2_regmap, AO_SECURE_REG0,
> +			   AO_SECURE_REG0_AHB_SRAM_BITS_19_12,
> +			   FIELD_PREP(AO_SECURE_REG0_AHB_SRAM_BITS_19_12,
> +				      priv->sram_pa >> 12));
> +
> +	ret = reset_control_reset(priv->arc_reset);
> +	if (ret) {
> +		clk_disable_unprepare(priv->arc_pclk);
> +		return ret;
> +	}
> +
> +	usleep_range(10, 100);

I've seen this kind of mysterious timeouts in other patchset based vendor trees.
You likely don't know why it is needed so I won't ask.

> +
> +	/*
> +	 * Convert from 0xd9000000 to 0xc9000000 as the vendor driver does.
> +	 * This only seems to be relevant for the AO_CPU_CNTL register. It is
> +	 * unknown why this is needed.
> +	 */
> +	translated_sram_addr = priv->sram_pa - MESON_AO_RPROC_MEMORY_OFFSET;
> +
> +	writel(FIELD_PREP(AO_CPU_CNTL_AHB_SRAM_BITS_31_20,
> +			  translated_sram_addr >> 20) |
> +	       AO_CPU_CNTL_UNKNONWN |
> +	       AO_CPU_CNTL_RUN,
> +	       priv->cpu_base + AO_CPU_CNTL);
> +	usleep_range(20, 200);
> +
> +	return 0;
> +}
> +
> +static int meson_mx_ao_arc_rproc_stop(struct rproc *rproc)
> +{
> +	struct meson_mx_ao_arc_rproc_priv *priv = rproc->priv;
> +
> +	writel(AO_CPU_CNTL_HALT, priv->cpu_base + AO_CPU_CNTL);
> +
> +	clk_disable_unprepare(priv->arc_pclk);
> +
> +	return 0;
> +}
> +
> +static void *meson_mx_ao_arc_rproc_da_to_va(struct rproc *rproc, u64 da,
> +					    size_t len, bool *is_iomem)
> +{
> +	struct meson_mx_ao_arc_rproc_priv *priv = rproc->priv;
> +
> +	if ((da + len) > priv->sram_size)
> +		return NULL;

I know from your comment in meson_mx_ao_arc_rproc_start() that the remote
processor's view of the SRAM is always mapped to 0.  Had I not read this driver
from top to bottom I'd find the above very perplexing.  Please add a comment.

> +
> +	return (void *)priv->sram_va + da;
> +}
> +
> +static struct rproc_ops meson_mx_ao_arc_rproc_ops = {
> +	.start		= meson_mx_ao_arc_rproc_start,
> +	.stop		= meson_mx_ao_arc_rproc_stop,
> +	.da_to_va	= meson_mx_ao_arc_rproc_da_to_va,
> +	.get_boot_addr	= rproc_elf_get_boot_addr,
> +	.load		= rproc_elf_load_segments,
> +	.sanity_check	= rproc_elf_sanity_check,
> +};
> +
> +static int meson_mx_ao_arc_rproc_probe(struct platform_device *pdev)
> +{
> +	struct meson_mx_ao_arc_rproc_priv *priv;
> +	struct device *dev = &pdev->dev;
> +	const char *fw_name = NULL;
> +	struct rproc *rproc;
> +	int ret;
> +
> +	device_property_read_string(dev, "firmware-name", &fw_name);
> +
> +	rproc = devm_rproc_alloc(dev, "meson-mx-ao-arc",
> +				 &meson_mx_ao_arc_rproc_ops, fw_name,
> +				 sizeof(*priv));
> +	if (!rproc)
> +		return -ENOMEM;
> +
> +	rproc->has_iommu = false;
> +	priv = rproc->priv;
> +
> +	priv->sram_pool = of_gen_pool_get(dev->of_node, "sram", 0);
> +	if (!priv->sram_pool) {
> +		dev_err(dev, "Could not get SRAM pool\n");
> +		return -ENODEV;
> +	}
> +
> +	priv->sram_size = gen_pool_avail(priv->sram_pool);
> +
> +	priv->sram_va = gen_pool_alloc(priv->sram_pool, priv->sram_size);
> +	if (!priv->sram_va) {
> +		dev_err(dev, "Could not alloc memory in SRAM pool\n");
> +		return -ENOMEM;
> +	}
> +
> +	priv->sram_pa = gen_pool_virt_to_phys(priv->sram_pool, priv->sram_va);
> +	if (priv->sram_pa & ~MESON_AO_RPROC_SRAM_USABLE_BITS) {
> +		dev_err(dev, "SRAM address contains unusable bits\n");
> +		ret = -EINVAL;
> +		goto err_free_genpool;
> +	}
> +
> +	priv->secbus2_regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
> +							       "amlogic,secbus2");
> +	if (IS_ERR(priv->secbus2_regmap)) {
> +		dev_err(dev, "Failed to find SECBUS2 regmap\n");
> +		ret = PTR_ERR(priv->secbus2_regmap);
> +		goto err_free_genpool;
> +	}
> +
> +	priv->remap_base = devm_platform_ioremap_resource_byname(pdev, "remap");
> +	if (IS_ERR(priv->remap_base)) {
> +		ret = PTR_ERR(priv->remap_base);
> +		goto err_free_genpool;
> +	}
> +
> +	priv->cpu_base = devm_platform_ioremap_resource_byname(pdev, "cpu");
> +	if (IS_ERR(priv->cpu_base)) {
> +		ret = PTR_ERR(priv->cpu_base);
> +		goto err_free_genpool;
> +	}
> +
> +	priv->arc_reset = devm_reset_control_get_exclusive(dev, NULL);
> +	if (IS_ERR(priv->arc_reset)) {

Function __reset_control_get() in __devm_reset_control_get() can return NULL so
this should be IS_ERR_OR_NULL().

> +		dev_err(dev, "Failed to get ARC reset\n");
> +		ret = PTR_ERR(priv->arc_reset);
> +		goto err_free_genpool;
> +	}
> +
> +	priv->arc_pclk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->arc_pclk)) {
> +		dev_err(dev, "Failed to get the ARC PCLK\n");
> +		ret = PTR_ERR(priv->arc_pclk);
> +		goto err_free_genpool;
> +	}
> +
> +	platform_set_drvdata(pdev, rproc);
> +
> +	ret = rproc_add(rproc);
> +	if (ret)
> +		goto err_free_genpool;
> +
> +	return 0;
> +
> +err_free_genpool:
> +	gen_pool_free(priv->sram_pool, priv->sram_va, priv->sram_size);
> +	return ret;
> +}
> +
> +static int meson_mx_ao_arc_rproc_remove(struct platform_device *pdev)
> +{
> +	struct rproc *rproc = platform_get_drvdata(pdev);
> +	struct meson_mx_ao_arc_rproc_priv *priv = rproc->priv;
> +
> +	rproc_del(rproc);
> +	gen_pool_free(priv->sram_pool, priv->sram_va, priv->sram_size);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id meson_mx_ao_arc_rproc_match[] = {
> +	{ .compatible = "amlogic,meson8-ao-arc" },
> +	{ .compatible = "amlogic,meson8b-ao-arc" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, meson_mx_ao_arc_rproc_match);
> +
> +static struct platform_driver meson_mx_ao_arc_rproc_driver = {
> +	.probe = meson_mx_ao_arc_rproc_probe,
> +	.remove = meson_mx_ao_arc_rproc_remove,
> +	.driver = {
> +		.name = "meson-mx-ao-arc-rproc",
> +		.of_match_table = meson_mx_ao_arc_rproc_match,
> +	},
> +};
> +module_platform_driver(meson_mx_ao_arc_rproc_driver);

This driver is squeaky clean. With the above:

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> +
> +MODULE_DESCRIPTION("Amlogic Meson6/8/8b/8m2 AO ARC remote processor driver");
> +MODULE_AUTHOR("Martin Blumenstingl <martin.blumenstingl@googlemail.com>");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.32.0
> 

Re: [PATCH v3 2/2] remoteproc: meson-mx-ao-arc: Add a driver for the AO ARC remote procesor

[Martin Blumenstingl]

Hi Mathieu,

thanks for taking the time to look into this!

(I will address any of your comments that I am not mentioning in this
email anymore. Thanks a lot for the suggestions!)

On Wed, Jul 28, 2021 at 7:58 PM Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
[...]
> > +     writel(FIELD_PREP(AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU,
> > +                            priv->sram_pa >> 14),
> Indentation problem
The idea here is to align priv->sram_pa with AO_REMAP_REG0... which
are both arguments to FIELD_PREP
Maybe using something like this will make that easier to read:
    tmp = FIELD_PREP(AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU,
                                     priv->sram_pa >> 14);
    writel(tmp, priv->remap_base + AO_REMAP_REG0);

What do you think: leave it as is or use a separate variable?

[...]
> > +     usleep_range(10, 100);
>
> I've seen this kind of mysterious timeouts in other patchset based vendor trees.
> You likely don't know why it is needed so I won't ask.
unfortunately this is also the case here

[...]
> > +     priv->arc_reset = devm_reset_control_get_exclusive(dev, NULL);
> > +     if (IS_ERR(priv->arc_reset)) {
>
> Function __reset_control_get() in __devm_reset_control_get() can return NULL so
> this should be IS_ERR_OR_NULL().
The logic in there is: return optional ? NULL : ERR_PTR(-...);
I am requesting a mandatory reset line here, so reset core will never
return NULL
See also [0]

For this reason I am not planning to change this

[...]
> This driver is squeaky clean. With the above:
>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
awesome, thank you!


Best regards,
Martin


[0] https://elixir.bootlin.com/linux/v5.14-rc4/source/include/linux/reset.h#L227

Re: [PATCH v3 2/2] remoteproc: meson-mx-ao-arc: Add a driver for the AO ARC remote procesor

[Mathieu Poirier]

On Wed, Aug 04, 2021 at 11:03:57PM +0200, Martin Blumenstingl wrote:
> Hi Mathieu,
> 
> thanks for taking the time to look into this!
> 
> (I will address any of your comments that I am not mentioning in this
> email anymore. Thanks a lot for the suggestions!)
> 
> On Wed, Jul 28, 2021 at 7:58 PM Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
> [...]
> > > +     writel(FIELD_PREP(AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU,
> > > +                            priv->sram_pa >> 14),
> > Indentation problem
> The idea here is to align priv->sram_pa with AO_REMAP_REG0... which
> are both arguments to FIELD_PREP

Right, this is what I would have expected.  When I applied the patch on my side
"priv->sram_pa ..." was aligned wiht the 'M' of "AO_REMAP_ ...".  

> Maybe using something like this will make that easier to read:
>     tmp = FIELD_PREP(AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU,
>                                      priv->sram_pa >> 14);
>     writel(tmp, priv->remap_base + AO_REMAP_REG0);

I think the main problem is that
AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU is simply too long.  I
suggest making is shorter and add a comment to describe exactly what it does.

> 
> What do you think: leave it as is or use a separate variable?
> 
> [...]
> > > +     usleep_range(10, 100);
> >
> > I've seen this kind of mysterious timeouts in other patchset based vendor trees.
> > You likely don't know why it is needed so I won't ask.
> unfortunately this is also the case here
> 
> [...]
> > > +     priv->arc_reset = devm_reset_control_get_exclusive(dev, NULL);
> > > +     if (IS_ERR(priv->arc_reset)) {
> >
> > Function __reset_control_get() in __devm_reset_control_get() can return NULL so
> > this should be IS_ERR_OR_NULL().
> The logic in there is: return optional ? NULL : ERR_PTR(-...);

Ok, so you meant to do that.  And I just checked reset_control_reset() and it does
account for a NULL parameter.  I'm good with this one but add a comment to
make sure future readers don't think you've omitted to properly deal with the
NULL return value.

> I am requesting a mandatory reset line here, so reset core will never
> return NULL
> See also [0]

Indeed, I've read that too.  Nonetheless __reset_control_get() can return NULL
by way of __reset_control_get_from_lookup().

> 
> For this reason I am not planning to change this
> 
> [...]
> > This driver is squeaky clean. With the above:
> >
> > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> awesome, thank you!
> 
> 
> Best regards,
> Martin
> 
> 
> [0] https://elixir.bootlin.com/linux/v5.14-rc4/source/include/linux/reset.h#L227

Re: [PATCH v3 2/2] remoteproc: meson-mx-ao-arc: Add a driver for the AO ARC remote procesor

[Martin Blumenstingl]

Hi Mathieu,

On Thu, Aug 5, 2021 at 6:15 PM Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
>
> On Wed, Aug 04, 2021 at 11:03:57PM +0200, Martin Blumenstingl wrote:
> > Hi Mathieu,
> >
> > thanks for taking the time to look into this!
> >
> > (I will address any of your comments that I am not mentioning in this
> > email anymore. Thanks a lot for the suggestions!)
> >
> > On Wed, Jul 28, 2021 at 7:58 PM Mathieu Poirier
> > <mathieu.poirier@linaro.org> wrote:
> > [...]
> > > > +     writel(FIELD_PREP(AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU,
> > > > +                            priv->sram_pa >> 14),
> > > Indentation problem
> > The idea here is to align priv->sram_pa with AO_REMAP_REG0... which
> > are both arguments to FIELD_PREP
>
> Right, this is what I would have expected.  When I applied the patch on my side
> "priv->sram_pa ..." was aligned wiht the 'M' of "AO_REMAP_ ...".
>
> > Maybe using something like this will make that easier to read:
> >     tmp = FIELD_PREP(AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU,
> >                                      priv->sram_pa >> 14);
> >     writel(tmp, priv->remap_base + AO_REMAP_REG0);
>
> I think the main problem is that
> AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU is simply too long.  I
> suggest making is shorter and add a comment to describe exactly what it does.
AO_CPU_CNTL_AHB_SRAM_BITS_31_20 is used below and when looking at it
now I think the alignment is also strange.
For the next version I'll go with the tmp variable as I think it
improves readability, even with the long(er) macro names.

[...]
> > > > +     priv->arc_reset = devm_reset_control_get_exclusive(dev, NULL);
> > > > +     if (IS_ERR(priv->arc_reset)) {
> > >
> > > Function __reset_control_get() in __devm_reset_control_get() can return NULL so
> > > this should be IS_ERR_OR_NULL().
> > The logic in there is: return optional ? NULL : ERR_PTR(-...);
>
> Ok, so you meant to do that.  And I just checked reset_control_reset() and it does
> account for a NULL parameter.  I'm good with this one but add a comment to
> make sure future readers don't think you've omitted to properly deal with the
> NULL return value.
>
> > I am requesting a mandatory reset line here, so reset core will never
> > return NULL
> > See also [0]
>
> Indeed, I've read that too.  Nonetheless __reset_control_get() can return NULL
> by way of __reset_control_get_from_lookup().
I could not find where __reset_control_get_from_lookup returns NULL in
case optional is false (which it is in this case because
devm_reset_control_get_exclusive requests a "mandatory" reset line).
Can you please point me to the problematic line(s) as I'd like to send
a patch (which fixes this) to the reset subsystem maintainers

$ git grep -A1 devm_reset_control_get_exclusive | grep IS_ERR_OR_NULL
drivers/remoteproc/ti_k3_r5_remoteproc.c-       if
(IS_ERR_OR_NULL(core->reset)) {
$

I suspect that this can be simplified then as well.


Best regards,
Martin


[0] https://elixir.bootlin.com/linux/v5.14-rc4/source/include/linux/reset.h#L227
[1] https://elixir.bootlin.com/linux/v5.14-rc4/source/drivers/reset/core.c#L932

Re: [PATCH v3 2/2] remoteproc: meson-mx-ao-arc: Add a driver for the AO ARC remote procesor

[Mathieu Poirier]

On Sun, 8 Aug 2021 at 14:37, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
>
> Hi Mathieu,
>
> On Thu, Aug 5, 2021 at 6:15 PM Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
> >
> > On Wed, Aug 04, 2021 at 11:03:57PM +0200, Martin Blumenstingl wrote:
> > > Hi Mathieu,
> > >
> > > thanks for taking the time to look into this!
> > >
> > > (I will address any of your comments that I am not mentioning in this
> > > email anymore. Thanks a lot for the suggestions!)
> > >
> > > On Wed, Jul 28, 2021 at 7:58 PM Mathieu Poirier
> > > <mathieu.poirier@linaro.org> wrote:
> > > [...]
> > > > > +     writel(FIELD_PREP(AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU,
> > > > > +                            priv->sram_pa >> 14),
> > > > Indentation problem
> > > The idea here is to align priv->sram_pa with AO_REMAP_REG0... which
> > > are both arguments to FIELD_PREP
> >
> > Right, this is what I would have expected.  When I applied the patch on my side
> > "priv->sram_pa ..." was aligned wiht the 'M' of "AO_REMAP_ ...".
> >
> > > Maybe using something like this will make that easier to read:
> > >     tmp = FIELD_PREP(AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU,
> > >                                      priv->sram_pa >> 14);
> > >     writel(tmp, priv->remap_base + AO_REMAP_REG0);
> >
> > I think the main problem is that
> > AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU is simply too long.  I
> > suggest making is shorter and add a comment to describe exactly what it does.
> AO_CPU_CNTL_AHB_SRAM_BITS_31_20 is used below and when looking at it
> now I think the alignment is also strange.
> For the next version I'll go with the tmp variable as I think it
> improves readability, even with the long(er) macro names.
>
> [...]
> > > > > +     priv->arc_reset = devm_reset_control_get_exclusive(dev, NULL);
> > > > > +     if (IS_ERR(priv->arc_reset)) {
> > > >
> > > > Function __reset_control_get() in __devm_reset_control_get() can return NULL so
> > > > this should be IS_ERR_OR_NULL().
> > > The logic in there is: return optional ? NULL : ERR_PTR(-...);
> >
> > Ok, so you meant to do that.  And I just checked reset_control_reset() and it does
> > account for a NULL parameter.  I'm good with this one but add a comment to
> > make sure future readers don't think you've omitted to properly deal with the
> > NULL return value.
> >
> > > I am requesting a mandatory reset line here, so reset core will never
> > > return NULL
> > > See also [0]
> >
> > Indeed, I've read that too.  Nonetheless __reset_control_get() can return NULL
> > by way of __reset_control_get_from_lookup().
> I could not find where __reset_control_get_from_lookup returns NULL in
> case optional is false (which it is in this case because
> devm_reset_control_get_exclusive requests a "mandatory" reset line).
> Can you please point me to the problematic line(s) as I'd like to send
> a patch (which fixes this) to the reset subsystem maintainers
>

I am currently traveling - I will get back to you in a week or so.

> $ git grep -A1 devm_reset_control_get_exclusive | grep IS_ERR_OR_NULL
> drivers/remoteproc/ti_k3_r5_remoteproc.c-       if
> (IS_ERR_OR_NULL(core->reset)) {
> $
>
> I suspect that this can be simplified then as well.
>
>
> Best regards,
> Martin
>
>
> [0] https://elixir.bootlin.com/linux/v5.14-rc4/source/include/linux/reset.h#L227
> [1] https://elixir.bootlin.com/linux/v5.14-rc4/source/drivers/reset/core.c#L932

Re: [PATCH v3 2/2] remoteproc: meson-mx-ao-arc: Add a driver for the AO ARC remote procesor

[Mathieu Poirier]

On Thu, Aug 05, 2021 at 10:15:06AM -0600, Mathieu Poirier wrote:
> On Wed, Aug 04, 2021 at 11:03:57PM +0200, Martin Blumenstingl wrote:
> > Hi Mathieu,
> > 
> > thanks for taking the time to look into this!
> > 
> > (I will address any of your comments that I am not mentioning in this
> > email anymore. Thanks a lot for the suggestions!)
> > 
> > On Wed, Jul 28, 2021 at 7:58 PM Mathieu Poirier
> > <mathieu.poirier@linaro.org> wrote:
> > [...]
> > > > +     writel(FIELD_PREP(AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU,
> > > > +                            priv->sram_pa >> 14),
> > > Indentation problem
> > The idea here is to align priv->sram_pa with AO_REMAP_REG0... which
> > are both arguments to FIELD_PREP
> 
> Right, this is what I would have expected.  When I applied the patch on my side
> "priv->sram_pa ..." was aligned wiht the 'M' of "AO_REMAP_ ...".  
> 
> > Maybe using something like this will make that easier to read:
> >     tmp = FIELD_PREP(AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU,
> >                                      priv->sram_pa >> 14);
> >     writel(tmp, priv->remap_base + AO_REMAP_REG0);
> 
> I think the main problem is that
> AO_REMAP_REG0_REMAP_AHB_SRAM_BITS_17_14_FOR_ARM_CPU is simply too long.  I
> suggest making is shorter and add a comment to describe exactly what it does.
> 
> > 
> > What do you think: leave it as is or use a separate variable?
> > 
> > [...]
> > > > +     usleep_range(10, 100);
> > >
> > > I've seen this kind of mysterious timeouts in other patchset based vendor trees.
> > > You likely don't know why it is needed so I won't ask.
> > unfortunately this is also the case here
> > 
> > [...]
> > > > +     priv->arc_reset = devm_reset_control_get_exclusive(dev, NULL);
> > > > +     if (IS_ERR(priv->arc_reset)) {
> > >
> > > Function __reset_control_get() in __devm_reset_control_get() can return NULL so
> > > this should be IS_ERR_OR_NULL().
> > The logic in there is: return optional ? NULL : ERR_PTR(-...);
> 
> Ok, so you meant to do that.  And I just checked reset_control_reset() and it does
> account for a NULL parameter.  I'm good with this one but add a comment to
> make sure future readers don't think you've omitted to properly deal with the
> NULL return value.
> 
> > I am requesting a mandatory reset line here, so reset core will never
> > return NULL
> > See also [0]
> 
> Indeed, I've read that too.  Nonetheless __reset_control_get() can return NULL
> by way of __reset_control_get_from_lookup().
> 

You are correct, in your case checking for IS_ERR() is sufficient.

> > 
> > For this reason I am not planning to change this
> > 
> > [...]
> > > This driver is squeaky clean. With the above:
> > >
> > > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > awesome, thank you!
> > 
> > 
> > Best regards,
> > Martin
> > 
> > 
> > [0] https://elixir.bootlin.com/linux/v5.14-rc4/source/include/linux/reset.h#L227