Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

Conor Dooley
2023-04-24T16:52:33Z

[-- Attachment #1.1: Type: text/plain, Size: 5616 bytes --]

Hey Changhuang,

On Thu, Apr 20, 2023 at 03:00:10PM +0800, Changhuang Liang wrote:
> On 2023/4/20 2:29, Conor Dooley wrote:
> > On Tue, Apr 18, 2023 at 08:56:41PM -0700, Changhuang Liang wrote:
> >> Add AON PMU for StarFive JH7110 SoC, it can be used to turn on/off DPHY
> >> rx/tx power switch, and it don't need the properties of reg and
> >> interrupts.
> >
> > Putting this here since the DT guys are more likely to see it this way..
> > Given how the implementation of the code driving this new
> > power-controller and the code driving the existing one are rather
> > different (you've basically re-written the entire driver in this series),
> > should the dphy driver implement its own power-controller?
> >
> > I know originally Changuang had tried something along those lines:
> > https://lore.kernel.org/linux-riscv/5dc4ddc2-9d15-ebb2-38bc-8a544ca67e0d@starfivetech.com/
> >
> > I see that that was shut down pretty much, partly due to the
> > non-standard property, hence this series adding the dphy power domain to
> > the existing driver.
> >
> > If it was done by looking up the pmu with a
> > of_find_compatible_node(NULL, "power-controller", "starfive,jh7110-aon-pmu")
> > type thing, would that make sense? Although, maybe that is not a
> > question for you, and this series may actually have been better entirely
> > bundled with the dphy series so the whole thing can be reviewed as a
> > unit. I've added
> >
> > IOW, don't change this patch, or the dts patch, but move all of the
> > code back into the phy driver..
> >
>
> Maybe this way can not do that? power domain is binding before driver probe,
> if I use "of_find_compatible_node" it phy(DPHY rx) probe. Maybe I can only operate
> this power switch in my phy(DPHY rx) driver, so the all patch of this series isn't
> make sense.

I'm a wee bit lost here, as I unfortunately know little about how Linux
handles this power-domain stuff. If the DPHY tries to probe and some
pre-requisite does not yet exist, you can return -EPROBE_DEFER right?

But I don't think that's what you are asking, as using
of_find_compatible_node() doesn't depend on there being a driver AFAIU.

> In my opinion, We will also submit DPHY TX module later which use this series.
> Maybe this series should independent?

Is the DPHY tx module a different driver to the rx one?
If yes, does it have a different bit you must set in the syscon?

+CC Walker, do you have a register map for the jh7110? My TRM only says
what the registers are, but not the bits in them. Would make life easier
if I had that info.

I'm fine with taking this code, I just want to make sure that the soc
driver doing this is the right thing to do.
I was kinda hoping that combining with the DPHY-rx series might allow
the PHY folk to spot if you are doing something here with the power
domains that doesn't make sense.

> > Sorry for not asking this sooner Changhuang,
> > Conor.
> >
> > (hopefully this didn't get sent twice, mutt complained of a bad email
> > addr during sending the first time)
> >
>
> I'm sorry for that, I will notice later.

No, this was my mail client doing things that I was unsure of. You
didn't do anything wrong.

> >> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> >> ---
> >> .../bindings/power/starfive,jh7110-pmu.yaml | 15 +++++++++++++--
> >> include/dt-bindings/power/starfive,jh7110-pmu.h | 3 +++
> >> 2 files changed, 16 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/power/starfive,jh7110-pmu.yaml b/Documentation/devicetree/bindings/power/starfive,jh7110-pmu.yaml
> >> index 98eb8b4110e7..c50507c38e14 100644
> >> --- a/Documentation/devicetree/bindings/power/starfive,jh7110-pmu.yaml
> >> +++ b/Documentation/devicetree/bindings/power/starfive,jh7110-pmu.yaml
> >> @@ -8,6 +8,7 @@ title: StarFive JH7110 Power Management Unit
> >>
> >> maintainers:
> >> - Walker Chen <walker.chen@starfivetech.com>
> >> + - Changhuang Liang <changhuang.liang@starfivetech.com>
> >>
> >> description: |
> >> StarFive JH7110 SoC includes support for multiple power domains which can be
> >> @@ -17,6 +18,7 @@ properties:
> >> compatible:
> >> enum:
> >> - starfive,jh7110-pmu
> >> + - starfive,jh7110-aon-pmu

I was speaking to Rob about this over the weekend, he asked:
'Why isn't "starfive,jh7110-aon-syscon" just the power-domain provider
itself?'
Do we actually need to add a new binding for this at all?

Cheers,
Conor.

> >>
> >> reg:
> >> maxItems: 1
> >> @@ -29,10 +31,19 @@ properties:
> >>
> >> required:
> >> - compatible
> >> - - reg
> >> - - interrupts
> >> - "#power-domain-cells"
> >>
> >> +allOf:
> >> + - if:
> >> + properties:
> >> + compatible:
> >> + contains:
> >> + const: starfive,jh7110-pmu
> >> + then:
> >> + required:
> >> + - reg
> >> + - interrupts
> >> +
> >> additionalProperties: false
> >>
> >> examples:
> >> diff --git a/include/dt-bindings/power/starfive,jh7110-pmu.h b/include/dt-bindings/power/starfive,jh7110-pmu.h
> >> index 132bfe401fc8..0bfd6700c144 100644
> >> --- a/include/dt-bindings/power/starfive,jh7110-pmu.h
> >> +++ b/include/dt-bindings/power/starfive,jh7110-pmu.h
> >> @@ -14,4 +14,7 @@
> >> #define JH7110_PD_ISP 5
> >> #define JH7110_PD_VENC 6
> >>
> >> +#define JH7110_PD_DPHY_TX 0
> >> +#define JH7110_PD_DPHY_RX 1
> >> +
> >> #endif
> >> --
> >> 2.25.1
> >>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]