Re: [PATCH v5 13/13] arm64: dts: sc7180: Add qupv3_0 and qupv3_1

[Doug Anderson]

Hi,

On Fri, Nov 8, 2019 at 5:29 PM Rajendra Nayak <rnayak@xxxxxxxxxxxxxx> wrote:
>
> From: Roja Rani Yarubandi <rojay@xxxxxxxxxxxxxx>
>
> Add QUP SE instances configuration for sc7180.
>
> Signed-off-by: Roja Rani Yarubandi <rojay@xxxxxxxxxxxxxx>
> Signed-off-by: Rajendra Nayak <rnayak@xxxxxxxxxxxxxx>
> Reviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> ---
>  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 146 +++++
>  arch/arm64/boot/dts/qcom/sc7180.dtsi    | 675 ++++++++++++++++++++++++
>  2 files changed, 821 insertions(+)

Comments below could be done in a follow-up patch if it makes more sense.


> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index e1d6278d85f7..666e9b92c7ad 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi

At the top of this file, please add aliases for all i2c and spi
devices (like sdm845 did).  Right now trying to use command line i2c
tools is super confusing because busses are super jumbled.


> +                       i2c2: i2c@888000 {
> +                               compatible = "qcom,geni-i2c";
> +                               reg = <0 0x00888000 0 0x4000>;
> +                               clock-names = "se";
> +                               clocks = <&gcc GCC_QUPV3_WRAP0_S2_CLK>;
> +                               pinctrl-names = "default";
> +                               pinctrl-0 = <&qup_i2c2_default>;
> +                               interrupts = <GIC_SPI 603 IRQ_TYPE_LEVEL_HIGH>;
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +                               status = "disabled";
> +                       };

Where is spi2?


> +                       i2c4: i2c@890000 {
> +                               compatible = "qcom,geni-i2c";
> +                               reg = <0 0x00890000 0 0x4000>;
> +                               clock-names = "se";
> +                               clocks = <&gcc GCC_QUPV3_WRAP0_S4_CLK>;
> +                               pinctrl-names = "default";
> +                               pinctrl-0 = <&qup_i2c4_default>;
> +                               interrupts = <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH>;
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +                               status = "disabled";
> +                       };

Where is spi4?


> +                       i2c7: i2c@a84000 {
> +                               compatible = "qcom,geni-i2c";
> +                               reg = <0 0x00a84000 0 0x4000>;
> +                               clock-names = "se";
> +                               clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>;
> +                               pinctrl-names = "default";
> +                               pinctrl-0 = <&qup_i2c7_default>;
> +                               interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>;
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +                               status = "disabled";
> +                       };

Where is spi7?


> +                       i2c9: i2c@a8c000 {
> +                               compatible = "qcom,geni-i2c";
> +                               reg = <0 0x00a8c000 0 0x4000>;
> +                               clock-names = "se";
> +                               clocks = <&gcc GCC_QUPV3_WRAP1_S3_CLK>;
> +                               pinctrl-names = "default";
> +                               pinctrl-0 = <&qup_i2c9_default>;
> +                               interrupts = <GIC_SPI 356 IRQ_TYPE_LEVEL_HIGH>;
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +                               status = "disabled";
> +                       };

Where is spi9?


> +                       qup_spi1_default: qup-spi1-default {
> +                               pinmux {
> +                                       pins = "gpio0", "gpio1",
> +                                              "gpio2", "gpio3",
> +                                              "gpio12", "gpio94";

Please just mux one of the chip selects by default.  It seems like it
would be _much_ more common to have a single SPI device on the bus and
then every board doesn't have to override this.


> +                       qup_spi6_default: qup-spi6-default {
> +                               pinmux {
> +                                       pins = "gpio59", "gpio60",
> +                                              "gpio61", "gpio62",
> +                                              "gpio68", "gpio72";

Please just mux one of the chip selects by default.  It seems like it
would be _much_ more common to have a single SPI device on the bus and
then every board doesn't have to override this.


> +                       qup_spi10_default: qup-spi10-default {
> +                               pinmux {
> +                                       pins = "gpio86", "gpio87",
> +                                              "gpio88", "gpio89",
> +                                              "gpio90", "gpio91";

Please just mux one of the chip selects by default.  It seems like it
would be _much_ more common to have a single SPI device on the bus and
then every board doesn't have to override this.


-Doug