[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <3f003164-f77d-4357-a21c-14d9ec16e46a@arm.com>
Date: Thu, 10 Jul 2025 23:55:15 +0100
From: Andre Przywara <andre.przywara@....com>
To: wens@...nel.org
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
Hi,
On 10/07/2025 05:14, Chen-Yu Tsai wrote:
> 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.
Yeah, fair enough, I was just curious. It's fine either way, we can
adjust this internal detail later, if need be.
>>> +};
>>> +
>>> +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.
Right, I was adittedly lazy yesterday, but now looked it up: indeed
there is a lock around every call to genpd_power_{off,on}, so it's all good.
Cheers,
Andre
>
>
> 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