* [PATCH] pci/quirks: Add quirk for Bayhub O2 SD controller @ 2021-12-08 0:09 Rajat Jain 2021-12-09 10:05 ` Ulf Hansson 2021-12-15 18:04 ` Bjorn Helgaas 0 siblings, 2 replies; 5+ messages in thread From: Rajat Jain @ 2021-12-08 0:09 UTC (permalink / raw) To: Adrian Hunter, Ulf Hansson, Bjorn Helgaas, linux-mmc, linux-kernel, linux-pci Cc: Rajat Jain, rajatxjain, jsbarnes, gwendal This particular SD controller from O2 / Bayhub only allows dword accesses to its LTR max latency registers: https://github.com/rajatxjain/public_shared/blob/main/OZ711LV2_appnote.pdf Thus add a quirk that saves and restores these registers manually using dword acesses: LTR Max Snoop Latency Register LTR Max No-Snoop Latency Register Signed-off-by: Rajat Jain <rajatja@google.com> --- drivers/mmc/host/sdhci-pci.h | 1 - drivers/pci/quirks.c | 39 ++++++++++++++++++++++++++++++++++++ include/linux/pci_ids.h | 1 + 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h index 5e3193278ff9..d47cc0ba7ca4 100644 --- a/drivers/mmc/host/sdhci-pci.h +++ b/drivers/mmc/host/sdhci-pci.h @@ -10,7 +10,6 @@ #define PCI_DEVICE_ID_O2_SDS1 0x8421 #define PCI_DEVICE_ID_O2_FUJIN2 0x8520 #define PCI_DEVICE_ID_O2_SEABIRD0 0x8620 -#define PCI_DEVICE_ID_O2_SEABIRD1 0x8621 #define PCI_DEVICE_ID_INTEL_PCH_SDIO0 0x8809 #define PCI_DEVICE_ID_INTEL_PCH_SDIO1 0x880a diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 003950c738d2..b7bd19802744 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -5857,3 +5857,42 @@ static void nvidia_ion_ahci_fixup(struct pci_dev *pdev) pdev->dev_flags |= PCI_DEV_FLAGS_HAS_MSI_MASKING; } DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0ab8, nvidia_ion_ahci_fixup); + +/* + * Bayhub OZ711LV2 SD controller has an errata that only allows DWORD accesses + * to the LTR max latency registers. Thus need to save and restore these + * registers manually. + */ +static void o2_seabird1_save_ltr(struct pci_dev *dev) +{ + struct pci_cap_saved_state *save_state; + u32 *reg32; + + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR); + if (save_state) { + reg32 = &save_state->cap.data[0]; + /* Preserve PCI_LTR_MAX_SNOOP_LAT & PCI_LTR_MAX_NOSNOOP_LAT */ + pci_read_config_dword(dev, 0x234, reg32); + } else { + pci_err(dev, "quirk can't save LTR snoop latency\n"); + } +} + +static void o2_seabird1_restore_ltr(struct pci_dev *dev) +{ + struct pci_cap_saved_state *save_state; + u32 *reg32; + + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR); + if (save_state) { + reg32 = &save_state->cap.data[0]; + /* Restore PCI_LTR_MAX_SNOOP_LAT & PCI_LTR_MAX_NOSNOOP_LAT */ + pci_write_config_dword(dev, 0x234, *reg32); + } else { + pci_err(dev, "quirk can't restore LTR snoop latency\n"); + } +} +DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_O2, PCI_DEVICE_ID_O2_SEABIRD1, + o2_seabird1_save_ltr); +DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_O2, PCI_DEVICE_ID_O2_SEABIRD1, + o2_seabird1_restore_ltr); diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index 011f2f1ea5bb..6ed16aa38196 100644 --- a/include/linux/pci_ids.h +++ b/include/linux/pci_ids.h @@ -1717,6 +1717,7 @@ #define PCI_DEVICE_ID_O2_8221 0x8221 #define PCI_DEVICE_ID_O2_8320 0x8320 #define PCI_DEVICE_ID_O2_8321 0x8321 +#define PCI_DEVICE_ID_O2_SEABIRD1 0x8621 #define PCI_VENDOR_ID_3DFX 0x121a #define PCI_DEVICE_ID_3DFX_VOODOO 0x0001 -- 2.34.1.400.ga245620fadb-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] pci/quirks: Add quirk for Bayhub O2 SD controller 2021-12-08 0:09 [PATCH] pci/quirks: Add quirk for Bayhub O2 SD controller Rajat Jain @ 2021-12-09 10:05 ` Ulf Hansson 2021-12-15 18:04 ` Bjorn Helgaas 1 sibling, 0 replies; 5+ messages in thread From: Ulf Hansson @ 2021-12-09 10:05 UTC (permalink / raw) To: Rajat Jain Cc: Adrian Hunter, Bjorn Helgaas, linux-mmc, linux-kernel, linux-pci, rajatxjain, jsbarnes, gwendal On Wed, 8 Dec 2021 at 01:09, Rajat Jain <rajatja@google.com> wrote: > > This particular SD controller from O2 / Bayhub only allows dword > accesses to its LTR max latency registers: > https://github.com/rajatxjain/public_shared/blob/main/OZ711LV2_appnote.pdf > > Thus add a quirk that saves and restores these registers > manually using dword acesses: > LTR Max Snoop Latency Register > LTR Max No-Snoop Latency Register > > Signed-off-by: Rajat Jain <rajatja@google.com> Acked-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > drivers/mmc/host/sdhci-pci.h | 1 - > drivers/pci/quirks.c | 39 ++++++++++++++++++++++++++++++++++++ > include/linux/pci_ids.h | 1 + > 3 files changed, 40 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h > index 5e3193278ff9..d47cc0ba7ca4 100644 > --- a/drivers/mmc/host/sdhci-pci.h > +++ b/drivers/mmc/host/sdhci-pci.h > @@ -10,7 +10,6 @@ > #define PCI_DEVICE_ID_O2_SDS1 0x8421 > #define PCI_DEVICE_ID_O2_FUJIN2 0x8520 > #define PCI_DEVICE_ID_O2_SEABIRD0 0x8620 > -#define PCI_DEVICE_ID_O2_SEABIRD1 0x8621 > > #define PCI_DEVICE_ID_INTEL_PCH_SDIO0 0x8809 > #define PCI_DEVICE_ID_INTEL_PCH_SDIO1 0x880a > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index 003950c738d2..b7bd19802744 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -5857,3 +5857,42 @@ static void nvidia_ion_ahci_fixup(struct pci_dev *pdev) > pdev->dev_flags |= PCI_DEV_FLAGS_HAS_MSI_MASKING; > } > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0ab8, nvidia_ion_ahci_fixup); > + > +/* > + * Bayhub OZ711LV2 SD controller has an errata that only allows DWORD accesses > + * to the LTR max latency registers. Thus need to save and restore these > + * registers manually. > + */ > +static void o2_seabird1_save_ltr(struct pci_dev *dev) > +{ > + struct pci_cap_saved_state *save_state; > + u32 *reg32; > + > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR); > + if (save_state) { > + reg32 = &save_state->cap.data[0]; > + /* Preserve PCI_LTR_MAX_SNOOP_LAT & PCI_LTR_MAX_NOSNOOP_LAT */ > + pci_read_config_dword(dev, 0x234, reg32); > + } else { > + pci_err(dev, "quirk can't save LTR snoop latency\n"); > + } > +} > + > +static void o2_seabird1_restore_ltr(struct pci_dev *dev) > +{ > + struct pci_cap_saved_state *save_state; > + u32 *reg32; > + > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR); > + if (save_state) { > + reg32 = &save_state->cap.data[0]; > + /* Restore PCI_LTR_MAX_SNOOP_LAT & PCI_LTR_MAX_NOSNOOP_LAT */ > + pci_write_config_dword(dev, 0x234, *reg32); > + } else { > + pci_err(dev, "quirk can't restore LTR snoop latency\n"); > + } > +} > +DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_O2, PCI_DEVICE_ID_O2_SEABIRD1, > + o2_seabird1_save_ltr); > +DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_O2, PCI_DEVICE_ID_O2_SEABIRD1, > + o2_seabird1_restore_ltr); > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > index 011f2f1ea5bb..6ed16aa38196 100644 > --- a/include/linux/pci_ids.h > +++ b/include/linux/pci_ids.h > @@ -1717,6 +1717,7 @@ > #define PCI_DEVICE_ID_O2_8221 0x8221 > #define PCI_DEVICE_ID_O2_8320 0x8320 > #define PCI_DEVICE_ID_O2_8321 0x8321 > +#define PCI_DEVICE_ID_O2_SEABIRD1 0x8621 > > #define PCI_VENDOR_ID_3DFX 0x121a > #define PCI_DEVICE_ID_3DFX_VOODOO 0x0001 > -- > 2.34.1.400.ga245620fadb-goog > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pci/quirks: Add quirk for Bayhub O2 SD controller 2021-12-08 0:09 [PATCH] pci/quirks: Add quirk for Bayhub O2 SD controller Rajat Jain 2021-12-09 10:05 ` Ulf Hansson @ 2021-12-15 18:04 ` Bjorn Helgaas 2021-12-15 18:15 ` Rajat Jain 1 sibling, 1 reply; 5+ messages in thread From: Bjorn Helgaas @ 2021-12-15 18:04 UTC (permalink / raw) To: Rajat Jain Cc: Adrian Hunter, Ulf Hansson, Bjorn Helgaas, linux-mmc, linux-kernel, linux-pci, rajatxjain, jsbarnes, gwendal On Tue, Dec 07, 2021 at 04:09:48PM -0800, Rajat Jain wrote: > This particular SD controller from O2 / Bayhub only allows dword > accesses to its LTR max latency registers: > https://github.com/rajatxjain/public_shared/blob/main/OZ711LV2_appnote.pdf What happens if we use a non-dword access? Unsupported Request? Invalid data returned? Writes ignored? I guess word accesses must not cause PCIe errors, since we still do them in pci_save_ltr_state() and pci_restore_ltr_state() even with this patch. The app note says 0x234 (Max Latency registers), 0x248 (L1 PM Substates Control 1), and 0x24c (L1 PM Substates Control 2) are all broken, but the patch only mentions 0x234. I guess for 0x248 and 0x24c (the L1 PM Substates Control registers), we're just lucky because those are dword registers, and all current users already do dword accesses. What if we instead changed pci_save_ltr_state() and pci_restore_ltr_state() to do a single dword access instead of two word accesses? That kind of sweeps it under the rug, but we're already doing that for 0x248 and 0x24c. If we did that, we shouldn't need a quirk at all, but the hardware bug is still lurking, and we should add a comment about it somewhere. I guess setpci (and maybe lspci) could still do smaller accesses and see whatever the bad behavior is. Hmmm. Maybe we just have to live with that. The app note doesn't actually say how to identify the part -- no "affected Device ID", for instance. Are we confident that the other O2_* devices are unaffected? > Thus add a quirk that saves and restores these registers > manually using dword acesses: > LTR Max Snoop Latency Register > LTR Max No-Snoop Latency Register > > Signed-off-by: Rajat Jain <rajatja@google.com> > --- > drivers/mmc/host/sdhci-pci.h | 1 - > drivers/pci/quirks.c | 39 ++++++++++++++++++++++++++++++++++++ > include/linux/pci_ids.h | 1 + > 3 files changed, 40 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h > index 5e3193278ff9..d47cc0ba7ca4 100644 > --- a/drivers/mmc/host/sdhci-pci.h > +++ b/drivers/mmc/host/sdhci-pci.h > @@ -10,7 +10,6 @@ > #define PCI_DEVICE_ID_O2_SDS1 0x8421 > #define PCI_DEVICE_ID_O2_FUJIN2 0x8520 > #define PCI_DEVICE_ID_O2_SEABIRD0 0x8620 > -#define PCI_DEVICE_ID_O2_SEABIRD1 0x8621 > > #define PCI_DEVICE_ID_INTEL_PCH_SDIO0 0x8809 > #define PCI_DEVICE_ID_INTEL_PCH_SDIO1 0x880a > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index 003950c738d2..b7bd19802744 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -5857,3 +5857,42 @@ static void nvidia_ion_ahci_fixup(struct pci_dev *pdev) > pdev->dev_flags |= PCI_DEV_FLAGS_HAS_MSI_MASKING; > } > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0ab8, nvidia_ion_ahci_fixup); > + > +/* > + * Bayhub OZ711LV2 SD controller has an errata that only allows DWORD accesses > + * to the LTR max latency registers. Thus need to save and restore these > + * registers manually. > + */ > +static void o2_seabird1_save_ltr(struct pci_dev *dev) > +{ > + struct pci_cap_saved_state *save_state; > + u32 *reg32; > + > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR); > + if (save_state) { > + reg32 = &save_state->cap.data[0]; > + /* Preserve PCI_LTR_MAX_SNOOP_LAT & PCI_LTR_MAX_NOSNOOP_LAT */ > + pci_read_config_dword(dev, 0x234, reg32); > + } else { > + pci_err(dev, "quirk can't save LTR snoop latency\n"); > + } > +} > + > +static void o2_seabird1_restore_ltr(struct pci_dev *dev) > +{ > + struct pci_cap_saved_state *save_state; > + u32 *reg32; > + > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR); > + if (save_state) { > + reg32 = &save_state->cap.data[0]; > + /* Restore PCI_LTR_MAX_SNOOP_LAT & PCI_LTR_MAX_NOSNOOP_LAT */ > + pci_write_config_dword(dev, 0x234, *reg32); > + } else { > + pci_err(dev, "quirk can't restore LTR snoop latency\n"); > + } > +} > +DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_O2, PCI_DEVICE_ID_O2_SEABIRD1, > + o2_seabird1_save_ltr); > +DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_O2, PCI_DEVICE_ID_O2_SEABIRD1, > + o2_seabird1_restore_ltr); > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > index 011f2f1ea5bb..6ed16aa38196 100644 > --- a/include/linux/pci_ids.h > +++ b/include/linux/pci_ids.h > @@ -1717,6 +1717,7 @@ > #define PCI_DEVICE_ID_O2_8221 0x8221 > #define PCI_DEVICE_ID_O2_8320 0x8320 > #define PCI_DEVICE_ID_O2_8321 0x8321 > +#define PCI_DEVICE_ID_O2_SEABIRD1 0x8621 > > #define PCI_VENDOR_ID_3DFX 0x121a > #define PCI_DEVICE_ID_3DFX_VOODOO 0x0001 > -- > 2.34.1.400.ga245620fadb-goog > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pci/quirks: Add quirk for Bayhub O2 SD controller 2021-12-15 18:04 ` Bjorn Helgaas @ 2021-12-15 18:15 ` Rajat Jain 2021-12-15 20:27 ` Bjorn Helgaas 0 siblings, 1 reply; 5+ messages in thread From: Rajat Jain @ 2021-12-15 18:15 UTC (permalink / raw) To: Bjorn Helgaas Cc: Rajat Jain, Adrian Hunter, Ulf Hansson, Bjorn Helgaas, linux-mmc, Linux Kernel Mailing List, linux-pci, Jesse Barnes, gwendal Hi Bjorn, Thanks for taking a look. On Wed, Dec 15, 2021 at 10:04 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Dec 07, 2021 at 04:09:48PM -0800, Rajat Jain wrote: > > This particular SD controller from O2 / Bayhub only allows dword > > accesses to its LTR max latency registers: > > https://github.com/rajatxjain/public_shared/blob/main/OZ711LV2_appnote.pdf > > What happens if we use a non-dword access? Unsupported Request? > Invalid data returned? Writes ignored? Invalid data values are read / written. > > I guess word accesses must not cause PCIe errors, since we still do > them in pci_save_ltr_state() and pci_restore_ltr_state() even with > this patch. Yes, that is correct. > > The app note says 0x234 (Max Latency registers), 0x248 (L1 PM > Substates Control 1), and 0x24c (L1 PM Substates Control 2) are all > broken, but the patch only mentions 0x234. > > I guess for 0x248 and 0x24c (the L1 PM Substates Control registers), > we're just lucky because those are dword registers, and all current > users already do dword accesses. Yes, that is right. > > What if we instead changed pci_save_ltr_state() and > pci_restore_ltr_state() to do a single dword access instead of two > word accesses? That kind of sweeps it under the rug, but we're > already doing that for 0x248 and 0x24c. Yes, that is what I had in mind originally, and actually I'd prefer that too. I was afraid you might disagree :-). It sounds like we're on the same page though, so should I send a patch with that approach? > > If we did that, we shouldn't need a quirk at all, but the hardware bug > is still lurking, and we should add a comment about it somewhere. I can add a comment in pci_save_ltr_state() / pci_restore_ltr_state(). > > I guess setpci (and maybe lspci) could still do smaller accesses and > see whatever the bad behavior is. Hmmm. Maybe we just have to live > with that. > > The app note doesn't actually say how to identify the part -- no > "affected Device ID", for instance. Are we confident that the other > O2_* devices are unaffected? Yes, I noticed that. I confirmed with them that no other parts are affected (in an internal bug unfortunately). I can ask them to update their appnote also. Thanks & Best Regards, Rajat > > > Thus add a quirk that saves and restores these registers > > manually using dword acesses: > > LTR Max Snoop Latency Register > > LTR Max No-Snoop Latency Register > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > --- > > drivers/mmc/host/sdhci-pci.h | 1 - > > drivers/pci/quirks.c | 39 ++++++++++++++++++++++++++++++++++++ > > include/linux/pci_ids.h | 1 + > > 3 files changed, 40 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h > > index 5e3193278ff9..d47cc0ba7ca4 100644 > > --- a/drivers/mmc/host/sdhci-pci.h > > +++ b/drivers/mmc/host/sdhci-pci.h > > @@ -10,7 +10,6 @@ > > #define PCI_DEVICE_ID_O2_SDS1 0x8421 > > #define PCI_DEVICE_ID_O2_FUJIN2 0x8520 > > #define PCI_DEVICE_ID_O2_SEABIRD0 0x8620 > > -#define PCI_DEVICE_ID_O2_SEABIRD1 0x8621 > > > > #define PCI_DEVICE_ID_INTEL_PCH_SDIO0 0x8809 > > #define PCI_DEVICE_ID_INTEL_PCH_SDIO1 0x880a > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > index 003950c738d2..b7bd19802744 100644 > > --- a/drivers/pci/quirks.c > > +++ b/drivers/pci/quirks.c > > @@ -5857,3 +5857,42 @@ static void nvidia_ion_ahci_fixup(struct pci_dev *pdev) > > pdev->dev_flags |= PCI_DEV_FLAGS_HAS_MSI_MASKING; > > } > > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0ab8, nvidia_ion_ahci_fixup); > > + > > +/* > > + * Bayhub OZ711LV2 SD controller has an errata that only allows DWORD accesses > > + * to the LTR max latency registers. Thus need to save and restore these > > + * registers manually. > > + */ > > +static void o2_seabird1_save_ltr(struct pci_dev *dev) > > +{ > > + struct pci_cap_saved_state *save_state; > > + u32 *reg32; > > + > > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR); > > + if (save_state) { > > + reg32 = &save_state->cap.data[0]; > > + /* Preserve PCI_LTR_MAX_SNOOP_LAT & PCI_LTR_MAX_NOSNOOP_LAT */ > > + pci_read_config_dword(dev, 0x234, reg32); > > + } else { > > + pci_err(dev, "quirk can't save LTR snoop latency\n"); > > + } > > +} > > + > > +static void o2_seabird1_restore_ltr(struct pci_dev *dev) > > +{ > > + struct pci_cap_saved_state *save_state; > > + u32 *reg32; > > + > > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR); > > + if (save_state) { > > + reg32 = &save_state->cap.data[0]; > > + /* Restore PCI_LTR_MAX_SNOOP_LAT & PCI_LTR_MAX_NOSNOOP_LAT */ > > + pci_write_config_dword(dev, 0x234, *reg32); > > + } else { > > + pci_err(dev, "quirk can't restore LTR snoop latency\n"); > > + } > > +} > > +DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_O2, PCI_DEVICE_ID_O2_SEABIRD1, > > + o2_seabird1_save_ltr); > > +DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_O2, PCI_DEVICE_ID_O2_SEABIRD1, > > + o2_seabird1_restore_ltr); > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > > index 011f2f1ea5bb..6ed16aa38196 100644 > > --- a/include/linux/pci_ids.h > > +++ b/include/linux/pci_ids.h > > @@ -1717,6 +1717,7 @@ > > #define PCI_DEVICE_ID_O2_8221 0x8221 > > #define PCI_DEVICE_ID_O2_8320 0x8320 > > #define PCI_DEVICE_ID_O2_8321 0x8321 > > +#define PCI_DEVICE_ID_O2_SEABIRD1 0x8621 > > > > #define PCI_VENDOR_ID_3DFX 0x121a > > #define PCI_DEVICE_ID_3DFX_VOODOO 0x0001 > > -- > > 2.34.1.400.ga245620fadb-goog > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pci/quirks: Add quirk for Bayhub O2 SD controller 2021-12-15 18:15 ` Rajat Jain @ 2021-12-15 20:27 ` Bjorn Helgaas 0 siblings, 0 replies; 5+ messages in thread From: Bjorn Helgaas @ 2021-12-15 20:27 UTC (permalink / raw) To: Rajat Jain Cc: Rajat Jain, Adrian Hunter, Ulf Hansson, Bjorn Helgaas, linux-mmc, Linux Kernel Mailing List, linux-pci, Jesse Barnes, gwendal On Wed, Dec 15, 2021 at 10:15:02AM -0800, Rajat Jain wrote: > On Wed, Dec 15, 2021 at 10:04 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Dec 07, 2021 at 04:09:48PM -0800, Rajat Jain wrote: > > > This particular SD controller from O2 / Bayhub only allows dword > > > accesses to its LTR max latency registers: > > > https://github.com/rajatxjain/public_shared/blob/main/OZ711LV2_appnote.pdf > > > > What happens if we use a non-dword access? Unsupported Request? > > Invalid data returned? Writes ignored? > > Invalid data values are read / written. > > > I guess word accesses must not cause PCIe errors, since we still do > > them in pci_save_ltr_state() and pci_restore_ltr_state() even with > > this patch. > > Yes, that is correct. > > > The app note says 0x234 (Max Latency registers), 0x248 (L1 PM > > Substates Control 1), and 0x24c (L1 PM Substates Control 2) are all > > broken, but the patch only mentions 0x234. > > > > I guess for 0x248 and 0x24c (the L1 PM Substates Control registers), > > we're just lucky because those are dword registers, and all current > > users already do dword accesses. > > Yes, that is right. > > > What if we instead changed pci_save_ltr_state() and > > pci_restore_ltr_state() to do a single dword access instead of two > > word accesses? That kind of sweeps it under the rug, but we're > > already doing that for 0x248 and 0x24c. > > Yes, that is what I had in mind originally, and actually I'd prefer > that too. I was afraid you might disagree :-). It sounds like we're on > the same page though, so should I send a patch with that approach? I think so. I don't *like* papering over it, but the quirk only compensates for one situation (pci_save_ltr_state() and pci_restore_ltr_state()). Any other places where we might read/write the LTR values will still fail. We don't have any other places yet, but if/when we get ASPM L1.2 figured out, I think we will. If we had a quirk mechanism for filtering config accesses to certain devices, that would be ideal, but I don't think we have that. If you squint hard enough, aer_inject.c has something like that, but it's not general purpose. > > If we did that, we shouldn't need a quirk at all, but the hardware bug > > is still lurking, and we should add a comment about it somewhere. > > I can add a comment in pci_save_ltr_state() / pci_restore_ltr_state(). Maybe also in pcie_aspm_cap_init() for the L1 PM part. Just a one-liner should be enough. All the details will be in the commit log and the app note. Bjorn ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-12-15 20:27 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-08 0:09 [PATCH] pci/quirks: Add quirk for Bayhub O2 SD controller Rajat Jain 2021-12-09 10:05 ` Ulf Hansson 2021-12-15 18:04 ` Bjorn Helgaas 2021-12-15 18:15 ` Rajat Jain 2021-12-15 20:27 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).