From: Linus Walleij <linus.walleij@linaro.org> To: Kishon Vijay Abraham I <kishon@ti.com>, Bartosz Golaszewski <bgolaszewski@baylibre.com> Cc: "Luca Ceresoli" <luca@lucaceresoli.net>, "Pali Rohár" <pali@kernel.org>, "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>, linux-pci <linux-pci@vger.kernel.org>, Linux-OMAP <linux-omap@vger.kernel.org>, "Linux ARM" <linux-arm-kernel@lists.infradead.org>, linux-kernel <linux-kernel@vger.kernel.org>, "Rob Herring" <robh@kernel.org>, "Bjorn Helgaas" <bhelgaas@google.com> Subject: Re: [PATCH v2] PCI: dra7xx: Fix reset behaviour Date: Fri, 25 Jun 2021 01:11:15 +0200 [thread overview] Message-ID: <CACRpkdYp17MLavG_OMSHmS5DfMGLXrfK11nDfJX86rM9LJ9DtQ@mail.gmail.com> (raw) In-Reply-To: <4d4c0d4d-41b4-4756-5189-bffa15f88406@ti.com> On Tue, Jun 22, 2021 at 3:57 PM Kishon Vijay Abraham I <kishon@ti.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. Yours, Linus Walleij
WARNING: multiple messages have this Message-ID (diff)
From: Linus Walleij <linus.walleij@linaro.org> To: Kishon Vijay Abraham I <kishon@ti.com>, Bartosz Golaszewski <bgolaszewski@baylibre.com> Cc: "Luca Ceresoli" <luca@lucaceresoli.net>, "Pali Rohár" <pali@kernel.org>, "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>, linux-pci <linux-pci@vger.kernel.org>, Linux-OMAP <linux-omap@vger.kernel.org>, "Linux ARM" <linux-arm-kernel@lists.infradead.org>, linux-kernel <linux-kernel@vger.kernel.org>, "Rob Herring" <robh@kernel.org>, "Bjorn Helgaas" <bhelgaas@google.com> Subject: Re: [PATCH v2] PCI: dra7xx: Fix reset behaviour Date: Fri, 25 Jun 2021 01:11:15 +0200 [thread overview] Message-ID: <CACRpkdYp17MLavG_OMSHmS5DfMGLXrfK11nDfJX86rM9LJ9DtQ@mail.gmail.com> (raw) In-Reply-To: <4d4c0d4d-41b4-4756-5189-bffa15f88406@ti.com> On Tue, Jun 22, 2021 at 3:57 PM Kishon Vijay Abraham I <kishon@ti.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. Yours, Linus Walleij _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-06-24 23:11 UTC|newest] Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-31 9:05 [PATCH v2] PCI: dra7xx: Fix reset behaviour Luca Ceresoli 2021-05-31 9:05 ` Luca Ceresoli 2021-05-31 13:32 ` Pali Rohár 2021-05-31 13:32 ` Pali Rohár 2021-05-31 13:54 ` Luca Ceresoli 2021-05-31 13:54 ` Luca Ceresoli 2021-05-31 16:00 ` Kishon Vijay Abraham I 2021-05-31 16:00 ` Kishon Vijay Abraham I 2021-05-31 16:22 ` Pali Rohár 2021-05-31 16:22 ` Pali Rohár 2021-06-22 10:57 ` Luca Ceresoli 2021-06-22 10:57 ` Luca Ceresoli 2021-06-22 11:06 ` Pali Rohár 2021-06-22 11:06 ` Pali Rohár 2021-06-22 11:56 ` Lorenzo Pieralisi 2021-06-22 11:56 ` Lorenzo Pieralisi 2021-06-22 12:16 ` Pali Rohár 2021-06-22 12:16 ` Pali Rohár 2021-06-22 13:31 ` Luca Ceresoli 2021-06-22 13:31 ` Luca Ceresoli 2021-06-22 13:57 ` Kishon Vijay Abraham I 2021-06-22 13:57 ` Kishon Vijay Abraham I 2021-06-22 20:52 ` Pali Rohár 2021-06-22 20:52 ` Pali Rohár 2021-06-22 21:08 ` Luca Ceresoli 2021-06-22 21:08 ` Luca Ceresoli 2021-06-22 21:19 ` Pali Rohár 2021-06-22 21:19 ` Pali Rohár 2021-06-22 21:36 ` Luca Ceresoli 2021-06-22 21:36 ` Luca Ceresoli 2021-06-22 22:23 ` Pali Rohár 2021-06-22 22:23 ` Pali Rohár 2021-06-24 21:31 ` Luca Ceresoli 2021-06-24 21:31 ` Luca Ceresoli 2021-06-24 21:42 ` Pali Rohár 2021-06-24 21:42 ` Pali Rohár 2021-06-24 23:18 ` Linus Walleij 2021-06-24 23:18 ` Linus Walleij 2021-06-24 23:34 ` Pali Rohár 2021-06-24 23:34 ` Pali Rohár 2021-06-25 0:09 ` Linus Walleij 2021-06-25 0:09 ` Linus Walleij 2021-06-25 8:05 ` Luca Ceresoli 2021-06-25 8:05 ` Luca Ceresoli 2021-06-22 21:04 ` Luca Ceresoli 2021-06-22 21:04 ` Luca Ceresoli 2021-06-24 23:11 ` Linus Walleij [this message] 2021-06-24 23:11 ` Linus Walleij 2021-06-25 8:10 ` Luca Ceresoli 2021-06-25 8:10 ` Luca Ceresoli 2021-06-22 14:23 ` Lorenzo Pieralisi 2021-06-22 14:23 ` Lorenzo Pieralisi 2021-06-22 20:48 ` Pali Rohár 2021-06-22 20:48 ` Pali Rohár 2021-06-22 20:55 ` Pali Rohár 2021-06-22 20:55 ` Pali Rohár 2021-06-22 21:13 ` Luca Ceresoli 2021-06-22 21:13 ` Luca Ceresoli 2021-06-01 9:03 ` Luca Ceresoli 2021-06-01 9:03 ` Luca Ceresoli
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CACRpkdYp17MLavG_OMSHmS5DfMGLXrfK11nDfJX86rM9LJ9DtQ@mail.gmail.com \ --to=linus.walleij@linaro.org \ --cc=bgolaszewski@baylibre.com \ --cc=bhelgaas@google.com \ --cc=kishon@ti.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=luca@lucaceresoli.net \ --cc=pali@kernel.org \ --cc=robh@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.