RE: [PATCH 3/3] PCI: dwc: Add dw_pcie_ep_{read,write}_dbi[2] helpers
Hello Serge, > From: Serge Semin, Sent: Monday, November 13, 2023 9:41 PM > > On Mon, Nov 13, 2023 at 10:33:00AM +0900, Yoshihiro Shimoda wrote: > > The current code calculated some dbi[2] registers' offset by calling > > dw_pcie_ep_get_dbi[2]_offset() in each function. To improve code > > readability, add dw_pcie_ep_{read,write}_dbi[2} and some data-width > > related helpers. > > Thanks for submitting this cleanup patch. That's exactly what I meant > here > and Mani later here > > Please note a few nitpicks below. Thank you for your review! > > > > Signed-off-by: Yoshihiro Shimoda > > --- > > .../pci/controller/dwc/pcie-designware-ep.c | 230 ++ > > 1 file changed, 129 insertions(+), 101 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c > > b/drivers/pci/controller/dwc/pcie-designware-ep.c > > index 1100671db887..dcbed49c9613 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > > @@ -65,24 +65,89 @@ static unsigned int dw_pcie_ep_get_dbi2_offset(struct > > dw_pcie_ep *ep, u8 func_no > > return dbi2_offset; > > } > > > > +static u32 dw_pcie_ep_read_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg, > > + size_t size) > > +{ > > + unsigned int offset = dw_pcie_ep_get_dbi_offset(ep, func_no); > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > + > > + return dw_pcie_read_dbi(pci, offset + reg, size); > > +} > > + > > +static void dw_pcie_ep_write_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 > > reg, > > +size_t size, u32 val) > > +{ > > + unsigned int offset = dw_pcie_ep_get_dbi_offset(ep, func_no); > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > + > > + dw_pcie_write_dbi(pci, offset + reg, size, val); > > +} > > + > > +static void dw_pcie_ep_write_dbi2(struct dw_pcie_ep *ep, u8 func_no, u32 > > reg, > > + size_t size, u32 val) > > +{ > > + unsigned int offset = dw_pcie_ep_get_dbi2_offset(ep, func_no); > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > + > > + dw_pcie_write_dbi2(pci, offset + reg, size, val); > > +} > > + > > +static inline void dw_pcie_ep_writel_dbi(struct dw_pcie_ep *ep, u8 func_no, > > +u32 reg, u32 val) > > +{ > > + dw_pcie_ep_write_dbi(ep, func_no, reg, 0x4, val); > > +} > > + > > +static inline u32 dw_pcie_ep_readl_dbi(struct dw_pcie_ep *ep, u8 func_no, > > + u32 reg) > > +{ > > + return dw_pcie_ep_read_dbi(ep, func_no, reg, 0x4); > > +} > > + > > +static inline void dw_pcie_ep_writew_dbi(struct dw_pcie_ep *ep, u8 func_no, > > +u32 reg, u16 val) > > +{ > > + dw_pcie_ep_write_dbi(ep, func_no, reg, 0x2, val); > > +} > > + > > +static inline u16 dw_pcie_ep_readw_dbi(struct dw_pcie_ep *ep, u8 func_no, > > + u32 reg) > > +{ > > + return dw_pcie_ep_read_dbi(ep, func_no, reg, 0x2); > > +} > > + > > +static inline void dw_pcie_ep_writeb_dbi(struct dw_pcie_ep *ep, u8 func_no, > > +u32 reg, u8 val) > > +{ > > + dw_pcie_ep_write_dbi(ep, func_no, reg, 0x1, val); > > +} > > + > > +static inline u8 dw_pcie_ep_readb_dbi(struct dw_pcie_ep *ep, u8 func_no, > > + u32 reg) > > +{ > > + return dw_pcie_ep_read_dbi(ep, func_no, reg, 0x1); > > +} > > + > > +static inline void dw_pcie_ep_writel_dbi2(struct dw_pcie_ep *ep, u8 > > func_no, > > + u32 reg, u32 val) > > +{ > > + dw_pcie_ep_write_dbi2(ep, func_no, reg, 0x4, val); > > +} > > + > > I am not sure whether the methods above are supposed to be defined > here instead of being moved to the "pcie-designware.h" header file > together with dw_pcie_ep_get_dbi2_offset() and > dw_pcie_ep_get_dbi_offset(). The later place seems more suitable > seeing the accessors are generic, look similar to the > dw_pcie_{write,read}_dbi{,2}() functions and might be useful in the > platform drivers. On the other hand no LLDDs would have used it > currently. So I'll leave this as a food for thoughts for the driver > and subsystem maintainers. Perhaps, when a device driver needs to use these functions actually, we can move these functions to pcie-designware.h, I think. > > static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, u8 func_no, > >enum pci_barno bar, int flags) > > { > > - unsigned int dbi_offset, dbi2_offset; > > struct dw_pcie_ep *ep = &pci->ep; > > u32 reg, reg_dbi2; > > > > - dbi_offset = dw_pcie_ep_get_dbi_offset(ep, func_no); > > - dbi2_offset = dw_pcie_ep_get_dbi2_offset(ep, func_no); > > - > > - reg = dbi_offset + PCI_BASE_ADDRESS_0 + (4 * bar); > > - reg_dbi2 = dbi2_offset + PCI_BASE_ADDRESS_0 + (4 * bar); > > > + reg = PCI_BASE_ADDRESS_0 + (4 * bar); > > + reg_dbi2 = PCI_BASE_ADDRESS_0 + (4 * bar); >
Re: [PATCH 3/3] PCI: dwc: Add dw_pcie_ep_{read,write}_dbi[2] helpers
On Mon, Nov 13, 2023 at 10:33:00AM +0900, Yoshihiro Shimoda wrote: > The current code calculated some dbi[2] registers' offset by calling > dw_pcie_ep_get_dbi[2]_offset() in each function. To improve code > readability, add dw_pcie_ep_{read,write}_dbi[2} and some data-width > related helpers. Thanks for submitting this cleanup patch. That's exactly what I meant here https://lore.kernel.org/linux-pci/j4g4ijnxd7qyacszlwyi3tdztkw2nmnjwyhdqf2l2yj3h2mvje@iqsrqiodqbhq/ and Mani later here https://lore.kernel.org/linux-pci/20230728023444.GA4433@thinkpad/ Please note a few nitpicks below. > > Signed-off-by: Yoshihiro Shimoda > --- > .../pci/controller/dwc/pcie-designware-ep.c | 230 ++ > 1 file changed, 129 insertions(+), 101 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c > b/drivers/pci/controller/dwc/pcie-designware-ep.c > index 1100671db887..dcbed49c9613 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > @@ -65,24 +65,89 @@ static unsigned int dw_pcie_ep_get_dbi2_offset(struct > dw_pcie_ep *ep, u8 func_no > return dbi2_offset; > } > > +static u32 dw_pcie_ep_read_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg, > +size_t size) > +{ > + unsigned int offset = dw_pcie_ep_get_dbi_offset(ep, func_no); > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > + > + return dw_pcie_read_dbi(pci, offset + reg, size); > +} > + > +static void dw_pcie_ep_write_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg, > + size_t size, u32 val) > +{ > + unsigned int offset = dw_pcie_ep_get_dbi_offset(ep, func_no); > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > + > + dw_pcie_write_dbi(pci, offset + reg, size, val); > +} > + > +static void dw_pcie_ep_write_dbi2(struct dw_pcie_ep *ep, u8 func_no, u32 reg, > + size_t size, u32 val) > +{ > + unsigned int offset = dw_pcie_ep_get_dbi2_offset(ep, func_no); > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > + > + dw_pcie_write_dbi2(pci, offset + reg, size, val); > +} > + > +static inline void dw_pcie_ep_writel_dbi(struct dw_pcie_ep *ep, u8 func_no, > + u32 reg, u32 val) > +{ > + dw_pcie_ep_write_dbi(ep, func_no, reg, 0x4, val); > +} > + > +static inline u32 dw_pcie_ep_readl_dbi(struct dw_pcie_ep *ep, u8 func_no, > +u32 reg) > +{ > + return dw_pcie_ep_read_dbi(ep, func_no, reg, 0x4); > +} > + > +static inline void dw_pcie_ep_writew_dbi(struct dw_pcie_ep *ep, u8 func_no, > + u32 reg, u16 val) > +{ > + dw_pcie_ep_write_dbi(ep, func_no, reg, 0x2, val); > +} > + > +static inline u16 dw_pcie_ep_readw_dbi(struct dw_pcie_ep *ep, u8 func_no, > +u32 reg) > +{ > + return dw_pcie_ep_read_dbi(ep, func_no, reg, 0x2); > +} > + > +static inline void dw_pcie_ep_writeb_dbi(struct dw_pcie_ep *ep, u8 func_no, > + u32 reg, u8 val) > +{ > + dw_pcie_ep_write_dbi(ep, func_no, reg, 0x1, val); > +} > + > +static inline u8 dw_pcie_ep_readb_dbi(struct dw_pcie_ep *ep, u8 func_no, > + u32 reg) > +{ > + return dw_pcie_ep_read_dbi(ep, func_no, reg, 0x1); > +} > + > +static inline void dw_pcie_ep_writel_dbi2(struct dw_pcie_ep *ep, u8 func_no, > + u32 reg, u32 val) > +{ > + dw_pcie_ep_write_dbi2(ep, func_no, reg, 0x4, val); > +} > + I am not sure whether the methods above are supposed to be defined here instead of being moved to the "pcie-designware.h" header file together with dw_pcie_ep_get_dbi2_offset() and dw_pcie_ep_get_dbi_offset(). The later place seems more suitable seeing the accessors are generic, look similar to the dw_pcie_{write,read}_dbi{,2}() functions and might be useful in the platform drivers. On the other hand no LLDDs would have used it currently. So I'll leave this as a food for thoughts for the driver and subsystem maintainers. > static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, u8 func_no, > enum pci_barno bar, int flags) > { > - unsigned int dbi_offset, dbi2_offset; > struct dw_pcie_ep *ep = &pci->ep; > u32 reg, reg_dbi2; > > - dbi_offset = dw_pcie_ep_get_dbi_offset(ep, func_no); > - dbi2_offset = dw_pcie_ep_get_dbi2_offset(ep, func_no); > - > - reg = dbi_offset + PCI_BASE_ADDRESS_0 + (4 * bar); > - reg_dbi2 = dbi2_offset + PCI_BASE_ADDRESS_0 + (4 * bar); > + reg = PCI_BASE_ADDRESS_0 + (4 * bar); > + reg_dbi2 = PCI_BASE_ADDRESS_0 + (4 * bar); Semantics of the both variables is identical, could you please drop "reg_dbi2" and just use the "reg" variable instead here? You must have just missed it because a similar change is done in the rest of the places in this patch. > dw_pcie_dbi_
[PATCH 3/3] PCI: dwc: Add dw_pcie_ep_{read,write}_dbi[2] helpers
The current code calculated some dbi[2] registers' offset by calling dw_pcie_ep_get_dbi[2]_offset() in each function. To improve code readability, add dw_pcie_ep_{read,write}_dbi[2} and some data-width related helpers. Signed-off-by: Yoshihiro Shimoda --- .../pci/controller/dwc/pcie-designware-ep.c | 230 ++ 1 file changed, 129 insertions(+), 101 deletions(-) diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c index 1100671db887..dcbed49c9613 100644 --- a/drivers/pci/controller/dwc/pcie-designware-ep.c +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c @@ -65,24 +65,89 @@ static unsigned int dw_pcie_ep_get_dbi2_offset(struct dw_pcie_ep *ep, u8 func_no return dbi2_offset; } +static u32 dw_pcie_ep_read_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg, + size_t size) +{ + unsigned int offset = dw_pcie_ep_get_dbi_offset(ep, func_no); + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); + + return dw_pcie_read_dbi(pci, offset + reg, size); +} + +static void dw_pcie_ep_write_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg, +size_t size, u32 val) +{ + unsigned int offset = dw_pcie_ep_get_dbi_offset(ep, func_no); + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); + + dw_pcie_write_dbi(pci, offset + reg, size, val); +} + +static void dw_pcie_ep_write_dbi2(struct dw_pcie_ep *ep, u8 func_no, u32 reg, + size_t size, u32 val) +{ + unsigned int offset = dw_pcie_ep_get_dbi2_offset(ep, func_no); + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); + + dw_pcie_write_dbi2(pci, offset + reg, size, val); +} + +static inline void dw_pcie_ep_writel_dbi(struct dw_pcie_ep *ep, u8 func_no, +u32 reg, u32 val) +{ + dw_pcie_ep_write_dbi(ep, func_no, reg, 0x4, val); +} + +static inline u32 dw_pcie_ep_readl_dbi(struct dw_pcie_ep *ep, u8 func_no, + u32 reg) +{ + return dw_pcie_ep_read_dbi(ep, func_no, reg, 0x4); +} + +static inline void dw_pcie_ep_writew_dbi(struct dw_pcie_ep *ep, u8 func_no, +u32 reg, u16 val) +{ + dw_pcie_ep_write_dbi(ep, func_no, reg, 0x2, val); +} + +static inline u16 dw_pcie_ep_readw_dbi(struct dw_pcie_ep *ep, u8 func_no, + u32 reg) +{ + return dw_pcie_ep_read_dbi(ep, func_no, reg, 0x2); +} + +static inline void dw_pcie_ep_writeb_dbi(struct dw_pcie_ep *ep, u8 func_no, +u32 reg, u8 val) +{ + dw_pcie_ep_write_dbi(ep, func_no, reg, 0x1, val); +} + +static inline u8 dw_pcie_ep_readb_dbi(struct dw_pcie_ep *ep, u8 func_no, + u32 reg) +{ + return dw_pcie_ep_read_dbi(ep, func_no, reg, 0x1); +} + +static inline void dw_pcie_ep_writel_dbi2(struct dw_pcie_ep *ep, u8 func_no, + u32 reg, u32 val) +{ + dw_pcie_ep_write_dbi2(ep, func_no, reg, 0x4, val); +} + static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, u8 func_no, enum pci_barno bar, int flags) { - unsigned int dbi_offset, dbi2_offset; struct dw_pcie_ep *ep = &pci->ep; u32 reg, reg_dbi2; - dbi_offset = dw_pcie_ep_get_dbi_offset(ep, func_no); - dbi2_offset = dw_pcie_ep_get_dbi2_offset(ep, func_no); - - reg = dbi_offset + PCI_BASE_ADDRESS_0 + (4 * bar); - reg_dbi2 = dbi2_offset + PCI_BASE_ADDRESS_0 + (4 * bar); + reg = PCI_BASE_ADDRESS_0 + (4 * bar); + reg_dbi2 = PCI_BASE_ADDRESS_0 + (4 * bar); dw_pcie_dbi_ro_wr_en(pci); - dw_pcie_writel_dbi2(pci, reg_dbi2, 0x0); - dw_pcie_writel_dbi(pci, reg, 0x0); + dw_pcie_ep_writel_dbi2(ep, func_no, reg_dbi2, 0x0); + dw_pcie_ep_writel_dbi(ep, func_no, reg, 0x0); if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) { - dw_pcie_writel_dbi2(pci, reg_dbi2 + 4, 0x0); - dw_pcie_writel_dbi(pci, reg + 4, 0x0); + dw_pcie_ep_writel_dbi2(ep, func_no, reg_dbi2 + 4, 0x0); + dw_pcie_ep_writel_dbi(ep, func_no, reg + 4, 0x0); } dw_pcie_dbi_ro_wr_dis(pci); } @@ -99,19 +164,15 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar) EXPORT_SYMBOL_GPL(dw_pcie_ep_reset_bar); static u8 __dw_pcie_ep_find_next_cap(struct dw_pcie_ep *ep, u8 func_no, - u8 cap_ptr, u8 cap) +u8 cap_ptr, u8 cap) { - struct dw_pcie *pci = to_dw_pcie_from_ep(ep); - unsigned int dbi_offset = 0; u8 cap_id, next_cap_ptr; u16 reg; if (!cap_ptr) return 0; - dbi_offset = dw_pcie_ep_get_dbi_offset(ep, func_no); - - reg = dw_pcie_readw_dbi(pci, dbi_offset + cap_ptr); + reg = dw_pcie_ep_readw_dbi(ep, func_no, cap_ptr);