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

[Rajendra Nayak]

On 12/6/2019 5:55 PM, Doug Anderson wrote:
> 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.

sure, I'll add it.

> > +                       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?

so looks like these qup instances (qup2/4/7/9) can only be configured to be used as i2c or uart
and not spi since we have only 2 pins for them and spi needs 4.
 
> > +                       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.

yes, i will fix all of them to remove the additional chip select muxes.

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation