[v18,04/20] PCI: dwc: Change arguments of dw_pcie_prog_outbound_atu()

Commit Message

Yoshihiro Shimoda July 21, 2023, 7:44 a.m. UTC

The __dw_pcie_prog_outbound_atu() currently has 6 arguments.
To support INTx IRQs in the future, it requires an additional 2
arguments. For improved code readability, introduce the struct
dw_pcie_ob_atu_cfg and update the arguments of
dw_pcie_prog_outbound_atu().

Consequently, remove __dw_pcie_prog_outbound_atu() and
dw_pcie_prog_ep_outbound_atu() because there is no longer
a need.

No behavior changes.

Signed-off-by: Yoshihiro Shimoda <[e-mail]>
Reviewed-by: Serge Semin <[e-mail]>
---
 .../pci/controller/dwc/pcie-designware-ep.c   | 21 +++++---
 .../pci/controller/dwc/pcie-designware-host.c | 52 +++++++++++++------
 drivers/pci/controller/dwc/pcie-designware.c  | 49 ++++++-----------
 drivers/pci/controller/dwc/pcie-designware.h  | 15 ++++--
 4 files changed, 77 insertions(+), 60 deletions(-)

Comments

Manivannan Sadhasivam July 24, 2023, 7:45 a.m. UTC | #1

On Fri, Jul 21, 2023 at 04:44:36PM +0900, Yoshihiro Shimoda wrote:
> The __dw_pcie_prog_outbound_atu() currently has 6 arguments.
> To support INTx IRQs in the future, it requires an additional 2
> arguments. For improved code readability, introduce the struct
> dw_pcie_ob_atu_cfg and update the arguments of
> dw_pcie_prog_outbound_atu().
> 
> Consequently, remove __dw_pcie_prog_outbound_atu() and
> dw_pcie_prog_ep_outbound_atu() because there is no longer
> a need.
> 
> No behavior changes.
> 
> Signed-off-by: Yoshihiro Shimoda <[e-mail]>

One nit below. With that,

Reviewed-by: Manivannan Sadhasivam <[e-mail]>

> Reviewed-by: Serge Semin <[e-mail]>
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 21 +++++---
>  .../pci/controller/dwc/pcie-designware-host.c | 52 +++++++++++++------
>  drivers/pci/controller/dwc/pcie-designware.c  | 49 ++++++-----------
>  drivers/pci/controller/dwc/pcie-designware.h  | 15 ++++--
>  4 files changed, 77 insertions(+), 60 deletions(-)
> 

[...]

> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 3c06e025c905..85de0d8346fa 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -288,6 +288,15 @@ enum dw_pcie_core_rst {
>  	DW_PCIE_NUM_CORE_RSTS
>  };
>  
> +struct dw_pcie_ob_atu_cfg {
> +	int index;
> +	int type;
> +	u8 func_no;
> +	u64 cpu_addr;
> +	u64 pci_addr;
> +	u64 size;

Reorder the members in below order to avoid holes:

u64
int
u8

- Mani

> +};
> +
>  struct dw_pcie_host_ops {
>  	int (*host_init)(struct dw_pcie_rp *pp);
>  	void (*host_deinit)(struct dw_pcie_rp *pp);
> @@ -416,10 +425,8 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
>  int dw_pcie_link_up(struct dw_pcie *pci);
>  void dw_pcie_upconfig_setup(struct dw_pcie *pci);
>  int dw_pcie_wait_for_link(struct dw_pcie *pci);
> -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> -			      u64 cpu_addr, u64 pci_addr, u64 size);
> -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> -				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
> +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> +			      const struct dw_pcie_ob_atu_cfg *atu);
>  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
>  			     u64 cpu_addr, u64 pci_addr, u64 size);
>  int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> -- 
> 2.25.1
>

Serge Semin July 26, 2023, 5:02 a.m. UTC | #2

On Mon, Jul 24, 2023 at 01:15:56PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Jul 21, 2023 at 04:44:36PM +0900, Yoshihiro Shimoda wrote:
> > The __dw_pcie_prog_outbound_atu() currently has 6 arguments.
> > To support INTx IRQs in the future, it requires an additional 2
> > arguments. For improved code readability, introduce the struct
> > dw_pcie_ob_atu_cfg and update the arguments of
> > dw_pcie_prog_outbound_atu().
> > 
> > Consequently, remove __dw_pcie_prog_outbound_atu() and
> > dw_pcie_prog_ep_outbound_atu() because there is no longer
> > a need.
> > 
> > No behavior changes.
> > 
> > Signed-off-by: Yoshihiro Shimoda <[e-mail]>
> 
> One nit below. With that,
> 
> Reviewed-by: Manivannan Sadhasivam <[e-mail]>
> 
> > Reviewed-by: Serge Semin <[e-mail]>
> > ---
> >  .../pci/controller/dwc/pcie-designware-ep.c   | 21 +++++---
> >  .../pci/controller/dwc/pcie-designware-host.c | 52 +++++++++++++------
> >  drivers/pci/controller/dwc/pcie-designware.c  | 49 ++++++-----------
> >  drivers/pci/controller/dwc/pcie-designware.h  | 15 ++++--
> >  4 files changed, 77 insertions(+), 60 deletions(-)
> > 
> 
> [...]
> 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 3c06e025c905..85de0d8346fa 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -288,6 +288,15 @@ enum dw_pcie_core_rst {
> >  	DW_PCIE_NUM_CORE_RSTS
> >  };
> >  
> > +struct dw_pcie_ob_atu_cfg {
> > +	int index;
> > +	int type;
> > +	u8 func_no;
> > +	u64 cpu_addr;
> > +	u64 pci_addr;
> > +	u64 size;
> 

> Reorder the members in below order to avoid holes:
> 
> u64
> int
> u8

One more time. Your suggestion won't prevent the compiler from adding
the pads. (If by "holes" you meant the padding. Otherwise please
elaborate what you meant?). The structure will have the same size of
40 bytes in both cases. So your suggestion will just worsen the
structure readability from having a more natural parameters order (MW
index, type, function, and then the mapping parameters) to a redundant
type-based order.

-Serge(y)

> 
> - Mani
> 
> > +};
> > +
> >  struct dw_pcie_host_ops {
> >  	int (*host_init)(struct dw_pcie_rp *pp);
> >  	void (*host_deinit)(struct dw_pcie_rp *pp);
> > @@ -416,10 +425,8 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
> >  int dw_pcie_link_up(struct dw_pcie *pci);
> >  void dw_pcie_upconfig_setup(struct dw_pcie *pci);
> >  int dw_pcie_wait_for_link(struct dw_pcie *pci);
> > -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > -			      u64 cpu_addr, u64 pci_addr, u64 size);
> > -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > -				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
> > +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> > +			      const struct dw_pcie_ob_atu_cfg *atu);
> >  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> >  			     u64 cpu_addr, u64 pci_addr, u64 size);
> >  int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > -- 
> > 2.25.1
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

Manivannan Sadhasivam July 26, 2023, 1 p.m. UTC | #3

On Wed, Jul 26, 2023 at 08:02:24AM +0300, Serge Semin wrote:
> On Mon, Jul 24, 2023 at 01:15:56PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Jul 21, 2023 at 04:44:36PM +0900, Yoshihiro Shimoda wrote:
> > > The __dw_pcie_prog_outbound_atu() currently has 6 arguments.
> > > To support INTx IRQs in the future, it requires an additional 2
> > > arguments. For improved code readability, introduce the struct
> > > dw_pcie_ob_atu_cfg and update the arguments of
> > > dw_pcie_prog_outbound_atu().
> > > 
> > > Consequently, remove __dw_pcie_prog_outbound_atu() and
> > > dw_pcie_prog_ep_outbound_atu() because there is no longer
> > > a need.
> > > 
> > > No behavior changes.
> > > 
> > > Signed-off-by: Yoshihiro Shimoda <[e-mail]>
> > 
> > One nit below. With that,
> > 
> > Reviewed-by: Manivannan Sadhasivam <[e-mail]>
> > 
> > > Reviewed-by: Serge Semin <[e-mail]>
> > > ---
> > >  .../pci/controller/dwc/pcie-designware-ep.c   | 21 +++++---
> > >  .../pci/controller/dwc/pcie-designware-host.c | 52 +++++++++++++------
> > >  drivers/pci/controller/dwc/pcie-designware.c  | 49 ++++++-----------
> > >  drivers/pci/controller/dwc/pcie-designware.h  | 15 ++++--
> > >  4 files changed, 77 insertions(+), 60 deletions(-)
> > > 
> > 
> > [...]
> > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > index 3c06e025c905..85de0d8346fa 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > @@ -288,6 +288,15 @@ enum dw_pcie_core_rst {
> > >  	DW_PCIE_NUM_CORE_RSTS
> > >  };
> > >  
> > > +struct dw_pcie_ob_atu_cfg {
> > > +	int index;
> > > +	int type;
> > > +	u8 func_no;
> > > +	u64 cpu_addr;
> > > +	u64 pci_addr;
> > > +	u64 size;
> > 
> 
> > Reorder the members in below order to avoid holes:
> > 
> > u64
> > int
> > u8
> 
> One more time. Your suggestion won't prevent the compiler from adding
> the pads. (If by "holes" you meant the padding. Otherwise please
> elaborate what you meant?).

Struct padding is often referred as struct holes. So yes, I'm referring the
same.

> The structure will have the same size of
> 40 bytes in both cases. So your suggestion will just worsen the
> structure readability from having a more natural parameters order (MW
> index, type, function, and then the mapping parameters) to a redundant
> type-based order.
> 

This is a common comment I provide for all structures. Even though the current
result (reordering) doesn't save any space, when the structure grows big (who
knows), we often see more holes/padding being inserted by the compiler if the
members are not ordered in the descending order w.r.t their size.

I agree that it makes more clear if the members are grouped based on their
function etc... but for large structures this would often add more padding/hole.

- Mani

> -Serge(y)
> 
> > 
> > - Mani
> > 
> > > +};
> > > +
> > >  struct dw_pcie_host_ops {
> > >  	int (*host_init)(struct dw_pcie_rp *pp);
> > >  	void (*host_deinit)(struct dw_pcie_rp *pp);
> > > @@ -416,10 +425,8 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
> > >  int dw_pcie_link_up(struct dw_pcie *pci);
> > >  void dw_pcie_upconfig_setup(struct dw_pcie *pci);
> > >  int dw_pcie_wait_for_link(struct dw_pcie *pci);
> > > -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > > -			      u64 cpu_addr, u64 pci_addr, u64 size);
> > > -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > -				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
> > > +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> > > +			      const struct dw_pcie_ob_atu_cfg *atu);
> > >  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> > >  			     u64 cpu_addr, u64 pci_addr, u64 size);
> > >  int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > -- 
> > > 2.25.1
> > > 
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்

Serge Semin July 26, 2023, 11:38 p.m. UTC | #4

On Wed, Jul 26, 2023 at 06:30:15PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jul 26, 2023 at 08:02:24AM +0300, Serge Semin wrote:
> > On Mon, Jul 24, 2023 at 01:15:56PM +0530, Manivannan Sadhasivam wrote:
> > > On Fri, Jul 21, 2023 at 04:44:36PM +0900, Yoshihiro Shimoda wrote:
> > > > The __dw_pcie_prog_outbound_atu() currently has 6 arguments.
> > > > To support INTx IRQs in the future, it requires an additional 2
> > > > arguments. For improved code readability, introduce the struct
> > > > dw_pcie_ob_atu_cfg and update the arguments of
> > > > dw_pcie_prog_outbound_atu().
> > > > 
> > > > Consequently, remove __dw_pcie_prog_outbound_atu() and
> > > > dw_pcie_prog_ep_outbound_atu() because there is no longer
> > > > a need.
> > > > 
> > > > No behavior changes.
> > > > 
> > > > Signed-off-by: Yoshihiro Shimoda <[e-mail]>
> > > 
> > > One nit below. With that,
> > > 
> > > Reviewed-by: Manivannan Sadhasivam <[e-mail]>
> > > 
> > > > Reviewed-by: Serge Semin <[e-mail]>
> > > > ---
> > > >  .../pci/controller/dwc/pcie-designware-ep.c   | 21 +++++---
> > > >  .../pci/controller/dwc/pcie-designware-host.c | 52 +++++++++++++------
> > > >  drivers/pci/controller/dwc/pcie-designware.c  | 49 ++++++-----------
> > > >  drivers/pci/controller/dwc/pcie-designware.h  | 15 ++++--
> > > >  4 files changed, 77 insertions(+), 60 deletions(-)
> > > > 
> > > 
> > > [...]
> > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > > index 3c06e025c905..85de0d8346fa 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > > @@ -288,6 +288,15 @@ enum dw_pcie_core_rst {
> > > >  	DW_PCIE_NUM_CORE_RSTS
> > > >  };
> > > >  
> > > > +struct dw_pcie_ob_atu_cfg {
> > > > +	int index;
> > > > +	int type;
> > > > +	u8 func_no;
> > > > +	u64 cpu_addr;
> > > > +	u64 pci_addr;
> > > > +	u64 size;
> > > 
> > 
> > > Reorder the members in below order to avoid holes:
> > > 
> > > u64
> > > int
> > > u8
> > 
> > One more time. Your suggestion won't prevent the compiler from adding
> > the pads. (If by "holes" you meant the padding. Otherwise please
> > elaborate what you meant?).
> 
> Struct padding is often referred as struct holes. So yes, I'm referring the
> same.
> 
> > The structure will have the same size of
> > 40 bytes in both cases. So your suggestion will just worsen the
> > structure readability from having a more natural parameters order (MW
> > index, type, function, and then the mapping parameters) to a redundant
> > type-based order.
> > 
> 

> This is a common comment I provide for all structures. Even though the current
> result (reordering) doesn't save any space, when the structure grows big (who
> knows), we often see more holes/padding being inserted by the compiler if the
> members are not ordered in the descending order w.r.t their size.
> 
> I agree that it makes more clear if the members are grouped based on their
> function etc... but for large structures this would often add more padding/hole.

This structure will never be big enough to be considered for such
strange optimization. Moreover practicality almost always beats some
theoretical considerations. In this case there is no any reason to
reorder the fields as you say.

Speaking in general I very much doubt that saving a few bytes of
memory can be considered as a better option than having a more
readable structure especially these days. Moreover for all these years
I never met anybody asking to set the descending order of
the members or maintaining such limitation in the commonly used kernel
structures. What is normally done:
1. Move an embedded object to the head of the structure for the
container_of-macro optimization.
2. Group up the commonly used fields to optimize the system cache
utilization.
3. Logical grouping the members, which naturally may lead to the more
optimal cache utilization.
4. Move a field to a certain place of the structure to fill in the
pads.

Even if the "descending alignment" requirement minimizes the number of
the pads it isn't the only possible way to do so in the particular
cases and it looks too harsh to be blindly applied all the time. If a
few bytes is so important why not do the same for instance for the
local variables too? They are also normally size-aligned in the stack
memory, which is much more precious in kernel.

Anyway in this case changing the fields order is absolutely redundant.
Even a provided afterwards update doesn't cause the structure size
change. So for the sake of readability it's better to leave its fields
ordered as is.

-Serge(y)

> 
> - Mani
> 
> > -Serge(y)
> > 
> > > 
> > > - Mani
> > > 
> > > > +};
> > > > +
> > > >  struct dw_pcie_host_ops {
> > > >  	int (*host_init)(struct dw_pcie_rp *pp);
> > > >  	void (*host_deinit)(struct dw_pcie_rp *pp);
> > > > @@ -416,10 +425,8 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
> > > >  int dw_pcie_link_up(struct dw_pcie *pci);
> > > >  void dw_pcie_upconfig_setup(struct dw_pcie *pci);
> > > >  int dw_pcie_wait_for_link(struct dw_pcie *pci);
> > > > -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > > > -			      u64 cpu_addr, u64 pci_addr, u64 size);
> > > > -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > -				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
> > > > +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> > > > +			      const struct dw_pcie_ob_atu_cfg *atu);
> > > >  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> > > >  			     u64 cpu_addr, u64 pci_addr, u64 size);
> > > >  int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > -- 
> > > > 2.25.1
> > > > 
> > > 
> > > -- 
> > > மணிவண்ணன் சதாசிவம்
> 
> -- 
> மணிவண்ணன் சதாசிவம்

Yoshihiro Shimoda July 27, 2023, 1:06 a.m. UTC | #5

Hi Serge,

> From: Serge Semin, Sent: Thursday, July 27, 2023 8:39 AM
> On Wed, Jul 26, 2023 at 06:30:15PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Jul 26, 2023 at 08:02:24AM +0300, Serge Semin wrote:
> > > On Mon, Jul 24, 2023 at 01:15:56PM +0530, Manivannan Sadhasivam wrote:
> > > > On Fri, Jul 21, 2023 at 04:44:36PM +0900, Yoshihiro Shimoda wrote:
> > > > > The __dw_pcie_prog_outbound_atu() currently has 6 arguments.
> > > > > To support INTx IRQs in the future, it requires an additional 2
> > > > > arguments. For improved code readability, introduce the struct
> > > > > dw_pcie_ob_atu_cfg and update the arguments of
> > > > > dw_pcie_prog_outbound_atu().
> > > > >
> > > > > Consequently, remove __dw_pcie_prog_outbound_atu() and
> > > > > dw_pcie_prog_ep_outbound_atu() because there is no longer
> > > > > a need.
> > > > >
> > > > > No behavior changes.
> > > > >
> > > > > Signed-off-by: Yoshihiro Shimoda <[e-mail]>
> > > >
> > > > One nit below. With that,
> > > >
> > > > Reviewed-by: Manivannan Sadhasivam <[e-mail]>
> > > >
> > > > > Reviewed-by: Serge Semin <[e-mail]>
> > > > > ---
> > > > >  .../pci/controller/dwc/pcie-designware-ep.c   | 21 +++++---
> > > > >  .../pci/controller/dwc/pcie-designware-host.c | 52 +++++++++++++------
> > > > >  drivers/pci/controller/dwc/pcie-designware.c  | 49 ++++++-----------
> > > > >  drivers/pci/controller/dwc/pcie-designware.h  | 15 ++++--
> > > > >  4 files changed, 77 insertions(+), 60 deletions(-)
> > > > >
> > > >
> > > > [...]
> > > >
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > > > index 3c06e025c905..85de0d8346fa 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > > > @@ -288,6 +288,15 @@ enum dw_pcie_core_rst {
> > > > >  	DW_PCIE_NUM_CORE_RSTS
> > > > >  };
> > > > >
> > > > > +struct dw_pcie_ob_atu_cfg {
> > > > > +	int index;
> > > > > +	int type;
> > > > > +	u8 func_no;
> > > > > +	u64 cpu_addr;
> > > > > +	u64 pci_addr;
> > > > > +	u64 size;
> > > >
> > >
> > > > Reorder the members in below order to avoid holes:
> > > >
> > > > u64
> > > > int
> > > > u8
> > >
> > > One more time. Your suggestion won't prevent the compiler from adding
> > > the pads. (If by "holes" you meant the padding. Otherwise please
> > > elaborate what you meant?).
> >
> > Struct padding is often referred as struct holes. So yes, I'm referring the
> > same.
> >
> > > The structure will have the same size of
> > > 40 bytes in both cases. So your suggestion will just worsen the
> > > structure readability from having a more natural parameters order (MW
> > > index, type, function, and then the mapping parameters) to a redundant
> > > type-based order.
> > >
> >
> 
> > This is a common comment I provide for all structures. Even though the current
> > result (reordering) doesn't save any space, when the structure grows big (who
> > knows), we often see more holes/padding being inserted by the compiler if the
> > members are not ordered in the descending order w.r.t their size.
> >
> > I agree that it makes more clear if the members are grouped based on their
> > function etc... but for large structures this would often add more padding/hole.
> 
> This structure will never be big enough to be considered for such
> strange optimization. Moreover practicality almost always beats some
> theoretical considerations. In this case there is no any reason to
> reorder the fields as you say.
> 
> Speaking in general I very much doubt that saving a few bytes of
> memory can be considered as a better option than having a more
> readable structure especially these days. Moreover for all these years
> I never met anybody asking to set the descending order of
> the members or maintaining such limitation in the commonly used kernel
> structures. What is normally done:
> 1. Move an embedded object to the head of the structure for the
> container_of-macro optimization.
> 2. Group up the commonly used fields to optimize the system cache
> utilization.
> 3. Logical grouping the members, which naturally may lead to the more
> optimal cache utilization.
> 4. Move a field to a certain place of the structure to fill in the
> pads.
> 
> Even if the "descending alignment" requirement minimizes the number of
> the pads it isn't the only possible way to do so in the particular
> cases and it looks too harsh to be blindly applied all the time. If a
> few bytes is so important why not do the same for instance for the
> local variables too? They are also normally size-aligned in the stack
> memory, which is much more precious in kernel.

I found some patches to save memory by avoiding padding/hole in 2023:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=86c6bb0edffa9fc02b4e3801b48c8e82114f1352
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=48cd6bc5b22d68b8bbc8601f3c7ddeed99541a0b

> Anyway in this case changing the fields order is absolutely redundant.
> Even a provided afterwards update doesn't cause the structure size
> change. So for the sake of readability it's better to leave its fields
> ordered as is.

I should have asked you before you suggested this ordering [1], but
I don't know why the current ordering is good readability.

-----
struct dw_pcie_ob_atu_cfg {
        int index;
        int type;
        u8 func_no;
        u64 cpu_addr;
        u64 pci_addr;
        u64 size;
};
-----

The ordering of struct dw_pcie_ob_atu_cfg seems to related to the arguments
of original functions which are dw_pcie_prog_{ep}_outbound_atu().

-----
int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
                             u64 cpu_addr, u64 pci_addr, u64 size);
int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
                                int type, u64 cpu_addr, u64 pci_addr, u64 size);
-----

About the patch, I relied on the arguments order in the code like below:
-----
-               ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
-                                               pp->io_base, pp->io_bus_addr,
-                                               pp->io_size);
+               atu.type = PCIE_ATU_TYPE_IO;
+               atu.cpu_addr = pp->io_base;
+               atu.pci_addr = pp->io_bus_addr;
+               atu.size = pp->io_size;
-----

For reviewing the patch, I believe this is good ordering. However,
about ordering of the struct dw_pcie_ob_atu_cfg members, I think
that both ordering is the same readability. Perhaps, we should add
comments in the struct like below?
-----
struct dw_pcie_ob_atu_cfg {
        /* The following members are required on both host and endpoint */
        u64 cpu_addr;
        u64 pci_addr;
        u64 size;
        int index;
        int type;

        /* The following member is required on endpoint */
        u8 func_no;
};
-----
# Each "The following member(s) is/are" can be dropped?

And then, we add new members like below:
-----
struct dw_pcie_ob_atu_cfg {
        /* The following members are required on both host and endpoint */
        u64 cpu_addr;
        u64 pci_addr;
        u64 size;
        int index;
        int type;

        /* The following member is required on endpoint */
        u8 func_no;

        /* The following members are optional for endpoint */
        u8 routing;
        u8 code;
};
-----

There is the good ordering for padding/hole unexpectedly :)
But, what do you think?

Best regards,
Yoshihiro Shimoda

> -Serge(y)
> 
> >
> > - Mani
> >
> > > -Serge(y)
> > >
> > > >
> > > > - Mani
> > > >
> > > > > +};
> > > > > +
> > > > >  struct dw_pcie_host_ops {
> > > > >  	int (*host_init)(struct dw_pcie_rp *pp);
> > > > >  	void (*host_deinit)(struct dw_pcie_rp *pp);
> > > > > @@ -416,10 +425,8 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
> > > > >  int dw_pcie_link_up(struct dw_pcie *pci);
> > > > >  void dw_pcie_upconfig_setup(struct dw_pcie *pci);
> > > > >  int dw_pcie_wait_for_link(struct dw_pcie *pci);
> > > > > -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > > > > -			      u64 cpu_addr, u64 pci_addr, u64 size);
> > > > > -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > > -				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
> > > > > +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> > > > > +			      const struct dw_pcie_ob_atu_cfg *atu);
> > > > >  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> > > > >  			     u64 cpu_addr, u64 pci_addr, u64 size);
> > > > >  int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > > --
> > > > > 2.25.1
> > > > >
> > > >
> > > > --
> > > > மணிவண்ணன் சதாசிவம்
> >
> > --
> > மணிவண்ணன் சதாசிவம்

Manivannan Sadhasivam July 27, 2023, 11:03 a.m. UTC | #6

On Thu, Jul 27, 2023 at 02:38:44AM +0300, Serge Semin wrote:
> On Wed, Jul 26, 2023 at 06:30:15PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Jul 26, 2023 at 08:02:24AM +0300, Serge Semin wrote:
> > > On Mon, Jul 24, 2023 at 01:15:56PM +0530, Manivannan Sadhasivam wrote:
> > > > On Fri, Jul 21, 2023 at 04:44:36PM +0900, Yoshihiro Shimoda wrote:
> > > > > The __dw_pcie_prog_outbound_atu() currently has 6 arguments.
> > > > > To support INTx IRQs in the future, it requires an additional 2
> > > > > arguments. For improved code readability, introduce the struct
> > > > > dw_pcie_ob_atu_cfg and update the arguments of
> > > > > dw_pcie_prog_outbound_atu().
> > > > > 
> > > > > Consequently, remove __dw_pcie_prog_outbound_atu() and
> > > > > dw_pcie_prog_ep_outbound_atu() because there is no longer
> > > > > a need.
> > > > > 
> > > > > No behavior changes.
> > > > > 
> > > > > Signed-off-by: Yoshihiro Shimoda <[e-mail]>
> > > > 
> > > > One nit below. With that,
> > > > 
> > > > Reviewed-by: Manivannan Sadhasivam <[e-mail]>
> > > > 
> > > > > Reviewed-by: Serge Semin <[e-mail]>
> > > > > ---
> > > > >  .../pci/controller/dwc/pcie-designware-ep.c   | 21 +++++---
> > > > >  .../pci/controller/dwc/pcie-designware-host.c | 52 +++++++++++++------
> > > > >  drivers/pci/controller/dwc/pcie-designware.c  | 49 ++++++-----------
> > > > >  drivers/pci/controller/dwc/pcie-designware.h  | 15 ++++--
> > > > >  4 files changed, 77 insertions(+), 60 deletions(-)
> > > > > 
> > > > 
> > > > [...]
> > > > 
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > > > index 3c06e025c905..85de0d8346fa 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > > > @@ -288,6 +288,15 @@ enum dw_pcie_core_rst {
> > > > >  	DW_PCIE_NUM_CORE_RSTS
> > > > >  };
> > > > >  
> > > > > +struct dw_pcie_ob_atu_cfg {
> > > > > +	int index;
> > > > > +	int type;
> > > > > +	u8 func_no;
> > > > > +	u64 cpu_addr;
> > > > > +	u64 pci_addr;
> > > > > +	u64 size;
> > > > 
> > > 
> > > > Reorder the members in below order to avoid holes:
> > > > 
> > > > u64
> > > > int
> > > > u8
> > > 
> > > One more time. Your suggestion won't prevent the compiler from adding
> > > the pads. (If by "holes" you meant the padding. Otherwise please
> > > elaborate what you meant?).
> > 
> > Struct padding is often referred as struct holes. So yes, I'm referring the
> > same.
> > 
> > > The structure will have the same size of
> > > 40 bytes in both cases. So your suggestion will just worsen the
> > > structure readability from having a more natural parameters order (MW
> > > index, type, function, and then the mapping parameters) to a redundant
> > > type-based order.
> > > 
> > 
> 
> > This is a common comment I provide for all structures. Even though the current
> > result (reordering) doesn't save any space, when the structure grows big (who
> > knows), we often see more holes/padding being inserted by the compiler if the
> > members are not ordered in the descending order w.r.t their size.
> > 
> > I agree that it makes more clear if the members are grouped based on their
> > function etc... but for large structures this would often add more padding/hole.
> 
> This structure will never be big enough to be considered for such
> strange optimization. Moreover practicality almost always beats some
> theoretical considerations. In this case there is no any reason to
> reorder the fields as you say.
> 
> Speaking in general I very much doubt that saving a few bytes of
> memory can be considered as a better option than having a more
> readable structure especially these days. Moreover for all these years
> I never met anybody asking to set the descending order of
> the members or maintaining such limitation in the commonly used kernel
> structures. What is normally done:
> 1. Move an embedded object to the head of the structure for the
> container_of-macro optimization.
> 2. Group up the commonly used fields to optimize the system cache
> utilization.
> 3. Logical grouping the members, which naturally may lead to the more
> optimal cache utilization.

Indeed.

> 4. Move a field to a certain place of the structure to fill in the
> pads.
> 

This is what I try to avoid by grouping the members. If you move a field to
a certain place, wouldn't it affect readability?

But I do not want to argue more on this. Please see below.

> Even if the "descending alignment" requirement minimizes the number of
> the pads it isn't the only possible way to do so in the particular
> cases and it looks too harsh to be blindly applied all the time. If a
> few bytes is so important why not do the same for instance for the
> local variables too? They are also normally size-aligned in the stack
> memory, which is much more precious in kernel.
> 

Well, for local variables I prefer reverse Xmas tree order which is what widely
used throughout the kernel. But we do not care about their ordering because, it
won't grow too much like a structure (not talking about recursive case).

> Anyway in this case changing the fields order is absolutely redundant.
> Even a provided afterwards update doesn't cause the structure size
> change. So for the sake of readability it's better to leave its fields
> ordered as is.
> 

I certainly agree that reordering wouldn't save any space for this structure.
As a maintainer, I prefer to keep this pattern so that I don't have to worry
about the padding issues in the future and hence the suggestion.

But feel free to drop it as I don't have a strong objection to this specific
case.

- Mani

> -Serge(y)
> 
> > 
> > - Mani
> > 
> > > -Serge(y)
> > > 
> > > > 
> > > > - Mani
> > > > 
> > > > > +};
> > > > > +
> > > > >  struct dw_pcie_host_ops {
> > > > >  	int (*host_init)(struct dw_pcie_rp *pp);
> > > > >  	void (*host_deinit)(struct dw_pcie_rp *pp);
> > > > > @@ -416,10 +425,8 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
> > > > >  int dw_pcie_link_up(struct dw_pcie *pci);
> > > > >  void dw_pcie_upconfig_setup(struct dw_pcie *pci);
> > > > >  int dw_pcie_wait_for_link(struct dw_pcie *pci);
> > > > > -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > > > > -			      u64 cpu_addr, u64 pci_addr, u64 size);
> > > > > -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > > -				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
> > > > > +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> > > > > +			      const struct dw_pcie_ob_atu_cfg *atu);
> > > > >  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> > > > >  			     u64 cpu_addr, u64 pci_addr, u64 size);
> > > > >  int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > > -- 
> > > > > 2.25.1
> > > > > 
> > > > 
> > > > -- 
> > > > மணிவண்ணன் சதாசிவம்
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்

Serge Semin July 27, 2023, 12:21 p.m. UTC | #7

On Thu, Jul 27, 2023 at 04:33:43PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Jul 27, 2023 at 02:38:44AM +0300, Serge Semin wrote:
> > On Wed, Jul 26, 2023 at 06:30:15PM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, Jul 26, 2023 at 08:02:24AM +0300, Serge Semin wrote:
> > > > On Mon, Jul 24, 2023 at 01:15:56PM +0530, Manivannan Sadhasivam wrote:
> > > > > On Fri, Jul 21, 2023 at 04:44:36PM +0900, Yoshihiro Shimoda wrote:
> > > > > > The __dw_pcie_prog_outbound_atu() currently has 6 arguments.
> > > > > > To support INTx IRQs in the future, it requires an additional 2
> > > > > > arguments. For improved code readability, introduce the struct
> > > > > > dw_pcie_ob_atu_cfg and update the arguments of
> > > > > > dw_pcie_prog_outbound_atu().
> > > > > > 
> > > > > > Consequently, remove __dw_pcie_prog_outbound_atu() and
> > > > > > dw_pcie_prog_ep_outbound_atu() because there is no longer
> > > > > > a need.
> > > > > > 
> > > > > > No behavior changes.
> > > > > > 
> > > > > > Signed-off-by: Yoshihiro Shimoda <[e-mail]>
> > > > > 
> > > > > One nit below. With that,
> > > > > 
> > > > > Reviewed-by: Manivannan Sadhasivam <[e-mail]>
> > > > > 
> > > > > > Reviewed-by: Serge Semin <[e-mail]>
> > > > > > ---
> > > > > >  .../pci/controller/dwc/pcie-designware-ep.c   | 21 +++++---
> > > > > >  .../pci/controller/dwc/pcie-designware-host.c | 52 +++++++++++++------
> > > > > >  drivers/pci/controller/dwc/pcie-designware.c  | 49 ++++++-----------
> > > > > >  drivers/pci/controller/dwc/pcie-designware.h  | 15 ++++--
> > > > > >  4 files changed, 77 insertions(+), 60 deletions(-)
> > > > > > 
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > > > > index 3c06e025c905..85de0d8346fa 100644
> > > > > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > > > > @@ -288,6 +288,15 @@ enum dw_pcie_core_rst {
> > > > > >  	DW_PCIE_NUM_CORE_RSTS
> > > > > >  };
> > > > > >  
> > > > > > +struct dw_pcie_ob_atu_cfg {
> > > > > > +	int index;
> > > > > > +	int type;
> > > > > > +	u8 func_no;
> > > > > > +	u64 cpu_addr;
> > > > > > +	u64 pci_addr;
> > > > > > +	u64 size;
> > > > > 
> > > > 
> > > > > Reorder the members in below order to avoid holes:
> > > > > 
> > > > > u64
> > > > > int
> > > > > u8
> > > > 
> > > > One more time. Your suggestion won't prevent the compiler from adding
> > > > the pads. (If by "holes" you meant the padding. Otherwise please
> > > > elaborate what you meant?).
> > > 
> > > Struct padding is often referred as struct holes. So yes, I'm referring the
> > > same.
> > > 
> > > > The structure will have the same size of
> > > > 40 bytes in both cases. So your suggestion will just worsen the
> > > > structure readability from having a more natural parameters order (MW
> > > > index, type, function, and then the mapping parameters) to a redundant
> > > > type-based order.
> > > > 
> > > 
> > 
> > > This is a common comment I provide for all structures. Even though the current
> > > result (reordering) doesn't save any space, when the structure grows big (who
> > > knows), we often see more holes/padding being inserted by the compiler if the
> > > members are not ordered in the descending order w.r.t their size.
> > > 
> > > I agree that it makes more clear if the members are grouped based on their
> > > function etc... but for large structures this would often add more padding/hole.
> > 
> > This structure will never be big enough to be considered for such
> > strange optimization. Moreover practicality almost always beats some
> > theoretical considerations. In this case there is no any reason to
> > reorder the fields as you say.
> > 
> > Speaking in general I very much doubt that saving a few bytes of
> > memory can be considered as a better option than having a more
> > readable structure especially these days. Moreover for all these years
> > I never met anybody asking to set the descending order of
> > the members or maintaining such limitation in the commonly used kernel
> > structures. What is normally done:
> > 1. Move an embedded object to the head of the structure for the
> > container_of-macro optimization.
> > 2. Group up the commonly used fields to optimize the system cache
> > utilization.
> > 3. Logical grouping the members, which naturally may lead to the more
> > optimal cache utilization.
> 
> Indeed.
> 
> > 4. Move a field to a certain place of the structure to fill in the
> > pads.
> > 
> 
> This is what I try to avoid by grouping the members. If you move a field to
> a certain place, wouldn't it affect readability?
> 
> But I do not want to argue more on this. Please see below.
> 
> > Even if the "descending alignment" requirement minimizes the number of
> > the pads it isn't the only possible way to do so in the particular
> > cases and it looks too harsh to be blindly applied all the time. If a
> > few bytes is so important why not do the same for instance for the
> > local variables too? They are also normally size-aligned in the stack
> > memory, which is much more precious in kernel.
> > 
> 
> Well, for local variables I prefer reverse Xmas tree order which is what widely
> used throughout the kernel. But we do not care about their ordering because, it
> won't grow too much like a structure (not talking about recursive case).
> 
> > Anyway in this case changing the fields order is absolutely redundant.
> > Even a provided afterwards update doesn't cause the structure size
> > change. So for the sake of readability it's better to leave its fields
> > ordered as is.
> > 
> 
> I certainly agree that reordering wouldn't save any space for this structure.
> As a maintainer, I prefer to keep this pattern so that I don't have to worry
> about the padding issues in the future and hence the suggestion.
> 
> But feel free to drop it as I don't have a strong objection to this specific
> case.

Agreed then.

Yoshihiro, could you please ignore the Mani' comment regarding the
"descending alignment" order and retain the fields order is in your
current patch?

I'll get back to the series review tomorrow. Please no rush with
resubmitting.

-Serge(y)

> 
> - Mani
> 
> > -Serge(y)
> > 
> > > 
> > > - Mani
> > > 
> > > > -Serge(y)
> > > > 
> > > > > 
> > > > > - Mani
> > > > > 
> > > > > > +};
> > > > > > +
> > > > > >  struct dw_pcie_host_ops {
> > > > > >  	int (*host_init)(struct dw_pcie_rp *pp);
> > > > > >  	void (*host_deinit)(struct dw_pcie_rp *pp);
> > > > > > @@ -416,10 +425,8 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
> > > > > >  int dw_pcie_link_up(struct dw_pcie *pci);
> > > > > >  void dw_pcie_upconfig_setup(struct dw_pcie *pci);
> > > > > >  int dw_pcie_wait_for_link(struct dw_pcie *pci);
> > > > > > -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > > > > > -			      u64 cpu_addr, u64 pci_addr, u64 size);
> > > > > > -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > > > -				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
> > > > > > +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> > > > > > +			      const struct dw_pcie_ob_atu_cfg *atu);
> > > > > >  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> > > > > >  			     u64 cpu_addr, u64 pci_addr, u64 size);
> > > > > >  int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > > > -- 
> > > > > > 2.25.1
> > > > > > 
> > > > > 
> > > > > -- 
> > > > > மணிவண்ணன் சதாசிவம்
> > > 
> > > -- 
> > > மணிவண்ணன் சதாசிவம்
> 
> -- 
> மணிவண்ணன் சதாசிவம்

Serge Semin July 29, 2023, 2:06 a.m. UTC | #8

On Fri, Jul 21, 2023 at 04:44:36PM +0900, Yoshihiro Shimoda wrote:
> The __dw_pcie_prog_outbound_atu() currently has 6 arguments.
> To support INTx IRQs in the future, it requires an additional 2
> arguments. For improved code readability, introduce the struct
> dw_pcie_ob_atu_cfg and update the arguments of
> dw_pcie_prog_outbound_atu().
> 
> Consequently, remove __dw_pcie_prog_outbound_atu() and
> dw_pcie_prog_ep_outbound_atu() because there is no longer
> a need.
> 
> No behavior changes.

So you decided not to use a suggested by me in v17 more detailed patch
log? C&P it here just in case if you change your mind:

This is a preparation before adding the Msg-type outbound iATU
mapping. The respective update will require two more arguments added
to __dw_pcie_prog_outbound_atu(). That will make the already
complicated function prototype even more hard to comprehend accepting
_eight_ arguments. In order to prevent that and keep the code
more-or-less readable all the outbound iATU-related arguments are
moved to the new config-structure: struct dw_pcie_ob_atu_cfg pointer
to which shall be passed to dw_pcie_prog_outbound_atu(). The structure
is supposed to be locally defined and populated with the outbound iATU
settings implied by the caller context.

As a result of the denoted change there is no longer need in having
the two distinctive methods for the Host and End-point outbound iATU
setups since the corresponding code can directly call the
dw_pcie_prog_outbound_atu() method with the config-structure
populated. Thus dw_pcie_prog_ep_outbound_atu() is dropped.

-Serge(y)

> 
> Signed-off-by: Yoshihiro Shimoda <[e-mail]>
> Reviewed-by: Serge Semin <[e-mail]>
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 21 +++++---
>  .../pci/controller/dwc/pcie-designware-host.c | 52 +++++++++++++------
>  drivers/pci/controller/dwc/pcie-designware.c  | 49 ++++++-----------
>  drivers/pci/controller/dwc/pcie-designware.h  | 15 ++++--
>  4 files changed, 77 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 27278010ecec..fe2e0d765be9 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -182,9 +182,8 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
>  	return 0;
>  }
>  
> -static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
> -				   phys_addr_t phys_addr,
> -				   u64 pci_addr, size_t size)
> +static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep,
> +				   struct dw_pcie_ob_atu_cfg *atu)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>  	u32 free_win;
> @@ -196,13 +195,13 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
>  		return -EINVAL;
>  	}
>  
> -	ret = dw_pcie_prog_ep_outbound_atu(pci, func_no, free_win, PCIE_ATU_TYPE_MEM,
> -					   phys_addr, pci_addr, size);
> +	atu->index = free_win;
> +	ret = dw_pcie_prog_outbound_atu(pci, atu);
>  	if (ret)
>  		return ret;
>  
>  	set_bit(free_win, ep->ob_window_map);
> -	ep->outbound_addr[free_win] = phys_addr;
> +	ep->outbound_addr[free_win] = atu->cpu_addr;
>  
>  	return 0;
>  }
> @@ -305,8 +304,14 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  	int ret;
>  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> -
> -	ret = dw_pcie_ep_outbound_atu(ep, func_no, addr, pci_addr, size);
> +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> +
> +	atu.func_no = func_no;
> +	atu.type = PCIE_ATU_TYPE_MEM;
> +	atu.cpu_addr = addr;
> +	atu.pci_addr = pci_addr;
> +	atu.size = size;
> +	ret = dw_pcie_ep_outbound_atu(ep, &atu);
>  	if (ret) {
>  		dev_err(pci->dev, "Failed to enable address\n");
>  		return ret;
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index cf61733bf78d..7419185721f2 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -549,6 +549,7 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
>  {
>  	struct dw_pcie_rp *pp = bus->sysdata;
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct dw_pcie_ob_atu_cfg atu = { 0 };
>  	int type, ret;
>  	u32 busdev;
>  
> @@ -571,8 +572,12 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
>  	else
>  		type = PCIE_ATU_TYPE_CFG1;
>  
> -	ret = dw_pcie_prog_outbound_atu(pci, 0, type, pp->cfg0_base, busdev,
> -					pp->cfg0_size);
> +	atu.type = type;
> +	atu.cpu_addr = pp->cfg0_base;
> +	atu.pci_addr = busdev;
> +	atu.size = pp->cfg0_size;
> +
> +	ret = dw_pcie_prog_outbound_atu(pci, &atu);
>  	if (ret)
>  		return NULL;
>  
> @@ -584,6 +589,7 @@ static int dw_pcie_rd_other_conf(struct pci_bus *bus, unsigned int devfn,
>  {
>  	struct dw_pcie_rp *pp = bus->sysdata;
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct dw_pcie_ob_atu_cfg atu = { 0 };
>  	int ret;
>  
>  	ret = pci_generic_config_read(bus, devfn, where, size, val);
> @@ -591,9 +597,12 @@ static int dw_pcie_rd_other_conf(struct pci_bus *bus, unsigned int devfn,
>  		return ret;
>  
>  	if (pp->cfg0_io_shared) {
> -		ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
> -						pp->io_base, pp->io_bus_addr,
> -						pp->io_size);
> +		atu.type = PCIE_ATU_TYPE_IO;
> +		atu.cpu_addr = pp->io_base;
> +		atu.pci_addr = pp->io_bus_addr;
> +		atu.size = pp->io_size;
> +
> +		ret = dw_pcie_prog_outbound_atu(pci, &atu);
>  		if (ret)
>  			return PCIBIOS_SET_FAILED;
>  	}
> @@ -606,6 +615,7 @@ static int dw_pcie_wr_other_conf(struct pci_bus *bus, unsigned int devfn,
>  {
>  	struct dw_pcie_rp *pp = bus->sysdata;
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct dw_pcie_ob_atu_cfg atu = { 0 };
>  	int ret;
>  
>  	ret = pci_generic_config_write(bus, devfn, where, size, val);
> @@ -613,9 +623,12 @@ static int dw_pcie_wr_other_conf(struct pci_bus *bus, unsigned int devfn,
>  		return ret;
>  
>  	if (pp->cfg0_io_shared) {
> -		ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
> -						pp->io_base, pp->io_bus_addr,
> -						pp->io_size);
> +		atu.type = PCIE_ATU_TYPE_IO;
> +		atu.cpu_addr = pp->io_base;
> +		atu.pci_addr = pp->io_bus_addr;
> +		atu.size = pp->io_size;
> +
> +		ret = dw_pcie_prog_outbound_atu(pci, &atu);
>  		if (ret)
>  			return PCIBIOS_SET_FAILED;
>  	}
> @@ -650,6 +663,7 @@ static struct pci_ops dw_pcie_ops = {
>  static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct dw_pcie_ob_atu_cfg atu = { 0 };
>  	struct resource_entry *entry;
>  	int i, ret;
>  
> @@ -677,10 +691,13 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>  		if (pci->num_ob_windows <= ++i)
>  			break;
>  
> -		ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
> -						entry->res->start,
> -						entry->res->start - entry->offset,
> -						resource_size(entry->res));
> +		atu.index = i;
> +		atu.type = PCIE_ATU_TYPE_MEM;
> +		atu.cpu_addr = entry->res->start;
> +		atu.pci_addr = entry->res->start - entry->offset;
> +		atu.size = resource_size(entry->res);
> +
> +		ret = dw_pcie_prog_outbound_atu(pci, &atu);
>  		if (ret) {
>  			dev_err(pci->dev, "Failed to set MEM range %pr\n",
>  				entry->res);
> @@ -690,10 +707,13 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>  
>  	if (pp->io_size) {
>  		if (pci->num_ob_windows > ++i) {
> -			ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_IO,
> -							pp->io_base,
> -							pp->io_bus_addr,
> -							pp->io_size);
> +			atu.index = i;
> +			atu.type = PCIE_ATU_TYPE_IO;
> +			atu.cpu_addr = pp->io_base;
> +			atu.pci_addr = pp->io_bus_addr;
> +			atu.size = pp->io_size;
> +
> +			ret = dw_pcie_prog_outbound_atu(pci, &atu);
>  			if (ret) {
>  				dev_err(pci->dev, "Failed to set IO range %pr\n",
>  					entry->res);
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 2459f2a61b9b..49b785509576 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -464,56 +464,56 @@ static inline u32 dw_pcie_enable_ecrc(u32 val)
>  	return val | PCIE_ATU_TD;
>  }
>  
> -static int __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> -				       int index, int type, u64 cpu_addr,
> -				       u64 pci_addr, u64 size)
> +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> +			      const struct dw_pcie_ob_atu_cfg *atu)
>  {
> +	u64 cpu_addr = atu->cpu_addr;
>  	u32 retries, val;
>  	u64 limit_addr;
>  
>  	if (pci->ops && pci->ops->cpu_addr_fixup)
>  		cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
>  
> -	limit_addr = cpu_addr + size - 1;
> +	limit_addr = cpu_addr + atu->size - 1;
>  
>  	if ((limit_addr & ~pci->region_limit) != (cpu_addr & ~pci->region_limit) ||
>  	    !IS_ALIGNED(cpu_addr, pci->region_align) ||
> -	    !IS_ALIGNED(pci_addr, pci->region_align) || !size) {
> +	    !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size) {
>  		return -EINVAL;
>  	}
>  
> -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_BASE,
> +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_BASE,
>  			      lower_32_bits(cpu_addr));
> -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_BASE,
> +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_BASE,
>  			      upper_32_bits(cpu_addr));
>  
> -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LIMIT,
> +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LIMIT,
>  			      lower_32_bits(limit_addr));
>  	if (dw_pcie_ver_is_ge(pci, 460A))
> -		dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_LIMIT,
> +		dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_LIMIT,
>  				      upper_32_bits(limit_addr));
>  
> -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_TARGET,
> -			      lower_32_bits(pci_addr));
> -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_TARGET,
> -			      upper_32_bits(pci_addr));
> +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_TARGET,
> +			      lower_32_bits(atu->pci_addr));
> +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_TARGET,
> +			      upper_32_bits(atu->pci_addr));
>  
> -	val = type | PCIE_ATU_FUNC_NUM(func_no);
> +	val = atu->type | PCIE_ATU_FUNC_NUM(atu->func_no);
>  	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) &&
>  	    dw_pcie_ver_is_ge(pci, 460A))
>  		val |= PCIE_ATU_INCREASE_REGION_SIZE;
>  	if (dw_pcie_ver_is(pci, 490A))
>  		val = dw_pcie_enable_ecrc(val);
> -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL1, val);
> +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL1, val);
>  
> -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
> +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
>  
>  	/*
>  	 * Make sure ATU enable takes effect before any subsequent config
>  	 * and I/O accesses.
>  	 */
>  	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
> -		val = dw_pcie_readl_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2);
> +		val = dw_pcie_readl_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2);
>  		if (val & PCIE_ATU_ENABLE)
>  			return 0;
>  
> @@ -525,21 +525,6 @@ static int __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
>  	return -ETIMEDOUT;
>  }
>  
> -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> -			      u64 cpu_addr, u64 pci_addr, u64 size)
> -{
> -	return __dw_pcie_prog_outbound_atu(pci, 0, index, type,
> -					   cpu_addr, pci_addr, size);
> -}
> -
> -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> -				 int type, u64 cpu_addr, u64 pci_addr,
> -				 u64 size)
> -{
> -	return __dw_pcie_prog_outbound_atu(pci, func_no, index, type,
> -					   cpu_addr, pci_addr, size);
> -}
> -
>  static inline u32 dw_pcie_readl_atu_ib(struct dw_pcie *pci, u32 index, u32 reg)
>  {
>  	return dw_pcie_readl_atu(pci, PCIE_ATU_REGION_DIR_IB, index, reg);
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 3c06e025c905..85de0d8346fa 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -288,6 +288,15 @@ enum dw_pcie_core_rst {
>  	DW_PCIE_NUM_CORE_RSTS
>  };
>  
> +struct dw_pcie_ob_atu_cfg {
> +	int index;
> +	int type;
> +	u8 func_no;
> +	u64 cpu_addr;
> +	u64 pci_addr;
> +	u64 size;
> +};
> +
>  struct dw_pcie_host_ops {
>  	int (*host_init)(struct dw_pcie_rp *pp);
>  	void (*host_deinit)(struct dw_pcie_rp *pp);
> @@ -416,10 +425,8 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
>  int dw_pcie_link_up(struct dw_pcie *pci);
>  void dw_pcie_upconfig_setup(struct dw_pcie *pci);
>  int dw_pcie_wait_for_link(struct dw_pcie *pci);
> -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> -			      u64 cpu_addr, u64 pci_addr, u64 size);
> -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> -				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
> +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> +			      const struct dw_pcie_ob_atu_cfg *atu);
>  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
>  			     u64 cpu_addr, u64 pci_addr, u64 size);
>  int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> -- 
> 2.25.1
>

Yoshihiro Shimoda July 31, 2023, 1:24 a.m. UTC | #9

Hi Serge,

> From: Serge Semin, Sent: Saturday, July 29, 2023 11:07 AM
> 
> On Fri, Jul 21, 2023 at 04:44:36PM +0900, Yoshihiro Shimoda wrote:
> > The __dw_pcie_prog_outbound_atu() currently has 6 arguments.
> > To support INTx IRQs in the future, it requires an additional 2
> > arguments. For improved code readability, introduce the struct
> > dw_pcie_ob_atu_cfg and update the arguments of
> > dw_pcie_prog_outbound_atu().
> >
> > Consequently, remove __dw_pcie_prog_outbound_atu() and
> > dw_pcie_prog_ep_outbound_atu() because there is no longer
> > a need.
> >
> > No behavior changes.
> 
> So you decided not to use a suggested by me in v17 more detailed patch
> log?

You're correct. I thought your suggested comments was too detailed.

Best regards,
Yoshihiro Shimoda

> C&P it here just in case if you change your mind:
> 
> This is a preparation before adding the Msg-type outbound iATU
> mapping. The respective update will require two more arguments added
> to __dw_pcie_prog_outbound_atu(). That will make the already
> complicated function prototype even more hard to comprehend accepting
> _eight_ arguments. In order to prevent that and keep the code
> more-or-less readable all the outbound iATU-related arguments are
> moved to the new config-structure: struct dw_pcie_ob_atu_cfg pointer
> to which shall be passed to dw_pcie_prog_outbound_atu(). The structure
> is supposed to be locally defined and populated with the outbound iATU
> settings implied by the caller context.
> 
> As a result of the denoted change there is no longer need in having
> the two distinctive methods for the Host and End-point outbound iATU
> setups since the corresponding code can directly call the
> dw_pcie_prog_outbound_atu() method with the config-structure
> populated. Thus dw_pcie_prog_ep_outbound_atu() is dropped.
> 
> -Serge(y)
> 
> >
> > Signed-off-by: Yoshihiro Shimoda <[e-mail]>
> > Reviewed-by: Serge Semin <[e-mail]>
> > ---
> >  .../pci/controller/dwc/pcie-designware-ep.c   | 21 +++++---
> >  .../pci/controller/dwc/pcie-designware-host.c | 52 +++++++++++++------
> >  drivers/pci/controller/dwc/pcie-designware.c  | 49 ++++++-----------
> >  drivers/pci/controller/dwc/pcie-designware.h  | 15 ++++--
> >  4 files changed, 77 insertions(+), 60 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 27278010ecec..fe2e0d765be9 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -182,9 +182,8 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
> >  	return 0;
> >  }
> >
> > -static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
> > -				   phys_addr_t phys_addr,
> > -				   u64 pci_addr, size_t size)
> > +static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep,
> > +				   struct dw_pcie_ob_atu_cfg *atu)
> >  {
> >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >  	u32 free_win;
> > @@ -196,13 +195,13 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
> >  		return -EINVAL;
> >  	}
> >
> > -	ret = dw_pcie_prog_ep_outbound_atu(pci, func_no, free_win, PCIE_ATU_TYPE_MEM,
> > -					   phys_addr, pci_addr, size);
> > +	atu->index = free_win;
> > +	ret = dw_pcie_prog_outbound_atu(pci, atu);
> >  	if (ret)
> >  		return ret;
> >
> >  	set_bit(free_win, ep->ob_window_map);
> > -	ep->outbound_addr[free_win] = phys_addr;
> > +	ep->outbound_addr[free_win] = atu->cpu_addr;
> >
> >  	return 0;
> >  }
> > @@ -305,8 +304,14 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> >  	int ret;
> >  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > -
> > -	ret = dw_pcie_ep_outbound_atu(ep, func_no, addr, pci_addr, size);
> > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > +
> > +	atu.func_no = func_no;
> > +	atu.type = PCIE_ATU_TYPE_MEM;
> > +	atu.cpu_addr = addr;
> > +	atu.pci_addr = pci_addr;
> > +	atu.size = size;
> > +	ret = dw_pcie_ep_outbound_atu(ep, &atu);
> >  	if (ret) {
> >  		dev_err(pci->dev, "Failed to enable address\n");
> >  		return ret;
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index cf61733bf78d..7419185721f2 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -549,6 +549,7 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> >  {
> >  	struct dw_pcie_rp *pp = bus->sysdata;
> >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> >  	int type, ret;
> >  	u32 busdev;
> >
> > @@ -571,8 +572,12 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> >  	else
> >  		type = PCIE_ATU_TYPE_CFG1;
> >
> > -	ret = dw_pcie_prog_outbound_atu(pci, 0, type, pp->cfg0_base, busdev,
> > -					pp->cfg0_size);
> > +	atu.type = type;
> > +	atu.cpu_addr = pp->cfg0_base;
> > +	atu.pci_addr = busdev;
> > +	atu.size = pp->cfg0_size;
> > +
> > +	ret = dw_pcie_prog_outbound_atu(pci, &atu);
> >  	if (ret)
> >  		return NULL;
> >
> > @@ -584,6 +589,7 @@ static int dw_pcie_rd_other_conf(struct pci_bus *bus, unsigned int devfn,
> >  {
> >  	struct dw_pcie_rp *pp = bus->sysdata;
> >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> >  	int ret;
> >
> >  	ret = pci_generic_config_read(bus, devfn, where, size, val);
> > @@ -591,9 +597,12 @@ static int dw_pcie_rd_other_conf(struct pci_bus *bus, unsigned int devfn,
> >  		return ret;
> >
> >  	if (pp->cfg0_io_shared) {
> > -		ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
> > -						pp->io_base, pp->io_bus_addr,
> > -						pp->io_size);
> > +		atu.type = PCIE_ATU_TYPE_IO;
> > +		atu.cpu_addr = pp->io_base;
> > +		atu.pci_addr = pp->io_bus_addr;
> > +		atu.size = pp->io_size;
> > +
> > +		ret = dw_pcie_prog_outbound_atu(pci, &atu);
> >  		if (ret)
> >  			return PCIBIOS_SET_FAILED;
> >  	}
> > @@ -606,6 +615,7 @@ static int dw_pcie_wr_other_conf(struct pci_bus *bus, unsigned int devfn,
> >  {
> >  	struct dw_pcie_rp *pp = bus->sysdata;
> >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> >  	int ret;
> >
> >  	ret = pci_generic_config_write(bus, devfn, where, size, val);
> > @@ -613,9 +623,12 @@ static int dw_pcie_wr_other_conf(struct pci_bus *bus, unsigned int devfn,
> >  		return ret;
> >
> >  	if (pp->cfg0_io_shared) {
> > -		ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
> > -						pp->io_base, pp->io_bus_addr,
> > -						pp->io_size);
> > +		atu.type = PCIE_ATU_TYPE_IO;
> > +		atu.cpu_addr = pp->io_base;
> > +		atu.pci_addr = pp->io_bus_addr;
> > +		atu.size = pp->io_size;
> > +
> > +		ret = dw_pcie_prog_outbound_atu(pci, &atu);
> >  		if (ret)
> >  			return PCIBIOS_SET_FAILED;
> >  	}
> > @@ -650,6 +663,7 @@ static struct pci_ops dw_pcie_ops = {
> >  static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> >  {
> >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> >  	struct resource_entry *entry;
> >  	int i, ret;
> >
> > @@ -677,10 +691,13 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> >  		if (pci->num_ob_windows <= ++i)
> >  			break;
> >
> > -		ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
> > -						entry->res->start,
> > -						entry->res->start - entry->offset,
> > -						resource_size(entry->res));
> > +		atu.index = i;
> > +		atu.type = PCIE_ATU_TYPE_MEM;
> > +		atu.cpu_addr = entry->res->start;
> > +		atu.pci_addr = entry->res->start - entry->offset;
> > +		atu.size = resource_size(entry->res);
> > +
> > +		ret = dw_pcie_prog_outbound_atu(pci, &atu);
> >  		if (ret) {
> >  			dev_err(pci->dev, "Failed to set MEM range %pr\n",
> >  				entry->res);
> > @@ -690,10 +707,13 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> >
> >  	if (pp->io_size) {
> >  		if (pci->num_ob_windows > ++i) {
> > -			ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_IO,
> > -							pp->io_base,
> > -							pp->io_bus_addr,
> > -							pp->io_size);
> > +			atu.index = i;
> > +			atu.type = PCIE_ATU_TYPE_IO;
> > +			atu.cpu_addr = pp->io_base;
> > +			atu.pci_addr = pp->io_bus_addr;
> > +			atu.size = pp->io_size;
> > +
> > +			ret = dw_pcie_prog_outbound_atu(pci, &atu);
> >  			if (ret) {
> >  				dev_err(pci->dev, "Failed to set IO range %pr\n",
> >  					entry->res);
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 2459f2a61b9b..49b785509576 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -464,56 +464,56 @@ static inline u32 dw_pcie_enable_ecrc(u32 val)
> >  	return val | PCIE_ATU_TD;
> >  }
> >
> > -static int __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> > -				       int index, int type, u64 cpu_addr,
> > -				       u64 pci_addr, u64 size)
> > +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> > +			      const struct dw_pcie_ob_atu_cfg *atu)
> >  {
> > +	u64 cpu_addr = atu->cpu_addr;
> >  	u32 retries, val;
> >  	u64 limit_addr;
> >
> >  	if (pci->ops && pci->ops->cpu_addr_fixup)
> >  		cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
> >
> > -	limit_addr = cpu_addr + size - 1;
> > +	limit_addr = cpu_addr + atu->size - 1;
> >
> >  	if ((limit_addr & ~pci->region_limit) != (cpu_addr & ~pci->region_limit) ||
> >  	    !IS_ALIGNED(cpu_addr, pci->region_align) ||
> > -	    !IS_ALIGNED(pci_addr, pci->region_align) || !size) {
> > +	    !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size) {
> >  		return -EINVAL;
> >  	}
> >
> > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_BASE,
> > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_BASE,
> >  			      lower_32_bits(cpu_addr));
> > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_BASE,
> > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_BASE,
> >  			      upper_32_bits(cpu_addr));
> >
> > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LIMIT,
> > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LIMIT,
> >  			      lower_32_bits(limit_addr));
> >  	if (dw_pcie_ver_is_ge(pci, 460A))
> > -		dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_LIMIT,
> > +		dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_LIMIT,
> >  				      upper_32_bits(limit_addr));
> >
> > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_TARGET,
> > -			      lower_32_bits(pci_addr));
> > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_TARGET,
> > -			      upper_32_bits(pci_addr));
> > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_TARGET,
> > +			      lower_32_bits(atu->pci_addr));
> > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_TARGET,
> > +			      upper_32_bits(atu->pci_addr));
> >
> > -	val = type | PCIE_ATU_FUNC_NUM(func_no);
> > +	val = atu->type | PCIE_ATU_FUNC_NUM(atu->func_no);
> >  	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) &&
> >  	    dw_pcie_ver_is_ge(pci, 460A))
> >  		val |= PCIE_ATU_INCREASE_REGION_SIZE;
> >  	if (dw_pcie_ver_is(pci, 490A))
> >  		val = dw_pcie_enable_ecrc(val);
> > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL1, val);
> > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL1, val);
> >
> > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
> > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
> >
> >  	/*
> >  	 * Make sure ATU enable takes effect before any subsequent config
> >  	 * and I/O accesses.
> >  	 */
> >  	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
> > -		val = dw_pcie_readl_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2);
> > +		val = dw_pcie_readl_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2);
> >  		if (val & PCIE_ATU_ENABLE)
> >  			return 0;
> >
> > @@ -525,21 +525,6 @@ static int __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> >  	return -ETIMEDOUT;
> >  }
> >
> > -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > -			      u64 cpu_addr, u64 pci_addr, u64 size)
> > -{
> > -	return __dw_pcie_prog_outbound_atu(pci, 0, index, type,
> > -					   cpu_addr, pci_addr, size);
> > -}
> > -
> > -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > -				 int type, u64 cpu_addr, u64 pci_addr,
> > -				 u64 size)
> > -{
> > -	return __dw_pcie_prog_outbound_atu(pci, func_no, index, type,
> > -					   cpu_addr, pci_addr, size);
> > -}
> > -
> >  static inline u32 dw_pcie_readl_atu_ib(struct dw_pcie *pci, u32 index, u32 reg)
> >  {
> >  	return dw_pcie_readl_atu(pci, PCIE_ATU_REGION_DIR_IB, index, reg);
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 3c06e025c905..85de0d8346fa 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -288,6 +288,15 @@ enum dw_pcie_core_rst {
> >  	DW_PCIE_NUM_CORE_RSTS
> >  };
> >
> > +struct dw_pcie_ob_atu_cfg {
> > +	int index;
> > +	int type;
> > +	u8 func_no;
> > +	u64 cpu_addr;
> > +	u64 pci_addr;
> > +	u64 size;
> > +};
> > +
> >  struct dw_pcie_host_ops {
> >  	int (*host_init)(struct dw_pcie_rp *pp);
> >  	void (*host_deinit)(struct dw_pcie_rp *pp);
> > @@ -416,10 +425,8 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
> >  int dw_pcie_link_up(struct dw_pcie *pci);
> >  void dw_pcie_upconfig_setup(struct dw_pcie *pci);
> >  int dw_pcie_wait_for_link(struct dw_pcie *pci);
> > -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > -			      u64 cpu_addr, u64 pci_addr, u64 size);
> > -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > -				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
> > +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> > +			      const struct dw_pcie_ob_atu_cfg *atu);
> >  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> >  			     u64 cpu_addr, u64 pci_addr, u64 size);
> >  int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > --
> > 2.25.1
> >

Serge Semin July 31, 2023, 9:33 p.m. UTC | #10

On Mon, Jul 31, 2023 at 01:24:27AM +0000, Yoshihiro Shimoda wrote:
> Hi Serge,
> 
> > From: Serge Semin, Sent: Saturday, July 29, 2023 11:07 AM
> > 
> > On Fri, Jul 21, 2023 at 04:44:36PM +0900, Yoshihiro Shimoda wrote:
> > > The __dw_pcie_prog_outbound_atu() currently has 6 arguments.
> > > To support INTx IRQs in the future, it requires an additional 2
> > > arguments. For improved code readability, introduce the struct
> > > dw_pcie_ob_atu_cfg and update the arguments of
> > > dw_pcie_prog_outbound_atu().
> > >
> > > Consequently, remove __dw_pcie_prog_outbound_atu() and
> > > dw_pcie_prog_ep_outbound_atu() because there is no longer
> > > a need.
> > >
> > > No behavior changes.
> > 
> > So you decided not to use a suggested by me in v17 more detailed patch
> > log?
> 
> You're correct. I thought your suggested comments was too detailed.

I strongly recommend for you to use mine instead. It gives more
details about the change and the patch context. Moreover it much more
clearer justifies the change implemented in the patch.

-Serge(y)

> 
> Best regards,
> Yoshihiro Shimoda
> 
> > C&P it here just in case if you change your mind:
> > 
> > This is a preparation before adding the Msg-type outbound iATU
> > mapping. The respective update will require two more arguments added
> > to __dw_pcie_prog_outbound_atu(). That will make the already
> > complicated function prototype even more hard to comprehend accepting
> > _eight_ arguments. In order to prevent that and keep the code
> > more-or-less readable all the outbound iATU-related arguments are
> > moved to the new config-structure: struct dw_pcie_ob_atu_cfg pointer
> > to which shall be passed to dw_pcie_prog_outbound_atu(). The structure
> > is supposed to be locally defined and populated with the outbound iATU
> > settings implied by the caller context.
> > 
> > As a result of the denoted change there is no longer need in having
> > the two distinctive methods for the Host and End-point outbound iATU
> > setups since the corresponding code can directly call the
> > dw_pcie_prog_outbound_atu() method with the config-structure
> > populated. Thus dw_pcie_prog_ep_outbound_atu() is dropped.
> > 
> > -Serge(y)
> > 
> > >
> > > Signed-off-by: Yoshihiro Shimoda <[e-mail]>
> > > Reviewed-by: Serge Semin <[e-mail]>
> > > ---
> > >  .../pci/controller/dwc/pcie-designware-ep.c   | 21 +++++---
> > >  .../pci/controller/dwc/pcie-designware-host.c | 52 +++++++++++++------
> > >  drivers/pci/controller/dwc/pcie-designware.c  | 49 ++++++-----------
> > >  drivers/pci/controller/dwc/pcie-designware.h  | 15 ++++--
> > >  4 files changed, 77 insertions(+), 60 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > index 27278010ecec..fe2e0d765be9 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > @@ -182,9 +182,8 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
> > >  	return 0;
> > >  }
> > >
> > > -static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
> > > -				   phys_addr_t phys_addr,
> > > -				   u64 pci_addr, size_t size)
> > > +static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep,
> > > +				   struct dw_pcie_ob_atu_cfg *atu)
> > >  {
> > >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > >  	u32 free_win;
> > > @@ -196,13 +195,13 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
> > >  		return -EINVAL;
> > >  	}
> > >
> > > -	ret = dw_pcie_prog_ep_outbound_atu(pci, func_no, free_win, PCIE_ATU_TYPE_MEM,
> > > -					   phys_addr, pci_addr, size);
> > > +	atu->index = free_win;
> > > +	ret = dw_pcie_prog_outbound_atu(pci, atu);
> > >  	if (ret)
> > >  		return ret;
> > >
> > >  	set_bit(free_win, ep->ob_window_map);
> > > -	ep->outbound_addr[free_win] = phys_addr;
> > > +	ep->outbound_addr[free_win] = atu->cpu_addr;
> > >
> > >  	return 0;
> > >  }
> > > @@ -305,8 +304,14 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> > >  	int ret;
> > >  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> > >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > -
> > > -	ret = dw_pcie_ep_outbound_atu(ep, func_no, addr, pci_addr, size);
> > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > > +
> > > +	atu.func_no = func_no;
> > > +	atu.type = PCIE_ATU_TYPE_MEM;
> > > +	atu.cpu_addr = addr;
> > > +	atu.pci_addr = pci_addr;
> > > +	atu.size = size;
> > > +	ret = dw_pcie_ep_outbound_atu(ep, &atu);
> > >  	if (ret) {
> > >  		dev_err(pci->dev, "Failed to enable address\n");
> > >  		return ret;
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index cf61733bf78d..7419185721f2 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -549,6 +549,7 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> > >  {
> > >  	struct dw_pcie_rp *pp = bus->sysdata;
> > >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > >  	int type, ret;
> > >  	u32 busdev;
> > >
> > > @@ -571,8 +572,12 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> > >  	else
> > >  		type = PCIE_ATU_TYPE_CFG1;
> > >
> > > -	ret = dw_pcie_prog_outbound_atu(pci, 0, type, pp->cfg0_base, busdev,
> > > -					pp->cfg0_size);
> > > +	atu.type = type;
> > > +	atu.cpu_addr = pp->cfg0_base;
> > > +	atu.pci_addr = busdev;
> > > +	atu.size = pp->cfg0_size;
> > > +
> > > +	ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > >  	if (ret)
> > >  		return NULL;
> > >
> > > @@ -584,6 +589,7 @@ static int dw_pcie_rd_other_conf(struct pci_bus *bus, unsigned int devfn,
> > >  {
> > >  	struct dw_pcie_rp *pp = bus->sysdata;
> > >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > >  	int ret;
> > >
> > >  	ret = pci_generic_config_read(bus, devfn, where, size, val);
> > > @@ -591,9 +597,12 @@ static int dw_pcie_rd_other_conf(struct pci_bus *bus, unsigned int devfn,
> > >  		return ret;
> > >
> > >  	if (pp->cfg0_io_shared) {
> > > -		ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
> > > -						pp->io_base, pp->io_bus_addr,
> > > -						pp->io_size);
> > > +		atu.type = PCIE_ATU_TYPE_IO;
> > > +		atu.cpu_addr = pp->io_base;
> > > +		atu.pci_addr = pp->io_bus_addr;
> > > +		atu.size = pp->io_size;
> > > +
> > > +		ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > >  		if (ret)
> > >  			return PCIBIOS_SET_FAILED;
> > >  	}
> > > @@ -606,6 +615,7 @@ static int dw_pcie_wr_other_conf(struct pci_bus *bus, unsigned int devfn,
> > >  {
> > >  	struct dw_pcie_rp *pp = bus->sysdata;
> > >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > >  	int ret;
> > >
> > >  	ret = pci_generic_config_write(bus, devfn, where, size, val);
> > > @@ -613,9 +623,12 @@ static int dw_pcie_wr_other_conf(struct pci_bus *bus, unsigned int devfn,
> > >  		return ret;
> > >
> > >  	if (pp->cfg0_io_shared) {
> > > -		ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
> > > -						pp->io_base, pp->io_bus_addr,
> > > -						pp->io_size);
> > > +		atu.type = PCIE_ATU_TYPE_IO;
> > > +		atu.cpu_addr = pp->io_base;
> > > +		atu.pci_addr = pp->io_bus_addr;
> > > +		atu.size = pp->io_size;
> > > +
> > > +		ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > >  		if (ret)
> > >  			return PCIBIOS_SET_FAILED;
> > >  	}
> > > @@ -650,6 +663,7 @@ static struct pci_ops dw_pcie_ops = {
> > >  static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> > >  {
> > >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > >  	struct resource_entry *entry;
> > >  	int i, ret;
> > >
> > > @@ -677,10 +691,13 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> > >  		if (pci->num_ob_windows <= ++i)
> > >  			break;
> > >
> > > -		ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
> > > -						entry->res->start,
> > > -						entry->res->start - entry->offset,
> > > -						resource_size(entry->res));
> > > +		atu.index = i;
> > > +		atu.type = PCIE_ATU_TYPE_MEM;
> > > +		atu.cpu_addr = entry->res->start;
> > > +		atu.pci_addr = entry->res->start - entry->offset;
> > > +		atu.size = resource_size(entry->res);
> > > +
> > > +		ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > >  		if (ret) {
> > >  			dev_err(pci->dev, "Failed to set MEM range %pr\n",
> > >  				entry->res);
> > > @@ -690,10 +707,13 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> > >
> > >  	if (pp->io_size) {
> > >  		if (pci->num_ob_windows > ++i) {
> > > -			ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_IO,
> > > -							pp->io_base,
> > > -							pp->io_bus_addr,
> > > -							pp->io_size);
> > > +			atu.index = i;
> > > +			atu.type = PCIE_ATU_TYPE_IO;
> > > +			atu.cpu_addr = pp->io_base;
> > > +			atu.pci_addr = pp->io_bus_addr;
> > > +			atu.size = pp->io_size;
> > > +
> > > +			ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > >  			if (ret) {
> > >  				dev_err(pci->dev, "Failed to set IO range %pr\n",
> > >  					entry->res);
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > index 2459f2a61b9b..49b785509576 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > @@ -464,56 +464,56 @@ static inline u32 dw_pcie_enable_ecrc(u32 val)
> > >  	return val | PCIE_ATU_TD;
> > >  }
> > >
> > > -static int __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> > > -				       int index, int type, u64 cpu_addr,
> > > -				       u64 pci_addr, u64 size)
> > > +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> > > +			      const struct dw_pcie_ob_atu_cfg *atu)
> > >  {
> > > +	u64 cpu_addr = atu->cpu_addr;
> > >  	u32 retries, val;
> > >  	u64 limit_addr;
> > >
> > >  	if (pci->ops && pci->ops->cpu_addr_fixup)
> > >  		cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
> > >
> > > -	limit_addr = cpu_addr + size - 1;
> > > +	limit_addr = cpu_addr + atu->size - 1;
> > >
> > >  	if ((limit_addr & ~pci->region_limit) != (cpu_addr & ~pci->region_limit) ||
> > >  	    !IS_ALIGNED(cpu_addr, pci->region_align) ||
> > > -	    !IS_ALIGNED(pci_addr, pci->region_align) || !size) {
> > > +	    !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size) {
> > >  		return -EINVAL;
> > >  	}
> > >
> > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_BASE,
> > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_BASE,
> > >  			      lower_32_bits(cpu_addr));
> > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_BASE,
> > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_BASE,
> > >  			      upper_32_bits(cpu_addr));
> > >
> > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LIMIT,
> > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LIMIT,
> > >  			      lower_32_bits(limit_addr));
> > >  	if (dw_pcie_ver_is_ge(pci, 460A))
> > > -		dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_LIMIT,
> > > +		dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_LIMIT,
> > >  				      upper_32_bits(limit_addr));
> > >
> > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_TARGET,
> > > -			      lower_32_bits(pci_addr));
> > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_TARGET,
> > > -			      upper_32_bits(pci_addr));
> > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_TARGET,
> > > +			      lower_32_bits(atu->pci_addr));
> > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_TARGET,
> > > +			      upper_32_bits(atu->pci_addr));
> > >
> > > -	val = type | PCIE_ATU_FUNC_NUM(func_no);
> > > +	val = atu->type | PCIE_ATU_FUNC_NUM(atu->func_no);
> > >  	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) &&
> > >  	    dw_pcie_ver_is_ge(pci, 460A))
> > >  		val |= PCIE_ATU_INCREASE_REGION_SIZE;
> > >  	if (dw_pcie_ver_is(pci, 490A))
> > >  		val = dw_pcie_enable_ecrc(val);
> > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL1, val);
> > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL1, val);
> > >
> > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
> > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
> > >
> > >  	/*
> > >  	 * Make sure ATU enable takes effect before any subsequent config
> > >  	 * and I/O accesses.
> > >  	 */
> > >  	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
> > > -		val = dw_pcie_readl_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2);
> > > +		val = dw_pcie_readl_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2);
> > >  		if (val & PCIE_ATU_ENABLE)
> > >  			return 0;
> > >
> > > @@ -525,21 +525,6 @@ static int __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> > >  	return -ETIMEDOUT;
> > >  }
> > >
> > > -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > > -			      u64 cpu_addr, u64 pci_addr, u64 size)
> > > -{
> > > -	return __dw_pcie_prog_outbound_atu(pci, 0, index, type,
> > > -					   cpu_addr, pci_addr, size);
> > > -}
> > > -
> > > -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > -				 int type, u64 cpu_addr, u64 pci_addr,
> > > -				 u64 size)
> > > -{
> > > -	return __dw_pcie_prog_outbound_atu(pci, func_no, index, type,
> > > -					   cpu_addr, pci_addr, size);
> > > -}
> > > -
> > >  static inline u32 dw_pcie_readl_atu_ib(struct dw_pcie *pci, u32 index, u32 reg)
> > >  {
> > >  	return dw_pcie_readl_atu(pci, PCIE_ATU_REGION_DIR_IB, index, reg);
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > index 3c06e025c905..85de0d8346fa 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > @@ -288,6 +288,15 @@ enum dw_pcie_core_rst {
> > >  	DW_PCIE_NUM_CORE_RSTS
> > >  };
> > >
> > > +struct dw_pcie_ob_atu_cfg {
> > > +	int index;
> > > +	int type;
> > > +	u8 func_no;
> > > +	u64 cpu_addr;
> > > +	u64 pci_addr;
> > > +	u64 size;
> > > +};
> > > +
> > >  struct dw_pcie_host_ops {
> > >  	int (*host_init)(struct dw_pcie_rp *pp);
> > >  	void (*host_deinit)(struct dw_pcie_rp *pp);
> > > @@ -416,10 +425,8 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
> > >  int dw_pcie_link_up(struct dw_pcie *pci);
> > >  void dw_pcie_upconfig_setup(struct dw_pcie *pci);
> > >  int dw_pcie_wait_for_link(struct dw_pcie *pci);
> > > -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > > -			      u64 cpu_addr, u64 pci_addr, u64 size);
> > > -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > -				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
> > > +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> > > +			      const struct dw_pcie_ob_atu_cfg *atu);
> > >  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> > >  			     u64 cpu_addr, u64 pci_addr, u64 size);
> > >  int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > --
> > > 2.25.1
> > >

Yoshihiro Shimoda Aug. 1, 2023, 1:29 a.m. UTC | #11

Hi Serge,

> From: Serge Semin, Sent: Tuesday, August 1, 2023 6:33 AM
> 
> On Mon, Jul 31, 2023 at 01:24:27AM +0000, Yoshihiro Shimoda wrote:
> > Hi Serge,
> >
> > > From: Serge Semin, Sent: Saturday, July 29, 2023 11:07 AM
> > >
> > > On Fri, Jul 21, 2023 at 04:44:36PM +0900, Yoshihiro Shimoda wrote:
> > > > The __dw_pcie_prog_outbound_atu() currently has 6 arguments.
> > > > To support INTx IRQs in the future, it requires an additional 2
> > > > arguments. For improved code readability, introduce the struct
> > > > dw_pcie_ob_atu_cfg and update the arguments of
> > > > dw_pcie_prog_outbound_atu().
> > > >
> > > > Consequently, remove __dw_pcie_prog_outbound_atu() and
> > > > dw_pcie_prog_ep_outbound_atu() because there is no longer
> > > > a need.
> > > >
> > > > No behavior changes.
> > >
> > > So you decided not to use a suggested by me in v17 more detailed patch
> > > log?
> >
> > You're correct. I thought your suggested comments was too detailed.
> 
> I strongly recommend for you to use mine instead. It gives more
> details about the change and the patch context. Moreover it much more
> clearer justifies the change implemented in the patch.

I didn't realize that you have a strong recommendation about the comments
you suggested. I'll replace the commit description and add your Suggested-by
tag on v19.

Best regards,
Yoshihiro Shimoda

> -Serge(y)
> 
> >
> > Best regards,
> > Yoshihiro Shimoda
> >
> > > C&P it here just in case if you change your mind:
> > >
> > > This is a preparation before adding the Msg-type outbound iATU
> > > mapping. The respective update will require two more arguments added
> > > to __dw_pcie_prog_outbound_atu(). That will make the already
> > > complicated function prototype even more hard to comprehend accepting
> > > _eight_ arguments. In order to prevent that and keep the code
> > > more-or-less readable all the outbound iATU-related arguments are
> > > moved to the new config-structure: struct dw_pcie_ob_atu_cfg pointer
> > > to which shall be passed to dw_pcie_prog_outbound_atu(). The structure
> > > is supposed to be locally defined and populated with the outbound iATU
> > > settings implied by the caller context.
> > >
> > > As a result of the denoted change there is no longer need in having
> > > the two distinctive methods for the Host and End-point outbound iATU
> > > setups since the corresponding code can directly call the
> > > dw_pcie_prog_outbound_atu() method with the config-structure
> > > populated. Thus dw_pcie_prog_ep_outbound_atu() is dropped.
> > >
> > > -Serge(y)
> > >
> > > >
> > > > Signed-off-by: Yoshihiro Shimoda <[e-mail]>
> > > > Reviewed-by: Serge Semin <[e-mail]>
> > > > ---
> > > >  .../pci/controller/dwc/pcie-designware-ep.c   | 21 +++++---
> > > >  .../pci/controller/dwc/pcie-designware-host.c | 52 +++++++++++++------
> > > >  drivers/pci/controller/dwc/pcie-designware.c  | 49 ++++++-----------
> > > >  drivers/pci/controller/dwc/pcie-designware.h  | 15 ++++--
> > > >  4 files changed, 77 insertions(+), 60 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > index 27278010ecec..fe2e0d765be9 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > @@ -182,9 +182,8 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
> > > >  	return 0;
> > > >  }
> > > >
> > > > -static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
> > > > -				   phys_addr_t phys_addr,
> > > > -				   u64 pci_addr, size_t size)
> > > > +static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep,
> > > > +				   struct dw_pcie_ob_atu_cfg *atu)
> > > >  {
> > > >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > >  	u32 free_win;
> > > > @@ -196,13 +195,13 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
> > > >  		return -EINVAL;
> > > >  	}
> > > >
> > > > -	ret = dw_pcie_prog_ep_outbound_atu(pci, func_no, free_win, PCIE_ATU_TYPE_MEM,
> > > > -					   phys_addr, pci_addr, size);
> > > > +	atu->index = free_win;
> > > > +	ret = dw_pcie_prog_outbound_atu(pci, atu);
> > > >  	if (ret)
> > > >  		return ret;
> > > >
> > > >  	set_bit(free_win, ep->ob_window_map);
> > > > -	ep->outbound_addr[free_win] = phys_addr;
> > > > +	ep->outbound_addr[free_win] = atu->cpu_addr;
> > > >
> > > >  	return 0;
> > > >  }
> > > > @@ -305,8 +304,14 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> > > >  	int ret;
> > > >  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> > > >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > -
> > > > -	ret = dw_pcie_ep_outbound_atu(ep, func_no, addr, pci_addr, size);
> > > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > > > +
> > > > +	atu.func_no = func_no;
> > > > +	atu.type = PCIE_ATU_TYPE_MEM;
> > > > +	atu.cpu_addr = addr;
> > > > +	atu.pci_addr = pci_addr;
> > > > +	atu.size = size;
> > > > +	ret = dw_pcie_ep_outbound_atu(ep, &atu);
> > > >  	if (ret) {
> > > >  		dev_err(pci->dev, "Failed to enable address\n");
> > > >  		return ret;
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > index cf61733bf78d..7419185721f2 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > @@ -549,6 +549,7 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> > > >  {
> > > >  	struct dw_pcie_rp *pp = bus->sysdata;
> > > >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > > >  	int type, ret;
> > > >  	u32 busdev;
> > > >
> > > > @@ -571,8 +572,12 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> > > >  	else
> > > >  		type = PCIE_ATU_TYPE_CFG1;
> > > >
> > > > -	ret = dw_pcie_prog_outbound_atu(pci, 0, type, pp->cfg0_base, busdev,
> > > > -					pp->cfg0_size);
> > > > +	atu.type = type;
> > > > +	atu.cpu_addr = pp->cfg0_base;
> > > > +	atu.pci_addr = busdev;
> > > > +	atu.size = pp->cfg0_size;
> > > > +
> > > > +	ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > > >  	if (ret)
> > > >  		return NULL;
> > > >
> > > > @@ -584,6 +589,7 @@ static int dw_pcie_rd_other_conf(struct pci_bus *bus, unsigned int devfn,
> > > >  {
> > > >  	struct dw_pcie_rp *pp = bus->sysdata;
> > > >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > > >  	int ret;
> > > >
> > > >  	ret = pci_generic_config_read(bus, devfn, where, size, val);
> > > > @@ -591,9 +597,12 @@ static int dw_pcie_rd_other_conf(struct pci_bus *bus, unsigned int devfn,
> > > >  		return ret;
> > > >
> > > >  	if (pp->cfg0_io_shared) {
> > > > -		ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
> > > > -						pp->io_base, pp->io_bus_addr,
> > > > -						pp->io_size);
> > > > +		atu.type = PCIE_ATU_TYPE_IO;
> > > > +		atu.cpu_addr = pp->io_base;
> > > > +		atu.pci_addr = pp->io_bus_addr;
> > > > +		atu.size = pp->io_size;
> > > > +
> > > > +		ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > > >  		if (ret)
> > > >  			return PCIBIOS_SET_FAILED;
> > > >  	}
> > > > @@ -606,6 +615,7 @@ static int dw_pcie_wr_other_conf(struct pci_bus *bus, unsigned int devfn,
> > > >  {
> > > >  	struct dw_pcie_rp *pp = bus->sysdata;
> > > >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > > >  	int ret;
> > > >
> > > >  	ret = pci_generic_config_write(bus, devfn, where, size, val);
> > > > @@ -613,9 +623,12 @@ static int dw_pcie_wr_other_conf(struct pci_bus *bus, unsigned int devfn,
> > > >  		return ret;
> > > >
> > > >  	if (pp->cfg0_io_shared) {
> > > > -		ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
> > > > -						pp->io_base, pp->io_bus_addr,
> > > > -						pp->io_size);
> > > > +		atu.type = PCIE_ATU_TYPE_IO;
> > > > +		atu.cpu_addr = pp->io_base;
> > > > +		atu.pci_addr = pp->io_bus_addr;
> > > > +		atu.size = pp->io_size;
> > > > +
> > > > +		ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > > >  		if (ret)
> > > >  			return PCIBIOS_SET_FAILED;
> > > >  	}
> > > > @@ -650,6 +663,7 @@ static struct pci_ops dw_pcie_ops = {
> > > >  static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> > > >  {
> > > >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > > >  	struct resource_entry *entry;
> > > >  	int i, ret;
> > > >
> > > > @@ -677,10 +691,13 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> > > >  		if (pci->num_ob_windows <= ++i)
> > > >  			break;
> > > >
> > > > -		ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
> > > > -						entry->res->start,
> > > > -						entry->res->start - entry->offset,
> > > > -						resource_size(entry->res));
> > > > +		atu.index = i;
> > > > +		atu.type = PCIE_ATU_TYPE_MEM;
> > > > +		atu.cpu_addr = entry->res->start;
> > > > +		atu.pci_addr = entry->res->start - entry->offset;
> > > > +		atu.size = resource_size(entry->res);
> > > > +
> > > > +		ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > > >  		if (ret) {
> > > >  			dev_err(pci->dev, "Failed to set MEM range %pr\n",
> > > >  				entry->res);
> > > > @@ -690,10 +707,13 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> > > >
> > > >  	if (pp->io_size) {
> > > >  		if (pci->num_ob_windows > ++i) {
> > > > -			ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_IO,
> > > > -							pp->io_base,
> > > > -							pp->io_bus_addr,
> > > > -							pp->io_size);
> > > > +			atu.index = i;
> > > > +			atu.type = PCIE_ATU_TYPE_IO;
> > > > +			atu.cpu_addr = pp->io_base;
> > > > +			atu.pci_addr = pp->io_bus_addr;
> > > > +			atu.size = pp->io_size;
> > > > +
> > > > +			ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > > >  			if (ret) {
> > > >  				dev_err(pci->dev, "Failed to set IO range %pr\n",
> > > >  					entry->res);
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > > index 2459f2a61b9b..49b785509576 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > @@ -464,56 +464,56 @@ static inline u32 dw_pcie_enable_ecrc(u32 val)
> > > >  	return val | PCIE_ATU_TD;
> > > >  }
> > > >
> > > > -static int __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> > > > -				       int index, int type, u64 cpu_addr,
> > > > -				       u64 pci_addr, u64 size)
> > > > +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> > > > +			      const struct dw_pcie_ob_atu_cfg *atu)
> > > >  {
> > > > +	u64 cpu_addr = atu->cpu_addr;
> > > >  	u32 retries, val;
> > > >  	u64 limit_addr;
> > > >
> > > >  	if (pci->ops && pci->ops->cpu_addr_fixup)
> > > >  		cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
> > > >
> > > > -	limit_addr = cpu_addr + size - 1;
> > > > +	limit_addr = cpu_addr + atu->size - 1;
> > > >
> > > >  	if ((limit_addr & ~pci->region_limit) != (cpu_addr & ~pci->region_limit) ||
> > > >  	    !IS_ALIGNED(cpu_addr, pci->region_align) ||
> > > > -	    !IS_ALIGNED(pci_addr, pci->region_align) || !size) {
> > > > +	    !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size) {
> > > >  		return -EINVAL;
> > > >  	}
> > > >
> > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_BASE,
> > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_BASE,
> > > >  			      lower_32_bits(cpu_addr));
> > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_BASE,
> > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_BASE,
> > > >  			      upper_32_bits(cpu_addr));
> > > >
> > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LIMIT,
> > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LIMIT,
> > > >  			      lower_32_bits(limit_addr));
> > > >  	if (dw_pcie_ver_is_ge(pci, 460A))
> > > > -		dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_LIMIT,
> > > > +		dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_LIMIT,
> > > >  				      upper_32_bits(limit_addr));
> > > >
> > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_TARGET,
> > > > -			      lower_32_bits(pci_addr));
> > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_TARGET,
> > > > -			      upper_32_bits(pci_addr));
> > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_TARGET,
> > > > +			      lower_32_bits(atu->pci_addr));
> > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_TARGET,
> > > > +			      upper_32_bits(atu->pci_addr));
> > > >
> > > > -	val = type | PCIE_ATU_FUNC_NUM(func_no);
> > > > +	val = atu->type | PCIE_ATU_FUNC_NUM(atu->func_no);
> > > >  	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) &&
> > > >  	    dw_pcie_ver_is_ge(pci, 460A))
> > > >  		val |= PCIE_ATU_INCREASE_REGION_SIZE;
> > > >  	if (dw_pcie_ver_is(pci, 490A))
> > > >  		val = dw_pcie_enable_ecrc(val);
> > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL1, val);
> > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL1, val);
> > > >
> > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
> > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
> > > >
> > > >  	/*
> > > >  	 * Make sure ATU enable takes effect before any subsequent config
> > > >  	 * and I/O accesses.
> > > >  	 */
> > > >  	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
> > > > -		val = dw_pcie_readl_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2);
> > > > +		val = dw_pcie_readl_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2);
> > > >  		if (val & PCIE_ATU_ENABLE)
> > > >  			return 0;
> > > >
> > > > @@ -525,21 +525,6 @@ static int __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> > > >  	return -ETIMEDOUT;
> > > >  }
> > > >
> > > > -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > > > -			      u64 cpu_addr, u64 pci_addr, u64 size)
> > > > -{
> > > > -	return __dw_pcie_prog_outbound_atu(pci, 0, index, type,
> > > > -					   cpu_addr, pci_addr, size);
> > > > -}
> > > > -
> > > > -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > -				 int type, u64 cpu_addr, u64 pci_addr,
> > > > -				 u64 size)
> > > > -{
> > > > -	return __dw_pcie_prog_outbound_atu(pci, func_no, index, type,
> > > > -					   cpu_addr, pci_addr, size);
> > > > -}
> > > > -
> > > >  static inline u32 dw_pcie_readl_atu_ib(struct dw_pcie *pci, u32 index, u32 reg)
> > > >  {
> > > >  	return dw_pcie_readl_atu(pci, PCIE_ATU_REGION_DIR_IB, index, reg);
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > > index 3c06e025c905..85de0d8346fa 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > > @@ -288,6 +288,15 @@ enum dw_pcie_core_rst {
> > > >  	DW_PCIE_NUM_CORE_RSTS
> > > >  };
> > > >
> > > > +struct dw_pcie_ob_atu_cfg {
> > > > +	int index;
> > > > +	int type;
> > > > +	u8 func_no;
> > > > +	u64 cpu_addr;
> > > > +	u64 pci_addr;
> > > > +	u64 size;
> > > > +};
> > > > +
> > > >  struct dw_pcie_host_ops {
> > > >  	int (*host_init)(struct dw_pcie_rp *pp);
> > > >  	void (*host_deinit)(struct dw_pcie_rp *pp);
> > > > @@ -416,10 +425,8 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
> > > >  int dw_pcie_link_up(struct dw_pcie *pci);
> > > >  void dw_pcie_upconfig_setup(struct dw_pcie *pci);
> > > >  int dw_pcie_wait_for_link(struct dw_pcie *pci);
> > > > -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > > > -			      u64 cpu_addr, u64 pci_addr, u64 size);
> > > > -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > -				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
> > > > +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> > > > +			      const struct dw_pcie_ob_atu_cfg *atu);
> > > >  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> > > >  			     u64 cpu_addr, u64 pci_addr, u64 size);
> > > >  int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > --
> > > > 2.25.1
> > > >

Serge Semin Aug. 1, 2023, 1:44 a.m. UTC | #12

On Tue, Aug 01, 2023 at 01:29:10AM +0000, Yoshihiro Shimoda wrote:
> Hi Serge,
> 
> > From: Serge Semin, Sent: Tuesday, August 1, 2023 6:33 AM
> > 
> > On Mon, Jul 31, 2023 at 01:24:27AM +0000, Yoshihiro Shimoda wrote:
> > > Hi Serge,
> > >
> > > > From: Serge Semin, Sent: Saturday, July 29, 2023 11:07 AM
> > > >
> > > > On Fri, Jul 21, 2023 at 04:44:36PM +0900, Yoshihiro Shimoda wrote:
> > > > > The __dw_pcie_prog_outbound_atu() currently has 6 arguments.
> > > > > To support INTx IRQs in the future, it requires an additional 2
> > > > > arguments. For improved code readability, introduce the struct
> > > > > dw_pcie_ob_atu_cfg and update the arguments of
> > > > > dw_pcie_prog_outbound_atu().
> > > > >
> > > > > Consequently, remove __dw_pcie_prog_outbound_atu() and
> > > > > dw_pcie_prog_ep_outbound_atu() because there is no longer
> > > > > a need.
> > > > >
> > > > > No behavior changes.
> > > >
> > > > So you decided not to use a suggested by me in v17 more detailed patch
> > > > log?
> > >
> > > You're correct. I thought your suggested comments was too detailed.
> > 
> > I strongly recommend for you to use mine instead. It gives more
> > details about the change and the patch context. Moreover it much more
> > clearer justifies the change implemented in the patch.
> 

> I didn't realize that you have a strong recommendation about the comments
> you suggested. I'll replace the commit description and add your Suggested-by
> tag on v19.

Just to note if there is a misunderstanding on your side. Suggested-by tag is
relevant to the patch idea in general.
See Documentation/process/submitting-patches.rst:559 for details.
So you don't need to add the tag if somebody just suggested an
alternative patch description.

-Serge(y)

> 
> Best regards,
> Yoshihiro Shimoda
> 
> > -Serge(y)
> > 
> > >
> > > Best regards,
> > > Yoshihiro Shimoda
> > >
> > > > C&P it here just in case if you change your mind:
> > > >
> > > > This is a preparation before adding the Msg-type outbound iATU
> > > > mapping. The respective update will require two more arguments added
> > > > to __dw_pcie_prog_outbound_atu(). That will make the already
> > > > complicated function prototype even more hard to comprehend accepting
> > > > _eight_ arguments. In order to prevent that and keep the code
> > > > more-or-less readable all the outbound iATU-related arguments are
> > > > moved to the new config-structure: struct dw_pcie_ob_atu_cfg pointer
> > > > to which shall be passed to dw_pcie_prog_outbound_atu(). The structure
> > > > is supposed to be locally defined and populated with the outbound iATU
> > > > settings implied by the caller context.
> > > >
> > > > As a result of the denoted change there is no longer need in having
> > > > the two distinctive methods for the Host and End-point outbound iATU
> > > > setups since the corresponding code can directly call the
> > > > dw_pcie_prog_outbound_atu() method with the config-structure
> > > > populated. Thus dw_pcie_prog_ep_outbound_atu() is dropped.
> > > >
> > > > -Serge(y)
> > > >
> > > > >
> > > > > Signed-off-by: Yoshihiro Shimoda <[e-mail]>
> > > > > Reviewed-by: Serge Semin <[e-mail]>
> > > > > ---
> > > > >  .../pci/controller/dwc/pcie-designware-ep.c   | 21 +++++---
> > > > >  .../pci/controller/dwc/pcie-designware-host.c | 52 +++++++++++++------
> > > > >  drivers/pci/controller/dwc/pcie-designware.c  | 49 ++++++-----------
> > > > >  drivers/pci/controller/dwc/pcie-designware.h  | 15 ++++--
> > > > >  4 files changed, 77 insertions(+), 60 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > > index 27278010ecec..fe2e0d765be9 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > > @@ -182,9 +182,8 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
> > > > >  	return 0;
> > > > >  }
> > > > >
> > > > > -static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
> > > > > -				   phys_addr_t phys_addr,
> > > > > -				   u64 pci_addr, size_t size)
> > > > > +static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep,
> > > > > +				   struct dw_pcie_ob_atu_cfg *atu)
> > > > >  {
> > > > >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > >  	u32 free_win;
> > > > > @@ -196,13 +195,13 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
> > > > >  		return -EINVAL;
> > > > >  	}
> > > > >
> > > > > -	ret = dw_pcie_prog_ep_outbound_atu(pci, func_no, free_win, PCIE_ATU_TYPE_MEM,
> > > > > -					   phys_addr, pci_addr, size);
> > > > > +	atu->index = free_win;
> > > > > +	ret = dw_pcie_prog_outbound_atu(pci, atu);
> > > > >  	if (ret)
> > > > >  		return ret;
> > > > >
> > > > >  	set_bit(free_win, ep->ob_window_map);
> > > > > -	ep->outbound_addr[free_win] = phys_addr;
> > > > > +	ep->outbound_addr[free_win] = atu->cpu_addr;
> > > > >
> > > > >  	return 0;
> > > > >  }
> > > > > @@ -305,8 +304,14 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> > > > >  	int ret;
> > > > >  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> > > > >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > > -
> > > > > -	ret = dw_pcie_ep_outbound_atu(ep, func_no, addr, pci_addr, size);
> > > > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > > > > +
> > > > > +	atu.func_no = func_no;
> > > > > +	atu.type = PCIE_ATU_TYPE_MEM;
> > > > > +	atu.cpu_addr = addr;
> > > > > +	atu.pci_addr = pci_addr;
> > > > > +	atu.size = size;
> > > > > +	ret = dw_pcie_ep_outbound_atu(ep, &atu);
> > > > >  	if (ret) {
> > > > >  		dev_err(pci->dev, "Failed to enable address\n");
> > > > >  		return ret;
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > index cf61733bf78d..7419185721f2 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > @@ -549,6 +549,7 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> > > > >  {
> > > > >  	struct dw_pcie_rp *pp = bus->sysdata;
> > > > >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > > > >  	int type, ret;
> > > > >  	u32 busdev;
> > > > >
> > > > > @@ -571,8 +572,12 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> > > > >  	else
> > > > >  		type = PCIE_ATU_TYPE_CFG1;
> > > > >
> > > > > -	ret = dw_pcie_prog_outbound_atu(pci, 0, type, pp->cfg0_base, busdev,
> > > > > -					pp->cfg0_size);
> > > > > +	atu.type = type;
> > > > > +	atu.cpu_addr = pp->cfg0_base;
> > > > > +	atu.pci_addr = busdev;
> > > > > +	atu.size = pp->cfg0_size;
> > > > > +
> > > > > +	ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > > > >  	if (ret)
> > > > >  		return NULL;
> > > > >
> > > > > @@ -584,6 +589,7 @@ static int dw_pcie_rd_other_conf(struct pci_bus *bus, unsigned int devfn,
> > > > >  {
> > > > >  	struct dw_pcie_rp *pp = bus->sysdata;
> > > > >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > > > >  	int ret;
> > > > >
> > > > >  	ret = pci_generic_config_read(bus, devfn, where, size, val);
> > > > > @@ -591,9 +597,12 @@ static int dw_pcie_rd_other_conf(struct pci_bus *bus, unsigned int devfn,
> > > > >  		return ret;
> > > > >
> > > > >  	if (pp->cfg0_io_shared) {
> > > > > -		ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
> > > > > -						pp->io_base, pp->io_bus_addr,
> > > > > -						pp->io_size);
> > > > > +		atu.type = PCIE_ATU_TYPE_IO;
> > > > > +		atu.cpu_addr = pp->io_base;
> > > > > +		atu.pci_addr = pp->io_bus_addr;
> > > > > +		atu.size = pp->io_size;
> > > > > +
> > > > > +		ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > > > >  		if (ret)
> > > > >  			return PCIBIOS_SET_FAILED;
> > > > >  	}
> > > > > @@ -606,6 +615,7 @@ static int dw_pcie_wr_other_conf(struct pci_bus *bus, unsigned int devfn,
> > > > >  {
> > > > >  	struct dw_pcie_rp *pp = bus->sysdata;
> > > > >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > > > >  	int ret;
> > > > >
> > > > >  	ret = pci_generic_config_write(bus, devfn, where, size, val);
> > > > > @@ -613,9 +623,12 @@ static int dw_pcie_wr_other_conf(struct pci_bus *bus, unsigned int devfn,
> > > > >  		return ret;
> > > > >
> > > > >  	if (pp->cfg0_io_shared) {
> > > > > -		ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
> > > > > -						pp->io_base, pp->io_bus_addr,
> > > > > -						pp->io_size);
> > > > > +		atu.type = PCIE_ATU_TYPE_IO;
> > > > > +		atu.cpu_addr = pp->io_base;
> > > > > +		atu.pci_addr = pp->io_bus_addr;
> > > > > +		atu.size = pp->io_size;
> > > > > +
> > > > > +		ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > > > >  		if (ret)
> > > > >  			return PCIBIOS_SET_FAILED;
> > > > >  	}
> > > > > @@ -650,6 +663,7 @@ static struct pci_ops dw_pcie_ops = {
> > > > >  static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> > > > >  {
> > > > >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > > > >  	struct resource_entry *entry;
> > > > >  	int i, ret;
> > > > >
> > > > > @@ -677,10 +691,13 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> > > > >  		if (pci->num_ob_windows <= ++i)
> > > > >  			break;
> > > > >
> > > > > -		ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
> > > > > -						entry->res->start,
> > > > > -						entry->res->start - entry->offset,
> > > > > -						resource_size(entry->res));
> > > > > +		atu.index = i;
> > > > > +		atu.type = PCIE_ATU_TYPE_MEM;
> > > > > +		atu.cpu_addr = entry->res->start;
> > > > > +		atu.pci_addr = entry->res->start - entry->offset;
> > > > > +		atu.size = resource_size(entry->res);
> > > > > +
> > > > > +		ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > > > >  		if (ret) {
> > > > >  			dev_err(pci->dev, "Failed to set MEM range %pr\n",
> > > > >  				entry->res);
> > > > > @@ -690,10 +707,13 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> > > > >
> > > > >  	if (pp->io_size) {
> > > > >  		if (pci->num_ob_windows > ++i) {
> > > > > -			ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_IO,
> > > > > -							pp->io_base,
> > > > > -							pp->io_bus_addr,
> > > > > -							pp->io_size);
> > > > > +			atu.index = i;
> > > > > +			atu.type = PCIE_ATU_TYPE_IO;
> > > > > +			atu.cpu_addr = pp->io_base;
> > > > > +			atu.pci_addr = pp->io_bus_addr;
> > > > > +			atu.size = pp->io_size;
> > > > > +
> > > > > +			ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > > > >  			if (ret) {
> > > > >  				dev_err(pci->dev, "Failed to set IO range %pr\n",
> > > > >  					entry->res);
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > index 2459f2a61b9b..49b785509576 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > @@ -464,56 +464,56 @@ static inline u32 dw_pcie_enable_ecrc(u32 val)
> > > > >  	return val | PCIE_ATU_TD;
> > > > >  }
> > > > >
> > > > > -static int __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> > > > > -				       int index, int type, u64 cpu_addr,
> > > > > -				       u64 pci_addr, u64 size)
> > > > > +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> > > > > +			      const struct dw_pcie_ob_atu_cfg *atu)
> > > > >  {
> > > > > +	u64 cpu_addr = atu->cpu_addr;
> > > > >  	u32 retries, val;
> > > > >  	u64 limit_addr;
> > > > >
> > > > >  	if (pci->ops && pci->ops->cpu_addr_fixup)
> > > > >  		cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
> > > > >
> > > > > -	limit_addr = cpu_addr + size - 1;
> > > > > +	limit_addr = cpu_addr + atu->size - 1;
> > > > >
> > > > >  	if ((limit_addr & ~pci->region_limit) != (cpu_addr & ~pci->region_limit) ||
> > > > >  	    !IS_ALIGNED(cpu_addr, pci->region_align) ||
> > > > > -	    !IS_ALIGNED(pci_addr, pci->region_align) || !size) {
> > > > > +	    !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size) {
> > > > >  		return -EINVAL;
> > > > >  	}
> > > > >
> > > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_BASE,
> > > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_BASE,
> > > > >  			      lower_32_bits(cpu_addr));
> > > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_BASE,
> > > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_BASE,
> > > > >  			      upper_32_bits(cpu_addr));
> > > > >
> > > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LIMIT,
> > > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LIMIT,
> > > > >  			      lower_32_bits(limit_addr));
> > > > >  	if (dw_pcie_ver_is_ge(pci, 460A))
> > > > > -		dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_LIMIT,
> > > > > +		dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_LIMIT,
> > > > >  				      upper_32_bits(limit_addr));
> > > > >
> > > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_TARGET,
> > > > > -			      lower_32_bits(pci_addr));
> > > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_TARGET,
> > > > > -			      upper_32_bits(pci_addr));
> > > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_TARGET,
> > > > > +			      lower_32_bits(atu->pci_addr));
> > > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_TARGET,
> > > > > +			      upper_32_bits(atu->pci_addr));
> > > > >
> > > > > -	val = type | PCIE_ATU_FUNC_NUM(func_no);
> > > > > +	val = atu->type | PCIE_ATU_FUNC_NUM(atu->func_no);
> > > > >  	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) &&
> > > > >  	    dw_pcie_ver_is_ge(pci, 460A))
> > > > >  		val |= PCIE_ATU_INCREASE_REGION_SIZE;
> > > > >  	if (dw_pcie_ver_is(pci, 490A))
> > > > >  		val = dw_pcie_enable_ecrc(val);
> > > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL1, val);
> > > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL1, val);
> > > > >
> > > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
> > > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
> > > > >
> > > > >  	/*
> > > > >  	 * Make sure ATU enable takes effect before any subsequent config
> > > > >  	 * and I/O accesses.
> > > > >  	 */
> > > > >  	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
> > > > > -		val = dw_pcie_readl_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2);
> > > > > +		val = dw_pcie_readl_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2);
> > > > >  		if (val & PCIE_ATU_ENABLE)
> > > > >  			return 0;
> > > > >
> > > > > @@ -525,21 +525,6 @@ static int __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> > > > >  	return -ETIMEDOUT;
> > > > >  }
> > > > >
> > > > > -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > > > > -			      u64 cpu_addr, u64 pci_addr, u64 size)
> > > > > -{
> > > > > -	return __dw_pcie_prog_outbound_atu(pci, 0, index, type,
> > > > > -					   cpu_addr, pci_addr, size);
> > > > > -}
> > > > > -
> > > > > -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > > -				 int type, u64 cpu_addr, u64 pci_addr,
> > > > > -				 u64 size)
> > > > > -{
> > > > > -	return __dw_pcie_prog_outbound_atu(pci, func_no, index, type,
> > > > > -					   cpu_addr, pci_addr, size);
> > > > > -}
> > > > > -
> > > > >  static inline u32 dw_pcie_readl_atu_ib(struct dw_pcie *pci, u32 index, u32 reg)
> > > > >  {
> > > > >  	return dw_pcie_readl_atu(pci, PCIE_ATU_REGION_DIR_IB, index, reg);
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > > > index 3c06e025c905..85de0d8346fa 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > > > @@ -288,6 +288,15 @@ enum dw_pcie_core_rst {
> > > > >  	DW_PCIE_NUM_CORE_RSTS
> > > > >  };
> > > > >
> > > > > +struct dw_pcie_ob_atu_cfg {
> > > > > +	int index;
> > > > > +	int type;
> > > > > +	u8 func_no;
> > > > > +	u64 cpu_addr;
> > > > > +	u64 pci_addr;
> > > > > +	u64 size;
> > > > > +};
> > > > > +
> > > > >  struct dw_pcie_host_ops {
> > > > >  	int (*host_init)(struct dw_pcie_rp *pp);
> > > > >  	void (*host_deinit)(struct dw_pcie_rp *pp);
> > > > > @@ -416,10 +425,8 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
> > > > >  int dw_pcie_link_up(struct dw_pcie *pci);
> > > > >  void dw_pcie_upconfig_setup(struct dw_pcie *pci);
> > > > >  int dw_pcie_wait_for_link(struct dw_pcie *pci);
> > > > > -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > > > > -			      u64 cpu_addr, u64 pci_addr, u64 size);
> > > > > -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > > -				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
> > > > > +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> > > > > +			      const struct dw_pcie_ob_atu_cfg *atu);
> > > > >  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> > > > >  			     u64 cpu_addr, u64 pci_addr, u64 size);
> > > > >  int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > > --
> > > > > 2.25.1
> > > > >

Yoshihiro Shimoda Aug. 1, 2023, 7:02 a.m. UTC | #13

Hi Serge,

> From: Serge Semin, Sent: Tuesday, August 1, 2023 10:45 AM
> 
> On Tue, Aug 01, 2023 at 01:29:10AM +0000, Yoshihiro Shimoda wrote:
> > Hi Serge,
> >
> > > From: Serge Semin, Sent: Tuesday, August 1, 2023 6:33 AM
> > >
> > > On Mon, Jul 31, 2023 at 01:24:27AM +0000, Yoshihiro Shimoda wrote:
> > > > Hi Serge,
> > > >
> > > > > From: Serge Semin, Sent: Saturday, July 29, 2023 11:07 AM
> > > > >
> > > > > On Fri, Jul 21, 2023 at 04:44:36PM +0900, Yoshihiro Shimoda wrote:
> > > > > > The __dw_pcie_prog_outbound_atu() currently has 6 arguments.
> > > > > > To support INTx IRQs in the future, it requires an additional 2
> > > > > > arguments. For improved code readability, introduce the struct
> > > > > > dw_pcie_ob_atu_cfg and update the arguments of
> > > > > > dw_pcie_prog_outbound_atu().
> > > > > >
> > > > > > Consequently, remove __dw_pcie_prog_outbound_atu() and
> > > > > > dw_pcie_prog_ep_outbound_atu() because there is no longer
> > > > > > a need.
> > > > > >
> > > > > > No behavior changes.
> > > > >
> > > > > So you decided not to use a suggested by me in v17 more detailed patch
> > > > > log?
> > > >
> > > > You're correct. I thought your suggested comments was too detailed.
> > >
> > > I strongly recommend for you to use mine instead. It gives more
> > > details about the change and the patch context. Moreover it much more
> > > clearer justifies the change implemented in the patch.
> >
> 
> > I didn't realize that you have a strong recommendation about the comments
> > you suggested. I'll replace the commit description and add your Suggested-by
> > tag on v19.
> 
> Just to note if there is a misunderstanding on your side. Suggested-by tag is
> relevant to the patch idea in general.
> See Documentation/process/submitting-patches.rst:559 for details.
> So you don't need to add the tag if somebody just suggested an
> alternative patch description.

Thank you for your comments. So, I will not add Suggested-by tag.

Best regards,
Yoshihiro Shimoda

> -Serge(y)
> 
> >
> > Best regards,
> > Yoshihiro Shimoda
> >
> > > -Serge(y)
> > >
> > > >
> > > > Best regards,
> > > > Yoshihiro Shimoda
> > > >
> > > > > C&P it here just in case if you change your mind:
> > > > >
> > > > > This is a preparation before adding the Msg-type outbound iATU
> > > > > mapping. The respective update will require two more arguments added
> > > > > to __dw_pcie_prog_outbound_atu(). That will make the already
> > > > > complicated function prototype even more hard to comprehend accepting
> > > > > _eight_ arguments. In order to prevent that and keep the code
> > > > > more-or-less readable all the outbound iATU-related arguments are
> > > > > moved to the new config-structure: struct dw_pcie_ob_atu_cfg pointer
> > > > > to which shall be passed to dw_pcie_prog_outbound_atu(). The structure
> > > > > is supposed to be locally defined and populated with the outbound iATU
> > > > > settings implied by the caller context.
> > > > >
> > > > > As a result of the denoted change there is no longer need in having
> > > > > the two distinctive methods for the Host and End-point outbound iATU
> > > > > setups since the corresponding code can directly call the
> > > > > dw_pcie_prog_outbound_atu() method with the config-structure
> > > > > populated. Thus dw_pcie_prog_ep_outbound_atu() is dropped.
> > > > >
> > > > > -Serge(y)
> > > > >
> > > > > >
> > > > > > Signed-off-by: Yoshihiro Shimoda <[e-mail]>
> > > > > > Reviewed-by: Serge Semin <[e-mail]>
> > > > > > ---
> > > > > >  .../pci/controller/dwc/pcie-designware-ep.c   | 21 +++++---
> > > > > >  .../pci/controller/dwc/pcie-designware-host.c | 52 +++++++++++++------
> > > > > >  drivers/pci/controller/dwc/pcie-designware.c  | 49 ++++++-----------
> > > > > >  drivers/pci/controller/dwc/pcie-designware.h  | 15 ++++--
> > > > > >  4 files changed, 77 insertions(+), 60 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > > > index 27278010ecec..fe2e0d765be9 100644
> > > > > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > > > @@ -182,9 +182,8 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
> > > > > >  	return 0;
> > > > > >  }
> > > > > >
> > > > > > -static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
> > > > > > -				   phys_addr_t phys_addr,
> > > > > > -				   u64 pci_addr, size_t size)
> > > > > > +static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep,
> > > > > > +				   struct dw_pcie_ob_atu_cfg *atu)
> > > > > >  {
> > > > > >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > > >  	u32 free_win;
> > > > > > @@ -196,13 +195,13 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
> > > > > >  		return -EINVAL;
> > > > > >  	}
> > > > > >
> > > > > > -	ret = dw_pcie_prog_ep_outbound_atu(pci, func_no, free_win, PCIE_ATU_TYPE_MEM,
> > > > > > -					   phys_addr, pci_addr, size);
> > > > > > +	atu->index = free_win;
> > > > > > +	ret = dw_pcie_prog_outbound_atu(pci, atu);
> > > > > >  	if (ret)
> > > > > >  		return ret;
> > > > > >
> > > > > >  	set_bit(free_win, ep->ob_window_map);
> > > > > > -	ep->outbound_addr[free_win] = phys_addr;
> > > > > > +	ep->outbound_addr[free_win] = atu->cpu_addr;
> > > > > >
> > > > > >  	return 0;
> > > > > >  }
> > > > > > @@ -305,8 +304,14 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> > > > > >  	int ret;
> > > > > >  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> > > > > >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > > > -
> > > > > > -	ret = dw_pcie_ep_outbound_atu(ep, func_no, addr, pci_addr, size);
> > > > > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > > > > > +
> > > > > > +	atu.func_no = func_no;
> > > > > > +	atu.type = PCIE_ATU_TYPE_MEM;
> > > > > > +	atu.cpu_addr = addr;
> > > > > > +	atu.pci_addr = pci_addr;
> > > > > > +	atu.size = size;
> > > > > > +	ret = dw_pcie_ep_outbound_atu(ep, &atu);
> > > > > >  	if (ret) {
> > > > > >  		dev_err(pci->dev, "Failed to enable address\n");
> > > > > >  		return ret;
> > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > index cf61733bf78d..7419185721f2 100644
> > > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > @@ -549,6 +549,7 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> > > > > >  {
> > > > > >  	struct dw_pcie_rp *pp = bus->sysdata;
> > > > > >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > > > > >  	int type, ret;
> > > > > >  	u32 busdev;
> > > > > >
> > > > > > @@ -571,8 +572,12 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> > > > > >  	else
> > > > > >  		type = PCIE_ATU_TYPE_CFG1;
> > > > > >
> > > > > > -	ret = dw_pcie_prog_outbound_atu(pci, 0, type, pp->cfg0_base, busdev,
> > > > > > -					pp->cfg0_size);
> > > > > > +	atu.type = type;
> > > > > > +	atu.cpu_addr = pp->cfg0_base;
> > > > > > +	atu.pci_addr = busdev;
> > > > > > +	atu.size = pp->cfg0_size;
> > > > > > +
> > > > > > +	ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > > > > >  	if (ret)
> > > > > >  		return NULL;
> > > > > >
> > > > > > @@ -584,6 +589,7 @@ static int dw_pcie_rd_other_conf(struct pci_bus *bus, unsigned int devfn,
> > > > > >  {
> > > > > >  	struct dw_pcie_rp *pp = bus->sysdata;
> > > > > >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > > > > >  	int ret;
> > > > > >
> > > > > >  	ret = pci_generic_config_read(bus, devfn, where, size, val);
> > > > > > @@ -591,9 +597,12 @@ static int dw_pcie_rd_other_conf(struct pci_bus *bus, unsigned int devfn,
> > > > > >  		return ret;
> > > > > >
> > > > > >  	if (pp->cfg0_io_shared) {
> > > > > > -		ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
> > > > > > -						pp->io_base, pp->io_bus_addr,
> > > > > > -						pp->io_size);
> > > > > > +		atu.type = PCIE_ATU_TYPE_IO;
> > > > > > +		atu.cpu_addr = pp->io_base;
> > > > > > +		atu.pci_addr = pp->io_bus_addr;
> > > > > > +		atu.size = pp->io_size;
> > > > > > +
> > > > > > +		ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > > > > >  		if (ret)
> > > > > >  			return PCIBIOS_SET_FAILED;
> > > > > >  	}
> > > > > > @@ -606,6 +615,7 @@ static int dw_pcie_wr_other_conf(struct pci_bus *bus, unsigned int devfn,
> > > > > >  {
> > > > > >  	struct dw_pcie_rp *pp = bus->sysdata;
> > > > > >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > > > > >  	int ret;
> > > > > >
> > > > > >  	ret = pci_generic_config_write(bus, devfn, where, size, val);
> > > > > > @@ -613,9 +623,12 @@ static int dw_pcie_wr_other_conf(struct pci_bus *bus, unsigned int devfn,
> > > > > >  		return ret;
> > > > > >
> > > > > >  	if (pp->cfg0_io_shared) {
> > > > > > -		ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
> > > > > > -						pp->io_base, pp->io_bus_addr,
> > > > > > -						pp->io_size);
> > > > > > +		atu.type = PCIE_ATU_TYPE_IO;
> > > > > > +		atu.cpu_addr = pp->io_base;
> > > > > > +		atu.pci_addr = pp->io_bus_addr;
> > > > > > +		atu.size = pp->io_size;
> > > > > > +
> > > > > > +		ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > > > > >  		if (ret)
> > > > > >  			return PCIBIOS_SET_FAILED;
> > > > > >  	}
> > > > > > @@ -650,6 +663,7 @@ static struct pci_ops dw_pcie_ops = {
> > > > > >  static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> > > > > >  {
> > > > > >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > > > > >  	struct resource_entry *entry;
> > > > > >  	int i, ret;
> > > > > >
> > > > > > @@ -677,10 +691,13 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> > > > > >  		if (pci->num_ob_windows <= ++i)
> > > > > >  			break;
> > > > > >
> > > > > > -		ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
> > > > > > -						entry->res->start,
> > > > > > -						entry->res->start - entry->offset,
> > > > > > -						resource_size(entry->res));
> > > > > > +		atu.index = i;
> > > > > > +		atu.type = PCIE_ATU_TYPE_MEM;
> > > > > > +		atu.cpu_addr = entry->res->start;
> > > > > > +		atu.pci_addr = entry->res->start - entry->offset;
> > > > > > +		atu.size = resource_size(entry->res);
> > > > > > +
> > > > > > +		ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > > > > >  		if (ret) {
> > > > > >  			dev_err(pci->dev, "Failed to set MEM range %pr\n",
> > > > > >  				entry->res);
> > > > > > @@ -690,10 +707,13 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> > > > > >
> > > > > >  	if (pp->io_size) {
> > > > > >  		if (pci->num_ob_windows > ++i) {
> > > > > > -			ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_IO,
> > > > > > -							pp->io_base,
> > > > > > -							pp->io_bus_addr,
> > > > > > -							pp->io_size);
> > > > > > +			atu.index = i;
> > > > > > +			atu.type = PCIE_ATU_TYPE_IO;
> > > > > > +			atu.cpu_addr = pp->io_base;
> > > > > > +			atu.pci_addr = pp->io_bus_addr;
> > > > > > +			atu.size = pp->io_size;
> > > > > > +
> > > > > > +			ret = dw_pcie_prog_outbound_atu(pci, &atu);
> > > > > >  			if (ret) {
> > > > > >  				dev_err(pci->dev, "Failed to set IO range %pr\n",
> > > > > >  					entry->res);
> > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > > index 2459f2a61b9b..49b785509576 100644
> > > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > > @@ -464,56 +464,56 @@ static inline u32 dw_pcie_enable_ecrc(u32 val)
> > > > > >  	return val | PCIE_ATU_TD;
> > > > > >  }
> > > > > >
> > > > > > -static int __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> > > > > > -				       int index, int type, u64 cpu_addr,
> > > > > > -				       u64 pci_addr, u64 size)
> > > > > > +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> > > > > > +			      const struct dw_pcie_ob_atu_cfg *atu)
> > > > > >  {
> > > > > > +	u64 cpu_addr = atu->cpu_addr;
> > > > > >  	u32 retries, val;
> > > > > >  	u64 limit_addr;
> > > > > >
> > > > > >  	if (pci->ops && pci->ops->cpu_addr_fixup)
> > > > > >  		cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
> > > > > >
> > > > > > -	limit_addr = cpu_addr + size - 1;
> > > > > > +	limit_addr = cpu_addr + atu->size - 1;
> > > > > >
> > > > > >  	if ((limit_addr & ~pci->region_limit) != (cpu_addr & ~pci->region_limit) ||
> > > > > >  	    !IS_ALIGNED(cpu_addr, pci->region_align) ||
> > > > > > -	    !IS_ALIGNED(pci_addr, pci->region_align) || !size) {
> > > > > > +	    !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size) {
> > > > > >  		return -EINVAL;
> > > > > >  	}
> > > > > >
> > > > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_BASE,
> > > > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_BASE,
> > > > > >  			      lower_32_bits(cpu_addr));
> > > > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_BASE,
> > > > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_BASE,
> > > > > >  			      upper_32_bits(cpu_addr));
> > > > > >
> > > > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LIMIT,
> > > > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LIMIT,
> > > > > >  			      lower_32_bits(limit_addr));
> > > > > >  	if (dw_pcie_ver_is_ge(pci, 460A))
> > > > > > -		dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_LIMIT,
> > > > > > +		dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_LIMIT,
> > > > > >  				      upper_32_bits(limit_addr));
> > > > > >
> > > > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_TARGET,
> > > > > > -			      lower_32_bits(pci_addr));
> > > > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_TARGET,
> > > > > > -			      upper_32_bits(pci_addr));
> > > > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_TARGET,
> > > > > > +			      lower_32_bits(atu->pci_addr));
> > > > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_TARGET,
> > > > > > +			      upper_32_bits(atu->pci_addr));
> > > > > >
> > > > > > -	val = type | PCIE_ATU_FUNC_NUM(func_no);
> > > > > > +	val = atu->type | PCIE_ATU_FUNC_NUM(atu->func_no);
> > > > > >  	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) &&
> > > > > >  	    dw_pcie_ver_is_ge(pci, 460A))
> > > > > >  		val |= PCIE_ATU_INCREASE_REGION_SIZE;
> > > > > >  	if (dw_pcie_ver_is(pci, 490A))
> > > > > >  		val = dw_pcie_enable_ecrc(val);
> > > > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL1, val);
> > > > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL1, val);
> > > > > >
> > > > > > -	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
> > > > > > +	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
> > > > > >
> > > > > >  	/*
> > > > > >  	 * Make sure ATU enable takes effect before any subsequent config
> > > > > >  	 * and I/O accesses.
> > > > > >  	 */
> > > > > >  	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
> > > > > > -		val = dw_pcie_readl_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2);
> > > > > > +		val = dw_pcie_readl_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2);
> > > > > >  		if (val & PCIE_ATU_ENABLE)
> > > > > >  			return 0;
> > > > > >
> > > > > > @@ -525,21 +525,6 @@ static int __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> > > > > >  	return -ETIMEDOUT;
> > > > > >  }
> > > > > >
> > > > > > -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > > > > > -			      u64 cpu_addr, u64 pci_addr, u64 size)
> > > > > > -{
> > > > > > -	return __dw_pcie_prog_outbound_atu(pci, 0, index, type,
> > > > > > -					   cpu_addr, pci_addr, size);
> > > > > > -}
> > > > > > -
> > > > > > -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > > > -				 int type, u64 cpu_addr, u64 pci_addr,
> > > > > > -				 u64 size)
> > > > > > -{
> > > > > > -	return __dw_pcie_prog_outbound_atu(pci, func_no, index, type,
> > > > > > -					   cpu_addr, pci_addr, size);
> > > > > > -}
> > > > > > -
> > > > > >  static inline u32 dw_pcie_readl_atu_ib(struct dw_pcie *pci, u32 index, u32 reg)
> > > > > >  {
> > > > > >  	return dw_pcie_readl_atu(pci, PCIE_ATU_REGION_DIR_IB, index, reg);
> > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > > > > index 3c06e025c905..85de0d8346fa 100644
> > > > > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > > > > @@ -288,6 +288,15 @@ enum dw_pcie_core_rst {
> > > > > >  	DW_PCIE_NUM_CORE_RSTS
> > > > > >  };
> > > > > >
> > > > > > +struct dw_pcie_ob_atu_cfg {
> > > > > > +	int index;
> > > > > > +	int type;
> > > > > > +	u8 func_no;
> > > > > > +	u64 cpu_addr;
> > > > > > +	u64 pci_addr;
> > > > > > +	u64 size;
> > > > > > +};
> > > > > > +
> > > > > >  struct dw_pcie_host_ops {
> > > > > >  	int (*host_init)(struct dw_pcie_rp *pp);
> > > > > >  	void (*host_deinit)(struct dw_pcie_rp *pp);
> > > > > > @@ -416,10 +425,8 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
> > > > > >  int dw_pcie_link_up(struct dw_pcie *pci);
> > > > > >  void dw_pcie_upconfig_setup(struct dw_pcie *pci);
> > > > > >  int dw_pcie_wait_for_link(struct dw_pcie *pci);
> > > > > > -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > > > > > -			      u64 cpu_addr, u64 pci_addr, u64 size);
> > > > > > -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > > > -				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
> > > > > > +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> > > > > > +			      const struct dw_pcie_ob_atu_cfg *atu);
> > > > > >  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> > > > > >  			     u64 cpu_addr, u64 pci_addr, u64 size);
> > > > > >  int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > > > --
> > > > > > 2.25.1
> > > > > >

Patch

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 27278010ecec..fe2e0d765be9 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -182,9 +182,8 @@  static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
 	return 0;
 }
 
-static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
-				   phys_addr_t phys_addr,
-				   u64 pci_addr, size_t size)
+static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep,
+				   struct dw_pcie_ob_atu_cfg *atu)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 	u32 free_win;
@@ -196,13 +195,13 @@  static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
 		return -EINVAL;
 	}
 
-	ret = dw_pcie_prog_ep_outbound_atu(pci, func_no, free_win, PCIE_ATU_TYPE_MEM,
-					   phys_addr, pci_addr, size);
+	atu->index = free_win;
+	ret = dw_pcie_prog_outbound_atu(pci, atu);
 	if (ret)
 		return ret;
 
 	set_bit(free_win, ep->ob_window_map);
-	ep->outbound_addr[free_win] = phys_addr;
+	ep->outbound_addr[free_win] = atu->cpu_addr;
 
 	return 0;
 }
@@ -305,8 +304,14 @@  static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 	int ret;
 	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
-
-	ret = dw_pcie_ep_outbound_atu(ep, func_no, addr, pci_addr, size);
+	struct dw_pcie_ob_atu_cfg atu = { 0 };
+
+	atu.func_no = func_no;
+	atu.type = PCIE_ATU_TYPE_MEM;
+	atu.cpu_addr = addr;
+	atu.pci_addr = pci_addr;
+	atu.size = size;
+	ret = dw_pcie_ep_outbound_atu(ep, &atu);
 	if (ret) {
 		dev_err(pci->dev, "Failed to enable address\n");
 		return ret;
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index cf61733bf78d..7419185721f2 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -549,6 +549,7 @@  static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
 {
 	struct dw_pcie_rp *pp = bus->sysdata;
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct dw_pcie_ob_atu_cfg atu = { 0 };
 	int type, ret;
 	u32 busdev;
 
@@ -571,8 +572,12 @@  static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
 	else
 		type = PCIE_ATU_TYPE_CFG1;
 
-	ret = dw_pcie_prog_outbound_atu(pci, 0, type, pp->cfg0_base, busdev,
-					pp->cfg0_size);
+	atu.type = type;
+	atu.cpu_addr = pp->cfg0_base;
+	atu.pci_addr = busdev;
+	atu.size = pp->cfg0_size;
+
+	ret = dw_pcie_prog_outbound_atu(pci, &atu);
 	if (ret)
 		return NULL;
 
@@ -584,6 +589,7 @@  static int dw_pcie_rd_other_conf(struct pci_bus *bus, unsigned int devfn,
 {
 	struct dw_pcie_rp *pp = bus->sysdata;
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct dw_pcie_ob_atu_cfg atu = { 0 };
 	int ret;
 
 	ret = pci_generic_config_read(bus, devfn, where, size, val);
@@ -591,9 +597,12 @@  static int dw_pcie_rd_other_conf(struct pci_bus *bus, unsigned int devfn,
 		return ret;
 
 	if (pp->cfg0_io_shared) {
-		ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
-						pp->io_base, pp->io_bus_addr,
-						pp->io_size);
+		atu.type = PCIE_ATU_TYPE_IO;
+		atu.cpu_addr = pp->io_base;
+		atu.pci_addr = pp->io_bus_addr;
+		atu.size = pp->io_size;
+
+		ret = dw_pcie_prog_outbound_atu(pci, &atu);
 		if (ret)
 			return PCIBIOS_SET_FAILED;
 	}
@@ -606,6 +615,7 @@  static int dw_pcie_wr_other_conf(struct pci_bus *bus, unsigned int devfn,
 {
 	struct dw_pcie_rp *pp = bus->sysdata;
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct dw_pcie_ob_atu_cfg atu = { 0 };
 	int ret;
 
 	ret = pci_generic_config_write(bus, devfn, where, size, val);
@@ -613,9 +623,12 @@  static int dw_pcie_wr_other_conf(struct pci_bus *bus, unsigned int devfn,
 		return ret;
 
 	if (pp->cfg0_io_shared) {
-		ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
-						pp->io_base, pp->io_bus_addr,
-						pp->io_size);
+		atu.type = PCIE_ATU_TYPE_IO;
+		atu.cpu_addr = pp->io_base;
+		atu.pci_addr = pp->io_bus_addr;
+		atu.size = pp->io_size;
+
+		ret = dw_pcie_prog_outbound_atu(pci, &atu);
 		if (ret)
 			return PCIBIOS_SET_FAILED;
 	}
@@ -650,6 +663,7 @@  static struct pci_ops dw_pcie_ops = {
 static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct dw_pcie_ob_atu_cfg atu = { 0 };
 	struct resource_entry *entry;
 	int i, ret;
 
@@ -677,10 +691,13 @@  static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 		if (pci->num_ob_windows <= ++i)
 			break;
 
-		ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
-						entry->res->start,
-						entry->res->start - entry->offset,
-						resource_size(entry->res));
+		atu.index = i;
+		atu.type = PCIE_ATU_TYPE_MEM;
+		atu.cpu_addr = entry->res->start;
+		atu.pci_addr = entry->res->start - entry->offset;
+		atu.size = resource_size(entry->res);
+
+		ret = dw_pcie_prog_outbound_atu(pci, &atu);
 		if (ret) {
 			dev_err(pci->dev, "Failed to set MEM range %pr\n",
 				entry->res);
@@ -690,10 +707,13 @@  static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 
 	if (pp->io_size) {
 		if (pci->num_ob_windows > ++i) {
-			ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_IO,
-							pp->io_base,
-							pp->io_bus_addr,
-							pp->io_size);
+			atu.index = i;
+			atu.type = PCIE_ATU_TYPE_IO;
+			atu.cpu_addr = pp->io_base;
+			atu.pci_addr = pp->io_bus_addr;
+			atu.size = pp->io_size;
+
+			ret = dw_pcie_prog_outbound_atu(pci, &atu);
 			if (ret) {
 				dev_err(pci->dev, "Failed to set IO range %pr\n",
 					entry->res);
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 2459f2a61b9b..49b785509576 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -464,56 +464,56 @@  static inline u32 dw_pcie_enable_ecrc(u32 val)
 	return val | PCIE_ATU_TD;
 }
 
-static int __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
-				       int index, int type, u64 cpu_addr,
-				       u64 pci_addr, u64 size)
+int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
+			      const struct dw_pcie_ob_atu_cfg *atu)
 {
+	u64 cpu_addr = atu->cpu_addr;
 	u32 retries, val;
 	u64 limit_addr;
 
 	if (pci->ops && pci->ops->cpu_addr_fixup)
 		cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
 
-	limit_addr = cpu_addr + size - 1;
+	limit_addr = cpu_addr + atu->size - 1;
 
 	if ((limit_addr & ~pci->region_limit) != (cpu_addr & ~pci->region_limit) ||
 	    !IS_ALIGNED(cpu_addr, pci->region_align) ||
-	    !IS_ALIGNED(pci_addr, pci->region_align) || !size) {
+	    !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size) {
 		return -EINVAL;
 	}
 
-	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_BASE,
+	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_BASE,
 			      lower_32_bits(cpu_addr));
-	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_BASE,
+	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_BASE,
 			      upper_32_bits(cpu_addr));
 
-	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LIMIT,
+	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LIMIT,
 			      lower_32_bits(limit_addr));
 	if (dw_pcie_ver_is_ge(pci, 460A))
-		dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_LIMIT,
+		dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_LIMIT,
 				      upper_32_bits(limit_addr));
 
-	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_LOWER_TARGET,
-			      lower_32_bits(pci_addr));
-	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_UPPER_TARGET,
-			      upper_32_bits(pci_addr));
+	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_TARGET,
+			      lower_32_bits(atu->pci_addr));
+	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_TARGET,
+			      upper_32_bits(atu->pci_addr));
 
-	val = type | PCIE_ATU_FUNC_NUM(func_no);
+	val = atu->type | PCIE_ATU_FUNC_NUM(atu->func_no);
 	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) &&
 	    dw_pcie_ver_is_ge(pci, 460A))
 		val |= PCIE_ATU_INCREASE_REGION_SIZE;
 	if (dw_pcie_ver_is(pci, 490A))
 		val = dw_pcie_enable_ecrc(val);
-	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL1, val);
+	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL1, val);
 
-	dw_pcie_writel_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
+	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
 
 	/*
 	 * Make sure ATU enable takes effect before any subsequent config
 	 * and I/O accesses.
 	 */
 	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
-		val = dw_pcie_readl_atu_ob(pci, index, PCIE_ATU_REGION_CTRL2);
+		val = dw_pcie_readl_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL2);
 		if (val & PCIE_ATU_ENABLE)
 			return 0;
 
@@ -525,21 +525,6 @@  static int __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
 	return -ETIMEDOUT;
 }
 
-int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
-			      u64 cpu_addr, u64 pci_addr, u64 size)
-{
-	return __dw_pcie_prog_outbound_atu(pci, 0, index, type,
-					   cpu_addr, pci_addr, size);
-}
-
-int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
-				 int type, u64 cpu_addr, u64 pci_addr,
-				 u64 size)
-{
-	return __dw_pcie_prog_outbound_atu(pci, func_no, index, type,
-					   cpu_addr, pci_addr, size);
-}
-
 static inline u32 dw_pcie_readl_atu_ib(struct dw_pcie *pci, u32 index, u32 reg)
 {
 	return dw_pcie_readl_atu(pci, PCIE_ATU_REGION_DIR_IB, index, reg);
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 3c06e025c905..85de0d8346fa 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -288,6 +288,15 @@  enum dw_pcie_core_rst {
 	DW_PCIE_NUM_CORE_RSTS
 };
 
+struct dw_pcie_ob_atu_cfg {
+	int index;
+	int type;
+	u8 func_no;
+	u64 cpu_addr;
+	u64 pci_addr;
+	u64 size;
+};
+
 struct dw_pcie_host_ops {
 	int (*host_init)(struct dw_pcie_rp *pp);
 	void (*host_deinit)(struct dw_pcie_rp *pp);
@@ -416,10 +425,8 @@  void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
 int dw_pcie_link_up(struct dw_pcie *pci);
 void dw_pcie_upconfig_setup(struct dw_pcie *pci);
 int dw_pcie_wait_for_link(struct dw_pcie *pci);
-int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
-			      u64 cpu_addr, u64 pci_addr, u64 size);
-int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
-				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
+int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
+			      const struct dw_pcie_ob_atu_cfg *atu);
 int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
 			     u64 cpu_addr, u64 pci_addr, u64 size);
 int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,