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

From: Rajendra Nayak <rnayak@codeaurora.org>
To: Doug Anderson <dianders@chromium.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Stephen Boyd <sboyd@codeaurora.org>,
	Andy Gross <andy.gross@linaro.org>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-arm-msm@vger.kernel.org,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	devicetree@vger.kernel.org
Subject: Re: [PATCH 2/2] arm64: dts: sdm845: Add serial console support
Date: Wed, 7 Feb 2018 09:42:59 +0530	[thread overview]
Message-ID: <ade06d48-2003-026a-2e99-06ff0223efb9@codeaurora.org> (raw)
In-Reply-To: <CAD=FV=Whxf1VRJ6qx9mioEEkTeS+uJfJKPbOMcGy-Wigh_NORw@mail.gmail.com>

On 02/07/2018 01:19 AM, Doug Anderson wrote:
> Hi,
> 
> On Tue, Feb 6, 2018 at 11:06 AM, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
>> On Tue 06 Feb 10:37 PST 2018, Doug Anderson wrote:
>>
>>> Hi,
>>>
>>> On Fri, Jan 26, 2018 at 2:18 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>>> On 01/25, Rajendra Nayak wrote:
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
>>>>> new file mode 100644
>>>>> index 000000000000..b97f99e6f4b4
>>>>> --- /dev/null
>>>>> +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi
>>>>> @@ -0,0 +1,32 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +/*
>>>>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>>>>> + */
>>>>> +
>>>>> +&tlmm {
>>>>
>>>> I'm not the maintainer, but I find this approach to the pins
>>>> really annoying. I have to flip to another file to figure out how
>>>> a board has configured the pins. And we may bring in a bunch of
>>>> settings that we don't ever use on some board too. Why can't we
>>>> put the settings in the board file directly?
>>>
>>> I'm not so familiar with how things work with Qualcomm, but in general
>>> I think putting this in the "board" file is a bad idea.  I'd be OK
>>> with putting this directly in the SoC file (though it might get
>>> unwieldy?), but not moving things to the board file as was done with
>>> v2 of this patch.
>>>
>>> Said another way: nearly board that uses SDM845 that uses UART2 will
>>> have the same definitions for these pins so we shouldn't be
>>> duplicating it across every board, right?
>>>
>>
>> We've run into several cases where different boards uses the same
>> function but requires board specific electrical configuration.
>>
>> So what we decided was to keep the pinmux in the soc-file (where e.g.
>> the uart definition is) and then extend it with the board specific
>> electrical properties (the pinconf), in the board files.
>>
>> This does come with the complexity of having the pinctrl nodes split in
>> two places, but the responsibilities of the two parts is clear and we
>> remove the need for all board files to ensure the appropriate pinmux is
>> in place.
>>
>>
>> NB. We did discuss adding "sane defaults" for the pinconf in the soc
>> dtsi, but we end up spending considerable time debugging issues stemming
>> from not having the right pinconf; so better make this explicit and say
>> that the board has to specify it's config.
> 
> Whoops, saw your responses _after_ I sent my response to v2.  In any
> case this makes sense to me then!  On Rockchip boards I've been
> involved in we often added "sane defaults", but I can see how that
> could be confusing in different ways.  I'm happy with your choice and
> it seems like a happy medium.  The sdm845.dtsi file can have the main
> definition of the nodes and can thus refer to the nodes.  Then you
> just add the extra bit in the board file.
> 
> What you propose is not what happened in v2 of the series
> <https://patchwork.kernel.org/patch/10194201/> though.  In v2 _both_
> the pinconf and the pinmux moved to the board file.  That's wrong.

got it. I'll fix this up in my v3. Thanks for the review.

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