| Message ID | 20211005102430.63716-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | mmc: sdhci-pci: Add some CD GPIO related quirks | expand |
On Tue, Oct 05, 2021 at 01:24:24PM +0300, Andy Shevchenko wrote: > It appears that one of the supported platform magically worked with the > custom IRQ handler (any hints how?) while having two PCB designs with > an opposite CD sense level. Here is an attempt to fix it by quirking out > CD GPIO. > > Patch 1 introduces two additional quirks (it's done this way due to > patch 3, see below). Patch 2 is an actual fix for the mentioned platform. > If backported need to be taken with patch 1 together. Patch 3 is (RFT) > clean up. The questionable part here is the locking scheme. Shouldn't > we do something similar in the generic IRQ handler of SDHCI? Or Broxton > case has something quite different in mind? > > Patches 4-6 are dead-code removals. Patch 4 accompanying patch 2, patches > 5-6 just similar to it, but (functionally) independent. Would like to hear > if it's okay to do. > > Any comments, hints, advice are welcome! Adrian, Ulf, any comments on this? At least first two fix the regression and would be nice to have them in sooner than later (I can split them separately if required).
On 07/10/2021 19:42, Andy Shevchenko wrote: > On Tue, Oct 05, 2021 at 01:24:24PM +0300, Andy Shevchenko wrote: >> It appears that one of the supported platform magically worked with the >> custom IRQ handler (any hints how?) while having two PCB designs with >> an opposite CD sense level. Here is an attempt to fix it by quirking out >> CD GPIO. >> >> Patch 1 introduces two additional quirks (it's done this way due to >> patch 3, see below). Patch 2 is an actual fix for the mentioned platform. >> If backported need to be taken with patch 1 together. Patch 3 is (RFT) >> clean up. The questionable part here is the locking scheme. Shouldn't >> we do something similar in the generic IRQ handler of SDHCI? Or Broxton >> case has something quite different in mind? >> >> Patches 4-6 are dead-code removals. Patch 4 accompanying patch 2, patches >> 5-6 just similar to it, but (functionally) independent. Would like to hear >> if it's okay to do. >> >> Any comments, hints, advice are welcome! > > Adrian, Ulf, any comments on this? At least first two fix the regression and > would be nice to have them in sooner than later (I can split them separately > if required). I am not sure we need new quirks, given that we can just hook the callback and do anything that way. However I really haven't had time to look closely yet, sorry.
It looks like you were previously using the `cd-gpios` DT property to determine this. It also sounds like you switched from DT to ACPI so you lost the ability to use this field? Can you not use something like the following: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/v5.10/drivers/mmc/host/sdhci-acpi.c;l=945 p.s., Sorry for resending. I sent it as rich text before. On Thu, Oct 7, 2021 at 10:47 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Oct 05, 2021 at 01:24:24PM +0300, Andy Shevchenko wrote: > > It appears that one of the supported platform magically worked with the > > custom IRQ handler (any hints how?) while having two PCB designs with > > an opposite CD sense level. Here is an attempt to fix it by quirking out > > CD GPIO. > > > > Patch 1 introduces two additional quirks (it's done this way due to > > patch 3, see below). Patch 2 is an actual fix for the mentioned platform. > > If backported need to be taken with patch 1 together. Patch 3 is (RFT) > > clean up. The questionable part here is the locking scheme. Shouldn't > > we do something similar in the generic IRQ handler of SDHCI? Or Broxton > > case has something quite different in mind? > > > > Patches 4-6 are dead-code removals. Patch 4 accompanying patch 2, patches > > 5-6 just similar to it, but (functionally) independent. Would like to hear > > if it's okay to do. > > > > Any comments, hints, advice are welcome! > > Adrian, Ulf, any comments on this? At least first two fix the regression and > would be nice to have them in sooner than later (I can split them separately > if required). > > -- > With Best Regards, > Andy Shevchenko > >
On Thu, Oct 7, 2021 at 10:45 PM Adrian Hunter <adrian.hunter@intel.com> wrote: > On 07/10/2021 19:42, Andy Shevchenko wrote: > > On Tue, Oct 05, 2021 at 01:24:24PM +0300, Andy Shevchenko wrote: ... > > Adrian, Ulf, any comments on this? At least first two fix the regression and > > would be nice to have them in sooner than later (I can split them separately > > if required). > > I am not sure we need new quirks, given that we can just hook the callback > and do anything that way. However I really haven't had time to look closely > yet, sorry. Adrian, thanks for review, I'll address your comments in v2.
On Fri, Oct 8, 2021 at 1:29 AM Raul Rangel <rrangel@chromium.org> wrote: > > It looks like you were previously using the `cd-gpios` DT property to > determine this. No, it's not the case. The case is that we switched from hard coded stuff (board files) to ACPI. > It also sounds like you switched from DT to ACPI so > you lost the ability to use this field? > > Can you not use something like the following: > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/v5.10/drivers/mmc/host/sdhci-acpi.c;l=945 No. It won't work. Anything like this won't work. The problem here is on PCB level due to different uSD connectors () being use: one is Active High, the other is Active Low for CD signal. ... > > > Any comments, hints, advice are welcome!
It appears that one of the supported platform magically worked with the custom IRQ handler (any hints how?) while having two PCB designs with an opposite CD sense level. Here is an attempt to fix it by quirking out CD GPIO. Patch 1 introduces two additional quirks (it's done this way due to patch 3, see below). Patch 2 is an actual fix for the mentioned platform. If backported need to be taken with patch 1 together. Patch 3 is (RFT) clean up. The questionable part here is the locking scheme. Shouldn't we do something similar in the generic IRQ handler of SDHCI? Or Broxton case has something quite different in mind? Patches 4-6 are dead-code removals. Patch 4 accompanying patch 2, patches 5-6 just similar to it, but (functionally) independent. Would like to hear if it's okay to do. Any comments, hints, advice are welcome! Andy Shevchenko (6): mmc: sdhci: Introduce couple of quirks to ignore particular state of CD GPIO mmc: sdhci-pci: Read card detect from ACPI for Intel Merrifield mmc: sdhci: Replace bxt_get_cd() with SDHCI_QUIRK_CARD_DETECTION_IF_GPIO_HIGH mmc: sdhci-pci: Remove dead code (struct sdhci_pci_data et al) mmc: sdhci-pci: Remove dead code (cd_gpio, cd_irq et al) mmc: sdhci-pci: Remove dead code (rst_n_gpio et al) drivers/mmc/host/Makefile | 1 - drivers/mmc/host/sdhci-acpi.c | 24 +---- drivers/mmc/host/sdhci-pci-core.c | 166 +++-------------------------- drivers/mmc/host/sdhci-pci-data.c | 6 -- drivers/mmc/host/sdhci-pci.h | 5 - drivers/mmc/host/sdhci.c | 13 ++- drivers/mmc/host/sdhci.h | 4 + include/linux/mmc/sdhci-pci-data.h | 18 ---- 8 files changed, 27 insertions(+), 210 deletions(-) delete mode 100644 drivers/mmc/host/sdhci-pci-data.c delete mode 100644 include/linux/mmc/sdhci-pci-data.h