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