RE: [PATCH v3 04/14] PCI: dwc: Kconfig: Add iMX PCIe EP mode support

[Hongxing Zhu <hongxing.zhu@nxp.com>]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: 2022年9月23日 22:15
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; robh+dt@kernel.org;
> lorenzo.pieralisi@arm.com; shawnguo@kernel.org; kishon@ti.com;
> kw@linux.com; Frank Li <frank.li@nxp.com>; linux-pci@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 04/14] PCI: dwc: Kconfig: Add iMX PCIe EP mode
> support
> 
> On Fri, Sep 23, 2022 at 02:06:50PM +0800, Richard Zhu wrote:
> > Since i.MX PCIe is one dual mode PCIe controller.
> 
> This is not a sentence.
Okay, would be changed.

> 
> > Add i.MX PCIe EP mode support, and split the PCIe modes to the Root
> > Complex mode and Endpoint mode.
> 
> Add blank lines between paragraphs or rewrap into a single paragraph that fills
> 75 columns.
Okay, would refine the commit log later.

> 
> I think you should split "[12/14] PCI: imx6: Add iMX8MM PCIe EP mode"
> into:
> 
>   - A patch that adds the generic endpoint infrastructure, e.g.,
>     imx6_pcie_ep_init(), imx6_pcie_ep_raise_irq(), imx6_add_pcie_ep().
> 
>   - A second patch that adds the i.MX8MM identifiers.
> 
> That way the i.MX8MM patch will be analogous to the i.MX8MQ and i.MX8MP
> patches.
> 
> Then you could squash this Kconfig patch into the generic endpoint
> infrastructure patch because this patch is what selects PCIE_DW_EP, which is
> what ensures that dw_pcie_ep_reset_bar(), dw_pcie_ep_raise_legacy_irq(), etc.,
> are available.
Good suggestion. Thanks a lot.
Would change it following this way.

> 
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -92,10 +92,33 @@ config PCI_EXYNOS
> >  	  functions to implement the driver.
> >
> >  config PCI_IMX6
> > -	bool "Freescale i.MX6/7/8 PCIe controller"
> > +	bool
> > +
> > +config PCI_IMX6_HOST
> > +	bool "Freescale i.MX6/7/8 PCIe controller host mode"
> >  	depends on ARCH_MXC || COMPILE_TEST
> >  	depends on PCI_MSI_IRQ_DOMAIN
> >  	select PCIE_DW_HOST
> > +	select PCI_IMX6
> > +	help
> > +	  Enables support for the PCIe controller Root Complex mode in the
> > +	  iMX6/7/8 SoCs.
> 
> > +	  This controller can work either as EP or RC. In order to enable
> > +	  host-specific features PCIE_DW_HOST must be selected and in order
> > +	  to enable device-specific features PCIE_DW_EP must be selected.
> 
> I don't think these three lines are useful to the user.  They only describe what
> Kconfig does when PCI_IMX6_HOST is enabled, which is really an internal
> implementation detail.
Okay, would refine them later.

> 
> > +config PCI_IMX6_EP
> > +	bool "Freescale i.MX6/7/8 PCIe controller endpoint mode"
> > +	depends on ARCH_MXC || COMPILE_TEST
> > +	depends on PCI_ENDPOINT
> > +	select PCIE_DW_EP
> > +	select PCI_IMX6
> > +	help
> > +	  Enables support for the PCIe controller endpoint mode in the
> > +	  iMX6/7/8 SoCs.
> > +	  This controller can work either as EP or RC. In order to enable
> > +	  host-specific features PCIE_DW_HOST must be selected and in order
> > +	  to enable device-specific features PCIE_DW_EP must be selected.
> 
> Ditto.
Okay, would refine them later.

Best Regards
Richard Zhu

> 
> >  config PCIE_SPEAR13XX
> >  	bool "STMicroelectronics SPEAr PCIe controller"
> > --
> > 2.25.1
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists
> > .infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&amp;data=05%7
> C0
> >
> 1%7Chongxing.zhu%40nxp.com%7Cc9b4cdb1fe6048bcbf2808da9d6e1245%7
> C686ea1
> >
> d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637995393327822891%7CUnk
> nown%7CTW
> >
> FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV
> CI6
> >
> Mn0%3D%7C3000%7C%7C%7C&amp;sdata=mtOA1T3wSvr4IRfbF8WpMlu%2B
> pMh6vQDPKab
> > 17Y4zdeE%3D&amp;reserved=0