RE: [PATCH v22 14/16] PCI: dwc: rcar-gen4: Add R-Car Gen4 PCIe Endpoint support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Serge,

> From: Serge Semin, Sent: Monday, September 25, 2023 7:53 PM
> To: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> Cc: lpieralisi@xxxxxxxxxx; kw@xxxxxxxxx; robh@xxxxxxxxxx; bhelgaas@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx; jingoohan1@xxxxxxxxx; gustavo.pimentel@xxxxxxxxxxxx; mani@xxxxxxxxxx;
> marek.vasut+renesas@xxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-renesas-soc@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v22 14/16] PCI: dwc: rcar-gen4: Add R-Car Gen4 PCIe Endpoint support
> 
> On Mon, Sep 25, 2023 at 04:21:28PM +0900, Yoshihiro Shimoda wrote:
> > Add R-Car Gen4 PCIe controller for endpoint mode. This controller is based
> > on Synopsys DesignWare PCIe.
> >
> > Link:
<snip>
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > Signed-off-by: Krzysztof Wilczyński <kwilczynski@xxxxxxxxxx>
> > Reviewed-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> > ---
> >  drivers/pci/controller/dwc/Kconfig          |  11 ++
> >  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 136 +++++++++++++++++++-
> >  2 files changed, 144 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > index bc69fcab2e2a..e7fd37717998 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -429,4 +429,15 @@ config PCIE_RCAR_GEN4_HOST
> >  	  To compile this driver as a module, choose M here: the module will be
> >  	  called pcie-rcar-gen4.ko. This uses the DesignWare core.
> >
> > +config PCIE_RCAR_GEN4_EP
> > +	tristate "Renesas R-Car Gen4 PCIe controller (endpoint mode)"
> > +	depends on ARCH_RENESAS || COMPILE_TEST
> > +	depends on PCI_ENDPOINT
> > +	select PCIE_DW_EP
> > +	select PCIE_RCAR_GEN4
> > +	help
> > +	  Say Y here if you want PCIe controller (endpoint mode) on R-Car Gen4
> > +	  SoCs. To compile this driver as a module, choose M here: the module
> > +	  will be called pcie-rcar-gen4.ko. This uses the DesignWare core.
> > +
> >  endmenu
> > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > index f6b3c3ef187c..d1b31ea909ba 100644
> > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > @@ -45,6 +45,9 @@
> >  #define RCAR_NUM_SPEED_CHANGE_RETRIES	10
> >  #define RCAR_MAX_LINK_SPEED		4
> >
> > +#define RCAR_GEN4_PCIE_EP_FUNC_DBI_OFFSET	0x1000
> > +#define RCAR_GEN4_PCIE_EP_FUNC_DBI2_OFFSET	0x800
> > +
> >  struct rcar_gen4_pcie {
> >  	struct dw_pcie dw;
> >  	void __iomem *base;
> > @@ -53,6 +56,7 @@ struct rcar_gen4_pcie {
> >  };
> >  #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
> >
> > +/* Common */
> >  static void rcar_gen4_pcie_ltssm_enable(struct rcar_gen4_pcie *rcar,
> >  					bool enable)
> >  {
> > @@ -311,6 +315,9 @@ static int rcar_gen4_add_dw_pcie_rp(struct rcar_gen4_pcie *rcar)
> >  {
> >  	struct dw_pcie_rp *pp = &rcar->dw.pp;
> >
> > +	if (!IS_ENABLED(CONFIG_PCIE_RCAR_GEN4_HOST))
> > +		return -ENODEV;
> > +
> >  	pp->num_vectors = MAX_MSI_IRQS;
> >  	pp->ops = &rcar_gen4_pcie_host_ops;
> >  	rcar->mode = DW_PCIE_RC_TYPE;
> > @@ -323,8 +330,114 @@ static void rcar_gen4_remove_dw_pcie_rp(struct rcar_gen4_pcie *rcar)
> >  	dw_pcie_host_deinit(&rcar->dw.pp);
> >  }
> >
> > +/* Endpoind mode */
> > +static void rcar_gen4_pcie_ep_pre_init(struct dw_pcie_ep *ep)
> > +{
> > +	struct dw_pcie *dw = to_dw_pcie_from_ep(ep);
> > +	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> > +	int ret;
> > +
> > +	ret = rcar_gen4_pcie_common_init(rcar);
> > +	if (ret)
> > +		return;
> > +
> > +	writel(PCIEDMAINTSTSEN_INIT, rcar->base + PCIEDMAINTSTSEN);
> > +}
> > +
> > +static void rcar_gen4_pcie_ep_init(struct dw_pcie_ep *ep)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +	enum pci_barno bar;
> > +
> > +	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++)
> > +		dw_pcie_ep_reset_bar(pci, bar);
> > +}
> > +
> > +static void rcar_gen4_pcie_ep_deinit(struct dw_pcie_ep *ep)
> > +{
> > +	struct dw_pcie *dw = to_dw_pcie_from_ep(ep);
> > +	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> > +
> > +	writel(0, rcar->base + PCIEDMAINTSTSEN);
> > +	rcar_gen4_pcie_common_deinit(rcar);
> > +}
> > +
> > +static int rcar_gen4_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> > +				       unsigned int type, u16 interrupt_num)
> > +{
> > +	struct dw_pcie *dw = to_dw_pcie_from_ep(ep);
> > +
> > +	switch (type) {
> > +	case PCI_IRQ_LEGACY:
> > +		return dw_pcie_ep_raise_legacy_irq(ep, func_no);
> > +	case PCI_IRQ_MSI:
> > +		return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
> > +	default:
> > +		dev_err(dw->dev, "Unknown IRQ type\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct pci_epc_features rcar_gen4_pcie_epc_features = {
> > +	.linkup_notifier = false,
> > +	.msi_capable = true,
> > +	.msix_capable = false,
> > +	.reserved_bar = 1 << BAR_1 | 1 << BAR_3 | 1 << BAR_5,
> > +	.align = SZ_1M,
> > +};
> > +
> > +static const struct pci_epc_features*
> > +rcar_gen4_pcie_ep_get_features(struct dw_pcie_ep *ep)
> > +{
> > +	return &rcar_gen4_pcie_epc_features;
> > +}
> > +
> > +static unsigned int rcar_gen4_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
> > +						       u8 func_no)
> > +{
> > +	return func_no * RCAR_GEN4_PCIE_EP_FUNC_DBI_OFFSET;
> > +}
> > +
> > +static unsigned int rcar_gen4_pcie_ep_get_dbi2_offset(struct dw_pcie_ep *ep,
> > +						      u8 func_no)
> > +{
> > +	return func_no * RCAR_GEN4_PCIE_EP_FUNC_DBI2_OFFSET;
> > +}
> > +
> > +static const struct dw_pcie_ep_ops pcie_ep_ops = {
> > +	.pre_init = rcar_gen4_pcie_ep_pre_init,
> > +	.ep_init = rcar_gen4_pcie_ep_init,
> > +	.deinit = rcar_gen4_pcie_ep_deinit,
> > +	.raise_irq = rcar_gen4_pcie_ep_raise_irq,
> > +	.get_features = rcar_gen4_pcie_ep_get_features,
> > +	.func_conf_select = rcar_gen4_pcie_ep_func_conf_select,
> > +	.get_dbi2_offset = rcar_gen4_pcie_ep_get_dbi2_offset,
> > +};
> > +
> > +static int rcar_gen4_add_dw_pcie_ep(struct rcar_gen4_pcie *rcar)
> > +{
> > +	struct dw_pcie_ep *ep = &rcar->dw.ep;
> > +
> > +	if (!IS_ENABLED(CONFIG_PCIE_RCAR_GEN4_EP))
> > +		return -ENODEV;
> > +
> > +	rcar->mode = DW_PCIE_EP_TYPE;
> > +	ep->ops = &pcie_ep_ops;
> > +
> > +	return dw_pcie_ep_init(ep);
> > +}
> > +
> > +static void rcar_gen4_remove_dw_pcie_ep(struct rcar_gen4_pcie *rcar)
> > +{
> > +	dw_pcie_ep_exit(&rcar->dw.ep);
> > +}
> > +
> > +/* Common */
> >  static int rcar_gen4_pcie_probe(struct platform_device *pdev)
> >  {
> > +	enum dw_pcie_device_mode mode;
> >  	struct rcar_gen4_pcie *rcar;
> >  	int err;
> >
> > @@ -340,7 +453,13 @@ static int rcar_gen4_pcie_probe(struct platform_device *pdev)
> >  	if (err)
> >  		return err;
> >
> 
> > -	err = rcar_gen4_add_dw_pcie_rp(rcar);
> > +	mode = (enum dw_pcie_device_mode)of_device_get_match_data(&pdev->dev);
> > +
> > +	if (mode == DW_PCIE_RC_TYPE)
> > +		err = rcar_gen4_add_dw_pcie_rp(rcar);
> > +	else if (mode == DW_PCIE_EP_TYPE)
> > +		err = rcar_gen4_add_dw_pcie_ep(rcar);
> > +
> 
> So now you have two places with the controller mode initialization:
> 1. rcar_gen4_pcie_of_match
> 2. rcar_gen4_add_dw_pcie_rp() and rcar_gen4_add_dw_pcie_ep()
> It looks a bit clumsy and less maintainable than could be. What I
> suggest is to create a new method which would do what is done above,
> but also would initialize the rcar_gen4_pcie->mode field:
> 
> static int rcar_gen4_add_dw_pcie(struct rcar_gen4_pcie *rcar)
> {
> 	rcar->mode = device_get_match_data(&rcar->pdev->dev);
> 	switch (rcar->mode) {
> 	case DW_PCIE_RC_TYPE:
> 		return rcar_gen4_add_dw_pcie_rp(rcar);
> 	case DW_PCIE_EP_TYPE:
> 		return rcar_gen4_add_dw_pcie_ep(rcar);
> 	}
> 
> 	return -EINVAL;
> }

Thank you for your suggestion. This code is better than my code.

> Of course the rcar_gen4_pcie->mode field initialization should be
> dropped from the rcar_gen4_add_dw_pcie_rp() and
> rcar_gen4_add_dw_pcie_ep() methods.
> 
> and ...
> 
> >  	if (err)
> >  		goto err_unprepare;
> >
> > @@ -356,12 +475,23 @@ static void rcar_gen4_pcie_remove(struct platform_device *pdev)
> >  {
> >  	struct rcar_gen4_pcie *rcar = platform_get_drvdata(pdev);
> >
> 
> > -	rcar_gen4_remove_dw_pcie_rp(rcar);
> > +	if (rcar->mode == DW_PCIE_RC_TYPE)
> > +		rcar_gen4_remove_dw_pcie_rp(rcar);
> > +	else if (rcar->mode == DW_PCIE_EP_TYPE)
> > +		rcar_gen4_remove_dw_pcie_ep(rcar);
> > +
> 
> ... similarly I would have added a respective antagonist:
> 
> static void rcar_gen4_remove_dw_pcie(struct rcar_gen4_pcie *rcar)
> {
> 	switch (rcar->mode) {
> 	case DW_PCIE_RC_TYPE:
> 		rcar_gen4_remove_dw_pcie_rp(rcar);
> 		break;
> 	case DW_PCIE_EP_TYPE:
> 		rcar_gen4_remove_dw_pcie_ep(rcar);
> 		break;
> 	}
> }
> 
> which would have been called from here instead of the open-coded
> switch-case statement. Thus the driver design will be preserved (a set
> of the init/deinit, add/remove, probe/remove and functional helper
> methods) and the mode initialization will be localized in a single
> place.

I understood it. I'll fix them on v23.

Best regards,
Yoshihiro Shimoda

> -Serge(y)
> 
> >  	rcar_gen4_pcie_unprepare(rcar);
> >  }
> >
> >  static const struct of_device_id rcar_gen4_pcie_of_match[] = {
> > -	{ .compatible = "renesas,rcar-gen4-pcie", },
> > +	{
> > +		.compatible = "renesas,rcar-gen4-pcie",
> > +		.data = (void *)DW_PCIE_RC_TYPE,
> > +	},
> > +	{
> > +		.compatible = "renesas,rcar-gen4-pcie-ep",
> > +		.data = (void *)DW_PCIE_EP_TYPE,
> > +	},
> >  	{},
> >  };
> >  MODULE_DEVICE_TABLE(of, rcar_gen4_pcie_of_match);
> > --
> > 2.25.1
> >





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux