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

From: Bjorn Helgaas <helgaas@kernel.org>
To: Richard 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@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,
	linux-imx@nxp.com
Subject: Re: [PATCH v3 04/14] PCI: dwc: Kconfig: Add iMX PCIe EP mode support
Date: Fri, 23 Sep 2022 09:15:26 -0500	[thread overview]
Message-ID: <20220923141526.GA1388290@bhelgaas> (raw)
In-Reply-To: <1663913220-9523-5-git-send-email-hongxing.zhu@nxp.com>

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.

> 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.

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.

> --- 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.

> +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.

>  config PCIE_SPEAR13XX
>  	bool "STMicroelectronics SPEAr PCIe controller"
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel