Hello Bjorn, On Tue, Jan 11, 2022 at 8:39 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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@xxxxxxxxxx> > > 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 > >
