On Tue 06 Feb 11:49 PST 2018, Doug Anderson wrote:

Hi,

On Tue, Feb 6, 2018 at 11:06 AM, Bjorn Andersson
[off-list ref] wrote:

quoted

On Tue 06 Feb 10:37 PST 2018, Doug Anderson wrote:

quoted

Hi,

On Fri, Jan 26, 2018 at 2:18 PM, Stephen Boyd [off-list ref] wrote:

quoted

On 01/25, Rajendra Nayak wrote:

quoted

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.


To make it concrete, you'd have something like this (this has the
wrong bindings from the UART, but folks get the picture hopefully):


In sdm845.dtsi:

    qup_uart2: serial at a84000 {
      compatible = "qcom,geni-console", "qcom,geni-uart";
      reg = <0xa84000 0x4000>;
      reg-names = "se_phys";
      clock-names = "se-clk", "m-ahb", "s-ahb";
      clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>,
         <&gcc GCC_QUPV3_WRAP_1_M_AHB_CLK>,
         <&gcc GCC_QUPV3_WRAP_1_S_AHB_CLK>;
      pinctrl-names = "default", "sleep";
      pinctrl-0 = <&qup_uart2_default>;
      pinctrl-1 = <&qup_uart2_sleep>;
      interrupts = <GIC_SPI 354 IRQ_TYPE_NONE>;
      qcom,wrapper-core = <&qup_1>;
      status = "disabled";
    };

    tlmm: pinctrl at 3400000 {
      compatible = "qcom,sdm845-pinctrl";
      reg = <0x03400000 0xc00000>;
      interrupts = <GIC_SPI 208 IRQ_TYPE_NONE>;
      gpio-controller;
      #gpio-cells = <2>;
      interrupt-controller;
      #interrupt-cells = <2>;

      qup_uart2_default: qup_uart2_default {
        pinmux {
          function = "qup9";
          pins = "gpio4", "gpio5";
        };
      };

      qup_uart2_sleep: qup_uart2_sleep {
        pinmux {
          function = "gpio";
          pins = "gpio4", "gpio5";
        };
      };
    };

In sdm845-mtp.dts:

&qup_uart2_default {
  pinconf {
    pins = "gpio4", "gpio5";
    drive-strength = <2>;
    bias-disable;
  };
};

&qup_uart2_sleep {
  pinconf {
    pins = "gpio4", "gpio5";
    drive-strength = <2>;
    bias-disable;
  };
};

Correct.


This example does however show another thing that I really do not like;
When you have a lot of nodes I find it very useful to maintain some sort
of grouping, to know that I can find a node describing properties
related to some block close to related blocks - e.g. nodes describing
a pmic block is close to other nodes for that pmic.

Today we seem to have a mixture of bus-based grouping, arbitrary
grouping and no grouping at all in our upstream dtsi files, so I think
we should set some guidelines here as well.

Regards,
Bjorn