Re: [PATCH v2 3/3] arm64: dts: sdm845: Add serial console support

From: Rajendra Nayak
Date: Tue Feb 06 2018 - 23:29:10 EST


[]..

>> @@ -10,4 +10,46 @@
>> / {
>> model = "Qualcomm Technologies, Inc. SDM845 MTP";
>> compatible = "qcom,sdm845-mtp";
>> +
>> + aliases {
>> + serial0 = &qup_uart2;
>> + };
>> +
>> + chosen {
>> + stdout-path = "serial0";
>> + };
>> +
>> + soc {
>
> I don't know if there's an official position, but in general I'm seen
> people use the actual "soc" alias here. AKA at the top level of this
> dts, just do:
>
> &soc {
> ...
> };
>
> Normally doing stuff like that is useful so you don't need to
> replicate the whole hierarchy. In this case that's not a huge
> savings, but it can be nice to be consistent. In the very least it
> saves you a level of indentation.
>
>
>> + serial@a84000 {
>> + status = "okay";
>> + };
>
> Similarly here you can use the alias from the sdm845.dtsi file to
> avoid replicating the hierarchy. AKA at the top level do:
>
> &qup_uart2 {
> status = "okay";
> };
>
> In this case it saves you 2 levels of indentation.

Right. Andy/Bjorn, are there any preferences here?
I see we don't do this for the other board files, and I not sure
theres a specific reasoning for how its currently done and if we
need to stick to it.

>
>> +
>> + pinctrl@3400000 {
>> + qup_uart2_default: qup_uart2_default {
>
> I'm not sure how persnickety I should be, but according to
> <https://elinux.org/Device_Tree_Linux>:
>
> node names use dash "-" instead of underscore "_"
>
> ...but, of course, labels can't use dashes (and the same page says to
> use underscore for labels). This is why, in rk3288 for instance, you
> see:
>
> i2c2_xfer: i2c2-xfer {
> rockchip,pins = <6 9 RK_FUNC_1 &pcfg_pull_none>,
> <6 10 RK_FUNC_1 &pcfg_pull_none>;
> };
>
> AKA the label and the node name are the same but the label uses "_"
> and the node names use "-".

Sure, I'll fix these up.

[]
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> index 02520f19e4ca..c4ce70840acf 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> @@ -4,6 +4,7 @@
>> */
>>
>> #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +#include <dt-bindings/clock/qcom,gcc-sdm845.h>
>>
>> / {
>> model = "Qualcomm Technologies, Inc. SDM845";
>> @@ -273,5 +274,25 @@
>> cell-index = <0>;
>> };
>>
>> + qup_1: qcom,geni_se@ac0000 {
>> + compatible = "qcom,geni-se-qup";
>> + reg = <0xac0000 0x6000>;
>
> I think you may have mentioned this in another context, but this
> doesn't match the current bindings. Some clocks should be here.
> ...and it looks like the uart should be a subnode.

right, these were tested with the v1 set for serial. I will update them.

regards
Rajendra

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