[PATCH 0/2] PCI: Convert the Apple controller to host bridge hooks

[PATCH 0/2] PCI: Convert the Apple controller to host bridge hooks

[Marc Zyngier]

The Apple PCIe controller requires some additional attention when
enabling an endpoint device, so that the RID gets correctly mapped to
a SID on its way to the IOMMU.

So far, we have need relying on a custom bus notifier to perform this
task, but Frank Li's series [1] is a better approach as it puts the
complexity in the core code instead of the host controller driver, and
this series builds on that:

- allow the new {en,dis}able_device() to be provided via pci_ecam_ops

- convert the Apple PCIe driver to that infrastructure

Patches on top of 6.13-rc1, plus Frank's v7 series.

[1] https://lore.kernel.org/r/20241203-imx95_lut-v7-0-d0cd6293225e@nxp.com

Marc Zyngier (2):
  PCI: host-generic: Allow {en,dis}able_device() to be provided via
    pci_ecam_ops
  PCI: apple: Convert to {en,dis}able_device() callbacks

 drivers/pci/controller/pci-host-common.c |  2 +
 drivers/pci/controller/pcie-apple.c      | 75 +++++-------------------
 include/linux/pci-ecam.h                 |  4 ++
 3 files changed, 21 insertions(+), 60 deletions(-)

-- 
2.39.2

[PATCH 1/2] PCI: host-generic: Allow {en,dis}able_device() to be provided via pci_ecam_ops

[Marc Zyngier]

In order to let host controller drivers using the host-generic
infrastructure use the {en,dis}able_device() callbacks that can
be used to configure sideband RID mapping hardware, provide these
two callbacks as part of the pci_ecap_ops structure.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/pci/controller/pci-host-common.c | 2 ++
 include/linux/pci-ecam.h                 | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
index cf5f59a745b37..f441bfd6f96a8 100644
--- a/drivers/pci/controller/pci-host-common.c
+++ b/drivers/pci/controller/pci-host-common.c
@@ -75,6 +75,8 @@ int pci_host_common_probe(struct platform_device *pdev)
 
 	bridge->sysdata = cfg;
 	bridge->ops = (struct pci_ops *)&ops->pci_ops;
+	bridge->enable_device = ops->enable_device;
+	bridge->disable_device = ops->disable_device;
 	bridge->msi_domain = true;
 
 	return pci_host_probe(bridge);
diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
index 3a4860bd27586..3a10f8cfc3ad5 100644
--- a/include/linux/pci-ecam.h
+++ b/include/linux/pci-ecam.h
@@ -45,6 +45,10 @@ struct pci_ecam_ops {
 	unsigned int			bus_shift;
 	struct pci_ops			pci_ops;
 	int				(*init)(struct pci_config_window *);
+	int				(*enable_device)(struct pci_host_bridge *,
+							 struct pci_dev *);
+	void				(*disable_device)(struct pci_host_bridge *,
+							  struct pci_dev *);
 };
 
 /*
-- 
2.39.2

Re: [PATCH 1/2] PCI: host-generic: Allow {en,dis}able_device() to be provided via pci_ecam_ops

[Frank Li]

On Wed, Dec 04, 2024 at 03:01:44PM +0000, Marc Zyngier wrote:
> In order to let host controller drivers using the host-generic
> infrastructure use the {en,dis}able_device() callbacks that can
> be used to configure sideband RID mapping hardware, provide these
> two callbacks as part of the pci_ecap_ops structure.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/pci/controller/pci-host-common.c | 2 ++
>  include/linux/pci-ecam.h                 | 4 ++++
>  2 files changed, 6 insertions(+)
>
> diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
> index cf5f59a745b37..f441bfd6f96a8 100644
> --- a/drivers/pci/controller/pci-host-common.c
> +++ b/drivers/pci/controller/pci-host-common.c
> @@ -75,6 +75,8 @@ int pci_host_common_probe(struct platform_device *pdev)
>
>  	bridge->sysdata = cfg;
>  	bridge->ops = (struct pci_ops *)&ops->pci_ops;
> +	bridge->enable_device = ops->enable_device;
> +	bridge->disable_device = ops->disable_device;
>  	bridge->msi_domain = true;
>
>  	return pci_host_probe(bridge);
> diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
> index 3a4860bd27586..3a10f8cfc3ad5 100644
> --- a/include/linux/pci-ecam.h
> +++ b/include/linux/pci-ecam.h
> @@ -45,6 +45,10 @@ struct pci_ecam_ops {
>  	unsigned int			bus_shift;
>  	struct pci_ops			pci_ops;
>  	int				(*init)(struct pci_config_window *);
> +	int				(*enable_device)(struct pci_host_bridge *,
> +							 struct pci_dev *);
> +	void				(*disable_device)(struct pci_host_bridge *,
> +							  struct pci_dev *);
>  };
>
>  /*
> --
> 2.39.2
>

[PATCH 2/2] PCI: apple: Convert to {en,dis}able_device() callbacks

[Marc Zyngier]

Now that the core host-bridge infrastructure is able to give
us a callback on each device being added or removed, convert
the bus-notifier hack to it.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/pci/controller/pcie-apple.c | 75 ++++++-----------------------
 1 file changed, 15 insertions(+), 60 deletions(-)

diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index fefab2758a064..a7e51bc1c2fe8 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -26,7 +26,6 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/msi.h>
-#include <linux/notifier.h>
 #include <linux/of_irq.h>
 #include <linux/pci-ecam.h>
 
@@ -667,12 +666,16 @@ static struct apple_pcie_port *apple_pcie_get_port(struct pci_dev *pdev)
 	return NULL;
 }
 
-static int apple_pcie_add_device(struct apple_pcie_port *port,
-				 struct pci_dev *pdev)
+static int apple_pcie_enable_device(struct pci_host_bridge *bridge, struct pci_dev *pdev)
 {
 	u32 sid, rid = pci_dev_id(pdev);
+	struct apple_pcie_port *port;
 	int idx, err;
 
+	port = apple_pcie_get_port(pdev);
+	if (!port)
+		return 0;
+
 	dev_dbg(&pdev->dev, "added to bus %s, index %d\n",
 		pci_name(pdev->bus->self), port->idx);
 
@@ -698,12 +701,16 @@ static int apple_pcie_add_device(struct apple_pcie_port *port,
 	return idx >= 0 ? 0 : -ENOSPC;
 }
 
-static void apple_pcie_release_device(struct apple_pcie_port *port,
-				      struct pci_dev *pdev)
+static void apple_pcie_disable_device(struct pci_host_bridge *bridge, struct pci_dev *pdev)
 {
+	struct apple_pcie_port *port;
 	u32 rid = pci_dev_id(pdev);
 	int idx;
 
+	port = apple_pcie_get_port(pdev);
+	if (!port)
+		return;
+
 	mutex_lock(&port->pcie->lock);
 
 	for_each_set_bit(idx, port->sid_map, port->sid_map_sz) {
@@ -721,45 +728,6 @@ static void apple_pcie_release_device(struct apple_pcie_port *port,
 	mutex_unlock(&port->pcie->lock);
 }
 
-static int apple_pcie_bus_notifier(struct notifier_block *nb,
-				   unsigned long action,
-				   void *data)
-{
-	struct device *dev = data;
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct apple_pcie_port *port;
-	int err;
-
-	/*
-	 * This is a bit ugly. We assume that if we get notified for
-	 * any PCI device, we must be in charge of it, and that there
-	 * is no other PCI controller in the whole system. It probably
-	 * holds for now, but who knows for how long?
-	 */
-	port = apple_pcie_get_port(pdev);
-	if (!port)
-		return NOTIFY_DONE;
-
-	switch (action) {
-	case BUS_NOTIFY_ADD_DEVICE:
-		err = apple_pcie_add_device(port, pdev);
-		if (err)
-			return notifier_from_errno(err);
-		break;
-	case BUS_NOTIFY_DEL_DEVICE:
-		apple_pcie_release_device(port, pdev);
-		break;
-	default:
-		return NOTIFY_DONE;
-	}
-
-	return NOTIFY_OK;
-}
-
-static struct notifier_block apple_pcie_nb = {
-	.notifier_call = apple_pcie_bus_notifier,
-};
-
 static int apple_pcie_init(struct pci_config_window *cfg)
 {
 	struct device *dev = cfg->parent;
@@ -799,23 +767,10 @@ static int apple_pcie_init(struct pci_config_window *cfg)
 	return 0;
 }
 
-static int apple_pcie_probe(struct platform_device *pdev)
-{
-	int ret;
-
-	ret = bus_register_notifier(&pci_bus_type, &apple_pcie_nb);
-	if (ret)
-		return ret;
-
-	ret = pci_host_common_probe(pdev);
-	if (ret)
-		bus_unregister_notifier(&pci_bus_type, &apple_pcie_nb);
-
-	return ret;
-}
-
 static const struct pci_ecam_ops apple_pcie_cfg_ecam_ops = {
 	.init		= apple_pcie_init,
+	.enable_device	= apple_pcie_enable_device,
+	.disable_device	= apple_pcie_disable_device,
 	.pci_ops	= {
 		.map_bus	= pci_ecam_map_bus,
 		.read		= pci_generic_config_read,
@@ -830,7 +785,7 @@ static const struct of_device_id apple_pcie_of_match[] = {
 MODULE_DEVICE_TABLE(of, apple_pcie_of_match);
 
 static struct platform_driver apple_pcie_driver = {
-	.probe	= apple_pcie_probe,
+	.probe	= pci_host_common_probe,
 	.driver	= {
 		.name			= "pcie-apple",
 		.of_match_table		= apple_pcie_of_match,
-- 
2.39.2

Re: [PATCH 2/2] PCI: apple: Convert to {en,dis}able_device() callbacks

[Frank Li]

On Wed, Dec 04, 2024 at 03:01:45PM +0000, Marc Zyngier wrote:
> Now that the core host-bridge infrastructure is able to give
> us a callback on each device being added or removed, convert
> the bus-notifier hack to it.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Reviewed-by: Frank Li <Frank.Li@nxp.com>


> ---
>  drivers/pci/controller/pcie-apple.c | 75 ++++++-----------------------
>  1 file changed, 15 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index fefab2758a064..a7e51bc1c2fe8 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -26,7 +26,6 @@
>  #include <linux/list.h>
>  #include <linux/module.h>
>  #include <linux/msi.h>
> -#include <linux/notifier.h>
>  #include <linux/of_irq.h>
>  #include <linux/pci-ecam.h>
>
> @@ -667,12 +666,16 @@ static struct apple_pcie_port *apple_pcie_get_port(struct pci_dev *pdev)
>  	return NULL;
>  }
>
> -static int apple_pcie_add_device(struct apple_pcie_port *port,
> -				 struct pci_dev *pdev)
> +static int apple_pcie_enable_device(struct pci_host_bridge *bridge, struct pci_dev *pdev)
>  {
>  	u32 sid, rid = pci_dev_id(pdev);
> +	struct apple_pcie_port *port;
>  	int idx, err;
>
> +	port = apple_pcie_get_port(pdev);
> +	if (!port)
> +		return 0;
> +
>  	dev_dbg(&pdev->dev, "added to bus %s, index %d\n",
>  		pci_name(pdev->bus->self), port->idx);
>
> @@ -698,12 +701,16 @@ static int apple_pcie_add_device(struct apple_pcie_port *port,
>  	return idx >= 0 ? 0 : -ENOSPC;
>  }
>
> -static void apple_pcie_release_device(struct apple_pcie_port *port,
> -				      struct pci_dev *pdev)
> +static void apple_pcie_disable_device(struct pci_host_bridge *bridge, struct pci_dev *pdev)
>  {
> +	struct apple_pcie_port *port;
>  	u32 rid = pci_dev_id(pdev);
>  	int idx;
>
> +	port = apple_pcie_get_port(pdev);
> +	if (!port)
> +		return;
> +
>  	mutex_lock(&port->pcie->lock);
>
>  	for_each_set_bit(idx, port->sid_map, port->sid_map_sz) {
> @@ -721,45 +728,6 @@ static void apple_pcie_release_device(struct apple_pcie_port *port,
>  	mutex_unlock(&port->pcie->lock);
>  }
>
> -static int apple_pcie_bus_notifier(struct notifier_block *nb,
> -				   unsigned long action,
> -				   void *data)
> -{
> -	struct device *dev = data;
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -	struct apple_pcie_port *port;
> -	int err;
> -
> -	/*
> -	 * This is a bit ugly. We assume that if we get notified for
> -	 * any PCI device, we must be in charge of it, and that there
> -	 * is no other PCI controller in the whole system. It probably
> -	 * holds for now, but who knows for how long?
> -	 */
> -	port = apple_pcie_get_port(pdev);
> -	if (!port)
> -		return NOTIFY_DONE;
> -
> -	switch (action) {
> -	case BUS_NOTIFY_ADD_DEVICE:
> -		err = apple_pcie_add_device(port, pdev);
> -		if (err)
> -			return notifier_from_errno(err);
> -		break;
> -	case BUS_NOTIFY_DEL_DEVICE:
> -		apple_pcie_release_device(port, pdev);
> -		break;
> -	default:
> -		return NOTIFY_DONE;
> -	}
> -
> -	return NOTIFY_OK;
> -}
> -
> -static struct notifier_block apple_pcie_nb = {
> -	.notifier_call = apple_pcie_bus_notifier,
> -};
> -
>  static int apple_pcie_init(struct pci_config_window *cfg)
>  {
>  	struct device *dev = cfg->parent;
> @@ -799,23 +767,10 @@ static int apple_pcie_init(struct pci_config_window *cfg)
>  	return 0;
>  }
>
> -static int apple_pcie_probe(struct platform_device *pdev)
> -{
> -	int ret;
> -
> -	ret = bus_register_notifier(&pci_bus_type, &apple_pcie_nb);
> -	if (ret)
> -		return ret;
> -
> -	ret = pci_host_common_probe(pdev);
> -	if (ret)
> -		bus_unregister_notifier(&pci_bus_type, &apple_pcie_nb);
> -
> -	return ret;
> -}
> -
>  static const struct pci_ecam_ops apple_pcie_cfg_ecam_ops = {
>  	.init		= apple_pcie_init,
> +	.enable_device	= apple_pcie_enable_device,
> +	.disable_device	= apple_pcie_disable_device,
>  	.pci_ops	= {
>  		.map_bus	= pci_ecam_map_bus,
>  		.read		= pci_generic_config_read,
> @@ -830,7 +785,7 @@ static const struct of_device_id apple_pcie_of_match[] = {
>  MODULE_DEVICE_TABLE(of, apple_pcie_of_match);
>
>  static struct platform_driver apple_pcie_driver = {
> -	.probe	= apple_pcie_probe,
> +	.probe	= pci_host_common_probe,
>  	.driver	= {
>  		.name			= "pcie-apple",
>  		.of_match_table		= apple_pcie_of_match,
> --
> 2.39.2
>

Re: [PATCH 0/2] PCI: Convert the Apple controller to host bridge hooks

[Manivannan Sadhasivam]

On Wed, Dec 04, 2024 at 03:01:43PM +0000, Marc Zyngier wrote:
> The Apple PCIe controller requires some additional attention when
> enabling an endpoint device, so that the RID gets correctly mapped to
> a SID on its way to the IOMMU.
> 
> So far, we have need relying on a custom bus notifier to perform this
> task, but Frank Li's series [1] is a better approach as it puts the
> complexity in the core code instead of the host controller driver, and
> this series builds on that:
> 
> - allow the new {en,dis}able_device() to be provided via pci_ecam_ops
> 
> - convert the Apple PCIe driver to that infrastructure
> 
> Patches on top of 6.13-rc1, plus Frank's v7 series.
> 
> [1] https://lore.kernel.org/r/20241203-imx95_lut-v7-0-d0cd6293225e@nxp.com
> 

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Marc Zyngier (2):
>   PCI: host-generic: Allow {en,dis}able_device() to be provided via
>     pci_ecam_ops
>   PCI: apple: Convert to {en,dis}able_device() callbacks
> 
>  drivers/pci/controller/pci-host-common.c |  2 +
>  drivers/pci/controller/pcie-apple.c      | 75 +++++-------------------
>  include/linux/pci-ecam.h                 |  4 ++
>  3 files changed, 21 insertions(+), 60 deletions(-)
> 
> -- 
> 2.39.2
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 0/2] PCI: Convert the Apple controller to host bridge hooks

[Bjorn Helgaas]

On Wed, Dec 04, 2024 at 03:01:43PM +0000, Marc Zyngier wrote:
> The Apple PCIe controller requires some additional attention when
> enabling an endpoint device, so that the RID gets correctly mapped to
> a SID on its way to the IOMMU.
> 
> So far, we have need relying on a custom bus notifier to perform this
> task, but Frank Li's series [1] is a better approach as it puts the
> complexity in the core code instead of the host controller driver, and
> this series builds on that:
> 
> - allow the new {en,dis}able_device() to be provided via pci_ecam_ops
> 
> - convert the Apple PCIe driver to that infrastructure
> 
> Patches on top of 6.13-rc1, plus Frank's v7 series.
> 
> [1] https://lore.kernel.org/r/20241203-imx95_lut-v7-0-d0cd6293225e@nxp.com
> 
> Marc Zyngier (2):
>   PCI: host-generic: Allow {en,dis}able_device() to be provided via
>     pci_ecam_ops
>   PCI: apple: Convert to {en,dis}able_device() callbacks
> 
>  drivers/pci/controller/pci-host-common.c |  2 +
>  drivers/pci/controller/pcie-apple.c      | 75 +++++-------------------
>  include/linux/pci-ecam.h                 |  4 ++
>  3 files changed, 21 insertions(+), 60 deletions(-)

Applied to pci/controller/iommu-map for v6.14, thank you!

Bjorn