Re: [PATCH v3 03/10] arm64: dts: sdm845: Introduce ADSP and CDSP PAS nodes
Quoting Doug Anderson (2019-01-23 15:24:36)
> Hi,
>
> On Tue, Jan 22, 2019 at 5:09 PM Bjorn Andersson
> wrote:
> >
> > On Tue 22 Jan 16:40 PST 2019, Doug Anderson wrote:
> >
> > > Hi,
> > >
> > > On Tue, Jan 22, 2019 at 4:26 PM Bjorn Andersson
> > > wrote:
> > > > > > + 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).
> > >
> > > Note that, as defined in the device tree, "xo_board" is actually 38.4.
> > > IIUC that is not actually a fake/bogus clock but represents the actual
> > > crystal on the board. There's a divide by 2 in the CPU though so most
> > > peripherals consider "xo" as 19.2.
> > >
> >
> > There's the 38.4MHz XO connected to the PMIC, but the signal going into
> > the CXO_IN pad of the SoC is supposed to come from LNBBCLK1 and be
> > 19.2MHz.
>
> Ah, thanks for pointing me to the right clock! :-)
>
> OK, so something is definitely wonky then. "xo_board" is definitely
> 38.4 currently in the device tree. That's my fault due to commit
> 5ea3939cf51f ("arm64: dts: sdm845: Fix xo_board clock name and
> speed"). ...but, in my defense:
>
> A) The hardcoded "divide by 2" for "bi_tcxo" in "clk-rpmh.c" came from
> Qualcomm.
>
> B) The parent of "bi_tcxo" has always been "xo_board"
>
> C) Children of "bi_tcxo" have always been only correct if "bi_tcxo" was 19.2
>
>
> ...so if "cxo" is really 19.2 then we need to update clk-rpmh to get
> rid of the hardcoded divide by 2 I think?
>
It looks OK to me the way it is. Here's why. xo_board is the crystal on
the board. That is 38.4 MHz. The lnbbclk1 signal is 19.2MHz (really? I
thought it was still 38.4 and there was a divider on the CXO pin inside
the SoC). The RPMh driver design to do a div-by-2 on the parent clk is
there to model how the PMIC or the SoC is dividing down that crystal
frequency to be 19.2 MHz for the CXO that the rest of the clk tree in
the SoC sees. So if devices still want to use something that isn't the
rpmh 'bi_tcxo' clk, they should be referencing something like lnbbclk1
as their input clk, and that lnbbclk1 should be a new fixed factor clk
specified in DT that takes the xo_board and creates the lnbbclk1
frequency out of it. Or if we want to get really fancy we can populate
that from the PMIC clk div module itself and then show how the PMIC is
doing the division from the buffer. I don't know if anyone really cares
about this level of detail though.
The reason that certain devices may not want to specify the RPMh clk
'bi_tcxo' is because they may not want the RPM to keep the XO buffer
enabled across low power modes. I don't think this is very common, but
it may be that there isn't any need to do anything besides calculate
some frequency based on the CXO frequency at the SoC's pin and thus
specifying the "raw" XO clk works just as well. I have no idea if that's
the case here.
Re: [PATCH v3 03/10] arm64: dts: sdm845: Introduce ADSP and CDSP PAS nodes
Hi,
On Tue, Jan 22, 2019 at 5:09 PM Bjorn Andersson
wrote:
>
> On Tue 22 Jan 16:40 PST 2019, Doug Anderson wrote:
>
> > Hi,
> >
> > On Tue, Jan 22, 2019 at 4:26 PM Bjorn Andersson
> > wrote:
> > > > > + 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).
> >
> > Note that, as defined in the device tree, "xo_board" is actually 38.4.
> > IIUC that is not actually a fake/bogus clock but represents the actual
> > crystal on the board. There's a divide by 2 in the CPU though so most
> > peripherals consider "xo" as 19.2.
> >
>
> There's the 38.4MHz XO connected to the PMIC, but the signal going into
> the CXO_IN pad of the SoC is supposed to come from LNBBCLK1 and be
> 19.2MHz.
Ah, thanks for pointing me to the right clock! :-)
OK, so something is definitely wonky then. "xo_board" is definitely
38.4 currently in the device tree. That's my fault due to commit
5ea3939cf51f ("arm64: dts: sdm845: Fix xo_board clock name and
speed"). ...but, in my defense:
A) The hardcoded "divide by 2" for "bi_tcxo" in "clk-rpmh.c" came from Qualcomm.
B) The parent of "bi_tcxo" has always been "xo_board"
C) Children of "bi_tcxo" have always been only correct if "bi_tcxo" was 19.2
...so if "cxo" is really 19.2 then we need to update clk-rpmh to get
rid of the hardcoded divide by 2 I think?
-Doug
Re: [PATCH v3 03/10] arm64: dts: sdm845: Introduce ADSP and CDSP PAS nodes
On Tue 22 Jan 16:40 PST 2019, Doug Anderson wrote: > Hi, > > On Tue, Jan 22, 2019 at 4:26 PM Bjorn Andersson > wrote: > > > > + 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). > > Note that, as defined in the device tree, "xo_board" is actually 38.4. > IIUC that is not actually a fake/bogus clock but represents the actual > crystal on the board. There's a divide by 2 in the CPU though so most > peripherals consider "xo" as 19.2. > There's the 38.4MHz XO connected to the PMIC, but the signal going into the CXO_IN pad of the SoC is supposed to come from LNBBCLK1 and be 19.2MHz. > ...OK, confirmed. The actual RF_XO_CLK pin on the board is truly > connected to 38.4. > And the three RF clocks from the PMIC are all ticking at 38.4MHz. The "xo" I need here is the LNBBCLK1 (RPMH_CXO_CLK in clk-rpmh), for the purpose of preventing the root clock to be turned off if apps goes to suspend while the modem is booting, before it has had a chance to tell RPM(h) that it needs it to be on. Regards, Bjorn
Re: [PATCH v3 03/10] arm64: dts: sdm845: Introduce ADSP and CDSP PAS nodes
Hi, On Tue, Jan 22, 2019 at 4:26 PM Bjorn Andersson wrote: > > > + 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). Note that, as defined in the device tree, "xo_board" is actually 38.4. IIUC that is not actually a fake/bogus clock but represents the actual crystal on the board. There's a divide by 2 in the CPU though so most peripherals consider "xo" as 19.2. ...OK, confirmed. The actual RF_XO_CLK pin on the board is truly connected to 38.4. -Doug
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
> 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
> > ---
> >
> > 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
Re: [PATCH v3 03/10] arm64: dts: sdm845: Introduce ADSP and CDSP PAS nodes
Hi,
On Mon, Jan 21, 2019 at 9:52 PM Bjorn Andersson
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
> ---
>
> 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 ?
> + 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?
-Doug
[PATCH v3 03/10] arm64: dts: sdm845: Introduce ADSP and CDSP PAS nodes
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
---
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(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
index af8c6a2445a2..02b8357c8ce8 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
@@ -48,6 +48,10 @@
};
};
+&adsp_pas {
+ status = "okay";
+};
+
&apps_rsc {
pm8998-rpmh-regulators {
compatible = "qcom,pm8998-rpmh-regulators";
@@ -344,6 +348,10 @@
};
};
+&cdsp_pas {
+ status = "okay";
+};
+
&gcc {
protected-clocks = ,
,
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi
b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 64f57cc5c61a..1033b77856e6 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -319,6 +319,64 @@
};
};
+ 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";
+
+ memory-region = <&adsp_mem>;
+
+ qcom,smem-states = <&adsp_smp2p_out 0>;
+ qcom,smem-state-names = "stop";
+
+ status = "disabled";
+
+ glink-edge {
+ interrupts = ;
+ label = "lpass";
+ qcom,remote-pid = <2>;
+ mboxes = <&apss_shared 8>;
+ };
+ };
+
+ cdsp_pas: remoteproc-cdsp {
+ compatible = "qcom,sdm845-cdsp-pas";
+
+ interrupts-extended = <&intc GIC_SPI 578 IRQ_TYPE_EDGE_RISING>,
+ <&cdsp_smp2p_in 0 IRQ_TYPE_EDGE_RISING>,
+ <&cdsp_smp2p_in 1 IRQ_TYPE_EDGE_RISING>,
+ <&cdsp_smp2p_in 2 IRQ_TYPE_EDGE_RISING>,
+ <&cdsp_smp2p_in 3 IRQ_TYPE_EDGE_RISING>;
+ interrupt-names = "wdog", "fatal", "ready",
+ "handover", "stop-ack";
+
+ clocks = <&xo_board>;
+ clock-names = "xo";
+
+ memory-region = <&cdsp_mem>;
+
+ qcom,smem-states = <&cdsp_smp2p_out 0>;
+ qcom,smem-state-names = "stop";
+
+ status = "disabled";
+
+ glink-edge {
+ interrupts = ;
+ label = "turing";
+ qcom,remote-pid = <5>;
+ mboxes = <&apss_shared 4>;
+ };
+ };
+
tcsr_mutex: hwlock {
compatible = "qcom,tcsr-mutex";
syscon = <&tcsr_mutex_regs 0 0x1000>;
--
2.18.0
