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 <[email protected]>
---
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
Hi Bjorn,
Sorry, I forgot to mention...
On Tue, Dec 21, 2021 at 5:21 PM Rajat Jain <[email protected]> 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 <[email protected]>
... 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
>
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 <[email protected]>
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
>
Hello Bjorn,
On Tue, Jan 11, 2022 at 8:39 AM Bjorn Helgaas <[email protected]> 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 <[email protected]>
>
> 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
> >