[v1,3/4] soc: starfive: Add StarFive JH71XX pmu driver

Add generic power domain driver for the StarFive JH71XX SoC.

Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
---
 MAINTAINERS                       |   8 +
 drivers/soc/Kconfig               |   1 +
 drivers/soc/Makefile              |   1 +
 drivers/soc/starfive/Kconfig      |   9 +
 drivers/soc/starfive/Makefile     |   3 +
 drivers/soc/starfive/jh71xx_pmu.c | 380 ++++++++++++++++++++++++++++++
 include/soc/starfive/pm_domains.h |  15 ++
 7 files changed, 417 insertions(+)
 create mode 100644 drivers/soc/starfive/Kconfig
 create mode 100644 drivers/soc/starfive/Makefile
 create mode 100644 drivers/soc/starfive/jh71xx_pmu.c
 create mode 100644 include/soc/starfive/pm_domains.h

On Fri, 18 Nov 2022 at 14:35, Walker Chen <walker.chen@starfivetech.com> wrote:
>
> Add generic power domain driver for the StarFive JH71XX SoC.

SoCs

> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> ---
>  MAINTAINERS                       |   8 +
>  drivers/soc/Kconfig               |   1 +
>  drivers/soc/Makefile              |   1 +
>  drivers/soc/starfive/Kconfig      |   9 +
>  drivers/soc/starfive/Makefile     |   3 +
>  drivers/soc/starfive/jh71xx_pmu.c | 380 ++++++++++++++++++++++++++++++
>  include/soc/starfive/pm_domains.h |  15 ++
>  7 files changed, 417 insertions(+)
>  create mode 100644 drivers/soc/starfive/Kconfig
>  create mode 100644 drivers/soc/starfive/Makefile
>  create mode 100644 drivers/soc/starfive/jh71xx_pmu.c
>  create mode 100644 include/soc/starfive/pm_domains.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a70c1d0f303e..112f1e698723 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19623,6 +19623,14 @@ F:     Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
>  F:     drivers/reset/starfive/
>  F:     include/dt-bindings/reset/starfive*

checkpatch.pl complains about not sorting these lines alphabetically.

> +STARFIVE JH71XX PMU CONTROLLER DRIVER
> +M:     Walker Chen <walker.chen@starfivetech.com>
> +S:     Maintained
> +F:     Documentation/devicetree/bindings/power/starfive*
> +F:     drivers/soc/starfive/jh71xx_pmu.c
> +F:     include/soc/starfive/pm_domains.h
> +F:     include/dt-bindings/power/jh7110-power.h
> +
>  STATIC BRANCH/CALL
>  M:     Peter Zijlstra <peterz@infradead.org>
>  M:     Josh Poimboeuf <jpoimboe@kernel.org>
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index e461c071189b..628fda4d5ed9 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -21,6 +21,7 @@ source "drivers/soc/renesas/Kconfig"
>  source "drivers/soc/rockchip/Kconfig"
>  source "drivers/soc/samsung/Kconfig"
>  source "drivers/soc/sifive/Kconfig"
> +source "drivers/soc/starfive/Kconfig"
>  source "drivers/soc/sunxi/Kconfig"
>  source "drivers/soc/tegra/Kconfig"
>  source "drivers/soc/ti/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 534669840858..cbe076f42068 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -27,6 +27,7 @@ obj-y                         += renesas/
>  obj-y                          += rockchip/
>  obj-$(CONFIG_SOC_SAMSUNG)      += samsung/
>  obj-y                          += sifive/
> +obj-y                          += starfive/
>  obj-y                          += sunxi/
>  obj-$(CONFIG_ARCH_TEGRA)       += tegra/
>  obj-y                          += ti/
> diff --git a/drivers/soc/starfive/Kconfig b/drivers/soc/starfive/Kconfig
> new file mode 100644
> index 000000000000..2bbcc1397b15
> --- /dev/null
> +++ b/drivers/soc/starfive/Kconfig
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +config JH71XX_PMU
> +       bool "Support PMU for StarFive JH71XX Soc"
> +       depends on PM && (SOC_STARFIVE || COMPILE_TEST)
> +       select PM_GENERIC_DOMAINS
> +       help
> +         Say y here to enable power domain support.

Please expand this help text. Even checkpatch.pl complains about it.

> +
> diff --git a/drivers/soc/starfive/Makefile b/drivers/soc/starfive/Makefile
> new file mode 100644
> index 000000000000..13b589d6b5f3
> --- /dev/null
> +++ b/drivers/soc/starfive/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_JH71XX_PMU)       += jh71xx_pmu.o
> diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c
> new file mode 100644
> index 000000000000..e6c0083d166e
> --- /dev/null
> +++ b/drivers/soc/starfive/jh71xx_pmu.c
> @@ -0,0 +1,380 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * StarFive JH71XX Power Domain Controller Driver
> + *
> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <dt-bindings/power/jh7110-power.h>
> +
> +/* register offset */
> +#define HW_EVENT_TURN_ON_MASK          0x04
> +#define HW_EVENT_TURN_OFF_MASK         0x08
> +#define SW_TURN_ON_POWER_MODE          0x0C
> +#define SW_TURN_OFF_POWER_MODE         0x10
> +#define SW_ENCOURAGE                   0x44
> +#define PMU_INT_MASK                   0x48
> +#define PCH_BYPASS                     0x4C
> +#define PCH_PSTATE                     0x50
> +#define PCH_TIMEOUT                    0x54
> +#define LP_TIMEOUT                     0x58
> +#define HW_TURN_ON_MODE                        0x5C
> +#define CURR_POWER_MODE                        0x80
> +#define PMU_EVENT_STATUS               0x88
> +#define PMU_INT_STATUS                 0x8C
> +
> +/* sw encourage cfg */
> +#define SW_MODE_ENCOURAGE_EN_LO                0x05
> +#define SW_MODE_ENCOURAGE_EN_HI                0x50
> +#define SW_MODE_ENCOURAGE_DIS_LO       0x0A
> +#define SW_MODE_ENCOURAGE_DIS_HI       0xA0
> +#define SW_MODE_ENCOURAGE_ON           0xFF
> +
> +/* pmu int status */
> +#define PMU_INT_SEQ_DONE               BIT(0)
> +#define PMU_INT_HW_REQ                 BIT(1)
> +#define PMU_INT_SW_FAIL                        GENMASK(3, 2)
> +#define PMU_INT_HW_FAIL                        GENMASK(5, 4)
> +#define PMU_INT_PCH_FAIL               GENMASK(8, 6)
> +#define PMU_INT_FAIL_MASK              (PMU_INT_SW_FAIL | \
> +                                       PMU_INT_HW_FAIL | \
> +                                       PMU_INT_PCH_FAIL)
> +#define PMU_INT_ALL_MASK               (PMU_INT_SEQ_DONE | \
> +                                       PMU_INT_HW_REQ | \
> +                                       PMU_INT_FAIL_MASK)
> +
> +#define DELAY_US                       10
> +#define TIMEOUT_US                     100000

Please add a JH71XX_PMU_ prefix to these definitions to avoid
accidentally clashing with a definition added to a generic header.

> +
> +struct starfive_power_dev {
> +       struct generic_pm_domain genpd;
> +       struct starfive_pmu *power;
> +       uint32_t mask;

u32

> +};
> +
> +struct starfive_pmu {
> +       void __iomem *base;
> +       spinlock_t lock;
> +       int irq;
> +       struct device *pdev;
> +       struct starfive_power_dev *dev;
> +       struct genpd_onecell_data genpd_data;
> +       struct generic_pm_domain **genpd;
> +};
> +
> +struct starfive_pmu_data {
> +       const char * const name;
> +       uint8_t bit;

u8

> +       unsigned int flags;
> +};
> +
> +static void __iomem *pmu_base;
> +
> +static inline void pmu_writel(u32 val, u32 offset)
> +{
> +       writel(val, pmu_base + offset);
> +}
> +
> +void starfive_pmu_hw_event_turn_on(u32 mask)
> +{
> +       pmu_writel(mask, HW_EVENT_TURN_ON_MASK);
> +}
> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_on);
> +
> +void starfive_pmu_hw_event_turn_off(u32 mask)
> +{
> +       pmu_writel(mask, HW_EVENT_TURN_OFF_MASK);
> +}
> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_off);
> +
> +static int starfive_pmu_get_state(struct starfive_power_dev *pmd, bool *is_on)
> +{
> +       struct starfive_pmu *pmu = pmd->power;
> +
> +       if (!pmd->mask) {
> +               *is_on = false;
> +               return -EINVAL;
> +       }
> +
> +       *is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;
> +
> +       return 0;
> +}

Maybe just return an int where 0 = off, 1 = on and negative is an error.

> +
> +static int starfive_pmu_set_state(struct starfive_power_dev *pmd, bool on)
> +{
> +       struct starfive_pmu *pmu = pmd->power;
> +       unsigned long flags;
> +       uint32_t val;
> +       uint32_t mode;
> +       uint32_t encourage_lo;
> +       uint32_t encourage_hi;

u32

> +       bool is_on;
> +       int ret;
> +
> +       if (!pmd->mask)
> +               return -EINVAL;
> +
> +       if (is_on == on) {
> +               dev_info(pmu->pdev, "pm domain [%s] is already %sable status.\n",
> +                               pmd->genpd.name, on ? "en" : "dis");
> +               return 0;
> +       }
> +
> +       spin_lock_irqsave(&pmu->lock, flags);
> +
> +       if (on) {
> +               mode = SW_TURN_ON_POWER_MODE;
> +               encourage_lo = SW_MODE_ENCOURAGE_EN_LO;
> +               encourage_hi = SW_MODE_ENCOURAGE_EN_HI;
> +       } else {
> +               mode = SW_TURN_OFF_POWER_MODE;
> +               encourage_lo = SW_MODE_ENCOURAGE_DIS_LO;
> +               encourage_hi = SW_MODE_ENCOURAGE_DIS_HI;
> +       }
> +
> +       __raw_writel(pmd->mask, pmu->base + mode);
> +
> +       /* write SW_ENCOURAGE to make the configuration take effect */
> +       __raw_writel(SW_MODE_ENCOURAGE_ON, pmu->base + SW_ENCOURAGE);
> +       __raw_writel(encourage_lo, pmu->base + SW_ENCOURAGE);
> +       __raw_writel(encourage_hi, pmu->base + SW_ENCOURAGE);
> +
> +       spin_unlock_irqrestore(&pmu->lock, flags);
> +
> +       if (on) {
> +               ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
> +                                               val & pmd->mask, DELAY_US,
> +                                               TIMEOUT_US);
> +               if (ret) {
> +                       dev_err(pmu->pdev, "%s: failed to power on\n", pmd->genpd.name);
> +                       return -ETIMEDOUT;
> +               }
> +       } else {
> +               ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
> +                                               !(val & pmd->mask), DELAY_US,
> +                                               TIMEOUT_US);
> +               if (ret) {
> +                       dev_err(pmu->pdev, "%s: failed to power off\n", pmd->genpd.name);
> +                       return -ETIMEDOUT;
> +               }
> +       }

You can move the dev_err out. Eg. dev_err(pmu->pdev, "%s: failed to
power %s\n", pmd->genpd.name, on ? "on" : "off")

> +       return 0;
> +}
> +
> +static int starfive_pmu_on(struct generic_pm_domain *genpd)
> +{
> +       struct starfive_power_dev *pmd = container_of(genpd,
> +               struct starfive_power_dev, genpd);

checkpatch.pl --strict complains that arguments doesn't match the open
parenthesis

> +
> +       return starfive_pmu_set_state(pmd, true);
> +}
> +
> +static int starfive_pmu_off(struct generic_pm_domain *genpd)
> +{
> +       struct starfive_power_dev *pmd = container_of(genpd,
> +               struct starfive_power_dev, genpd);
> +
> +       return starfive_pmu_set_state(pmd, false);
> +}
> +
> +static void starfive_pmu_int_enable(struct starfive_pmu *pmu, u32 mask, bool enable)
> +{
> +       u32 val = __raw_readl(pmu->base + PMU_INT_MASK);
> +
> +       if (enable)
> +               val &= ~mask;
> +       else
> +               val |= mask;
> +
> +       __raw_writel(val, pmu->base + PMU_INT_MASK);
> +}

Consider adding comments to functions that need a lock to be held to
be used safely.

> +
> +static irqreturn_t starfive_pmu_interrupt(int irq, void *data)
> +{
> +       struct starfive_pmu *pmu = data;
> +       unsigned long flags;
> +       u32 val;
> +
> +       spin_lock_irqsave(&pmu->lock, flags);
> +       val = __raw_readl(pmu->base + PMU_INT_STATUS);
> +
> +       if (val & PMU_INT_SEQ_DONE)
> +               dev_dbg(pmu->pdev, "sequence done.\n");
> +       if (val & PMU_INT_HW_REQ)
> +               dev_dbg(pmu->pdev, "hardware encourage requestion.\n");
> +       if (val & PMU_INT_SW_FAIL)
> +               dev_err(pmu->pdev, "software encourage fail.\n");
> +       if (val & PMU_INT_HW_FAIL)
> +               dev_err(pmu->pdev, "hardware encourage fail.\n");
> +       if (val & PMU_INT_PCH_FAIL)
> +               dev_err(pmu->pdev, "p-channel fail event.\n");
> +
> +       /* clear interrupts */
> +       __raw_writel(val, pmu->base + PMU_INT_STATUS);
> +       __raw_writel(val, pmu->base + PMU_EVENT_STATUS);
> +
> +       spin_unlock_irqrestore(&pmu->lock, flags);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static int starfive_pmu_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +       const struct starfive_pmu_data *entry, *table;
> +       struct starfive_pmu *pmu;
> +       unsigned int i;
> +       uint8_t max_bit = 0;
> +       int ret;
> +
> +       pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
> +       if (!pmu)
> +               return -ENOMEM;
> +
> +       pmu_base = pmu->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(pmu->base))
> +               return PTR_ERR(pmu->base);
> +
> +       /* initialize pmu interrupt  */
> +       pmu->irq = platform_get_irq(pdev, 0);
> +       if (pmu->irq < 0)
> +               return pmu->irq;
> +
> +       ret = devm_request_irq(dev, pmu->irq, starfive_pmu_interrupt,
> +                              0, pdev->name, pmu);
> +       if (ret)
> +               dev_err(dev, "request irq failed.\n");
> +
> +       table = of_device_get_match_data(dev);
> +       if (!table)
> +               return -EINVAL;
> +
> +       pmu->pdev = dev;
> +       pmu->genpd_data.num_domains = 0;
> +       i = 0;
> +       for (entry = table; entry->name; entry++) {
> +               max_bit = max(max_bit, entry->bit);
> +               i++;
> +       }
> +
> +       if (!i)
> +               return -ENODEV;
> +
> +       pmu->genpd_data.num_domains = max_bit + 1;
> +
> +       pmu->dev = devm_kcalloc(dev, pmu->genpd_data.num_domains,
> +                                 sizeof(struct starfive_power_dev),
> +                                 GFP_KERNEL);
> +       if (!pmu->dev)
> +               return -ENOMEM;
> +
> +       pmu->genpd = devm_kcalloc(dev, pmu->genpd_data.num_domains,
> +                                   sizeof(struct generic_pm_domain *),
> +                                   GFP_KERNEL);
> +       if (!pmu->genpd)
> +               return -ENOMEM;
> +
> +       pmu->genpd_data.domains = pmu->genpd;
> +
> +       i = 0;
> +       for (entry = table; entry->name; entry++) {
> +               struct starfive_power_dev *pmd = &pmu->dev[i];
> +               bool is_on;
> +
> +               pmd->power = pmu;
> +               pmd->mask = BIT(entry->bit);
> +               pmd->genpd.name = entry->name;
> +               pmd->genpd.flags = entry->flags;
> +
> +               ret = starfive_pmu_get_state(pmd, &is_on);
> +               if (ret)
> +                       dev_warn(dev, "unable to get current state for %s\n",
> +                                pmd->genpd.name);
> +
> +               pmd->genpd.power_on = starfive_pmu_on;
> +               pmd->genpd.power_off = starfive_pmu_off;
> +
> +               pm_genpd_init(&pmd->genpd, NULL, !is_on);
> +               pmu->genpd[entry->bit] = &pmd->genpd;
> +
> +               i++;
> +       }
> +
> +       spin_lock_init(&pmu->lock);
> +       starfive_pmu_int_enable(pmu, PMU_INT_ALL_MASK & ~PMU_INT_PCH_FAIL, true);
> +
> +       ret = of_genpd_add_provider_onecell(np, &pmu->genpd_data);
> +       if (ret) {
> +               dev_err(dev, "failed to register genpd driver: %d\n", ret);
> +               return ret;
> +       }
> +
> +       dev_info(dev, "registered %u power domains\n", i);
> +
> +       return 0;
> +}
> +
> +static const struct starfive_pmu_data jh7110_power_domains[] = {
> +       {
> +               .name = "SYSTOP",
> +               .bit = JH7110_PD_SYSTOP,
> +               .flags = GENPD_FLAG_ALWAYS_ON,
> +       }, {
> +               .name = "CPU",
> +               .bit = JH7110_PD_CPU,
> +               .flags = GENPD_FLAG_ALWAYS_ON,
> +       }, {
> +               .name = "GPUA",
> +               .bit = JH7110_PD_GPUA,
> +       }, {
> +               .name = "VDEC",
> +               .bit = JH7110_PD_VDEC,
> +       }, {
> +               .name = "VOUT",
> +               .bit = JH7110_PD_VOUT,
> +       }, {
> +               .name = "ISP",
> +               .bit = JH7110_PD_ISP,
> +       }, {
> +               .name = "VENC",
> +               .bit = JH7110_PD_VENC,
> +       }, {
> +               .name = "GPUB",
> +               .bit = JH7110_PD_GPUB,
> +       }, {
> +               /* sentinel */
> +       },
> +};
> +
> +static const struct of_device_id starfive_pmu_of_match[] = {
> +       {
> +               .compatible = "starfive,jh7110-pmu",
> +               .data = &jh7110_power_domains,
> +       }, {
> +               /* sentinel */
> +       }
> +};
> +
> +static struct platform_driver jh71xx_pmu_driver = {
> +       .driver = {
> +               .name = "jh71xx-pmu",
> +               .of_match_table = starfive_pmu_of_match,
> +       },
> +       .probe  = starfive_pmu_probe,
> +};
> +builtin_platform_driver(jh71xx_pmu_driver);

You're using a mix of starfive_, starfive_pmu_ and jh71xx_pmu_
prefixes. Please choose one, preferably jh71xx_pmu_ since that's what
you call the driver.

> +MODULE_LICENSE("GPL");
> diff --git a/include/soc/starfive/pm_domains.h b/include/soc/starfive/pm_domains.h
> new file mode 100644
> index 000000000000..a20e28e9baf3
> --- /dev/null
> +++ b/include/soc/starfive/pm_domains.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> + * Author: Walker Chen <walker.chen@starfivetech.com>
> + */
> +
> +#ifndef __SOC_STARFIVE_PMDOMAINS_H__
> +#define __SOC_STARFIVE_PMDOMAINS_H__
> +
> +#include <linux/types.h>
> +
> +void starfive_pmu_hw_event_turn_on(u32 mask);
> +void starfive_pmu_hw_event_turn_off(u32 mask);
> +
> +#endif /* __SOC_STARFIVE_PMDOMAINS_H__ */

The header and functions within are named very generic, but
implemented by the jh71xx-specific driver.

Also who should use this header? These functions are never called by
anything in this series.

> --
> 2.17.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Hey Walker,

On Fri, Nov 18, 2022 at 09:32:15PM +0800, Walker Chen wrote:
> Add generic power domain driver for the StarFive JH71XX SoC.
> 
> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> ---
> diff --git a/drivers/soc/starfive/Kconfig b/drivers/soc/starfive/Kconfig
> new file mode 100644
> index 000000000000..2bbcc1397b15
> --- /dev/null
> +++ b/drivers/soc/starfive/Kconfig
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +config JH71XX_PMU
> +	bool "Support PMU for StarFive JH71XX Soc"
> +	depends on PM && (SOC_STARFIVE || COMPILE_TEST)

`default SOC_STARFIVE` perhaps?

> +	select PM_GENERIC_DOMAINS
> +	help
> +	  Say y here to enable power domain support.
> +
> diff --git a/drivers/soc/starfive/Makefile b/drivers/soc/starfive/Makefile
> new file mode 100644
> index 000000000000..13b589d6b5f3
> --- /dev/null
> +++ b/drivers/soc/starfive/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_JH71XX_PMU)	+= jh71xx_pmu.o
> diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c
> new file mode 100644
> index 000000000000..e6c0083d166e
> --- /dev/null
> +++ b/drivers/soc/starfive/jh71xx_pmu.c
> @@ -0,0 +1,380 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * StarFive JH71XX Power Domain Controller Driver
> + *
> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <dt-bindings/power/jh7110-power.h>
>
> +/* register offset */
> +#define HW_EVENT_TURN_ON_MASK		0x04
> +#define HW_EVENT_TURN_OFF_MASK		0x08
> +#define SW_TURN_ON_POWER_MODE		0x0C
> +#define SW_TURN_OFF_POWER_MODE		0x10
> +#define SW_ENCOURAGE			0x44
> +#define PMU_INT_MASK			0x48
> +#define PCH_BYPASS			0x4C
> +#define PCH_PSTATE			0x50
> +#define PCH_TIMEOUT			0x54
> +#define LP_TIMEOUT			0x58
> +#define HW_TURN_ON_MODE			0x5C
> +#define CURR_POWER_MODE			0x80
> +#define PMU_EVENT_STATUS		0x88
> +#define PMU_INT_STATUS			0x8C
> +
> +/* sw encourage cfg */
> +#define SW_MODE_ENCOURAGE_EN_LO		0x05
> +#define SW_MODE_ENCOURAGE_EN_HI		0x50
> +#define SW_MODE_ENCOURAGE_DIS_LO	0x0A
> +#define SW_MODE_ENCOURAGE_DIS_HI	0xA0
> +#define SW_MODE_ENCOURAGE_ON		0xFF
> +
> +/* pmu int status */
> +#define PMU_INT_SEQ_DONE		BIT(0)
> +#define PMU_INT_HW_REQ			BIT(1)
> +#define PMU_INT_SW_FAIL			GENMASK(3, 2)
> +#define PMU_INT_HW_FAIL			GENMASK(5, 4)
> +#define PMU_INT_PCH_FAIL		GENMASK(8, 6)
> +#define PMU_INT_FAIL_MASK		(PMU_INT_SW_FAIL | \
> +					PMU_INT_HW_FAIL | \
> +					PMU_INT_PCH_FAIL)
> +#define PMU_INT_ALL_MASK		(PMU_INT_SEQ_DONE | \
> +					PMU_INT_HW_REQ | \
> +					PMU_INT_FAIL_MASK)
> +
> +#define DELAY_US			10
> +#define TIMEOUT_US			100000

Some JH71XX_PMU_ prefixes for the above, as I think Emil pointed out,
would be great.

> +struct starfive_power_dev {
> +	struct generic_pm_domain genpd;
> +	struct starfive_pmu *power;
> +	uint32_t mask;
> +};
> +
> +struct starfive_pmu {
> +	void __iomem *base;
> +	spinlock_t lock;
> +	int irq;
> +	struct device *pdev;
> +	struct starfive_power_dev *dev;
> +	struct genpd_onecell_data genpd_data;
> +	struct generic_pm_domain **genpd;
> +};
> +
> +struct starfive_pmu_data {
> +	const char * const name;
> +	uint8_t bit;
> +	unsigned int flags;

Generally, ordering pointers first in these structs would be nice.

> +};
> +
> +static void __iomem *pmu_base;
> +
> +static inline void pmu_writel(u32 val, u32 offset)
> +{
> +	writel(val, pmu_base + offset);
> +}
> +
> +void starfive_pmu_hw_event_turn_on(u32 mask)
> +{
> +	pmu_writel(mask, HW_EVENT_TURN_ON_MASK);
> +}
> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_on);
> +
> +void starfive_pmu_hw_event_turn_off(u32 mask)
> +{
> +	pmu_writel(mask, HW_EVENT_TURN_OFF_MASK);
> +}
> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_off);

Where are the users for these exports? Also, should they be exported as
GPL?

Either way, what is the point of the extra layer of abstraction here
around the writel()?

> +static int starfive_pmu_get_state(struct starfive_power_dev *pmd, bool *is_on)
> +{
> +	struct starfive_pmu *pmu = pmd->power;
> +
> +	if (!pmd->mask) {
> +		*is_on = false;
> +		return -EINVAL;
> +	}
> +
> +	*is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;

Is there a specific reason that you are using the __raw variants here
(and elsewhere) in the driver?

> +
> +	return 0;
> +}
> +
> +static int starfive_pmu_set_state(struct starfive_power_dev *pmd, bool on)
> +{
> +	struct starfive_pmu *pmu = pmd->power;
> +	unsigned long flags;
> +	uint32_t val;
> +	uint32_t mode;
> +	uint32_t encourage_lo;
> +	uint32_t encourage_hi;
> +	bool is_on;
> +	int ret;
> +
> +	if (!pmd->mask)
> +		return -EINVAL;
> +
> +	if (is_on == on) {
> +		dev_info(pmu->pdev, "pm domain [%s] is already %sable status.\n",
> +				pmd->genpd.name, on ? "en" : "dis");

Am I missing something here: you've just declared is_on, so it must be
false & therefore this check is all a little pointless? The check just
becomes if (false == on) which I don't think is what you're going for
here? Or did I miss something obvious?

> +		return 0;
> +	}
> +
> +	spin_lock_irqsave(&pmu->lock, flags);
> +
> +	if (on) {
> +		mode = SW_TURN_ON_POWER_MODE;
> +		encourage_lo = SW_MODE_ENCOURAGE_EN_LO;
> +		encourage_hi = SW_MODE_ENCOURAGE_EN_HI;
> +	} else {
> +		mode = SW_TURN_OFF_POWER_MODE;
> +		encourage_lo = SW_MODE_ENCOURAGE_DIS_LO;
> +		encourage_hi = SW_MODE_ENCOURAGE_DIS_HI;
> +	}
> +
> +	__raw_writel(pmd->mask, pmu->base + mode);
> +
> +	/* write SW_ENCOURAGE to make the configuration take effect */
> +	__raw_writel(SW_MODE_ENCOURAGE_ON, pmu->base + SW_ENCOURAGE);

Is register "sticky"? IOW, could you set it in probe and leave this mode
always on? Or does it need to be set every time you want to use this
feature?

> +	__raw_writel(encourage_lo, pmu->base + SW_ENCOURAGE);
> +	__raw_writel(encourage_hi, pmu->base + SW_ENCOURAGE);
> +
> +	spin_unlock_irqrestore(&pmu->lock, flags);
> +
> +	if (on) {
> +		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
> +						val & pmd->mask, DELAY_US,
> +						TIMEOUT_US);
> +		if (ret) {
> +			dev_err(pmu->pdev, "%s: failed to power on\n", pmd->genpd.name);
> +			return -ETIMEDOUT;
> +		}
> +	} else {
> +		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
> +						!(val & pmd->mask), DELAY_US,
> +						TIMEOUT_US);

Could you not just decide the 3rd arg outside of the readl_poll..() and
save on duplicating the wait logic here?

> +		if (ret) {
> +			dev_err(pmu->pdev, "%s: failed to power off\n", pmd->genpd.name);
> +			return -ETIMEDOUT;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int starfive_pmu_on(struct generic_pm_domain *genpd)
> +{
> +	struct starfive_power_dev *pmd = container_of(genpd,
> +		struct starfive_power_dev, genpd);
> +
> +	return starfive_pmu_set_state(pmd, true);
> +}
> +
> +static int starfive_pmu_off(struct generic_pm_domain *genpd)
> +{
> +	struct starfive_power_dev *pmd = container_of(genpd,
> +		struct starfive_power_dev, genpd);
> +
> +	return starfive_pmu_set_state(pmd, false);
> +}
> +
> +static void starfive_pmu_int_enable(struct starfive_pmu *pmu, u32 mask, bool enable)
> +{
> +	u32 val = __raw_readl(pmu->base + PMU_INT_MASK);
> +
> +	if (enable)
> +		val &= ~mask;
> +	else
> +		val |= mask;
> +
> +	__raw_writel(val, pmu->base + PMU_INT_MASK);
> +}
> +
> +static irqreturn_t starfive_pmu_interrupt(int irq, void *data)
> +{
> +	struct starfive_pmu *pmu = data;
> +	unsigned long flags;
> +	u32 val;
> +
> +	spin_lock_irqsave(&pmu->lock, flags);
> +	val = __raw_readl(pmu->base + PMU_INT_STATUS);
> +
> +	if (val & PMU_INT_SEQ_DONE)
> +		dev_dbg(pmu->pdev, "sequence done.\n");
> +	if (val & PMU_INT_HW_REQ)
> +		dev_dbg(pmu->pdev, "hardware encourage requestion.\n");
> +	if (val & PMU_INT_SW_FAIL)
> +		dev_err(pmu->pdev, "software encourage fail.\n");
> +	if (val & PMU_INT_HW_FAIL)
> +		dev_err(pmu->pdev, "hardware encourage fail.\n");
> +	if (val & PMU_INT_PCH_FAIL)
> +		dev_err(pmu->pdev, "p-channel fail event.\n");
> +
> +	/* clear interrupts */
> +	__raw_writel(val, pmu->base + PMU_INT_STATUS);
> +	__raw_writel(val, pmu->base + PMU_EVENT_STATUS);
> +
> +	spin_unlock_irqrestore(&pmu->lock, flags);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int starfive_pmu_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	const struct starfive_pmu_data *entry, *table;
> +	struct starfive_pmu *pmu;
> +	unsigned int i;
> +	uint8_t max_bit = 0;
> +	int ret;
> +
> +	pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
> +	if (!pmu)
> +		return -ENOMEM;
> +
> +	pmu_base = pmu->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(pmu->base))
> +		return PTR_ERR(pmu->base);
> +
> +	/* initialize pmu interrupt  */
> +	pmu->irq = platform_get_irq(pdev, 0);
> +	if (pmu->irq < 0)
> +		return pmu->irq;
> +
> +	ret = devm_request_irq(dev, pmu->irq, starfive_pmu_interrupt,
> +			       0, pdev->name, pmu);
> +	if (ret)
> +		dev_err(dev, "request irq failed.\n");
> +
> +	table = of_device_get_match_data(dev);
> +	if (!table)
> +		return -EINVAL;
> +
> +	pmu->pdev = dev;
> +	pmu->genpd_data.num_domains = 0;
> +	i = 0;
> +	for (entry = table; entry->name; entry++) {
> +		max_bit = max(max_bit, entry->bit);
> +		i++;
> +	}

This looks like something that could be replaced by the functions in
linux/list.h, no? Same below.

> +
> +	if (!i)
> +		return -ENODEV;
> +
> +	pmu->genpd_data.num_domains = max_bit + 1;
> +
> +	pmu->dev = devm_kcalloc(dev, pmu->genpd_data.num_domains,
> +				  sizeof(struct starfive_power_dev),
> +				  GFP_KERNEL);
> +	if (!pmu->dev)
> +		return -ENOMEM;
> +
> +	pmu->genpd = devm_kcalloc(dev, pmu->genpd_data.num_domains,
> +				    sizeof(struct generic_pm_domain *),
> +				    GFP_KERNEL);

This two allocations both look like checkpatch would whinge about their
alignment.

> +	if (!pmu->genpd)
> +		return -ENOMEM;
> +
> +	pmu->genpd_data.domains = pmu->genpd;
> +
> +	i = 0;
> +	for (entry = table; entry->name; entry++) {
> +		struct starfive_power_dev *pmd = &pmu->dev[i];
> +		bool is_on;
> +
> +		pmd->power = pmu;
> +		pmd->mask = BIT(entry->bit);
> +		pmd->genpd.name = entry->name;
> +		pmd->genpd.flags = entry->flags;
> +
> +		ret = starfive_pmu_get_state(pmd, &is_on);
> +		if (ret)
> +			dev_warn(dev, "unable to get current state for %s\n",
> +				 pmd->genpd.name);

In what scenario would it not be possible to get the state? Again, I
hope I'm not missing something obvious. From what I can see, this is the
only caller of starfive_pmu_get_state(), which looks like:
> > +	struct starfive_pmu *pmu = pmd->power;
> > +
> > +	if (!pmd->mask) {

How can !pmd->mask evaluate to true, given it's just been set to BIT(n)?

> > +		*is_on = false;
> > +		return -EINVAL;
> > +	}
> > +
> > +	*is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;
> > +
> > +	return 0;

If I've not missed something, looks like you could likely remove the
function and just do the readl() here?

> +
> +		pmd->genpd.power_on = starfive_pmu_on;
> +		pmd->genpd.power_off = starfive_pmu_off;
> +
> +		pm_genpd_init(&pmd->genpd, NULL, !is_on);
> +		pmu->genpd[entry->bit] = &pmd->genpd;
> +
> +		i++;
> +	}
> +
> +	spin_lock_init(&pmu->lock);
> +	starfive_pmu_int_enable(pmu, PMU_INT_ALL_MASK & ~PMU_INT_PCH_FAIL, true);
> +
> +	ret = of_genpd_add_provider_onecell(np, &pmu->genpd_data);
> +	if (ret) {
> +		dev_err(dev, "failed to register genpd driver: %d\n", ret);
> +		return ret;
> +	}
> +
> +	dev_info(dev, "registered %u power domains\n", i);
> +
> +	return 0;
> +}
> +
> +static const struct starfive_pmu_data jh7110_power_domains[] = {

Is this driver jh7110 only or actually jh71XX? Have you just started out
by implementing one SoC both intend to support both?

> +	{
> +		.name = "SYSTOP",
> +		.bit = JH7110_PD_SYSTOP,
> +		.flags = GENPD_FLAG_ALWAYS_ON,
> +	}, {
> +		.name = "CPU",
> +		.bit = JH7110_PD_CPU,
> +		.flags = GENPD_FLAG_ALWAYS_ON,
> +	}, {
> +		.name = "GPUA",
> +		.bit = JH7110_PD_GPUA,
> +	}, {
> +		.name = "VDEC",
> +		.bit = JH7110_PD_VDEC,
> +	}, {
> +		.name = "VOUT",
> +		.bit = JH7110_PD_VOUT,
> +	}, {
> +		.name = "ISP",
> +		.bit = JH7110_PD_ISP,
> +	}, {
> +		.name = "VENC",
> +		.bit = JH7110_PD_VENC,
> +	}, {
> +		.name = "GPUB",
> +		.bit = JH7110_PD_GPUB,
> +	}, {
> +		/* sentinel */
> +	},

Can drop this , after the sentinel ;)

I don't know /jack/ about power domain stuff, so I can barely review
this at all sadly.
Thanks,
Conor.

> +};
> +
> +static const struct of_device_id starfive_pmu_of_match[] = {
> +	{
> +		.compatible = "starfive,jh7110-pmu",
> +		.data = &jh7110_power_domains,
> +	}, {
> +		/* sentinel */
> +	}
> +};
> +
> +static struct platform_driver jh71xx_pmu_driver = {
> +	.driver = {
> +		.name = "jh71xx-pmu",
> +		.of_match_table = starfive_pmu_of_match,
> +	},
> +	.probe  = starfive_pmu_probe,
> +};
> +builtin_platform_driver(jh71xx_pmu_driver);

Hi Walker,

Walker Chen <walker.chen@starfivetech.com> writes:

> Add generic power domain driver for the StarFive JH71XX SoC.
>
> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>

A bit more detail about the features power domain hardware would be
helpful for reviewers.  I'm assuming there's no public documenation for
this, but if there is, a link to that would be great also.

[...]

> diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c
> new file mode 100644
> index 000000000000..e6c0083d166e
> --- /dev/null
> +++ b/drivers/soc/starfive/jh71xx_pmu.c
> @@ -0,0 +1,380 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * StarFive JH71XX Power Domain Controller Driver
> + *
> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <dt-bindings/power/jh7110-power.h>
> +
> +/* register offset */
> +#define HW_EVENT_TURN_ON_MASK		0x04
> +#define HW_EVENT_TURN_OFF_MASK		0x08
> +#define SW_TURN_ON_POWER_MODE		0x0C
> +#define SW_TURN_OFF_POWER_MODE		0x10
> +#define SW_ENCOURAGE			0x44
> +#define PMU_INT_MASK			0x48
> +#define PCH_BYPASS			0x4C
> +#define PCH_PSTATE			0x50
> +#define PCH_TIMEOUT			0x54
> +#define LP_TIMEOUT			0x58
> +#define HW_TURN_ON_MODE			0x5C
> +#define CURR_POWER_MODE			0x80
> +#define PMU_EVENT_STATUS		0x88
> +#define PMU_INT_STATUS			0x8C
> +
> +/* sw encourage cfg */
> +#define SW_MODE_ENCOURAGE_EN_LO		0x05
> +#define SW_MODE_ENCOURAGE_EN_HI		0x50
> +#define SW_MODE_ENCOURAGE_DIS_LO	0x0A
> +#define SW_MODE_ENCOURAGE_DIS_HI	0xA0
> +#define SW_MODE_ENCOURAGE_ON		0xFF
> +
> +/* pmu int status */
> +#define PMU_INT_SEQ_DONE		BIT(0)
> +#define PMU_INT_HW_REQ			BIT(1)
> +#define PMU_INT_SW_FAIL			GENMASK(3, 2)
> +#define PMU_INT_HW_FAIL			GENMASK(5, 4)
> +#define PMU_INT_PCH_FAIL		GENMASK(8, 6)
> +#define PMU_INT_FAIL_MASK		(PMU_INT_SW_FAIL | \
> +					PMU_INT_HW_FAIL | \
> +					PMU_INT_PCH_FAIL)
> +#define PMU_INT_ALL_MASK		(PMU_INT_SEQ_DONE | \
> +					PMU_INT_HW_REQ | \
> +					PMU_INT_FAIL_MASK)
> +
> +#define DELAY_US			10
> +#define TIMEOUT_US			100000

Where do these delay/timeout numbers come from?  Is this just based one
experimentation, or is this something described by the HW docs.  Please
add some comments.

> +
> +struct starfive_power_dev {
> +	struct generic_pm_domain genpd;
> +	struct starfive_pmu *power;
> +	uint32_t mask;
> +};
> +
> +struct starfive_pmu {
> +	void __iomem *base;
> +	spinlock_t lock;
> +	int irq;
> +	struct device *pdev;
> +	struct starfive_power_dev *dev;
> +	struct genpd_onecell_data genpd_data;
> +	struct generic_pm_domain **genpd;
> +};
> +
> +struct starfive_pmu_data {
> +	const char * const name;
> +	uint8_t bit;
> +	unsigned int flags;
> +};
> +
> +static void __iomem *pmu_base;
> +
> +static inline void pmu_writel(u32 val, u32 offset)
> +{
> +	writel(val, pmu_base + offset);
> +}

You use writel() in this helper, but __raw_writel() throughout the rest
of the driver.  If that's intentional, you should explain a bit more
about why.  If not, please make it consistent.

If you're going to use a wrapper/helper, you should use it throughout.
But in this case, I'm not sure it's adding anything to readability.  In
fact, it's only adding confusion since you don't use it for most of the
register accesses.

> +void starfive_pmu_hw_event_turn_on(u32 mask)
> +{
> +	pmu_writel(mask, HW_EVENT_TURN_ON_MASK);
> +}
> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_on);
> +
> +void starfive_pmu_hw_event_turn_off(u32 mask)
> +{
> +	pmu_writel(mask, HW_EVENT_TURN_OFF_MASK);
> +}
> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_off);

This looks wrong.  Why are you allowing external users to power on/off the
domains?  The sate of the domain should be managed by the PM core (and
runtime PM use-counts etc.) allowing external users to change the domain
state is going to lead to the PM core being confused about the state of
the domain.

> +static int starfive_pmu_get_state(struct starfive_power_dev *pmd, bool *is_on)
> +{
> +	struct starfive_pmu *pmu = pmd->power;
> +
> +	if (!pmd->mask) {
> +		*is_on = false;
> +		return -EINVAL;
> +	}
> +
> +	*is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;
> +
> +	return 0;
> +}
> +
> +static int starfive_pmu_set_state(struct starfive_power_dev *pmd, bool on)
> +{
> +	struct starfive_pmu *pmu = pmd->power;
> +	unsigned long flags;
> +	uint32_t val;
> +	uint32_t mode;
> +	uint32_t encourage_lo;
> +	uint32_t encourage_hi;
> +	bool is_on;
> +	int ret;
> +
> +	if (!pmd->mask)
> +		return -EINVAL;
> +
> +	if (is_on == on) {

Comparing an uninitialzed stack variable to 'on'?

> +		dev_info(pmu->pdev, "pm domain [%s] is already %sable status.\n",
> +				pmd->genpd.name, on ? "en" : "dis");

Using dev_info() will probably make this a bit verbose.  I'd suggest
dev_dbg() for this kind of message.

> +		return 0;
> +	}
> +
> +	spin_lock_irqsave(&pmu->lock, flags);
> +
> +	if (on) {
> +		mode = SW_TURN_ON_POWER_MODE;
> +		encourage_lo = SW_MODE_ENCOURAGE_EN_LO;
> +		encourage_hi = SW_MODE_ENCOURAGE_EN_HI;
> +	} else {
> +		mode = SW_TURN_OFF_POWER_MODE;
> +		encourage_lo = SW_MODE_ENCOURAGE_DIS_LO;
> +		encourage_hi = SW_MODE_ENCOURAGE_DIS_HI;
> +	}
> +
> +	__raw_writel(pmd->mask, pmu->base + mode);
> +
> +	/* write SW_ENCOURAGE to make the configuration take effect */
> +	__raw_writel(SW_MODE_ENCOURAGE_ON, pmu->base + SW_ENCOURAGE);
> +	__raw_writel(encourage_lo, pmu->base + SW_ENCOURAGE);
> +	__raw_writel(encourage_hi, pmu->base + SW_ENCOURAGE);

This "encourage" feature is peculiar.  What happens if you do not do
this?  Does it take a lot longer for the domain to change state? or does
it not change at all?  In the absence of HW specs, a bit of description
here would be helpful.

> +	spin_unlock_irqrestore(&pmu->lock, flags);
> +
> +	if (on) {
> +		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
> +						val & pmd->mask, DELAY_US,
> +						TIMEOUT_US);
> +		if (ret) {
> +			dev_err(pmu->pdev, "%s: failed to power on\n", pmd->genpd.name);
> +			return -ETIMEDOUT;
> +		}
> +	} else {
> +		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
> +						!(val & pmd->mask), DELAY_US,
> +						TIMEOUT_US);
> +		if (ret) {
> +			dev_err(pmu->pdev, "%s: failed to power off\n", pmd->genpd.name);
> +			return -ETIMEDOUT;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int starfive_pmu_on(struct generic_pm_domain *genpd)
> +{
> +	struct starfive_power_dev *pmd = container_of(genpd,
> +		struct starfive_power_dev, genpd);
> +
> +	return starfive_pmu_set_state(pmd, true);
> +}
> +
> +static int starfive_pmu_off(struct generic_pm_domain *genpd)
> +{
> +	struct starfive_power_dev *pmd = container_of(genpd,
> +		struct starfive_power_dev, genpd);
> +
> +	return starfive_pmu_set_state(pmd, false);
> +}
> +
> +static void starfive_pmu_int_enable(struct starfive_pmu *pmu, u32 mask, bool enable)
> +{
> +	u32 val = __raw_readl(pmu->base + PMU_INT_MASK);
> +
> +	if (enable)
> +		val &= ~mask;
> +	else
> +		val |= mask;
> +
> +	__raw_writel(val, pmu->base + PMU_INT_MASK);
> +}
> +
> +static irqreturn_t starfive_pmu_interrupt(int irq, void *data)
> +{
> +	struct starfive_pmu *pmu = data;
> +	unsigned long flags;
> +	u32 val;
> +
> +	spin_lock_irqsave(&pmu->lock, flags);

I don't think you need to spinlock in the interrupt itself, as
interrupts will already be disabled, and this handler is not
IRQF_SHARED.

> +	val = __raw_readl(pmu->base + PMU_INT_STATUS);
> +
> +	if (val & PMU_INT_SEQ_DONE)
> +		dev_dbg(pmu->pdev, "sequence done.\n");
> +	if (val & PMU_INT_HW_REQ)
> +		dev_dbg(pmu->pdev, "hardware encourage requestion.\n");
> +	if (val & PMU_INT_SW_FAIL)
> +		dev_err(pmu->pdev, "software encourage fail.\n");
> +	if (val & PMU_INT_HW_FAIL)
> +		dev_err(pmu->pdev, "hardware encourage fail.\n");
> +	if (val & PMU_INT_PCH_FAIL)
> +		dev_err(pmu->pdev, "p-channel fail event.\n");
> +
> +	/* clear interrupts */
> +	__raw_writel(val, pmu->base + PMU_INT_STATUS);
> +	__raw_writel(val, pmu->base + PMU_EVENT_STATUS);
> +
> +	spin_unlock_irqrestore(&pmu->lock, flags);
> +
> +	return IRQ_HANDLED;
> +}
> +

[...]

Kevin

On 2022/11/25 19:17, Conor Dooley wrote:
> On Fri, Nov 25, 2022 at 06:04:59PM +0800, Walker Chen wrote:
>> On 2022/11/19 8:24, Conor Dooley wrote:
>> > On Fri, Nov 18, 2022 at 09:32:15PM +0800, Walker Chen wrote:
> 
>> >> +void starfive_pmu_hw_event_turn_off(u32 mask)
>> >> +{
>> >> +	pmu_writel(mask, HW_EVENT_TURN_OFF_MASK);
>> >> +}
>> >> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_off);
>> > 
>> > Where are the users for these exports? Also, should they be exported as
>> > GPL?
>> > 
>> > Either way, what is the point of the extra layer of abstraction here
>> > around the writel()?
>> 
>> The two export functions are only prepared for GPU module. But accordint to
>>  the latest information, it seems that there is no open source plan for GPU. 
>> So the two functions will be drop in next version of patch.
> 
> That's a shame!

Need to comply with certain commercial terms.

> 
>> >> +static int starfive_pmu_get_state(struct starfive_power_dev *pmd, bool *is_on)
>> >> +{
>> >> +	struct starfive_pmu *pmu = pmd->power;
>> >> +
>> >> +	if (!pmd->mask) {
>> >> +		*is_on = false;
>> >> +		return -EINVAL;
>> >> +	}
>> >> +
>> >> +	*is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;
>> > 
>> > Is there a specific reason that you are using the __raw variants here
>> > (and elsewhere) in the driver?
>> 
>> Will use unified function '__raw_readl' and '__raw_writel'
> 
> No no, I want to know *why* you are using the __raw accessors here. My
> understanding was that __raw variants are unbarriered & unordered with
> respect to other io accesses.
> 
> I do notice that the bcm driver you mentioned uses the __raw variants,
> but only __raw variants - whereas you use readl() which is ordered and
> barriered & __raw_readl().
> 
> Is there a reason why you would not use readl() or readl_relaxed()?

Your question led me to deeply understand the usage of these io accessors.
__raw_readl / __raw_writel denotes native byte order, no memory barrier.
readl / writel do guarantee the byte order with barrier, ensure that previous writes are done.
Seem that non-raw accessors are more safe.

> 
>> >> +
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +static int starfive_pmu_set_state(struct starfive_power_dev *pmd, bool on)
>> >> +{
>> >> +	struct starfive_pmu *pmu = pmd->power;
>> >> +	unsigned long flags;
>> >> +	uint32_t val;
>> >> +	uint32_t mode;
>> >> +	uint32_t encourage_lo;
>> >> +	uint32_t encourage_hi;
>> >> +	bool is_on;
>> >> +	int ret;
>> >> +
>> >> +	if (!pmd->mask)
>> >> +		return -EINVAL;
>> >> +
>> >> +	if (is_on == on) {
>> >> +		dev_info(pmu->pdev, "pm domain [%s] is already %sable status.\n",
>> >> +				pmd->genpd.name, on ? "en" : "dis");
>> > 
>> > Am I missing something here: you've just declared is_on, so it must be
>> > false & therefore this check is all a little pointless? The check just
>> > becomes if (false == on) which I don't think is what you're going for
>> > here? Or did I miss something obvious?
>> 
>> Sorry, indeed I missed several lines of code. It should be witten like this:
>> 	ret = jh71xx_pmu_get_state(pmd, &is_on);
>>         if (ret) {
>>                 dev_dbg(pmu->pdev, "unable to get current state for %s\n",
>>                         pmd->genpd.name);
>>                 return ret;
>>         }
> 
> Heh, this looks more sane :)
> 
>> 
>>         if (is_on == on) {
>>                 dev_dbg(pmu->pdev, "pm domain [%s] is already %sable status.\n",
>>                         pmd->genpd.name, on ? "en" : "dis");
>>                 return 0;
>>         }
>> 
>> > 
>> >> +		return 0;
>> >> +	}
>> >> +
>> >> +	spin_lock_irqsave(&pmu->lock, flags);
>> >> +
>> >> +	if (on) {
>> >> +		mode = SW_TURN_ON_POWER_MODE;
>> >> +		encourage_lo = SW_MODE_ENCOURAGE_EN_LO;
>> >> +		encourage_hi = SW_MODE_ENCOURAGE_EN_HI;
>> >> +	} else {
>> >> +		mode = SW_TURN_OFF_POWER_MODE;
>> >> +		encourage_lo = SW_MODE_ENCOURAGE_DIS_LO;
>> >> +		encourage_hi = SW_MODE_ENCOURAGE_DIS_HI;
>> >> +	}
>> >> +
>> >> +	__raw_writel(pmd->mask, pmu->base + mode);
>> >> +
>> >> +	/* write SW_ENCOURAGE to make the configuration take effect */
>> >> +	__raw_writel(SW_MODE_ENCOURAGE_ON, pmu->base + SW_ENCOURAGE);
>> > 
>> > Is register "sticky"? IOW, could you set it in probe and leave this mode
>> > always on? Or does it need to be set every time you want to use this
>> > feature?
>> 
>> These power domain registers need to be set by other module according to the
>> specific situation. 
>> For example some power domains should be turned off via System PM mechanism
>> when system goes to sleep, 
>> and then they are turned on when system resume.
> 
> I was just wondering if SW_MODE_ENCOURAGE_ON would retain it's value
> during operation or if it had to be written every time. It's fine if
> that's not the case.

Writing SW_MODE_ENCOURAGE_ON (0xFF) to SW_ENCOURAGE register, the purpose is to reset the 
state machine which is going to parse instruction sequence. It has to be written every time. 

> 
>> >> +	__raw_writel(encourage_lo, pmu->base + SW_ENCOURAGE);
>> >> +	__raw_writel(encourage_hi, pmu->base + SW_ENCOURAGE);
>> >> +
>> >> +	spin_unlock_irqrestore(&pmu->lock, flags);
>> >> +
>> >> +	if (on) {
>> >> +		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
>> >> +						val & pmd->mask, DELAY_US,
>> >> +						TIMEOUT_US);
>> >> +		if (ret) {
>> >> +			dev_err(pmu->pdev, "%s: failed to power on\n", pmd->genpd.name);
>> >> +			return -ETIMEDOUT;
>> >> +		}
>> >> +	} else {
>> >> +		ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
>> >> +						!(val & pmd->mask), DELAY_US,
>> >> +						TIMEOUT_US);
>> > 
>> > Could you not just decide the 3rd arg outside of the readl_poll..() and
>> > save on duplicating the wait logic here?
>> 
>> Seems that the readl_poll..() can only be called in two cases 
>> because the CURR_POWER_MODE register is read to val and then operation with mask every time.
>> 
> 
> I'm sorry, I completely forgot that read*_poll..() actually not actually
> a function. Please ignore this comment!
> 
>> >> +static int starfive_pmu_probe(struct platform_device *pdev)
>> >> +{
>> >> +	struct device *dev = &pdev->dev;
>> >> +	struct device_node *np = dev->of_node;
>> >> +	const struct starfive_pmu_data *entry, *table;
>> >> +	struct starfive_pmu *pmu;
>> >> +	unsigned int i;
>> >> +	uint8_t max_bit = 0;
>> >> +	int ret;
>> >> +
>> >> +	pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
>> >> +	if (!pmu)
>> >> +		return -ENOMEM;
>> >> +
>> >> +	pmu_base = pmu->base = devm_platform_ioremap_resource(pdev, 0);
>> >> +	if (IS_ERR(pmu->base))
>> >> +		return PTR_ERR(pmu->base);
>> >> +
>> >> +	/* initialize pmu interrupt  */
>> >> +	pmu->irq = platform_get_irq(pdev, 0);
>> >> +	if (pmu->irq < 0)
>> >> +		return pmu->irq;
>> >> +
>> >> +	ret = devm_request_irq(dev, pmu->irq, starfive_pmu_interrupt,
>> >> +			       0, pdev->name, pmu);
>> >> +	if (ret)
>> >> +		dev_err(dev, "request irq failed.\n");
>> >> +
>> >> +	table = of_device_get_match_data(dev);
>> >> +	if (!table)
>> >> +		return -EINVAL;
>> >> +
>> >> +	pmu->pdev = dev;
>> >> +	pmu->genpd_data.num_domains = 0;
>> >> +	i = 0;
>> >> +	for (entry = table; entry->name; entry++) {
>> >> +		max_bit = max(max_bit, entry->bit);
>> >> +		i++;
>> >> +	}
>> > 
>> > This looks like something that could be replaced by the functions in
>> > linux/list.h, no? Same below.
>> 
>> Nowadays other platforms on linux mainline mostly write in this way or write like this: 
>> 	for (i = 0; i < num; i++) {
>> 		...
>> 		pm_genpd_init(&pmd[i]->genpd, NULL, !is_on);
>> 	}
> 
> That's not what this specific bit of code is doing though, right? You're
> walking jh7110_power_domains to find the highest bit. I was looking at what
> some other drivers do, and took a look at drivers/soc/qcom/rpmhpd.c
> where they know the size of this struct at compile time & so can do
> store the number of power domains in the match data. If you did that,
> you could use a loop like the one other platforms use.
> 
>> >> +	if (!pmu->genpd)
>> >> +		return -ENOMEM;
>> >> +
>> >> +	pmu->genpd_data.domains = pmu->genpd;
>> >> +
>> >> +	i = 0;
>> >> +	for (entry = table; entry->name; entry++) {
> 
> And it's the same here. By now, you should know how many power domains
> you have, no?
> 
> Anyways, as I said before I don't know much about this power domain
> stuff, it's just that these two loops seem odd.

About the usage of loop, I took a look at other platforms like drivers/soc/qcom/rpmhpd.c,
maybe that way is simpler and easier to understand. I will try to change to that looping in next version.

> 
>> >> +		struct starfive_power_dev *pmd = &pmu->dev[i];
>> >> +		bool is_on;
>> >> +
>> >> +		pmd->power = pmu;
>> >> +		pmd->mask = BIT(entry->bit);
>> >> +		pmd->genpd.name = entry->name;
>> >> +		pmd->genpd.flags = entry->flags;
>> >> +
>> >> +		ret = starfive_pmu_get_state(pmd, &is_on);
>> >> +		if (ret)
>> >> +			dev_warn(dev, "unable to get current state for %s\n",
>> >> +				 pmd->genpd.name);
> 
>> >> +static const struct starfive_pmu_data jh7110_power_domains[] = {
>> > 
>> > Is this driver jh7110 only or actually jh71XX? Have you just started out
>> > by implementing one SoC both intend to support both?
>> 
>> JH7110 is our first SoC, probably the next generation of SoC jh7120 still
>> use this controller driver.
>> Maybe now the naming for JH71XX is not very suitable.
> 
> Right. My question was more about if this supported the JH7100 too, but
> I saw from your answer to Emil that it won't. I don't have a preference
> as to whether you call it jh71xx or jh7110, I'll leave that up to
> yourselves and Emil. This particular struct should still be called
> `jh7110_power_domains` though since it is particular to this SoC, no
> matter what you end up calling the file etc.

Emil has gave me a good suggestion.

> 
>> > I don't know /jack/ about power domain stuff, so I can barely review
>> > this at all sadly.
> 
>> Thank you for taking the time to review the code, you helped me a lot.
>> Thank you so much.
> 
> No worries, looking forward to getting my board :)
> 
Have you purchased a VisionFive 2 board online?  Do you need the shopping link ? 
https://forum.rvspace.org/t/how-to-purchase-visionfive-2/665

Best Regards,
Walker Chen

Hey Walker,

Apologies for my formatting here. 

On 1 December 2022 03:56:27 GMT, Walker Chen <walker.chen@starfivetech.com> wrote:
>On 2022/11/25 19:17, Conor Dooley wrote:
>> On Fri, Nov 25, 2022 at 06:04:59PM +0800, Walker Chen wrote:
>>> On 2022/11/19 8:24, Conor Dooley wrote:
>>> > On Fri, Nov 18, 2022 at 09:32:15PM +0800, Walker Chen wrote:
>> 
>>> >> +void starfive_pmu_hw_event_turn_off(u32 mask)
>>> >> +{
>>> >> +	pmu_writel(mask, HW_EVENT_TURN_OFF_MASK);
>>> >> +}
>>> >> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_off);
>>> > 
>>> > Where are the users for these exports? Also, should they be exported as
>>> > GPL?
>>> > 
>>> > Either way, what is the point of the extra layer of abstraction here
>>> > around the writel()?
>>> 
>>> The two export functions are only prepared for GPU module. But accordint to
>>>  the latest information, it seems that there is no open source plan for GPU. 
>>> So the two functions will be drop in next version of patch.
>> 
>> That's a shame!
>
>Need to comply with certain commercial terms.
>
>> 
>>> >> +static int starfive_pmu_get_state(struct starfive_power_dev *pmd, bool *is_on)
>>> >> +{
>>> >> +	struct starfive_pmu *pmu = pmd->power;
>>> >> +
>>> >> +	if (!pmd->mask) {
>>> >> +		*is_on = false;
>>> >> +		return -EINVAL;
>>> >> +	}
>>> >> +
>>> >> +	*is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;
>>> > 
>>> > Is there a specific reason that you are using the __raw variants here
>>> > (and elsewhere) in the driver?
>>> 
>>> Will use unified function '__raw_readl' and '__raw_writel'
>> 
>> No no, I want to know *why* you are using the __raw accessors here. My
>> understanding was that __raw variants are unbarriered & unordered with
>> respect to other io accesses.
>> 
>> I do notice that the bcm driver you mentioned uses the __raw variants,
>> but only __raw variants - whereas you use readl() which is ordered and
>> barriered & __raw_readl().
>> 
>> Is there a reason why you would not use readl() or readl_relaxed()?
>
>Your question led me to deeply understand the usage of these io accessors.
>__raw_readl / __raw_writel denotes native byte order, no memory barrier.
>readl / writel do guarantee the byte order with barrier, ensure that previous writes are done.
>Seem that non-raw accessors are more safe.

Yeah, if there's no good reason to use these "raw"  versions then please use readl/readl_relaxed.


>> No worries, looking forward to getting my board :)
>> 
>Have you purchased a VisionFive 2 board online?  

I have :)