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.