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'.