linux-kernel - Re: [PATCH] arm64: dts: mt8192: Add adsp power domain controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fd4e49139285e8506938ab142ccd429bae980fe2.camel@mediatek.com>
Date:   Fri, 2 Dec 2022 08:00:04 +0000
From:   Allen-KH Cheng (程冠勳) 
        <Allen-KH.Cheng@...iatek.com>
To:     "wenst@...omium.org" <wenst@...omium.org>,
        "angelogioacchino.delregno@...labora.com" 
        <angelogioacchino.delregno@...labora.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Project_Global_Chrome_Upstream_Group 
        <Project_Global_Chrome_Upstream_Group@...iatek.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>
Subject: Re: [PATCH] arm64: dts: mt8192: Add adsp power domain controller

Hi Angelo and Chen-Yu,

On Thu, 2022-12-01 at 11:10 +0100, AngeloGioacchino Del Regno wrote:
> Il 01/12/22 11:05, Chen-Yu Tsai ha scritto:
> > On Thu, Dec 1, 2022 at 5:39 PM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@...labora.com> wrote:
> > > 
> > > Il 01/12/22 10:10, Chen-Yu Tsai ha scritto:
> > > > On Thu, Dec 1, 2022 at 5:07 PM AngeloGioacchino Del Regno
> > > > <angelogioacchino.delregno@...labora.com> wrote:
> > > > > 
> > > > > Il 01/12/22 08:33, Allen-KH Cheng ha scritto:
> > > > > > Add adsp power domain controller node for mt8192 SoC.
> > > > > > 
> > > > > > Signed-off-by: Allen-KH Cheng <allen-kh.cheng@...iatek.com>
> > > > > > ---
> > > > > > Ref: 
> > > > > > https://lore.kernel.org/all/2ec80bd8-dfef-d2e6-eb41-6e6088043e33@collabora.com/
> > > > > >        [Allen-KH Cheng <allen-kh.cheng@...iatek.com>]
> > > > > > ---
> > > > > > ---
> > > > > >     arch/arm64/boot/dts/mediatek/mt8192.dtsi | 8 ++++++++
> > > > > >     include/dt-bindings/power/mt8192-power.h | 1 +
> > > > > >     2 files changed, 9 insertions(+)
> > > > > > 
> > > > > 
> > > > > Allen, thanks for this one, but it's incomplete...
> > > > > 
> > > > > First of all, you must add the power domain on the driver
> > > > > itself, specifically,
> > > > > in drivers/soc/mediatek/mt8192-pm-domains.h - otherwise this
> > > > > change will have no
> > > > > effect!
> > > > 
> > > > Actually it's worse. The driver doesn't know about the new
> > > > power domain,
> > > > and so it will fail to probe. We might need to make the power
> > > > domain driver
> > > > fail gracefully and skip unknown power domains.
> > > > 
> > > 
> > > Right. It's worse. I don't know, though, if gracefully skipping
> > > unknown power
> > > domains in the driver would be a good decision... as sometimes
> > > error messages
> > > go unnoticed.
> > > 
> > > When the platform "explodes" instead, you're forced to read that
> > > log carefully
> > > and get it working again... Anyway, I'm only thinking out loud,
> > > nothing less and
> > > nothing more than that :-)
> > 
> > Me too. :)
> > 
> > > By the way, we can probably expand on that topic a bit later, as
> > > it's outside of
> > > the scope of this specific change.
> > > 
> > > Back to topic, if we get one series containing both changes
> > > (devicetree, bindings
> > > and driver) with the right Fixes tags and/or Cc stable, we
> > > shouldn't have such
> > > issue on backports so we can probably ignore that potential
> > > issue, I think? :-)
> > 
> > Everything goes through the soc tree, so they should appear in
> > Linus's tree
> > and get picked by stable at more or less the same time. I think we
> > would
> > want the driver change to appear before the dts change, for
> > bisectability's
> > sake (because of header dependencies and driver errors).
> > 
> > So we probably want:
> > 1. driver + binding header changes
> > 2. dtsi changes
> > 
> > And have these merged through fixes so that the history between
> > them is linear.
> > 
> 
> Perfect, I fully agree.
> 

Thank you for your comments.

I need to check internally with my coworkers for driver and will update
v2.

Best Regards,
Allen

> > 
> > ChenYu
> > 
> > > Cheers,
> > > Angelo
> > > 
> > > > ChenYu
> > > > 
> > > > > ...Then, as Chen-Yu said, you should also add the power
> > > > > domain to the scp_adsp
> > > > > clock node as that's solving the lockup issue...
> > > > > 
> > > > > .......and last, but not least: we need a Fixes tag to
> > > > > backport this fix, here
> > > > > and on the commit that adds the missing power domain in the
> > > > > driver.
> > > > > 
> > > > > Thanks,
> > > > > Angelo
> > > > > 
> > > > > > diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > > > > > b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > > > > > index 424fc89cc6f7..e71afba871fc 100644
> > > > > > --- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > > > > > +++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > > > > > @@ -514,6 +514,14 @@
> > > > > >                                                 };
> > > > > >                                         };
> > > > > >                                 };
> > > > > > +
> > > > > > +                             power-domain@...192_POWER_DOM
> > > > > > AIN_ADSP {
> > > > > > +                                     reg =
> > > > > > <MT8192_POWER_DOMAIN_ADSP>;
> > > > > > +                                     clocks = <&topckgen
> > > > > > CLK_TOP_ADSP_SEL>;
> > > > > > +                                     clock-names = "adsp";
> > > > > > +                                     mediatek,infracfg =
> > > > > > <&infracfg>;
> > > > > > +                                     #power-domain-cells =
> > > > > > <0>;
> > > > > > +                             };
> > > > > >                         };
> > > > > >                 };
> > > > > > 
> > > > > > diff --git a/include/dt-bindings/power/mt8192-power.h
> > > > > > b/include/dt-bindings/power/mt8192-power.h
> > > > > > index 4eaa53d7270a..63e81cd0d06d 100644
> > > > > > --- a/include/dt-bindings/power/mt8192-power.h
> > > > > > +++ b/include/dt-bindings/power/mt8192-power.h
> > > > > > @@ -28,5 +28,6 @@
> > > > > >     #define MT8192_POWER_DOMAIN_CAM_RAWA        18
> > > > > >     #define MT8192_POWER_DOMAIN_CAM_RAWB        19
> > > > > >     #define MT8192_POWER_DOMAIN_CAM_RAWC        20
> > > > > > +#define MT8192_POWER_DOMAIN_ADSP     21
> > > > > > 
> > > > > >     #endif /* _DT_BINDINGS_POWER_MT8192_POWER_H */
> > > > > > 
> > > 
> > > 
> 
>