DTB backward/forward compatibility with "pinctrl: bcm2835: Change init order for gpio hogs"

DTB backward/forward compatibility with "pinctrl: bcm2835: Change init order for gpio hogs"

[Florian Fainelli]

Hi,

I am a bit frustrated by this commit, we picked it up via the stable 
5.10 and 5.15 trees into our downstream tree, and in the absence of a 
suitable 'gpio-ranges' property for the GPIO controller, the SPI 
controller keeps getting -EPROBE_DEFER for its chip select. If the 
property is present, then all is well.

Now the problem in my case is that the boot loader is responsible for 
providing the DTB to the kernel, and until recently, we did not update 
it to contain a suitable 'gpio-ranges' property. Now that it has been 
updated however, older kernels which *do not* have said change in the 
subject are also getting -EPROBE_DEFER for the SPI chip select.

So this is just breaking backward/forward compatibility with the DTB 
unless both are updated in lock steps which is *extremely* inconvenient.

This is death by a thousand cuts.

So how do we remedy this?
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: DTB backward/forward compatibility with "pinctrl: bcm2835: Change init order for gpio hogs"

[Phil Elwell]

Florian,

On 25/01/2022 19:39, Florian Fainelli wrote:
> Hi,
> 
> I am a bit frustrated by this commit, we picked it up via the stable 5.10 and 
> 5.15 trees into our downstream tree, and in the absence of a suitable 
> 'gpio-ranges' property for the GPIO controller, the SPI controller keeps getting 
> -EPROBE_DEFER for its chip select. If the property is present, then all is well.
> 
> Now the problem in my case is that the boot loader is responsible for providing 
> the DTB to the kernel, and until recently, we did not update it to contain a 
> suitable 'gpio-ranges' property. Now that it has been updated however, older 
> kernels which *do not* have said change in the subject are also getting 
> -EPROBE_DEFER for the SPI chip select.
> 
> So this is just breaking backward/forward compatibility with the DTB unless both 
> are updated in lock steps which is *extremely* inconvenient.
> 
> This is death by a thousand cuts.
> 
> So how do we remedy this?

It's unfortunate that both the DTS and driver were incorrect, and that the fixes 
were inter-dependent. At Raspberry Pi we make a point of treating the DTBs as 
part of the kernel, updating them at the same time. Is that not possible in your 
situation? You don't specify which bootloader you are using (or even which 
platform), but U-boot can be configured to use the DTB loaded and adjusted by 
the firmware.

Phil

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: DTB backward/forward compatibility with "pinctrl: bcm2835: Change init order for gpio hogs"

[Florian Fainelli]

On 1/25/2022 11:48 AM, Phil Elwell wrote:
> Florian,
> 
> On 25/01/2022 19:39, Florian Fainelli wrote:
>> Hi,
>>
>> I am a bit frustrated by this commit, we picked it up via the stable 
>> 5.10 and 5.15 trees into our downstream tree, and in the absence of a 
>> suitable 'gpio-ranges' property for the GPIO controller, the SPI 
>> controller keeps getting -EPROBE_DEFER for its chip select. If the 
>> property is present, then all is well.
>>
>> Now the problem in my case is that the boot loader is responsible for 
>> providing the DTB to the kernel, and until recently, we did not update 
>> it to contain a suitable 'gpio-ranges' property. Now that it has been 
>> updated however, older kernels which *do not* have said change in the 
>> subject are also getting -EPROBE_DEFER for the SPI chip select.
>>
>> So this is just breaking backward/forward compatibility with the DTB 
>> unless both are updated in lock steps which is *extremely* inconvenient.
>>
>> This is death by a thousand cuts.
>>
>> So how do we remedy this?
> 
> It's unfortunate that both the DTS and driver were incorrect, and that 
> the fixes were inter-dependent. At Raspberry Pi we make a point of 
> treating the DTBs as part of the kernel, updating them at the same time. 
> Is that not possible in your situation? You don't specify which 
> bootloader you are using (or even which platform), but U-boot can be 
> configured to use the DTB loaded and adjusted by the firmware.

We use a boot loader called BOLT which has an offline DTS generation 
part an a runtime patching part. At no point in time has it been 
necessary, desirable or even reliably possible to look at the kernel 
version and mangle the Device Tree such that "things" work. I do not 
even want to fathom what the code doing that would look like other than 
a big pile of mess. This is utterly error prone and completely breaks 
the boot loader/kernel abstraction.

We strive to be able to update the boot loader and the kernel seemingly 
independently from one another even though obviously there are times 
where locksteps are necessary. What I like to do is:

- put the changes in the DTB that are necessary for a *future* kernel 
well in advance such that by the time that newer kernel gets used these 
properties are already there

- adding new properties does not break older kernels, they simply ignore 
them

And this allows us to define a combination that always works while 
having a sliding window of backward/forward compatibility if we have 
locksteps in between.

I can make changes in the downstream kernel such that we prune DT 
properties, but once we open that door, the list goes on and on and it 
is just a damage control strategy anyway.

Why cannot the pinctrl framework infer that we have a 1:1 mapping in the 
absence of a 'gpio-ranges' property, but instead does the following:

# cat /sys/kernel/debug/pinctrl/47e200000.gpio-pinctrl-bcm2711/gpio-ranges
GPIO ranges handled:
0: pinctrl-bcm2711 GPIOS [4294967295 - 56] PINS [0 - 57]
#

clearly it determined the end GPIO but it is not able to determine the 
start GPIO?

And now, on a 5.10 kernel which does contain both the 'gpio-ranges' 
property and the said change to pinctrl-bcm2835.c I have this:

# cat /sys/kernel/debug/pinctrl/47e200000.gpio-pinctrl-bcm2711/gpio-ranges
GPIO ranges handled:
0: pinctrl-bcm2711 GPIOS [4294967295 - 56] PINS [0 - 57]
0: pinctrl-bcm2711 GPIOS [454 - 511] PINS [0 - 57]
#

What the heck?
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: DTB backward/forward compatibility with "pinctrl: bcm2835: Change init order for gpio hogs"

[Florian Fainelli]

On 1/25/2022 11:58 AM, Florian Fainelli wrote:
> 
> 
> On 1/25/2022 11:48 AM, Phil Elwell wrote:
>> Florian,
>>
>> On 25/01/2022 19:39, Florian Fainelli wrote:
>>> Hi,
>>>
>>> I am a bit frustrated by this commit, we picked it up via the stable 
>>> 5.10 and 5.15 trees into our downstream tree, and in the absence of a 
>>> suitable 'gpio-ranges' property for the GPIO controller, the SPI 
>>> controller keeps getting -EPROBE_DEFER for its chip select. If the 
>>> property is present, then all is well.
>>>
>>> Now the problem in my case is that the boot loader is responsible for 
>>> providing the DTB to the kernel, and until recently, we did not 
>>> update it to contain a suitable 'gpio-ranges' property. Now that it 
>>> has been updated however, older kernels which *do not* have said 
>>> change in the subject are also getting -EPROBE_DEFER for the SPI chip 
>>> select.
>>>
>>> So this is just breaking backward/forward compatibility with the DTB 
>>> unless both are updated in lock steps which is *extremely* inconvenient.
>>>
>>> This is death by a thousand cuts.
>>>
>>> So how do we remedy this?
>>
>> It's unfortunate that both the DTS and driver were incorrect, and that 
>> the fixes were inter-dependent. At Raspberry Pi we make a point of 
>> treating the DTBs as part of the kernel, updating them at the same 
>> time. Is that not possible in your situation? You don't specify which 
>> bootloader you are using (or even which platform), but U-boot can be 
>> configured to use the DTB loaded and adjusted by the firmware.
> 
> We use a boot loader called BOLT which has an offline DTS generation 
> part an a runtime patching part. At no point in time has it been 
> necessary, desirable or even reliably possible to look at the kernel 
> version and mangle the Device Tree such that "things" work. I do not 
> even want to fathom what the code doing that would look like other than 
> a big pile of mess. This is utterly error prone and completely breaks 
> the boot loader/kernel abstraction.
> 
> We strive to be able to update the boot loader and the kernel seemingly 
> independently from one another even though obviously there are times 
> where locksteps are necessary. What I like to do is:
> 
> - put the changes in the DTB that are necessary for a *future* kernel 
> well in advance such that by the time that newer kernel gets used these 
> properties are already there
> 
> - adding new properties does not break older kernels, they simply ignore 
> them
> 
> And this allows us to define a combination that always works while 
> having a sliding window of backward/forward compatibility if we have 
> locksteps in between.
> 
> I can make changes in the downstream kernel such that we prune DT 
> properties, but once we open that door, the list goes on and on and it 
> is just a damage control strategy anyway.
> 
> Why cannot the pinctrl framework infer that we have a 1:1 mapping in the 
> absence of a 'gpio-ranges' property, but instead does the following:
> 
> # cat /sys/kernel/debug/pinctrl/47e200000.gpio-pinctrl-bcm2711/gpio-ranges
> GPIO ranges handled:
> 0: pinctrl-bcm2711 GPIOS [4294967295 - 56] PINS [0 - 57]
> #
> 
> clearly it determined the end GPIO but it is not able to determine the 
> start GPIO?
> 
> And now, on a 5.10 kernel which does contain both the 'gpio-ranges' 
> property and the said change to pinctrl-bcm2835.c I have this:
> 
> # cat /sys/kernel/debug/pinctrl/47e200000.gpio-pinctrl-bcm2711/gpio-ranges
> GPIO ranges handled:
> 0: pinctrl-bcm2711 GPIOS [4294967295 - 56] PINS [0 - 57]
> 0: pinctrl-bcm2711 GPIOS [454 - 511] PINS [0 - 57]
> #
> 
> What the heck?

And just to be clear, at this point I understand that the old kernel, 
new DTB/DTS is not something we can obviously fix, however new kernel 
old DTB/DTS *without* 'gpio-ranges' is something that should still work 
to ease the pain.

I will dig more into why we have this duplicated debugfs output (most 
likely we are not unwinding an error path when we get -EPROBE_DEFER) and 
also why pinctrl is not able to figure out the gpio start properly 
without 'gpio-ranges'.
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: DTB backward/forward compatibility with "pinctrl: bcm2835: Change init order for gpio hogs"

[Stefan Wahren]

Hi,

Am 26.01.22 um 02:33 schrieb Florian Fainelli:
>
>
> On 1/25/2022 11:58 AM, Florian Fainelli wrote:
>>
>>
>> On 1/25/2022 11:48 AM, Phil Elwell wrote:
>>> Florian,
>>>
>>> On 25/01/2022 19:39, Florian Fainelli wrote:
>>>> Hi,
>>>>
>>>> I am a bit frustrated by this commit, we picked it up via the
>>>> stable 5.10 and 5.15 trees into our downstream tree, and in the
>>>> absence of a suitable 'gpio-ranges' property for the GPIO
>>>> controller, the SPI controller keeps getting -EPROBE_DEFER for its
>>>> chip select. If the property is present, then all is well.
>>>>
>>>> Now the problem in my case is that the boot loader is responsible
>>>> for providing the DTB to the kernel, and until recently, we did not
>>>> update it to contain a suitable 'gpio-ranges' property. Now that it
>>>> has been updated however, older kernels which *do not* have said
>>>> change in the subject are also getting -EPROBE_DEFER for the SPI
>>>> chip select.
>>>>
>>>> So this is just breaking backward/forward compatibility with the
>>>> DTB unless both are updated in lock steps which is *extremely*
>>>> inconvenient.
>>>>
>>>> This is death by a thousand cuts.
>>>>
>>>> So how do we remedy this?
>>>
>>> It's unfortunate that both the DTS and driver were incorrect, and
>>> that the fixes were inter-dependent. At Raspberry Pi we make a point
>>> of treating the DTBs as part of the kernel, updating them at the
>>> same time. Is that not possible in your situation? You don't specify
>>> which bootloader you are using (or even which platform), but U-boot
>>> can be configured to use the DTB loaded and adjusted by the firmware.
>>
>> We use a boot loader called BOLT which has an offline DTS generation
>> part an a runtime patching part. At no point in time has it been
>> necessary, desirable or even reliably possible to look at the kernel
>> version and mangle the Device Tree such that "things" work. I do not
>> even want to fathom what the code doing that would look like other
>> than a big pile of mess. This is utterly error prone and completely
>> breaks the boot loader/kernel abstraction.
>>
>> We strive to be able to update the boot loader and the kernel
>> seemingly independently from one another even though obviously there
>> are times where locksteps are necessary. What I like to do is:
>>
>> - put the changes in the DTB that are necessary for a *future* kernel
>> well in advance such that by the time that newer kernel gets used
>> these properties are already there
>>
>> - adding new properties does not break older kernels, they simply
>> ignore them
>>
>> And this allows us to define a combination that always works while
>> having a sliding window of backward/forward compatibility if we have
>> locksteps in between.
>>
>> I can make changes in the downstream kernel such that we prune DT
>> properties, but once we open that door, the list goes on and on and
>> it is just a damage control strategy anyway.
>>
>> Why cannot the pinctrl framework infer that we have a 1:1 mapping in
>> the absence of a 'gpio-ranges' property, but instead does the following:
>>
>> # cat
>> /sys/kernel/debug/pinctrl/47e200000.gpio-pinctrl-bcm2711/gpio-ranges
>> GPIO ranges handled:
>> 0: pinctrl-bcm2711 GPIOS [4294967295 - 56] PINS [0 - 57]
the driver init the gpio base with -1 which seems to lead to this huge
unsigned integer. Very confusing from my PoV.
>> #
>>
>> clearly it determined the end GPIO but it is not able to determine
>> the start GPIO?
>>
>> And now, on a 5.10 kernel which does contain both the 'gpio-ranges'
>> property and the said change to pinctrl-bcm2835.c I have this:
>>
>> # cat
>> /sys/kernel/debug/pinctrl/47e200000.gpio-pinctrl-bcm2711/gpio-ranges
>> GPIO ranges handled:
>> 0: pinctrl-bcm2711 GPIOS [4294967295 - 56] PINS [0 - 57]
>> 0: pinctrl-bcm2711 GPIOS [454 - 511] PINS [0 - 57]
>> #
>>
>> What the heck?
>
> And just to be clear, at this point I understand that the old kernel,
> new DTB/DTS is not something we can obviously fix, however new kernel
> old DTB/DTS *without* 'gpio-ranges' is something that should still
> work to ease the pain.

The whole problem started with make an optional DT property a required
one afterwards.

The only workaround i can think of is to check for the absence of
'gpio-ranges' in the pinctrl driver and mitigate. Not nice :(

>
> I will dig more into why we have this duplicated debugfs output (most
> likely we are not unwinding an error path when we get -EPROBE_DEFER)
> and also why pinctrl is not able to figure out the gpio start properly
> without 'gpio-ranges'.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: DTB backward/forward compatibility with "pinctrl: bcm2835: Change init order for gpio hogs"

[Stefan Wahren]

Hi Florian,

reanimate this old thread

Am 27.01.22 um 17:31 schrieb Stefan Wahren:
> Hi,
>
> Am 26.01.22 um 02:33 schrieb Florian Fainelli:
>>
>> On 1/25/2022 11:58 AM, Florian Fainelli wrote:
>>>
>>> On 1/25/2022 11:48 AM, Phil Elwell wrote:
>>>> Florian,
>>>>
>>>> On 25/01/2022 19:39, Florian Fainelli wrote:
>>>>> Hi,
>>>>>
>>>>> I am a bit frustrated by this commit, we picked it up via the
>>>>> stable 5.10 and 5.15 trees into our downstream tree, and in the
>>>>> absence of a suitable 'gpio-ranges' property for the GPIO
>>>>> controller, the SPI controller keeps getting -EPROBE_DEFER for its
>>>>> chip select. If the property is present, then all is well.
>>>>>
>>>>> Now the problem in my case is that the boot loader is responsible
>>>>> for providing the DTB to the kernel, and until recently, we did not
>>>>> update it to contain a suitable 'gpio-ranges' property. Now that it
>>>>> has been updated however, older kernels which *do not* have said
>>>>> change in the subject are also getting -EPROBE_DEFER for the SPI
>>>>> chip select.
>>>>>
>>>>> So this is just breaking backward/forward compatibility with the
>>>>> DTB unless both are updated in lock steps which is *extremely*
>>>>> inconvenient.
>>>>>
>>>>> This is death by a thousand cuts.
>>>>>
>>>>> So how do we remedy this?
>>>> It's unfortunate that both the DTS and driver were incorrect, and
>>>> that the fixes were inter-dependent. At Raspberry Pi we make a point
>>>> of treating the DTBs as part of the kernel, updating them at the
>>>> same time. Is that not possible in your situation? You don't specify
>>>> which bootloader you are using (or even which platform), but U-boot
>>>> can be configured to use the DTB loaded and adjusted by the firmware.
>>> We use a boot loader called BOLT which has an offline DTS generation
>>> part an a runtime patching part. At no point in time has it been
>>> necessary, desirable or even reliably possible to look at the kernel
>>> version and mangle the Device Tree such that "things" work. I do not
>>> even want to fathom what the code doing that would look like other
>>> than a big pile of mess. This is utterly error prone and completely
>>> breaks the boot loader/kernel abstraction.
>>>
>>> We strive to be able to update the boot loader and the kernel
>>> seemingly independently from one another even though obviously there
>>> are times where locksteps are necessary. What I like to do is:
>>>
>>> - put the changes in the DTB that are necessary for a *future* kernel
>>> well in advance such that by the time that newer kernel gets used
>>> these properties are already there
>>>
>>> - adding new properties does not break older kernels, they simply
>>> ignore them
>>>
>>> And this allows us to define a combination that always works while
>>> having a sliding window of backward/forward compatibility if we have
>>> locksteps in between.
>>>
>>> I can make changes in the downstream kernel such that we prune DT
>>> properties, but once we open that door, the list goes on and on and
>>> it is just a damage control strategy anyway.
>>>
>>> Why cannot the pinctrl framework infer that we have a 1:1 mapping in
>>> the absence of a 'gpio-ranges' property, but instead does the following:
>>>
>>> # cat
>>> /sys/kernel/debug/pinctrl/47e200000.gpio-pinctrl-bcm2711/gpio-ranges
>>> GPIO ranges handled:
>>> 0: pinctrl-bcm2711 GPIOS [4294967295 - 56] PINS [0 - 57]
> the driver init the gpio base with -1 which seems to lead to this huge
> unsigned integer. Very confusing from my PoV.
>>> #
>>>
>>> clearly it determined the end GPIO but it is not able to determine
>>> the start GPIO?
>>>
>>> And now, on a 5.10 kernel which does contain both the 'gpio-ranges'
>>> property and the said change to pinctrl-bcm2835.c I have this:
>>>
>>> # cat
>>> /sys/kernel/debug/pinctrl/47e200000.gpio-pinctrl-bcm2711/gpio-ranges
>>> GPIO ranges handled:
>>> 0: pinctrl-bcm2711 GPIOS [4294967295 - 56] PINS [0 - 57]
>>> 0: pinctrl-bcm2711 GPIOS [454 - 511] PINS [0 - 57]
>>> #
>>>
>>> What the heck?
>> And just to be clear, at this point I understand that the old kernel,
>> new DTB/DTS is not something we can obviously fix, however new kernel
>> old DTB/DTS *without* 'gpio-ranges' is something that should still
>> work to ease the pain.
> The whole problem started with make an optional DT property a required
> one afterwards.
>
> The only workaround i can think of is to check for the absence of
> 'gpio-ranges' in the pinctrl driver and mitigate. Not nice :(

it seems that other platform stumbled on the same issue:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220304&id=9b4924da4711674e62d97d4f5360446cc78337af

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220304&id=7ed07855773814337b9814f1c3e866df52ebce68

I will try to prepare a patch.

Best regards
Stefan

>
>> I will dig more into why we have this duplicated debugfs output (most
>> likely we are not unwinding an error path when we get -EPROBE_DEFER)
>> and also why pinctrl is not able to figure out the gpio start properly
>> without 'gpio-ranges'.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: DTB backward/forward compatibility with "pinctrl: bcm2835: Change init order for gpio hogs"

[Florian Fainelli]

Hi Stefan,

On 3/6/2022 7:03 AM, Stefan Wahren wrote:
> Hi Florian,
> 
> reanimate this old thread
> 
> Am 27.01.22 um 17:31 schrieb Stefan Wahren:
>> Hi,
>>
>> Am 26.01.22 um 02:33 schrieb Florian Fainelli:
>>>
>>> On 1/25/2022 11:58 AM, Florian Fainelli wrote:
>>>>
>>>> On 1/25/2022 11:48 AM, Phil Elwell wrote:
>>>>> Florian,
>>>>>
>>>>> On 25/01/2022 19:39, Florian Fainelli wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I am a bit frustrated by this commit, we picked it up via the
>>>>>> stable 5.10 and 5.15 trees into our downstream tree, and in the
>>>>>> absence of a suitable 'gpio-ranges' property for the GPIO
>>>>>> controller, the SPI controller keeps getting -EPROBE_DEFER for its
>>>>>> chip select. If the property is present, then all is well.
>>>>>>
>>>>>> Now the problem in my case is that the boot loader is responsible
>>>>>> for providing the DTB to the kernel, and until recently, we did not
>>>>>> update it to contain a suitable 'gpio-ranges' property. Now that it
>>>>>> has been updated however, older kernels which *do not* have said
>>>>>> change in the subject are also getting -EPROBE_DEFER for the SPI
>>>>>> chip select.
>>>>>>
>>>>>> So this is just breaking backward/forward compatibility with the
>>>>>> DTB unless both are updated in lock steps which is *extremely*
>>>>>> inconvenient.
>>>>>>
>>>>>> This is death by a thousand cuts.
>>>>>>
>>>>>> So how do we remedy this?
>>>>> It's unfortunate that both the DTS and driver were incorrect, and
>>>>> that the fixes were inter-dependent. At Raspberry Pi we make a point
>>>>> of treating the DTBs as part of the kernel, updating them at the
>>>>> same time. Is that not possible in your situation? You don't specify
>>>>> which bootloader you are using (or even which platform), but U-boot
>>>>> can be configured to use the DTB loaded and adjusted by the firmware.
>>>> We use a boot loader called BOLT which has an offline DTS generation
>>>> part an a runtime patching part. At no point in time has it been
>>>> necessary, desirable or even reliably possible to look at the kernel
>>>> version and mangle the Device Tree such that "things" work. I do not
>>>> even want to fathom what the code doing that would look like other
>>>> than a big pile of mess. This is utterly error prone and completely
>>>> breaks the boot loader/kernel abstraction.
>>>>
>>>> We strive to be able to update the boot loader and the kernel
>>>> seemingly independently from one another even though obviously there
>>>> are times where locksteps are necessary. What I like to do is:
>>>>
>>>> - put the changes in the DTB that are necessary for a *future* kernel
>>>> well in advance such that by the time that newer kernel gets used
>>>> these properties are already there
>>>>
>>>> - adding new properties does not break older kernels, they simply
>>>> ignore them
>>>>
>>>> And this allows us to define a combination that always works while
>>>> having a sliding window of backward/forward compatibility if we have
>>>> locksteps in between.
>>>>
>>>> I can make changes in the downstream kernel such that we prune DT
>>>> properties, but once we open that door, the list goes on and on and
>>>> it is just a damage control strategy anyway.
>>>>
>>>> Why cannot the pinctrl framework infer that we have a 1:1 mapping in
>>>> the absence of a 'gpio-ranges' property, but instead does the following:
>>>>
>>>> # cat
>>>> /sys/kernel/debug/pinctrl/47e200000.gpio-pinctrl-bcm2711/gpio-ranges
>>>> GPIO ranges handled:
>>>> 0: pinctrl-bcm2711 GPIOS [4294967295 - 56] PINS [0 - 57]
>> the driver init the gpio base with -1 which seems to lead to this huge
>> unsigned integer. Very confusing from my PoV.
>>>> #
>>>>
>>>> clearly it determined the end GPIO but it is not able to determine
>>>> the start GPIO?
>>>>
>>>> And now, on a 5.10 kernel which does contain both the 'gpio-ranges'
>>>> property and the said change to pinctrl-bcm2835.c I have this:
>>>>
>>>> # cat
>>>> /sys/kernel/debug/pinctrl/47e200000.gpio-pinctrl-bcm2711/gpio-ranges
>>>> GPIO ranges handled:
>>>> 0: pinctrl-bcm2711 GPIOS [4294967295 - 56] PINS [0 - 57]
>>>> 0: pinctrl-bcm2711 GPIOS [454 - 511] PINS [0 - 57]
>>>> #
>>>>
>>>> What the heck?
>>> And just to be clear, at this point I understand that the old kernel,
>>> new DTB/DTS is not something we can obviously fix, however new kernel
>>> old DTB/DTS *without* 'gpio-ranges' is something that should still
>>> work to ease the pain.
>> The whole problem started with make an optional DT property a required
>> one afterwards.
>>
>> The only workaround i can think of is to check for the absence of
>> 'gpio-ranges' in the pinctrl driver and mitigate. Not nice :(
> 
> it seems that other platform stumbled on the same issue:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220304&id=9b4924da4711674e62d97d4f5360446cc78337af
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220304&id=7ed07855773814337b9814f1c3e866df52ebce68
> 
> I will try to prepare a patch.

Thank you! Do you think we could have a core function expose which would 
do essentially avoid drivers having to sprinkle checks for the absence 
of a 'gpio-ranges' property?
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: DTB backward/forward compatibility with "pinctrl: bcm2835: Change init order for gpio hogs"

[Stefan Wahren]

Am 06.03.22 um 17:54 schrieb Florian Fainelli:
> Hi Stefan,
>
> On 3/6/2022 7:03 AM, Stefan Wahren wrote:
>> Hi Florian,
>>
>> reanimate this old thread
>>
>> Am 27.01.22 um 17:31 schrieb Stefan Wahren:
>>> Hi,
>>>
>>> Am 26.01.22 um 02:33 schrieb Florian Fainelli:
>>>>
>>>> On 1/25/2022 11:58 AM, Florian Fainelli wrote:
>>>>>
>>>>> On 1/25/2022 11:48 AM, Phil Elwell wrote:
>>>>>> Florian,
>>>>>>
>>>>>> On 25/01/2022 19:39, Florian Fainelli wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> I am a bit frustrated by this commit, we picked it up via the
>>>>>>> stable 5.10 and 5.15 trees into our downstream tree, and in the
>>>>>>> absence of a suitable 'gpio-ranges' property for the GPIO
>>>>>>> controller, the SPI controller keeps getting -EPROBE_DEFER for its
>>>>>>> chip select. If the property is present, then all is well.
>>>>>>>
>>>>>>> Now the problem in my case is that the boot loader is responsible
>>>>>>> for providing the DTB to the kernel, and until recently, we did not
>>>>>>> update it to contain a suitable 'gpio-ranges' property. Now that it
>>>>>>> has been updated however, older kernels which *do not* have said
>>>>>>> change in the subject are also getting -EPROBE_DEFER for the SPI
>>>>>>> chip select.
>>>>>>>
>>>>>>> So this is just breaking backward/forward compatibility with the
>>>>>>> DTB unless both are updated in lock steps which is *extremely*
>>>>>>> inconvenient.
>>>>>>>
>>>>>>> This is death by a thousand cuts.
>>>>>>>
>>>>>>> So how do we remedy this?
>>>>>> It's unfortunate that both the DTS and driver were incorrect, and
>>>>>> that the fixes were inter-dependent. At Raspberry Pi we make a point
>>>>>> of treating the DTBs as part of the kernel, updating them at the
>>>>>> same time. Is that not possible in your situation? You don't specify
>>>>>> which bootloader you are using (or even which platform), but U-boot
>>>>>> can be configured to use the DTB loaded and adjusted by the
>>>>>> firmware.
>>>>> We use a boot loader called BOLT which has an offline DTS generation
>>>>> part an a runtime patching part. At no point in time has it been
>>>>> necessary, desirable or even reliably possible to look at the kernel
>>>>> version and mangle the Device Tree such that "things" work. I do not
>>>>> even want to fathom what the code doing that would look like other
>>>>> than a big pile of mess. This is utterly error prone and completely
>>>>> breaks the boot loader/kernel abstraction.
>>>>>
>>>>> We strive to be able to update the boot loader and the kernel
>>>>> seemingly independently from one another even though obviously there
>>>>> are times where locksteps are necessary. What I like to do is:
>>>>>
>>>>> - put the changes in the DTB that are necessary for a *future* kernel
>>>>> well in advance such that by the time that newer kernel gets used
>>>>> these properties are already there
>>>>>
>>>>> - adding new properties does not break older kernels, they simply
>>>>> ignore them
>>>>>
>>>>> And this allows us to define a combination that always works while
>>>>> having a sliding window of backward/forward compatibility if we have
>>>>> locksteps in between.
>>>>>
>>>>> I can make changes in the downstream kernel such that we prune DT
>>>>> properties, but once we open that door, the list goes on and on and
>>>>> it is just a damage control strategy anyway.
>>>>>
>>>>> Why cannot the pinctrl framework infer that we have a 1:1 mapping in
>>>>> the absence of a 'gpio-ranges' property, but instead does the
>>>>> following:
>>>>>
>>>>> # cat
>>>>> /sys/kernel/debug/pinctrl/47e200000.gpio-pinctrl-bcm2711/gpio-ranges
>>>>> GPIO ranges handled:
>>>>> 0: pinctrl-bcm2711 GPIOS [4294967295 - 56] PINS [0 - 57]
>>> the driver init the gpio base with -1 which seems to lead to this huge
>>> unsigned integer. Very confusing from my PoV.
>>>>> #
>>>>>
>>>>> clearly it determined the end GPIO but it is not able to determine
>>>>> the start GPIO?
>>>>>
>>>>> And now, on a 5.10 kernel which does contain both the 'gpio-ranges'
>>>>> property and the said change to pinctrl-bcm2835.c I have this:
>>>>>
>>>>> # cat
>>>>> /sys/kernel/debug/pinctrl/47e200000.gpio-pinctrl-bcm2711/gpio-ranges
>>>>> GPIO ranges handled:
>>>>> 0: pinctrl-bcm2711 GPIOS [4294967295 - 56] PINS [0 - 57]
>>>>> 0: pinctrl-bcm2711 GPIOS [454 - 511] PINS [0 - 57]
>>>>> #
>>>>>
>>>>> What the heck?
>>>> And just to be clear, at this point I understand that the old kernel,
>>>> new DTB/DTS is not something we can obviously fix, however new kernel
>>>> old DTB/DTS *without* 'gpio-ranges' is something that should still
>>>> work to ease the pain.
>>> The whole problem started with make an optional DT property a required
>>> one afterwards.
>>>
>>> The only workaround i can think of is to check for the absence of
>>> 'gpio-ranges' in the pinctrl driver and mitigate. Not nice :(
>>
>> it seems that other platform stumbled on the same issue:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220304&id=9b4924da4711674e62d97d4f5360446cc78337af
>>
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220304&id=7ed07855773814337b9814f1c3e866df52ebce68
>>
>>
>> I will try to prepare a patch.
>
> Thank you! Do you think we could have a core function expose which
> would do essentially avoid drivers having to sprinkle checks for the
> absence of a 'gpio-ranges' property?
Hm, i don't think we can have one function to handle all cases. But i
can think of something like gpiochip_add_data_with_pin_range() which
handles a lot.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: DTB backward/forward compatibility with "pinctrl: bcm2835: Change init order for gpio hogs"

[Stefan Wahren]

Hi,

Am 06.03.22 um 17:54 schrieb Florian Fainelli:
> Hi Stefan,
>
> On 3/6/2022 7:03 AM, Stefan Wahren wrote:
>> Hi Florian,
>>
>> it seems that other platform stumbled on the same issue:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220304&id=9b4924da4711674e62d97d4f5360446cc78337af
>>
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220304&id=7ed07855773814337b9814f1c3e866df52ebce68
>>
>>
>> I will try to prepare a patch.
>
> Thank you! Do you think we could have a core function expose which
> would do essentially avoid drivers having to sprinkle checks for the
> absence of a 'gpio-ranges' property?

unfortunately my first attempts to implement it similiar to the
solutions above failed. I read the explanations from the original fix
from Christian Lamparter [1] and from my understanding the backward
compatibility must be implemented into gpiochip_add_data() or a variant.
Before the gpiochip is not registered and afterwards it would be too late.

My test scenario (worst case) is an additional gpio hog for the
Raspberry Pi 3 B with removed gpio-ranges:

diff --git a/arch/arm/boot/dts/bcm2837-rpi-3-b-plus.dts
b/arch/arm/boot/dts/bcm2837-rpi-3-b-plus.dts
index e12938b..a5163cc 100644
--- a/arch/arm/boot/dts/bcm2837-rpi-3-b-plus.dts
+++ b/arch/arm/boot/dts/bcm2837-rpi-3-b-plus.dts
@@ -123,6 +123,14 @@
                          "SD_DATA1_R",
                          "SD_DATA2_R",
                          "SD_DATA3_R";
+
+       ant1: ant1-hog {
+               gpio-hog;
+               gpios = <23 GPIO_ACTIVE_HIGH>;
+               output-high;
+               line-name = "ant1";
+       };
 };
 
 &hdmi {
diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi
index c113661..4b65954 100644
--- a/arch/arm/boot/dts/bcm283x.dtsi
+++ b/arch/arm/boot/dts/bcm283x.dtsi
@@ -126,7 +126,7 @@
                        interrupt-controller;
                        #interrupt-cells = <2>;
 
-                       gpio-ranges = <&gpio 0 0 54>;
+                       // gpio-ranges = <&gpio 0 0 54>;
 
                        /* Defines common pin muxing groups
                         *

[1] -
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220307&id=a86caa9ba5d70696ceb35d1d39caa20d8b641387
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: DTB backward/forward compatibility with "pinctrl: bcm2835: Change init order for gpio hogs"

[Stefan Wahren]

Am 07.03.22 um 12:38 schrieb Stefan Wahren:
> Hi,
>
> Am 06.03.22 um 17:54 schrieb Florian Fainelli:
>> Hi Stefan,
>>
>> On 3/6/2022 7:03 AM, Stefan Wahren wrote:
>>> Hi Florian,
>>>
>>> it seems that other platform stumbled on the same issue:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220304&id=9b4924da4711674e62d97d4f5360446cc78337af
>>>
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220304&id=7ed07855773814337b9814f1c3e866df52ebce68
>>>
>>>
>>> I will try to prepare a patch.
>> Thank you! Do you think we could have a core function expose which
>> would do essentially avoid drivers having to sprinkle checks for the
>> absence of a 'gpio-ranges' property?
> unfortunately my first attempts to implement it similiar to the
> solutions above failed. I read the explanations from the original fix
> from Christian Lamparter [1] and from my understanding the backward
> compatibility must be implemented into gpiochip_add_data() or a variant.
> Before the gpiochip is not registered and afterwards it would be too late.
>
> My test scenario (worst case) is an additional gpio hog for the
> Raspberry Pi 3 B with removed gpio-ranges:
>
Not sure this is the right place, but the following hack make the
Raspberry Pi boot for the defined worst case szenario (no gpio-ranges
but at least one gpio-hog):

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c

index 91dcf2c..23386dd 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -933,6 +933,24 @@ static int of_gpiochip_add_pin_range(struct
gpio_chip *chip
)
        if (!np)
                return 0;
 
+       if (!of_property_read_bool(np, "gpio-ranges")) {
+               pr_warn("%pOF: gpio-ranges missing. Try to fallback!\n",
np);
+
+               pctldev = of_pinctrl_get(np);
+               of_node_put(np);
+
+               if (!pctldev)
+                       return 0;
+
+               gpiochip_add_pin_range(chip,
+                               pinctrl_dev_get_devname(pctldev),
+                               0,
+                               0,
+                               chip->ngpio);
+
+               return 0;
+       }
+
        group_names = of_find_property(np, group_names_propname, NULL);
 
        for (;; index++) {
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: DTB backward/forward compatibility with "pinctrl: bcm2835: Change init order for gpio hogs"

[Florian Fainelli]

On 3/8/22 11:13 AM, Stefan Wahren wrote:
> Am 07.03.22 um 12:38 schrieb Stefan Wahren:
>> Hi,
>>
>> Am 06.03.22 um 17:54 schrieb Florian Fainelli:
>>> Hi Stefan,
>>>
>>> On 3/6/2022 7:03 AM, Stefan Wahren wrote:
>>>> Hi Florian,
>>>>
>>>> it seems that other platform stumbled on the same issue:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220304&id=9b4924da4711674e62d97d4f5360446cc78337af
>>>>
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220304&id=7ed07855773814337b9814f1c3e866df52ebce68
>>>>
>>>>
>>>> I will try to prepare a patch.
>>> Thank you! Do you think we could have a core function expose which
>>> would do essentially avoid drivers having to sprinkle checks for the
>>> absence of a 'gpio-ranges' property?
>> unfortunately my first attempts to implement it similiar to the
>> solutions above failed. I read the explanations from the original fix
>> from Christian Lamparter [1] and from my understanding the backward
>> compatibility must be implemented into gpiochip_add_data() or a variant.
>> Before the gpiochip is not registered and afterwards it would be too late.
>>
>> My test scenario (worst case) is an additional gpio hog for the
>> Raspberry Pi 3 B with removed gpio-ranges:
>>
> Not sure this is the right place, but the following hack make the
> Raspberry Pi boot for the defined worst case szenario (no gpio-ranges
> but at least one gpio-hog):

This worked with 'gpio-ranges' as well as without a 'gpio-ranges', the
kernel did warn as you added the message:

[    0.588730] /rdb/gpio@2200000: gpio-ranges missing. Try to fallback!

and gpio-ranges was the same before/after the patch:

# cat /sys/kernel/debug/pinctrl/47e200000.gpio-pinctrl-bcm2711/gpio-ranges
GPIO ranges handled:
0: pinctrl-bcm2711 GPIOS [4294967295 - 56] PINS [0 - 57]
0: pinctrl-bcm2711 GPIOS [454 - 511] PINS [0 - 57]
#


thanks a lot Stefan!

> 
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> 
> index 91dcf2c..23386dd 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -933,6 +933,24 @@ static int of_gpiochip_add_pin_range(struct
> gpio_chip *chip
> )
>         if (!np)
>                 return 0;
>  
> +       if (!of_property_read_bool(np, "gpio-ranges")) {
> +               pr_warn("%pOF: gpio-ranges missing. Try to fallback!\n",
> np);
> +
> +               pctldev = of_pinctrl_get(np);
> +               of_node_put(np);
> +
> +               if (!pctldev)
> +                       return 0;
> +
> +               gpiochip_add_pin_range(chip,
> +                               pinctrl_dev_get_devname(pctldev),
> +                               0,
> +                               0,
> +                               chip->ngpio);
> +
> +               return 0;
> +       }
> +
>         group_names = of_find_property(np, group_names_propname, NULL);
>  
>         for (;; index++) {
> 


-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel