Re: [PATCH 4/4] mmc: sdhci-brcmstb: rpmb sharing by claiming host for TZOS

[Krzysztof Kozlowski <krzk@kernel.org>]

On 11/07/2025 17:42, Kamal Dasu wrote:
> +
> +static int sdhci_brcmstb_sdio_share_init(struct platform_device *pdev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(&pdev->dev);
> +	struct device_node *np = pdev->dev.of_node;
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_brcmstb_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	struct brcmstb_sdio_share_info *si;
> +	void __iomem *sdio_sh_regs;
> +	int ret;
> +
> +	/* sdio_share block */
> +	sdio_sh_regs = devm_platform_ioremap_resource_byname(pdev, "share");
> +	if (IS_ERR(sdio_sh_regs))
> +		return 0;
> +
> +	si = devm_kcalloc(&pdev->dev, 1, sizeof(struct brcmstb_sdio_share_info),

sizeof(*)

> +			  GFP_KERNEL);
> +	if (!si)
> +		return -ENOMEM;
> +
> +	si->share_reg = sdio_sh_regs;
> +	ret = of_hwspin_lock_get_id(np, 0);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to get hwspinlock id %d\n", ret);

Uh? So you changed the ABI in the driver but not in the bindings? No,
this must be always synced. Look what your binding is saying. Is it
optional? Yes.

And why would you print errors anyway on deferred probe? Twice! One here
and warning in your probe.

> +		return ret;
> +	}
> +
> +	si->hwlock = devm_hwspin_lock_request_specific(&pdev->dev, ret);
> +	if (!si->hwlock) {
> +		dev_err(&pdev->dev, "failed to request hwspinlock\n");

Syntax is: return dev_err_probe

> +		return -ENXIO;
> +	}
> +
> +	si->irq_recv = platform_get_irq_byname_optional(pdev, "recv_ipi0");
> +	if (si->irq_recv < 0) {
> +		ret = si->irq_recv;
> +		dev_err(&pdev->dev, "recv_ipi0 IRQ not found\n");

Syntax is: return dev_err_probe

> +		return ret;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, si->irq_recv,
> +			       sdhci_brcmstb_recv_ipi0_irq,
> +			       0, "mmc_recv_ipi0", host);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "mmc_recv_ipi0 IRQ request_irq failed\n");
> +		return ret;

return dev_err_probe

> +	}
> +
> +	si->ipis0_reg = devm_platform_ioremap_resource_byname(pdev, "flshr_ipis0");
> +	if (IS_ERR(si->ipis0_reg))
> +		return -ENXIO;
> +
> +	priv->si = si;
> +	si->host = host;
> +	init_waitqueue_head(&si->wq);
> +	/* acquire hwsem */
> +	sdhci_brcmstb_aquire_hwsem(si);
> +	si->claim_thread =
> +		kthread_run(sdhci_brcmstb_host_claim_thread, si,
> +			    "ksdshrthread/%s", mmc_hostname(host->mmc));
> +	if (IS_ERR(si->claim_thread)) {
> +		ret = PTR_ERR(si->claim_thread);
> +		dev_err(&pdev->dev, "failed to run claim thread\n");
> +		return -ENOEXEC;
> +	}
> +
> +	return 0;
> +}
> +
>  static u32 sdhci_brcmstb_cqhci_irq(struct sdhci_host *host, u32 intmask)
>  {
>  	int cmd_error = 0;
> @@ -482,8 +720,11 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>  		goto err;
>  
>  	pltfm_host->clk = clk;
> -	return res;
> +	res  = sdhci_brcmstb_sdio_share_init(pdev);

Only single space before '='

> +	if (res)
> +		dev_warn(&pdev->dev, "sdio share unavailable\n");

Why do you warn on completely optional hwlock? Drop, this should be
silent. You already print errors earlier and there is no point to print
error twice.

>  
> +	return 0;
>  err:
>  	sdhci_pltfm_free(pdev);
>  	clk_disable_unprepare(base_clk);


Best regards,
Krzysztof