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@xxxxxxxxxxxxxxxx/ > > > > 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@xxxxxxxxxxxxxxxx> > >> --- > >> .../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@xxxxxxxxxxxxxxxx> > >> + - Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx> > >> > >> 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:
signature.asc
Description: PGP signature
