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 = >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 = >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.
>
