| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ffviz6ak6qsn2reg5y35aerzy7wxfx6fzix6xjyminbhfcguus@clszdjakdcjd>
Date: Thu, 13 Jun 2024 02:05:02 +0200
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Tomeu Vizoso <tomeu@...euvizoso.net>
Cc: Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
Robin Murphy <robin.murphy@....com>, Heiko Stuebner <heiko@...ech.de>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Oded Gabbay <ogabbay@...nel.org>, Tomeu Vizoso <tomeu.vizoso@...euvizoso.net>,
David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>, Philipp Zabel <p.zabel@...gutronix.de>,
Sumit Semwal <sumit.semwal@...aro.org>, Christian König <christian.koenig@....com>,
iommu@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
dri-devel@...ts.freedesktop.org, linux-media@...r.kernel.org, linaro-mm-sig@...ts.linaro.org
Subject: Re: [PATCH 2/9] iommu/rockchip: Attach multiple power domains
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@...euvizoso.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@...omium.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
>
>
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists