[PATCH 0/2] Support Child MFD Cells for the i.MX6QDL Reset Controller

[PATCH 0/2] Support Child MFD Cells for the i.MX6QDL Reset Controller

[Bastian Krause]

Allow passing the reboot mode from the OS to the bootloader via the
syscon-reboot-mode binding. Add a "simple-mfd" to support probing such a
child node. The actual reboot mode node could then be defined in a board
device-tree or fixed up by the bootloader.

To: Philipp Zabel <p.zabel@pengutronix.de>
To: Rob Herring <robh+dt@kernel.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
To: Shawn Guo <shawnguo@kernel.org>
To: Sascha Hauer <s.hauer@pengutronix.de>
To: Pengutronix Kernel Team <kernel@pengutronix.de>
To: Fabio Estevam <festevam@gmail.com>
To: NXP Linux Team <linux-imx@nxp.com>
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: Ahmad Fatoum <a.fatoum@pengutronix.de>
Signed-off-by: Bastian Krause <bst@pengutronix.de>

---
Bastian Krause (2):
      dt-bindings: reset: imx-src: add syscon and simple-mfd compatibles
      ARM: dts: imx6qdl: support child mfd cells for the reset controller

 Documentation/devicetree/bindings/reset/fsl,imx-src.yaml | 2 ++
 arch/arm/boot/dts/imx6qdl.dtsi                           | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)
---
base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
change-id: 20230113-syscon-child-mfd-d95f324c8bbf

Best regards,
-- 
Bastian Krause <bst@pengutronix.de>

[PATCH 1/2] dt-bindings: reset: imx-src: add syscon and simple-mfd compatibles

[Bastian Krause]

This allows passing the reboot mode from the OS to the bootloader via the
syscon-reboot-mode binding. Add a "simple-mfd" to support probing such a
child node. The actual reboot mode node could then be defined in a
board device-tree or fixed up by the bootloader.

Suggested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Signed-off-by: Bastian Krause <bst@pengutronix.de>
---
 Documentation/devicetree/bindings/reset/fsl,imx-src.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml b/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
index b11ac533f914c..9ec734e14e9f5 100644
--- a/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
+++ b/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
@@ -38,6 +38,8 @@ properties:
       - items:
           - const: "fsl,imx6q-src"
           - const: "fsl,imx51-src"
+          - const: "syscon"
+          - const: "simple-mfd"
       - items:
           - const: "fsl,imx6sx-src"
           - const: "fsl,imx51-src"

-- 
2.30.2

Re: [PATCH 1/2] dt-bindings: reset: imx-src: add syscon and simple-mfd compatibles

[Krzysztof Kozlowski]

On 13/01/2023 18:32, Bastian Krause wrote:
> This allows passing the reboot mode from the OS to the bootloader via the
> syscon-reboot-mode binding. Add a "simple-mfd" to support probing such a
> child node. The actual reboot mode node could then be defined in a
> board device-tree or fixed up by the bootloader.
> 
> Suggested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Signed-off-by: Bastian Krause <bst@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/reset/fsl,imx-src.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml b/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
> index b11ac533f914c..9ec734e14e9f5 100644
> --- a/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
> +++ b/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
> @@ -38,6 +38,8 @@ properties:
>        - items:
>            - const: "fsl,imx6q-src"
>            - const: "fsl,imx51-src"
> +          - const: "syscon"
> +          - const: "simple-mfd"

1. Why only this variant can be used as suscon-reboot-mode and not others?

2. Drop quotes. In a follow up patch you can also remove quotes from
other compatibles.


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: reset: imx-src: add syscon and simple-mfd compatibles

[Krzysztof Kozlowski]

On 13/01/2023 18:32, Bastian Krause wrote:
> This allows passing the reboot mode from the OS to the bootloader via the
> syscon-reboot-mode binding. Add a "simple-mfd" to support probing such a
> child node. The actual reboot mode node could then be defined in a
> board device-tree or fixed up by the bootloader.
> 
> Suggested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Signed-off-by: Bastian Krause <bst@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/reset/fsl,imx-src.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml b/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
> index b11ac533f914c..9ec734e14e9f5 100644
> --- a/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
> +++ b/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
> @@ -38,6 +38,8 @@ properties:
>        - items:
>            - const: "fsl,imx6q-src"
>            - const: "fsl,imx51-src"
> +          - const: "syscon"
> +          - const: "simple-mfd"

This is not sufficient. You can easily check it by yourself - add
syscon-reboot-mode node and run dtbs_check.

Best regards,
Krzysztof

[PATCH 2/2] ARM: dts: imx6qdl: support child mfd cells for the reset controller

[Bastian Krause]

The actual syscon-reboot-mode child node can be added by a board
device-tree or fixed up by the bootloader. For the child node to be
probed, the compatible needs to include simple-mfd. The binding now
specifies this, so have the SoC dtsi adhere to it.

Suggested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Signed-off-by: Bastian Krause <bst@pengutronix.de>
---
 arch/arm/boot/dts/imx6qdl.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index ff1e0173b39be..b16be39458aa6 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -865,7 +865,8 @@ epit2: epit@20d4000 { /* EPIT2 */
 			};
 
 			src: reset-controller@20d8000 {
-				compatible = "fsl,imx6q-src", "fsl,imx51-src";
+				compatible = "fsl,imx6q-src", "fsl,imx51-src",
+					     "syscon", "simple-mfd";
 				reg = <0x020d8000 0x4000>;
 				interrupts = <0 91 IRQ_TYPE_LEVEL_HIGH>,
 					     <0 96 IRQ_TYPE_LEVEL_HIGH>;

-- 
2.30.2

Re: [PATCH 2/2] ARM: dts: imx6qdl: support child mfd cells for the reset controller

[Krzysztof Kozlowski]

On 13/01/2023 18:32, Bastian Krause wrote:
> The actual syscon-reboot-mode child node can be added by a board
> device-tree or fixed up by the bootloader. For the child node to be
> probed, the compatible needs to include simple-mfd. The binding now
> specifies this, so have the SoC dtsi adhere to it.
> 
> Suggested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Signed-off-by: Bastian Krause <bst@pengutronix.de>
> ---
>  arch/arm/boot/dts/imx6qdl.dtsi | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
> index ff1e0173b39be..b16be39458aa6 100644
> --- a/arch/arm/boot/dts/imx6qdl.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> @@ -865,7 +865,8 @@ epit2: epit@20d4000 { /* EPIT2 */
>  			};
>  
>  			src: reset-controller@20d8000 {
> -				compatible = "fsl,imx6q-src", "fsl,imx51-src";
> +				compatible = "fsl,imx6q-src", "fsl,imx51-src",
> +					     "syscon", "simple-mfd";

You need children here. Otherwise simple-mfd does not make sense. If you
expect something else to add children (I don't understand why, usually
reboot capability is fixed per SoC and only sometimes extended with some
other means), then this "else" will also change compatible.

Best regards,
Krzysztof

Re: [PATCH 2/2] ARM: dts: imx6qdl: support child mfd cells for the reset controller

[Krzysztof Kozlowski]

On 16/01/2023 09:20, Krzysztof Kozlowski wrote:
> On 13/01/2023 18:32, Bastian Krause wrote:
>> The actual syscon-reboot-mode child node can be added by a board
>> device-tree or fixed up by the bootloader. For the child node to be
>> probed, the compatible needs to include simple-mfd. The binding now
>> specifies this, so have the SoC dtsi adhere to it.
>>
>> Suggested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> Signed-off-by: Bastian Krause <bst@pengutronix.de>
>> ---
>>  arch/arm/boot/dts/imx6qdl.dtsi | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
>> index ff1e0173b39be..b16be39458aa6 100644
>> --- a/arch/arm/boot/dts/imx6qdl.dtsi
>> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
>> @@ -865,7 +865,8 @@ epit2: epit@20d4000 { /* EPIT2 */
>>  			};
>>  
>>  			src: reset-controller@20d8000 {
>> -				compatible = "fsl,imx6q-src", "fsl,imx51-src";
>> +				compatible = "fsl,imx6q-src", "fsl,imx51-src",
>> +					     "syscon", "simple-mfd";
> 
> You need children here. Otherwise simple-mfd does not make sense. If you
> expect something else to add children (I don't understand why, usually
> reboot capability is fixed per SoC and only sometimes extended with some
> other means), then this "else" will also change compatible.

Hm, although dtbs_check would complain... so let's clarify - why you are
not adding syscon-reboot now? It should be always here - either SoC
supports it or it does not. Board support is other thing, but this is
DTSI for SoC.

Best regards,
Krzysztof

Re: [PATCH 2/2] ARM: dts: imx6qdl: support child mfd cells for the reset controller

[Ahmad Fatoum]

Hello Krzysztof,

On 16.01.23 09:20, Krzysztof Kozlowski wrote:
> On 13/01/2023 18:32, Bastian Krause wrote:
>> The actual syscon-reboot-mode child node can be added by a board
>> device-tree or fixed up by the bootloader. For the child node to be
>> probed, the compatible needs to include simple-mfd. The binding now
>> specifies this, so have the SoC dtsi adhere to it.
>>
>> Suggested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> Signed-off-by: Bastian Krause <bst@pengutronix.de>
>> ---
>>  arch/arm/boot/dts/imx6qdl.dtsi | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
>> index ff1e0173b39be..b16be39458aa6 100644
>> --- a/arch/arm/boot/dts/imx6qdl.dtsi
>> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
>> @@ -865,7 +865,8 @@ epit2: epit@20d4000 { /* EPIT2 */
>>  			};
>>  
>>  			src: reset-controller@20d8000 {
>> -				compatible = "fsl,imx6q-src", "fsl,imx51-src";
>> +				compatible = "fsl,imx6q-src", "fsl,imx51-src",
>> +					     "syscon", "simple-mfd";
> 
> You need children here. Otherwise simple-mfd does not make sense. If you
> expect something else to add children (I don't understand why, usually
> reboot capability is fixed per SoC and only sometimes extended with some
> other means), then this "else" will also change compatible.

It's about syscon-reboot-mode, not syscon-reboot. Such modes are board-
not soc-specific. Yes, a board DTS can override a compatible, but this
is error prone as you can't just add a compatible, you need to hardcode
the other compatibles in the SoC dtsi, which may change for good reason
in future. barebox supports fixing up syscon-reboot-mode nodes defined
in its own device tree to the Linux DT. While in theory, it could check
if the parent node contains syscon/simple-mfd and fix it up if necessary,
it sounds to me like this should be rather part of the upstream DT.

Cheers,
Ahmad

> 
> Best regards,
> Krzysztof
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/2] ARM: dts: imx6qdl: support child mfd cells for the reset controller

[Krzysztof Kozlowski]

On 16/01/2023 09:27, Ahmad Fatoum wrote:
> Hello Krzysztof,
> 
> On 16.01.23 09:20, Krzysztof Kozlowski wrote:
>> On 13/01/2023 18:32, Bastian Krause wrote:
>>> The actual syscon-reboot-mode child node can be added by a board
>>> device-tree or fixed up by the bootloader. For the child node to be
>>> probed, the compatible needs to include simple-mfd. The binding now
>>> specifies this, so have the SoC dtsi adhere to it.
>>>
>>> Suggested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>> Signed-off-by: Bastian Krause <bst@pengutronix.de>
>>> ---
>>>  arch/arm/boot/dts/imx6qdl.dtsi | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
>>> index ff1e0173b39be..b16be39458aa6 100644
>>> --- a/arch/arm/boot/dts/imx6qdl.dtsi
>>> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
>>> @@ -865,7 +865,8 @@ epit2: epit@20d4000 { /* EPIT2 */
>>>  			};
>>>  
>>>  			src: reset-controller@20d8000 {
>>> -				compatible = "fsl,imx6q-src", "fsl,imx51-src";
>>> +				compatible = "fsl,imx6q-src", "fsl,imx51-src",
>>> +					     "syscon", "simple-mfd";
>>
>> You need children here. Otherwise simple-mfd does not make sense. If you
>> expect something else to add children (I don't understand why, usually
>> reboot capability is fixed per SoC and only sometimes extended with some
>> other means), then this "else" will also change compatible.
> 
> It's about syscon-reboot-mode, not syscon-reboot. Such modes are board-
> not soc-specific. 

syscon-reboot-mode is also mostly SoC specific. What exactly would
differ on different boards? Register offsets of SoC component? Register
values used by SoC power management unit?

Anyway, the binding is then not correct - it does not allow
syscon-reboot-mode child. I'll comment there.


Best regards,
Krzysztof

Re: [PATCH 2/2] ARM: dts: imx6qdl: support child mfd cells for the reset controller

[Ahmad Fatoum]

On 16.01.23 10:14, Krzysztof Kozlowski wrote:
> On 16/01/2023 09:27, Ahmad Fatoum wrote:
>> Hello Krzysztof,
>>
>> On 16.01.23 09:20, Krzysztof Kozlowski wrote:
>>> On 13/01/2023 18:32, Bastian Krause wrote:
>>>> The actual syscon-reboot-mode child node can be added by a board
>>>> device-tree or fixed up by the bootloader. For the child node to be
>>>> probed, the compatible needs to include simple-mfd. The binding now
>>>> specifies this, so have the SoC dtsi adhere to it.
>>>>
>>>> Suggested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>>> Signed-off-by: Bastian Krause <bst@pengutronix.de>
>>>> ---
>>>>  arch/arm/boot/dts/imx6qdl.dtsi | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
>>>> index ff1e0173b39be..b16be39458aa6 100644
>>>> --- a/arch/arm/boot/dts/imx6qdl.dtsi
>>>> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
>>>> @@ -865,7 +865,8 @@ epit2: epit@20d4000 { /* EPIT2 */
>>>>  			};
>>>>  
>>>>  			src: reset-controller@20d8000 {
>>>> -				compatible = "fsl,imx6q-src", "fsl,imx51-src";
>>>> +				compatible = "fsl,imx6q-src", "fsl,imx51-src",
>>>> +					     "syscon", "simple-mfd";
>>>
>>> You need children here. Otherwise simple-mfd does not make sense. If you
>>> expect something else to add children (I don't understand why, usually
>>> reboot capability is fixed per SoC and only sometimes extended with some
>>> other means), then this "else" will also change compatible.
>>
>> It's about syscon-reboot-mode, not syscon-reboot. Such modes are board-
>> not soc-specific. 
> 
> syscon-reboot-mode is also mostly SoC specific. What exactly would
> differ on different boards? Register offsets of SoC component? Register
> values used by SoC power management unit?

The modes supported. Let's say you want a bootloader mode that drops
the board's bootloader into a fastboot gadget mode. You'd add a
syscon-reboot-mode pointing at one of the non-volatile registers and
you would define a magic value to indicate fastboot, both in the
bootloader and Linux.

In theory, the reboot mode could also talk to the bootrom[1] to change
the bootsource. This is also not board-agnostic, because it may not
make sense to have a spinor reboot mode if your board doesn't have one.

We have this scheme for STM32MP1 already and that's why I suggested
Bastian to do it likewise for i.MX as he needs this functionality:
https://lore.kernel.org/all/20201021102855.18026-1-a.fatoum@pengutronix.de/
https://elixir.bootlin.com/barebox/latest/source/arch/arm/dts/stm32mp151.dtsi#L44

Here's the barebox documentation for reboot modes btw:
https://www.barebox.org/doc/latest/user/reboot-mode.html

[1]: The i.MX bootrom samples two registers to determine boot mode
     override. The upstream syscon-reboot-mode binding supports only
     one 32-bit value and thus can be used as-is only for communicating
     with the "user" bootloader.


> Anyway, the binding is then not correct - it does not allow
> syscon-reboot-mode child. I'll comment there.
> 
> 
> Best regards,
> Krzysztof
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/2] ARM: dts: imx6qdl: support child mfd cells for the reset controller

[Krzysztof Kozlowski]

On 16/01/2023 10:26, Ahmad Fatoum wrote:
> On 16.01.23 10:14, Krzysztof Kozlowski wrote:
>> On 16/01/2023 09:27, Ahmad Fatoum wrote:
>>> Hello Krzysztof,
>>>
>>> On 16.01.23 09:20, Krzysztof Kozlowski wrote:
>>>> On 13/01/2023 18:32, Bastian Krause wrote:
>>>>> The actual syscon-reboot-mode child node can be added by a board
>>>>> device-tree or fixed up by the bootloader. For the child node to be
>>>>> probed, the compatible needs to include simple-mfd. The binding now
>>>>> specifies this, so have the SoC dtsi adhere to it.
>>>>>
>>>>> Suggested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>>>> Signed-off-by: Bastian Krause <bst@pengutronix.de>
>>>>> ---
>>>>>  arch/arm/boot/dts/imx6qdl.dtsi | 3 ++-
>>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
>>>>> index ff1e0173b39be..b16be39458aa6 100644
>>>>> --- a/arch/arm/boot/dts/imx6qdl.dtsi
>>>>> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
>>>>> @@ -865,7 +865,8 @@ epit2: epit@20d4000 { /* EPIT2 */
>>>>>  			};
>>>>>  
>>>>>  			src: reset-controller@20d8000 {
>>>>> -				compatible = "fsl,imx6q-src", "fsl,imx51-src";
>>>>> +				compatible = "fsl,imx6q-src", "fsl,imx51-src",
>>>>> +					     "syscon", "simple-mfd";
>>>>
>>>> You need children here. Otherwise simple-mfd does not make sense. If you
>>>> expect something else to add children (I don't understand why, usually
>>>> reboot capability is fixed per SoC and only sometimes extended with some
>>>> other means), then this "else" will also change compatible.
>>>
>>> It's about syscon-reboot-mode, not syscon-reboot. Such modes are board-
>>> not soc-specific. 
>>
>> syscon-reboot-mode is also mostly SoC specific. What exactly would
>> differ on different boards? Register offsets of SoC component? Register
>> values used by SoC power management unit?
> 
> The modes supported. Let's say you want a bootloader mode that drops
> the board's bootloader into a fastboot gadget mode. You'd add a
> syscon-reboot-mode pointing at one of the non-volatile registers and
> you would define a magic value to indicate fastboot, both in the
> bootloader and Linux.

Bootloader and other firmware (e.g. ATF) is tightly tied to SoC, not to
board. There might be differences between firmware used and OS (e.g.
ChromeOS uses their own bootloader, different than Linux and Android on
the same SoC), but again this is not board specific.

> 
> In theory, the reboot mode could also talk to the bootrom[1] to change
> the bootsource. This is also not board-agnostic, because it may not
> make sense to have a spinor reboot mode if your board doesn't have one.
> 
> We have this scheme for STM32MP1 already and that's why I suggested
> Bastian to do it likewise for i.MX as he needs this functionality:
> https://lore.kernel.org/all/20201021102855.18026-1-a.fatoum@pengutronix.de/

I don't understand why you use clearly wrong patches as examples. Bad
patterns and bugs are not reason to use same approach.

The binding is wrong - you do not allow syscon-reboot-mode and if you
ever tested your patches, you would see the errors.

> https://elixir.bootlin.com/barebox/latest/source/arch/arm/dts/stm32mp151.dtsi#L44

Whether this part is correct, tricky to say. Why these offsets are not
valid for other board?


Best regards,
Krzysztof

Re: [PATCH 2/2] ARM: dts: imx6qdl: support child mfd cells for the reset controller

[Ahmad Fatoum]

Hello Krzysztof,

On 16.01.23 11:15, Krzysztof Kozlowski wrote:
>>>> It's about syscon-reboot-mode, not syscon-reboot. Such modes are board-
>>>> not soc-specific. 
>>>
>>> syscon-reboot-mode is also mostly SoC specific. What exactly would
>>> differ on different boards? Register offsets of SoC component? Register
>>> values used by SoC power management unit?
>>
>> The modes supported. Let's say you want a bootloader mode that drops
>> the board's bootloader into a fastboot gadget mode. You'd add a
>> syscon-reboot-mode pointing at one of the non-volatile registers and
>> you would define a magic value to indicate fastboot, both in the
>> bootloader and Linux.
> 
> Bootloader and other firmware (e.g. ATF) is tightly tied to SoC, not to
> board. There might be differences between firmware used and OS (e.g.
> ChromeOS uses their own bootloader, different than Linux and Android on
> the same SoC), but again this is not board specific.

The bootloader probes from a board device tree and it also implements
initialization, update, boot and fallback logic specific to the board and
part of that is what reboot modes are supported. E.g. ST had particular
reboot modes in mind (e.g. reboot into eMMC as usb mass storage gadget),
but that's just a convention they chose for the platform, not something
inherent to the SoC.

>> In theory, the reboot mode could also talk to the bootrom[1] to change
>> the bootsource. This is also not board-agnostic, because it may not
>> make sense to have a spinor reboot mode if your board doesn't have one.
>>
>> We have this scheme for STM32MP1 already and that's why I suggested
>> Bastian to do it likewise for i.MX as he needs this functionality:
>> https://lore.kernel.org/all/20201021102855.18026-1-a.fatoum@pengutronix.de/
> 
> I don't understand why you use clearly wrong patches as examples. Bad
> patterns and bugs are not reason to use same approach.

I am trying to give you some context. It may be evident to you what's
so clearly wrong about them, but for me it worked and I am trying to
understand where you see a problem.

> The binding is wrong - you do not allow syscon-reboot-mode and if you
> ever tested your patches, you would see the errors.

I did indeed not dump the device tree after the bootloader fixed it up and
run into through the DT bindings checker. 

>> https://elixir.bootlin.com/barebox/latest/source/arch/arm/dts/stm32mp151.dtsi#L44
> 
> Whether this part is correct, tricky to say. Why these offsets are not
> valid for other board?

The offsets are valid, it may just not work. Also the user may choose to place
the reboot mode somewhere else within the syscon if the register is unused
otherwise.

Still we could probably add a reboot-mode child node to the device tree
with no extra modes and leave it disabled. That way boards can fill in the
modes they support, enable it and use it. Does this work for you?

Thanks,
Ahmad


> 
> 
> Best regards,
> Krzysztof
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |