Re: [PATCH v3 3/5] arm64: dts: ti: Add support for AM642 SoC

From: Suman Anna <s-anna@ti.com>
To: Nishanth Menon <nm@ti.com>
Cc: devicetree@vger.kernel.org, Vignesh Raghavendra <vigneshr@ti.com>,
	Dave Gerlach <d-gerlach@ti.com>, Tony Lindgren <tony@atomide.com>,
	Sekhar Nori <nsekhar@ti.com>,
	Kishon Vijay Abraham <kishon@ti.com>,
	Lokesh Vutla <lokeshvutla@ti.com>,
	Rob Herring <robh+dt@kernel.org>,
	Aswath Govindraju <a-govindraju@ti.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 3/5] arm64: dts: ti: Add support for AM642 SoC
Date: Thu, 21 Jan 2021 12:13:46 -0600	[thread overview]
Message-ID: <4ee6f005-2eee-42b2-b573-e10602839e1b@ti.com> (raw)
In-Reply-To: <20210121174639.jqbvem6b4ozd3six@sterling>

On 1/21/21 11:46 AM, Nishanth Menon wrote:
> On 11:25-20210121, Suman Anna wrote:
>> On 1/20/21 2:25 PM, Dave Gerlach wrote:
>>> The AM642 SoC belongs to the K3 Multicore SoC architecture platform,
>>> providing advanced system integration to enable applications such as
>>> Motor Drives, PLC, Remote IO and IoT Gateways.
>>>
>>> Some highlights of this SoC are:
>>> * Dual Cortex-A53s in a single cluster, two clusters of dual Cortex-R5F
>>>   MCUs, and a single Cortex-M4F.
>>> * Two Gigabit Industrial Communication Subsystems (ICSSG).
>>> * Integrated Ethernet switch supporting up to a total of two external
>>>   ports.
>>> * PCIe-GEN2x1L, USB3/USB2, 2xCAN-FD, eMMC and SD, UFS, OSPI memory
>>>   controller, QSPI, I2C, eCAP/eQEP, ePWM, ADC, among other
>>>   peripherals.
>>> * Centralized System Controller for Security, Power, and Resource
>>>   Management (DMSC).
>>>
>>> See AM64X Technical Reference Manual (SPRUIM2, Nov 2020)
>>> for further details: https://www.ti.com/lit/pdf/spruim2
>>>
>>> Introduce basic support for the AM642 SoC to enable ramdisk or MMC
>>> boot. Introduce the sdhci, i2c, spi, and uart MAIN domain periperhals
>>> under cbass_main and the i2c, spi, and uart MCU domain periperhals
>>> under cbass_mcu.
>>>
>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>>
>> Hmm, there are a few pieces contributed by me, so please do add
>>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
> 
> Sure, thanks..
> 
> [...]
> 
>>> +
>>> +	sdhci0: mmc@fa10000 {
>>> +		compatible = "ti,am64-sdhci-8bit";
>>
>> Hmm, I tried booting this series on top of 5.11-rc1 + Nishanth's current
>> ti-k3-dts-next. So, boot of these patches using this baseline fails when using
>> MMC rootfs, but is ok when using initramfs. This particular compatible and the
>> corresponding driver change are only in linux-next coming through couple of
>> patches from the MMC subsystem.
>>
>> I am not sure why we would be including stuff that's dependent on some other
>> patches being merged from a different sub-system? Strangely, this ought to be
>> caught by dtbs_check, but it is not throwing any errors.
>>
>> IMHO, these should only be added if you have no other external dependencies
>> especially when you are applying on a 5.11-rc baseline. The MMC pull-requests
>> would not go through arm-soc either.
>>
> 
> Yes, I am aware of this - this is no different from integration we have
> done in the past as well.. intent is to get bindings in via subsystem
> trees and dts changes via arm-soc. I always insist that basic ramdisk
> boot always in the basic introduction tree. mmc, nfs are add-ons that
> get added via subsystem tree and I host the dts changes - in this case
> every dts node binding is fine with subsystems already queued in
> linux-next. And this is no different from what I have noticed on other
> ARM SoC maintainer trees as well.
> 

Hmm, this is kinda counter-intuitive. When I see a dts node, I am expecting the
required driver functionality to have been in (or atleast the binding as per
documentation), and not having to need to pick additional patches.

If the intent is to verify/test everything against linux-next and not the
baseline tree, then I guess this works. But in general, this kinda goes against
the rules set in submitting patches. For example, see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/submitting-patches.rst#n44

And sure enough, this is what I get when I run checkpatch against your tree.

WARNING: DT compatible string "ti,am64-sdhci-8bit" appears un-documented --
check ./Documentation/devicetree/bindings/
#347: FILE: arch/arm64/boot/dts/ti/k3-am64-main.dtsi:298:
+		compatible = "ti,am64-sdhci-8bit";

WARNING: DT compatible string "ti,am64-sdhci-4bit" appears un-documented --
check ./Documentation/devicetree/bindings/
#365: FILE: arch/arm64/boot/dts/ti/k3-am64-main.dtsi:316:
+		compatible = "ti,am64-sdhci-4bit";

regards
Suman

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel