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

[Suman Anna <s-anna@ti.com>]

+ Arnd and Tony

On 1/21/21 12:39 PM, Nishanth Menon wrote:
> On 12:13-20210121, Suman Anna wrote:
>> 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
> 
> What is counter intutive about a -next branch be tested against
> linux-next tree?

The -next process is well understood. FWIW, you are not sending your PR against
-next branch, but against primarily a -rc1 or -rc2 baseline.

As a developer, when I am submitting patches, I am making sure that things are
functional against the baseline you use. For example, when I split functionality
into a driver portions and dts portions, I need to make sure both those
individual pieces boot fine and do not cause regressions, even though for the
final functionality, you need both.

> 
>> 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.
> 
> Also read https://www.kernel.org/doc/html/v5.11-rc4/process/2.Process.html#next-trees
> 
> You should probably realize linux-next is an integral part of the
> process for us now.
> 
>>
>> 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";
> 
> 
> you are saying basically - wait a complete kernel cycle after a driver
> is introduced before we can even test a driver SoC support introduced
> without an user in the same kernel version.. which is a disaster and bit-rot
> 
> OR
> 
> Let the subsystem maintainers also carry the patches for dts - which is
> going to be another disaster and creates all kind of avoidable merge
> conflicts.
> 
> OR
> 
> I stage a rc1 and rc2 merge cycle - which makes no sense - these nodes
> dont get activated without a compatible match, which gets enabled only
> when the corresponding subsystem is merged - they dont break existing
> functionality even when the subsystem is merged, it just increases
> the functionality as it should. (not to mention that all my follow on
> kernel merge trees will have to be rc2 based - since majority nodes
> will be introduced there)
> 
> dts already has a pain point that we are trying to manage logically
> here, this is not a MISRA-C ASIL-D process - follow and exact verbatim
> word to word process, that is just plain ridiculous.
> 
> When rc1 comes together, which is what my next branch is for, things
> should be cohesive - we dont introduce regressions and broken trees -
> which is exactly what the -next process makes sure happens.
> 
> 
> Now, if you want to launch a product with my -next branch - go ahead, I
> don't intent it for current kernel version - you are on your own.
> 
> If there is a real risk of upstream next-breaking - speakup with an
> real example - All I care about is keeping upstream functional and
> useable.

This is all moot when your own tree doesn't boot properly. In this case, you are
adding MMC nodes, but yet for a boot test, you are saying use linux-next for the
nodes that were added or you need additional driver patches (which is not how
maintainer-level trees are verified).

Arnd,
Can you please guide us here as to what is expected in general, given that the
pull-request from Nishanth goes through you, and if there is some pre-existing
norms around this?

Tony,
Appreciate your input as well since you probably have dealt with these kinda of
dependencies on OMAP.

regards
Suman

> 
> I recheck the linux-next tree almost daily basis for consistency, and I
> do appreciate the concern here (and passion) - point is, I think we
> might be a bit of an over-reaction if we just look at the other options
> in front of us - not to mention, maybe drop the entire idea of dt coming
> in from ARM SoC - let the subsystem member create merge conflict and
> duke it out.. I don't think any of us want to see that kind of mayhem.
> 


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