[v11,3/5] PCI: Move pci_clear_and_set_dword() helper to PCI header
Commit Message
The clear and set pattern is commonly used for accessing PCI config,
move the helper pci_clear_and_set_dword() from aspm.c into PCI header.
In addition, rename to pci_clear_and_set_config_dword() to retain the
"config" information and match the other accessors.
No functional change intended.
Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Tested-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
---
drivers/pci/access.c | 12 ++++++++
drivers/pci/pcie/aspm.c | 65 +++++++++++++++++++----------------------
include/linux/pci.h | 2 ++
3 files changed, 44 insertions(+), 35 deletions(-)
Comments
On Tue, 21 Nov 2023, Shuai Xue wrote:
> The clear and set pattern is commonly used for accessing PCI config,
> move the helper pci_clear_and_set_dword() from aspm.c into PCI header.
> In addition, rename to pci_clear_and_set_config_dword() to retain the
> "config" information and match the other accessors.
>
> No functional change intended.
>
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> Tested-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> ---
> drivers/pci/access.c | 12 ++++++++
> drivers/pci/pcie/aspm.c | 65 +++++++++++++++++++----------------------
> include/linux/pci.h | 2 ++
> 3 files changed, 44 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 6554a2e89d36..6449056b57dd 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -598,3 +598,15 @@ int pci_write_config_dword(const struct pci_dev *dev, int where,
> return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val);
> }
> EXPORT_SYMBOL(pci_write_config_dword);
> +
> +void pci_clear_and_set_config_dword(const struct pci_dev *dev, int pos,
> + u32 clear, u32 set)
Just noting that annoyingly the ordering within the name is inconsistent
between:
pci_clear_and_set_config_dword()
and
pcie_capability_clear_and_set_dword()
And if changed, it would be again annoyingly inconsistent with
pci_read/write_config_*(), oh well... And renaming pci_read/write_config_*
into the hierarchical pci_config_read/write_*() form for would touch only
~6k lines... ;-D
> + pci_clear_and_set_config_dword(child,
> + child->l1ss + PCI_L1SS_CTL1, 0,
> + cl1_2_enables);
Adding clear and set only variants into the header like there are for
pcie_capability_*() would remove the need to add those 0 parameters.
IMO, it improves code readability considerably.
+ Bjorn for PCI.
On 2023/11/22 21:14, Ilpo Järvinen wrote:
Hi, Ilpo,
Thank you for your reply. Please see my comments inline.
> On Tue, 21 Nov 2023, Shuai Xue wrote:
>
>> The clear and set pattern is commonly used for accessing PCI config,
>> move the helper pci_clear_and_set_dword() from aspm.c into PCI header.
>> In addition, rename to pci_clear_and_set_config_dword() to retain the
>> "config" information and match the other accessors.
>>
>> No functional change intended.
>>
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>> Tested-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>> ---
>> drivers/pci/access.c | 12 ++++++++
>> drivers/pci/pcie/aspm.c | 65 +++++++++++++++++++----------------------
>> include/linux/pci.h | 2 ++
>> 3 files changed, 44 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
>> index 6554a2e89d36..6449056b57dd 100644
>> --- a/drivers/pci/access.c
>> +++ b/drivers/pci/access.c
>> @@ -598,3 +598,15 @@ int pci_write_config_dword(const struct pci_dev *dev, int where,
>> return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val);
>> }
>> EXPORT_SYMBOL(pci_write_config_dword);
>> +
>> +void pci_clear_and_set_config_dword(const struct pci_dev *dev, int pos,
>> + u32 clear, u32 set)
>
> Just noting that annoyingly the ordering within the name is inconsistent
> between:
> pci_clear_and_set_config_dword()
> and
> pcie_capability_clear_and_set_dword()
>
> And if changed, it would be again annoyingly inconsistent with
> pci_read/write_config_*(), oh well... And renaming pci_read/write_config_*
> into the hierarchical pci_config_read/write_*() form for would touch only
> ~6k lines... ;-D
I think it is a good question, but I don't have a clear answer. I don't
know much about the name history. As you mentioned, the above two
accessors are the foundation operation, may it comes to @Bjorn decision.
The pci_clear_and_set_config_dword() is a variant of bellow pci accessors:
pci_read_config_dword()
pci_write_config_dword()
At last, they are consistend :)
>
>> + pci_clear_and_set_config_dword(child,
>> + child->l1ss + PCI_L1SS_CTL1, 0,
>> + cl1_2_enables);
>
> Adding clear and set only variants into the header like there are for
> pcie_capability_*() would remove the need to add those 0 parameters.
> IMO, it improves code readability considerably.
>
Agreed.
Best Regards,
Shuai
On Mon, Nov 27, 2023 at 09:34:05AM +0800, Shuai Xue wrote:
> On 2023/11/22 21:14, Ilpo Järvinen wrote:
> > On Tue, 21 Nov 2023, Shuai Xue wrote:
> >
> >> The clear and set pattern is commonly used for accessing PCI config,
> >> move the helper pci_clear_and_set_dword() from aspm.c into PCI header.
> >> In addition, rename to pci_clear_and_set_config_dword() to retain the
> >> "config" information and match the other accessors.
> >>
> >> No functional change intended.
> >>
> >> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> >> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> >> Tested-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> >> ---
> >> drivers/pci/access.c | 12 ++++++++
> >> drivers/pci/pcie/aspm.c | 65 +++++++++++++++++++----------------------
> >> include/linux/pci.h | 2 ++
> >> 3 files changed, 44 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> >> index 6554a2e89d36..6449056b57dd 100644
> >> --- a/drivers/pci/access.c
> >> +++ b/drivers/pci/access.c
> >> @@ -598,3 +598,15 @@ int pci_write_config_dword(const struct pci_dev *dev, int where,
> >> return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val);
> >> }
> >> EXPORT_SYMBOL(pci_write_config_dword);
> >> +
> >> +void pci_clear_and_set_config_dword(const struct pci_dev *dev, int pos,
> >> + u32 clear, u32 set)
> >
> > Just noting that annoyingly the ordering within the name is inconsistent
> > between:
> > pci_clear_and_set_config_dword()
> > and
> > pcie_capability_clear_and_set_dword()
> >
> > And if changed, it would be again annoyingly inconsistent with
> > pci_read/write_config_*(), oh well... And renaming pci_read/write_config_*
> > into the hierarchical pci_config_read/write_*() form for would touch only
> > ~6k lines... ;-D
>
> I think it is a good question, but I don't have a clear answer. I don't
> know much about the name history. As you mentioned, the above two
> accessors are the foundation operation, may it comes to @Bjorn decision.
>
> The pci_clear_and_set_config_dword() is a variant of below pci accessors:
>
> pci_read_config_dword()
> pci_write_config_dword()
>
> At last, they are consistent :)
"pcie_capability_clear_and_set_dword" is specific to the PCIe
Capability, doesn't work for arbitrary config space, and doesn't
include the word "config".
"pci_clear_and_set_config_dword" seems consistent with the arbitrary
config space accessor pattern.
At least "clear_and_set" is consistent across both.
I'm not too bothered by the difference between "clear_and_set_dword"
(for the PCIe capability) and "clear_and_set_config_dword" (for
arbitrary things).
Yes, "pcie_capability_clear_and_set_config_dword" would be a little
more consistent, but seems excessively wordy (no pun intended).
But maybe I'm missing your point, Ilpo. If so, what would you
propose?
Bjorn
On Wed, 29 Nov 2023, Bjorn Helgaas wrote:
> On Mon, Nov 27, 2023 at 09:34:05AM +0800, Shuai Xue wrote:
> > On 2023/11/22 21:14, Ilpo Järvinen wrote:
> > > On Tue, 21 Nov 2023, Shuai Xue wrote:
> > >
> > >> The clear and set pattern is commonly used for accessing PCI config,
> > >> move the helper pci_clear_and_set_dword() from aspm.c into PCI header.
> > >> In addition, rename to pci_clear_and_set_config_dword() to retain the
> > >> "config" information and match the other accessors.
> > >>
> > >> No functional change intended.
> > >> +
> > >> +void pci_clear_and_set_config_dword(const struct pci_dev *dev, int pos,
> > >> + u32 clear, u32 set)
> > >
> > > Just noting that annoyingly the ordering within the name is inconsistent
> > > between:
> > > pci_clear_and_set_config_dword()
> > > and
> > > pcie_capability_clear_and_set_dword()
> > >
> > > And if changed, it would be again annoyingly inconsistent with
> > > pci_read/write_config_*(), oh well... And renaming pci_read/write_config_*
> > > into the hierarchical pci_config_read/write_*() form for would touch only
> > > ~6k lines... ;-D
> >
> > I think it is a good question, but I don't have a clear answer. I don't
> > know much about the name history. As you mentioned, the above two
> > accessors are the foundation operation, may it comes to @Bjorn decision.
> >
> > The pci_clear_and_set_config_dword() is a variant of below pci accessors:
> >
> > pci_read_config_dword()
> > pci_write_config_dword()
> >
> > At last, they are consistent :)
>
> "pcie_capability_clear_and_set_dword" is specific to the PCIe
> Capability, doesn't work for arbitrary config space, and doesn't
> include the word "config".
>
> "pci_clear_and_set_config_dword" seems consistent with the arbitrary
> config space accessor pattern.
>
> At least "clear_and_set" is consistent across both.
>
> I'm not too bothered by the difference between "clear_and_set_dword"
> (for the PCIe capability) and "clear_and_set_config_dword" (for
> arbitrary things).
>
> Yes, "pcie_capability_clear_and_set_config_dword" would be a little
> more consistent, but seems excessively wordy (no pun intended).
>
> But maybe I'm missing your point, Ilpo. If so, what would you
> propose?
What I was hoping for a way to (eventually) have consistency in naming
like this (that is, the place where "config" or "capabilitity" appears
in the name):
pci_config_read_dword()
pci_config_clear_and_set_dword()
pcie_capability_read_dword()
pcie_capability_clear_and_set_dword()
(+ the omitted clear/set/write & size variants)
But thanks to pci_read_config_dword() & friends being there since dawn of
time and with 6k+ instances, I guess I'm just dreaming of impossible
things.
On Thu, Nov 30, 2023 at 12:52:20PM +0200, Ilpo Järvinen wrote:
> On Wed, 29 Nov 2023, Bjorn Helgaas wrote:
> > On Mon, Nov 27, 2023 at 09:34:05AM +0800, Shuai Xue wrote:
> > > On 2023/11/22 21:14, Ilpo Järvinen wrote:
> > > > On Tue, 21 Nov 2023, Shuai Xue wrote:
> > > >
> > > >> The clear and set pattern is commonly used for accessing PCI config,
> > > >> move the helper pci_clear_and_set_dword() from aspm.c into PCI header.
> > > >> In addition, rename to pci_clear_and_set_config_dword() to retain the
> > > >> "config" information and match the other accessors.
> > > >>
> > > >> No functional change intended.
>
> > > >> +
> > > >> +void pci_clear_and_set_config_dword(const struct pci_dev *dev, int pos,
> > > >> + u32 clear, u32 set)
> > > >
> > > > Just noting that annoyingly the ordering within the name is inconsistent
> > > > between:
> > > > pci_clear_and_set_config_dword()
> > > > and
> > > > pcie_capability_clear_and_set_dword()
> > > >
> > > > And if changed, it would be again annoyingly inconsistent with
> > > > pci_read/write_config_*(), oh well... And renaming pci_read/write_config_*
> > > > into the hierarchical pci_config_read/write_*() form for would touch only
> > > > ~6k lines... ;-D
> > >
> > > I think it is a good question, but I don't have a clear answer. I don't
> > > know much about the name history. As you mentioned, the above two
> > > accessors are the foundation operation, may it comes to @Bjorn decision.
> > >
> > > The pci_clear_and_set_config_dword() is a variant of below pci accessors:
> > >
> > > pci_read_config_dword()
> > > pci_write_config_dword()
> > >
> > > At last, they are consistent :)
> >
> > "pcie_capability_clear_and_set_dword" is specific to the PCIe
> > Capability, doesn't work for arbitrary config space, and doesn't
> > include the word "config".
> >
> > "pci_clear_and_set_config_dword" seems consistent with the arbitrary
> > config space accessor pattern.
> >
> > At least "clear_and_set" is consistent across both.
> >
> > I'm not too bothered by the difference between "clear_and_set_dword"
> > (for the PCIe capability) and "clear_and_set_config_dword" (for
> > arbitrary things).
> >
> > Yes, "pcie_capability_clear_and_set_config_dword" would be a little
> > more consistent, but seems excessively wordy (no pun intended).
> >
> > But maybe I'm missing your point, Ilpo. If so, what would you
> > propose?
>
> What I was hoping for a way to (eventually) have consistency in naming
> like this (that is, the place where "config" or "capabilitity" appears
> in the name):
>
> pci_config_read_dword()
> pci_config_clear_and_set_dword()
> pcie_capability_read_dword()
> pcie_capability_clear_and_set_dword()
Ah, I see, thanks.
> But thanks to pci_read_config_dword() & friends being there since dawn of
> time and with 6k+ instances, I guess I'm just dreaming of impossible
> things.
Yeah, I think so.
Bjorn
@@ -598,3 +598,15 @@ int pci_write_config_dword(const struct pci_dev *dev, int where,
return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val);
}
EXPORT_SYMBOL(pci_write_config_dword);
+
+void pci_clear_and_set_config_dword(const struct pci_dev *dev, int pos,
+ u32 clear, u32 set)
+{
+ u32 val;
+
+ pci_read_config_dword(dev, pos, &val);
+ val &= ~clear;
+ val |= set;
+ pci_write_config_dword(dev, pos, val);
+}
+EXPORT_SYMBOL(pci_clear_and_set_config_dword);
@@ -423,17 +423,6 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
}
}
-static void pci_clear_and_set_dword(struct pci_dev *pdev, int pos,
- u32 clear, u32 set)
-{
- u32 val;
-
- pci_read_config_dword(pdev, pos, &val);
- val &= ~clear;
- val |= set;
- pci_write_config_dword(pdev, pos, val);
-}
-
/* Calculate L1.2 PM substate timing parameters */
static void aspm_calc_l12_info(struct pcie_link_state *link,
u32 parent_l1ss_cap, u32 child_l1ss_cap)
@@ -494,10 +483,12 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
cl1_2_enables = cctl1 & PCI_L1SS_CTL1_L1_2_MASK;
if (pl1_2_enables || cl1_2_enables) {
- pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
- PCI_L1SS_CTL1_L1_2_MASK, 0);
- pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
- PCI_L1SS_CTL1_L1_2_MASK, 0);
+ pci_clear_and_set_config_dword(child,
+ child->l1ss + PCI_L1SS_CTL1,
+ PCI_L1SS_CTL1_L1_2_MASK, 0);
+ pci_clear_and_set_config_dword(parent,
+ parent->l1ss + PCI_L1SS_CTL1,
+ PCI_L1SS_CTL1_L1_2_MASK, 0);
}
/* Program T_POWER_ON times in both ports */
@@ -505,22 +496,26 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
pci_write_config_dword(child, child->l1ss + PCI_L1SS_CTL2, ctl2);
/* Program Common_Mode_Restore_Time in upstream device */
- pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
- PCI_L1SS_CTL1_CM_RESTORE_TIME, ctl1);
+ pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
+ PCI_L1SS_CTL1_CM_RESTORE_TIME, ctl1);
/* Program LTR_L1.2_THRESHOLD time in both ports */
- pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
- PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
- PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);
- pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
- PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
- PCI_L1SS_CTL1_LTR_L12_TH_SCALE, ctl1);
+ pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
+ PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+ PCI_L1SS_CTL1_LTR_L12_TH_SCALE,
+ ctl1);
+ pci_clear_and_set_config_dword(child, child->l1ss + PCI_L1SS_CTL1,
+ PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+ PCI_L1SS_CTL1_LTR_L12_TH_SCALE,
+ ctl1);
if (pl1_2_enables || cl1_2_enables) {
- pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1, 0,
- pl1_2_enables);
- pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1, 0,
- cl1_2_enables);
+ pci_clear_and_set_config_dword(parent,
+ parent->l1ss + PCI_L1SS_CTL1, 0,
+ pl1_2_enables);
+ pci_clear_and_set_config_dword(child,
+ child->l1ss + PCI_L1SS_CTL1, 0,
+ cl1_2_enables);
}
}
@@ -680,10 +675,10 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
*/
/* Disable all L1 substates */
- pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
- PCI_L1SS_CTL1_L1SS_MASK, 0);
- pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
- PCI_L1SS_CTL1_L1SS_MASK, 0);
+ pci_clear_and_set_config_dword(child, child->l1ss + PCI_L1SS_CTL1,
+ PCI_L1SS_CTL1_L1SS_MASK, 0);
+ pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
+ PCI_L1SS_CTL1_L1SS_MASK, 0);
/*
* If needed, disable L1, and it gets enabled later
* in pcie_config_aspm_link().
@@ -706,10 +701,10 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
val |= PCI_L1SS_CTL1_PCIPM_L1_2;
/* Enable what we need to enable */
- pci_clear_and_set_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
- PCI_L1SS_CTL1_L1SS_MASK, val);
- pci_clear_and_set_dword(child, child->l1ss + PCI_L1SS_CTL1,
- PCI_L1SS_CTL1_L1SS_MASK, val);
+ pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
+ PCI_L1SS_CTL1_L1SS_MASK, val);
+ pci_clear_and_set_config_dword(child, child->l1ss + PCI_L1SS_CTL1,
+ PCI_L1SS_CTL1_L1SS_MASK, val);
}
static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
@@ -1213,6 +1213,8 @@ int pci_read_config_dword(const struct pci_dev *dev, int where, u32 *val);
int pci_write_config_byte(const struct pci_dev *dev, int where, u8 val);
int pci_write_config_word(const struct pci_dev *dev, int where, u16 val);
int pci_write_config_dword(const struct pci_dev *dev, int where, u32 val);
+void pci_clear_and_set_config_dword(const struct pci_dev *dev, int pos,
+ u32 clear, u32 set);
int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val);
int pcie_capability_read_dword(struct pci_dev *dev, int pos, u32 *val);