public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Add a MicroBlaze remoteproc driver and binding
@ 2026-04-14 16:15 Ben Levinsky
  2026-04-14 16:15 ` [PATCH 1/2] dt-bindings: remoteproc: add AMD MicroBlaze binding Ben Levinsky
  2026-04-14 16:15 ` [PATCH 2/2] remoteproc: add AMD MicroBlaze driver Ben Levinsky
  0 siblings, 2 replies; 15+ messages in thread
From: Ben Levinsky @ 2026-04-14 16:15 UTC (permalink / raw)
  To: andersson, mathieu.poirier
  Cc: robh, krzk+dt, conor+dt, linux-remoteproc, devicetree,
	linux-kernel, ben.levinsky, tanmay.shah, michal.simek

This series adds initial remoteproc support for AMD MicroBlaze soft
processors.

The binding models the MicroBlaze remote processor as a child node whose
reg property describes the executable firmware memory window in the
MicroBlaze-local address space. The parent bus ranges property provides
the standard devicetree address translation to the Linux-visible system
physical address.

The driver uses that translated memory window as the executable
remoteproc carveout and coredump segment, holds the MicroBlaze in reset
through an active-low GPIO until firmware loading completes, and allows
the firmware image to be selected with the optional firmware-name
property. Firmware images without a resource table are also accepted.

This initial series focuses on basic load/start/stop support for a
single executable memory window. It does not add any transport or
mailbox integration.

Ben Levinsky (2):
  dt-bindings: remoteproc: add AMD MicroBlaze binding
  remoteproc: add AMD MicroBlaze driver

 .../bindings/remoteproc/amd,microblaze.yaml   |  67 ++++++
 MAINTAINERS                                   |   7 +
 drivers/remoteproc/Kconfig                    |  15 ++
 drivers/remoteproc/Makefile                   |   1 +
 drivers/remoteproc/amd_microblaze_rproc.c     | 202 ++++++++++++++++++
 5 files changed, 292 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/amd,microblaze.yaml
 create mode 100644 drivers/remoteproc/amd_microblaze_rproc.c

-- 
2.34.1

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/2] dt-bindings: remoteproc: add AMD MicroBlaze binding
  2026-04-14 16:15 [PATCH 0/2] Add a MicroBlaze remoteproc driver and binding Ben Levinsky
@ 2026-04-14 16:15 ` Ben Levinsky
  2026-04-14 17:29   ` Rob Herring (Arm)
  2026-04-14 17:53   ` Krzysztof Kozlowski
  2026-04-14 16:15 ` [PATCH 2/2] remoteproc: add AMD MicroBlaze driver Ben Levinsky
  1 sibling, 2 replies; 15+ messages in thread
From: Ben Levinsky @ 2026-04-14 16:15 UTC (permalink / raw)
  To: andersson, mathieu.poirier
  Cc: robh, krzk+dt, conor+dt, linux-remoteproc, devicetree,
	linux-kernel, ben.levinsky, tanmay.shah, michal.simek

Describe an AMD MicroBlaze remote processor controlled
through the remoteproc framework.

The binding models the MicroBlaze remoteproc device as a
child node whose reg property describes the executable firmware
memory window in the MicroBlaze-local address space. The parent bus
node provides standard devicetree address translation through ranges
so Linux can access the same memory through the system physical
address space.

An active-low reset GPIO holds the MicroBlaze in reset until
firmware loading completes.

Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
---
 .../bindings/remoteproc/amd,microblaze.yaml   | 67 +++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/amd,microblaze.yaml

diff --git a/Documentation/devicetree/bindings/remoteproc/amd,microblaze.yaml b/Documentation/devicetree/bindings/remoteproc/amd,microblaze.yaml
new file mode 100644
index 000000000000..2811610204a9
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/amd,microblaze.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/remoteproc/amd,microblaze.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMD MicroBlaze remote processor
+
+maintainers:
+  - Ben Levinsky <ben.levinsky@amd.com>
+
+description:
+  MicroBlaze remote processor controlled by Linux through the remoteproc
+  framework.
+
+  The executable firmware memory window is described in the
+  MicroBlaze-local address space by the node's reg property and translated
+  to the system physical address space with standard devicetree address
+  translation provided by the parent bus node's ranges property.
+
+properties:
+  $nodename:
+    pattern: "^remoteproc@[0-9a-f]+$"
+
+  compatible:
+    const: amd,microblaze
+
+  reg:
+    maxItems: 1
+    description:
+      MicroBlaze-local address and size of the executable firmware memory
+      window.
+
+  firmware-name:
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - reset-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    / {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      bus@b0000000 {
+        compatible = "simple-pm-bus";
+        #address-cells = <1>;
+        #size-cells = <1>;
+        ranges = <0x0 0x0 0xb0000000 0x40000>;
+
+        remoteproc@0 {
+          compatible = "amd,microblaze";
+          reg = <0x0 0x40000>;
+          reset-gpios = <&mbv_reset_gpio 0 GPIO_ACTIVE_LOW>;
+        };
+      };
+    };
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/2] remoteproc: add AMD MicroBlaze driver
  2026-04-14 16:15 [PATCH 0/2] Add a MicroBlaze remoteproc driver and binding Ben Levinsky
  2026-04-14 16:15 ` [PATCH 1/2] dt-bindings: remoteproc: add AMD MicroBlaze binding Ben Levinsky
@ 2026-04-14 16:15 ` Ben Levinsky
  2026-04-14 17:56   ` Krzysztof Kozlowski
  1 sibling, 1 reply; 15+ messages in thread
From: Ben Levinsky @ 2026-04-14 16:15 UTC (permalink / raw)
  To: andersson, mathieu.poirier
  Cc: robh, krzk+dt, conor+dt, linux-remoteproc, devicetree,
	linux-kernel, ben.levinsky, tanmay.shah, michal.simek

 Add an AMD MicroBlaze remoteproc driver.

 The driver parses the executable firmware memory window from
 the remoteproc device node's reg property, interprets that
 address and size in the MicroBlaze-local address space, and
 then uses standard devicetree address translation through the
 parent bus ranges property to obtain the corresponding
 Linux-visible system physical address.

 The resulting translated region is registered as the executable
 remoteproc carveout and coredump segment.

 The MicroBlaze is controlled through an active-low reset GPIO and kept in
 reset until firmware loading completes.

 The firmware-name property is optional, allowing firmware to be
 assigned later through the remoteproc framework.

Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
---
 MAINTAINERS                               |   7 +
 drivers/remoteproc/Kconfig                |  15 ++
 drivers/remoteproc/Makefile               |   1 +
 drivers/remoteproc/amd_microblaze_rproc.c | 202 ++++++++++++++++++++++
 4 files changed, 225 insertions(+)
 create mode 100644 drivers/remoteproc/amd_microblaze_rproc.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a7e30df8ba5d..a70397b9ca71 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1172,6 +1172,13 @@ F:	drivers/gpu/drm/amd/include/vi_structs.h
 F:	include/uapi/linux/kfd_ioctl.h
 F:	include/uapi/linux/kfd_sysfs.h
 
+AMD MICROBLAZE REMOTEPROC DRIVER
+M:	Ben Levinsky <ben.levinsky@amd.com>
+L:	linux-remoteproc@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/remoteproc/amd,microblaze.yaml
+F:	drivers/remoteproc/amd_microblaze_rproc.c
+
 AMD MP2 I2C DRIVER
 M:	Elie Morisse <syniurge@gmail.com>
 M:	Shyam Sundar S K <shyam-sundar.s-k@amd.com>
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index ee54436fea5a..76de743241a7 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -23,6 +23,21 @@ config REMOTEPROC_CDEV
 
 	  It's safe to say N if you don't want to use this interface.
 
+config AMD_MICROBLAZE_REMOTEPROC
+	tristate "AMD MicroBlaze remoteproc support"
+	depends on OF
+	depends on GPIOLIB || COMPILE_TEST
+	help
+	  Say y or m here to support a MicroBlaze remote processor controlled
+	  by Linux through the remoteproc framework.
+
+	  This driver matches designs where executable firmware memory is
+	  described in the MicroBlaze-local address space and translated to
+	  the system physical address space with standard devicetree address
+	  translation.
+
+	  If unsure, say N.
+
 config IMX_REMOTEPROC
 	tristate "i.MX remoteproc support"
 	depends on ARCH_MXC
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 1c7598b8475d..8717a0aec2e6 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -11,6 +11,7 @@ remoteproc-y				+= remoteproc_sysfs.o
 remoteproc-y				+= remoteproc_virtio.o
 remoteproc-y				+= remoteproc_elf_loader.o
 obj-$(CONFIG_REMOTEPROC_CDEV)		+= remoteproc_cdev.o
+obj-$(CONFIG_AMD_MICROBLAZE_REMOTEPROC)	+= amd_microblaze_rproc.o
 obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
 obj-$(CONFIG_IMX_DSP_REMOTEPROC)	+= imx_dsp_rproc.o
 obj-$(CONFIG_INGENIC_VPU_RPROC)		+= ingenic_rproc.o
diff --git a/drivers/remoteproc/amd_microblaze_rproc.c b/drivers/remoteproc/amd_microblaze_rproc.c
new file mode 100644
index 000000000000..c664a70d246b
--- /dev/null
+++ b/drivers/remoteproc/amd_microblaze_rproc.c
@@ -0,0 +1,202 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD MicroBlaze Remote Processor driver
+ *
+ * Copyright (C) 2026 Advanced Micro Devices, Inc.
+ *
+ * This driver supports a MicroBlaze remote processor managed by Linux
+ * through the remoteproc framework.
+ *
+ * The executable firmware memory is described in the MicroBlaze-local
+ * address space and translated to the Linux-visible system physical
+ * address with standard devicetree address translation.
+ *
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/gpio/consumer.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/remoteproc.h>
+
+#include "remoteproc_internal.h"
+
+struct mb_rproc {
+	struct device *dev;
+	struct gpio_desc *reset;
+};
+
+static int mb_rproc_mem_region_map(struct rproc *rproc,
+				   struct rproc_mem_entry *mem)
+{
+	void __iomem *va;
+
+	va = ioremap_wc(mem->dma, mem->len);
+	if (!va)
+		return -ENOMEM;
+
+	mem->va = (__force void *)va;
+	mem->is_iomem = true;
+
+	return 0;
+}
+
+static int mb_rproc_mem_region_unmap(struct rproc *rproc,
+				     struct rproc_mem_entry *mem)
+{
+	iounmap((void __iomem *)mem->va);
+
+	return 0;
+}
+
+static int mb_rproc_prepare(struct rproc *rproc)
+{
+	struct mb_rproc *mb = rproc->priv;
+	struct rproc_mem_entry *mem;
+	struct resource res;
+	u64 da, size;
+	int ret;
+
+	ret = of_property_read_reg(mb->dev->of_node, 0, &da, &size);
+	if (ret) {
+		dev_err(mb->dev, "failed to parse executable memory reg\n");
+		return ret;
+	}
+
+	if (!size || size > U32_MAX) {
+		dev_err(mb->dev, "invalid executable memory size\n");
+		return -EINVAL;
+	}
+
+	ret = of_address_to_resource(mb->dev->of_node, 0, &res);
+	if (ret) {
+		dev_err(mb->dev, "failed to translate executable memory reg\n");
+		return ret;
+	}
+
+	mem = rproc_mem_entry_init(mb->dev, NULL, (dma_addr_t)res.start,
+				   (size_t)size, da,
+				   mb_rproc_mem_region_map,
+				   mb_rproc_mem_region_unmap,
+				   dev_name(mb->dev));
+	if (!mem)
+		return -ENOMEM;
+
+	rproc_add_carveout(rproc, mem);
+	rproc_coredump_add_segment(rproc, da, (size_t)size);
+
+	return 0;
+}
+
+static int mb_rproc_start(struct rproc *rproc)
+{
+	struct mb_rproc *mb = rproc->priv;
+
+	/* reset-gpios is declared active-low, so logical 0 releases reset */
+	gpiod_set_value_cansleep(mb->reset, 0);
+
+	return 0;
+}
+
+static int mb_rproc_stop(struct rproc *rproc)
+{
+	struct mb_rproc *mb = rproc->priv;
+
+	/* reset-gpios is declared active-low, so logical 1 asserts reset */
+	gpiod_set_value_cansleep(mb->reset, 1);
+
+	return 0;
+}
+
+static int mb_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
+{
+	int ret;
+
+	ret = rproc_elf_load_rsc_table(rproc, fw);
+	if (ret == -EINVAL) {
+		dev_dbg(&rproc->dev, "no resource table found\n");
+		return 0;
+	}
+
+	return ret;
+}
+
+static const struct rproc_ops mb_rproc_ops = {
+	.prepare	= mb_rproc_prepare,
+	.start		= mb_rproc_start,
+	.stop		= mb_rproc_stop,
+	.load		= rproc_elf_load_segments,
+	.sanity_check	= rproc_elf_sanity_check,
+	.get_boot_addr	= rproc_elf_get_boot_addr,
+	.parse_fw	= mb_rproc_parse_fw,
+};
+
+static int mb_rproc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mb_rproc *mb;
+	struct rproc *rproc;
+	const char *fw_name = NULL;
+	int ret;
+
+	ret = rproc_of_parse_firmware(dev, 0, &fw_name);
+	if (ret < 0 && ret != -EINVAL)
+		return dev_err_probe(dev, ret,
+				     "failed to parse firmware-name property\n");
+
+	rproc = devm_rproc_alloc(dev, dev_name(dev), &mb_rproc_ops, fw_name,
+				 sizeof(*mb));
+	if (!rproc)
+		return -ENOMEM;
+
+	mb = rproc->priv;
+	mb->dev = dev;
+
+	/*
+	 * Keep the MicroBlaze in reset until remoteproc has finished loading
+	 * firmware into the executable memory window described by reg and
+	 * translated through the parent bus ranges property.
+	 */
+	mb->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(mb->reset))
+		return dev_err_probe(dev, PTR_ERR(mb->reset),
+				     "failed to get reset gpio\n");
+
+	rproc->auto_boot = false;
+
+	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to set DMA mask\n");
+
+	platform_set_drvdata(pdev, rproc);
+
+	ret = devm_rproc_add(dev, rproc);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register rproc\n");
+
+	dev_dbg(dev, "MicroBlaze remoteproc registered\n");
+
+	return 0;
+}
+
+static const struct of_device_id mb_rproc_of_match[] = {
+	{ .compatible = "amd,microblaze" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mb_rproc_of_match);
+
+static struct platform_driver mb_rproc_driver = {
+	.probe = mb_rproc_probe,
+	.driver = {
+		.name = "amd-microblaze-rproc",
+		.of_match_table = mb_rproc_of_match,
+	},
+};
+module_platform_driver(mb_rproc_driver);
+
+MODULE_DESCRIPTION("AMD MicroBlaze Remote Processor driver");
+MODULE_AUTHOR("Ben Levinsky");
+MODULE_LICENSE("GPL");
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] dt-bindings: remoteproc: add AMD MicroBlaze binding
  2026-04-14 16:15 ` [PATCH 1/2] dt-bindings: remoteproc: add AMD MicroBlaze binding Ben Levinsky
@ 2026-04-14 17:29   ` Rob Herring (Arm)
  2026-04-14 17:53   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 15+ messages in thread
From: Rob Herring (Arm) @ 2026-04-14 17:29 UTC (permalink / raw)
  To: Ben Levinsky
  Cc: tanmay.shah, linux-kernel, mathieu.poirier, conor+dt, devicetree,
	andersson, michal.simek, krzk+dt, linux-remoteproc


On Tue, 14 Apr 2026 09:15:57 -0700, Ben Levinsky wrote:
> Describe an AMD MicroBlaze remote processor controlled
> through the remoteproc framework.
> 
> The binding models the MicroBlaze remoteproc device as a
> child node whose reg property describes the executable firmware
> memory window in the MicroBlaze-local address space. The parent bus
> node provides standard devicetree address translation through ranges
> so Linux can access the same memory through the system physical
> address space.
> 
> An active-low reset GPIO holds the MicroBlaze in reset until
> firmware loading completes.
> 
> Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
> ---
>  .../bindings/remoteproc/amd,microblaze.yaml   | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/amd,microblaze.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/remoteproc/amd,microblaze.example.dtb: /: 'compatible' is a required property
	from schema $id: http://devicetree.org/schemas/root-node.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/remoteproc/amd,microblaze.example.dtb: /: 'model' is a required property
	from schema $id: http://devicetree.org/schemas/root-node.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/remoteproc/amd,microblaze.example.dtb: bus@b0000000 (simple-pm-bus): 'anyOf' conditional failed, one must be fixed:
	'clocks' is a required property
	'power-domains' is a required property
	from schema $id: http://devicetree.org/schemas/bus/simple-pm-bus.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.kernel.org/project/devicetree/patch/20260414161558.2579920-2-ben.levinsky@amd.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] dt-bindings: remoteproc: add AMD MicroBlaze binding
  2026-04-14 16:15 ` [PATCH 1/2] dt-bindings: remoteproc: add AMD MicroBlaze binding Ben Levinsky
  2026-04-14 17:29   ` Rob Herring (Arm)
@ 2026-04-14 17:53   ` Krzysztof Kozlowski
  2026-04-15  6:16     ` Michal Simek
  1 sibling, 1 reply; 15+ messages in thread
From: Krzysztof Kozlowski @ 2026-04-14 17:53 UTC (permalink / raw)
  To: Ben Levinsky, andersson, mathieu.poirier
  Cc: robh, krzk+dt, conor+dt, linux-remoteproc, devicetree,
	linux-kernel, tanmay.shah, michal.simek

On 14/04/2026 18:15, Ben Levinsky wrote:

A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> +---
> +$id: http://devicetree.org/schemas/remoteproc/amd,microblaze.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AMD MicroBlaze remote processor
> +
> +maintainers:
> +  - Ben Levinsky <ben.levinsky@amd.com>
> +
> +description:
> +  MicroBlaze remote processor controlled by Linux through the remoteproc
> +  framework.

Describe hardware, not Linux frameworks. IOW, Linux framework is here
irrelevant.

> +
> +  The executable firmware memory window is described in the
> +  MicroBlaze-local address space by the node's reg property and translated
> +  to the system physical address space with standard devicetree address
> +  translation provided by the parent bus node's ranges property.
> +
> +properties:
> +  $nodename:
> +    pattern: "^remoteproc@[0-9a-f]+$"
> +
> +  compatible:
> +    const: amd,microblaze

microblaze is architecture, so this feels way too generic. You need SoC
specific compatibles and I suggest do not reference architecture, but
name or the function of the processor, if there are such.


> +
> +  reg:
> +    maxItems: 1
> +    description:
> +      MicroBlaze-local address and size of the executable firmware memory
> +      window.
> +
> +  firmware-name:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - reset-gpios
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    / {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      bus@b0000000 {
> +        compatible = "simple-pm-bus";
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        ranges = <0x0 0x0 0xb0000000 0x40000>;
> +

Drop all above.

> +        remoteproc@0 {
> +          compatible = "amd,microblaze";
> +          reg = <0x0 0x40000>;
> +          reset-gpios = <&mbv_reset_gpio 0 GPIO_ACTIVE_LOW>;
> +        };
> +      };
> +    };


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] remoteproc: add AMD MicroBlaze driver
  2026-04-14 16:15 ` [PATCH 2/2] remoteproc: add AMD MicroBlaze driver Ben Levinsky
@ 2026-04-14 17:56   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2026-04-14 17:56 UTC (permalink / raw)
  To: Ben Levinsky, andersson, mathieu.poirier
  Cc: robh, krzk+dt, conor+dt, linux-remoteproc, devicetree,
	linux-kernel, tanmay.shah, michal.simek

On 14/04/2026 18:15, Ben Levinsky wrote:
>  Add an AMD MicroBlaze remoteproc driver.
> 
>  The driver parses the executable firmware memory window from
>  the remoteproc device node's reg property, interprets that
>  address and size in the MicroBlaze-local address space, and
>  then uses standard devicetree address translation through the
>  parent bus ranges property to obtain the corresponding
>  Linux-visible system physical address.
> 
>  The resulting translated region is registered as the executable
>  remoteproc carveout and coredump segment.
> 
>  The MicroBlaze is controlled through an active-low reset GPIO and kept in
>  reset until firmware loading completes.
> 
>  The firmware-name property is optional, allowing firmware to be
>  assigned later through the remoteproc framework.
> 

Fix your commit msg so it uses sane style, not some indentation.

Look at how other commits are written, if you have doubts.

...

> +
> +static int mb_rproc_start(struct rproc *rproc)
> +{
> +	struct mb_rproc *mb = rproc->priv;
> +
> +	/* reset-gpios is declared active-low, so logical 0 releases reset */

If reset-gpios is declared active-high, then logical 0 also releases reset.

Drop comment, not correct.

> +	gpiod_set_value_cansleep(mb->reset, 0);
> +
> +	return 0;
> +}
> +
> +static int mb_rproc_stop(struct rproc *rproc)
> +{
> +	struct mb_rproc *mb = rproc->priv;
> +
> +	/* reset-gpios is declared active-low, so logical 1 asserts reset */
> +	gpiod_set_value_cansleep(mb->reset, 1);
> +
> +	return 0;
> +}
> +
> +static int mb_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
> +{
> +	int ret;
> +
> +	ret = rproc_elf_load_rsc_table(rproc, fw);
> +	if (ret == -EINVAL) {
> +		dev_dbg(&rproc->dev, "no resource table found\n");
> +		return 0;
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct rproc_ops mb_rproc_ops = {
> +	.prepare	= mb_rproc_prepare,
> +	.start		= mb_rproc_start,
> +	.stop		= mb_rproc_stop,
> +	.load		= rproc_elf_load_segments,
> +	.sanity_check	= rproc_elf_sanity_check,
> +	.get_boot_addr	= rproc_elf_get_boot_addr,
> +	.parse_fw	= mb_rproc_parse_fw,
> +};
> +
> +static int mb_rproc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mb_rproc *mb;
> +	struct rproc *rproc;
> +	const char *fw_name = NULL;
> +	int ret;
> +
> +	ret = rproc_of_parse_firmware(dev, 0, &fw_name);
> +	if (ret < 0 && ret != -EINVAL)
> +		return dev_err_probe(dev, ret,
> +				     "failed to parse firmware-name property\n");
> +
> +	rproc = devm_rproc_alloc(dev, dev_name(dev), &mb_rproc_ops, fw_name,
> +				 sizeof(*mb));
> +	if (!rproc)
> +		return -ENOMEM;
> +
> +	mb = rproc->priv;
> +	mb->dev = dev;
> +
> +	/*
> +	 * Keep the MicroBlaze in reset until remoteproc has finished loading
> +	 * firmware into the executable memory window described by reg and
> +	 * translated through the parent bus ranges property.
> +	 */
> +	mb->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(mb->reset))
> +		return dev_err_probe(dev, PTR_ERR(mb->reset),
> +				     "failed to get reset gpio\n");
> +
> +	rproc->auto_boot = false;
> +
> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to set DMA mask\n");
> +
> +	platform_set_drvdata(pdev, rproc);
> +
> +	ret = devm_rproc_add(dev, rproc);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register rproc\n");
> +
> +	dev_dbg(dev, "MicroBlaze remoteproc registered\n");

Drop. This does not look like useful printk message. Drivers should be
silent on success:
https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/coding-style.rst#L913
https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/debugging/driver_development_debugging_guide.rst#L79

Core already gives way to see probe success.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] dt-bindings: remoteproc: add AMD MicroBlaze binding
  2026-04-14 17:53   ` Krzysztof Kozlowski
@ 2026-04-15  6:16     ` Michal Simek
  2026-04-15  6:50       ` Krzysztof Kozlowski
  2026-04-15 12:19       ` Rob Herring
  0 siblings, 2 replies; 15+ messages in thread
From: Michal Simek @ 2026-04-15  6:16 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Ben Levinsky, andersson, mathieu.poirier
  Cc: robh, krzk+dt, conor+dt, linux-remoteproc, devicetree,
	linux-kernel, tanmay.shah



On 4/14/26 19:53, Krzysztof Kozlowski wrote:
> On 14/04/2026 18:15, Ben Levinsky wrote:
> 
> A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
> prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> 
>> +---
>> +$id: http://devicetree.org/schemas/remoteproc/amd,microblaze.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: AMD MicroBlaze remote processor
>> +
>> +maintainers:
>> +  - Ben Levinsky <ben.levinsky@amd.com>
>> +
>> +description:
>> +  MicroBlaze remote processor controlled by Linux through the remoteproc
>> +  framework.
> 
> Describe hardware, not Linux frameworks. IOW, Linux framework is here
> irrelevant.
> 
>> +
>> +  The executable firmware memory window is described in the
>> +  MicroBlaze-local address space by the node's reg property and translated
>> +  to the system physical address space with standard devicetree address
>> +  translation provided by the parent bus node's ranges property.
>> +
>> +properties:
>> +  $nodename:
>> +    pattern: "^remoteproc@[0-9a-f]+$"
>> +
>> +  compatible:
>> +    const: amd,microblaze
> 
> microblaze is architecture, so this feels way too generic. You need SoC
> specific compatibles and I suggest do not reference architecture, but
> name or the function of the processor, if there are such.

I have been arguing internally that I think when you look at driver itself it 
can be pretty much generic loader for any firmware and doesn't really matter if 
target subsystem is Microblaze/Risc-V/whatever based. And I was suggesting them 
to use more generic name.

Because at the end of day reg property is pointing to location where firmware 
should be loaded and gpio is a way how to start that subsystem and there is 
nothing Microblaze specific.

I can also imagine that the same driver could be extended with optional power 
domain, power regulator and clock properties if there is a need to drive them 
before subsystem gets out of reset.

Does it make sense?

Thanks,
Michal

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] dt-bindings: remoteproc: add AMD MicroBlaze binding
  2026-04-15  6:16     ` Michal Simek
@ 2026-04-15  6:50       ` Krzysztof Kozlowski
  2026-04-15  6:55         ` Michal Simek
  2026-04-15 12:19       ` Rob Herring
  1 sibling, 1 reply; 15+ messages in thread
From: Krzysztof Kozlowski @ 2026-04-15  6:50 UTC (permalink / raw)
  To: Michal Simek, Ben Levinsky, andersson, mathieu.poirier
  Cc: robh, krzk+dt, conor+dt, linux-remoteproc, devicetree,
	linux-kernel, tanmay.shah

On 15/04/2026 08:16, Michal Simek wrote:
> 
> 
> On 4/14/26 19:53, Krzysztof Kozlowski wrote:
>> On 14/04/2026 18:15, Ben Levinsky wrote:
>>
>> A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
>> prefix is already stating that these are bindings.
>> See also:
>> https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>>
>>> +---
>>> +$id: http://devicetree.org/schemas/remoteproc/amd,microblaze.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: AMD MicroBlaze remote processor
>>> +
>>> +maintainers:
>>> +  - Ben Levinsky <ben.levinsky@amd.com>
>>> +
>>> +description:
>>> +  MicroBlaze remote processor controlled by Linux through the remoteproc
>>> +  framework.
>>
>> Describe hardware, not Linux frameworks. IOW, Linux framework is here
>> irrelevant.
>>
>>> +
>>> +  The executable firmware memory window is described in the
>>> +  MicroBlaze-local address space by the node's reg property and translated
>>> +  to the system physical address space with standard devicetree address
>>> +  translation provided by the parent bus node's ranges property.
>>> +
>>> +properties:
>>> +  $nodename:
>>> +    pattern: "^remoteproc@[0-9a-f]+$"
>>> +
>>> +  compatible:
>>> +    const: amd,microblaze
>>
>> microblaze is architecture, so this feels way too generic. You need SoC
>> specific compatibles and I suggest do not reference architecture, but
>> name or the function of the processor, if there are such.
> 
> I have been arguing internally that I think when you look at driver itself it 
> can be pretty much generic loader for any firmware and doesn't really matter if 

Luckily I don't speak about driver :)

> target subsystem is Microblaze/Risc-V/whatever based. And I was suggesting them 
> to use more generic name.

So the binding is for drivers - generic loader? Then simply no. Not
suitable for DT.

> 
> Because at the end of day reg property is pointing to location where firmware 
> should be loaded and gpio is a way how to start that subsystem and there is 
> nothing Microblaze specific.
> 
> I can also imagine that the same driver could be extended with optional power 
> domain, power regulator and clock properties if there is a need to drive them 
> before subsystem gets out of reset.
> 
> Does it make sense?

Yes, drop from DT. No need for generic stuff. Or describe the hardware.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] dt-bindings: remoteproc: add AMD MicroBlaze binding
  2026-04-15  6:50       ` Krzysztof Kozlowski
@ 2026-04-15  6:55         ` Michal Simek
  2026-04-15  7:07           ` Krzysztof Kozlowski
  0 siblings, 1 reply; 15+ messages in thread
From: Michal Simek @ 2026-04-15  6:55 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Ben Levinsky, andersson, mathieu.poirier
  Cc: robh, krzk+dt, conor+dt, linux-remoteproc, devicetree,
	linux-kernel, tanmay.shah



On 4/15/26 08:50, Krzysztof Kozlowski wrote:
> On 15/04/2026 08:16, Michal Simek wrote:
>>
>>
>> On 4/14/26 19:53, Krzysztof Kozlowski wrote:
>>> On 14/04/2026 18:15, Ben Levinsky wrote:
>>>
>>> A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
>>> prefix is already stating that these are bindings.
>>> See also:
>>> https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>>>
>>>> +---
>>>> +$id: http://devicetree.org/schemas/remoteproc/amd,microblaze.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: AMD MicroBlaze remote processor
>>>> +
>>>> +maintainers:
>>>> +  - Ben Levinsky <ben.levinsky@amd.com>
>>>> +
>>>> +description:
>>>> +  MicroBlaze remote processor controlled by Linux through the remoteproc
>>>> +  framework.
>>>
>>> Describe hardware, not Linux frameworks. IOW, Linux framework is here
>>> irrelevant.
>>>
>>>> +
>>>> +  The executable firmware memory window is described in the
>>>> +  MicroBlaze-local address space by the node's reg property and translated
>>>> +  to the system physical address space with standard devicetree address
>>>> +  translation provided by the parent bus node's ranges property.
>>>> +
>>>> +properties:
>>>> +  $nodename:
>>>> +    pattern: "^remoteproc@[0-9a-f]+$"
>>>> +
>>>> +  compatible:
>>>> +    const: amd,microblaze
>>>
>>> microblaze is architecture, so this feels way too generic. You need SoC
>>> specific compatibles and I suggest do not reference architecture, but
>>> name or the function of the processor, if there are such.
>>
>> I have been arguing internally that I think when you look at driver itself it
>> can be pretty much generic loader for any firmware and doesn't really matter if
> 
> Luckily I don't speak about driver :)

:-)

> 
>> target subsystem is Microblaze/Risc-V/whatever based. And I was suggesting them
>> to use more generic name.
> 
> So the binding is for drivers - generic loader? Then simply no. Not
> suitable for DT.
> 
>>
>> Because at the end of day reg property is pointing to location where firmware
>> should be loaded and gpio is a way how to start that subsystem and there is
>> nothing Microblaze specific.
>>
>> I can also imagine that the same driver could be extended with optional power
>> domain, power regulator and clock properties if there is a need to drive them
>> before subsystem gets out of reset.
>>
>> Does it make sense?
> 
> Yes, drop from DT. No need for generic stuff. Or describe the hardware.

You need to describe that connection to HW. GPIOs, memory location, etc.
It means there must be any description.

Thanks,
Michal







^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] dt-bindings: remoteproc: add AMD MicroBlaze binding
  2026-04-15  6:55         ` Michal Simek
@ 2026-04-15  7:07           ` Krzysztof Kozlowski
  2026-04-15  8:06             ` Michal Simek
  0 siblings, 1 reply; 15+ messages in thread
From: Krzysztof Kozlowski @ 2026-04-15  7:07 UTC (permalink / raw)
  To: Michal Simek, Ben Levinsky, andersson, mathieu.poirier
  Cc: robh, krzk+dt, conor+dt, linux-remoteproc, devicetree,
	linux-kernel, tanmay.shah

On 15/04/2026 08:55, Michal Simek wrote:
>>>
>>> Does it make sense?
>>
>> Yes, drop from DT. No need for generic stuff. Or describe the hardware.
> 
> You need to describe that connection to HW. GPIOs, memory location, etc.
> It means there must be any description.

No, you can write user-space driver or pass everything through SW nodes.
No need for DT description.

But if you want a DT description, then it must be for the specific
hardware, since the hardware is not generic.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] dt-bindings: remoteproc: add AMD MicroBlaze binding
  2026-04-15  7:07           ` Krzysztof Kozlowski
@ 2026-04-15  8:06             ` Michal Simek
  2026-04-15  8:24               ` Krzysztof Kozlowski
  0 siblings, 1 reply; 15+ messages in thread
From: Michal Simek @ 2026-04-15  8:06 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Ben Levinsky, andersson, mathieu.poirier
  Cc: robh, krzk+dt, conor+dt, linux-remoteproc, devicetree,
	linux-kernel, tanmay.shah



On 4/15/26 09:07, Krzysztof Kozlowski wrote:
> On 15/04/2026 08:55, Michal Simek wrote:
>>>>
>>>> Does it make sense?
>>>
>>> Yes, drop from DT. No need for generic stuff. Or describe the hardware.
>>
>> You need to describe that connection to HW. GPIOs, memory location, etc.
>> It means there must be any description.
> 
> No, you can write user-space driver or pass everything through SW nodes.
> No need for DT description.

The firmware memory typically sits behind AXI-to-AXI bridges and 

interconnect switches. The bus topology varies between FPGA designs. 

DT ranges-based address translation is the standard way to describe 

this, and pushing it into userspace would just mean hardcoding what 

ranges already provides.

I don't think SW nodes should be used here.

> 
> But if you want a DT description, then it must be for the specific
> hardware, since the hardware is not generic.

But there is specific HW loaded. It is loaded at power up and don't change over 
life cycle. What I am just saying that access to this fixed HW (in fpga) is 
generic. At this stage memory and gpio only.

What I was trying to say is that the hardware topology (memory window + 

reset GPIO) is the same regardless of the soft-core cpu (MicroBlaze,
RISC-V, etc.)/fpga, so naming it after the ISA architecture felt wrong to me 

too.

When I look at other bindings. For example this one.
Documentation/devicetree/bindings/remoteproc/qcom,glink-rpm-edge.yaml

the compatible describes the communication mechanism (FIFO-based G-Link), not 
the specific processor behind it. 

  

Our case is similar the compatible describes the control mechanism firmware 
loaded through a memory window, processor started via GPIO reset. What sits 
behind that interface varies and is opaque to the binding.
  

Would something like "amd,mem-gpio-rproc" be acceptable, following the same 
pattern where the compatible identifies the interface mechanism?

Thanks,
Michal


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] dt-bindings: remoteproc: add AMD MicroBlaze binding
  2026-04-15  8:06             ` Michal Simek
@ 2026-04-15  8:24               ` Krzysztof Kozlowski
  2026-04-15  8:35                 ` Michal Simek
  0 siblings, 1 reply; 15+ messages in thread
From: Krzysztof Kozlowski @ 2026-04-15  8:24 UTC (permalink / raw)
  To: Michal Simek, Ben Levinsky, andersson, mathieu.poirier
  Cc: robh, krzk+dt, conor+dt, linux-remoteproc, devicetree,
	linux-kernel, tanmay.shah

On 15/04/2026 10:06, Michal Simek wrote:
> 
> 
> On 4/15/26 09:07, Krzysztof Kozlowski wrote:
>> On 15/04/2026 08:55, Michal Simek wrote:
>>>>>
>>>>> Does it make sense?
>>>>
>>>> Yes, drop from DT. No need for generic stuff. Or describe the hardware.
>>>
>>> You need to describe that connection to HW. GPIOs, memory location, etc.
>>> It means there must be any description.
>>
>> No, you can write user-space driver or pass everything through SW nodes.
>> No need for DT description.
> 
> The firmware memory typically sits behind AXI-to-AXI bridges and 
> 
> interconnect switches. The bus topology varies between FPGA designs. 
> 
> DT ranges-based address translation is the standard way to describe 
> 
> this, and pushing it into userspace would just mean hardcoding what 
> 
> ranges already provides.
> 
> I don't think SW nodes should be used here.
> 
>>
>> But if you want a DT description, then it must be for the specific
>> hardware, since the hardware is not generic.
> 
> But there is specific HW loaded. It is loaded at power up and don't change over 
> life cycle. What I am just saying that access to this fixed HW (in fpga) is 
> generic. At this stage memory and gpio only.
> 
> What I was trying to say is that the hardware topology (memory window + 
> 
> reset GPIO) is the same regardless of the soft-core cpu (MicroBlaze,
> RISC-V, etc.)/fpga, so naming it after the ISA architecture felt wrong to me 
> 
> too.
> 
> When I look at other bindings. For example this one.
> Documentation/devicetree/bindings/remoteproc/qcom,glink-rpm-edge.yaml

That's a subnode of other device. Not an independent device.

Plus I dislike most of Qualcomm remoteproc bindings and find them way to
downstreamish, written to match downstream approaches without respecting
DT rules.

> 
> the compatible describes the communication mechanism (FIFO-based G-Link), not 
> the specific processor behind it. 
> 
>   
> 
> Our case is similar the compatible describes the control mechanism firmware 
> loaded through a memory window, processor started via GPIO reset. What sits 
> behind that interface varies and is opaque to the binding.
>   
> 
> Would something like "amd,mem-gpio-rproc" be acceptable, following the same 
> pattern where the compatible identifies the interface mechanism?

Not for me. You have a very specific physical remote processor. That's
what you write bindings for.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] dt-bindings: remoteproc: add AMD MicroBlaze binding
  2026-04-15  8:24               ` Krzysztof Kozlowski
@ 2026-04-15  8:35                 ` Michal Simek
  0 siblings, 0 replies; 15+ messages in thread
From: Michal Simek @ 2026-04-15  8:35 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Ben Levinsky, andersson, mathieu.poirier
  Cc: robh, krzk+dt, conor+dt, linux-remoteproc, devicetree,
	linux-kernel, tanmay.shah



On 4/15/26 10:24, Krzysztof Kozlowski wrote:
> On 15/04/2026 10:06, Michal Simek wrote:
>>
>>
>> On 4/15/26 09:07, Krzysztof Kozlowski wrote:
>>> On 15/04/2026 08:55, Michal Simek wrote:
>>>>>>
>>>>>> Does it make sense?
>>>>>
>>>>> Yes, drop from DT. No need for generic stuff. Or describe the hardware.
>>>>
>>>> You need to describe that connection to HW. GPIOs, memory location, etc.
>>>> It means there must be any description.
>>>
>>> No, you can write user-space driver or pass everything through SW nodes.
>>> No need for DT description.
>>
>> The firmware memory typically sits behind AXI-to-AXI bridges and
>>
>> interconnect switches. The bus topology varies between FPGA designs.
>>
>> DT ranges-based address translation is the standard way to describe
>>
>> this, and pushing it into userspace would just mean hardcoding what
>>
>> ranges already provides.
>>
>> I don't think SW nodes should be used here.
>>
>>>
>>> But if you want a DT description, then it must be for the specific
>>> hardware, since the hardware is not generic.
>>
>> But there is specific HW loaded. It is loaded at power up and don't change over
>> life cycle. What I am just saying that access to this fixed HW (in fpga) is
>> generic. At this stage memory and gpio only.
>>
>> What I was trying to say is that the hardware topology (memory window +
>>
>> reset GPIO) is the same regardless of the soft-core cpu (MicroBlaze,
>> RISC-V, etc.)/fpga, so naming it after the ISA architecture felt wrong to me
>>
>> too.
>>
>> When I look at other bindings. For example this one.
>> Documentation/devicetree/bindings/remoteproc/qcom,glink-rpm-edge.yaml
> 
> That's a subnode of other device. Not an independent device.
> 
> Plus I dislike most of Qualcomm remoteproc bindings and find them way to
> downstreamish, written to match downstream approaches without respecting
> DT rules.
> 
>>
>> the compatible describes the communication mechanism (FIFO-based G-Link), not
>> the specific processor behind it.
>>
>>    
>>
>> Our case is similar the compatible describes the control mechanism firmware
>> loaded through a memory window, processor started via GPIO reset. What sits
>> behind that interface varies and is opaque to the binding.
>>    
>>
>> Would something like "amd,mem-gpio-rproc" be acceptable, following the same
>> pattern where the compatible identifies the interface mechanism?
> 
> Not for me. You have a very specific physical remote processor. That's
> what you write bindings for.

And we have it which is Microblaze with bram (block ram) which is mapped to host 
CPU memory range which is going to be loaded by firmware and start by toggling 
gpio.
I think what we only don't have is proper compatible string which will describe 
this HW. If you don't like generic name we can describe it specifically for this 
configuration.

amd,microblaze-mem-gpio-rproc for example without any generic fallback.

Or we could create a spec, design recommendation for this type of configuration 
if that helps.

What do you suggest?

Thanks,
Michal




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] dt-bindings: remoteproc: add AMD MicroBlaze binding
  2026-04-15  6:16     ` Michal Simek
  2026-04-15  6:50       ` Krzysztof Kozlowski
@ 2026-04-15 12:19       ` Rob Herring
  2026-04-15 12:41         ` Michal Simek
  1 sibling, 1 reply; 15+ messages in thread
From: Rob Herring @ 2026-04-15 12:19 UTC (permalink / raw)
  To: Michal Simek
  Cc: Krzysztof Kozlowski, Ben Levinsky, andersson, mathieu.poirier,
	krzk+dt, conor+dt, linux-remoteproc, devicetree, linux-kernel,
	tanmay.shah

On Wed, Apr 15, 2026 at 1:16 AM Michal Simek <michal.simek@amd.com> wrote:
>
>
>
> On 4/14/26 19:53, Krzysztof Kozlowski wrote:
> > On 14/04/2026 18:15, Ben Levinsky wrote:
> >
> > A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
> > prefix is already stating that these are bindings.
> > See also:
> > https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> >
> >> +---
> >> +$id: http://devicetree.org/schemas/remoteproc/amd,microblaze.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: AMD MicroBlaze remote processor
> >> +
> >> +maintainers:
> >> +  - Ben Levinsky <ben.levinsky@amd.com>
> >> +
> >> +description:
> >> +  MicroBlaze remote processor controlled by Linux through the remoteproc
> >> +  framework.
> >
> > Describe hardware, not Linux frameworks. IOW, Linux framework is here
> > irrelevant.
> >
> >> +
> >> +  The executable firmware memory window is described in the
> >> +  MicroBlaze-local address space by the node's reg property and translated
> >> +  to the system physical address space with standard devicetree address
> >> +  translation provided by the parent bus node's ranges property.
> >> +
> >> +properties:
> >> +  $nodename:
> >> +    pattern: "^remoteproc@[0-9a-f]+$"
> >> +
> >> +  compatible:
> >> +    const: amd,microblaze
> >
> > microblaze is architecture, so this feels way too generic. You need SoC
> > specific compatibles and I suggest do not reference architecture, but
> > name or the function of the processor, if there are such.
>
> I have been arguing internally that I think when you look at driver itself it
> can be pretty much generic loader for any firmware and doesn't really matter if
> target subsystem is Microblaze/Risc-V/whatever based. And I was suggesting them
> to use more generic name.

Generic to AMD though, not everyone, right?

I agree it probably doesn't matter what the processor arch is. The
compatible just needs to be specific enough when there's some
quirk/feature in the interface to the operating system, that we can
distinguish the specific implementation *without* a DT update.

> Because at the end of day reg property is pointing to location where firmware
> should be loaded and gpio is a way how to start that subsystem and there is
> nothing Microblaze specific.
>
> I can also imagine that the same driver could be extended with optional power
> domain, power regulator and clock properties if there is a need to drive them
> before subsystem gets out of reset.

That never works because then there's timing/ordering constraints for
enabling/disabling all those resources. Then we end up with a never
ending stream of properties added which results in a poorly designed
binding.

Rob

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] dt-bindings: remoteproc: add AMD MicroBlaze binding
  2026-04-15 12:19       ` Rob Herring
@ 2026-04-15 12:41         ` Michal Simek
  0 siblings, 0 replies; 15+ messages in thread
From: Michal Simek @ 2026-04-15 12:41 UTC (permalink / raw)
  To: Rob Herring
  Cc: Krzysztof Kozlowski, Ben Levinsky, andersson, mathieu.poirier,
	krzk+dt, conor+dt, linux-remoteproc, devicetree, linux-kernel,
	tanmay.shah



On 4/15/26 14:19, Rob Herring wrote:
> On Wed, Apr 15, 2026 at 1:16 AM Michal Simek <michal.simek@amd.com> wrote:
>>
>>
>>
>> On 4/14/26 19:53, Krzysztof Kozlowski wrote:
>>> On 14/04/2026 18:15, Ben Levinsky wrote:
>>>
>>> A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
>>> prefix is already stating that these are bindings.
>>> See also:
>>> https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>>>
>>>> +---
>>>> +$id: http://devicetree.org/schemas/remoteproc/amd,microblaze.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: AMD MicroBlaze remote processor
>>>> +
>>>> +maintainers:
>>>> +  - Ben Levinsky <ben.levinsky@amd.com>
>>>> +
>>>> +description:
>>>> +  MicroBlaze remote processor controlled by Linux through the remoteproc
>>>> +  framework.
>>>
>>> Describe hardware, not Linux frameworks. IOW, Linux framework is here
>>> irrelevant.
>>>
>>>> +
>>>> +  The executable firmware memory window is described in the
>>>> +  MicroBlaze-local address space by the node's reg property and translated
>>>> +  to the system physical address space with standard devicetree address
>>>> +  translation provided by the parent bus node's ranges property.
>>>> +
>>>> +properties:
>>>> +  $nodename:
>>>> +    pattern: "^remoteproc@[0-9a-f]+$"
>>>> +
>>>> +  compatible:
>>>> +    const: amd,microblaze
>>>
>>> microblaze is architecture, so this feels way too generic. You need SoC
>>> specific compatibles and I suggest do not reference architecture, but
>>> name or the function of the processor, if there are such.
>>
>> I have been arguing internally that I think when you look at driver itself it
>> can be pretty much generic loader for any firmware and doesn't really matter if
>> target subsystem is Microblaze/Risc-V/whatever based. And I was suggesting them
>> to use more generic name.
> 
> Generic to AMD though, not everyone, right?
> 
> I agree it probably doesn't matter what the processor arch is. The
> compatible just needs to be specific enough when there's some
> quirk/feature in the interface to the operating system, that we can
> distinguish the specific implementation *without* a DT update.


If any fpga vendor creates the same configuration description will be the same 
for them too. But not a problem with having it generic to AMD only.

I think the point is to come up with proper compatible string which you will 
agree on.


> 
>> Because at the end of day reg property is pointing to location where firmware
>> should be loaded and gpio is a way how to start that subsystem and there is
>> nothing Microblaze specific.
>>
>> I can also imagine that the same driver could be extended with optional power
>> domain, power regulator and clock properties if there is a need to drive them
>> before subsystem gets out of reset.
> 
> That never works because then there's timing/ordering constraints for
> enabling/disabling all those resources. Then we end up with a never
> ending stream of properties added which results in a poorly designed
> binding.

Actually even current binding should have clock described and handled because 
clock has to be enabled before releasing GPIO out of reset. If it is coming 
outside of chip it should still be modeled.

Anyway I don't think we have ever end in a never ending stream of properties for 
our IP and this is not going to be the case too.

Thanks,
Michal


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2026-04-15 12:41 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-14 16:15 [PATCH 0/2] Add a MicroBlaze remoteproc driver and binding Ben Levinsky
2026-04-14 16:15 ` [PATCH 1/2] dt-bindings: remoteproc: add AMD MicroBlaze binding Ben Levinsky
2026-04-14 17:29   ` Rob Herring (Arm)
2026-04-14 17:53   ` Krzysztof Kozlowski
2026-04-15  6:16     ` Michal Simek
2026-04-15  6:50       ` Krzysztof Kozlowski
2026-04-15  6:55         ` Michal Simek
2026-04-15  7:07           ` Krzysztof Kozlowski
2026-04-15  8:06             ` Michal Simek
2026-04-15  8:24               ` Krzysztof Kozlowski
2026-04-15  8:35                 ` Michal Simek
2026-04-15 12:19       ` Rob Herring
2026-04-15 12:41         ` Michal Simek
2026-04-14 16:15 ` [PATCH 2/2] remoteproc: add AMD MicroBlaze driver Ben Levinsky
2026-04-14 17:56   ` Krzysztof Kozlowski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox