Re: [PATCH v5 5/6] arm64: dts: cix: add initial CIX P1(SKY1) dts support

[Krzysztof Kozlowski <krzk@kernel.org>]

On 27/03/2025 07:44, Peter Chen wrote:
>>>
>>> On 25-03-25 10:52:10, Marc Zyngier wrote:
>>>>> +     timer {
>>>>> +             compatible = "arm,armv8-timer";
>>>>> +             interrupt-names = "sec-phys", "phys", "virt", "hyp-phys", "hyp-virt";
>>>>> +             interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW 0>,
>>>>> +                          <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW 0>,
>>>>> +                          <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW 0>,
>>>>> +                          <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW 0>,
>>>>> +                          <GIC_PPI 12 IRQ_TYPE_LEVEL_LOW 0>;
>>>>> +     };
>>>>> +};
>>>>
>>>> I don't think there is anything wrong here, but it is also a pretty
>>>> useless DT. There isn't even a UART to interact with the machine and
>>>> find out whether it has actually booted.
>>>>
>>>
>>> UEFI uses the same UART, so we could see all kernel boot logs until
>>> switch to use kernel UART driver for printk. If you would like boot
>>> to the console at initramfs, just add uart node like patchset v1.
>>
>> What's the point in upstreaming something that requires extra changes
>> just to boot it? It only outlines these patches are not useful as they
>> stand.
>>
>>>
>>>> I reckon this should be part of the initial DT, as this otherwise
>>>> serves little purpose.
>>>>
>>>
>>> Without this initial support, we can't add some base drivers, like
>>> mailbox. The dt_binding_check will report warnings/errors [1].
>>
>> Of course you can. You just add additional patches to this series,
>> making it something that is actually useful. So far, this series only
>> serves as marketing material.
>>
>>> Full UART support depends on clock, clock control needs mailbox
>>> to talk with FW using SCMI protocol.
>>
>> Then do it. You obviously have existing DT support for it already.
>>
>>> There is no any support for CIX SoC, so we had to add one small step by
>>> step.
>>
>> No, you are deliberately choosing to make this platform useless.
>>
>> That's a bit sad, and a waste of everybody's time.
>>
> 
> Hi Marc,
> 
> Thanks for your interesting of our platform, and your comments
> help us a lot. But I don't think it wastes reviewers and maintainers
> time, a clean patch set saves everyone's time during upstream process.
> 
> For how to organize the patch set for SoC, Krzysztof gave good summary
> at [1]. We are going on upstream [2], this patch set is just a start
> and base but not like you said for marketing purpose.


I do not think I suggested in [1] to ever send new SoC containing only
CPUs and interrupt controller, without even serial. My instruction [1]
was how to organize it. The DTS can be even fully complete, see the
upstreaming example I have been using all the time - Qualcomm SM8650:

https://lore.kernel.org/all/20231124-topic-sm8650-upstream-dt-v4-0-e402e73cc5f0@linaro.org/

Entire SoC sent to mailing list on the day one of public release of that
flagship Qualcomm SoC. The SoC DTSI and board DTS have almost complete
picture, except few trickier pieces... but it even has full display and
GPU! Plus, as I explained on my email on samsung-soc, that DTS/DTSI
patchset references all other bindings with their state, so SoC
maintainers can understand what is the overall progress and what will be
the result in DT schema checks, if they apply the patchset.

The minimum, absolute minimum submission is with the serial nodes. I
would prefer to have some storage or any other interface as well, but
that's fine.


> 
> [1] https://lore.kernel.org/linux-samsung-soc/CADrjBPq_0nUYRABKpskRF_dhHu+4K=duPVZX==0pr+cjSL_caQ@mail.gmail.com/T/#m2d9130a1342ab201ab49670fa6c858ee3724c83c
> [2] https://lore.kernel.org/lkml/20250325101807.2202758-1-guomin.chen@cixtech.com/
> 


Best regards,
Krzysztof