On Thu, Jun 13, 2024 at 2:05 AM Sebastian Reichel
<sebastian.reichel@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > ---
>
> 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@xxxxxxxxxxxx>
> > */
> >
> > +#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
> >
> >
