Re: [PATCH v1 6/7] soc: starfive: Add dphy pmu support

[Conor Dooley]

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@xxxxxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxx>
> +M:	Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxxxx>");
> +MODULE_AUTHOR("Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>");
>  MODULE_DESCRIPTION("StarFive JH71XX PMU Driver");
>  MODULE_LICENSE("GPL");
> -- 
> 2.25.1
>

Attachment: signature.asc
Description: PGP signature