diff mbox series

[3/4] memory: brcmstb_dpfe: support DPFE API v4

Message ID 20231205184741.3092376-4-mmayer@broadcom.com
State New
Headers
Series memory: brcmstb_dpfe: support DPFE API v4 |

Commit Message

Markus Mayer Dec. 5, 2023, 6:47 p.m. UTC 
  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 <mmayer@broadcom.com>
---
 drivers/memory/brcmstb_dpfe.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

Comments

Florian Fainelli Dec. 5, 2023, 7:05 p.m. UTC | #1 
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 <mmayer@broadcom.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Krzysztof Kozlowski Dec. 6, 2023, 11:10 a.m. UTC | #2 
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 <mmayer@broadcom.com>
> ---

...

> +
>  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
Florian Fainelli Dec. 6, 2023, 4:18 p.m. UTC | #3 
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 <mmayer@broadcom.com>
>> ---
> 
> ...
> 
>> +
>>   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.
Krzysztof Kozlowski Dec. 6, 2023, 5:31 p.m. UTC | #4 
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 <mmayer@broadcom.com>
>>> ---
>>
>> ...
>>
>>> +
>>>   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
Markus Mayer Dec. 6, 2023, 6:48 p.m. UTC | #5 
On Wed, 6 Dec 2023 at 09:32, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> 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 <mmayer@broadcom.com>
> >>
> >> ...
> >>
> >>> +
> >>>   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
diff mbox series

Patch

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 },
 	{}
 };