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