On Tue, Apr 25, 2023 at 11:41:38AM +0800, Changhuang Liang wrote: > On 2023/4/25 0:52, Conor Dooley wrote: > > 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? > > > > Yes, DPHY tx module is a different driver to the DPHY rx. And I have do a > different bit in PATCH 1: > > #define JH7110_PD_DPHY_TX 0 > #define JH7110_PD_DPHY_RX 1 > > also in PATCH 5: > > static const struct jh71xx_domain_info jh7110_aon_power_domains[] = { > [JH7110_PD_DPHY_TX] = { > .name = "DPHY-TX", > .bit = 30, > }, > [JH7110_PD_DPHY_RX] = { > .name = "DPHY-RX", > .bit = 31, > }, > }; > > > +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. > > > > I asked about our soc colleagues. This syscon register, > offset 0x00: > bit[31] ---> dphy rx power switch > bit[30] ---> dphy tx power switch > other bits ---> Reserved Okay. Unless someone explicitly disagrees, I'm fine with doing this stand-alone from the DPHY drivers. > >>> 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. > > > [...] > >>>> - 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?' > > Maybe not, this syscon only offset "0x00" configure power switch. > other offset configure other functions, maybe not power, so this > "starfive,jh7110-aon-syscon" not the power-domain itself. > > > Do we actually need to add a new binding for this at all? > > > > Cheers, > > Conor. > > > > Maybe this patch do that. > https://lore.kernel.org/all/20230414024157.53203-6-xingyu.wu@xxxxxxxxxxxxxxxx/ This makes it a child-node right? I think Rob already said no to that in and earlier revision of this series. What he meant the other day was making the syscon itself a power domain controller, since the child node has no meaningful properties (reg, interrupts etc). Cheers, Conor.
Attachment:
signature.asc
Description: PGP signature
