Message-ID: <9ada04bb73e980f981d3b933552f0eda47d4fd26.camel@mediatek.com>
Date: Wed, 28 May 2025 04:01:55 +0000
From: Nancy Lin (林欣螢) <Nancy.Lin@...iatek.com>
To: "lgirdwood@...il.com" <lgirdwood@...il.com>, "robh@...nel.org"
	<robh@...nel.org>, "chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "matthias.bgg@...il.com"
	<matthias.bgg@...il.com>, "krzk@...nel.org" <krzk@...nel.org>
CC: Singo Chang (張興國) <Singo.Chang@...iatek.com>,
	"broonie@...nel.org" <broonie@...nel.org>,
	Project_Global_Chrome_Upstream_Group
	<Project_Global_Chrome_Upstream_Group@...iatek.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Jason-JH Lin (林睿祥) <Jason-JH.Lin@...iatek.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>,
	Paul-pl Chen (陳柏霖) <Paul-pl.Chen@...iatek.com>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>
Subject: Re: [PATCH 1/2] dt-bindings: regulator: mediatek: Add MT8196 vmm
 controller

Hi Krzysztof,

Thanks for your review.

On Thu, 2025-05-22 at 17:09 +0200, Krzysztof Kozlowski wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 22/05/2025 17:03, Nancy.Lin wrote:
> > From: Nancy Lin <nancy.lin@...iatek.com>
> > 
> > Add a device tree binding document for the MediaTek MT8196 VMM
> > (Vcore
> > for MultiMedia) regulator controller. The VMM controller acts as
> > the
> > main power supplier for multimedia power domains, such as those
> > used
> > by display, video encode and decode subsystems. It provides virtual
> > regulators that serve as the power sources for various multimedia
> > IPs,
> 
> Virtual regulators do not sound real, so feels like you want some
> sort
> of power domains?
> 
This regulator supplies power to the power domain. Before the power
domain can be powered up, the MediaTek PM driver first enables the
regulator that the power domain relies on, and then powers up the
domain. While "virtual" might not be the best term, it accurately
describes a regulator used to control the power switch.

Ultimately, the VCP (uP) handles the on/off control of the regulator.
Therefore, we encapsulate it as a standard regulator in the kernel.

The internal control of the VMM regulator is as follows:
   kernel                 HWCCF                          VCP (uP)
|--------------|      |--------------|   irq    |---------------------|
| VMM reglator | ---> | hardware     |  ----->  | get buck on/off irq |
|              |      | voter        |          | and then turn on/off|
|              |      |              |          | buck                |
|--------------|      |--------------|          |---------------------|

When the regulator needs to be turned on or off, it uses the voter
provided by hwccf to cast a vote for on/off. The VCP then receives the
corresponding IRQ for the regulator and performs the buck on/off
accordingly.

> A nit, subject: drop second/last, redundant "bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst*L18__;Iw!!CTRNKA9wMg0ARbw!lBqtzZ8DmC6f2kNnLXRCpHsof7XOYvrIMG4o_DXuvjYbg1SyFRrXrtEMLjIxS31cq82hqQowrczWRg$
> 

OK, I will remove the binding/document word in commit message body.
And change title to
   regulator: dt-bindings: Add MT8196 vmm controller


> > and coordinates with the hardware common clock framework (hwccf)
> > and
> > the Video Companion Processor (VCP) to manage the power domains of
> > these components. The regulator is controlled by the VCP firmware,
> > and the operating system signals its requirement through a voting
> > hardware block (hwccf).
> > 
> > Signed-off-by: Nancy Lin <nancy.lin@...iatek.com>
> > ---
> >  .../mediatek,mt8196-vmm-regulator.yaml        | 70
> > +++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/regulator/mediatek,mt8196-vmm-
> > regulator.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/regulator/mediatek,mt8196-vmm-
> > regulator.yaml
> > b/Documentation/devicetree/bindings/regulator/mediatek,mt8196-vmm-
> > regulator.yaml
> > new file mode 100644
> > index 000000000000..a50e35c2e238
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/regulator/mediatek,mt8196-
> > vmm-regulator.yaml
> > @@ -0,0 +1,70 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> > "https://urldefense.com/v3/__http://devicetree.org/schemas/regulato
> > r/mediatek,mt8196-vmm-
> > regulator.yaml*__;Iw!!CTRNKA9wMg0ARbw!lBqtzZ8DmC6f2kNnLXRCpHsof7XOY
> > vrIMG4o_DXuvjYbg1SyFRrXrtEMLjIxS31cq82hqQrOgUATjA$ "
> > +$schema:
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!lBqtzZ8DmC6f2kNnLXRCpHsof7XOYvrIMG4o_DXuvjYbg1SyFRrXrtEMLjIxS31cq82hqQr4UitgLg$
> > +
> > +title: MediaTek MT8196 VMM (Vcore for MultiMedia) Regulator
> > Controller
> > +
> > +maintainers:
> > +  - Nancy Lin <nancy.lin@...iatek.com>
> > +
> > +description: |
> > +  The MediaTek MT8196 VMM (Vcore for Multi Media) controller acts
> > as the
> > +  main power supplier for multimedia power domains, such as those
> > used by
> > +  display, video encode and decode subsystems. The VMM hardware
> > block
> > +  provides virtual regulators that serve as the power sources
> > (suppliers)
> > +  for various multimedia IPs. It coordinates with the MediaTek
> > hardware
> > +  common clock framework (HWCCF) and the Video Companion Processor
> > (VCP)
> > +  to manage the power domains of these multimedia components.
> > +
> > +  Each child node under the VMM node represents a virtual
> > regulator
> > +  (e.g., vdisp, vdec-vcore) and must specify a 'regulator-name'.
> > +
> > +properties:
> > +  compatible:
> > +    const: "mediatek,mt8196-vmm"
> 
> Not tested...
> 
My bad, I accidentally added extra quotation marks, but I ran
dt_binding_check and didn't see this error being                      
detected.

> > +
> > +  mediatek,hw-ccf:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: Phandle to the hardware common clock framework
> > syscon controller.
> 
> No, you cannot express clocks with syscon.
> 
HWCCF is MediaTek's internal naming; the functionality of the hardware
IP is voter. We use the IP to vote for buck on/off, and the VCP will
get the voting result to perform buck on/off. I will refine the
naming/description to "voter."

> > +
> > +  mediatek,vcp:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: Phandle to the Video Co-Processor (VCP) node.
> 
> For what purpose?
> 
This is used to ensure that the VCP uP is ready to receive the hwccf
IRQ and handle the corresponding buck on/off operations.

> > +
> > +patternProperties:
> > +  "^(vdisp|vdec-vcore)$":
> 
> Redundant nodes, useless. Drop these completely.
> 
OK.
> 
> > +    type: object
> > +    description: |
> > +      Virtual regulator for a specific multimedia domain.
> > +      The node name should match the supported regulator (e.g.,
> > vdisp, vdec-vcore).
> > +    properties:
> > +      regulator-name:
> 
> No, you cannot start redefining properties. This binding is nowhere
> close to hardware description. Looks like some copy-paste downstream
> driver, so binding to fulfill driver needs.
> 
> Please rework to match hardware. I suggest reaching internally to get
> some help how upstream drivers and bindings look like *prior* sending
> downstream code.
> 
I will review and refine the binding internally before submitting it.
Thanks!

Best regards,
Nancy

> Best regards,
> Krzysztof

Powered by blists - more mailing lists