mbox series

[v4,0/6] Add MSM8939 SoC support with two devices

Message ID 20230123023127.1186619-1-bryan.odonoghue@linaro.org (mailing list archive)
Headers show
Series Add MSM8939 SoC support with two devices | expand

Message

Bryan O'Donoghue Jan. 23, 2023, 2:31 a.m. UTC
V4:
- Adds Krzysztof's RB to snoc-mm
- Re-orders alphabetically missed nodes in previous iteration - Bjorn
- Adds LK address/size cells comment - Bjorn

- Left _AO for wcnss as downstream reference uses this - Bjorn/Bryan
- Uses qcom,ids.h and QCOM_ID_SOCNAME for qcom,msm-id - Bjorn
- Revises comment from "Regulator" to "Power supply" - Bjorn
- Leaves dummy power-domain reference in cpu defintion as this

- Relabels "cpu" to "CPU" to be more consistent with other dtsi - Bryan
- Moves msm8939 gcc to its own yaml file to capture 8939 specific form - Bryan

  is a required property and the dt checker complains - Stephan/Bryan
- Removes CPR entries from qfprom - Stephan
- Left MDSS interconnects. I don't see a bug to fix here - Stephan/Bryan
- power-domain in MDSS - dropped its not longer required after
  commit a6f033938beb ("dt-bindings: msm: dsi-controller-main: Fix power-domain constraint") - Stephan
- Adds gcc dsi1pll and dsi1pllbyte to gcc clock list.
  Reviewing the silicon documentation we see dsi0_phy_pll is used to clock
  GCC_BYTE1_CFG_RCGR : SRC_SEL
  Root Source Select
  000 : cxo
  001 : dsi0_phy_pll_out_byteclk
  010 : GPLL0_OUT_AUX
  011 : gnd
  100 : gnd
  101 : gnd
  110 : gnd
  111 : reserved - Stephan/Bryan

- pm8916_l16 -> pm8916_l6 in dsi definition, typo - Konrad
- Moved regulator_set_load location - Konrad

Previous: https://lore.kernel.org/lkml/20230118050948.bibhq26s6sgzullg@builder.lan/T/
Bootable: https://git.linaro.org/people/bryan.odonoghue/kernel.git/log/?h=linux-next-23-01-23-msm8939-nocpr

V3:
- Happily I don't currently depend on any other series to be merged.
  Bjorn and Chanwoo picked up everything I need to unblock this series. \(^o^)/

- Moves xo_board to RPM/PMIC clock gated CXO, not including rpmcc: obvs - Konrad/Bjorn
- qcom,msm-id = <239 0> - left as in V2 valid according to Sony references - bod
- cpu-release-addr - as stated below we rely on lk2nd to take the second cluster
  out of reset - bod
- smem child node update - Konrad
- Whitespace updates - Konrad
- gpu no interconnect - Konrad - No bod
- 19.2 MHz dropped from timer@b020000 - Konrad
- Added vreg_dummy comment - Konrad
- sdc_pins grouped - Konrad
- startup-delay-us = <0> - left as is
- bias - added no-bias - Konrad
- :g/msmgpio/s//tlmm/g - Konrad
- mdss/s//display-controller - Konrad
- l11 set-load - Korad

- l12 upper voltage raised to 3.3v since this is what the
  downstream kernel says when I boot and interrogate it - bod

- sdhc@address - Discussed with Krzysztof and implemented as discussed
- snoc-mm fix - Discussed with Krzysztof implemented if:then:else:not
- dtc -I dtb -fs apq8039-t2.dtb prodcues
  /soc@0/i2c@78b5000: duplicate unit-address
  as does every other component that uses this polymorphic dts node
- Renamed type-c i2c port manager IC to "typec" - Krzysztof

  /smsm/hexagon@1: Missing #address-cells in interrupt provider
  Same output as other upstream and recently upstreamed SoCs
  I left these alone for now

link: https://lore.kernel.org/lkml/20230103010904.3201835-1-bryan.odonoghue@linaro.org/T/
bootable: https://git.linaro.org/people/bryan.odonoghue/kernel.git/log/?h=linux-next-23-01-16-msm8939-nocpr

V2:
- Sorts core dtsi node list by address followed by alpahbetical sorting
  within address sorted nodes - Bjorn
- Drops use of 8916-pins - Bjorn
- Adds msm8939-pm8916.dtsi - Stephan
- Fixes every dts splat from previous submission minus non-converted
  .txt compat strings [1] and one yaml error in Bjorn's tree not in -next yet
- I haven't applied Dmitry's change for tsens since that's not been
  picked up yet
- Picks up a number of suggestions and fixes from Stephan Gerhold and Vincent Knecht

- Depends on

  Applied:
  [PATCH v4 0/7] remoteproc: qcom_q6v5_mss: Add MSM8909 and MSM8953 
  https://lore.kernel.org/linux-arm-msm/167216232800.738877.17567287056128563074.b4-ty@kernel.org/

  [PATCH v6 0/5] remoteproc: qcom: Add support for pronto-v3
  https://lore.kernel.org/linux-arm-msm/167216232801.738877.15895916910585144737.b4-ty@kernel.org/

  [PATCH v6 00/18] mdss-dsi-ctrl binding and dts fixes
  https://lore.kernel.org/linux-arm-msm/167233461766.1099840.17628700245792986354.b4-ty@kernel.org/

  Awaiting application:
  https://lore.kernel.org/linux-arm-msm/20221228133058.213886-1-bryan.odonoghue@linaro.org/

- Previous
  https://lore.kernel.org/linux-arm-msm/20220419010903.3109514-1-bryan.odonoghue@linaro.org/

- Bootable tree
  https://git.linaro.org/people/bryan.odonoghue/kernel.git/log/?h=linux-next-23-01-03-msm8939-no-cpr

- [1] DTC_CHK arch/arm64/boot/dts/qcom/apq8039-t2.dtb

  Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
  qcom/apq8039-t2.dtb: idle-states: cpu-sleep-0:compatible:0: 'qcom,idle-state-spc' is not one of ['arm,idle-state', 'riscv,idle-state']
        From schema: Documentation/devicetree/bindings/cpu/idle-states.yaml
  qcom/apq8039-t2.dtb: idle-states: cpu-sleep-0:compatible: ['qcom,idle-state-spc', 'arm,idle-state'] is too long
        From schema: Documentation/devicetree/bindings/cpu/idle-states.yaml
  arch/arm64/boot/dts/qcom/apq8039-t2.dtb:0:0: /cpus/idle-states/cpu-sleep-0: failed to match any schema with compatible: ['qcom,idle-state-spc', 'arm,idle-state']

  Documentation/devicetree/bindings/iommu/qcom,iommu.txt
  arch/arm64/boot/dts/qcom/apq8039-t2.dtb:0:0: /soc@0/iommu@1ef0000: failed to match any schema with compatible: ['qcom,msm8916-iommu', 'qcom,msm-iommu-v1']
  arch/arm64/boot/dts/qcom/apq8039-t2.dtb:0:0: /soc@0/iommu@1ef0000/iommu-ctx@4000: failed to match any schema with compatible: ['qcom,msm-iommu-v1-ns']
  arch/arm64/boot/dts/qcom/apq8039-t2.dtb:0:0: /soc@0/iommu@1ef0000/iommu-ctx@5000: failed to match any schema with compatible: ['qcom,msm-iommu-v1-sec']
  arch/arm64/boot/dts/qcom/apq8039-t2.dtb:0:0: /soc@0/iommu@1f08000: failed to match any schema with compatible: ['qcom,msm8916-iommu', 'qcom,msm-iommu-v1']
  arch/arm64/boot/dts/qcom/apq8039-t2.dtb:0:0: /soc@0/iommu@1f08000/iommu-ctx@1000: failed to match any schema with compatible: ['qcom,msm-iommu-v1-ns']
  arch/arm64/boot/dts/qcom/apq8039-t2.dtb:0:0: /soc@0/iommu@1f08000/iommu-ctx@2000: failed to match any schema with compatible: ['qcom,msm-iommu-v1-ns']

  arch/arm64/boot/dts/qcom/pm8916.dtsi f5d7bca55425c8
  qcom/apq8039-t2.dtb: pmic@0: 'extcon@1300' does not match any of the regexes: '(.*)?(wled|leds)@[0-9a-f]+$', '^adc-tm@[0-9a-f]+$', '^adc@[0-9a-f]+$', '^audio-codec@[0-9a-f]+$', '^charger@[0-9a-f]+$', '^mpps@[0-9a-f]+$', '^rtc@[0-9a-f]+$', '^temp-alarm@[0-9a-f]+$', '^usb-detect@[0-9a-f]+$', '^usb-vbus-regulator@[0-9a-f]+$', '^vibrator@[0-9a-f]+$', 'gpio@[0-9a-f]+$', 'pinctrl-[0-9]+', 'pon@[0-9a-f]+$'
        From schema: Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.yaml

  Documentation/devicetree/bindings/sound/qcom,msm8916-wcd-analog.txt
  arch/arm64/boot/dts/qcom/apq8039-t2.dtb:0:0: /soc@0/spmi@200f000/pmic@1/audio-codec@f000: failed to match any schema with compatible: ['qcom,pm8916-wcd-analog-codec']

  yaml documentation error not yet in -next
  arm64/boot/dts/qcom/apq8039-t2.dtb: remoteproc@4080000: qcom,halt-regs:0: [33] is too short
        From schema: Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml

  Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
  arch/arm64/boot/dts/qcom/apq8039-t2.dtb:0:0: /soc@0/usb@78d9000: failed to match any schema with compatible: ['qcom,ci-hdrc']

  Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt:            compatible = "qcom,kpss-acc-v2";
  arch/arm64/boot/dts/qcom/apq8039-t2.dtb:0:0: /soc@0/clock-controller@b088000: failed to match any schema with compatible: ['qcom,kpss-acc-v2']
  arch/arm64/boot/dts/qcom/apq8039-t2.dtb:0:0: /soc@0/clock-controller@b098000: failed to match any schema with compatible: ['qcom,kpss-acc-v2']
  arch/arm64/boot/dts/qcom/apq8039-t2.dtb:0:0: /soc@0/clock-controller@b0a8000: failed to match any schema with compatible: ['qcom,kpss-acc-v2']
  arch/arm64/boot/dts/qcom/apq8039-t2.dtb:0:0: /soc@0/clock-controller@b0b8000: failed to match any schema with compatible: ['qcom,kpss-acc-v2']
  arch/arm64/boot/dts/qcom/apq8039-t2.dtb:0:0: /soc@0/clock-controller@b188000: failed to match any schema with compatible: ['qcom,kpss-acc-v2']
  arch/arm64/boot/dts/qcom/apq8039-t2.dtb:0:0: /soc@0/clock-controller@b198000: failed to match any schema with compatible: ['qcom,kpss-acc-v2']
  arch/arm64/boot/dts/qcom/apq8039-t2.dtb:0:0: /soc@0/clock-controller@b1a8000: failed to match any schema with compatible: ['qcom,kpss-acc-v2']
  arch/arm64/boot/dts/qcom/apq8039-t2.dtb:0:0: /soc@0/clock-controller@b1b8000: failed to match any schema with compatible: ['qcom,kpss-acc-v2']

V1:
This series adds in MSM8939 SoC support with two supported devices.

- CPU
  MSM8939 is a non-PSCI compliant device. As such in the downstreaming
  shipped image custom code is used to bring non-boot cores out of reset.

  This drop specifies the boot-method as spin-table instead and is
  completely standard. To accomplish this, we rely on lk2nd.

  https://github.com/msm8916-mainline/lk2nd/pull/142

- Serial
- i2c
- USB
- eMMC
- MDP/DSI
- WiFi
- Bluetooth

What's not included

- CPR
  We have CPR working in a 4.19 kernel quite well but for now it feels like
  putting the cart before the horse to gate the SoC and boards on CPR.

- Venus
  I've been told this works but I haven't tried it myself and right now
  consider it maybe working but probably not 100%.

- Sound
  We have a copy-exactly from the 4.19 kernel here in the DTS.
  I haven't run the sound through any sort of reasonable test.
  Vincent Knecht has some PostmarketOS kernels which use a 5.17 version of
  this DTS to get sound up so, I think sound is in good shape.

- CAMSS
  There are slight differences between msm8916 and msm8939 for CAMSS. It
  doesn't feel like tons of work but, right now it is work we haven't even
  started.

- Devices
  I've booted on the Square device obviously and this is my regular
  hardware for upstream development. I've also booted on the Sony Xperia M4
  Aqua including mutli-core bring-up, WiFi and ADB.

Dependencies for this drop:

qcom-cpufreq-nvmem: Add msm8939 with some fixups
link: https://lore.kernel.org/linux-arm-msm/20220418162226.2983117-1-bryan.odonoghue@linaro.org/T/#t

Fix apq8016 compat string
link: https://lore.kernel.org/linux-arm-msm/20220418230956.3059563-1-bryan.odonoghue@linaro.org/T/#t

dt-bindings: soc: qcom: smd-rpm: Fix missing MSM8936 compatible
link: https://lore.kernel.org/linux-arm-msm/20220418231857.3061053-1-bryan.odonoghue@linaro.org/T/#u

Bootable tree here:
https://git.linaro.org/people/bryan.odonoghue/kernel.git/log/?h=v5.18-rc2%2bapq8039-without-cpr

Bryan O'Donoghue (5):
  dt-bindings: clock: msm8939: Move msm8939 to a distinct yaml file
  dt-bindings: interconnect: Exclude all non msm8939 from snoc-mm
  arm64: dts: qcom: Add msm8939 SoC
  arm64: dts: qcom: Add Square apq8039-t2 board
  arm64: dts: qcom: Add msm8939 Sony Xperia M4 Aqua

Stephan Gerhold (1):
  arm64: dts: qcom: Add msm8939-pm8916.dtsi include

 .../bindings/clock/qcom,gcc-msm8916.yaml      |    7 +-
 .../bindings/clock/qcom,gcc-msm8939.yaml      |   87 +
 .../bindings/interconnect/qcom,rpm.yaml       |   73 +-
 arch/arm64/boot/dts/qcom/Makefile             |    2 +
 arch/arm64/boot/dts/qcom/apq8039-t2.dts       |  545 ++++
 arch/arm64/boot/dts/qcom/msm8939-pm8916.dtsi  |   82 +
 .../qcom/msm8939-sony-xperia-kanuti-tulip.dts |  453 ++++
 arch/arm64/boot/dts/qcom/msm8939.dtsi         | 2353 +++++++++++++++++
 8 files changed, 3566 insertions(+), 36 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,gcc-msm8939.yaml
 create mode 100644 arch/arm64/boot/dts/qcom/apq8039-t2.dts
 create mode 100644 arch/arm64/boot/dts/qcom/msm8939-pm8916.dtsi
 create mode 100644 arch/arm64/boot/dts/qcom/msm8939-sony-xperia-kanuti-tulip.dts
 create mode 100644 arch/arm64/boot/dts/qcom/msm8939.dtsi

Comments

Bryan O'Donoghue Jan. 23, 2023, 11 a.m. UTC | #1
On 23/01/2023 02:31, Bryan O'Donoghue wrote:
> - Leaves dummy power-domain reference in cpu defintion as this
is required by the yaml
Bryan O'Donoghue Jan. 23, 2023, 11:08 a.m. UTC | #2
On 23/01/2023 02:31, Bryan O'Donoghue wrote:
> V4:
> - Adds Krzysztof's RB to snoc-mm
> - Re-orders alphabetically missed nodes in previous iteration - Bjorn
> - Adds LK address/size cells comment - Bjorn
> 
> - Left _AO for wcnss as downstream reference uses this - Bjorn/Bryan
> - Uses qcom,ids.h and QCOM_ID_SOCNAME for qcom,msm-id - Bjorn
> - Revises comment from "Regulator" to "Power supply" - Bjorn
> - Leaves dummy power-domain reference in cpu defintion as this
> 
> - Relabels "cpu" to "CPU" to be more consistent with other dtsi - Bryan
> - Moves msm8939 gcc to its own yaml file to capture 8939 specific form - Bryan
> 
>    is a required property and the dt checker complains - Stephan/Bryan
> - Removes CPR entries from qfprom - Stephan
> - Left MDSS interconnects. I don't see a bug to fix here - Stephan/Bryan
> - power-domain in MDSS - dropped its not longer required after
>    commit a6f033938beb ("dt-bindings: msm: dsi-controller-main: Fix power-domain constraint") - Stephan
> - Adds gcc dsi1pll and dsi1pllbyte to gcc clock list.
>    Reviewing the silicon documentation we see dsi0_phy_pll is used to clock
>    GCC_BYTE1_CFG_RCGR : SRC_SEL
>    Root Source Select
>    000 : cxo
>    001 : dsi0_phy_pll_out_byteclk
>    010 : GPLL0_OUT_AUX
>    011 : gnd
>    100 : gnd
>    101 : gnd
>    110 : gnd
>    111 : reserved - Stephan/Bryan
> 
> - pm8916_l16 -> pm8916_l6 in dsi definition, typo - Konrad
> - Moved regulator_set_load location - Konrad
> 
> Previous:https://lore.kernel.org/lkml/20230118050948.bibhq26s6sgzullg@builder.lan/T/
> Bootable:https://git.linaro.org/people/bryan.odonoghue/kernel.git/log/?h=linux-next-23-01-23-msm8939-nocpr

Let me translate my 2:30 am email to english, I meant to group this by 
reviewer

V4:
- Adds Krzysztof's RB to snoc-mm

- Re-orders alphabetically missed nodes in previous iteration - Bjorn
- Adds LK address/size cells comment - Bjorn
- Left _AO for wcnss as downstream reference uses this - Bjorn/Bryan
- Uses qcom,ids.h and QCOM_ID_SOCNAME for qcom,msm-id - Bjorn
- Revises comment from "Regulator" to "Power supply" - Bjorn
- Leaves dummy power-domain reference in cpu defintion as this is a
   required property and the dt checker complains - Stephan/Bryan

- Relabels "cpu" to "CPU" to be more consistent with other dtsi - Bryan
- Moves msm8939 gcc to its own yaml file to 8939 specifics -Bryan


- Removes CPR entries from qfprom - Stephan
- Left MDSS interconnects. I don't see a bug to fix here - Stephan/Bryan
- power-domain in MDSS - dropped its not longer required after
   commit a6f033938beb ("dt-bindings: msm: dsi-controller-main: Fix 
power-domain constraint") - Stephan
- Adds gcc dsi1pll and dsi1pllbyte to gcc clock list.
   Reviewing the silicon documentation we see dsi0_phy_pll is used to clock
   GCC_BYTE1_CFG_RCGR : SRC_SEL
   Root Source Select
   000 : cxo
   001 : dsi0_phy_pll_out_byteclk
   010 : GPLL0_OUT_AUX
   011 : gnd
   100 : gnd
   101 : gnd
   110 : gnd
   111 : reserved - Stephan/Bryan

- pm8916_l16 -> pm8916_l6 in dsi definition, typo - Konrad
- Moved regulator_set_load location - Konrad
Stephan Gerhold Jan. 23, 2023, 12:49 p.m. UTC | #3
On Mon, Jan 23, 2023 at 11:08:28AM +0000, Bryan O'Donoghue wrote:
> V4:
> - Left _AO for wcnss as downstream reference uses this - Bjorn/Bryan

Downstream is just an implementation and contains plenty of misleading
or even wrong information. IMO Bjorn is right here that VDDMX_AO is not
a logical choice.

The _AO (active-only) suffix means that the votes are only applied when
the processor making the vote is "active", that is when the Linux CPUs
are not in deep cpuidle mode.

For WCNSS the goal is to keep the necessary power domains active while
WCNSS is booting up, until it is able to make its own votes (handover).
The WCNSS firmware might then vote for VDDMX_AO internally because VDDMX
is not needed when the WCNSS CPU is suspended.

However, I would expect that the meaning is totally different when the
same vote is made from Linux. When Linux votes for _AO the "active"
state likely refers to the Linux CPUs, instead of the WCNSS CPU when
made from the WCNSS firmware.

Why does it work in downstream then? I would just assume "side effects":
  - Something else votes for VDDMX without _AO while WCNSS is booting
  - The Linux CPUs don't go into deep cpuidle state during startup
    - In particular, note how downstream often has "lpm_levels.sleep_disabled=1"
      on the kernel command line. This disables all cpuidle states until
      late after boot-up when userspace changes this setting. Without
      cpuidle, VDDMX_AO is identical to VDDMX.

Please change it to VDDMX (without _AO). It will most likely not make
any difference, but IMO it is logcially more correct and less
confusing/misleading. :)

> - Leaves dummy power-domain reference in cpu defintion as this is a
>   required property and the dt checker complains - Stephan/Bryan

It's only required though because you forgot to drop the DT schema patch
(3/4) when I suggested half a year ago that you make the MSM8939
cpufreq-qcom-nvmem changes together with the CPR stack [1]. :/

Anyway, it looks like qcom-cpufreq-nvmem.yaml requiring "cpr" power
domain unconditionally is a mistake anyway for multiple platforms.
[2] was recently submitted to fix this so that patch should allow you to
drop the dummy nodes. :)

[1]: https://lore.kernel.org/linux-arm-msm/Ysf8VRaXdGg+8Ev3@gerhold.net/
[2]: https://lore.kernel.org/linux-arm-msm/20230122174548.13758-1-ansuelsmth@gmail.com/

> - Left MDSS interconnects. I don't see a bug to fix here - Stephan/Bryan

Fair enough, if you would like to keep it I will likely send a revert
for the MSM8939 icc_sync_state() though. Because clearly it breaks
setups without a display and I don't see how one would fix that from the
device tree.

Also: The undocumented "register-mem" interconnect is still there. :)

> - power-domain in MDSS - dropped its not longer required after
>   commit a6f033938beb ("dt-bindings: msm: dsi-controller-main: Fix
> power-domain constraint") - Stephan

Thanks!

> - Adds gcc dsi1pll and dsi1pllbyte to gcc clock list.
>   Reviewing the silicon documentation we see dsi0_phy_pll is used to clock
>   GCC_BYTE1_CFG_RCGR : SRC_SEL
>   Root Source Select
>   000 : cxo
>   001 : dsi0_phy_pll_out_byteclk
>   010 : GPLL0_OUT_AUX
>   011 : gnd
>   100 : gnd
>   101 : gnd
>   110 : gnd
>   111 : reserved - Stephan/Bryan
> 

I'm confused. Are you not contradicting yourself here? You say that
dsi0_phy_pll (dsi ZERO) is used to clock GCC_BYTE1_CFG_RCGR. Then why
do you add dsi1_phy_pll (dsi ONE) to the gcc clock list?

To me this looks like a confirmation of what downstream does, that both
DSI byte clocks are actually sourced from the dsi0_phy and the PLL of
dsi1_phy is not used.

Thanks,
Stephan
Bryan O'Donoghue Jan. 23, 2023, 1:03 p.m. UTC | #4
On 23/01/2023 12:49, Stephan Gerhold wrote:
> Also: The undocumented "register-mem" interconnect is still there. 
Bryan O'Donoghue Jan. 23, 2023, 1:23 p.m. UTC | #5
On 23/01/2023 12:49, Stephan Gerhold wrote:
> It's only required though because you forgot to drop the DT schema patch
> (3/4) when I suggested half a year ago that you make the MSM8939
> cpufreq-qcom-nvmem changes together with the CPR stack [1]. :/

Didn't forget, tested that and as I recall there are side-effects 
removing 8939 from drivers/cpufreq/cpufreq-dt-platdev.c - not all 
processors were booted.

> Anyway, it looks like qcom-cpufreq-nvmem.yaml requiring "cpr" power
> domain unconditionally is a mistake anyway for multiple platforms.
> [2] was recently submitted to fix this so that patch should allow you to
> drop the dummy nodes. 
Stephan Gerhold Jan. 23, 2023, 2 p.m. UTC | #6
On Mon, Jan 23, 2023 at 01:23:22PM +0000, Bryan O'Donoghue wrote:
> On 23/01/2023 12:49, Stephan Gerhold wrote:
> > It's only required though because you forgot to drop the DT schema patch
> > (3/4) when I suggested half a year ago that you make the MSM8939
> > cpufreq-qcom-nvmem changes together with the CPR stack [1]. :/
> 
> Didn't forget, tested that and as I recall there are side-effects removing
> 8939 from drivers/cpufreq/cpufreq-dt-platdev.c - not all processors were
> booted.
> 

The cpufreq-dt-platdev.c addition for MSM8939 does not exist upstream
because you dropped it in your v3 back then. You just kept the DT schema
part. I don't have that addition and have no problems with SMP boot so
I would say it works fine without.

> > Anyway, it looks like qcom-cpufreq-nvmem.yaml requiring "cpr" power
> > domain unconditionally is a mistake anyway for multiple platforms.
> > [2] was recently submitted to fix this so that patch should allow you to
> > drop the dummy nodes. 
Bryan O'Donoghue Jan. 23, 2023, 4:14 p.m. UTC | #7
On 23/01/2023 14:00, Stephan Gerhold wrote:
> Unless this conclusion changes with your CPR patch set this means that
> both the DTS and the DT schema will need changes anyway, because you
> wouldn't need power-domain-names = "cpr", but rather
> 
> 	power-domains = <&rpmpd MSM8939_VDDMX_AO>, <&vreg_dummy>;
> 	power-domain-names = "mx", "cpr";

I have not been owning the CPR for 8939 so far but, this what we have in 
our 4.19 tree.

CPU0: cpu@100 {
         device_type = "cpu";
         compatible = "arm,cortex-a53", "arm,armv8";
         reg = <0x100>;
         next-level-cache = <&L2_1>;
         enable-method = "qcom,kpss-acc-v2";
         qcom,acc = <&acc0>;
         qcom,saw = <&saw0>;
         clocks = <&apcs1>;
         operating-points-v2 = <&cluster1_opp_table>;
         power-domains = <&cpr>;
         power-domain-names = "cpr";
         #cooling-cells = <2>;
         capacity-dmips-mhz = <1024>;
};

cpr: power-controller@b018000 {
         compatible = "qcom,msm8939-cpr", "qcom,cpr";
         reg = <0x0b018000 0x1000>;
         interrupts = <0 15 IRQ_TYPE_EDGE_RISING>;
         clocks = <&rpmcc CXO_SMD_CXO_A_CLK>;
         clock-names = "ref";
         power-domains = <&rpmpd MSM8939_VDDMX_AO>;
         #power-domain-cells = <0>;
         operating-points-v2 = <&cpr_opp_table>;
};

So the CPR code not the CPU code owns VDDMX_AO. I'm not sure if there's 
a good reason why it has been done that way.

Anyway, this feels like a bit of a departure from our core discussion. I 
will see if it is possible to drop the CPU power-domain entirely 
contingent on the patch you flagged.

---
bod
Konrad Dybcio Jan. 23, 2023, 4:21 p.m. UTC | #8
On 23.01.2023 13:49, Stephan Gerhold wrote:
> On Mon, Jan 23, 2023 at 11:08:28AM +0000, Bryan O'Donoghue wrote:
>> V4:
>> - Left _AO for wcnss as downstream reference uses this - Bjorn/Bryan
> 
> Downstream is just an implementation and contains plenty of misleading
> or even wrong information. IMO Bjorn is right here that VDDMX_AO is not
> a logical choice.
> 
> The _AO (active-only) suffix means that the votes are only applied when
> the processor making the vote is "active", that is when the Linux CPUs
> are not in deep cpuidle mode.
> 
> For WCNSS the goal is to keep the necessary power domains active while
> WCNSS is booting up, until it is able to make its own votes (handover).
> The WCNSS firmware might then vote for VDDMX_AO internally because VDDMX
> is not needed when the WCNSS CPU is suspended.
> 
> However, I would expect that the meaning is totally different when the
> same vote is made from Linux. When Linux votes for _AO the "active"
> state likely refers to the Linux CPUs, instead of the WCNSS CPU when
> made from the WCNSS firmware.
> 
> Why does it work in downstream then? I would just assume "side effects":
>   - Something else votes for VDDMX without _AO while WCNSS is booting
>   - The Linux CPUs don't go into deep cpuidle state during startup
>     - In particular, note how downstream often has "lpm_levels.sleep_disabled=1"
>       on the kernel command line. This disables all cpuidle states until
>       late after boot-up when userspace changes this setting. Without
>       cpuidle, VDDMX_AO is identical to VDDMX.
> 
> Please change it to VDDMX (without _AO). It will most likely not make
> any difference
Wouldn't it make wake-on-wifi-with-cpus-off possible?
(obviously given the wlan chip supports it and can ping
the cpu etc etc)

Konrad

 but IMO it is logcially more correct and less
> confusing/misleading. :)
> 
>> - Leaves dummy power-domain reference in cpu defintion as this is a
>>   required property and the dt checker complains - Stephan/Bryan
> 
> It's only required though because you forgot to drop the DT schema patch
> (3/4) when I suggested half a year ago that you make the MSM8939
> cpufreq-qcom-nvmem changes together with the CPR stack [1]. :/
> 
> Anyway, it looks like qcom-cpufreq-nvmem.yaml requiring "cpr" power
> domain unconditionally is a mistake anyway for multiple platforms.
> [2] was recently submitted to fix this so that patch should allow you to
> drop the dummy nodes. :)
> 
> [1]: https://lore.kernel.org/linux-arm-msm/Ysf8VRaXdGg+8Ev3@gerhold.net/
> [2]: https://lore.kernel.org/linux-arm-msm/20230122174548.13758-1-ansuelsmth@gmail.com/
> 
>> - Left MDSS interconnects. I don't see a bug to fix here - Stephan/Bryan
> 
> Fair enough, if you would like to keep it I will likely send a revert
> for the MSM8939 icc_sync_state() though. Because clearly it breaks
> setups without a display and I don't see how one would fix that from the
> device tree.
> 
> Also: The undocumented "register-mem" interconnect is still there. :)
> 
>> - power-domain in MDSS - dropped its not longer required after
>>   commit a6f033938beb ("dt-bindings: msm: dsi-controller-main: Fix
>> power-domain constraint") - Stephan
> 
> Thanks!
> 
>> - Adds gcc dsi1pll and dsi1pllbyte to gcc clock list.
>>   Reviewing the silicon documentation we see dsi0_phy_pll is used to clock
>>   GCC_BYTE1_CFG_RCGR : SRC_SEL
>>   Root Source Select
>>   000 : cxo
>>   001 : dsi0_phy_pll_out_byteclk
>>   010 : GPLL0_OUT_AUX
>>   011 : gnd
>>   100 : gnd
>>   101 : gnd
>>   110 : gnd
>>   111 : reserved - Stephan/Bryan
>>
> 
> I'm confused. Are you not contradicting yourself here? You say that
> dsi0_phy_pll (dsi ZERO) is used to clock GCC_BYTE1_CFG_RCGR. Then why
> do you add dsi1_phy_pll (dsi ONE) to the gcc clock list?
> 
> To me this looks like a confirmation of what downstream does, that both
> DSI byte clocks are actually sourced from the dsi0_phy and the PLL of
> dsi1_phy is not used.
> 
> Thanks,
> Stephan
Bryan O'Donoghue Jan. 23, 2023, 4:29 p.m. UTC | #9
On 23/01/2023 16:21, Konrad Dybcio wrote:
>> Please change it to VDDMX (without _AO). It will most likely not make
>> any difference
> Wouldn't it make wake-on-wifi-with-cpus-off possible?
> (obviously given the wlan chip supports it and can ping
> the cpu etc etc)
> 
> Konrad

WOWLAN is done via SMD not by raising of an interrupt between WCNSS and 
APSS directly and we do hit VDD min with AO in 4.19.

So, so counter-intuitively so long as the SMD interrupt is unmasked in 
suspend - not a specific WCNSS interrupt, we will wake on WLAN.

Its a complete tangent but, the WCNSS firmware has an SMD RPC call 
called "wake-on-wlan" or somesuch which *would* wake the system via 
interrupt but, appears to never have been implemented...

Anyway.

---
bod
Bryan O'Donoghue Jan. 26, 2023, 3:29 p.m. UTC | #10
On 23/01/2023 12:49, Stephan Gerhold wrote:
>> - Adds gcc dsi1pll and dsi1pllbyte to gcc clock list.
>>    Reviewing the silicon documentation we see dsi0_phy_pll is used to clock
>>    GCC_BYTE1_CFG_RCGR : SRC_SEL
>>    Root Source Select
>>    000 : cxo
>>    001 : dsi0_phy_pll_out_byteclk
>>    010 : GPLL0_OUT_AUX
>>    011 : gnd
>>    100 : gnd
>>    101 : gnd
>>    110 : gnd
>>    111 : reserved - Stephan/Bryan
>>
> I'm confused. Are you not contradicting yourself here? You say that
> dsi0_phy_pll (dsi ZERO) is used to clock GCC_BYTE1_CFG_RCGR. Then why
> do you add dsi1_phy_pll (dsi ONE) to the gcc clock list?

So my understanding of the clock tree here is that 
dsi0_phy_pll_out_byteclk is a legacy name.

Its perfectly possible to have DSI0 and DSI0_PHY switched off and to 
have DSI1/DSI1_PHY operable.

dsi0_phy_pll_out_byteclk is perhaps an unfortunate name and probably 
should have been renamed.

> To me this looks like a confirmation of what downstream does, that both
> DSI byte clocks are actually sourced from the dsi0_phy and the PLL of

A better name would have been dsiX_phy_pll_out_byteclk.

---
bod
Konrad Dybcio Jan. 26, 2023, 3:34 p.m. UTC | #11
On 26.01.2023 16:29, Bryan O'Donoghue wrote:
> On 23/01/2023 12:49, Stephan Gerhold wrote:
>>> - Adds gcc dsi1pll and dsi1pllbyte to gcc clock list.
>>>    Reviewing the silicon documentation we see dsi0_phy_pll is used to clock
>>>    GCC_BYTE1_CFG_RCGR : SRC_SEL
>>>    Root Source Select
>>>    000 : cxo
>>>    001 : dsi0_phy_pll_out_byteclk
>>>    010 : GPLL0_OUT_AUX
>>>    011 : gnd
>>>    100 : gnd
>>>    101 : gnd
>>>    110 : gnd
>>>    111 : reserved - Stephan/Bryan
>>>
>> I'm confused. Are you not contradicting yourself here? You say that
>> dsi0_phy_pll (dsi ZERO) is used to clock GCC_BYTE1_CFG_RCGR. Then why
>> do you add dsi1_phy_pll (dsi ONE) to the gcc clock list?
> 
> So my understanding of the clock tree here is that dsi0_phy_pll_out_byteclk is a legacy name.
> 
> Its perfectly possible to have DSI0 and DSI0_PHY switched off and to have DSI1/DSI1_PHY operable.
> 
> dsi0_phy_pll_out_byteclk is perhaps an unfortunate name and probably should have been renamed.
> 
>> To me this looks like a confirmation of what downstream does, that both
>> DSI byte clocks are actually sourced from the dsi0_phy and the PLL of
> 
> A better name would have been dsiX_phy_pll_out_byteclk.
I believe Stephan is just confused what the clock source of both
pairs of GCC DSI clocks are, as you're suggesting that:

phy_clock0
  |_gcc_clock0

and

phy_clock0 (yes, zero)
  |_gcc_clock1

whereas on most other SoCs the following is true:

phy_clock0
  |_gcc_clock0

phy_clock1
  |_gcc_clock_1

Konrad
> 
> ---
> bod
Bryan O'Donoghue Jan. 26, 2023, 4:32 p.m. UTC | #12
On 26/01/2023 15:34, Konrad Dybcio wrote:
>>> To me this looks like a confirmation of what downstream does, that both
>>> DSI byte clocks are actually sourced from the dsi0_phy and the PLL of
>> A better name would have been dsiX_phy_pll_out_byteclk.
> I believe Stephan is just confused what the clock source of both
> pairs of GCC DSI clocks are, as you're suggesting that:
> 
> phy_clock0
>    |_gcc_clock0
> 
> and
> 
> phy_clock0 (yes, zero)
>    |_gcc_clock1
> 
> whereas on most other SoCs the following is true:
> 
> phy_clock0
>    |_gcc_clock0
> 
> phy_clock1
>    |_gcc_clock_1
> 
> Konrad

The only input clock to GCC is XO or buffered CXO if routed through the 
PMIC.

You can select via GCC::RCGR where dsiX_phy_pll_out_byteclk is *sourced* 
from XO, GPLL0_AUX or P_DSI0_PHYPLL_BYTE.

So, obvs the byte clock can be any one of those input sources.

But the question is, if you select dsi0_phy_pll_out_byteclk - what 
provides it ?

Reviewing the LK bootloader for 3.18, it *looks* to me like the dsi0 pll 
is always switched on. The downstream kernel tree doesn't represent that.

0x01A9811C MDSS_DSI_0_CLK_CTRL
Type: RW
Reset State: 0x00000000 -> BIT(4) -> Turns on/off BYTECLK for the DSI. 
If set to 1, clock is ON.

Hmm. I think actually it must be the case that DSI1 is a slave of DSI0.

You can have both interfaces running or just DSI0 on its own.

Hmm, I'll change it.

---
bod
Bryan O'Donoghue Jan. 26, 2023, 4:45 p.m. UTC | #13
On 26/01/2023 16:32, Bryan O'Donoghue wrote:
> The only input clock to GCC is XO or buffered CXO if routed through the 
> PMIC.
> 
> You can select via GCC::RCGR where dsiX_phy_pll_out_byteclk is *sourced* 
> from XO, GPLL0_AUX or P_DSI0_PHYPLL_BYTE.
> 
> So, obvs the byte clock can be any one of those input sources.
> 
> But the question is, if you select dsi0_phy_pll_out_byteclk - what 
> provides it ?
> 
> Reviewing the LK bootloader for 3.18, it *looks* to me like the dsi0 pll 
> is always switched on. The downstream kernel tree doesn't represent that.
> 
> 0x01A9811C MDSS_DSI_0_CLK_CTRL
> Type: RW
> Reset State: 0x00000000 -> BIT(4) -> Turns on/off BYTECLK for the DSI. 
> If set to 1, clock is ON.
> 
> Hmm. I think actually it must be the case that DSI1 is a slave of DSI0.

* If and only if you set P_DSI0_PHYPLL_BYTE::SRC_SEL = 0x01, using 
SRC_SEL = 0 (XO) or SRC_SEL = 0x02 (GPLL0_AUX) should negate the dependency.

I'll review downstream further - perhaps DSI1 in practice doesn't set 
P_DSI0_PHYPLL_BYTE as the source clock..

---
bod