RE: [PATCH] PCI: dwc: Change the inheritance between the abstracted structures

[Z.q. Hou]

Hi Bjorn,

Thanks a lot for the comments!

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: 2021年4月7日 1:09
> To: Z.q. Hou <zhiqiang.hou@xxxxxxx>
> Cc: linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> lorenzo.pieralisi@xxxxxxx; robh@xxxxxxxxxx; bhelgaas@xxxxxxxxxx;
> Kishon Vijay Abraham I <kishon@xxxxxx>; Jingoo Han
> <jingoohan1@xxxxxxxxx>; Richard Zhu <hongxing.zhu@xxxxxxx>; Lucas
> Stach <l.stach@xxxxxxxxxxxxxx>; Murali Karicheri <m-karicheri2@xxxxxx>;
> M.h. Lian <minghuan.lian@xxxxxxx>; Mingkai Hu <mingkai.hu@xxxxxxx>;
> Roy Zang <roy.zang@xxxxxxx>; Yue Wang <yue.wang@xxxxxxxxxxx>;
> Jonathan Chocron <jonnyc@xxxxxxxxxx>; Thomas Petazzoni
> <thomas.petazzoni@xxxxxxxxxxx>; Jesper Nilsson
> <jesper.nilsson@xxxxxxxx>; Gustavo Pimentel
> <gustavo.pimentel@xxxxxxxxxxxx>; Xiaowei Song
> <songxiaowei@xxxxxxxxxxxxx>; Binghui Wang <wangbinghui@xxxxxxxxxxxxx>;
> Stanimir Varbanov <svarbanov@xxxxxxxxxx>; Pratyush Anand
> <pratyush.anand@xxxxxxxxx>; Kunihiko Hayashi
> <hayashi.kunihiko@xxxxxxxxxxxxx>; Jason Yan <yanaijie@xxxxxxxxxx>;
> Thierry Reding <thierry.reding@xxxxxxxxx>
> Subject: Re: [PATCH] PCI: dwc: Change the inheritance between the
> abstracted structures
> 
> On Tue, Apr 06, 2021 at 05:28:25PM +0800, Zhiqiang Hou wrote:
> > From: Hou Zhiqiang <Zhiqiang.Hou@xxxxxxx>
> >
> > Currently the core struct dw_pcie includes both struct pcie_port and
> > dw_pcie_ep and the RC and EP platform drivers directly includes the
> > dw_pcie. So it results in a RC or EP platform driver has 2 indirect
> > parents pcie_port and dw_pcie_ep, but it doesn't make sense let RC
> > platform driver includes the dw_pcie_ep and so does the EP platform
> > driver.
> >
> > This patch makes the struct pcie_port and dw_pcie_ep includes the core
> > struct dw_pcie and the RC and EP platform drivers include struct
> > pcie_port and dw_pcie_ep respectively.
> 
> I really like the way this patch is heading.  There's a lot of historical cruft in
> these drivers and this is a good step to cleaning it up.  Thanks a lot for
> working on this!
> 
> What does this patch apply to?  It doesn't apply cleanly to either my "main"
> branch or the "next" branch.  Try to send things that apply to "main" and if
> it needs to apply on top of something else, mention what that is.

Sorry, this patch was workout in several months ago, but seems my mailbox failed
to send it out that time, I can rebase it in these days if necessary.

> 
> > diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c
> > b/drivers/pci/controller/dwc/pci-dra7xx.c
> > index 12726c63366f..0e914df6eaba 100644
> > --- a/drivers/pci/controller/dwc/pci-dra7xx.c
> > +++ b/drivers/pci/controller/dwc/pci-dra7xx.c
> > @@ -85,7 +85,8 @@
> >  #define PCIE_B0_B1_TSYNCEN				BIT(0)
> >
> >  struct dra7xx_pcie {
> > -	struct dw_pcie		*pci;
> > +	struct pcie_port	*pp;
> > +	struct dw_pcie_ep	*ep;
> 
> 1) This is not related to your patch, but I think "pcie_port" used to
>    make more sense before we had endpoint drivers, but now it's the
>    wrong name.  Root Ports and Endpoints both have "PCIe Ports", but
>    IIUC "struct pcie_port" only applies to Root Ports, and "struct
>    dw_pcie_ep" is the analogue for Endpoints.
> 
>    It would be nice to coordinate these names with a separate patch,
>    e.g., maybe "dw_pcie_rc" (or "dw_pcie_rp") and "dw_pcie_ep".

Cannot agree more!

> 
> 2) We allocate struct dra7xx_pcie for both RPs and EPs.  But IIUC, RPs
>    only use "struct pcie_port", and EPs only use "struct dw_pcie_ep".
>    It doesn't seem right to keep both pointers when only one is ever
>    used.

The reason is this driver is for both RC mode and EP mode, and they are
using the same private struct dra7xx_pcie.
Do you have some suggestion on this case? Add a union of RC&EP struct
in the driver private struct and uses the corresponding union member
according to the PCIe controller's mode, is it better?

Struct dra7xxx_pcie {
	...
	union {
		struct pcie_port pp;
		struct dw_pcie_ep ep;
	} type;
	...
}


> 
> 3) I'm not sure why these should be pointers at all.  Why can't they
>    be directly embedded, e.g., "struct pcie_port pp" instead of
>    "struct pcie_port *pp"?  Obviously this would have to be done in a
>    way that we allocate an RC-specific structure or an EP-specific
>    one.

Agree, and this change can be done in a separate patch.

> 
> >  	void __iomem		*base;		/* DT ti_conf */
> >  	int			phy_count;	/* DT phy-names count */
> >  	struct phy		**phy;
> 
> > @@ -796,6 +798,17 @@ static int __init dra7xx_pcie_probe(struct
> > platform_device *pdev)
> >
> >  	switch (mode) {
> >  	case DW_PCIE_RC_TYPE:
> > +		pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL);
> 
> We know "mode" right after the of_match_device() at the top of this
> function.  I think we should allocate the RC or EP structure way up there,
> ideally with a single alloc for everything we need (dra7xx_pcie, pcie_port,
> dw_pcie_ep, etc).  That would be fewer allocs and would simplify error
> handling because if the alloc fails we wouldn't have to undo anything.

Put the alloc func here is to avoid adding one more 'switch (mode) {...}'.
This can be refined after changing the '*pp' to directly embedded one.

> 
> > +		if (!pp) {
> > +			ret = -ENOMEM;
> > +			goto err_gpio;
> > +		}
> > +
> > +		pci = &pp->pcie;
> > +		pci->dev = dev;
> > +		pci->ops = &dw_pcie_ops;
> > +		dra7xx->pp = pp;
> > +
> >  		if (!IS_ENABLED(CONFIG_PCI_DRA7XX_HOST)) {
> >  			ret = -ENODEV;
> >  			goto err_gpio;
> > @@ -813,6 +826,17 @@ static int __init dra7xx_pcie_probe(struct
> platform_device *pdev)
> >  			goto err_gpio;
> >  		break;
> >  	case DW_PCIE_EP_TYPE:
> > +		ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
> > +		if (!ep) {
> > +			ret = -ENOMEM;
> > +			goto err_gpio;
> > +		}
> > +
> > +		pci = &ep->pcie;
> > +		pci->dev = dev;
> > +		pci->ops = &dw_pcie_ops;
> > +		dra7xx->ep = ep;
> > +
> >  		if (!IS_ENABLED(CONFIG_PCI_DRA7XX_EP)) {
> >  			ret = -ENODEV;
> >  			goto err_gpio;
> 
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -171,12 +171,44 @@ enum dw_pcie_device_mode {
> >  	DW_PCIE_RC_TYPE,
> >  };
> >
> > +struct dw_pcie_ops {
> > +	u64	(*cpu_addr_fixup)(struct dw_pcie *pcie, u64 cpu_addr);
> > +	u32	(*read_dbi)(struct dw_pcie *pcie, void __iomem *base, u32 reg,
> > +			    size_t size);
> > +	void	(*write_dbi)(struct dw_pcie *pcie, void __iomem *base, u32 reg,
> > +			     size_t size, u32 val);
> > +	void    (*write_dbi2)(struct dw_pcie *pcie, void __iomem *base, u32
> reg,
> > +			      size_t size, u32 val);
> > +	int	(*link_up)(struct dw_pcie *pcie);
> > +	int	(*start_link)(struct dw_pcie *pcie);
> > +	void	(*stop_link)(struct dw_pcie *pcie);
> > +};
> 
> I *think* this is a pure code move.  It would make the patch easier to read
> if you did the move in a separate patch to reduce the size of this one.
> 
> > +struct dw_pcie {
> > +	struct device		*dev;
> > +	void __iomem		*dbi_base;
> > +	void __iomem		*dbi_base2;
> > +	/* Used when iatu_unroll_enabled is true */
> > +	void __iomem		*atu_base;
> > +	size_t			atu_size;
> > +	u32			num_ib_windows;
> > +	u32			num_ob_windows;
> > +	const struct dw_pcie_ops *ops;
> > +	unsigned int		version;
> > +	int			num_lanes;
> > +	int			link_gen;
> > +	u8			n_fts[2];
> > +	bool			iatu_unroll_enabled: 1;
> > +	bool			io_cfg_atu_shared: 1;
> > +};
> 
> Same here.

These code moving is not intended, but to avoid build error of 'incomplete definition',
so these must be changed concurrently.

> 
> >  struct kirin_pcie {
> > -	struct dw_pcie	*pci;
> > -	void __iomem	*apb_base;
> > -	void __iomem	*phy_base;
> > -	struct regmap	*crgctrl;
> > -	struct regmap	*sysctrl;
> > -	struct clk	*apb_sys_clk;
> > -	struct clk	*apb_phy_clk;
> > -	struct clk	*phy_ref_clk;
> > -	struct clk	*pcie_aclk;
> > -	struct clk	*pcie_aux_clk;
> > -	int		gpio_id_reset;
> > +	struct pcie_port	*pp;
> > +	void __iomem		*apb_base;
> > +	void __iomem		*phy_base;
> > +	struct regmap		*crgctrl;
> > +	struct regmap		*sysctrl;
> > +	struct clk		*apb_sys_clk;
> > +	struct clk		*apb_phy_clk;
> > +	struct clk		*phy_ref_clk;
> > +	struct clk		*pcie_aclk;
> > +	struct clk		*pcie_aux_clk;
> > +	int			gpio_id_reset;
> 
> Put reformats like this in a separate patch that doesn't actually change any
> code (no new or deleted members).  Then this patch will be smaller and the
> important changes will be more obvious.

Yes, will change in next version.

Thanks,
Zhiqiang