| Message ID | 20190122055112.30943-4-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Qualcomm AOSS QMP driver and modem dts | expand |
Hi, On Mon, Jan 21, 2019 at 9:52 PM Bjorn Andersson <bjorn.andersson@linaro.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@linaro.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 ? > + 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
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@linaro.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@linaro.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
Hi, On Tue, Jan 22, 2019 at 4:26 PM Bjorn Andersson <bjorn.andersson@linaro.org> 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
On Tue 22 Jan 16:40 PST 2019, Doug Anderson wrote: > Hi, > > On Tue, Jan 22, 2019 at 4:26 PM Bjorn Andersson > <bjorn.andersson@linaro.org> 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
Hi, On Tue, Jan 22, 2019 at 5:09 PM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Tue 22 Jan 16:40 PST 2019, Doug Anderson wrote: > > > Hi, > > > > On Tue, Jan 22, 2019 at 4:26 PM Bjorn Andersson > > <bjorn.andersson@linaro.org> 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
Quoting Doug Anderson (2019-01-23 15:24:36) > Hi, > > On Tue, Jan 22, 2019 at 5:09 PM Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > > > On Tue 22 Jan 16:40 PST 2019, Doug Anderson wrote: > > > > > Hi, > > > > > > On Tue, Jan 22, 2019 at 4:26 PM Bjorn Andersson > > > <bjorn.andersson@linaro.org> 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.
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 = <GCC_QSPI_CORE_CLK>, <GCC_QSPI_CORE_CLK_SRC>, 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 = <GIC_SPI 156 IRQ_TYPE_EDGE_RISING>; + 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 = <GIC_SPI 574 IRQ_TYPE_EDGE_RISING>; + label = "turing"; + qcom,remote-pid = <5>; + mboxes = <&apss_shared 4>; + }; + }; + tcsr_mutex: hwlock { compatible = "qcom,tcsr-mutex"; syscon = <&tcsr_mutex_regs 0 0x1000>;
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@linaro.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(+)