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

Rajendra Nayak
2019-10-22T06:15:25Z

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";
>> + };
>> + };
>> +