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

Rajendra Nayak
2019-12-10T10:33:49Z

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.