diff mbox series

[v16,08/22] PCI: microchip: Change the argument of plda_pcie_setup_iomems()

Message ID 20240328091835.14797-9-minda.chen@starfivetech.com (mailing list archive)
State Handled Elsewhere
Headers show
Series Refactoring Microchip PCIe driver and add StarFive PCIe | expand

Commit Message

Minda Chen March 28, 2024, 9:18 a.m. UTC 
If other vendor do not select PCI_HOST_COMMON, the driver data is not
struct pci_host_bridge.

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(-)

Comments

Bjorn Helgaas May 21, 2024, 10:21 p.m. UTC | #1 
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
>
Minda Chen May 22, 2024, 1:50 a.m. UTC | #2 
> 
> 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.
> 
It is modified for Starfive code. Starfive JH7110 PCIe do not select
PCI_HOST_COMMON
plda_pcie_setup_iomems() will be changed to common plda code.

I think I can modify the title and commit log like this.

Title: 
PCI: microchip: Get struct pci_host_bridge pointer from platform code

Since plda_pcie_setup_iomems() will be a common PLDA core driver function, but the 
argument0 is a struct platform_device pointer. plda_pcie_setup_iomems() actually
using struct pci_host_bridge pointer other than platform_device pointer. Further
more if a new PLDA core PCIe driver do not select PCI_HOST_COMMON, the platform
driver data is not struct pci_host_bridge pointer. So get struct pci_host_bridge pointer
from platform code function mc_platform_init() and make it to be an argument of 
plda_pcie_setup_iomems().


> > 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
> >
Bjorn Helgaas May 22, 2024, 10:10 p.m. UTC | #3 
On Wed, May 22, 2024 at 01:50:57AM +0000, Minda Chen wrote:
> > 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.
> > 
> It is modified for Starfive code. Starfive JH7110 PCIe do not select
> PCI_HOST_COMMON
> plda_pcie_setup_iomems() will be changed to common plda code.
> 
> I think I can modify the title and commit log like this.
> 
> Title: 
> PCI: microchip: Get struct pci_host_bridge pointer from platform code
> 
> Since plda_pcie_setup_iomems() will be a common PLDA core driver
> function, but the argument0 is a struct platform_device pointer.
> plda_pcie_setup_iomems() actually using struct pci_host_bridge
> pointer other than platform_device pointer. Further more if a new
> PLDA core PCIe driver do not select PCI_HOST_COMMON, the platform
> driver data is not struct pci_host_bridge pointer. So get struct
> pci_host_bridge pointer from platform code function
> mc_platform_init() and make it to be an argument of
> plda_pcie_setup_iomems().

OK, I see what you're doing.  This actually has nothing to do with
whether PCI_HOST_COMMON is *enabled*.  It has to do with whether
drivers use pci_host_common_probe().  Here's what I propose:

  PCI: plda: Pass pci_host_bridge to plda_pcie_setup_iomems()

  plda_pcie_setup_iomems() needs the bridge->windows list from struct
  pci_host_bridge and is currently used only by pcie-microchip-host.c.  This
  driver uses pci_host_common_probe(), which sets a pci_host_bridge as the
  drvdata, so plda_pcie_setup_iomems() used platform_get_drvdata() to find
  the pci_host_bridge.

  But we also want to use plda_pcie_setup_iomems() in the new pcie-starfive.c
  driver, which does not use pci_host_common_probe() and will have struct
  starfive_jh7110_pcie as its drvdata, so pass the pci_host_bridge directly
  to plda_pcie_setup_iomems() so it doesn't need platform_get_drvdata() to
  find it.

> > > 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
> > >
Minda Chen May 23, 2024, 1:09 a.m. UTC | #4 
> 
> On Wed, May 22, 2024 at 01:50:57AM +0000, Minda Chen wrote:
> > > 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.
> > >
> > It is modified for Starfive code. Starfive JH7110 PCIe do not select
> > PCI_HOST_COMMON
> > plda_pcie_setup_iomems() will be changed to common plda code.
> >
> > I think I can modify the title and commit log like this.
> >
> > Title:
> > PCI: microchip: Get struct pci_host_bridge pointer from platform code
> >
> > Since plda_pcie_setup_iomems() will be a common PLDA core driver
> > function, but the argument0 is a struct platform_device pointer.
> > plda_pcie_setup_iomems() actually using struct pci_host_bridge pointer
> > other than platform_device pointer. Further more if a new PLDA core
> > PCIe driver do not select PCI_HOST_COMMON, the platform driver data is
> > not struct pci_host_bridge pointer. So get struct pci_host_bridge
> > pointer from platform code function
> > mc_platform_init() and make it to be an argument of
> > plda_pcie_setup_iomems().
> 
> OK, I see what you're doing.  This actually has nothing to do with whether
> PCI_HOST_COMMON is *enabled*.  It has to do with whether drivers use
> pci_host_common_probe().  Here's what I propose:
> 
>   PCI: plda: Pass pci_host_bridge to plda_pcie_setup_iomems()
> 
>   plda_pcie_setup_iomems() needs the bridge->windows list from struct
>   pci_host_bridge and is currently used only by pcie-microchip-host.c.  This
>   driver uses pci_host_common_probe(), which sets a pci_host_bridge as the
>   drvdata, so plda_pcie_setup_iomems() used platform_get_drvdata() to find
>   the pci_host_bridge.
> 
>   But we also want to use plda_pcie_setup_iomems() in the new pcie-starfive.c
>   driver, which does not use pci_host_common_probe() and will have struct
>   starfive_jh7110_pcie as its drvdata, so pass the pci_host_bridge directly
>   to plda_pcie_setup_iomems() so it doesn't need platform_get_drvdata() to
>   find it.
> 
OK, Thanks. 

I see PCIe 6.10 changed have been merged to main line. 
Should I resend this patch set base on 6.10-rc1? 

> > > > 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
> > > >
Bjorn Helgaas May 23, 2024, 2:35 a.m. UTC | #5 
On Thu, May 23, 2024 at 01:09:58AM +0000, Minda Chen wrote:
> > On Wed, May 22, 2024 at 01:50:57AM +0000, Minda Chen wrote:
> > > > 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.
> > > >
> > > It is modified for Starfive code. Starfive JH7110 PCIe do not select
> > > PCI_HOST_COMMON
> > > plda_pcie_setup_iomems() will be changed to common plda code.
> > >
> > > I think I can modify the title and commit log like this.
> > >
> > > Title:
> > > PCI: microchip: Get struct pci_host_bridge pointer from platform code
> > >
> > > Since plda_pcie_setup_iomems() will be a common PLDA core driver
> > > function, but the argument0 is a struct platform_device pointer.
> > > plda_pcie_setup_iomems() actually using struct pci_host_bridge pointer
> > > other than platform_device pointer. Further more if a new PLDA core
> > > PCIe driver do not select PCI_HOST_COMMON, the platform driver data is
> > > not struct pci_host_bridge pointer. So get struct pci_host_bridge
> > > pointer from platform code function
> > > mc_platform_init() and make it to be an argument of
> > > plda_pcie_setup_iomems().
> > 
> > OK, I see what you're doing.  This actually has nothing to do with whether
> > PCI_HOST_COMMON is *enabled*.  It has to do with whether drivers use
> > pci_host_common_probe().  Here's what I propose:
> > 
> >   PCI: plda: Pass pci_host_bridge to plda_pcie_setup_iomems()
> > 
> >   plda_pcie_setup_iomems() needs the bridge->windows list from struct
> >   pci_host_bridge and is currently used only by pcie-microchip-host.c.  This
> >   driver uses pci_host_common_probe(), which sets a pci_host_bridge as the
> >   drvdata, so plda_pcie_setup_iomems() used platform_get_drvdata() to find
> >   the pci_host_bridge.
> > 
> >   But we also want to use plda_pcie_setup_iomems() in the new pcie-starfive.c
> >   driver, which does not use pci_host_common_probe() and will have struct
> >   starfive_jh7110_pcie as its drvdata, so pass the pci_host_bridge directly
> >   to plda_pcie_setup_iomems() so it doesn't need platform_get_drvdata() to
> >   find it.
> > 
> OK, Thanks. 
> 
> I see PCIe 6.10 changed have been merged to main line. 
> Should I resend this patch set base on 6.10-rc1? 

No need, I rebased it to f0bae243b2bc ("Merge tag 'pci-v6.10-changes'
of git://git.kernel.org/pub/scm/linux/kernel/git/pci/pci") already and
it will be a trivial rebase to v6.10-rc1 next week.

The current pci/controller/microchip branch is at:
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=ed261441e224
Let me know if anything is missing from there.  I can't merge it into
linux-next until v6.10-rc1 is tagged, but as soon as it is, I'll put
it in linux-next.

> > > > > 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
> > > > >
Minda Chen May 23, 2024, 9:22 a.m. UTC | #6 
> 
> On Thu, May 23, 2024 at 01:09:58AM +0000, Minda Chen wrote:
> > > On Wed, May 22, 2024 at 01:50:57AM +0000, Minda Chen wrote:
> > > > > 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.
> > > > >
> > > > It is modified for Starfive code. Starfive JH7110 PCIe do not
> > > > select PCI_HOST_COMMON
> > > > plda_pcie_setup_iomems() will be changed to common plda code.
> > > >
> > > > I think I can modify the title and commit log like this.
> > > >
> > > > Title:
> > > > PCI: microchip: Get struct pci_host_bridge pointer from platform
> > > > code
> > > >
> > > > Since plda_pcie_setup_iomems() will be a common PLDA core driver
> > > > function, but the argument0 is a struct platform_device pointer.
> > > > plda_pcie_setup_iomems() actually using struct pci_host_bridge
> > > > pointer other than platform_device pointer. Further more if a new
> > > > PLDA core PCIe driver do not select PCI_HOST_COMMON, the platform
> > > > driver data is not struct pci_host_bridge pointer. So get struct
> > > > pci_host_bridge pointer from platform code function
> > > > mc_platform_init() and make it to be an argument of
> > > > plda_pcie_setup_iomems().
> > >
> > > OK, I see what you're doing.  This actually has nothing to do with
> > > whether PCI_HOST_COMMON is *enabled*.  It has to do with whether
> > > drivers use pci_host_common_probe().  Here's what I propose:
> > >
> > >   PCI: plda: Pass pci_host_bridge to plda_pcie_setup_iomems()
> > >
> > >   plda_pcie_setup_iomems() needs the bridge->windows list from struct
> > >   pci_host_bridge and is currently used only by pcie-microchip-host.c.
> This
> > >   driver uses pci_host_common_probe(), which sets a pci_host_bridge as
> the
> > >   drvdata, so plda_pcie_setup_iomems() used platform_get_drvdata() to
> find
> > >   the pci_host_bridge.
> > >
> > >   But we also want to use plda_pcie_setup_iomems() in the new
> pcie-starfive.c
> > >   driver, which does not use pci_host_common_probe() and will have struct
> > >   starfive_jh7110_pcie as its drvdata, so pass the pci_host_bridge directly
> > >   to plda_pcie_setup_iomems() so it doesn't need platform_get_drvdata()
> to
> > >   find it.
> > >
> > OK, Thanks.
> >
> > I see PCIe 6.10 changed have been merged to main line.
> > Should I resend this patch set base on 6.10-rc1?
> 
> No need, I rebased it to f0bae243b2bc ("Merge tag 'pci-v6.10-changes'
> of git://git.kernel.org/pub/scm/linux/kernel/git/pci/pci") already and it will be a
> trivial rebase to v6.10-rc1 next week.
> 
> The current pci/controller/microchip branch is at:
> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=ed261441e22
> 4
> Let me know if anything is missing from there.  I can't merge it into linux-next
> until v6.10-rc1 is tagged, but as soon as it is, I'll put it in linux-next.
> 
I have checked the code and tested it with Starfive VisionFive v2 board, PCIe can work.
All are Okay. Thanks for correct the commit message. It is more elegant.

This is precious experience of developing refactor code. Thank all!

Hi Conor
   Thanks for help us for this patch set !
   I see mars dts have been merged to mainline, this version dts patch can not be merged
   I will resend the dts patch on v6.10-rc1.
Conor Dooley May 23, 2024, 9:36 a.m. UTC | #7 
On Thu, May 23, 2024 at 09:22:01AM +0000, Minda Chen wrote:
> Hi Conor
>    Thanks for help us for this patch set !
>    I see mars dts have been merged to mainline, this version dts patch can not be merged
>    I will resend the dts patch on v6.10-rc1.

No worries, the dts patches in this series are long gone out of
patchwork anyway so I wouldn't have even tried to apply them ;)

Cheers,
Conor.
diff mbox series

Patch

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;