* [PATCH] pci: Make DWORD accesses while saving / restoring LTR state @ 2021-12-22 1:21 Rajat Jain 2021-12-22 1:24 ` Rajat Jain 2022-01-11 16:39 ` Bjorn Helgaas 0 siblings, 2 replies; 4+ messages in thread From: Rajat Jain @ 2021-12-22 1:21 UTC (permalink / raw) To: Bjorn Helgaas, kw, Rajat Jain, Logan Gunthorpe, linux-pci, linux-kernel Cc: rajatxjain Some devices have an errata such that they only support DWORD accesses to some registers. For e.g. this Bayhub O2 device ([VID:DID] = [0x1217:0x8621]) only supports DWORD accesses to LTR latency registers and L1 PM substates control registers: https://github.com/rajatxjain/public_shared/blob/main/OZ711LV2_appnote.pdf Since L1 PM substate control registers are DWORD sized, and hence their access in the kernel is already DWORD sized, so we don't need to do anything for them. However, the LTR registers being WORD sized, are in need of a solution. This patch converts the WORD sized accesses to these registers, into DWORD sized accesses, while saving and restoring them. Signed-off-by: Rajat Jain <rajatja@google.com> --- drivers/pci/pci.c | 24 ++++++++++++++++-------- drivers/pci/pcie/aspm.c | 1 + 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 3d2fb394986a..efa8cd16827f 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1556,7 +1556,7 @@ static void pci_save_ltr_state(struct pci_dev *dev) { int ltr; struct pci_cap_saved_state *save_state; - u16 *cap; + u32 *cap; if (!pci_is_pcie(dev)) return; @@ -1571,25 +1571,33 @@ static void pci_save_ltr_state(struct pci_dev *dev) return; } - cap = (u16 *)&save_state->cap.data[0]; - pci_read_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap++); - pci_read_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, cap++); + /* + * We deliberately do a dword access to save both PCI_LTR_MAX_SNOOP_LAT + * and PCI_LTR_MAX_NOSNOOP_LAT together since some devices only support + * dword accesses to these registers. + */ + cap = &save_state->cap.data[0]; + pci_read_config_dword(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap); } static void pci_restore_ltr_state(struct pci_dev *dev) { struct pci_cap_saved_state *save_state; int ltr; - u16 *cap; + u32 *cap; save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR); ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR); if (!save_state || !ltr) return; - cap = (u16 *)&save_state->cap.data[0]; - pci_write_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap++); - pci_write_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, *cap++); + /* + * We deliberately do a dword access to restore both + * PCI_LTR_MAX_SNOOP_LAT and PCI_LTR_MAX_NOSNOOP_LAT together since + * some devices only support dword accesses to these registers. + */ + cap = &save_state->cap.data[0]; + pci_write_config_dword(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap); } /** diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 52c74682601a..083f47a7b69b 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -496,6 +496,7 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link, encode_l12_threshold(l1_2_threshold, &scale, &value); ctl1 |= t_common_mode << 8 | scale << 29 | value << 16; + /* Always make DWORD sized accesses to these registers */ pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1); pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, &pctl2); pci_read_config_dword(child, child->l1ss + PCI_L1SS_CTL1, &cctl1); -- 2.34.1.307.g9b7440fafd-goog ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] pci: Make DWORD accesses while saving / restoring LTR state 2021-12-22 1:21 [PATCH] pci: Make DWORD accesses while saving / restoring LTR state Rajat Jain @ 2021-12-22 1:24 ` Rajat Jain 2022-01-11 16:39 ` Bjorn Helgaas 1 sibling, 0 replies; 4+ messages in thread From: Rajat Jain @ 2021-12-22 1:24 UTC (permalink / raw) To: Bjorn Helgaas, kw, Rajat Jain, Logan Gunthorpe, linux-pci, linux-kernel Cc: rajatxjain Hi Bjorn, Sorry, I forgot to mention... On Tue, Dec 21, 2021 at 5:21 PM Rajat Jain <rajatja@google.com> wrote: > > Some devices have an errata such that they only support DWORD accesses > to some registers. > > For e.g. this Bayhub O2 device ([VID:DID] = [0x1217:0x8621]) only > supports DWORD accesses to LTR latency registers and L1 PM substates > control registers: > https://github.com/rajatxjain/public_shared/blob/main/OZ711LV2_appnote.pdf > > Since L1 PM substate control registers are DWORD sized, and hence their > access in the kernel is already DWORD sized, so we don't need to do > anything for them. > > However, the LTR registers being WORD sized, are in need of a solution. > This patch converts the WORD sized accesses to these registers, into > DWORD sized accesses, while saving and restoring them. > > Signed-off-by: Rajat Jain <rajatja@google.com> ... I have made a more generic proposal here: https://lore.kernel.org/linux-pci/CACK8Z6GR9pR0Oc_G70q2MxTVgrk+PXWTFbWT1EjARnuYCRavLw@mail.gmail.com/T/#t However that is a bigger effort in terms of implementation as well as reaching an agreement and acceptance with the community. So I'd appreciate it if this patch would please be included until a larger framework is put in place. Thanks & Best Regards, Rajat > --- > drivers/pci/pci.c | 24 ++++++++++++++++-------- > drivers/pci/pcie/aspm.c | 1 + > 2 files changed, 17 insertions(+), 8 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 3d2fb394986a..efa8cd16827f 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -1556,7 +1556,7 @@ static void pci_save_ltr_state(struct pci_dev *dev) > { > int ltr; > struct pci_cap_saved_state *save_state; > - u16 *cap; > + u32 *cap; > > if (!pci_is_pcie(dev)) > return; > @@ -1571,25 +1571,33 @@ static void pci_save_ltr_state(struct pci_dev *dev) > return; > } > > - cap = (u16 *)&save_state->cap.data[0]; > - pci_read_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap++); > - pci_read_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, cap++); > + /* > + * We deliberately do a dword access to save both PCI_LTR_MAX_SNOOP_LAT > + * and PCI_LTR_MAX_NOSNOOP_LAT together since some devices only support > + * dword accesses to these registers. > + */ > + cap = &save_state->cap.data[0]; > + pci_read_config_dword(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap); > } > > static void pci_restore_ltr_state(struct pci_dev *dev) > { > struct pci_cap_saved_state *save_state; > int ltr; > - u16 *cap; > + u32 *cap; > > save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR); > ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR); > if (!save_state || !ltr) > return; > > - cap = (u16 *)&save_state->cap.data[0]; > - pci_write_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap++); > - pci_write_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, *cap++); > + /* > + * We deliberately do a dword access to restore both > + * PCI_LTR_MAX_SNOOP_LAT and PCI_LTR_MAX_NOSNOOP_LAT together since > + * some devices only support dword accesses to these registers. > + */ > + cap = &save_state->cap.data[0]; > + pci_write_config_dword(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap); > } > > /** > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 52c74682601a..083f47a7b69b 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -496,6 +496,7 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link, > encode_l12_threshold(l1_2_threshold, &scale, &value); > ctl1 |= t_common_mode << 8 | scale << 29 | value << 16; > > + /* Always make DWORD sized accesses to these registers */ > pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1); > pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, &pctl2); > pci_read_config_dword(child, child->l1ss + PCI_L1SS_CTL1, &cctl1); > -- > 2.34.1.307.g9b7440fafd-goog > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] pci: Make DWORD accesses while saving / restoring LTR state 2021-12-22 1:21 [PATCH] pci: Make DWORD accesses while saving / restoring LTR state Rajat Jain 2021-12-22 1:24 ` Rajat Jain @ 2022-01-11 16:39 ` Bjorn Helgaas 2022-01-11 22:08 ` Rajat Jain 1 sibling, 1 reply; 4+ messages in thread From: Bjorn Helgaas @ 2022-01-11 16:39 UTC (permalink / raw) To: Rajat Jain Cc: Bjorn Helgaas, kw, Logan Gunthorpe, linux-pci, linux-kernel, rajatxjain On Tue, Dec 21, 2021 at 05:21:05PM -0800, Rajat Jain wrote: > Some devices have an errata such that they only support DWORD accesses > to some registers. > > For e.g. this Bayhub O2 device ([VID:DID] = [0x1217:0x8621]) only > supports DWORD accesses to LTR latency registers and L1 PM substates > control registers: > https://github.com/rajatxjain/public_shared/blob/main/OZ711LV2_appnote.pdf > > Since L1 PM substate control registers are DWORD sized, and hence their > access in the kernel is already DWORD sized, so we don't need to do > anything for them. > > However, the LTR registers being WORD sized, are in need of a solution. > This patch converts the WORD sized accesses to these registers, into > DWORD sized accesses, while saving and restoring them. > > Signed-off-by: Rajat Jain <rajatja@google.com> Applied to pci/enumeration for v5.17, thanks, Rajat! The app note suggests that this erratum only affects the registers at 0x234, 0x248, and 0x24c, i.e., the LTR snoop registers and the L1 SS control registers. Can you confirm that is true? Byte and word accesses to other parts of config space work correctly? I *assume* the other parts work correctly, because if byte and word accesses were broken, all sorts of things would not work, like PCI_COMMAND, PCI_STATUS, searching the capability list, etc. Bjorn > --- > drivers/pci/pci.c | 24 ++++++++++++++++-------- > drivers/pci/pcie/aspm.c | 1 + > 2 files changed, 17 insertions(+), 8 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 3d2fb394986a..efa8cd16827f 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -1556,7 +1556,7 @@ static void pci_save_ltr_state(struct pci_dev *dev) > { > int ltr; > struct pci_cap_saved_state *save_state; > - u16 *cap; > + u32 *cap; > > if (!pci_is_pcie(dev)) > return; > @@ -1571,25 +1571,33 @@ static void pci_save_ltr_state(struct pci_dev *dev) > return; > } > > - cap = (u16 *)&save_state->cap.data[0]; > - pci_read_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap++); > - pci_read_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, cap++); > + /* > + * We deliberately do a dword access to save both PCI_LTR_MAX_SNOOP_LAT > + * and PCI_LTR_MAX_NOSNOOP_LAT together since some devices only support > + * dword accesses to these registers. > + */ > + cap = &save_state->cap.data[0]; > + pci_read_config_dword(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap); > } > > static void pci_restore_ltr_state(struct pci_dev *dev) > { > struct pci_cap_saved_state *save_state; > int ltr; > - u16 *cap; > + u32 *cap; > > save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR); > ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR); > if (!save_state || !ltr) > return; > > - cap = (u16 *)&save_state->cap.data[0]; > - pci_write_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap++); > - pci_write_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, *cap++); > + /* > + * We deliberately do a dword access to restore both > + * PCI_LTR_MAX_SNOOP_LAT and PCI_LTR_MAX_NOSNOOP_LAT together since > + * some devices only support dword accesses to these registers. > + */ > + cap = &save_state->cap.data[0]; > + pci_write_config_dword(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap); > } > > /** > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 52c74682601a..083f47a7b69b 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -496,6 +496,7 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link, > encode_l12_threshold(l1_2_threshold, &scale, &value); > ctl1 |= t_common_mode << 8 | scale << 29 | value << 16; > > + /* Always make DWORD sized accesses to these registers */ > pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1); > pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, &pctl2); > pci_read_config_dword(child, child->l1ss + PCI_L1SS_CTL1, &cctl1); > -- > 2.34.1.307.g9b7440fafd-goog > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] pci: Make DWORD accesses while saving / restoring LTR state 2022-01-11 16:39 ` Bjorn Helgaas @ 2022-01-11 22:08 ` Rajat Jain 0 siblings, 0 replies; 4+ messages in thread From: Rajat Jain @ 2022-01-11 22:08 UTC (permalink / raw) To: Bjorn Helgaas Cc: Rajat Jain, Bjorn Helgaas, Krzysztof Wilczyński, Logan Gunthorpe, linux-pci, Linux Kernel Mailing List Hello Bjorn, On Tue, Jan 11, 2022 at 8:39 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Dec 21, 2021 at 05:21:05PM -0800, Rajat Jain wrote: > > Some devices have an errata such that they only support DWORD accesses > > to some registers. > > > > For e.g. this Bayhub O2 device ([VID:DID] = [0x1217:0x8621]) only > > supports DWORD accesses to LTR latency registers and L1 PM substates > > control registers: > > https://github.com/rajatxjain/public_shared/blob/main/OZ711LV2_appnote.pdf > > > > Since L1 PM substate control registers are DWORD sized, and hence their > > access in the kernel is already DWORD sized, so we don't need to do > > anything for them. > > > > However, the LTR registers being WORD sized, are in need of a solution. > > This patch converts the WORD sized accesses to these registers, into > > DWORD sized accesses, while saving and restoring them. > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > Applied to pci/enumeration for v5.17, thanks, Rajat! Thank you. > > The app note suggests that this erratum only affects the registers at > 0x234, 0x248, and 0x24c, i.e., the LTR snoop registers and the L1 SS > control registers. > > Can you confirm that is true? Byte and word accesses to other parts > of config space work correctly? > > I *assume* the other parts work correctly, because if byte and word > accesses were broken, all sorts of things would not work, like > PCI_COMMAND, PCI_STATUS, searching the capability list, etc. Yes, that is correct. The Bayhub SD controller works fine otherwise, so only these registers needed the quirk. Thanks & Best Regards, Rajat > > Bjorn > > > --- > > drivers/pci/pci.c | 24 ++++++++++++++++-------- > > drivers/pci/pcie/aspm.c | 1 + > > 2 files changed, 17 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index 3d2fb394986a..efa8cd16827f 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -1556,7 +1556,7 @@ static void pci_save_ltr_state(struct pci_dev *dev) > > { > > int ltr; > > struct pci_cap_saved_state *save_state; > > - u16 *cap; > > + u32 *cap; > > > > if (!pci_is_pcie(dev)) > > return; > > @@ -1571,25 +1571,33 @@ static void pci_save_ltr_state(struct pci_dev *dev) > > return; > > } > > > > - cap = (u16 *)&save_state->cap.data[0]; > > - pci_read_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap++); > > - pci_read_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, cap++); > > + /* > > + * We deliberately do a dword access to save both PCI_LTR_MAX_SNOOP_LAT > > + * and PCI_LTR_MAX_NOSNOOP_LAT together since some devices only support > > + * dword accesses to these registers. > > + */ > > + cap = &save_state->cap.data[0]; > > + pci_read_config_dword(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap); > > } > > > > static void pci_restore_ltr_state(struct pci_dev *dev) > > { > > struct pci_cap_saved_state *save_state; > > int ltr; > > - u16 *cap; > > + u32 *cap; > > > > save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR); > > ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR); > > if (!save_state || !ltr) > > return; > > > > - cap = (u16 *)&save_state->cap.data[0]; > > - pci_write_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap++); > > - pci_write_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, *cap++); > > + /* > > + * We deliberately do a dword access to restore both > > + * PCI_LTR_MAX_SNOOP_LAT and PCI_LTR_MAX_NOSNOOP_LAT together since > > + * some devices only support dword accesses to these registers. > > + */ > > + cap = &save_state->cap.data[0]; > > + pci_write_config_dword(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap); > > } > > > > /** > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > index 52c74682601a..083f47a7b69b 100644 > > --- a/drivers/pci/pcie/aspm.c > > +++ b/drivers/pci/pcie/aspm.c > > @@ -496,6 +496,7 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link, > > encode_l12_threshold(l1_2_threshold, &scale, &value); > > ctl1 |= t_common_mode << 8 | scale << 29 | value << 16; > > > > + /* Always make DWORD sized accesses to these registers */ > > pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1); > > pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, &pctl2); > > pci_read_config_dword(child, child->l1ss + PCI_L1SS_CTL1, &cctl1); > > -- > > 2.34.1.307.g9b7440fafd-goog > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-01-11 22:09 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-22 1:21 [PATCH] pci: Make DWORD accesses while saving / restoring LTR state Rajat Jain 2021-12-22 1:24 ` Rajat Jain 2022-01-11 16:39 ` Bjorn Helgaas 2022-01-11 22:08 ` Rajat Jain
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).