[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>
> ---
>  MAINTAINERS                       |   8 +

> 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*
>  
> +STARFIVE JH71XX PMU CONTROLLER DRIVER
> +M:	Walker Chen <walker.chen@starfivetech.com>
> +S:	Maintained

Should this be supported? (ditto the other series that you guys have
sent out in the last few days)

> +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

I noticed that you have not CCed Arnd on these patches, which makes me
wonder how do you intend getting these patches applied?
Until now, Palmer has mostly merged RISC-V drivers/soc patches, but in
the last few days I've taken over for drivers/soc/{sifive,microchip}.

Are you going to send PRs for this stuff to Arnd, or would you like me
to apply patches for drivers/soc/startech? I happy to do that for you if
you like.

If you're going to send pull requests, I am not sure if Arnd requires
GPG signed tags for them. Arnd?

Otherwise, if you want me to take them, please add something like the
following, in addition to the entry your series already adds for the
specific driver:
STARFIVE SOC DRIVERS
M:	Conor Dooley <conor@kernel.org>
S:	Maintained
T:	git https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/
F:	drivers/soc/starfive/
F:	include/soc/starfive/

Thanks,
Conor.
  

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);
  

On 18/11/2022 14:32, Walker Chen wrote:
> Add generic power domain driver for the StarFive JH71XX SoC.
> 
> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>

Thank you for your patch. There is something to discuss/improve.

> +
> +MODULE_AUTHOR("Walker Chen <walker.chen@starfivetech.com>");
> +MODULE_DESCRIPTION("StarFive JH71XX Power Domain 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);

These are not used outside of driver. Drop entire header file.


Best regards,
Krzysztof
  

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/19 2:31, Emil Renner Berthing wrote:
> 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.

This warning will be fixed in the next versioin of patch. 

> 
>> +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.

I think it should be described in this detail:
Say 'y' here to enables support for Power Management Unit that 
is used for enabling and disabling SoC devices.

> 
>> +
>> 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.

Good advice! Will be changed in the next version of patch.

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

Yes, checkpatch.pl complains about this. Will be fixed.

> 
>> +};
>> +
>> +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

Yes, checkpatch.pl complains about this. Will be fixed.

> 
>> +       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.

No obvious problems, this usage is referenced from /drivers/soc/bcm/bcm63xx/bcm63xx-power.c
Maybe drop this function, read the status of power domain to is_on directly before calling pm_genpd_init().

> 
>> +
>> +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

Yes, checkpatch.pl complains about this. Will be fixed.

> 
>> +       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")

I think use only one readl_poll_timeout_atomic() to distinguish between different situation 
according to the 3rd argument outside of the readl_poll..().

> 
>> +       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

Yes, checkpatch.pl complains about this. Will be fixed.

> 
>> +
>> +       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.

Well, your advice is very helpful.

> 
>> +
>> +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.

Yes, jh71xx_pmu_ is preferable, will use the unified name in the next version.

> 
>> +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.

These two functions will be used by the GPU module. Only the power-off of the GPU is not controlled 
by the software through PMU module. So here the functions are needed to export.

Thank you for taking the time to review this driver!

Best Regards,
Walker Chen
  

Hey Walker,
Just jumping in here...

On Thu, Nov 24, 2022 at 05:08:57PM +0800, Walker Chen wrote:
> On 2022/11/19 2:31, Emil Renner Berthing wrote:
> > On Fri, 18 Nov 2022 at 14:35, Walker Chen <walker.chen@starfivetech.com> wrote:

> >> 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.
> 
> These two functions will be used by the GPU module. Only the power-off
> of the GPU is not controlled by the software through PMU module. So
> here the functions are needed to export.

...the general policy is to avoid adding things without users. I think
they should be kept as static functions for now & when your GPU driver
is being upstreamed you can expose these functions. That way your usage
of them can be reviewed with the appropriate context.

Thanks,
Conor.
  

On 2022/11/24 17:28, Conor Dooley wrote:
> Hey Walker,
> Just jumping in here...
> 
> On Thu, Nov 24, 2022 at 05:08:57PM +0800, Walker Chen wrote:
>> On 2022/11/19 2:31, Emil Renner Berthing wrote:
>> > On Fri, 18 Nov 2022 at 14:35, Walker Chen <walker.chen@starfivetech.com> wrote:
> 
>> >> 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.
>> 
>> These two functions will be used by the GPU module. Only the power-off
>> of the GPU is not controlled by the software through PMU module. So
>> here the functions are needed to export.
> 
> ...the general policy is to avoid adding things without users. I think
> they should be kept as static functions for now & when your GPU driver
> is being upstreamed you can expose these functions. That way your usage
> of them can be reviewed with the appropriate context.
> 

OK, thank you for your advice. I will modify the code according to your suggestion
 in the next version of patch.

Best Regards,
Walker Chen
  

On 2022/11/19 3:36, Conor Dooley wrote:
> 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>
>> ---
>>  MAINTAINERS                       |   8 +
> 
>> 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*
>>  
>> +STARFIVE JH71XX PMU CONTROLLER DRIVER
>> +M:	Walker Chen <walker.chen@starfivetech.com>
>> +S:	Maintained
> 
> Should this be supported? (ditto the other series that you guys have
> sent out in the last few days)

Ok, will be changed to Supported.

> 
>> +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
> 
> I noticed that you have not CCed Arnd on these patches, which makes me
> wonder how do you intend getting these patches applied?
> Until now, Palmer has mostly merged RISC-V drivers/soc patches, but in
> the last few days I've taken over for drivers/soc/{sifive,microchip}.
> 
> Are you going to send PRs for this stuff to Arnd, or would you like me
> to apply patches for drivers/soc/startech? I happy to do that for you if
> you like.
> 
> If you're going to send pull requests, I am not sure if Arnd requires
> GPG signed tags for them. Arnd?
> 
> Otherwise, if you want me to take them, please add something like the
> following, in addition to the entry your series already adds for the
> specific driver:
> STARFIVE SOC DRIVERS
> M:	Conor Dooley <conor@kernel.org>
> S:	Maintained
> T:	git https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/
> F:	drivers/soc/starfive/
> F:	include/soc/starfive/

Thank you for taking the time to review and comment. I hope you apply these patches 
for drivers/soc/starfive. If you do that, I am very grateful to you!
The above description will be added to MAINTAINERS in the next version of patch.

Best Regards,
Walker Chen
  

On 2022/11/19 8:24, Conor Dooley wrote:
> 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?

OK, 'default SOC_STARFIVE' will be added to the next line of 'depends on PM && (SOC_STARFIVE || COMPILE_TEST)'

> 
>> +	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.

I will modify the name of macro according to your suggestions.

> 
>> +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.

These structs are referenced from drivers/soc/bcm/bcm63xx/bcm63xx-power.c

> 
>> +};
>> +
>> +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()?

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.

> 
>> +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'

> 
>> +
>> +	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;
        }

        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.

> 
>> +	__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.

> 
>> +		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.

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);
	}

> 
>> +
>> +	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.

Yes, I will adjust these alignment. Thank you for your remind.

> 
>> +	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?

I will modify the code like this:
static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, bool on)
{
	int ret;
	...
	
	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;
	}

	if (is_on == on) {
		dev_info(pmu->pdev, "pm domain [%s] is already %sable status.\n",
			 pmd->genpd.name, on ? "en" : "dis");
		return 0;
	}
	...
}

This place where jh71xx_pmu_get_state() is called will return and then can not set the state 
for power domain if pmd->mask is missed to assign a value.

> 
>> +
>> +		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?

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.

> 
>> +	{
>> +		.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.

Thank you for taking the time to review the code, you helped me a lot. Thank you so much.

Best Regards,
Walker Chen
  

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!

> >> +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()?

> >> +
> >> +	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.

> >> +	__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.

> >> +		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.

> > 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 :)
  

On 2022/11/21 18:14, Krzysztof Kozlowski wrote:
> On 18/11/2022 14:32, Walker Chen wrote:
>> Add generic power domain driver for the StarFive JH71XX SoC.
>> 
>> Signed-off-by: Walker Chen <walker.chen@starfivetech.com>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
>> +
>> +MODULE_AUTHOR("Walker Chen <walker.chen@starfivetech.com>");
>> +MODULE_DESCRIPTION("StarFive JH71XX Power Domain 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);
> 
> These are not used outside of driver. Drop entire header file.
> 

Yes, these two exported functions will be drop in the next version of patches.

Best Regards,
Walker Chen
  

On 2022/11/22 8:09, Kevin Hilman wrote:
> 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.
> 
> [...]
> 

OK, more detailed description will be added to commit message in the next version of patch.

>> 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.
>

Now the delay time is not accurate enough though the things can work normally.
It will be changed to more accurate value according to hardware feature.
 
>> +
>> +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.

Your suggestion will be taken seriously consider. Thanks.

> 
>> +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.
> 

These two functions are used by the GPU module. Only the power-off of the GPU is not controlled 
by the software through PMU module. So here the functions are needed to export. But according 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.

>> +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'?
> 

Sorry, indeed I missed several lines of code. It should be written 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;
        }

        if (is_on == on) {
                dev_dbg(pmu->pdev, "pm domain [%s] is already %sable status.\n",
                        pmd->genpd.name, on ? "en" : "dis");
                return 0;
        }

>> +		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.
> 

Will fix.

>> +		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.

There is need specific command sequence for controlling one power domain on JH7110 SoC. 
SW turn-on command sequence: Write the register Software encourage (offset 0x44) 0xff –> 0x05 –> 0x50
SW turn-off command sequence: Write the register Software encourage (offset 0x44) 0xff –> 0x0a –> 0xa0
More detailed comment will be added in next version.

> 
>> +	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.

Helpful suggestion, it will be fixed. thanks.

> 
>> +	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 :)