| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230411-iron-everybody-70b78e94aee5@spud>
Date: Tue, 11 Apr 2023 22:15:46 +0100
From: Conor Dooley <conor@...nel.org>
To: Changhuang Liang <changhuang.liang@...rfivetech.com>
Cc: Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Emil Renner Berthing <kernel@...il.dk>,
Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>,
Walker Chen <walker.chen@...rfivetech.com>,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v1 6/7] soc: starfive: Add dphy pmu support
On Mon, Apr 10, 2023 at 11:47:42PM -0700, Changhuang Liang wrote:
> Add dphy pmu to turn on/off the dphy power switch.
>
> Signed-off-by: Changhuang Liang <changhuang.liang@...rfivetech.com>
> ---
> MAINTAINERS | 1 +
> drivers/soc/starfive/jh71xx_pmu.c | 65 +++++++++++++++++++++++++++++++
> 2 files changed, 66 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0b2170e1e4ff..4d958f02403e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19944,6 +19944,7 @@ F: include/dt-bindings/reset/starfive?jh71*.h
>
> STARFIVE JH71XX PMU CONTROLLER DRIVER
> M: Walker Chen <walker.chen@...rfivetech.com>
> +M: Changhuang Liang <changhuang.liang@...rfivetech.com>
Unmentioned in the commit message, plus I would like an R-b or an Ack
from Walker.
> S: Supported
> F: Documentation/devicetree/bindings/power/starfive*
> F: drivers/soc/starfive/jh71xx_pmu.c
> diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c
> index 990db6735c48..d4092ca4dccf 100644
> --- a/drivers/soc/starfive/jh71xx_pmu.c
> +++ b/drivers/soc/starfive/jh71xx_pmu.c
> @@ -24,6 +24,9 @@
> #define JH71XX_PMU_EVENT_STATUS 0x88
> #define JH71XX_PMU_INT_STATUS 0x8C
>
> +/* DPHY pmu register offset */
> +#define JH71XX_PMU_DPHY_SWITCH 0x00
> +
> /* sw encourage cfg */
> #define JH71XX_PMU_SW_ENCOURAGE_EN_LO 0x05
> #define JH71XX_PMU_SW_ENCOURAGE_EN_HI 0x50
> @@ -94,6 +97,8 @@ static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_o
>
> if (pmu->match_data->pmu_type == JH71XX_PMU_GENERAL)
> offset = JH71XX_PMU_CURR_POWER_MODE;
> + else if (pmu->match_data->pmu_type == JH71XX_PMU_DPHY)
There are only two options for this "enum", so why `else if`?
> + offset = JH71XX_PMU_DPHY_SWITCH;
>
> regmap_read(pmu->base, offset, &val);
>
> @@ -170,6 +175,23 @@ static int jh71xx_pmu_general_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bo
> return 0;
> }
>
> +static int jh71xx_pmu_dphy_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
> +{
> + struct jh71xx_pmu *pmu = pmd->pmu;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&pmu->lock, flags);
> +
> + if (on)
> + regmap_update_bits(pmu->base, JH71XX_PMU_DPHY_SWITCH, mask, mask);
> + else
> + regmap_update_bits(pmu->base, JH71XX_PMU_DPHY_SWITCH, mask, 0);
> +
> + spin_unlock_irqrestore(&pmu->lock, flags);
> +
> + return 0;
> +}
> +
> static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
> {
> struct jh71xx_pmu *pmu = pmd->pmu;
> @@ -191,6 +213,8 @@ static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
>
> if (pmu->match_data->pmu_type == JH71XX_PMU_GENERAL)
> ret = jh71xx_pmu_general_set_state(pmd, mask, on);
> + else if (pmu->match_data->pmu_type == JH71XX_PMU_DPHY)
> + ret = jh71xx_pmu_dphy_set_state(pmd, mask, on);
Perhaps I am verging on over-complication, but I dislike this carry on.
Is this the only time we'll see a power domain provider coming out of
a syscon, or are there likely to be more?
Either way, I think having an ops struct w/ both parse_dt() and the
set_state() implementations would be neater than what you have here.
Very much open to dissenting opinions there though. Emil? Walker?
Cheers,
Conor.
>
> return ret;
> }
> @@ -280,6 +304,25 @@ static int jh7110_pmu_general_parse_dt(struct platform_device *pdev,
> return 0;
> }
>
> +static int jh7110_pmu_dphy_parse_dt(struct platform_device *pdev,
> + struct jh71xx_pmu *pmu)
> +{
> + struct device *parent;
> + struct device *dev = &pdev->dev;
> +
> + parent = pdev->dev.parent;
> + if (!parent) {
> + dev_err(dev, "No parent for syscon pmu\n");
> + return -ENODEV;
> + }
> +
> + pmu->base = syscon_node_to_regmap(parent->of_node);
> + if (IS_ERR(pmu->base))
> + return PTR_ERR(pmu->base);
> +
> + return 0;
> +}
> +
> static int jh71xx_pmu_init_domain(struct jh71xx_pmu *pmu, int index)
> {
> struct jh71xx_pmu_dev *pmd;
> @@ -409,10 +452,31 @@ static const struct jh71xx_pmu_match_data jh7110_pmu = {
> .pmu_parse_dt = jh7110_pmu_general_parse_dt,
> };
>
> +static const struct jh71xx_domain_info jh7110_dphy_power_domains[] = {
> + [JH7110_PD_DPHY_TX] = {
> + .name = "DPHY-TX",
> + .bit = 30,
> + },
> + [JH7110_PD_DPHY_RX] = {
> + .name = "DPHY-RX",
> + .bit = 31,
> + },
> +};
> +
> +static const struct jh71xx_pmu_match_data jh7110_pmu_dphy = {
> + .num_domains = ARRAY_SIZE(jh7110_dphy_power_domains),
> + .domain_info = jh7110_dphy_power_domains,
> + .pmu_type = JH71XX_PMU_DPHY,
> + .pmu_parse_dt = jh7110_pmu_dphy_parse_dt,
> +};
> +
> static const struct of_device_id jh71xx_pmu_of_match[] = {
> {
> .compatible = "starfive,jh7110-pmu",
> .data = (void *)&jh7110_pmu,
> + }, {
> + .compatible = "starfive,jh7110-pmu-dphy",
> + .data = (void *)&jh7110_pmu_dphy,
> }, {
> /* sentinel */
> }
> @@ -429,5 +493,6 @@ static struct platform_driver jh71xx_pmu_driver = {
> builtin_platform_driver(jh71xx_pmu_driver);
>
> MODULE_AUTHOR("Walker Chen <walker.chen@...rfivetech.com>");
> +MODULE_AUTHOR("Changhuang Liang <changhuang.liang@...rfivetech.com>");
> MODULE_DESCRIPTION("StarFive JH71XX PMU Driver");
> MODULE_LICENSE("GPL");
> --
> 2.25.1
>
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists