On 2.05.2022 21:49, Dmitry Baryshkov wrote:
> On Mon, 2 May 2022 at 22:01, Dmitry Baryshkov
> <dmitry.baryshkov@xxxxxxxxxx> wrote:
>>
>> On Mon, 2 May 2022 at 20:59, Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxx> wrote:
>>>
>>>
>>>
>>> On 1.05.2022 21:54, Dmitry Baryshkov wrote:
>>>> Add device tree node for uart20, which is typically used for Bluetooth attachment.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
>>>> Reviewed-by: Vinod Koul <vkoul@xxxxxxxxxx>
>>>> ---
>>>> arch/arm64/boot/dts/qcom/sm8450.dtsi | 22 ++++++++++++++++++++++
>>>> 1 file changed, 22 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>>>> index 4fcb6e2b096b..8b9d9c2cd02c 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>>>> @@ -996,6 +996,19 @@ spi20: spi@894000 {
>>>> status = "disabled";
>>>> };
>>>>
>>>> + uart20: serial@894000 {
>>> I think it should come before SPI alphabetically?
>>
>> Argh. I sorted it using the label!
>>
>>>
>>>> + compatible = "qcom,geni-uart";
>>>> + reg = <0 0x00894000 0 0x4000>;
>>>> + clock-names = "se";
>>>> + clocks = <&gcc GCC_QUPV3_WRAP2_S5_CLK>;
>>>> + pinctrl-names = "default";
>>>> + pinctrl-0 = <&qup_uart20_default>;
>>> No sleep state?
>>
>> No, uarts do not provide a sleep state.
>
> I've checked other dts. Usually the sleep state is provided by the
> board dts rather than the SoC's dtsi.
Weird. Perhaps we could consider centralizing that, since it's common(-ish) in the end?
>
>>
>>>
>>>> + interrupts = <GIC_SPI 587 IRQ_TYPE_LEVEL_HIGH>;
>>>> + #address-cells = <1>;
>>>> + #size-cells = <0>;
>>>> + status = "disabled";
>>>> + };
>>>> +
>>>> i2c21: i2c@898000 {
>>>> compatible = "qcom,geni-i2c";
>>>> reg = <0x0 0x00898000 0x0 0x4000>;
>>>> @@ -2757,6 +2770,15 @@ qup_uart7_tx: qup-uart7-tx {
>>>> drive-strength = <2>;
>>>> bias-disable;
>>>> };
>>>> +
>>>> + qup_uart20_default: qup-uart20-default {
>>>> + mux {
>>> Please drop the unnecessary mux{} here.
>>
>> Ack.
>>
>>>
>>>> + pins = "gpio76", "gpio77",
>>>> + "gpio78", "gpio79";
>>> I think these could fit into a single 100-char-long line>?
>>
>> I'll check.
>>
>>>
>>>> + function = "qup20";
>>> Are there no default properties for this setup? I think boards that don't use standard Qualcomm connectivity setups (like Bluetooth on this specific UART) are rather scarce and it'd be more convenient to keep a standard setting here and override it where need be instead of copy-pasting the same thing over and over in 95-100% of the boards.
>>
>> I see your point. Let's do this.
>
> Well, comparing with other SoC dtsi shows that most of them declare
> pins&functions in the dtsi and leave bias/ details to the board.dts
> (despite code duplication). So let's follow that approach.
Eh, you're right, but I'd still argue it's really a thing that used to be more relevant in the past though, especially as newer devices seem to get closer and closer to Qualcomm reference designs.. Maybe Bjorn could give some input on this matter?
Konrad
>
>>
>>>
>>> Konrad
>>>
>>>> + };
>>>> + };
>>>> +
>>>> };
>>>>
>>>> apps_smmu: iommu@15000000 {
>>>>
>>
>>
>>
>> --
>> With best wishes
>> Dmitry
>
>
>
