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

[Rajendra Nayak]

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@xxxxxxxxxxxxxxx/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