Date:   Wed, 4 Jan 2017 10:37:57 +0800
From:   Chen-Yu Tsai <wens@...e.org>
To:     André Przywara <andre.przywara@....com>
Cc:     Chen-Yu Tsai <wens@...e.org>,
        Maxime Ripard <maxime.ripard@...e-electrons.com>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Hans De Goede <hdegoede@...hat.com>,
        Icenowy Zheng <icenowy@...c.xyz>,
        Mark Rutland <mark.rutland@....com>,
        Rob Herring <robh+dt@...nel.org>,
        devicetree <devicetree@...r.kernel.org>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        linux-sunxi <linux-sunxi@...glegroups.com>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/5] arm64: dts: sun50i: add MMC nodes

On Wed, Jan 4, 2017 at 8:22 AM, André Przywara <andre.przywara@....com> wrote:
> On 03/01/17 13:28, Chen-Yu Tsai wrote:
>> On Tue, Jan 3, 2017 at 6:48 PM, André Przywara <andre.przywara@....com> wrote:
>>> On 03/01/17 02:52, Chen-Yu Tsai wrote:
>
> Hi Chen-Yu,
>
>>>> On Tue, Jan 3, 2017 at 7:03 AM, Andre Przywara <andre.przywara@....com> wrote:
>>>>
>>>> A commit message explaining the mmc controllers would be nice.
>>>
>>> OK.
>>>
>>>>> Signed-off-by: Andre Przywara <andre.przywara@....com>
>>>>> ---
>>>>>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 67 +++++++++++++++++++++++++++
>>>>>  1 file changed, 67 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>>>>> index e0dcab8..c680566 100644
>>>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>>>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>>>>> @@ -150,6 +150,32 @@
>>>>>                                 pins = "PB8", "PB9";
>>>>>                                 function = "uart0";
>>>>>                         };
>>>>> +
>>>>> +                       mmc0_pins: mmc0@0 {
>>>>> +                               pins = "PF0", "PF1", "PF2", "PF3", "PF4", "PF5";
>>>>> +                               function = "mmc0";
>>>>> +                               drive-strength = <30>;
>>>>> +                       };
>>>>> +
>>>>> +                       mmc0_default_cd_pin: mmc0_cd_pin@0 {
>>>>> +                               pins = "PF6";
>>>>> +                               function = "gpio_in";
>>>>> +                               bias-pull-up;
>>>>> +                       };
>>>>
>>>> We are starting to drop pinmux nodes for gpio usage.
>>>
>>> And replacing them with what?
>>> Or do you mean they go in the individual board .dts files?
>>> In this case I believe having a default pin defined here would help to
>>> define it in every .dts.
>>
>> Nope. I meant dropping them. Pinmux and gpio are orthogonal. One should not
>> need to specify a gpio pinmux to use it as a gpio. We added them because in
>> the past nothing was preventing someone from claiming an already muxed pin
>> as a gpio. On some platforms this is fine. For sunxi, this breaks the system,
>> as the gpio functions are muxed in.
>>
>> The idea moving forward is that these cases should be guarded in the driver.
>
> Ah, OK, you mean just referencing the pin like:
>         cd-gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 */
> in the board DT is enough? And the driver claiming it will (eventually?)
> prevent users from using them via the sysfs interface then?

cd-gpios = ....

should be enough to block other devices from claiming a pinmux (pinctrl-N = ...)
on the same pin, or vice versa, whichever claimed it first in kernel space.

Same goes for a pin already claimed by a device through a pinmux. It should
not be claimable as a gpio through the sysfs interface.

>> Of course we would have to deal with existing dtbs, but lets not add any more.
>>
>>>>> +
>>>>> +                       mmc1_pins: mmc1@0 {
>>>>> +                               pins = "PG0", "PG1", "PG2", "PG3", "PG4", "PG5";
>>>>> +                               function = "mmc1";
>>>>> +                               drive-strength = <30>;
>>>>> +                       };
>>>>> +
>>>>> +                       mmc2_pins: mmc2@0 {
>>>>> +                               pins = "PC1", "PC5", "PC6", "PC8", "PC9",
>>>>> +                                      "PC10", "PC11", "PC12", "PC13", "PC14",
>>>>> +                                      "PC15", "PC16";
>>>>> +                               function = "mmc2";
>>>>> +                               drive-strength = <30>;
>>>>> +                       };
>>>>
>>>> Moreover I think you should split out the pinmux nodes to a separate patch.
>>>
>>> I can surely do, just wondering what's the rationale is behind that?
>>
>> More or less the "do one thing in one patch" rationale. Of course you can
>> claim these are the defaults used in the reference design and pretty much
>> every board out there. Then it makes sense to do them together. :)
>
> Mmmh, probably a misunderstanding, but:
> Those pins are the possible pins that expose the MMC interface. Those
> nodes here name them to allow easy and concise reference by a board DT
> (as in the following patch).
> And in fact in case of the A64 there are _no_ alternative muxes for the
> three MMC controllers: SDC0 is only on PF0-5, SDC1 on PG0-PG5 and SDC2
> on PC1-PC16.
> So it's not a board design question, apart from using a controller or not.
>
> That's why I rather keep them together with the MMC controller nodes.

Cool. Please mention this in the commit log, as in they are the only
possible choice.S ince they are the only choices, please drop the @0
in the node name.

And you might want to rename mmc2_pins to something like
"mmc2_8bit_pins: mmc2_8bit". Who knows, someone might consider using
it for an SD card or SDIO, and the remaining pins for something else.

>>>>
>>>>>                 };
>>>>>
>>>>>                 uart0: serial@...8000 {
>>>>> @@ -240,6 +266,47 @@
>>>>>                         #size-cells = <0>;
>>>>>                 };
>>>>>
>>>>> +               mmc0: mmc@...f000 {
>>>>> +                       compatible = "allwinner,sun50i-a64-mmc",
>>>>> +                                    "allwinner,sun5i-a13-mmc";
>>>>
>>>> Given that sun5i doesn't support mmc delay timings, and the A64 has
>>>> calibration and delay timings, I wouldn't call them compatible.
>>>>
>>>> Or are you claiming that for the A64 has a delay of 0 for the
>>>> currently supported speeds, so the calibration doesn't really
>>>> matter? If so this should be mentioned in the commit message.
>>>
>>> Yes, that's my observation: Driving it with sun5-a13-mmc just works.
>>> This sun5i driver version does not (and will never) support higher
>>> transfer modes anyway, so for that subset they are compatible. This
>>> opens up the door to other operating systems not having a particular
>>> driver for the A64, for instance, also older Linux kernels.
>>> I know that sunxi doesn't use this compatible feature much, but IMHO we
>>> should really start thinking about the DT not just being Linux specific
>>> - or even being specific to a certain Linux version. And this case here
>>> is a good example: An A13 MMC driver can drive this device - if there is
>>> no better driver (an A64 one) available.
>>
>> Cool. Please put this in the commit log. :)
>
> Noted for a repost.
>
>>>
>>>>
>>>>> +                       reg = <0x01c0f000 0x1000>;
>>>>> +                       clocks = <&ccu 31>, <&ccu 75>;
>>>>
>>>> The clock / reset index macros are in the tree now.
>>>> Please switch to them.
>>>
>>> The include file is in the tree, but it isn't used in the current HEAD
>>> (as in #included and the actual macros being used in the .dtsi).
>>> So I was wondering if there is a patch pending for that?
>>
>> Not yet I think. Perhaps Maxime will do one once he gets back from vacation?
>
> Seems like everybody hopes for it and nobody bothers with this rather
> trivial patch ;-)

A bit of context here. This was changed _after_ the A64 patches were
pulled into arm-soc, by the arm-soc maintainers, to avoid ugly cross
tree merges. The idea was to change them back after -rc1. I don't know
if a patch to do that is ready though.

> Frankly I am not overly keen on having all those defines in the DTs,
> which rely on a bunch of include files scattered over the Linux include
> tree. This makes it unnecessarily complicated to compile this out of
> tree or move it over to some other piece of software (other OS, U-Boot).
> Also it gives people the impression that it's fine to change the
> definition in the include file at will (because it looks like Linux
> territory).

The dt-bindings related header files are split into a separate directory.
They are also included in the separate devicetree repository:

    https://git.kernel.org/cgit/linux/kernel/git/devicetree/devicetree-rebasing.git/

And no, I don't think it implies that it's fine to change it. The
include/dt-bindings/ subdirectory is part of the device tree bindings,
just like Documentation/devicetree/bindings. Does the latter give the
impression that it is fine to change it, because it's in the docs?

> Eventually some file has to hold the actual hardware information, I
> don't see why this shouldn't be the one .dtsi file. Reading that the
> clock for MMC controller 0 is MMC0_CLK isn't very informative.

For me, it makes it much easier to review, than say a plain number,
which I would have to dig through some file, probably the clock driver,
to check if it matches.

Regards
ChenYu

> Cheers,
> Andre.
>
>>>>> +                       clock-names = "ahb", "mmc";
>>>>> +                       resets = <&ccu 8>;
>>>>> +                       reset-names = "ahb";
>>>>> +                       interrupts = <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>;
>>>>> +                       status = "disabled";
>>>>> +                       #address-cells = <1>;
>>>>> +                       #size-cells = <0>;
>>>>> +               };
>>>>> +
>>>>> +               mmc1: mmc@...0000 {
>>>>> +                       compatible = "allwinner,sun50i-a64-mmc",
>>>>> +                                    "allwinner,sun5i-a13-mmc";
>>>>> +                       reg = <0x01c10000 0x1000>;
>>>>> +                       clocks = <&ccu 32>, <&ccu 76>;
>>>>> +                       clock-names = "ahb", "mmc";
>>>>> +                       resets = <&ccu 9>;
>>>>> +                       reset-names = "ahb";
>>>>> +                       interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
>>>>> +                       status = "disabled";
>>>>> +                       #address-cells = <1>;
>>>>> +                       #size-cells = <0>;
>>>>> +               };
>>>>> +
>>>>> +               mmc2: mmc@...1000 {
>>>>> +                       compatible = "allwinner,sun50i-a64-emmc";
>>>>> +                       reg = <0x01c11000 0x1000>;
>>>>> +                       clocks = <&ccu 33>, <&ccu 77>;
>>>>> +                       clock-names = "ahb", "mmc";
>>>>> +                       resets = <&ccu 10>;
>>>>> +                       reset-names = "ahb";
>>>>> +                       interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
>>>>> +                       status = "disabled";
>>>>> +                       #address-cells = <1>;
>>>>> +                       #size-cells = <0>;
>>>>> +               };
>>>>> +
>>>>>                 gic: interrupt-controller@...1000 {
>>>>>                         compatible = "arm,gic-400";
>>>>>                         reg = <0x01c81000 0x1000>,
>>>>> --
>>>>> 2.8.2
>>>>>
>>>
>

Powered by blists - more mailing lists