Received-SPF: pass (google.com: domain of linux-kernel+bounces-185500-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 21 May 2024 17:21:21 -0500
From: Bjorn Helgaas <helgaas@kernel.org>
To: Minda Chen <minda.chen@starfivetech.com>
Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Conor Dooley <conor@kernel.org>,
	Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
	Rob Herring <robh+dt@kernel.org>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Daire McNamara <daire.mcnamara@microchip.com>,
	Emil Renner Berthing <emil.renner.berthing@canonical.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-riscv@lists.infradead.org, linux-pci@vger.kernel.org,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Mason Huo <mason.huo@starfivetech.com>,
	Leyfoon Tan <leyfoon.tan@starfivetech.com>,
	Kevin Xie <kevin.xie@starfivetech.com>
Subject: Re: [PATCH v16 08/22] PCI: microchip: Change the argument of
 plda_pcie_setup_iomems()
Message-ID: <20240521222121.GA51329@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240328091835.14797-9-minda.chen@starfivetech.com>

The patch is OK, but the subject line is not very informative.  It
should be useful all by itself even without the commit log.  "Change
the argument of X" doesn't say anything about why we would want to do
that.

On Thu, Mar 28, 2024 at 05:18:21PM +0800, Minda Chen wrote:
> If other vendor do not select PCI_HOST_COMMON, the driver data is not
> struct pci_host_bridge.

Also, I don't think this is the real problem.  Your
PCIE_MICROCHIP_HOST Kconfig selects PCI_HOST_COMMON, and the driver
calls pci_host_common_probe(), so the driver wouldn't even build
without PCI_HOST_COMMON.

This patch is already applied and ready to go, but if you can tell us
what's really going on here, I'd like to update the commit log.

> Move calling platform_get_drvdata() to mc_platform_init().
> 
> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/pci/controller/plda/pcie-microchip-host.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c
> index 9b367927cd32..805870aed61d 100644
> --- a/drivers/pci/controller/plda/pcie-microchip-host.c
> +++ b/drivers/pci/controller/plda/pcie-microchip-host.c
> @@ -876,11 +876,10 @@ static void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index,
>  	writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR);
>  }
>  
> -static int plda_pcie_setup_iomems(struct platform_device *pdev,
> +static int plda_pcie_setup_iomems(struct pci_host_bridge *bridge,
>  				  struct plda_pcie_rp *port)
>  {
>  	void __iomem *bridge_base_addr = port->bridge_addr;
> -	struct pci_host_bridge *bridge = platform_get_drvdata(pdev);
>  	struct resource_entry *entry;
>  	u64 pci_addr;
>  	u32 index = 1;
> @@ -1018,6 +1017,7 @@ static int mc_platform_init(struct pci_config_window *cfg)
>  {
>  	struct device *dev = cfg->parent;
>  	struct platform_device *pdev = to_platform_device(dev);
> +	struct pci_host_bridge *bridge = platform_get_drvdata(pdev);
>  	void __iomem *bridge_base_addr =
>  		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
>  	int ret;
> @@ -1031,7 +1031,7 @@ static int mc_platform_init(struct pci_config_window *cfg)
>  	mc_pcie_enable_msi(port, cfg->win);
>  
>  	/* Configure non-config space outbound ranges */
> -	ret = plda_pcie_setup_iomems(pdev, &port->plda);
> +	ret = plda_pcie_setup_iomems(bridge, &port->plda);
>  	if (ret)
>  		return ret;
>  
> -- 
> 2.17.1
>