Message-ID: <e9ab9c22-f73b-fe72-820a-4f2825c3dabc@lucaceresoli.net>
Date:   Fri, 25 Jun 2021 10:10:06 +0200
From:   Luca Ceresoli <luca@...aceresoli.net>
To:     Linus Walleij <linus.walleij@...aro.org>,
        Kishon Vijay Abraham I <kishon@...com>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>
Cc:     Pali Rohár <pali@...nel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        linux-pci <linux-pci@...r.kernel.org>,
        Linux-OMAP <linux-omap@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Rob Herring <robh@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>
Subject: Re: [PATCH v2] PCI: dra7xx: Fix reset behaviour

Hi Linus,

On 25/06/21 01:11, Linus Walleij wrote:
> On Tue, Jun 22, 2021 at 3:57 PM Kishon Vijay Abraham I <kishon@...com> wrote:
> 
>> While the patch itself is correct, this kind-of changes the behavior on
>> already upstreamed platforms. Previously the driver expected #PERST to
>> be asserted be external means (or default power-up state) and only takes
>> care of de-asserting the #PERST line.
>>
>> There are 2 platforms that will be impacted due to this change
>> 1) arch/arm/boot/dts/am57xx-beagle-x15-common.dist (has an inverter on
>> GPIO line)
>> 2) arch/arm/boot/dts/am571x-idk.dts (directly connected to #PERST)
>>
>> For 1), gpiod_set_value(reset, 0) will assert the PERST line due to the
>> inverter (and GPIO_ACTIVE_LOW)
>> For 2), gpiod_set_value(reset, 0) will assert the PERST line because we
>> have GPIO_ACTIVE_HIGH
> 
> The presence of an inverter makes it necessary to model this the right
> way to get out of the situation.
> 
>> So this patch should have to be accompanied with DT changes (and this
>> patch also breaks old DT compatibility).
> 
> There are ways to deal with this perfectly. It may or may not be worth
> the extra work. But I can show how it is done.
> 
> Make the patch to the driver that assumes driving the gpio descriptor
> to 1 (asserted) has the desired effect.
> 
> In this patch, I would include a hunk that fixes the above device trees,
> so they are correct from this point. This is one of the few cases where
> I think it warrants to fix the driver and the DTS file at the same time,
> but the DTS can also be patched separately because of the described
> solution below:
> 
> To avoid regressions with old device trees, add code to
> drivers/gpio/gpiolib-of.c in function of_gpio_flags_quirks()
> to react to the old incorrect device trees. This is where we
> stockpile OF errors and bug fixes.
> 
> This needs to be pretty elaborate. It begins like this:
> 
> if (IS_ENABLED(CONFIG_PCI) &&
>       (of_machine_is_compatible("ti,am572x-beagle-x15") ||
>       of_machine_is_compatible("ti,am5718-idk")) &&
>       of_node_name_eq(np, "pcie")) {
>       /* ... add code to check and enforce the flags ... */
> }
> 
> You see the idea here. Include this in the patch to make the
> Perfect(TM) solution to this problem both fixing all device trees
> in place and dealing with the old erroneous ones using some
> elaborate code.
> 
> There are plenty of examples on how to detect warn and
> modify flags in of_gpio_flags_quirks() make it clear and add
> some warning prints and comments. Keep me and Bartosz
> in the loop. It'll look fine in the end.

Thanks for thaking the time to explain this in detail!

As I volounteered to write the patch, and since I'm lazy, I was going to
vote for the former solution. But to be honest the latter has some good
reason to exist as it handles the case of kernel upgrade without DT
upgrade...

My bad, this two-liner patch is growing so big. :-)

-- 
Luca

Powered by blists - more mailing lists