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

[Rajendra Nayak <rnayak@codeaurora.org>]

On 12/6/2019 5:55 PM, Doug Anderson wrote:
> Hi,
> 
> On Fri, Nov 8, 2019 at 5:29 PM Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>
>> From: Roja Rani Yarubandi <rojay@codeaurora.org>
>>
>> Add QUP SE instances configuration for sc7180.
>>
>> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>> ---
>>   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