On Thu, Sep 10, 2026 at 08:59:16AM +0000, Aniruddha Rao wrote:
[...]
> diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
> @@ -783,23 +894,32 @@ static int tegra_bpmp_probe(struct platform_device *pdev)
[...]
>  	if (!bpmp)
>  		return -ENOMEM;
>  
> -	bpmp->soc = of_device_get_match_data(&pdev->dev);
> -	bpmp->dev = &pdev->dev;
> +	bpmp->soc = device_get_match_data(&pdev->dev);
> +	if (!bpmp->soc)
> +		return -EINVAL;

Under what circumstances is this going to happen? We never checked this
on purpose because of_device_get_match_data() returns values straight
from the matching table, the same matching table that is used to bind
the driver to this device. So there can never be a case where you bind
to the device but don't get SoC data. Except maybe if you leave out the
SoC data from the table, in which case you deserve the crash that
follows.

>  
> -	err = tegra_bpmp_init_channels(bpmp);
> -	if (err < 0)
> -		return err;
> +	bpmp->dev = &pdev->dev;
>  
>  	platform_set_drvdata(pdev, bpmp);
>  
> -	err = bpmp->soc->ops->init(bpmp);
> -	if (err < 0)
> -		return err;
> +	if (!ACPI_HANDLE(bpmp->dev)) {
> +		err = tegra_bpmp_init_channels(bpmp);
> +		if (err < 0)
> +			return err;
> +	}

This looks a bit weird now, tying it to !ACPI here. I wonder if we
should just push this down into bpmp->soc->ops->init() instead. Having
this here was supposed to setup common infrastructure before going into
the detailed setup in tegra186_bpmp_setup_channels() and such, but since
this isn't common for ACPI I think we should move this into the SoC-
specific code, perhaps with a common helper between Tegra210 and
Tegra186 variants.

>  
> -	err = tegra_bpmp_request_mrq(bpmp, MRQ_PING,
> -				     tegra_bpmp_mrq_handle_ping, bpmp);
> -	if (err < 0)
> -		goto deinit;
> +	if (bpmp->soc->ops && bpmp->soc->ops->init) {
> +		err = bpmp->soc->ops->init(bpmp);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	if (!ACPI_HANDLE(bpmp->dev)) {
> +		err = tegra_bpmp_request_mrq(bpmp, MRQ_PING,
> +					     tegra_bpmp_mrq_handle_ping, bpmp);
> +		if (err < 0)
> +			goto deinit;
> +	}

How does the ping work on ACPI systems if we don't have a callback for
handling the MRQ?

>  
>  	err = tegra_bpmp_ping(bpmp);
>  	if (err < 0) {
> @@ -815,26 +935,30 @@ static int tegra_bpmp_probe(struct platform_device *pdev)
>  
>  	dev_info(&pdev->dev, "firmware: %.*s\n", (int)sizeof(tag), tag);
>  
> -	err = of_platform_default_populate(pdev->dev.of_node, NULL, &pdev->dev);
> -	if (err < 0)
> -		goto free_mrq;
> -
> -	if (of_property_present(pdev->dev.of_node, "#clock-cells")) {
> -		err = tegra_bpmp_init_clocks(bpmp);
> +	if (pdev->dev.of_node) {
> +		err = of_platform_default_populate(pdev->dev.of_node, NULL,
> +						   &pdev->dev);
>  		if (err < 0)
>  			goto free_mrq;
> -	}
>  
> -	if (of_property_present(pdev->dev.of_node, "#reset-cells")) {
> -		err = tegra_bpmp_init_resets(bpmp);
> -		if (err < 0)
> -			goto free_mrq;
> -	}
> +		if (of_property_present(pdev->dev.of_node, "#clock-cells")) {
> +			err = tegra_bpmp_init_clocks(bpmp);
> +			if (err < 0)
> +				goto free_mrq;
> +		}
>  
> -	if (of_property_present(pdev->dev.of_node, "#power-domain-cells")) {
> -		err = tegra_bpmp_init_powergates(bpmp);
> -		if (err < 0)
> -			goto free_mrq;
> +		if (of_property_present(pdev->dev.of_node, "#reset-cells")) {
> +			err = tegra_bpmp_init_resets(bpmp);
> +			if (err < 0)
> +				goto free_mrq;
> +		}
> +
> +		if (of_property_present(pdev->dev.of_node,
> +					"#power-domain-cells")) {
> +			err = tegra_bpmp_init_powergates(bpmp);
> +			if (err < 0)
> +				goto free_mrq;
> +		}
>  	}

All of the above could probably also be pushed down into the DT-specific
implementations, or just into the soc->init() from earlier.

Maybe even the ping in the middle could be pushed down since we also
need to special-case it.

>  
>  	err = tegra_bpmp_init_debugfs(bpmp);
> @@ -844,9 +968,10 @@ static int tegra_bpmp_probe(struct platform_device *pdev)
>  	return 0;
>  
>  free_mrq:
> -	tegra_bpmp_free_mrq(bpmp, MRQ_PING, bpmp);
> +	if (!ACPI_HANDLE(bpmp->dev))
> +		tegra_bpmp_free_mrq(bpmp, MRQ_PING, bpmp);

I think if we push freeing the MRQ into deinit, this could also be
cleaned up.

Thierry