Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 2 Nov 2022 14:44:20 -0500
From:   Nishanth Menon <nm@ti.com>
To:     Andrew Davis <afd@ti.com>
CC:     Jayesh Choudhary <j-choudhary@ti.com>, <vigneshr@ti.com>,
        <kristo@kernel.org>, <robh+dt@kernel.org>, <j-keerthy@ti.com>,
        <krzysztof.kozlowski+dt@linaro.org>, <s-anna@ti.com>,
        <linux-arm-kernel@lists.infradead.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/3] arm64: dts: ti: k3-am65-main: drop RNG clock
Message-ID: <20221102194420.umwuyk374g2mgg45@unlucky>
References: <20221031213237.52275-1-j-choudhary@ti.com>
 <20221031213237.52275-2-j-choudhary@ti.com>
 <20221102151706.krsi5lujydb4nswa@daybreak>
 <4f954c08-6a2e-93b5-6806-7b27b247496e@ti.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <4f954c08-6a2e-93b5-6806-7b27b247496e@ti.com>
User-Agent: NeoMutt/20171215
Precedence: bulk

On 12:04-20221102, Andrew Davis wrote:
> On 11/2/22 10:17 AM, Nishanth Menon wrote:
> > On 03:02-20221101, Jayesh Choudhary wrote:
> > > Drop RNG clock property as it is not controlled by rng-driver.
> > 
> > Does'nt tell me what is the alternative? why is the hardware description
> > not sufficient for control?
> > 
> > https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/am65x_sr2/clocks.html#clocks-for-sa2-ul0-device
> > Looks like a perfectly valid description - do we have a bug and firmware
> > does'nt allow control here?
> > 
> 
> We have three input clocks feeding the SA2UL module, x1, x2, pka. PKA goes
> to the PKA sub-module (isn't it nice when they make things simple). But x1 and
> x2 are miscellaneous and bus clocks respectively and route to several sub-modules.
> 
> All we drop here is the clock handle in the RNG sub-module, as that sub-module is
> not the owner of that clock (the parent SA2UL is). The alternative we could implement
> is to move the clock node up to the parent SA2UL node.
> 
> > > 
> > > Fixes: b366b2409c97 ("arm64: dts: ti: k3-am6: Add crypto accelarator node")
> > > Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> > > ---
> > >   arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 1 -
> > >   1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> > > index 4005a73cfea9..e166d7b7e3a1 100644
> > > --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> > > +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> > > @@ -126,7 +126,6 @@ rng: rng@4e10000 {
> > >   			compatible = "inside-secure,safexcel-eip76";
> > >   			reg = <0x0 0x4e10000 0x0 0x7d>;
> > >   			interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>;
> > > -			clocks = <&k3_clks 136 1>;
> > 
> > Does this mean that the crypto module's power-domains property should be
> > dropped as well?
> > 
> 
> Why? the power-domains property is in the correct spot (up in the parent node).
> 
> Now it is true we cant actually shut the SA2UL down since it is owned
> by the security processor, but since it is marked TI_SCI_PD_SHARED this
> should be fine.

The idea of the descriptions were to describe what is controllable by
firmware, if there is no control due to the specified reason, it is a
device tree bug, and should be documented when dropping it. If it serves
a purpose in the firmware by indicating usage for example - it has valid
reason to stick around as it is expected to be used by firmware for some
specific reason.

The commit description does bring up the above mentioned questions and
must be explained appropriately.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D