| Message ID | 20191023090219.15603-1-rnayak@codeaurora.org |
|---|---|
| Headers | show |
| Series | Add device tree support for sc7180 | expand |
On 2019-10-23 14:32, Rajendra Nayak wrote: > From: Maulik Shah <mkshah@codeaurora.org> > > Command_db provides mapping for resource key and address managed > by remote processor. Add cmd_db reserved memory area. > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > --- > arch/arm64/boot/dts/qcom/sc7180.dtsi | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi > b/arch/arm64/boot/dts/qcom/sc7180.dtsi > index f17684148595..dfa49ef2bce0 100644 > --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi > @@ -32,6 +32,18 @@ > }; > }; > > + reserved_memory: reserved-memory { > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + > + cmd_db: reserved-memory@80820000 { aop_cmd_db_mem: memory@80820000 { please use ^^ instead > + reg = <0x0 0x80820000 0x0 0x20000>; > + compatible = "qcom,cmd-db"; > + no-map; > + }; > + }; > + > cpus { > #address-cells = <2>; > #size-cells = <0>;
On 10/23/2019 5:46 PM, Sibi Sankar wrote: > On 2019-10-23 14:32, Rajendra Nayak wrote: >> From: Maulik Shah <mkshah@codeaurora.org> >> >> Command_db provides mapping for resource key and address managed >> by remote processor. Add cmd_db reserved memory area. >> >> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> >> --- >> arch/arm64/boot/dts/qcom/sc7180.dtsi | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi >> b/arch/arm64/boot/dts/qcom/sc7180.dtsi >> index f17684148595..dfa49ef2bce0 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi >> @@ -32,6 +32,18 @@ >> }; >> }; >> >> + reserved_memory: reserved-memory { >> + #address-cells = <2>; >> + #size-cells = <2>; >> + ranges; >> + >> + cmd_db: reserved-memory@80820000 { > > aop_cmd_db_mem: memory@80820000 { > please use ^^ instead right, I thought I looked up sm8150.dtsi to make sure the labeling for various things is consistent, but maybe i didn't. Will fix. Thanks. > >> + reg = <0x0 0x80820000 0x0 0x20000>; >> + compatible = "qcom,cmd-db"; >> + no-map; >> + }; >> + }; >> + >> cpus { >> #address-cells = <2>; >> #size-cells = <0>; >
Hi Rajendra/Maulik, On Wed, Oct 23, 2019 at 02:32:19PM +0530, Rajendra Nayak wrote: > From: Maulik Shah <mkshah@codeaurora.org> > > Add pdc interrupt controller for sc7180 > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > --- > v3: > Used the qcom,sdm845-pdc compatible for pdc node > > arch/arm64/boot/dts/qcom/sc7180.dtsi | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi > index f2981ada578f..07ea393c2b5f 100644 > --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi > @@ -184,6 +184,16 @@ > #power-domain-cells = <1>; > }; > > + pdc: interrupt-controller@b220000 { Aren't the nodes supposed to be ordered by address as for SDM845? If so this node should be added after 'qupv3_id_1: geniqup@ac0000', not before.
Quoting Rajendra Nayak (2019-10-23 02:02:16) > diff --git a/arch/arm64/boot/dts/qcom/pm6150.dtsi b/arch/arm64/boot/dts/qcom/pm6150.dtsi > new file mode 100644 > index 000000000000..20eb928e5ce3 > --- /dev/null > +++ b/arch/arm64/boot/dts/qcom/pm6150.dtsi > @@ -0,0 +1,85 @@ > +// SPDX-License-Identifier: BSD-3-Clause > +// Copyright (c) 2019, The Linux Foundation. All rights reserved. > + > +#include <dt-bindings/iio/qcom,spmi-vadc.h> > +#include <dt-bindings/input/linux-event-codes.h> > +#include <dt-bindings/interrupt-controller/irq.h> > +#include <dt-bindings/spmi/spmi.h> > +#include <dt-bindings/thermal/thermal.h> > + > +&spmi_bus { > + pm6150_lsid0: pmic@0 { > + compatible = "qcom,pm6150", "qcom,spmi-pmic"; > + reg = <0x0 SPMI_USID>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + pm6150_pon: pon@800 { > + compatible = "qcom,pm8998-pon"; > + reg = <0x800>; > + mode-bootloader = <0x2>; > + mode-recovery = <0x1>; Can this have status = "disabled"? Or is the idea that if the pmic power button isn't used it should be disabled in the board dts file? > + > + pwrkey { > + compatible = "qcom,pm8941-pwrkey"; > + interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>; > + debounce = <15625>; > + bias-pull-up; > + linux,code = <KEY_POWER>; > + }; > + }; > + > + pm6150_temp: temp-alarm@2400 { > + compatible = "qcom,spmi-temp-alarm"; > + reg = <0x2400>; > + interrupts = <0x0 0x24 0x0 IRQ_TYPE_EDGE_RISING>; > + io-channels = <&pm6150_adc ADC5_DIE_TEMP>; > + io-channel-names = "thermal"; > + #thermal-sensor-cells = <0>; > + }; > + > + pm6150_adc: adc@3100 { > + compatible = "qcom,spmi-adc5"; > + reg = <0x3100>; > + interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>; > + #address-cells = <1>; > + #size-cells = <0>; > + #io-channel-cells = <1>; > + > + adc-chan@ADC5_DIE_TEMP { > + reg = <ADC5_DIE_TEMP>; > + label = "die_temp"; > + }; > + }; > + > + pm6150_gpio: gpios@c000 { > + compatible = "qcom,pm6150-gpio", "qcom,spmi-gpio"; > + reg = <0xc000 0xa00>; Drop the size? > + gpio-controller; > + #gpio-cells = <2>; > + interrupts = <0 0xc0 0 IRQ_TYPE_NONE>, > + <0 0xc1 0 IRQ_TYPE_NONE>, > + <0 0xc2 0 IRQ_TYPE_NONE>, > + <0 0xc3 0 IRQ_TYPE_NONE>, > + <0 0xc4 0 IRQ_TYPE_NONE>, > + <0 0xc5 0 IRQ_TYPE_NONE>, > + <0 0xc6 0 IRQ_TYPE_NONE>, > + <0 0xc7 0 IRQ_TYPE_NONE>, > + <0 0xc8 0 IRQ_TYPE_NONE>, > + <0 0xc9 0 IRQ_TYPE_NONE>; Isn't this supposed to go away? > + > + interrupt-names = "pm6150_gpio1", "pm6150_gpio2", > + "pm6150_gpio3", "pm6150_gpio4", > + "pm6150_gpio5", "pm6150_gpio6", > + "pm6150_gpio7", "pm6150_gpio8", > + "pm6150_gpio9", "pm6150_gpio10"; And this? And have gpio-ranges and use the irqdomain work. Basically, should look like pm8998. > + }; > + }; > + > + pm6150_lsid1: pmic@1 { > + compatible = "qcom,pm6150", "qcom,spmi-pmic"; > + reg = <0x1 SPMI_USID>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > +};
Quoting Rajendra Nayak (2019-10-23 02:02:15) > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi > index 04808a07d7da..6584ac6e6c7b 100644 > --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi > @@ -224,6 +224,25 @@ > }; > }; > > + spmi_bus: spmi@c440000 { > + compatible = "qcom,spmi-pmic-arb"; > + reg = <0 0xc440000 0 0x1100>, Please pad out the registers to 8 numbers. See sdm845. > + <0 0xc600000 0 0x2000000>, > + <0 0xe600000 0 0x100000>, > + <0 0xe700000 0 0xa0000>, > + <0 0xc40a000 0 0x26000>; > + reg-names = "core", "chnls", "obsrvr", "intr", "cnfg"; > + interrupt-names = "periph_irq"; > + interrupts-extended = <&pdc 1 IRQ_TYPE_LEVEL_HIGH>; This is different than sdm845. I guess pdc is working? > + qcom,ee = <0>; > + qcom,channel = <0>; > + #address-cells = <1>; > + #size-cells = <1>; > + interrupt-controller; > + #interrupt-cells = <4>; > + cell-index = <0>; > + }; > +
Quoting Rajendra Nayak (2019-10-23 02:02:10) > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi > new file mode 100644 > index 000000000000..084854341ddd > --- /dev/null > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi > @@ -0,0 +1,300 @@ > +// SPDX-License-Identifier: BSD-3-Clause > +/* > + * SC7180 SoC device tree source > + * > + * Copyright (c) 2019, The Linux Foundation. All rights reserved. > + */ > + > +#include <dt-bindings/clock/qcom,gcc-sc7180.h> > +#include <dt-bindings/interrupt-controller/arm-gic.h> > + > +/ { > + interrupt-parent = <&intc>; > + > + #address-cells = <2>; > + #size-cells = <2>; > + > + chosen { }; > + > + clocks { > + xo_board: xo-board { > + compatible = "fixed-clock"; > + clock-frequency = <38400000>; > + clock-output-names = "xo_board"; Can you drop the output names property? I think we don't care that the name is "xo-board" instead of "xo_board" now. > + #clock-cells = <0>; > + }; > + > + sleep_clk: sleep-clk { > + compatible = "fixed-clock"; > + clock-frequency = <32764>; > + clock-output-names = "sleep_clk"; > + #clock-cells = <0>; > + }; > + }; > + [...] > + > + soc: soc { > + #address-cells = <2>; > + #size-cells = <2>; > + ranges = <0 0 0 0 0x10 0>; > + dma-ranges = <0 0 0 0 0x10 0>; Why the extra space here ^ ? > + compatible = "simple-bus"; > + > + gcc: clock-controller@100000 { > + compatible = "qcom,gcc-sc7180"; > + reg = <0 0x00100000 0 0x1f0000>; > + #clock-cells = <1>; > + #reset-cells = <1>; > + #power-domain-cells = <1>; > + }; > + > + qupv3_id_1: geniqup@ac0000 { > + compatible = "qcom,geni-se-qup"; > + reg = <0 0x00ac0000 0 0x6000>; > + clock-names = "m-ahb", "s-ahb"; > + clocks = <&gcc GCC_QUPV3_WRAP_1_M_AHB_CLK>, > + <&gcc GCC_QUPV3_WRAP_1_S_AHB_CLK>; > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + status = "disabled"; > + > + uart10: serial@a88000 { > + compatible = "qcom,geni-debug-uart"; > + reg = <0 0x00a88000 0 0x4000>; > + clock-names = "se"; > + clocks = <&gcc GCC_QUPV3_WRAP1_S2_CLK>; > + pinctrl-names = "default"; > + pinctrl-0 = <&qup_uart10_default>; > + interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>; > + status = "disabled"; > + }; Can we not add all the i2c/spi/uart cores here? > + };
Quoting Rajendra Nayak (2019-10-23 02:02:19) > From: Maulik Shah <mkshah@codeaurora.org> > > Add pdc interrupt controller for sc7180 > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > --- > v3: > Used the qcom,sdm845-pdc compatible for pdc node Everything else isn't doing the weird old compatible thing. Why not just add the new compatible and update the driver? I guess I'll have to go read the history.
On 2019-10-29 22:11, Stephen Boyd wrote: > Quoting Rajendra Nayak (2019-10-23 02:02:15) >> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi >> b/arch/arm64/boot/dts/qcom/sc7180.dtsi >> index 04808a07d7da..6584ac6e6c7b 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi >> @@ -224,6 +224,25 @@ >> }; >> }; >> >> + spmi_bus: spmi@c440000 { >> + compatible = "qcom,spmi-pmic-arb"; >> + reg = <0 0xc440000 0 0x1100>, > > Please pad out the registers to 8 numbers. See sdm845. Ok.. Will address it in the next series. > >> + <0 0xc600000 0 0x2000000>, >> + <0 0xe600000 0 0x100000>, >> + <0 0xe700000 0 0xa0000>, >> + <0 0xc40a000 0 0x26000>; >> + reg-names = "core", "chnls", "obsrvr", "intr", >> "cnfg"; >> + interrupt-names = "periph_irq"; >> + interrupts-extended = <&pdc 1 >> IRQ_TYPE_LEVEL_HIGH>; > > This is different than sdm845. I guess pdc is working? > Yes. For SDM845 pdc controller support was not yet added. That's why still the GIC interrupt is used. Where as for SC7180 the same is added with https://lore.kernel.org/patchwork/patch/1143335/. Yes. pdc is working. >> + qcom,ee = <0>; >> + qcom,channel = <0>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + interrupt-controller; >> + #interrupt-cells = <4>; >> + cell-index = <0>; >> + }; >> +
On 2019-10-29 22:08, Stephen Boyd wrote: > Quoting Rajendra Nayak (2019-10-23 02:02:16) >> diff --git a/arch/arm64/boot/dts/qcom/pm6150.dtsi >> b/arch/arm64/boot/dts/qcom/pm6150.dtsi >> new file mode 100644 >> index 000000000000..20eb928e5ce3 >> --- /dev/null >> +++ b/arch/arm64/boot/dts/qcom/pm6150.dtsi >> @@ -0,0 +1,85 @@ >> +// SPDX-License-Identifier: BSD-3-Clause >> +// Copyright (c) 2019, The Linux Foundation. All rights reserved. >> + >> +#include <dt-bindings/iio/qcom,spmi-vadc.h> >> +#include <dt-bindings/input/linux-event-codes.h> >> +#include <dt-bindings/interrupt-controller/irq.h> >> +#include <dt-bindings/spmi/spmi.h> >> +#include <dt-bindings/thermal/thermal.h> >> + >> +&spmi_bus { >> + pm6150_lsid0: pmic@0 { >> + compatible = "qcom,pm6150", "qcom,spmi-pmic"; >> + reg = <0x0 SPMI_USID>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + pm6150_pon: pon@800 { >> + compatible = "qcom,pm8998-pon"; >> + reg = <0x800>; >> + mode-bootloader = <0x2>; >> + mode-recovery = <0x1>; > > Can this have status = "disabled"? Or is the idea that if the pmic > power > button isn't used it should be disabled in the board dts file? > Yes. The idea is to go with latter option. Disable it in the board dts file if the pmic power button is not used. >> + >> + pwrkey { >> + compatible = "qcom,pm8941-pwrkey"; >> + interrupts = <0x0 0x8 0 >> IRQ_TYPE_EDGE_BOTH>; >> + debounce = <15625>; >> + bias-pull-up; >> + linux,code = <KEY_POWER>; >> + }; >> + }; >> + >> + pm6150_temp: temp-alarm@2400 { >> + compatible = "qcom,spmi-temp-alarm"; >> + reg = <0x2400>; >> + interrupts = <0x0 0x24 0x0 >> IRQ_TYPE_EDGE_RISING>; >> + io-channels = <&pm6150_adc ADC5_DIE_TEMP>; >> + io-channel-names = "thermal"; >> + #thermal-sensor-cells = <0>; >> + }; >> + >> + pm6150_adc: adc@3100 { >> + compatible = "qcom,spmi-adc5"; >> + reg = <0x3100>; >> + interrupts = <0x0 0x31 0x0 >> IRQ_TYPE_EDGE_RISING>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + #io-channel-cells = <1>; >> + >> + adc-chan@ADC5_DIE_TEMP { >> + reg = <ADC5_DIE_TEMP>; >> + label = "die_temp"; >> + }; >> + }; >> + >> + pm6150_gpio: gpios@c000 { >> + compatible = "qcom,pm6150-gpio", >> "qcom,spmi-gpio"; >> + reg = <0xc000 0xa00>; > > Drop the size? > Will drop it in next series. >> + gpio-controller; >> + #gpio-cells = <2>; >> + interrupts = <0 0xc0 0 IRQ_TYPE_NONE>, >> + <0 0xc1 0 IRQ_TYPE_NONE>, >> + <0 0xc2 0 IRQ_TYPE_NONE>, >> + <0 0xc3 0 IRQ_TYPE_NONE>, >> + <0 0xc4 0 IRQ_TYPE_NONE>, >> + <0 0xc5 0 IRQ_TYPE_NONE>, >> + <0 0xc6 0 IRQ_TYPE_NONE>, >> + <0 0xc7 0 IRQ_TYPE_NONE>, >> + <0 0xc8 0 IRQ_TYPE_NONE>, >> + <0 0xc9 0 IRQ_TYPE_NONE>; > > Isn't this supposed to go away? > Yes. We can remove them if we want to go with the way done for pm8998. >> + >> + interrupt-names = "pm6150_gpio1", >> "pm6150_gpio2", >> + "pm6150_gpio3", >> "pm6150_gpio4", >> + "pm6150_gpio5", >> "pm6150_gpio6", >> + "pm6150_gpio7", >> "pm6150_gpio8", >> + "pm6150_gpio9", >> "pm6150_gpio10"; > > And this? And have gpio-ranges and use the irqdomain work. Basically, > should look like pm8998. Ok.. We can go ahead with the pm8998 way as well. We will address it in next series. > >> + }; >> + }; >> + >> + pm6150_lsid1: pmic@1 { >> + compatible = "qcom,pm6150", "qcom,spmi-pmic"; >> + reg = <0x1 SPMI_USID>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + }; >> +};
Quoting kgunda@codeaurora.org (2019-10-29 23:06:43) > On 2019-10-29 22:11, Stephen Boyd wrote: > > Quoting Rajendra Nayak (2019-10-23 02:02:15) > >> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi > >> b/arch/arm64/boot/dts/qcom/sc7180.dtsi > >> index 04808a07d7da..6584ac6e6c7b 100644 > >> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi > >> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi > >> @@ -224,6 +224,25 @@ > >> }; > >> }; > >> > >> + spmi_bus: spmi@c440000 { > >> + compatible = "qcom,spmi-pmic-arb"; > >> + reg = <0 0xc440000 0 0x1100>, > > > > Please pad out the registers to 8 numbers. See sdm845. > Ok.. Will address it in the next series. > > > >> + <0 0xc600000 0 0x2000000>, > >> + <0 0xe600000 0 0x100000>, > >> + <0 0xe700000 0 0xa0000>, > >> + <0 0xc40a000 0 0x26000>; > >> + reg-names = "core", "chnls", "obsrvr", "intr", > >> "cnfg"; > >> + interrupt-names = "periph_irq"; > >> + interrupts-extended = <&pdc 1 > >> IRQ_TYPE_LEVEL_HIGH>; > > > > This is different than sdm845. I guess pdc is working? > > > Yes. For SDM845 pdc controller support was not yet added. That's why > still the GIC interrupt is used. > Where as for SC7180 the same is added with > https://lore.kernel.org/patchwork/patch/1143335/. > > Yes. pdc is working. Cool. The patch that adds pdc to the DT should come before this one then. In reality, it would be better if it was all squashed down into one big commit that just introduces the SoC file and one commit for PMICs and then one commit for the idp board. Then we don't have this ordering problem.
Quoting kgunda@codeaurora.org (2019-10-30 00:06:05) > On 2019-10-29 22:08, Stephen Boyd wrote: > > Quoting Rajendra Nayak (2019-10-23 02:02:16) > >> diff --git a/arch/arm64/boot/dts/qcom/pm6150.dtsi > >> b/arch/arm64/boot/dts/qcom/pm6150.dtsi > >> new file mode 100644 > >> index 000000000000..20eb928e5ce3 > >> --- /dev/null > >> +++ b/arch/arm64/boot/dts/qcom/pm6150.dtsi > >> @@ -0,0 +1,85 @@ > >> +// SPDX-License-Identifier: BSD-3-Clause > >> +// Copyright (c) 2019, The Linux Foundation. All rights reserved. > >> + > >> +#include <dt-bindings/iio/qcom,spmi-vadc.h> > >> +#include <dt-bindings/input/linux-event-codes.h> > >> +#include <dt-bindings/interrupt-controller/irq.h> > >> +#include <dt-bindings/spmi/spmi.h> > >> +#include <dt-bindings/thermal/thermal.h> > >> + > >> +&spmi_bus { > >> + pm6150_lsid0: pmic@0 { > >> + compatible = "qcom,pm6150", "qcom,spmi-pmic"; > >> + reg = <0x0 SPMI_USID>; > >> + #address-cells = <1>; > >> + #size-cells = <0>; > >> + > >> + pm6150_pon: pon@800 { > >> + compatible = "qcom,pm8998-pon"; > >> + reg = <0x800>; > >> + mode-bootloader = <0x2>; > >> + mode-recovery = <0x1>; > > > > Can this have status = "disabled"? Or is the idea that if the pmic > > power > > button isn't used it should be disabled in the board dts file? > > > Yes. The idea is to go with latter option. Disable it in the board dts > file if the > pmic power button is not used. Ok. Thanks. > >> + > >> + interrupt-names = "pm6150_gpio1", > >> "pm6150_gpio2", > >> + "pm6150_gpio3", > >> "pm6150_gpio4", > >> + "pm6150_gpio5", > >> "pm6150_gpio6", > >> + "pm6150_gpio7", > >> "pm6150_gpio8", > >> + "pm6150_gpio9", > >> "pm6150_gpio10"; > > > > And this? And have gpio-ranges and use the irqdomain work. Basically, > > should look like pm8998. > Ok.. We can go ahead with the pm8998 way as well. We will address it in > next series. Yes please use the pm8998 way..
On Tue, Oct 29, 2019 at 09:50:40AM -0700, Stephen Boyd wrote: > Quoting Rajendra Nayak (2019-10-23 02:02:19) > > From: Maulik Shah <mkshah@codeaurora.org> > > > > Add pdc interrupt controller for sc7180 > > > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > > --- > > v3: > > Used the qcom,sdm845-pdc compatible for pdc node > > Everything else isn't doing the weird old compatible thing. Why not just > add the new compatible and update the driver? I guess I'll have to go > read the history. Marc Zyngier complained on v2 about the churn from adding compatible strings for identical components, and I kinda see his point. I agree that using the 'sdm845' compatible string for sc7180 is odd too. Maybe we should introduce SoC independent compatible strings for IP blocks that are shared across multiple SoCs? If differentiation is needed SoC specific strings can be added.
On 10/26/2019 1:17 AM, Matthias Kaehlcke wrote: > Hi Rajendra/Maulik, > > On Wed, Oct 23, 2019 at 02:32:19PM +0530, Rajendra Nayak wrote: >> From: Maulik Shah <mkshah@codeaurora.org> >> >> Add pdc interrupt controller for sc7180 >> >> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> >> --- >> v3: >> Used the qcom,sdm845-pdc compatible for pdc node >> >> arch/arm64/boot/dts/qcom/sc7180.dtsi | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi >> index f2981ada578f..07ea393c2b5f 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi >> @@ -184,6 +184,16 @@ >> #power-domain-cells = <1>; >> }; >> >> + pdc: interrupt-controller@b220000 { > > Aren't the nodes supposed to be ordered by address as for SDM845? > If so this node should be added after 'qupv3_id_1: geniqup@ac0000', > not before. yes, indeed. my sorting seems to have gone wrong. Will fix and repost. thanks >
On 10/30/2019 8:07 PM, Stephen Boyd wrote: > Quoting kgunda@codeaurora.org (2019-10-29 23:06:43) >> On 2019-10-29 22:11, Stephen Boyd wrote: >>> Quoting Rajendra Nayak (2019-10-23 02:02:15) >>>> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi >>>> b/arch/arm64/boot/dts/qcom/sc7180.dtsi >>>> index 04808a07d7da..6584ac6e6c7b 100644 >>>> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi >>>> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi >>>> @@ -224,6 +224,25 @@ >>>> }; >>>> }; >>>> >>>> + spmi_bus: spmi@c440000 { >>>> + compatible = "qcom,spmi-pmic-arb"; >>>> + reg = <0 0xc440000 0 0x1100>, >>> >>> Please pad out the registers to 8 numbers. See sdm845. >> Ok.. Will address it in the next series. >>> >>>> + <0 0xc600000 0 0x2000000>, >>>> + <0 0xe600000 0 0x100000>, >>>> + <0 0xe700000 0 0xa0000>, >>>> + <0 0xc40a000 0 0x26000>; >>>> + reg-names = "core", "chnls", "obsrvr", "intr", >>>> "cnfg"; >>>> + interrupt-names = "periph_irq"; >>>> + interrupts-extended = <&pdc 1 >>>> IRQ_TYPE_LEVEL_HIGH>; >>> >>> This is different than sdm845. I guess pdc is working? >>> >> Yes. For SDM845 pdc controller support was not yet added. That's why >> still the GIC interrupt is used. >> Where as for SC7180 the same is added with >> https://lore.kernel.org/patchwork/patch/1143335/. >> >> Yes. pdc is working. > > Cool. The patch that adds pdc to the DT should come before this one > then. In reality, it would be better if it was all squashed down into > one big commit that just introduces the SoC file and one commit for > PMICs and then one commit for the idp board. Then we don't have this > ordering problem. I'll fix the ordering issues when I respin. I could squash all of the patches touching the SoC dtsi, but given the authorship for each varies, I will keep it as is perhaps.
On 10/29/2019 10:19 PM, Stephen Boyd wrote: > Quoting Rajendra Nayak (2019-10-23 02:02:10) >> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi >> new file mode 100644 >> index 000000000000..084854341ddd >> --- /dev/null >> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi >> @@ -0,0 +1,300 @@ >> +// SPDX-License-Identifier: BSD-3-Clause >> +/* >> + * SC7180 SoC device tree source >> + * >> + * Copyright (c) 2019, The Linux Foundation. All rights reserved. >> + */ >> + >> +#include <dt-bindings/clock/qcom,gcc-sc7180.h> >> +#include <dt-bindings/interrupt-controller/arm-gic.h> >> + >> +/ { >> + interrupt-parent = <&intc>; >> + >> + #address-cells = <2>; >> + #size-cells = <2>; >> + >> + chosen { }; >> + >> + clocks { >> + xo_board: xo-board { >> + compatible = "fixed-clock"; >> + clock-frequency = <38400000>; >> + clock-output-names = "xo_board"; > > Can you drop the output names property? I think we don't care that the > name is "xo-board" instead of "xo_board" now. sure, will do. > >> + #clock-cells = <0>; >> + }; >> + >> + sleep_clk: sleep-clk { >> + compatible = "fixed-clock"; >> + clock-frequency = <32764>; >> + clock-output-names = "sleep_clk"; >> + #clock-cells = <0>; >> + }; >> + }; >> + > [...] >> + >> + soc: soc { >> + #address-cells = <2>; >> + #size-cells = <2>; >> + ranges = <0 0 0 0 0x10 0>; >> + dma-ranges = <0 0 0 0 0x10 0>; > > Why the extra space here ^ ? typo, will fix. > >> + compatible = "simple-bus"; >> + >> + gcc: clock-controller@100000 { >> + compatible = "qcom,gcc-sc7180"; >> + reg = <0 0x00100000 0 0x1f0000>; >> + #clock-cells = <1>; >> + #reset-cells = <1>; >> + #power-domain-cells = <1>; >> + }; >> + >> + qupv3_id_1: geniqup@ac0000 { >> + compatible = "qcom,geni-se-qup"; >> + reg = <0 0x00ac0000 0 0x6000>; >> + clock-names = "m-ahb", "s-ahb"; >> + clocks = <&gcc GCC_QUPV3_WRAP_1_M_AHB_CLK>, >> + <&gcc GCC_QUPV3_WRAP_1_S_AHB_CLK>; >> + #address-cells = <2>; >> + #size-cells = <2>; >> + ranges; >> + status = "disabled"; >> + >> + uart10: serial@a88000 { >> + compatible = "qcom,geni-debug-uart"; >> + reg = <0 0x00a88000 0 0x4000>; >> + clock-names = "se"; >> + clocks = <&gcc GCC_QUPV3_WRAP1_S2_CLK>; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&qup_uart10_default>; >> + interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>; >> + status = "disabled"; >> + }; > > Can we not add all the i2c/spi/uart cores here? I see that these nodes are posted now [1]. Will pull it in as part of this series so it can be reviewed together. [1] https://lkml.org/lkml/2019/10/31/63
On 10/31/2019 1:20 AM, Matthias Kaehlcke wrote: > On Tue, Oct 29, 2019 at 09:50:40AM -0700, Stephen Boyd wrote: >> Quoting Rajendra Nayak (2019-10-23 02:02:19) >>> From: Maulik Shah <mkshah@codeaurora.org> >>> >>> Add pdc interrupt controller for sc7180 >>> >>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> >>> --- >>> v3: >>> Used the qcom,sdm845-pdc compatible for pdc node >> >> Everything else isn't doing the weird old compatible thing. Why not just >> add the new compatible and update the driver? I guess I'll have to go >> read the history. > > Marc Zyngier complained on v2 about the churn from adding compatible > strings for identical components, and I kinda see his point. > > I agree that using the 'sdm845' compatible string for sc7180 is odd too. > Maybe we should introduce SoC independent compatible strings for IP blocks > that are shared across multiple SoCs? If differentiation is needed SoC > specific strings can be added. Sure, I will perhaps add a qcom,pdc SoC independent compatible to avoid confusion.
On Sun 03 Nov 22:17 PST 2019, Rajendra Nayak wrote: > > > On 10/31/2019 1:20 AM, Matthias Kaehlcke wrote: > > On Tue, Oct 29, 2019 at 09:50:40AM -0700, Stephen Boyd wrote: > > > Quoting Rajendra Nayak (2019-10-23 02:02:19) > > > > From: Maulik Shah <mkshah@codeaurora.org> > > > > > > > > Add pdc interrupt controller for sc7180 > > > > > > > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > > > > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > > > > --- > > > > v3: > > > > Used the qcom,sdm845-pdc compatible for pdc node > > > > > > Everything else isn't doing the weird old compatible thing. Why not just > > > add the new compatible and update the driver? I guess I'll have to go > > > read the history. > > > > Marc Zyngier complained on v2 about the churn from adding compatible > > strings for identical components, and I kinda see his point. > > > > I agree that using the 'sdm845' compatible string for sc7180 is odd too. > > Maybe we should introduce SoC independent compatible strings for IP blocks > > that are shared across multiple SoCs? If differentiation is needed SoC > > specific strings can be added. > > Sure, I will perhaps add a qcom,pdc SoC independent compatible to avoid > confusion. > I agree, compatible = "qcom,sc7180-pdc", "qcom,pdc"; is the way to go. Reusing qcom,sdm845-pdc would prevent us from tackling any unforeseen issues/variations/erratas with one or the other platform in the future. Regards, Bjorn > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation
On 11/4/2019 12:03 PM, Bjorn Andersson wrote: > On Sun 03 Nov 22:17 PST 2019, Rajendra Nayak wrote: > >> >> >> On 10/31/2019 1:20 AM, Matthias Kaehlcke wrote: >>> On Tue, Oct 29, 2019 at 09:50:40AM -0700, Stephen Boyd wrote: >>>> Quoting Rajendra Nayak (2019-10-23 02:02:19) >>>>> From: Maulik Shah <mkshah@codeaurora.org> >>>>> >>>>> Add pdc interrupt controller for sc7180 >>>>> >>>>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >>>>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> >>>>> --- >>>>> v3: >>>>> Used the qcom,sdm845-pdc compatible for pdc node >>>> >>>> Everything else isn't doing the weird old compatible thing. Why not just >>>> add the new compatible and update the driver? I guess I'll have to go >>>> read the history. >>> >>> Marc Zyngier complained on v2 about the churn from adding compatible >>> strings for identical components, and I kinda see his point. >>> >>> I agree that using the 'sdm845' compatible string for sc7180 is odd too. >>> Maybe we should introduce SoC independent compatible strings for IP blocks >>> that are shared across multiple SoCs? If differentiation is needed SoC >>> specific strings can be added. >> >> Sure, I will perhaps add a qcom,pdc SoC independent compatible to avoid >> confusion. >> > > I agree, > > compatible = "qcom,sc7180-pdc", "qcom,pdc"; > > is the way to go. I wasn't planning on adding a qcom,sc7180-pdc, but instead just use the qcom,pdc one for sc7180. > > Reusing qcom,sdm845-pdc would prevent us from tackling any unforeseen > issues/variations/erratas with one or the other platform in the future. That was the intention of adding qcom,sc7180-pdc in the first place, but Marc Zyngier was not happy with the churn, given there aren't really any variations or erratas that we know of. > > Regards, > Bjorn > >> >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member >> of Code Aurora Forum, hosted by The Linux Foundation
On Sun 03 Nov 22:56 PST 2019, Rajendra Nayak wrote: > > > On 11/4/2019 12:03 PM, Bjorn Andersson wrote: > > On Sun 03 Nov 22:17 PST 2019, Rajendra Nayak wrote: > > > > > > > > > > > On 10/31/2019 1:20 AM, Matthias Kaehlcke wrote: > > > > On Tue, Oct 29, 2019 at 09:50:40AM -0700, Stephen Boyd wrote: > > > > > Quoting Rajendra Nayak (2019-10-23 02:02:19) > > > > > > From: Maulik Shah <mkshah@codeaurora.org> > > > > > > > > > > > > Add pdc interrupt controller for sc7180 > > > > > > > > > > > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > > > > > > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > > > > > > --- > > > > > > v3: > > > > > > Used the qcom,sdm845-pdc compatible for pdc node > > > > > > > > > > Everything else isn't doing the weird old compatible thing. Why not just > > > > > add the new compatible and update the driver? I guess I'll have to go > > > > > read the history. > > > > > > > > Marc Zyngier complained on v2 about the churn from adding compatible > > > > strings for identical components, and I kinda see his point. > > > > > > > > I agree that using the 'sdm845' compatible string for sc7180 is odd too. > > > > Maybe we should introduce SoC independent compatible strings for IP blocks > > > > that are shared across multiple SoCs? If differentiation is needed SoC > > > > specific strings can be added. > > > > > > Sure, I will perhaps add a qcom,pdc SoC independent compatible to avoid > > > confusion. > > > > > > > I agree, > > > > compatible = "qcom,sc7180-pdc", "qcom,pdc"; > > > > is the way to go. > > I wasn't planning on adding a qcom,sc7180-pdc, but instead just use the > qcom,pdc one for sc7180. > > > > > Reusing qcom,sdm845-pdc would prevent us from tackling any unforeseen > > issues/variations/erratas with one or the other platform in the future. > > That was the intention of adding qcom,sc7180-pdc in the first place, > but Marc Zyngier was not happy with the churn, given there aren't really > any variations or erratas that we know of. > Right, but by putting both compatibles in the dts and the generic one in the driver we avoid the driver churn and we're future compatible. And given that we haven't yet added the qcom,sdm845-pdc node to the sdm845.dtsi we don't need to maintain the qcom,sdm845-pdc in the driver. So switch qcom,sdm845-pdc to qcom,pdc in qcom-pdc.c. Regards, Bjorn > > > > Regards, > > Bjorn > > > > > > > > -- > > > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > > > of Code Aurora Forum, hosted by The Linux Foundation > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation
Quoting Bjorn Andersson (2019-11-03 23:10:27) > > Right, but by putting both compatibles in the dts and the generic one in > the driver we avoid the driver churn and we're future compatible. > > And given that we haven't yet added the qcom,sdm845-pdc node to the > sdm845.dtsi we don't need to maintain the qcom,sdm845-pdc in the driver. > So switch qcom,sdm845-pdc to qcom,pdc in qcom-pdc.c. > I like this plan!