| 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: <CAGb2v65WsPmAtUZRbf5Cu3Pb6Oi+dGH1RH3HhnZLmpJgh5Nu1g@mail.gmail.com>
Date: Thu, 10 Jul 2025 12:14:50 +0800
From: Chen-Yu Tsai <wens@...nel.org>
To: Andre Przywara <andre.przywara@....com>
Cc: Jernej Skrabec <jernej@...nel.org>, Samuel Holland <samuel@...lland.org>,
Ulf Hansson <ulf.hansson@...aro.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-sunxi@...ts.linux.dev,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 3/4] pmdomain: sunxi: add driver for Allwinner A523's
PCK-600 power controller
On Thu, Jul 10, 2025 at 7:46 AM Andre Przywara <andre.przywara@....com> wrote:
>
> Hi Chen-Yu,
>
> thanks for posting this! This is a quick first view, haven't compared
> against the BSP bits yet....
>
> On 09/07/2025 16:53, Chen-Yu Tsai wrote:
> > From: Chen-Yu Tsai <wens@...e.org>
> >
> > Allwinner A523 family has a second power controller, named PCK-600 in
> > the datasheets and BSP. It is likely based on ARM's PCK-600 hardware
> > block, with some additional delay controls. The only documentation for
> > this hardware is the BSP driver. The standard registers defined in ARM's
> > Power Policy Unit Architecture Specification line up. Some extra delay
> > controls are found in the reserved range of registers.
> >
> > Add a driver for this power controller. Delay control register values
> > and power domain names are from the BSP driver.
> >
> > Signed-off-by: Chen-Yu Tsai <wens@...e.org>
> > ---
> > drivers/pmdomain/sunxi/Kconfig | 8 +
> > drivers/pmdomain/sunxi/Makefile | 1 +
> > drivers/pmdomain/sunxi/sun55i-pck600.c | 225 +++++++++++++++++++++++++
> > 3 files changed, 234 insertions(+)
> > create mode 100644 drivers/pmdomain/sunxi/sun55i-pck600.c
> >
> > diff --git a/drivers/pmdomain/sunxi/Kconfig b/drivers/pmdomain/sunxi/Kconfig
> > index 43eecb3ea981..3e2b77cd9a2b 100644
> > --- a/drivers/pmdomain/sunxi/Kconfig
> > +++ b/drivers/pmdomain/sunxi/Kconfig
> > @@ -18,3 +18,11 @@ config SUN50I_H6_PRCM_PPU
> > Say y to enable the Allwinner H6/H616 PRCM power domain driver.
> > This is required to enable the Mali GPU in the H616 SoC, it is
> > optional for the H6.
> > +
> > +config SUN55I_PCK600
> > + bool "Allwinner A523 PCK-600 power domain driver"
>
> Any particular reason this is not tristate? The driver advertises itself
> as a platform driver module?
Cargo-culted from the D1 PPU driver. So, no particular reason.
> > + depends on PM
> > + select PM_GENERIC_DOMAINS
> > + help
> > + Say y to enable the PCK-600 power domain driver. This saves power
> > + when certain peripherals, such as the video engine, are idle.
>
> If I understand correctly, this driver is *required* to make use of
> those peripherals, and the video engine is not even the most prominent
> user. So regardless of the reset state of the power domain, I think the
> wording should be changed, to make sure distributions activate this
> option. At the moment it sounds highly optional. I wonder if we should
> use "default y if ARCH_SUNXI" even.
Makes sense. Though this was also cargo-culted from the D1 PPU. So I
guess I should fix both.
> > diff --git a/drivers/pmdomain/sunxi/Makefile b/drivers/pmdomain/sunxi/Makefile
> > index c1343e123759..e344b232fc9f 100644
> > --- a/drivers/pmdomain/sunxi/Makefile
> > +++ b/drivers/pmdomain/sunxi/Makefile
> > @@ -1,3 +1,4 @@
> > # SPDX-License-Identifier: GPL-2.0-only
> > obj-$(CONFIG_SUN20I_PPU) += sun20i-ppu.o
> > obj-$(CONFIG_SUN50I_H6_PRCM_PPU) += sun50i-h6-prcm-ppu.o
> > +obj-$(CONFIG_SUN55I_PCK600) += sun55i-pck600.o
> > diff --git a/drivers/pmdomain/sunxi/sun55i-pck600.c b/drivers/pmdomain/sunxi/sun55i-pck600.c
> > new file mode 100644
> > index 000000000000..7248f6113665
> > --- /dev/null
> > +++ b/drivers/pmdomain/sunxi/sun55i-pck600.c
> > @@ -0,0 +1,225 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Allwinner PCK-600 power domain support
>
> Can you please mention here that this device is based on the Arm PCK-600
> IP, as done in the commit message. And say that this is a minimal
> implementaton, just supporting the off/on states.
>
> Maybe also mention the relevant documentation: the "ARM CoreLink PCK‑600
> Power Control Kit" TRM and the "Arm Power Policy Unit" architecture
> specification (DEN0051E).
Will do.
> > + *
> > + * Copyright (c) 2025 Chen-Yu Tsai <wens@...e.org>
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/container_of.h>
> > +#include <linux/device.h>
> > +#include <linux/dev_printk.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/reset.h>
> > +#include <linux/slab.h>
> > +#include <linux/string_choices.h>
> > +
> > +#define PPU_PWPR 0x0
> > +#define PPU_PWSR 0x8
> > +#define PPU_DCDR0 0x170
>
> white space issue?
>
> > +#define PPU_DCDR1 0x174
> > +
> > +#define PPU_PWSR_PWR_STATUS GENMASK(3, 0)
>
> Would just PPU_PWR_STATUS be a better name, since it's used by both the
> PWPR and PWSR registers?
>
> > +#define PPU_POWER_MODE_ON 0x8
> > +#define PPU_POWER_MODE_OFF 0x0
> > +
> > +#define PPU_REG_SIZE 0x1000
> > +
> > +struct sunxi_pck600_desc {
> > + const char * const *pd_names;
> > + unsigned int num_domains;
> > + u32 logic_power_switch0_delay_offset;
> > + u32 logic_power_switch1_delay_offset;
> > + u32 off2on_delay_offset;
> > + u32 device_ctrl0_delay;
> > + u32 device_ctrl1_delay;
> > + u32 logic_power_switch0_delay;
> > + u32 logic_power_switch1_delay;
> > + u32 off2on_delay;
>
> Is there any indication that those parameters are different between
> different SoCs? I appreciate the idea of making this future-proof, but
> this might be a bit premature, if all SoCs use the same values?
It's hard to tell since the BSP driver only covers the A523. I'd just
keep this the way it is, since it makes it easier to generalize this
to cover PCK-600 in other platforms, if such a need ever presents itself.
> > +};
> > +
> > +struct sunxi_pck600_pd {
> > + struct generic_pm_domain genpd;
> > + struct sunxi_pck600 *pck;
> > + void __iomem *base;
> > +};
> > +
> > +struct sunxi_pck600 {
> > + struct device *dev;
> > + struct genpd_onecell_data genpd_data;
> > + struct sunxi_pck600_pd pds[];
> > +};
> > +
> > +#define to_sunxi_pd(gpd) container_of(gpd, struct sunxi_pck600_pd, genpd)
> > +
> > +static int sunxi_pck600_pd_set_power(struct sunxi_pck600_pd *pd, bool on)
> > +{
> > + struct sunxi_pck600 *pck = pd->pck;
> > + struct generic_pm_domain *genpd = &pd->genpd;
> > + int ret;
> > + u32 val, reg;
> > +
> > + val = on ? PPU_POWER_MODE_ON : PPU_POWER_MODE_OFF;
> > +
> > + reg = readl(pd->base + PPU_PWPR);
> > + FIELD_MODIFY(PPU_PWSR_PWR_STATUS, ®, val);
> > + writel(reg, pd->base + PPU_PWPR);
>
> Don't we need a lock here, or is this covered by the power domain framework?
AFAICT genpd has a lock for each power domain. Since each power domain has
its own set of registers, I think we're good here.
Thanks
ChenYu
> Cheers,
> Andre
>
> > +
> > + /* push write out to hardware */
> > + reg = readl(pd->base + PPU_PWPR);
> > +
> > + ret = readl_poll_timeout_atomic(pd->base + PPU_PWSR, reg,
> > + FIELD_GET(PPU_PWSR_PWR_STATUS, reg) == val,
> > + 0, 10000);
> > + if (ret)
> > + dev_err(pck->dev, "failed to turn domain \"%s\" %s: %d\n",
> > + genpd->name, str_on_off(on), ret);
> > +
> > + return ret;
> > +}
> > +
> > +static int sunxi_pck600_power_on(struct generic_pm_domain *domain)
> > +{
> > + struct sunxi_pck600_pd *pd = to_sunxi_pd(domain);
> > +
> > + return sunxi_pck600_pd_set_power(pd, true);
> > +}
> > +
> > +static int sunxi_pck600_power_off(struct generic_pm_domain *domain)
> > +{
> > + struct sunxi_pck600_pd *pd = to_sunxi_pd(domain);
> > +
> > + return sunxi_pck600_pd_set_power(pd, false);
> > +}
> > +
> > +static void sunxi_pck600_pd_setup(struct sunxi_pck600_pd *pd,
> > + const struct sunxi_pck600_desc *desc)
> > +{
> > + writel(desc->device_ctrl0_delay, pd->base + PPU_DCDR0);
> > + writel(desc->device_ctrl1_delay, pd->base + PPU_DCDR1);
> > + writel(desc->logic_power_switch0_delay,
> > + pd->base + desc->logic_power_switch0_delay_offset);
> > + writel(desc->logic_power_switch1_delay,
> > + pd->base + desc->logic_power_switch1_delay_offset);
> > + writel(desc->off2on_delay, pd->base + desc->off2on_delay_offset);
> > +}
> > +
> > +static int sunxi_pck600_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + const struct sunxi_pck600_desc *desc;
> > + struct genpd_onecell_data *genpds;
> > + struct sunxi_pck600 *pck;
> > + struct reset_control *rst;
> > + struct clk *clk;
> > + void __iomem *base;
> > + int i, ret;
> > +
> > + desc = of_device_get_match_data(dev);
> > +
> > + pck = devm_kzalloc(dev, struct_size(pck, pds, desc->num_domains), GFP_KERNEL);
> > + if (!pck)
> > + return -ENOMEM;
> > +
> > + pck->dev = &pdev->dev;
> > + platform_set_drvdata(pdev, pck);
> > +
> > + genpds = &pck->genpd_data;
> > + genpds->num_domains = desc->num_domains;
> > + genpds->domains = devm_kcalloc(dev, desc->num_domains,
> > + sizeof(*genpds->domains), GFP_KERNEL);
> > + if (!genpds->domains)
> > + return -ENOMEM;
> > +
> > + base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(base))
> > + return PTR_ERR(base);
> > +
> > + rst = devm_reset_control_get_exclusive_released(dev, NULL);
> > + if (IS_ERR(rst))
> > + return dev_err_probe(dev, PTR_ERR(rst), "failed to get reset control\n");
> > +
> > + clk = devm_clk_get_enabled(dev, NULL);
> > + if (IS_ERR(clk))
> > + return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
> > +
> > + for (i = 0; i < desc->num_domains; i++) {
> > + struct sunxi_pck600_pd *pd = &pck->pds[i];
> > +
> > + pd->genpd.name = desc->pd_names[i];
> > + pd->genpd.power_off = sunxi_pck600_power_off;
> > + pd->genpd.power_on = sunxi_pck600_power_on;
> > + pd->base = base + PPU_REG_SIZE * i;
> > +
> > + sunxi_pck600_pd_setup(pd, desc);
> > + ret = pm_genpd_init(&pd->genpd, NULL, false);
> > + if (ret) {
> > + dev_err_probe(dev, ret, "failed to initialize power domain\n");
> > + goto err_remove_pds;
> > + }
> > +
> > + genpds->domains[i] = &pd->genpd;
> > + }
> > +
> > + ret = of_genpd_add_provider_onecell(dev_of_node(dev), genpds);
> > + if (ret) {
> > + dev_err_probe(dev, ret, "failed to add PD provider\n");
> > + goto err_remove_pds;
> > + }
> > +
> > + return 0;
> > +
> > +err_remove_pds:
> > + for (i--; i >= 0; i--)
> > + pm_genpd_remove(genpds->domains[i]);
> > +
> > + return ret;
> > +}
> > +
> > +static const char * const sun55i_a523_pck600_pd_names[] = {
> > + "VE", "GPU", "VI", "VO0", "VO1", "DE", "NAND", "PCIE"
> > +};
> > +
> > +static const struct sunxi_pck600_desc sun55i_a523_pck600_desc = {
> > + .pd_names = sun55i_a523_pck600_pd_names,
> > + .num_domains = ARRAY_SIZE(sun55i_a523_pck600_pd_names),
> > + .logic_power_switch0_delay_offset = 0xc00,
> > + .logic_power_switch1_delay_offset = 0xc04,
> > + .off2on_delay_offset = 0xc10,
> > + .device_ctrl0_delay = 0xffffff,
> > + .device_ctrl1_delay = 0xffff,
> > + .logic_power_switch0_delay = 0x8080808,
> > + .logic_power_switch1_delay = 0x808,
> > + .off2on_delay = 0x8
> > +};
> > +
> > +static const struct of_device_id sunxi_pck600_of_match[] = {
> > + {
> > + .compatible = "allwinner,sun55i-a523-pck-600",
> > + .data = &sun55i_a523_pck600_desc,
> > + },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, sunxi_pck600_of_match);
> > +
> > +static struct platform_driver sunxi_pck600_driver = {
> > + .probe = sunxi_pck600_probe,
> > + .driver = {
> > + .name = "sunxi-pck-600",
> > + .of_match_table = sunxi_pck600_of_match,
> > + /* Power domains cannot be removed if in use. */
> > + .suppress_bind_attrs = true,
> > + },
> > +};
> > +module_platform_driver(sunxi_pck600_driver);
> > +
> > +MODULE_DESCRIPTION("Allwinner PCK-600 power domain driver");
> > +MODULE_AUTHOR("Chen-Yu Tsai <wens@...e.org>");
> > +MODULE_LICENSE("GPL");
>
>
Powered by blists - more mailing lists