Message-ID: <CAGXv+5Gzk2hDYFWTsN6V6LOfNyfrYg1ZqhsUHfTJMpOWh6f9ew@mail.gmail.com>
Date: Tue, 15 Jul 2025 12:33:42 +0800
From: Chen-Yu Tsai <wenst@...omium.org>
To: Laura Nao <laura.nao@...labora.com>
Cc: angelogioacchino.delregno@...labora.com, arnd@...db.de, 
	conor+dt@...nel.org, devicetree@...r.kernel.org, drew@...7.com, 
	gregkh@...uxfoundation.org, kabel@...nel.org, kernel@...labora.com, 
	khilman@...libre.com, krzk@...nel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, linux-mediatek@...ts.infradead.org, 
	m.wilczynski@...sung.com, matthias.bgg@...il.com, nm@...com, 
	pjp@...oraproject.org, quic_hyiwei@...cinc.com, robh@...nel.org, 
	tudor.ambarus@...aro.org, u.kleine-koenig@...libre.com, 
	ulf.hansson@...aro.org
Subject: Re: [RFC PATCH 2/3] dt-bindings: firmware: Document the MediaTek
 Hardware Voter (HWV)

On Fri, Jul 11, 2025 at 12:28 AM Laura Nao <laura.nao@...labora.com> wrote:
>
> On 7/7/25 12:40, AngeloGioacchino Del Regno wrote:
> > Il 03/07/25 10:56, AngeloGioacchino Del Regno ha scritto:
> >> Il 02/07/25 08:50, Krzysztof Kozlowski ha scritto:
> >>> On Tue, Jul 01, 2025 at 05:11:48PM +0200, AngeloGioacchino Del Regno wrote:
> >>>> Add documentation for the new MediaTek Hardware Voter, found in
> >>>> MediaTek SoCs like the MT8196 Kompanio Ultra for Chromebooks and
> >>>> the MT6991 Dimensity 9400 for Smartphones.
> >>>>
> >>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
> >>>> ---
> >>>>   .../mediatek,mt6991-hardware-voter.yaml       | 70 +++++++++++++++++++
> >>>>   1 file changed, 70 insertions(+)
> >>>>   create mode 100644 Documentation/devicetree/bindings/firmware/mediatek,mt6991- hardware-voter.yaml
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/firmware/mediatek,mt6991- hardware-voter.yaml b/Documentation/devicetree/bindings/firmware/ mediatek,mt6991-hardware-voter.yaml
> >>>> new file mode 100644
> >>>> index 000000000000..173b74c23a91
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/firmware/mediatek,mt6991-hardware- voter.yaml
> >>>> @@ -0,0 +1,70 @@
> >>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >>>> +# Copyright 2025 Collabora Ltd
> >>>> +%YAML 1.2
> >>>> +---
> >>>> +$id: http://devicetree.org/schemas/firmware/mediatek,mt6991-hardware-voter.yaml#
> >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>> +
> >>>> +title: MediaTek Hardware Voter (HWV)
> >>>> +
> >>>> +maintainers:
> >>>> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
> >>>> +
> >>>> +description:
> >>>> +  The MediaTek Hardware Voter (HWV) is a SoC-internal fixed-function MCU
> >>>> +  used to collect votes from both the Application Processor and from the
> >>>> +  various other remote processors present in the SoC, and transparently
> >>>> +  turn on or off various hardware resources (for example, power domains
> >>>> +  or system clocks) based on aggregation of votes done in the HWV MCU's
> >>>> +  internal state machine, therefore guaranteeing synchronization of the
> >>>> +  hardware resource requests between all components of the SoC and hence
> >>>> +  avoiding, for example, unclocked or unpowered access to the hardware.
> >>>> +
> >>>> +properties:
> >>>> +  $nodename:
> >>>> +    pattern: "^system-controller@[0-9a-f]+$"
> >>>> +
> >>>> +  compatible:
> >>>> +    const: mediatek,mt6991-hardware-voter
> >>>> +
> >>>> +  reg:
> >>>> +    items:
> >>>> +      - description: Address and size of the Hardware Voter MMIO
> >>>> +
> >>>
> >>> No resources here, so this should go to power controller
> >>>
> >>>> +  power-controller:
> >>>> +    $ref: /schemas/power/mediatek,power-controller.yaml
> >>>> +
> >>>> +required:
> >>>> +  - compatible
> >>>> +  - reg
> >>>> +
> >>>> +additionalProperties: true
> >>>> +
> >>>> +examples:
> >>>> + - |
> >>>> +   scp_hwv: system-controller@...00000 {
> >>>> +     compatible = "mediatek,mt6991-hardware-voter";
> >>>> +     reg = <0 0x14500000 0 0x3000>;
> >>>> +
> >>>> +     power-controller {
> >>>> +       compatible = "mediatek,mt8196-hwv-scp-power-controller";
> >>>
> >>> mt8196 in mt6991 is very confusing.
> >>>
> >>
> >> Yeah that wasn't intentional; fyi, it's almost the same soc, that's why I mixed
> >> them up... :-)
> >>
> >>> Anyway, this does not address my comment at all. You again create some
> >>> sort of syscon for voting, so no. You are supposed to use generic API
> >>> for voting: clocks, power domains, interconnects - whatever is there
> >>> applicable or necessary.
> >>>
> >>
> >> Making that loud and clear: Interconnect is not applicable.
> >>
> >> The only way to do what you're proposing would be to add a bunch of `reg`
> >> to each devicetree node for each clock controller and each power controller;
> >> I can do that, but looks a bit dirty - and still yet another syscon-like
> >> alternative, but without having a real syscon declared in there.
> >>
> >> Mind you - both clock and power controllers are writing both to their own
> >> register space (and enabling external regulators, etc, for power domains)
> >> and to the hardware voter MMIO (which means that the HWV, in hardware, is
> >> fundamentally broken).
> >>
> >> After this reply, the only option that is left to me is the following:
> >>
> >>          topckgen: clock-controller@...00000 {
> >>              compatible = "mediatek,mt8196-topckgen", "syscon";
> >>              reg = <0 0x10000000 0 0x800>, <0 0x14500010 0 0x48>,
> >>                    <0 0x14502c08 0 0x24>;
> >>              reg-names = "base", "hwvoter-base", "hwvoter-status";
> >>              #clock-cells = <1>;
> >>          };
> >>
> >>          imp_iic_wrap_north: clock-controller@...30000 {
> >>              compatible = "mediatek,mt8196-imp-iic-wrap-n", "syscon";
> >>              reg = <0 0x13c30000 0 0x1000>, <0 0x14500000 0 0xc>,
> >>                    <0 0x14502c00 0 0xc>;
> >>              reg-names = "base", "hwvoter-base", "hwvoter-status";
> >>              #clock-cells = <1>;
> >>          };
> >>
> >>          /* Power Manager with Hardware Voter */
> >>          spm_hwv: power-controller@...00218 {
> >>              compatible = "mediatek,mt8196-hwv-scp-power-controller";
> >>              reg = <0 0x14500218 0 0x20>, <0 0x14501410 0 0x20>,
> >>                    <0 0x14505514 0 0xc>;
> >>              reg-names = "hwvoter-base", "hwvoter-status", "hwvoter-ack";
> >>              #address-cells = <1>;
> >>              #size-cells = <0>;
> >>              #power-domain-cells = <1>;
> >>
> >>              /* SCPSYS hardware voter power domains */
> >>              mm_proc_dormant: power-domain@...196_POWER_DOMAIN_MM_PROC_DORMANT {
> >>                  ..... etc, all power domains
> >>
> >> At this point, I'm really not sure that this would be better than just passing
> >> the mediatek,hardware-voter syscon to the clock controllers - as what I've done
> >> previously was effectively representing the hardware in the devicetree as it is,
> >> matching the real HW layout 1:1 (because again, each of the whole HWV MCU(s) are
> >> embedded into each of the two power controllers, one for System power, and one
> >> for Multimedia power).
> >>
> >> (btw, hardware speaking, the power controller is child of a system controller:
> >> there are two system controllers - "scpsystem" is for "compute part", and the
> >> "hfrpsystem" is for the "multimedia part" of the soc).
> >>
> >>   _______________________________________
> >> |                                       |
> >> | SYSTEM CONTROLLER (SCPSYS or HFRPSYS) |
> >> |   _____________________               |
> >> |  |                     |              | <===> Clock Controllers (more than one)
> >> |  | Power Controller    |     SOME     |       (provide subsystem clocks for iso
> >> |  |                     |    OTHER     |        during power domain enablement
> >> |  |     ______________  |   BLOCKS     |        even if a PD is voted)
> >> |  |    |              | |              |       non-subsystem clocks are voted,
> >> |  |    | HW Voter MCU | |              |       but subsystem ones are not voted
> >> |  |    |______________| |              |
> >> |  |_____________________|              | ===> Rest of the SoC
> >> |_______________________________________|
> >>
> >>
> >> Hence I'm asking you - does your idea still stand?
> >>
> >> Because after this, sorry for that - this doesn't want to be an attack - but
> >> I'm starting to have doubts about an approach that doesn't involve syscons.
> >>
> >> Cheers,
> >> Angelo
> >
> > Sorry for the double reply, wanted to add some more words :-)
> >
> > As a note, I also thought about doing the following:
> >
> >     /* Secondary SCPSYS block with HWV capabilities */
> >     scp1_hwv: system-controller@...00000 {
> >         compatible = "mediatek,mt8196-scpsys", "syscon", "simple-mfd";
> >         reg = <0 0x14500000 0 0x3000>;
> >
> >         /* SCP Power Manager with Hardware Voter */
> >         spm_hwv: power-controller {
> >             compatible = "mediatek,mt8196-hwv-scp-power-controller";
> >             #address-cells = <1>;
> >             #size-cells = <0>;
> >             #power-domain-cells = <1>;
> >
> >             /* SCPSYS hardware voter power domains */
> >             mm_proc_dormant: power-domain@...196_POWER_DOMAIN_MM_PROC_DORMANT {
> >             ..... etc etc
> >             };
> >         };
> >
> >         imp_iic_wrap_north: clock-controller@...30000 {
> >             compatible = "mediatek,mt8196-imp-iic-wrap-n", "syscon";
> >             reg = <0 0x13c30000 0 0x1000>;
> >             #clock-cells = <1>;
> >         };
> >     };
> >
> > ...but that's also not applicable, because the clock controllers are physically
> > *not* inside of the scpsys1 block, so that would *also* misrepresent the hardware
> > in the devicetree (besides still using a syscon in a way or another).
> >
> > So... I really don't see any way out of that, which really leaves me with the two
> > options that I described in the previous reply.
> >
> > Summarizing, either:
> >  - Adding hwv MMIOs (a bunch of, and each very small) to each clock controller (but
> >    still all of them are poking at the same HWV controller, and I foresee that this
> >    will backfire in some future iteration of the HWV hardware)
> >  - Reverting back to using the "mediatek,hardware-voter" syscon, like done in
> >    https://lore.kernel.org/20250624143220.244549-10-laura.nao@collabora.com
> >
> > I tried really hard and thought about this for weeks (actually, started even before
> > your feedback on Laura's series), but now I'm out of practical options that are
> > both correctly representing the hardware and not making the implementation fragile
> > (or actually more fragile than the actually broken HW implementation's fragility,
> > anyway).
> >
> > And besides - re-reading what I wrote after a bunch of days, the first option of
> > adding a bunch of hwv mmios to all of the clock controllers is, in my opinion, a
> > (dirty) hack - because those mmios don't belong to the clock controllers, and would
> > again misrepresent the hardware in DT - especially keeping in mind the fact that
> > the clock controllers can be controlled with *and* (not or) without the HWV (and in
> > some instances, even if using HWV, we must still write to the clock controllers'
> > mmio for extra programming, as explained before).
> >
> > Every second I think about this I get more and more convinced that my way of
> > passing the SCPSYS-HWV system controller handle as a syscon is right.
> >
> > Angelo
> >
>
> I’ve given this some more thought over the past few days.
>
> I can't see of any other viable alternative either, other than splitting
> up the HWV MMIOs into multiple tiny reg entries across each consumer
> node as mentioned..which feels fragile and wouldn't really be an accurate
> representation of the HW, given they all ultimately target the same HWV MCU.
>
> All considered, modeling the HWV as a shared syscon passed to both clock
> and power controllers still seems to me like the more accurate
> representation of this specific hw layout, given both types of
> controllers interact with the same voter logic and MMIO region.
>
> Krzysztof, does the approach with scattered small reg entries per clock
> controller seem as awkward to you as it does to us? are there any other
> directions you think we should explore here?

Just to add to the argument, the hardware voter is really just an
alternative interface to the same clock or power controller; it is
not a separate controller. Trying to model it as such means you end
up with two devices fighting over control over the same hardware.

It really should be *one* clock controller with an alternative address
space. How we choose to model the hardware voter address space is up
for debate as Laura mentioned, but modeling it as another clock
controller is misrepresenting the hardware at best, and causing more
confusion for the implementation at worst.

ChenYu

Powered by blists - more mailing lists