| Message ID | 20231121013400.18367-4-xueshuai@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drivers/perf: add Synopsys DesignWare PCIe PMU driver support | expand |
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
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) +{ + 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); diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 1bf630059264..9dbc4d3c2591 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -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) diff --git a/include/linux/pci.h b/include/linux/pci.h index 8c7c2c3c6c65..72b0fb82a820 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -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);