It has become necessary to distinguish between the various DPFE API
versions by version number. Having just chip-specific compatible strings
and one generic version is no longer meeting our needs.
Also, a new DPFE API version, v4, needs to be supported by the driver.
As a result, an intermediate compatible string format is being
introduced: brcm,dpfe-cpu-v<N> where <N> represents the API version
number. This is more specific than the catch-all "brcm,dpfe-cpu" and
more generic than chip-specific compatible strings, such as
"brcm,bcm7271-dpfe-cpu".
The changes are split into several steps.
First, we update the binding and introduce the versioned compatible
strings.
Secondly, we add support for brcm,dpfe-cpu-v1 through brcm,dpfe-cpu-v3
in the driver to match existing API versions.
Thirdly, we introduce DPFE API v4.
Lastly, there is a change that isn't directly related to the
introduction of the new binding format or DPFE API v4. However, with the
increasing number of API versions, broadening compatibility can be
helpful. If registering the driver using the DT-provided compatible
string fails, the driver will try all DPFE APIs (except for v1) to see
if one might end up working. This can come in handy if the driver moves
on and learns about new API versions while Device Tree cannot be
updated.
Markus Mayer (4):
dt-bindings: memory: additional compatible strings for Broadcom DPFE
memory: brcmstb_dpfe: introduce version-specific compatible strings
memory: brcmstb_dpfe: support DPFE API v4
memory: brcmstb_dpfe: introduce best-effort API detection
.../memory-controllers/brcm,dpfe-cpu.yaml | 8 +-
drivers/memory/brcmstb_dpfe.c | 95 ++++++++++++++++++-
2 files changed, 100 insertions(+), 3 deletions(-)
--
2.43.0
Introduce compatible strings brcm,dpfe-cpu-v1 through brcm,dpfe-cpu-v3
to the Broadcom DPFE driver.
Signed-off-by: Markus Mayer <[email protected]>
---
drivers/memory/brcmstb_dpfe.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/memory/brcmstb_dpfe.c b/drivers/memory/brcmstb_dpfe.c
index a7ab3d377206..66876b409e59 100644
--- a/drivers/memory/brcmstb_dpfe.c
+++ b/drivers/memory/brcmstb_dpfe.c
@@ -924,6 +924,12 @@ static const struct of_device_id brcmstb_dpfe_of_match[] = {
{ .compatible = "brcm,bcm7271-dpfe-cpu", .data = &dpfe_api_old_v2 },
{ .compatible = "brcm,bcm7278-dpfe-cpu", .data = &dpfe_api_old_v2 },
{ .compatible = "brcm,bcm7211-dpfe-cpu", .data = &dpfe_api_new_v2 },
+
+ /* Match specific DCPU versions */
+ { .compatible = "brcm,dpfe-cpu-v1", .data = &dpfe_api_old_v2 },
+ { .compatible = "brcm,dpfe-cpu-v2", .data = &dpfe_api_new_v2 },
+ { .compatible = "brcm,dpfe-cpu-v3", .data = &dpfe_api_v3 },
+
/* API v3 is the default going forward */
{ .compatible = "brcm,dpfe-cpu", .data = &dpfe_api_v3 },
{}
--
2.43.0
Add versioned compatible strings for Broadcom DPFE. These take the form
brcm,dpfe-cpu-v<N> where <N> is a number from 1 to 4.
These API version related compatible strings are more specific than the
catch-all "brcm,dpfe-cpu" and more generic than chip-specific compatible
strings.
Signed-off-by: Markus Mayer <[email protected]>
---
.../bindings/memory-controllers/brcm,dpfe-cpu.yaml | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
index 08cbdcddfead..6dffa7b62baf 100644
--- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
@@ -16,6 +16,11 @@ properties:
- enum:
- brcm,bcm7271-dpfe-cpu
- brcm,bcm7268-dpfe-cpu
+ - enum:
+ - brcm,dpfe-cpu-v1
+ - brcm,dpfe-cpu-v2
+ - brcm,dpfe-cpu-v3
+ - brcm,dpfe-cpu-v4
- const: brcm,dpfe-cpu
reg:
@@ -40,7 +45,8 @@ additionalProperties: false
examples:
- |
dpfe-cpu@f1132000 {
- compatible = "brcm,bcm7271-dpfe-cpu", "brcm,dpfe-cpu";
+ compatible = "brcm,bcm7271-dpfe-cpu", "brcm,dpfe-cpu-v1",
+ "brcm,dpfe-cpu";
reg = <0xf1132000 0x180>,
<0xf1134000 0x1000>,
<0xf1138000 0x4000>;
--
2.43.0
Add support for version 4 of the DPFE API. This new version is largely
identical to version 3. The main difference is that all commands now
take the MHS version number as the first argument. Any other arguments
have been pushed down by one (i.e. what used to be arg0 in v3 is arg1 in
v4).
Signed-off-by: Markus Mayer <[email protected]>
---
drivers/memory/brcmstb_dpfe.c | 31 ++++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/drivers/memory/brcmstb_dpfe.c b/drivers/memory/brcmstb_dpfe.c
index 66876b409e59..0b0a9b85b605 100644
--- a/drivers/memory/brcmstb_dpfe.c
+++ b/drivers/memory/brcmstb_dpfe.c
@@ -37,6 +37,9 @@
#define DRVNAME "brcmstb-dpfe"
+/* Generic constants */
+#define MHS_VERSION 0x04000000
+
/* DCPU register offsets */
#define REG_DCPU_RESET 0x0
#define REG_TO_DCPU_MBOX 0x10
@@ -301,6 +304,28 @@ static const struct dpfe_api dpfe_api_v3 = {
},
};
+/* API v4 firmware commands */
+static struct dpfe_api dpfe_api_v4 = {
+ .version = 4,
+ .fw_name = NULL, /* We expect the firmware to have been downloaded! */
+ .sysfs_attrs = dpfe_v3_groups, /* Same as v3 */
+ .command = {
+ [DPFE_CMD_GET_INFO] = {
+ [MSG_HEADER] = DPFE_MSG_TYPE_COMMAND,
+ [MSG_COMMAND] = 0x0101,
+ [MSG_ARG_COUNT] = 2,
+ [MSG_ARG0] = MHS_VERSION,
+ [MSG_ARG0 + 1] = 1, /* Now the 2nd argument */
+ },
+ [DPFE_CMD_GET_REFRESH] = {
+ [MSG_HEADER] = DPFE_MSG_TYPE_COMMAND,
+ [MSG_COMMAND] = 0x0202,
+ [MSG_ARG_COUNT] = 1,
+ [MSG_ARG0] = MHS_VERSION,
+ },
+ },
+};
+
static const char *get_error_text(unsigned int i)
{
static const char * const error_text[] = {
@@ -929,8 +954,12 @@ static const struct of_device_id brcmstb_dpfe_of_match[] = {
{ .compatible = "brcm,dpfe-cpu-v1", .data = &dpfe_api_old_v2 },
{ .compatible = "brcm,dpfe-cpu-v2", .data = &dpfe_api_new_v2 },
{ .compatible = "brcm,dpfe-cpu-v3", .data = &dpfe_api_v3 },
+ { .compatible = "brcm,dpfe-cpu-v4", .data = &dpfe_api_v4 },
- /* API v3 is the default going forward */
+ /*
+ * For historical reasons, API v3 is the default if nothing else is
+ * specified.
+ */
{ .compatible = "brcm,dpfe-cpu", .data = &dpfe_api_v3 },
{}
};
--
2.43.0
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 <[email protected]>
---
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.
+ */
+ 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)
--
2.43.0
On 12/5/23 10:47, Markus Mayer wrote:
> Introduce compatible strings brcm,dpfe-cpu-v1 through brcm,dpfe-cpu-v3
> to the Broadcom DPFE driver.
>
> Signed-off-by: Markus Mayer <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 12/5/23 10:47, Markus Mayer wrote:
> Add support for version 4 of the DPFE API. This new version is largely
> identical to version 3. The main difference is that all commands now
> take the MHS version number as the first argument. Any other arguments
> have been pushed down by one (i.e. what used to be arg0 in v3 is arg1 in
> v4).
>
> Signed-off-by: Markus Mayer <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
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 <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 05/12/2023 19:47, Markus Mayer wrote:
> Introduce compatible strings brcm,dpfe-cpu-v1 through brcm,dpfe-cpu-v3
> to the Broadcom DPFE driver.
No, why?
>
> Signed-off-by: Markus Mayer <[email protected]>
> ---
> drivers/memory/brcmstb_dpfe.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/memory/brcmstb_dpfe.c b/drivers/memory/brcmstb_dpfe.c
> index a7ab3d377206..66876b409e59 100644
> --- a/drivers/memory/brcmstb_dpfe.c
> +++ b/drivers/memory/brcmstb_dpfe.c
> @@ -924,6 +924,12 @@ static const struct of_device_id brcmstb_dpfe_of_match[] = {
> { .compatible = "brcm,bcm7271-dpfe-cpu", .data = &dpfe_api_old_v2 },
> { .compatible = "brcm,bcm7278-dpfe-cpu", .data = &dpfe_api_old_v2 },
> { .compatible = "brcm,bcm7211-dpfe-cpu", .data = &dpfe_api_new_v2 },
> +
> + /* Match specific DCPU versions */
> + { .compatible = "brcm,dpfe-cpu-v1", .data = &dpfe_api_old_v2 },
> + { .compatible = "brcm,dpfe-cpu-v2", .data = &dpfe_api_new_v2 },
> + { .compatible = "brcm,dpfe-cpu-v3", .data = &dpfe_api_v3 },
Pointless change.
Best regards,
Krzysztof
On 05/12/2023 19:47, Markus Mayer wrote:
> Add support for version 4 of the DPFE API. This new version is largely
> identical to version 3. The main difference is that all commands now
> take the MHS version number as the first argument. Any other arguments
> have been pushed down by one (i.e. what used to be arg0 in v3 is arg1 in
> v4).
>
> Signed-off-by: Markus Mayer <[email protected]>
> ---
...
> +
> static const char *get_error_text(unsigned int i)
> {
> static const char * const error_text[] = {
> @@ -929,8 +954,12 @@ static const struct of_device_id brcmstb_dpfe_of_match[] = {
> { .compatible = "brcm,dpfe-cpu-v1", .data = &dpfe_api_old_v2 },
> { .compatible = "brcm,dpfe-cpu-v2", .data = &dpfe_api_new_v2 },
> { .compatible = "brcm,dpfe-cpu-v3", .data = &dpfe_api_v3 },
> + { .compatible = "brcm,dpfe-cpu-v4", .data = &dpfe_api_v4 },
>
No, use SoC specific compatible.
Best regards,
Krzysztof
On 05/12/2023 19:47, Markus Mayer wrote:
> Add versioned compatible strings for Broadcom DPFE. These take the form
> brcm,dpfe-cpu-v<N> where <N> is a number from 1 to 4.
>
> These API version related compatible strings are more specific than the
> catch-all "brcm,dpfe-cpu" and more generic than chip-specific compatible
> strings.
None of this explains: Why? I don't see any point in this and commit
does not explain.
>
> Signed-off-by: Markus Mayer <[email protected]>
> ---
> .../bindings/memory-controllers/brcm,dpfe-cpu.yaml | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> index 08cbdcddfead..6dffa7b62baf 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> @@ -16,6 +16,11 @@ properties:
> - enum:
> - brcm,bcm7271-dpfe-cpu
> - brcm,bcm7268-dpfe-cpu
> + - enum:
> + - brcm,dpfe-cpu-v1
> + - brcm,dpfe-cpu-v2
> + - brcm,dpfe-cpu-v3
> + - brcm,dpfe-cpu-v4
No, that's just wrong. So you want to say bcm7271 is brcm,dpfe-cpu-v4?
Best regards,
Krzysztof
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 <[email protected]>
> ---
> 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 05/12/2023 19:47, Markus Mayer wrote:
> It has become necessary to distinguish between the various DPFE API
> versions by version number. Having just chip-specific compatible strings
> and one generic version is no longer meeting our needs.
>
> Also, a new DPFE API version, v4, needs to be supported by the driver.
>
> As a result, an intermediate compatible string format is being
Introducing new SoC does not justify this. It's not correlated, not
related. Stop using some fake arguments to introduce something which we
do not want: versions.
> introduced: brcm,dpfe-cpu-v<N> where <N> represents the API version
> number. This is more specific than the catch-all "brcm,dpfe-cpu" and
> more generic than chip-specific compatible strings, such as
> "brcm,bcm7271-dpfe-cpu".
NAK
Best regards,
Krzysztof
On 12/6/2023 3:10 AM, Krzysztof Kozlowski wrote:
> On 05/12/2023 19:47, Markus Mayer wrote:
>> Add support for version 4 of the DPFE API. This new version is largely
>> identical to version 3. The main difference is that all commands now
>> take the MHS version number as the first argument. Any other arguments
>> have been pushed down by one (i.e. what used to be arg0 in v3 is arg1 in
>> v4).
>>
>> Signed-off-by: Markus Mayer <[email protected]>
>> ---
>
> ...
>
>> +
>> static const char *get_error_text(unsigned int i)
>> {
>> static const char * const error_text[] = {
>> @@ -929,8 +954,12 @@ static const struct of_device_id brcmstb_dpfe_of_match[] = {
>> { .compatible = "brcm,dpfe-cpu-v1", .data = &dpfe_api_old_v2 },
>> { .compatible = "brcm,dpfe-cpu-v2", .data = &dpfe_api_new_v2 },
>> { .compatible = "brcm,dpfe-cpu-v3", .data = &dpfe_api_v3 },
>> + { .compatible = "brcm,dpfe-cpu-v4", .data = &dpfe_api_v4 },
>>
>
> No, use SoC specific compatible.
This is not that simple because for a given SoC, the API implemented by
the firmware can change, in fact it has changed over the lifetime of a
given SoC as firmware updates get rolled out. Arguably the dialect
spoken by the firmware should not have changed and we told the firmware
team about that but it basically went nowhere and here we are.
The Device Tree gets populated by the boot loader which figures out
which API is spoken and places one of those compatible strings
accordingly for the kernel to avoid having to do any sort of run-time
detection which is slow and completely unnecessary when we can simply
tell it ahead of time what to use.
--
Florian
On 12/6/2023 3:09 AM, Krzysztof Kozlowski wrote:
> On 05/12/2023 19:47, Markus Mayer wrote:
>> Introduce compatible strings brcm,dpfe-cpu-v1 through brcm,dpfe-cpu-v3
>> to the Broadcom DPFE driver.
>
> No, why?
>
>>
>> Signed-off-by: Markus Mayer <[email protected]>
>> ---
>> drivers/memory/brcmstb_dpfe.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/memory/brcmstb_dpfe.c b/drivers/memory/brcmstb_dpfe.c
>> index a7ab3d377206..66876b409e59 100644
>> --- a/drivers/memory/brcmstb_dpfe.c
>> +++ b/drivers/memory/brcmstb_dpfe.c
>> @@ -924,6 +924,12 @@ static const struct of_device_id brcmstb_dpfe_of_match[] = {
>> { .compatible = "brcm,bcm7271-dpfe-cpu", .data = &dpfe_api_old_v2 },
>> { .compatible = "brcm,bcm7278-dpfe-cpu", .data = &dpfe_api_old_v2 },
>> { .compatible = "brcm,bcm7211-dpfe-cpu", .data = &dpfe_api_new_v2 },
>> +
>> + /* Match specific DCPU versions */
>> + { .compatible = "brcm,dpfe-cpu-v1", .data = &dpfe_api_old_v2 },
>> + { .compatible = "brcm,dpfe-cpu-v2", .data = &dpfe_api_new_v2 },
>> + { .compatible = "brcm,dpfe-cpu-v3", .data = &dpfe_api_v3 },
>
> Pointless change.
Is it possible to ask you as a maintainer to stop having those knee jerk
reactions and try to understand things a bit better, or simply request a
better explanation from the submitter?
--
Florian
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.
--
Florian
On 12/6/2023 3:09 AM, Krzysztof Kozlowski wrote:
> On 05/12/2023 19:47, Markus Mayer wrote:
>> Add versioned compatible strings for Broadcom DPFE. These take the form
>> brcm,dpfe-cpu-v<N> where <N> is a number from 1 to 4.
>>
>> These API version related compatible strings are more specific than the
>> catch-all "brcm,dpfe-cpu" and more generic than chip-specific compatible
>> strings.
>
> None of this explains: Why? I don't see any point in this and commit
> does not explain.
>
>>
>> Signed-off-by: Markus Mayer <[email protected]>
>> ---
>> .../bindings/memory-controllers/brcm,dpfe-cpu.yaml | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>> index 08cbdcddfead..6dffa7b62baf 100644
>> --- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>> +++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>> @@ -16,6 +16,11 @@ properties:
>> - enum:
>> - brcm,bcm7271-dpfe-cpu
>> - brcm,bcm7268-dpfe-cpu
>> + - enum:
>> + - brcm,dpfe-cpu-v1
>> + - brcm,dpfe-cpu-v2
>> + - brcm,dpfe-cpu-v3
>> + - brcm,dpfe-cpu-v4
>
> No, that's just wrong. So you want to say bcm7271 is brcm,dpfe-cpu-v4?
No as the example shows it "speaks" API v1.
I would be inclined to completely remove the chip specific compatible
strings from the binding because they are not sufficient or descriptive
enough to determine which API version is being spoken, since the
firmware is unfortunately allowed to change major APIs (and the
messaging format, because why not?) at a moments notice.
--
Florian
On 06/12/2023 17:18, Florian Fainelli wrote:
>
>
> On 12/6/2023 3:10 AM, Krzysztof Kozlowski wrote:
>> On 05/12/2023 19:47, Markus Mayer wrote:
>>> Add support for version 4 of the DPFE API. This new version is largely
>>> identical to version 3. The main difference is that all commands now
>>> take the MHS version number as the first argument. Any other arguments
>>> have been pushed down by one (i.e. what used to be arg0 in v3 is arg1 in
>>> v4).
>>>
>>> Signed-off-by: Markus Mayer <[email protected]>
>>> ---
>>
>> ...
>>
>>> +
>>> static const char *get_error_text(unsigned int i)
>>> {
>>> static const char * const error_text[] = {
>>> @@ -929,8 +954,12 @@ static const struct of_device_id brcmstb_dpfe_of_match[] = {
>>> { .compatible = "brcm,dpfe-cpu-v1", .data = &dpfe_api_old_v2 },
>>> { .compatible = "brcm,dpfe-cpu-v2", .data = &dpfe_api_new_v2 },
>>> { .compatible = "brcm,dpfe-cpu-v3", .data = &dpfe_api_v3 },
>>> + { .compatible = "brcm,dpfe-cpu-v4", .data = &dpfe_api_v4 },
>>>
>>
>> No, use SoC specific compatible.
>
> This is not that simple because for a given SoC, the API implemented by
> the firmware can change, in fact it has changed over the lifetime of a
> given SoC as firmware updates get rolled out. Arguably the dialect
> spoken by the firmware should not have changed and we told the firmware
> team about that but it basically went nowhere and here we are.
>
> The Device Tree gets populated by the boot loader which figures out
> which API is spoken and places one of those compatible strings
> accordingly for the kernel to avoid having to do any sort of run-time
> detection which is slow and completely unnecessary when we can simply
> tell it ahead of time what to use.
Thanks for providing justification, quite reasonable. A pity that none
of the commit msgs answered this way.
Best regards,
Krzysztof
On 06/12/2023 17:19, Florian Fainelli wrote:
>
>
> On 12/6/2023 3:09 AM, Krzysztof Kozlowski wrote:
>> On 05/12/2023 19:47, Markus Mayer wrote:
>>> Introduce compatible strings brcm,dpfe-cpu-v1 through brcm,dpfe-cpu-v3
>>> to the Broadcom DPFE driver.
>>
>> No, why?
>>
>>>
>>> Signed-off-by: Markus Mayer <[email protected]>
>>> ---
>>> drivers/memory/brcmstb_dpfe.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/memory/brcmstb_dpfe.c b/drivers/memory/brcmstb_dpfe.c
>>> index a7ab3d377206..66876b409e59 100644
>>> --- a/drivers/memory/brcmstb_dpfe.c
>>> +++ b/drivers/memory/brcmstb_dpfe.c
>>> @@ -924,6 +924,12 @@ static const struct of_device_id brcmstb_dpfe_of_match[] = {
>>> { .compatible = "brcm,bcm7271-dpfe-cpu", .data = &dpfe_api_old_v2 },
>>> { .compatible = "brcm,bcm7278-dpfe-cpu", .data = &dpfe_api_old_v2 },
>>> { .compatible = "brcm,bcm7211-dpfe-cpu", .data = &dpfe_api_new_v2 },
>>> +
>>> + /* Match specific DCPU versions */
>>> + { .compatible = "brcm,dpfe-cpu-v1", .data = &dpfe_api_old_v2 },
>>> + { .compatible = "brcm,dpfe-cpu-v2", .data = &dpfe_api_new_v2 },
>>> + { .compatible = "brcm,dpfe-cpu-v3", .data = &dpfe_api_v3 },
>>
>> Pointless change.
>
> Is it possible to ask you as a maintainer to stop having those knee jerk
> reactions and try to understand things a bit better, or simply request a
> better explanation from the submitter?
I asked: "Why?". None of the commits explain the rationale behind the
change. None of them say why such change is needed. They all repeat what
the patch is doing, which is pretty easy to see from the diff. The
commit must answer the trickiest question: why are we doing this?
Best regards,
Krzysztof
On 12/6/23 09:29, Krzysztof Kozlowski wrote:
> On 06/12/2023 17:32, Florian Fainelli wrote:
>>
>>
>> On 12/6/2023 3:09 AM, Krzysztof Kozlowski wrote:
>>> On 05/12/2023 19:47, Markus Mayer wrote:
>>>> Add versioned compatible strings for Broadcom DPFE. These take the form
>>>> brcm,dpfe-cpu-v<N> where <N> is a number from 1 to 4.
>>>>
>>>> These API version related compatible strings are more specific than the
>>>> catch-all "brcm,dpfe-cpu" and more generic than chip-specific compatible
>>>> strings.
>>>
>>> None of this explains: Why? I don't see any point in this and commit
>>> does not explain.
>>>
>>>>
>>>> Signed-off-by: Markus Mayer <[email protected]>
>>>> ---
>>>> .../bindings/memory-controllers/brcm,dpfe-cpu.yaml | 8 +++++++-
>>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>>> index 08cbdcddfead..6dffa7b62baf 100644
>>>> --- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>>> @@ -16,6 +16,11 @@ properties:
>>>> - enum:
>>>> - brcm,bcm7271-dpfe-cpu
>>>> - brcm,bcm7268-dpfe-cpu
>>>> + - enum:
>>>> + - brcm,dpfe-cpu-v1
>>>> + - brcm,dpfe-cpu-v2
>>>> + - brcm,dpfe-cpu-v3
>>>> + - brcm,dpfe-cpu-v4
>>>
>>> No, that's just wrong. So you want to say bcm7271 is brcm,dpfe-cpu-v4?
>>
>> No as the example shows it "speaks" API v1.
>
> Example is not a binding. It does not matter except of validating the
> binding. This is just incorrect.
>
>>
>> I would be inclined to completely remove the chip specific compatible
>> strings from the binding because they are not sufficient or descriptive
>> enough to determine which API version is being spoken, since the
>> firmware is unfortunately allowed to change major APIs (and the
>> messaging format, because why not?) at a moments notice.
>
> Then versions do not give you anything more.
The versions indicate exactly which API to be spoken to with the
firmware. The firmware API was not properly designed, it should have had
a way to indicate which API it has, regardless of the messaging format
it implements, but for reasons unknown that is not how it was implemented.
Essentially we need to know right away and ahead of time which API to be
used, otherwise that means doing runtime detection like what patch 4
does which you do not want to see.
--
Florian
On 06/12/2023 18:36, Florian Fainelli wrote:
> On 12/6/23 09:29, Krzysztof Kozlowski wrote:
>> On 06/12/2023 17:32, Florian Fainelli wrote:
>>>
>>>
>>> On 12/6/2023 3:09 AM, Krzysztof Kozlowski wrote:
>>>> On 05/12/2023 19:47, Markus Mayer wrote:
>>>>> Add versioned compatible strings for Broadcom DPFE. These take the form
>>>>> brcm,dpfe-cpu-v<N> where <N> is a number from 1 to 4.
>>>>>
>>>>> These API version related compatible strings are more specific than the
>>>>> catch-all "brcm,dpfe-cpu" and more generic than chip-specific compatible
>>>>> strings.
>>>>
>>>> None of this explains: Why? I don't see any point in this and commit
>>>> does not explain.
>>>>
>>>>>
>>>>> Signed-off-by: Markus Mayer <[email protected]>
>>>>> ---
>>>>> .../bindings/memory-controllers/brcm,dpfe-cpu.yaml | 8 +++++++-
>>>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>>>> index 08cbdcddfead..6dffa7b62baf 100644
>>>>> --- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>>>> @@ -16,6 +16,11 @@ properties:
>>>>> - enum:
>>>>> - brcm,bcm7271-dpfe-cpu
>>>>> - brcm,bcm7268-dpfe-cpu
>>>>> + - enum:
>>>>> + - brcm,dpfe-cpu-v1
>>>>> + - brcm,dpfe-cpu-v2
>>>>> + - brcm,dpfe-cpu-v3
>>>>> + - brcm,dpfe-cpu-v4
>>>>
>>>> No, that's just wrong. So you want to say bcm7271 is brcm,dpfe-cpu-v4?
>>>
>>> No as the example shows it "speaks" API v1.
>>
>> Example is not a binding. It does not matter except of validating the
>> binding. This is just incorrect.
>>
>>>
>>> I would be inclined to completely remove the chip specific compatible
>>> strings from the binding because they are not sufficient or descriptive
>>> enough to determine which API version is being spoken, since the
>>> firmware is unfortunately allowed to change major APIs (and the
>>> messaging format, because why not?) at a moments notice.
>>
>> Then versions do not give you anything more.
>
> The versions indicate exactly which API to be spoken to with the
> firmware. The firmware API was not properly designed, it should have had
> a way to indicate which API it has, regardless of the messaging format
> it implements, but for reasons unknown that is not how it was implemented.
>
> Essentially we need to know right away and ahead of time which API to be
> used, otherwise that means doing runtime detection like what patch 4
> does which you do not want to see.
Yeah, I see, you explained this deeper in response to 3/4, which I read
after this one.
Deprecating specific compatibles makes sense. If you have subset of FW
per given SoC, you could keep the specific compatible followed by subset
of version-compatibles (e.g. bcm7271 + v1 + generic fallback). However
then generic fallback is useless and you should actually drop it. The
only, *ONLY* point of generic fallback is to be used by OS alone. In
that case it cannot be used alone, so it is useless.
We do not use generic compatibles in a way of "I want to call all of
these devices a DPFE" or "I want to call it a default".
Now, if you do not have subset of FW per given SoC, so anything can
match with anything, then in one commit:
1. Deprecate specific compatible followed by useless generic fallback
2. Add versioned-compatibles alone, since generic fallback gives nothing.
Best regards,
Krzysztof
On 06/12/2023 17:32, Florian Fainelli wrote:
>
>
> On 12/6/2023 3:09 AM, Krzysztof Kozlowski wrote:
>> On 05/12/2023 19:47, Markus Mayer wrote:
>>> Add versioned compatible strings for Broadcom DPFE. These take the form
>>> brcm,dpfe-cpu-v<N> where <N> is a number from 1 to 4.
>>>
>>> These API version related compatible strings are more specific than the
>>> catch-all "brcm,dpfe-cpu" and more generic than chip-specific compatible
>>> strings.
>>
>> None of this explains: Why? I don't see any point in this and commit
>> does not explain.
>>
>>>
>>> Signed-off-by: Markus Mayer <[email protected]>
>>> ---
>>> .../bindings/memory-controllers/brcm,dpfe-cpu.yaml | 8 +++++++-
>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>> index 08cbdcddfead..6dffa7b62baf 100644
>>> --- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>> +++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>> @@ -16,6 +16,11 @@ properties:
>>> - enum:
>>> - brcm,bcm7271-dpfe-cpu
>>> - brcm,bcm7268-dpfe-cpu
>>> + - enum:
>>> + - brcm,dpfe-cpu-v1
>>> + - brcm,dpfe-cpu-v2
>>> + - brcm,dpfe-cpu-v3
>>> + - brcm,dpfe-cpu-v4
>>
>> No, that's just wrong. So you want to say bcm7271 is brcm,dpfe-cpu-v4?
>
> No as the example shows it "speaks" API v1.
Example is not a binding. It does not matter except of validating the
binding. This is just incorrect.
>
> I would be inclined to completely remove the chip specific compatible
> strings from the binding because they are not sufficient or descriptive
> enough to determine which API version is being spoken, since the
> firmware is unfortunately allowed to change major APIs (and the
> messaging format, because why not?) at a moments notice.
Then versions do not give you anything more.
Best regards,
Krzysztof
On Wed, 6 Dec 2023 at 09:32, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 06/12/2023 17:18, Florian Fainelli wrote:
> >
> >
> > On 12/6/2023 3:10 AM, Krzysztof Kozlowski wrote:
> >> On 05/12/2023 19:47, Markus Mayer wrote:
> >>> Add support for version 4 of the DPFE API. This new version is largely
> >>> identical to version 3. The main difference is that all commands now
> >>> take the MHS version number as the first argument. Any other arguments
> >>> have been pushed down by one (i.e. what used to be arg0 in v3 is arg1 in
> >>> v4).
> >>>
> >>> Signed-off-by: Markus Mayer <[email protected]>
> >>
> >> ...
> >>
> >>> +
> >>> static const char *get_error_text(unsigned int i)
> >>> {
> >>> static const char * const error_text[] = {
> >>> @@ -929,8 +954,12 @@ static const struct of_device_id brcmstb_dpfe_of_match[] = {
> >>> { .compatible = "brcm,dpfe-cpu-v1", .data = &dpfe_api_old_v2 },
> >>> { .compatible = "brcm,dpfe-cpu-v2", .data = &dpfe_api_new_v2 },
> >>> { .compatible = "brcm,dpfe-cpu-v3", .data = &dpfe_api_v3 },
> >>> + { .compatible = "brcm,dpfe-cpu-v4", .data = &dpfe_api_v4 },
> >>>
> >>
> >> No, use SoC specific compatible.
> >
> > This is not that simple because for a given SoC, the API implemented by
> > the firmware can change, in fact it has changed over the lifetime of a
> > given SoC as firmware updates get rolled out. Arguably the dialect
> > spoken by the firmware should not have changed and we told the firmware
> > team about that but it basically went nowhere and here we are.
> >
> > The Device Tree gets populated by the boot loader which figures out
> > which API is spoken and places one of those compatible strings
> > accordingly for the kernel to avoid having to do any sort of run-time
> > detection which is slow and completely unnecessary when we can simply
> > tell it ahead of time what to use.
>
> Thanks for providing justification, quite reasonable. A pity that none
> of the commit msgs answered this way.
The real pity is how this API was designed, making all of this
necessary in the first place.
We can definitely spell out more clearly in the commit messages what
is going on and why all of this is needed. I'll pull all the pieces
together from the various responses. As long as there's a way we can
reasonably implement what we need, we'll be happy.
> Best regards,
> Krzysztof