diff mbox series

[2/9] iommu/rockchip: Attach multiple power domains

Message ID 20240612-6-10-rocket-v1-2-060e48eea250@tomeuvizoso.net (mailing list archive)
State New
Headers show
Series New DRM accel driver for Rockchip's RKNN NPU | expand

Commit Message

Tomeu Vizoso June 12, 2024, 1:52 p.m. UTC
IOMMUs with multiple base addresses can also have multiple power
domains.

The base framework only takes care of a single power domain, as some
devices will need for these power domains to be powered on in a specific
order.

Use a helper function to stablish links in the order in which they are
in the DT.

This is needed by the IOMMU used by the NPU in the RK3588.

Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
---
 drivers/iommu/rockchip-iommu.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

Comments

Sebastian Reichel June 13, 2024, 12:05 a.m. UTC | #1
Hi,

On Wed, Jun 12, 2024 at 03:52:55PM GMT, Tomeu Vizoso wrote:
> IOMMUs with multiple base addresses can also have multiple power
> domains.
> 
> The base framework only takes care of a single power domain, as some
> devices will need for these power domains to be powered on in a specific
> order.
> 
> Use a helper function to stablish links in the order in which they are
> in the DT.
> 
> This is needed by the IOMMU used by the NPU in the RK3588.
> 
> Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
> ---

To me it looks like this is multiple IOMMUs, which should each get
their own node. I don't see a good reason for merging these
together.

I will still review this assuming there is one. That would require
to first of all update the DT binding:

Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml

1. It does not allow using "power-domain-names" property
2. It limits the number of allowed power-domains to 1
3. It limits the number of allowed base addresses to 2

Looking at the DT patch you also add more interrupts and clocks,
which are also limited by the binding. You should see a bunch of
warnings when you check the DTBS via 'make dtbs_check'

>  drivers/iommu/rockchip-iommu.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index f5629515bd78..673b0ebb6262 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -6,6 +6,8 @@
>   *			Daniel Kurtz <djkurtz@chromium.org>
>   */
>  
> +#include "linux/err.h"
> +#include "linux/pm_domain.h"
>  #include <linux/clk.h>
>  #include <linux/compiler.h>
>  #include <linux/delay.h>
> @@ -115,6 +117,7 @@ struct rk_iommu {
>  	struct iommu_device iommu;
>  	struct list_head node; /* entry in rk_iommu_domain.iommus */
>  	struct iommu_domain *domain; /* domain to which iommu is attached */
> +	struct dev_pm_domain_list *pmdomains;
>  };
>  
>  struct rk_iommudata {
> @@ -1186,6 +1189,7 @@ static int rk_iommu_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	const struct rk_iommu_ops *ops;
>  	int num_res = pdev->num_resources;
> +	int pm_domain_count;
>  	int err, i;
>  
>  	iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
> @@ -1271,6 +1275,35 @@ static int rk_iommu_probe(struct platform_device *pdev)
>  	if (!dma_dev)
>  		dma_dev = &pdev->dev;
>  
> +	pm_domain_count = of_property_count_strings(iommu->dev->of_node, "power-domain-names");

pm_domain_count = device_property_string_array_count(iommu->dev, "power-domain-names");

When possible using device_property_ is prefered, since it allows
reusing code for systems not using DT.

> +	if (pm_domain_count > 0) {
> +		const char **pm_domains = kvmalloc_array(pm_domain_count, sizeof(*pm_domains), GFP_KERNEL);
> +		struct dev_pm_domain_attach_data pm_domain_data = {
> +			.pd_names = pm_domains,
> +			.num_pd_names = pm_domain_count,
> +			.pd_flags = PD_FLAG_DEV_LINK_ON,
> +		};
> +		int i;
> +
> +		if (!pm_domain_data.pd_names) {
> +			err = -ENOMEM;
> +			goto err_remove_sysfs;
> +		}
> +
> +		for (i = 0; i < pm_domain_count; i++) {
> +			err = of_property_read_string_index(iommu->dev->of_node, "power-domain-names", i, &pm_domains[i]);
> +			if (err) {
> +				kfree(pm_domains);
> +				goto err_remove_sysfs;
> +			}
> +		}

There is a helper to read a string array:

err = device_property_read_string_array(iommu->dev, "power-domain-names", pm_domains, pm_domain_count);

-- Sebastian

> +
> +		err = dev_pm_domain_attach_list(iommu->dev, &pm_domain_data, &iommu->pmdomains);
> +		kfree(pm_domains);
> +		if (err < 0)
> +			goto err_remove_sysfs;
> +	}
> +
>  	pm_runtime_enable(dev);
>  
>  	for (i = 0; i < iommu->num_irq; i++) {
> @@ -1292,6 +1325,7 @@ static int rk_iommu_probe(struct platform_device *pdev)
>  	return 0;
>  err_pm_disable:
>  	pm_runtime_disable(dev);
> +	dev_pm_domain_detach_list(iommu->pmdomains);
>  err_remove_sysfs:
>  	iommu_device_sysfs_remove(&iommu->iommu);
>  err_unprepare_clocks:
> @@ -1310,6 +1344,8 @@ static void rk_iommu_shutdown(struct platform_device *pdev)
>  		devm_free_irq(iommu->dev, irq, iommu);
>  	}
>  
> +	dev_pm_domain_detach_list(iommu->pmdomains);
> +
>  	pm_runtime_force_suspend(&pdev->dev);
>  }
>  
> 
> -- 
> 2.45.2
> 
>
Tomeu Vizoso June 13, 2024, 9:24 a.m. UTC | #2
On Thu, Jun 13, 2024 at 2:05 AM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hi,
>
> On Wed, Jun 12, 2024 at 03:52:55PM GMT, Tomeu Vizoso wrote:
> > IOMMUs with multiple base addresses can also have multiple power
> > domains.
> >
> > The base framework only takes care of a single power domain, as some
> > devices will need for these power domains to be powered on in a specific
> > order.
> >
> > Use a helper function to stablish links in the order in which they are
> > in the DT.
> >
> > This is needed by the IOMMU used by the NPU in the RK3588.
> >
> > Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
> > ---
>
> To me it looks like this is multiple IOMMUs, which should each get
> their own node. I don't see a good reason for merging these
> together.

I have made quite a few attempts at splitting the IOMMUs and also the
cores, but I wasn't able to get things working stably. The TRM is
really scant about how the 4 IOMMU instances relate to each other, and
what the fourth one is for.

Given that the vendor driver treats them as a single IOMMU with four
instances and we don't have any information on them, I resigned myself
to just have them as a single device.

I would love to be proved wrong though and find a way fo getting
things stably as different devices so they can be powered on and off
as needed. We could save quite some code as well.

> I will still review this assuming there is one. That would require
> to first of all update the DT binding:
>
> Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml
>
> 1. It does not allow using "power-domain-names" property
> 2. It limits the number of allowed power-domains to 1
> 3. It limits the number of allowed base addresses to 2
>
> Looking at the DT patch you also add more interrupts and clocks,
> which are also limited by the binding. You should see a bunch of
> warnings when you check the DTBS via 'make dtbs_check'

Oops, yeah, I was limiting dtbs_check with DT_SCHEMA_FILES, now I see
the errors.

> >  drivers/iommu/rockchip-iommu.c | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> > index f5629515bd78..673b0ebb6262 100644
> > --- a/drivers/iommu/rockchip-iommu.c
> > +++ b/drivers/iommu/rockchip-iommu.c
> > @@ -6,6 +6,8 @@
> >   *                   Daniel Kurtz <djkurtz@chromium.org>
> >   */
> >
> > +#include "linux/err.h"
> > +#include "linux/pm_domain.h"
> >  #include <linux/clk.h>
> >  #include <linux/compiler.h>
> >  #include <linux/delay.h>
> > @@ -115,6 +117,7 @@ struct rk_iommu {
> >       struct iommu_device iommu;
> >       struct list_head node; /* entry in rk_iommu_domain.iommus */
> >       struct iommu_domain *domain; /* domain to which iommu is attached */
> > +     struct dev_pm_domain_list *pmdomains;
> >  };
> >
> >  struct rk_iommudata {
> > @@ -1186,6 +1189,7 @@ static int rk_iommu_probe(struct platform_device *pdev)
> >       struct resource *res;
> >       const struct rk_iommu_ops *ops;
> >       int num_res = pdev->num_resources;
> > +     int pm_domain_count;
> >       int err, i;
> >
> >       iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
> > @@ -1271,6 +1275,35 @@ static int rk_iommu_probe(struct platform_device *pdev)
> >       if (!dma_dev)
> >               dma_dev = &pdev->dev;
> >
> > +     pm_domain_count = of_property_count_strings(iommu->dev->of_node, "power-domain-names");
>
> pm_domain_count = device_property_string_array_count(iommu->dev, "power-domain-names");
>
> When possible using device_property_ is prefered, since it allows
> reusing code for systems not using DT.
>
> > +     if (pm_domain_count > 0) {
> > +             const char **pm_domains = kvmalloc_array(pm_domain_count, sizeof(*pm_domains), GFP_KERNEL);
> > +             struct dev_pm_domain_attach_data pm_domain_data = {
> > +                     .pd_names = pm_domains,
> > +                     .num_pd_names = pm_domain_count,
> > +                     .pd_flags = PD_FLAG_DEV_LINK_ON,
> > +             };
> > +             int i;
> > +
> > +             if (!pm_domain_data.pd_names) {
> > +                     err = -ENOMEM;
> > +                     goto err_remove_sysfs;
> > +             }
> > +
> > +             for (i = 0; i < pm_domain_count; i++) {
> > +                     err = of_property_read_string_index(iommu->dev->of_node, "power-domain-names", i, &pm_domains[i]);
> > +                     if (err) {
> > +                             kfree(pm_domains);
> > +                             goto err_remove_sysfs;
> > +                     }
> > +             }
>
> There is a helper to read a string array:
>
> err = device_property_read_string_array(iommu->dev, "power-domain-names", pm_domains, pm_domain_count);


Thanks for the review,

Tomeu

> -- Sebastian
>
> > +
> > +             err = dev_pm_domain_attach_list(iommu->dev, &pm_domain_data, &iommu->pmdomains);
> > +             kfree(pm_domains);
> > +             if (err < 0)
> > +                     goto err_remove_sysfs;
> > +     }
> > +
> >       pm_runtime_enable(dev);
> >
> >       for (i = 0; i < iommu->num_irq; i++) {
> > @@ -1292,6 +1325,7 @@ static int rk_iommu_probe(struct platform_device *pdev)
> >       return 0;
> >  err_pm_disable:
> >       pm_runtime_disable(dev);
> > +     dev_pm_domain_detach_list(iommu->pmdomains);
> >  err_remove_sysfs:
> >       iommu_device_sysfs_remove(&iommu->iommu);
> >  err_unprepare_clocks:
> > @@ -1310,6 +1344,8 @@ static void rk_iommu_shutdown(struct platform_device *pdev)
> >               devm_free_irq(iommu->dev, irq, iommu);
> >       }
> >
> > +     dev_pm_domain_detach_list(iommu->pmdomains);
> > +
> >       pm_runtime_force_suspend(&pdev->dev);
> >  }
> >
> >
> > --
> > 2.45.2
> >
> >
Tomeu Vizoso June 13, 2024, 9:34 a.m. UTC | #3
On Thu, Jun 13, 2024 at 11:24 AM Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
>
> On Thu, Jun 13, 2024 at 2:05 AM Sebastian Reichel
> <sebastian.reichel@collabora.com> wrote:
> >
> > Hi,
> >
> > On Wed, Jun 12, 2024 at 03:52:55PM GMT, Tomeu Vizoso wrote:
> > > IOMMUs with multiple base addresses can also have multiple power
> > > domains.
> > >
> > > The base framework only takes care of a single power domain, as some
> > > devices will need for these power domains to be powered on in a specific
> > > order.
> > >
> > > Use a helper function to stablish links in the order in which they are
> > > in the DT.
> > >
> > > This is needed by the IOMMU used by the NPU in the RK3588.
> > >
> > > Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
> > > ---
> >
> > To me it looks like this is multiple IOMMUs, which should each get
> > their own node. I don't see a good reason for merging these
> > together.
>
> I have made quite a few attempts at splitting the IOMMUs and also the
> cores, but I wasn't able to get things working stably. The TRM is
> really scant about how the 4 IOMMU instances relate to each other, and
> what the fourth one is for.
>
> Given that the vendor driver treats them as a single IOMMU with four
> instances and we don't have any information on them, I resigned myself
> to just have them as a single device.
>
> I would love to be proved wrong though and find a way fo getting
> things stably as different devices so they can be powered on and off
> as needed. We could save quite some code as well.

FWIW, here a few ways how I tried to structure the DT nodes, none of
these worked reliably:

https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-multiple-devices-power/arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L1163
https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-schema-subnodes//arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L1162
https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-multiple-devices//arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L1163
https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-multiple-iommus//arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L2669

I can very well imagine I missed some way of getting this to work, but
for every attempt, the domains, iommus and cores were resumed in
different orders that presumably caused problems during concurrent
execution fo workloads.

So I fell back to what the vendor driver does, which works reliably
(but all cores have to be powered on at the same time).

Thanks,

Tomeu

> > I will still review this assuming there is one. That would require
> > to first of all update the DT binding:
> >
> > Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml
> >
> > 1. It does not allow using "power-domain-names" property
> > 2. It limits the number of allowed power-domains to 1
> > 3. It limits the number of allowed base addresses to 2
> >
> > Looking at the DT patch you also add more interrupts and clocks,
> > which are also limited by the binding. You should see a bunch of
> > warnings when you check the DTBS via 'make dtbs_check'
>
> Oops, yeah, I was limiting dtbs_check with DT_SCHEMA_FILES, now I see
> the errors.
>
> > >  drivers/iommu/rockchip-iommu.c | 36 ++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 36 insertions(+)
> > >
> > > diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> > > index f5629515bd78..673b0ebb6262 100644
> > > --- a/drivers/iommu/rockchip-iommu.c
> > > +++ b/drivers/iommu/rockchip-iommu.c
> > > @@ -6,6 +6,8 @@
> > >   *                   Daniel Kurtz <djkurtz@chromium.org>
> > >   */
> > >
> > > +#include "linux/err.h"
> > > +#include "linux/pm_domain.h"
> > >  #include <linux/clk.h>
> > >  #include <linux/compiler.h>
> > >  #include <linux/delay.h>
> > > @@ -115,6 +117,7 @@ struct rk_iommu {
> > >       struct iommu_device iommu;
> > >       struct list_head node; /* entry in rk_iommu_domain.iommus */
> > >       struct iommu_domain *domain; /* domain to which iommu is attached */
> > > +     struct dev_pm_domain_list *pmdomains;
> > >  };
> > >
> > >  struct rk_iommudata {
> > > @@ -1186,6 +1189,7 @@ static int rk_iommu_probe(struct platform_device *pdev)
> > >       struct resource *res;
> > >       const struct rk_iommu_ops *ops;
> > >       int num_res = pdev->num_resources;
> > > +     int pm_domain_count;
> > >       int err, i;
> > >
> > >       iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
> > > @@ -1271,6 +1275,35 @@ static int rk_iommu_probe(struct platform_device *pdev)
> > >       if (!dma_dev)
> > >               dma_dev = &pdev->dev;
> > >
> > > +     pm_domain_count = of_property_count_strings(iommu->dev->of_node, "power-domain-names");
> >
> > pm_domain_count = device_property_string_array_count(iommu->dev, "power-domain-names");
> >
> > When possible using device_property_ is prefered, since it allows
> > reusing code for systems not using DT.
> >
> > > +     if (pm_domain_count > 0) {
> > > +             const char **pm_domains = kvmalloc_array(pm_domain_count, sizeof(*pm_domains), GFP_KERNEL);
> > > +             struct dev_pm_domain_attach_data pm_domain_data = {
> > > +                     .pd_names = pm_domains,
> > > +                     .num_pd_names = pm_domain_count,
> > > +                     .pd_flags = PD_FLAG_DEV_LINK_ON,
> > > +             };
> > > +             int i;
> > > +
> > > +             if (!pm_domain_data.pd_names) {
> > > +                     err = -ENOMEM;
> > > +                     goto err_remove_sysfs;
> > > +             }
> > > +
> > > +             for (i = 0; i < pm_domain_count; i++) {
> > > +                     err = of_property_read_string_index(iommu->dev->of_node, "power-domain-names", i, &pm_domains[i]);
> > > +                     if (err) {
> > > +                             kfree(pm_domains);
> > > +                             goto err_remove_sysfs;
> > > +                     }
> > > +             }
> >
> > There is a helper to read a string array:
> >
> > err = device_property_read_string_array(iommu->dev, "power-domain-names", pm_domains, pm_domain_count);
>
>
> Thanks for the review,
>
> Tomeu
>
> > -- Sebastian
> >
> > > +
> > > +             err = dev_pm_domain_attach_list(iommu->dev, &pm_domain_data, &iommu->pmdomains);
> > > +             kfree(pm_domains);
> > > +             if (err < 0)
> > > +                     goto err_remove_sysfs;
> > > +     }
> > > +
> > >       pm_runtime_enable(dev);
> > >
> > >       for (i = 0; i < iommu->num_irq; i++) {
> > > @@ -1292,6 +1325,7 @@ static int rk_iommu_probe(struct platform_device *pdev)
> > >       return 0;
> > >  err_pm_disable:
> > >       pm_runtime_disable(dev);
> > > +     dev_pm_domain_detach_list(iommu->pmdomains);
> > >  err_remove_sysfs:
> > >       iommu_device_sysfs_remove(&iommu->iommu);
> > >  err_unprepare_clocks:
> > > @@ -1310,6 +1344,8 @@ static void rk_iommu_shutdown(struct platform_device *pdev)
> > >               devm_free_irq(iommu->dev, irq, iommu);
> > >       }
> > >
> > > +     dev_pm_domain_detach_list(iommu->pmdomains);
> > > +
> > >       pm_runtime_force_suspend(&pdev->dev);
> > >  }
> > >
> > >
> > > --
> > > 2.45.2
> > >
> > >
Sebastian Reichel June 13, 2024, 9:38 p.m. UTC | #4
Hi,

On Thu, Jun 13, 2024 at 11:34:02AM GMT, Tomeu Vizoso wrote:
> On Thu, Jun 13, 2024 at 11:24 AM Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
> > On Thu, Jun 13, 2024 at 2:05 AM Sebastian Reichel
> > <sebastian.reichel@collabora.com> wrote:
> > > On Wed, Jun 12, 2024 at 03:52:55PM GMT, Tomeu Vizoso wrote:
> > > > IOMMUs with multiple base addresses can also have multiple power
> > > > domains.
> > > >
> > > > The base framework only takes care of a single power domain, as some
> > > > devices will need for these power domains to be powered on in a specific
> > > > order.
> > > >
> > > > Use a helper function to stablish links in the order in which they are
> > > > in the DT.
> > > >
> > > > This is needed by the IOMMU used by the NPU in the RK3588.
> > > >
> > > > Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
> > > > ---
> > >
> > > To me it looks like this is multiple IOMMUs, which should each get
> > > their own node. I don't see a good reason for merging these
> > > together.
> >
> > I have made quite a few attempts at splitting the IOMMUs and also the
> > cores, but I wasn't able to get things working stably. The TRM is
> > really scant about how the 4 IOMMU instances relate to each other, and
> > what the fourth one is for.
> >
> > Given that the vendor driver treats them as a single IOMMU with four
> > instances and we don't have any information on them, I resigned myself
> > to just have them as a single device.
> >
> > I would love to be proved wrong though and find a way fo getting
> > things stably as different devices so they can be powered on and off
> > as needed. We could save quite some code as well.
> 
> FWIW, here a few ways how I tried to structure the DT nodes, none of
> these worked reliably:
> 
> https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-multiple-devices-power/arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L1163
> https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-schema-subnodes//arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L1162
> https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-multiple-devices//arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L1163
> https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-multiple-iommus//arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L2669
> 
> I can very well imagine I missed some way of getting this to work, but
> for every attempt, the domains, iommus and cores were resumed in
> different orders that presumably caused problems during concurrent
> execution fo workloads.
> 
> So I fell back to what the vendor driver does, which works reliably
> (but all cores have to be powered on at the same time).

Mh. The "6.10-rocket-multiple-iommus" branch seems wrong. There is
only one iommu node in that. I would have expected a test with

rknn {
    // combined device

    iommus = <&iommu1>, <&iommu2>, ...;
};

Otherwise I think I would go with the schema-subnodes variant. The
driver can initially walk through the sub-nodes and collect the
resources into the main device, so on the driver side nothing would
really change. But that has a couple of advantages:

1. DT and DT binding are easier to read
2. It's similar to e.g. CPU cores each having their own node
3. Easy to extend to more cores in the future
4. The kernel can easily switch to proper per-core device model when
   the problem has been identified

-- Sebastian
Robin Murphy June 14, 2024, 12:07 p.m. UTC | #5
On 2024-06-13 10:38 pm, Sebastian Reichel wrote:
> Hi,
> 
> On Thu, Jun 13, 2024 at 11:34:02AM GMT, Tomeu Vizoso wrote:
>> On Thu, Jun 13, 2024 at 11:24 AM Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
>>> On Thu, Jun 13, 2024 at 2:05 AM Sebastian Reichel
>>> <sebastian.reichel@collabora.com> wrote:
>>>> On Wed, Jun 12, 2024 at 03:52:55PM GMT, Tomeu Vizoso wrote:
>>>>> IOMMUs with multiple base addresses can also have multiple power
>>>>> domains.
>>>>>
>>>>> The base framework only takes care of a single power domain, as some
>>>>> devices will need for these power domains to be powered on in a specific
>>>>> order.
>>>>>
>>>>> Use a helper function to stablish links in the order in which they are
>>>>> in the DT.
>>>>>
>>>>> This is needed by the IOMMU used by the NPU in the RK3588.
>>>>>
>>>>> Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
>>>>> ---
>>>>
>>>> To me it looks like this is multiple IOMMUs, which should each get
>>>> their own node. I don't see a good reason for merging these
>>>> together.
>>>
>>> I have made quite a few attempts at splitting the IOMMUs and also the
>>> cores, but I wasn't able to get things working stably. The TRM is
>>> really scant about how the 4 IOMMU instances relate to each other, and
>>> what the fourth one is for.
>>>
>>> Given that the vendor driver treats them as a single IOMMU with four
>>> instances and we don't have any information on them, I resigned myself
>>> to just have them as a single device.
>>>
>>> I would love to be proved wrong though and find a way fo getting
>>> things stably as different devices so they can be powered on and off
>>> as needed. We could save quite some code as well.
>>
>> FWIW, here a few ways how I tried to structure the DT nodes, none of
>> these worked reliably:
>>
>> https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-multiple-devices-power/arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L1163
>> https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-schema-subnodes//arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L1162
>> https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-multiple-devices//arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L1163
>> https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-multiple-iommus//arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L2669
>>
>> I can very well imagine I missed some way of getting this to work, but
>> for every attempt, the domains, iommus and cores were resumed in
>> different orders that presumably caused problems during concurrent
>> execution fo workloads.
>>
>> So I fell back to what the vendor driver does, which works reliably
>> (but all cores have to be powered on at the same time).
> 
> Mh. The "6.10-rocket-multiple-iommus" branch seems wrong. There is
> only one iommu node in that. I would have expected a test with
> 
> rknn {
>      // combined device
> 
>      iommus = <&iommu1>, <&iommu2>, ...;
> };
> 
> Otherwise I think I would go with the schema-subnodes variant. The
> driver can initially walk through the sub-nodes and collect the
> resources into the main device, so on the driver side nothing would
> really change. But that has a couple of advantages:
> 
> 1. DT and DT binding are easier to read
> 2. It's similar to e.g. CPU cores each having their own node
> 3. Easy to extend to more cores in the future
> 4. The kernel can easily switch to proper per-core device model when
>     the problem has been identified

It also would seem to permit describing and associating the per-core 
IOMMUs individually - apart from core 0's apparent coupling to whatever 
shared "uncore" stuff exists for the whole thing, from the distinct 
clocks, interrupts, power domains etc. lining up with each core I'd 
guess those IOMMUs are not interrelated the same way the ISP's 
read/write IOMMUs are (which was the main justification for adopting the 
multiple-reg design originally vs. distinct DT nodes like Exynos does). 
However, practically that would require the driver to at least populate 
per-core child devices to make DMA API or IOMMU API mappings with, since 
we couldn't spread the "collect the resources" trick into those 
subsystems as well.

Thanks,
Robin.
Tomeu Vizoso Sept. 11, 2024, 11:03 a.m. UTC | #6
On Thu, Jun 13, 2024 at 11:38 PM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hi,
>
> On Thu, Jun 13, 2024 at 11:34:02AM GMT, Tomeu Vizoso wrote:
> > On Thu, Jun 13, 2024 at 11:24 AM Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
> > > On Thu, Jun 13, 2024 at 2:05 AM Sebastian Reichel
> > > <sebastian.reichel@collabora.com> wrote:
> > > > On Wed, Jun 12, 2024 at 03:52:55PM GMT, Tomeu Vizoso wrote:
> > > > > IOMMUs with multiple base addresses can also have multiple power
> > > > > domains.
> > > > >
> > > > > The base framework only takes care of a single power domain, as some
> > > > > devices will need for these power domains to be powered on in a specific
> > > > > order.
> > > > >
> > > > > Use a helper function to stablish links in the order in which they are
> > > > > in the DT.
> > > > >
> > > > > This is needed by the IOMMU used by the NPU in the RK3588.
> > > > >
> > > > > Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
> > > > > ---
> > > >
> > > > To me it looks like this is multiple IOMMUs, which should each get
> > > > their own node. I don't see a good reason for merging these
> > > > together.
> > >
> > > I have made quite a few attempts at splitting the IOMMUs and also the
> > > cores, but I wasn't able to get things working stably. The TRM is
> > > really scant about how the 4 IOMMU instances relate to each other, and
> > > what the fourth one is for.
> > >
> > > Given that the vendor driver treats them as a single IOMMU with four
> > > instances and we don't have any information on them, I resigned myself
> > > to just have them as a single device.
> > >
> > > I would love to be proved wrong though and find a way fo getting
> > > things stably as different devices so they can be powered on and off
> > > as needed. We could save quite some code as well.
> >
> > FWIW, here a few ways how I tried to structure the DT nodes, none of
> > these worked reliably:
> >
> > https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-multiple-devices-power/arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L1163
> > https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-schema-subnodes//arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L1162
> > https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-multiple-devices//arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L1163
> > https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-multiple-iommus//arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L2669
> >
> > I can very well imagine I missed some way of getting this to work, but
> > for every attempt, the domains, iommus and cores were resumed in
> > different orders that presumably caused problems during concurrent
> > execution fo workloads.
> >
> > So I fell back to what the vendor driver does, which works reliably
> > (but all cores have to be powered on at the same time).
>
> Mh. The "6.10-rocket-multiple-iommus" branch seems wrong. There is
> only one iommu node in that. I would have expected a test with
>
> rknn {
>     // combined device
>
>     iommus = <&iommu1>, <&iommu2>, ...;
> };

You are right, I'm afraid I lost those changes...

> Otherwise I think I would go with the schema-subnodes variant. The
> driver can initially walk through the sub-nodes and collect the
> resources into the main device, so on the driver side nothing would
> really change. But that has a couple of advantages:
>
> 1. DT and DT binding are easier to read
> 2. It's similar to e.g. CPU cores each having their own node
> 3. Easy to extend to more cores in the future
> 4. The kernel can easily switch to proper per-core device model when
>    the problem has been identified

You mean having subnodes containing the different resources that a
core uses such as clocks, memory resources, power domain, etc? The
problem with that is that the existing code in the kernel assumes that
those resources are directly within a device node. Or do you suggest
something else?

Thanks,

Tomeu
Tomeu Vizoso Sept. 11, 2024, 11:07 a.m. UTC | #7
On Fri, Jun 14, 2024 at 2:07 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2024-06-13 10:38 pm, Sebastian Reichel wrote:
> > Hi,
> >
> > On Thu, Jun 13, 2024 at 11:34:02AM GMT, Tomeu Vizoso wrote:
> >> On Thu, Jun 13, 2024 at 11:24 AM Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
> >>> On Thu, Jun 13, 2024 at 2:05 AM Sebastian Reichel
> >>> <sebastian.reichel@collabora.com> wrote:
> >>>> On Wed, Jun 12, 2024 at 03:52:55PM GMT, Tomeu Vizoso wrote:
> >>>>> IOMMUs with multiple base addresses can also have multiple power
> >>>>> domains.
> >>>>>
> >>>>> The base framework only takes care of a single power domain, as some
> >>>>> devices will need for these power domains to be powered on in a specific
> >>>>> order.
> >>>>>
> >>>>> Use a helper function to stablish links in the order in which they are
> >>>>> in the DT.
> >>>>>
> >>>>> This is needed by the IOMMU used by the NPU in the RK3588.
> >>>>>
> >>>>> Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
> >>>>> ---
> >>>>
> >>>> To me it looks like this is multiple IOMMUs, which should each get
> >>>> their own node. I don't see a good reason for merging these
> >>>> together.
> >>>
> >>> I have made quite a few attempts at splitting the IOMMUs and also the
> >>> cores, but I wasn't able to get things working stably. The TRM is
> >>> really scant about how the 4 IOMMU instances relate to each other, and
> >>> what the fourth one is for.
> >>>
> >>> Given that the vendor driver treats them as a single IOMMU with four
> >>> instances and we don't have any information on them, I resigned myself
> >>> to just have them as a single device.
> >>>
> >>> I would love to be proved wrong though and find a way fo getting
> >>> things stably as different devices so they can be powered on and off
> >>> as needed. We could save quite some code as well.
> >>
> >> FWIW, here a few ways how I tried to structure the DT nodes, none of
> >> these worked reliably:
> >>
> >> https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-multiple-devices-power/arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L1163
> >> https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-schema-subnodes//arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L1162
> >> https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-multiple-devices//arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L1163
> >> https://gitlab.freedesktop.org/tomeu/linux/-/blob/6.10-rocket-multiple-iommus//arch/arm64/boot/dts/rockchip/rk3588s.dtsi?ref_type=heads#L2669
> >>
> >> I can very well imagine I missed some way of getting this to work, but
> >> for every attempt, the domains, iommus and cores were resumed in
> >> different orders that presumably caused problems during concurrent
> >> execution fo workloads.
> >>
> >> So I fell back to what the vendor driver does, which works reliably
> >> (but all cores have to be powered on at the same time).
> >
> > Mh. The "6.10-rocket-multiple-iommus" branch seems wrong. There is
> > only one iommu node in that. I would have expected a test with
> >
> > rknn {
> >      // combined device
> >
> >      iommus = <&iommu1>, <&iommu2>, ...;
> > };
> >
> > Otherwise I think I would go with the schema-subnodes variant. The
> > driver can initially walk through the sub-nodes and collect the
> > resources into the main device, so on the driver side nothing would
> > really change. But that has a couple of advantages:
> >
> > 1. DT and DT binding are easier to read
> > 2. It's similar to e.g. CPU cores each having their own node
> > 3. Easy to extend to more cores in the future
> > 4. The kernel can easily switch to proper per-core device model when
> >     the problem has been identified
>
> It also would seem to permit describing and associating the per-core
> IOMMUs individually - apart from core 0's apparent coupling to whatever
> shared "uncore" stuff exists for the whole thing, from the distinct
> clocks, interrupts, power domains etc. lining up with each core I'd
> guess those IOMMUs are not interrelated the same way the ISP's
> read/write IOMMUs are (which was the main justification for adopting the
> multiple-reg design originally vs. distinct DT nodes like Exynos does).
> However, practically that would require the driver to at least populate
> per-core child devices to make DMA API or IOMMU API mappings with, since
> we couldn't spread the "collect the resources" trick into those
> subsystems as well.

They seem to be interrelated in some way, because I need to program
the mappings in all three iommus for things to work, which is also
what the downstream driver does. Unfortunately, the TRM has zero
references to the MMU in the NPU section...

Regards,

Tomeu
diff mbox series

Patch

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index f5629515bd78..673b0ebb6262 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -6,6 +6,8 @@ 
  *			Daniel Kurtz <djkurtz@chromium.org>
  */
 
+#include "linux/err.h"
+#include "linux/pm_domain.h"
 #include <linux/clk.h>
 #include <linux/compiler.h>
 #include <linux/delay.h>
@@ -115,6 +117,7 @@  struct rk_iommu {
 	struct iommu_device iommu;
 	struct list_head node; /* entry in rk_iommu_domain.iommus */
 	struct iommu_domain *domain; /* domain to which iommu is attached */
+	struct dev_pm_domain_list *pmdomains;
 };
 
 struct rk_iommudata {
@@ -1186,6 +1189,7 @@  static int rk_iommu_probe(struct platform_device *pdev)
 	struct resource *res;
 	const struct rk_iommu_ops *ops;
 	int num_res = pdev->num_resources;
+	int pm_domain_count;
 	int err, i;
 
 	iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
@@ -1271,6 +1275,35 @@  static int rk_iommu_probe(struct platform_device *pdev)
 	if (!dma_dev)
 		dma_dev = &pdev->dev;
 
+	pm_domain_count = of_property_count_strings(iommu->dev->of_node, "power-domain-names");
+	if (pm_domain_count > 0) {
+		const char **pm_domains = kvmalloc_array(pm_domain_count, sizeof(*pm_domains), GFP_KERNEL);
+		struct dev_pm_domain_attach_data pm_domain_data = {
+			.pd_names = pm_domains,
+			.num_pd_names = pm_domain_count,
+			.pd_flags = PD_FLAG_DEV_LINK_ON,
+		};
+		int i;
+
+		if (!pm_domain_data.pd_names) {
+			err = -ENOMEM;
+			goto err_remove_sysfs;
+		}
+
+		for (i = 0; i < pm_domain_count; i++) {
+			err = of_property_read_string_index(iommu->dev->of_node, "power-domain-names", i, &pm_domains[i]);
+			if (err) {
+				kfree(pm_domains);
+				goto err_remove_sysfs;
+			}
+		}
+
+		err = dev_pm_domain_attach_list(iommu->dev, &pm_domain_data, &iommu->pmdomains);
+		kfree(pm_domains);
+		if (err < 0)
+			goto err_remove_sysfs;
+	}
+
 	pm_runtime_enable(dev);
 
 	for (i = 0; i < iommu->num_irq; i++) {
@@ -1292,6 +1325,7 @@  static int rk_iommu_probe(struct platform_device *pdev)
 	return 0;
 err_pm_disable:
 	pm_runtime_disable(dev);
+	dev_pm_domain_detach_list(iommu->pmdomains);
 err_remove_sysfs:
 	iommu_device_sysfs_remove(&iommu->iommu);
 err_unprepare_clocks:
@@ -1310,6 +1344,8 @@  static void rk_iommu_shutdown(struct platform_device *pdev)
 		devm_free_irq(iommu->dev, irq, iommu);
 	}
 
+	dev_pm_domain_detach_list(iommu->pmdomains);
+
 	pm_runtime_force_suspend(&pdev->dev);
 }