Message-ID: <d685a1d4-c07d-7dfa-f1fb-b35ceb2aa0eb@starfivetech.com>
Date:   Tue, 25 Apr 2023 11:41:38 +0800
From:   Changhuang Liang <changhuang.liang@...rfivetech.com>
To:     Conor Dooley <conor@...nel.org>
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



On 2023/4/25 0:52, Conor Dooley wrote:
> 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?
> 

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

>>> 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@...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?'

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@starfivetech.com/

>>>>  
>>>>    reg:
>>>>      maxItems: 1
>>>> @@ -29,10 +31,19 @@ properties:
>>>>  

Powered by blists - more mailing lists