Message-ID: <20230424-baffle-punch-ec73098f2b6a@spud>
Date:   Mon, 24 Apr 2023 17:52:23 +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>,
        Hal Feng <hal.feng@...rfivetech.com>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-riscv@...ts.infradead.org, vkoul@...nel.org,
        linux-phy@...ts.infradead.org
Subject: Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

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@...rfivetech.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@...rfivetech.com>
> >> +  - Changhuang Liang <changhuang.liang@...rfivetech.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
> >>

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists