linux-kernel - Re: [PATCH v3 03/10] arm64: dts: sdm845: Introduce ADSP and CDSP PAS nodes

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 22 Jan 2019 16:26:10 -0800
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Andy Gross <andy.gross@...aro.org>,
        David Brown <david.brown@...aro.org>,
        Sibi Sankar <sibis@...eaurora.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        devicetree@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 03/10] arm64: dts: sdm845: Introduce ADSP and CDSP PAS
 nodes

On Tue 22 Jan 15:46 PST 2019, Doug Anderson wrote:

> Hi,
> 
> On Mon, Jan 21, 2019 at 9:52 PM Bjorn Andersson
> <bjorn.andersson@...aro.org> wrote:
> >
> > Add the ADSP and CDSP nodes for PAS-based remoteproc, supporting booting
> > these cores on e.g. the MTP, and enable the same for the MTP.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@...aro.org>
> > ---
> >
> > Changes since v2:
> > - New patch
> >
> >  arch/arm64/boot/dts/qcom/sdm845-mtp.dts |  8 ++++
> >  arch/arm64/boot/dts/qcom/sdm845.dtsi    | 58 +++++++++++++++++++++++++
> >  2 files changed, 66 insertions(+)
> 
> It's a bit of a nit of mine that if it's not totally obvious what
> acronyms mean that they should be spelled out in places that use them.
> 
> In this case I believe ADSP is the Audio DSP.  Is CDSP the Camera DSP?  ...or ?
> 

C as in Compute. I'll spell these out as I respin the series.

> 
> > +       adsp_pas: remoteproc-adsp {
> > +               compatible = "qcom,sdm845-adsp-pas";
> > +
> > +               interrupts-extended = <&intc GIC_SPI 162 IRQ_TYPE_EDGE_RISING>,
> > +                                     <&adsp_smp2p_in 0 IRQ_TYPE_EDGE_RISING>,
> > +                                     <&adsp_smp2p_in 1 IRQ_TYPE_EDGE_RISING>,
> > +                                     <&adsp_smp2p_in 2 IRQ_TYPE_EDGE_RISING>,
> > +                                     <&adsp_smp2p_in 3 IRQ_TYPE_EDGE_RISING>;
> > +               interrupt-names = "wdog", "fatal", "ready",
> > +                                 "handover", "stop-ack";
> > +
> > +               clocks = <&xo_board>;
> > +               clock-names = "xo";
> 
> I've found that nearly all the places that refer to xo_board are wrong
> and should actually point to '<&rpmhcc RPMH_CXO_CLK>'.  Maybe yours
> should too?
> 

Yes, xo_board is a fake clock representing the 19.2MHz clock feeding the
cxo (or cxo2) pad of the SoC. So you're definitely right in that this
should be referencing the actual 19.2MHz clock.

We've kept referring to this as xo_board, as we don't handle probe
deferral when gcc will probe earlier than rpmcc in the boot and for
other non-clock drivers the fear of actually hitting 0 on the refcounter
for this (you don't want to disable the cxo while running the system).

I'll give it a spin with appropriate reference and see what happens, I
think this should either be changed or documented in the commit message.

Thanks,
Bjorn