Re: [PATCH 2/2] remoteproc: add AMD MicroBlaze driver

[Krzysztof Kozlowski]

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