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

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:

quoted

On Fri, Jul 21, 2023 at 04:44:36PM +0900, Yoshihiro Shimoda wrote:

quoted

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 <yoshihiro.shimoda.uh@renesas.com>

One nit below. With that,

Reviewed-by: Manivannan Sadhasivam <redacted>

quoted

Reviewed-by: Serge Semin <redacted>
---
 .../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(-)

[...]

quoted

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;

quoted

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)

quoted

- Mani

quoted

+};
+
 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

-- 
மணிவண்ணன் சதாசிவம்

-- 
மணிவண்ணன் சதாசிவம்