[4/4] memory: brcmstb_dpfe: introduce best-effort API detection
Commit Message
Add a best-effort probe function that tries all known DPFE versions to
see if one might actually work. This helps in cases where device tree
doesn't provide the proper version information for whatever reason. In
that case, the driver may still be able to register if one of the known
API versions ends up working.
Caveat: we have to skip "v1" during our best effort attempts. This is
due to the fact that attempting a firmware download as required by v1
will result in a memory access violation on anything but v1 hardware.
This would crash the kernel. Since we don't know the HW version, we need
to play it safe and skip v1.
Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
drivers/memory/brcmstb_dpfe.c | 58 ++++++++++++++++++++++++++++++++++-
1 file changed, 57 insertions(+), 1 deletion(-)
Comments
On 12/5/23 10:47, Markus Mayer wrote:
> Add a best-effort probe function that tries all known DPFE versions to
> see if one might actually work. This helps in cases where device tree
> doesn't provide the proper version information for whatever reason. In
> that case, the driver may still be able to register if one of the known
> API versions ends up working.
>
> Caveat: we have to skip "v1" during our best effort attempts. This is
> due to the fact that attempting a firmware download as required by v1
> will result in a memory access violation on anything but v1 hardware.
> This would crash the kernel. Since we don't know the HW version, we need
> to play it safe and skip v1.
>
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
On 05/12/2023 19:47, Markus Mayer wrote:
> Add a best-effort probe function that tries all known DPFE versions to
> see if one might actually work. This helps in cases where device tree
> doesn't provide the proper version information for whatever reason. In
So for incomplete DTS you now add elaborate, own, custom matching
function. That's not how the code should work.
> that case, the driver may still be able to register if one of the known
> API versions ends up working.
>
> Caveat: we have to skip "v1" during our best effort attempts. This is
> due to the fact that attempting a firmware download as required by v1
> will result in a memory access violation on anything but v1 hardware.
> This would crash the kernel. Since we don't know the HW version, we need
> to play it safe and skip v1.
None of this commit explains what is real problem being solved.
>
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
> drivers/memory/brcmstb_dpfe.c | 58 ++++++++++++++++++++++++++++++++++-
> 1 file changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/memory/brcmstb_dpfe.c b/drivers/memory/brcmstb_dpfe.c
> index 0b0a9b85b605..15f4ee3b8535 100644
> --- a/drivers/memory/brcmstb_dpfe.c
> +++ b/drivers/memory/brcmstb_dpfe.c
> @@ -879,6 +879,50 @@ static int brcmstb_dpfe_resume(struct platform_device *pdev)
> return brcmstb_dpfe_download_firmware(priv);
> }
>
> +static int brcmstb_dpfe_probe_best_effort(struct platform_device *pdev)
> +{
> + const char versioned_compat[] = "brcm,dpfe-cpu-v";
> + const char v1_str[] = "-v1";
> + const struct of_device_id *matches;
> + const struct dpfe_api *orig_dpfe_api;
> + struct device *dev = &pdev->dev;
> + struct brcmstb_dpfe_priv *priv;
> + int ret = -ENODEV;
> +
> + priv = dev_get_drvdata(dev);
> + orig_dpfe_api = priv->dpfe_api;
> + matches = dev->driver->of_match_table;
> +
> + /* Loop over all compatible strings */
> + for (; matches->compatible[0]; matches++) {
> + const char *compat = matches->compatible;
> + /* Find the ones that start with "brcm,dpfe-cpu-v" */
> + if (strstr(compat, versioned_compat) == compat) {
> + char *v1_ptr = strstr(compat, v1_str);
> + /*
> + * We must skip v1, since we don't know the hardware
> + * version and attempting a firmware download on v2 and
> + * newer would crash the kernel due to a memory access
> + * violation.
> + * We make sure to match "-v1" at the end of the string
> + * only.
> + */
> + if (v1_ptr && v1_ptr[sizeof(v1_str)] == '\0')
> + continue;
> + priv->dpfe_api = matches->data;
> + /* Fingers crossed... */
> + ret = brcmstb_dpfe_download_firmware(priv);
> + if (!ret)
> + return 0;
> + }
> + }
> +
> + /* It didn't work, so let's clean up. */
> + priv->dpfe_api = orig_dpfe_api;
> +
> + return ret;
> +}
> +
> static int brcmstb_dpfe_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -923,8 +967,20 @@ static int brcmstb_dpfe_probe(struct platform_device *pdev)
> }
>
> ret = brcmstb_dpfe_download_firmware(priv);
> + if (ret && ret != -EPROBE_DEFER) {
> + /*
> + * If the information provided by Device Tree didn't work, let's
> + * try all known version. Maybe one will work.
I don't understand how this comment is related to downloading firmware.
> + */
> + dev_warn(dev,
> + "DPFE v%d didn't work, reverting to best-effort\n",
> + priv->dpfe_api->version);
> + dev_warn(dev,
> + "Device Tree and / or the driver should be updated\n");
You are now introducing new warnings?
NAK
Best regards,
Krzysztof
On 12/6/2023 3:13 AM, Krzysztof Kozlowski wrote:
> On 05/12/2023 19:47, Markus Mayer wrote:
>> Add a best-effort probe function that tries all known DPFE versions to
>> see if one might actually work. This helps in cases where device tree
>> doesn't provide the proper version information for whatever reason. In
>
> So for incomplete DTS you now add elaborate, own, custom matching
> function. That's not how the code should work.
I suppose that we could drop that change and simply rely upon the Device
Tree containing an adequate compatible string. This change was added as
a transitional step for ourselves mostly. I agree this may not entirely
belong upstream.
@@ -879,6 +879,50 @@ static int brcmstb_dpfe_resume(struct platform_device *pdev)
return brcmstb_dpfe_download_firmware(priv);
}
+static int brcmstb_dpfe_probe_best_effort(struct platform_device *pdev)
+{
+ const char versioned_compat[] = "brcm,dpfe-cpu-v";
+ const char v1_str[] = "-v1";
+ const struct of_device_id *matches;
+ const struct dpfe_api *orig_dpfe_api;
+ struct device *dev = &pdev->dev;
+ struct brcmstb_dpfe_priv *priv;
+ int ret = -ENODEV;
+
+ priv = dev_get_drvdata(dev);
+ orig_dpfe_api = priv->dpfe_api;
+ matches = dev->driver->of_match_table;
+
+ /* Loop over all compatible strings */
+ for (; matches->compatible[0]; matches++) {
+ const char *compat = matches->compatible;
+ /* Find the ones that start with "brcm,dpfe-cpu-v" */
+ if (strstr(compat, versioned_compat) == compat) {
+ char *v1_ptr = strstr(compat, v1_str);
+ /*
+ * We must skip v1, since we don't know the hardware
+ * version and attempting a firmware download on v2 and
+ * newer would crash the kernel due to a memory access
+ * violation.
+ * We make sure to match "-v1" at the end of the string
+ * only.
+ */
+ if (v1_ptr && v1_ptr[sizeof(v1_str)] == '\0')
+ continue;
+ priv->dpfe_api = matches->data;
+ /* Fingers crossed... */
+ ret = brcmstb_dpfe_download_firmware(priv);
+ if (!ret)
+ return 0;
+ }
+ }
+
+ /* It didn't work, so let's clean up. */
+ priv->dpfe_api = orig_dpfe_api;
+
+ return ret;
+}
+
static int brcmstb_dpfe_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -923,8 +967,20 @@ static int brcmstb_dpfe_probe(struct platform_device *pdev)
}
ret = brcmstb_dpfe_download_firmware(priv);
+ if (ret && ret != -EPROBE_DEFER) {
+ /*
+ * If the information provided by Device Tree didn't work, let's
+ * try all known version. Maybe one will work.
+ */
+ dev_warn(dev,
+ "DPFE v%d didn't work, reverting to best-effort\n",
+ priv->dpfe_api->version);
+ dev_warn(dev,
+ "Device Tree and / or the driver should be updated\n");
+ ret = brcmstb_dpfe_probe_best_effort(pdev);
+ }
if (ret)
- return dev_err_probe(dev, ret, "Couldn't download firmware\n");
+ return dev_err_probe(dev, ret, "Unable to talk to DCPU\n");
ret = sysfs_create_groups(&pdev->dev.kobj, priv->dpfe_api->sysfs_attrs);
if (!ret)