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

[Tomeu Vizoso]

On Thu, Jun 13, 2024 at 2:05 AM Sebastian Reichel
<sebastian.reichel at 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 at 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 at 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
> >
> >