Re: [PATCH v2 02/13] arm64: dts: sc7180: Add minimal dts/dtsi files for SC7180 soc

[Rajendra Nayak <rnayak@codeaurora.org>]

Hi Matthias, thanks for the review

On 10/22/2019 5:38 AM, Matthias Kaehlcke wrote:
> Hi Rajendra,
> 
> I don't have all the hardware documentation for a full review, but
> find a few comments inline.
> 
[]..

>> +#include "sc7180.dtsi"
>> +
>> +/ {
>> +	model = "Qualcomm Technologies, Inc. SC7180 IDP";
>> +	compatible = "qcom,sc7180-idp";
>> +
>> +	aliases {
>> +		serial0 = &uart2;
>> +	};
>> +
>> +	chosen {
>> +		stdout-path = "serial0:115200n8";
>> +	};
>> +};
>> +
>> +&qupv3_id_0 {
>> +	status = "okay";
>> +};
>> +
>> +&uart2 {
>> +	status = "okay";
>> +};
>> +
>> +/* PINCTRL - additions to nodes defined in sc7180.dtsi */
>> +
>> +&qup_uart2_default {
>> +	pinconf-tx {
>> +		pins = "gpio44";
>> +		drive-strength = <2>;
>> +		bias-disable;
>> +	};
>> +
>> +	pinconf-rx {
>> +		pins = "gpio45";
>> +		drive-strength = <2>;
>> +		bias-pull-up;
>> +	};
>> +};
> 
> This config seems reasonable as default for a UART in general.
> Would it make sense to configure these in the SoC .dtsi?

I think the general rule of thumb that was followed was to have
all pinmux configurations in soc file and all pinconf setting in
the board, even though it meant a bit of duplication in some cases.
See [1] for some discussions around it that happened in the past.

[1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1603693.html

> 
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> new file mode 100644
>> index 000000000000..82bf7cdce6b8
>> --- /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>
> 
> Note: depends on "Add Global Clock controller (GCC) driver for SC7180"
> (https://patchwork.kernel.org/project/linux-arm-msm/list/?submitter=179717)
> which isn't merged yet.

Right, I did mention it in the cover letter, perhaps I should have mentioned it
as part of this patch as well.

[]..
>> +
>> +	soc: soc {
>> +		#address-cells = <2>;
>> +		#size-cells = <2>;
>> +		ranges = <0 0 0 0 0x10 0>;
>> +		dma-ranges = <0 0 0 0  0x10 0>;
>> +		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_0: geniqup@ac0000 {
> 
> The QUP enumeration is a bit confusing. The Hardware Register
> Description has QUPV3_0_QUPV3_ID_0 at 0x00800000 and
> QUPV3_1_QUPV3_ID_0 at 0x00a00000. This QUP apparently is
> the latter. In the SDM845 DT the QUP @ac0000 has the label
> 'qupv3_id_1', I guess this should be the same here.

I had a re-look at the documentation again and yes, you are
right, this seems exactly same as on sdm845 except that on
sdm845 the 2 blocks were named QUPV3_0_QUPV3_ID_1 at 0x00800000
and QUPV3_1_QUPV3_ID_1 at 0x00a00000.
I will match this up with the labeling approach we followed on
sdm845. Thanks.

> 
>> +			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";
>> +
>> +			uart2: serial@a88000 {
>> +				compatible = "qcom,geni-debug-uart";
>> +				reg = <0 0x00a88000 0 0x4000>;
> 
> Related to the comment above: on SDM845 this UART has the label
> 'uart10'. I understand these are different SoCs, but could you
> please clarify the enumeration of the SC7180 QUPs and their ports?

I will move this to uart10 once I have the qup instance marked with id_1.
On sdm845 the qup_id_0 had SE instances from 0 to 7 and qup_id_1 had it
from 8 to 15. I will follow the same here so this uart instance would
remain the same as on sdm845, which is uart10.

thanks,
Rajendra

> 
>> +				clock-names = "se";
>> +				clocks = <&gcc GCC_QUPV3_WRAP1_S2_CLK>;
>> +				pinctrl-names = "default";
>> +				pinctrl-0 = <&qup_uart2_default>;
>> +				interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>;
>> +				status = "disabled";
>> +			};
>> +		};
>> +



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