Message-ID: <q2rfyb2zrx55w632xjudhxs5w7bp3d4gmioav3iy4v32a3m6mj@fct2jalftvmb>
Date: Mon, 29 Sep 2025 10:19:20 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: "Aiqun(Maria) Yu" <aiqun.yu@....qualcomm.com>
Cc: Rob Herring <robh@...nel.org>,
        Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
        Bjorn Andersson <andersson@...nel.org>,
        Krzysztof Kozłowski <k.kozlowski.k@...il.com>,
        Jingyi Wang <jingyi.wang@....qualcomm.com>,
        Konrad Dybcio <konradybcio@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>, linux-arm-msm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        tingwei.zhang@....qualcomm.com, trilok.soni@....qualcomm.com,
        yijie.yang@....qualcomm.com,
        Ronak Raheja <ronak.raheja@....qualcomm.com>
Subject: Re: [PATCH 06/20] arm64: dts: qcom: kaanapali: Add USB support for
 Kaanapali SoC

On Mon, Sep 29, 2025 at 02:06:28PM +0800, Aiqun(Maria) Yu wrote:
> On 9/26/2025 10:47 PM, Rob Herring wrote:
> > On Fri, Sep 26, 2025 at 8:21 AM Konrad Dybcio
> > <konrad.dybcio@....qualcomm.com> wrote:
> >>
> >> On 9/25/25 11:31 PM, Rob Herring wrote:
> >>> On Thu, Sep 25, 2025 at 08:57:56AM -0500, Bjorn Andersson wrote:
> >>>> On Thu, Sep 25, 2025 at 10:50:10AM +0900, Krzysztof Kozłowski wrote:
> >>>>> On Thu, 25 Sept 2025 at 09:17, Jingyi Wang <jingyi.wang@....qualcomm.com> wrote:
> >>>>>>
> >>>>>> From: Ronak Raheja <ronak.raheja@....qualcomm.com>
> >>>>>>
> >>>>>> Add the base USB devicetree definitions for Kaanapali platform. The overall
> >>>>>> chipset contains a single DWC3 USB3 controller (rev. 200a), SS QMP PHY
> >>>>>> (rev. v8) and M31 eUSB2 PHY.
> >>>>>>
> >>>>>> Signed-off-by: Ronak Raheja <ronak.raheja@....qualcomm.com>
> >>>>>> Signed-off-by: Jingyi Wang <jingyi.wang@....qualcomm.com>
> >>>>>> ---
> >>>>>>  arch/arm64/boot/dts/qcom/kaanapali.dtsi | 155 ++++++++++++++++++++++++++++++++
> >>>>>>  1 file changed, 155 insertions(+)
> >>>>>>
> >>>>>
> >>>>>
> >>>>> Second try, without HTML:
> >>>>>
> >>>>> I really don't understand why you created such huge patchset.
> >>>>
> >>>> Because I looked at the logical changes that went into the big squash
> >>>> that was initially planned, and requested that some of those was kept
> >>>> intact - because they where independent logical changes.
> >>>>
> >>>>> Year
> >>>>> ago, two years ago, we were discussing it already and explained that's
> >>>>> just inflating the patchset without reason.
> >>>>>
> >>>>
> >>>> We used to add things node by node and that was indeed not
> >>>> comprehensible. Overall this adds features in large logical chunks, but
> >>>> there are a few of the patches that could have been squashed.
> >>>>
> >>>>> New Soc is one logical change. Maybe two. Not 18!
> >>>>
> >>>> I can see your argument for one patch to introduce the soc. But two
> >>>> doesn't make sense, because that incremental patch is going to be the
> >>>> kitchen sink.
> >>>>
> >>>>>
> >>>>> Not one patch per node or feature.
> >>>>>
> >>>>
> >>>> Definitely agree that we don't want one patch for every tiny block!
> >>>>
> >>>>> This hides big picture, makes difficult to review everything,
> >>>>> difficult to test.
> >>>>
> >>>> The big picture is already obscured due to the size of the content
> >>>> added.
> >>>>
> >>>> Comparing to previous targets, I see the baseline content in 2-3
> >>>> patches, and the remainder of the series being things that usually has
> >>>> been scattered in many more small changes in the following weeks or
> >>>> months.
> >>>>
> >>>> There's plenty of features in this series that are yet to be concluded
> >>>> for SM8750.
> >>>>
> >>>>> Your patch count for LWN stats doesn't matter to
> >>>>> us.
> >>>>
> >>>> I agree with this. That's why the QRD is 1 patch, and MTP is 4 (this I
> >>>> think should be squashed to 2) - compared to 13 patches for across the
> >>>> pair for SM8750 with less scope.
> >>>>
> >>>>>
> >>>>> NAK and I'm really disappointed I have to repeat the same review .
> >>>>
> >>>> I'm not sure what you're disappointed in, this initial series is larger
> >>>> than any we've seen before. I really like the work Jingyi has done here,
> >>>> aggregating the otherwise scattered patches into one series.
> >>>
> >>> The QCom folks can review all this first because I don't care to review
> >>> the 50+ binding (just bindings!) patches sent all at once right before
> >>> the merge window.
> >>
> >> Unfortunately this is sort of beyond our control. We don't expect you to
> >> review or apply these patches immediately.
> > 
> > It is *only* in your (QCom) control. I would love to have control over
> > receiving patches to review, but sadly I do not.
> > 
> > Then you should mark them RFC at least if you know they are going into 6.18.
> 
> We can take your advice for "RFC" for next situation for this.
> 
> It would be ideal if most of these patches could make it into the 6.18
> release—that is, get accepted before the merge window opens—since the
> 6.18 kernel is a very important version for us.

Most of the branches related to 6.18 tree should be already closed (and
were almost closed for the last week or two, depending on the
subsystem). 6.17 was released several hours ago, which means we are now
in the merge window towards 6.18.

> 
> While, we fully respect the reviewers' perspective if some patches are
> not yet clean or ready. In such cases, we’re prepared to put in
> additional effort on our side, including backporting as needed.

I'm not sure what do you mean here.

> 
> > 
> >> The platform announcement just happened to occur at this and not any other
> >> time, and we can't just ask the entire company to shift it to better
> >> accommodate the kernel release cycle..
> > 
> > That's exactly what we expect. Companies following the rules or
> > preferences of the kernel community is exactly what is expected and
> > required. Companies that continuously fail to do that result in
> > requirements that all patches be first signed off by trusted kernel
> > developers in those companies.
> From my understanding, the community is intended to be open to all
> developers—whether they contribute individually or through a company.
> Imposing a strict "Signed-off-by" requirement risks excluding developers
> who actively participate in this community effort.
> We still strongly encourage broader participation from both individual
> contributors and company-affiliated developers to foster a more open and
> inclusive environment.
> 
> That said, I do agree that companies should aim to conduct thorough
> internal reviews to reduce the burden on upstream maintainers and
> reviewers. For large patch sets, perhaps we can consider using
> "Reviewed-by" from trusted kernel developers within the company.
> 
> In fact, we did perform internal reviews before posting the Kaanapali
> patches. However, we also respect the community rule that "Reviewed-by"
> should only be added based on public review within the community.
> 
> Lastly, the principle of "upstream first" and submitting patches as
> early as possible remains a key guideline in the current kernel
> development process.
> 
> > 
> > What would you have done if the timing hit in the merge window where
> > you have trees which have policies of don't send new content during
> > merge windows? Just going to ignore that?
> > 
> 
> If some of the patches aren’t clean enough for the current review cycle
> and the merge window has just opened, would it be appropriate to resend
> them after the merge window closes—perhaps after October 12th?>

No need to, most of the patches will be reviewed. But the ship for 6.18
has already sailed. 6.19 is the earliest merge target.

> >> We do have an interest in sharing the work at the earliest time possible,
> >> and with all the legal knots included, this is what it came down to.
> >>
> >> I (and many others) made an internal push to upstream any pre-requisite
> >> patches that we didn't need to disclose any platform details for in
> >> advance, so this patchbomb is actually somewhat reduced.. but of course
> >> DT and bindings are the main course size-wise and we simply couldn't do
> >> it earlier.
> >>
> >> Give or take 80% of the bindings will be "boring", i.e. "add compatbile"
> >> or "add compatible and adjust clocks" because our hw is rather
> >> standardized and the interesting changes often happen at a level beyond
> >> bindings
> >>
> >>> One comment on commit messages though. Please explain how the h/w block
> >>> is or isn't compatible with some existing platforms. Many just state the
> >>> obvious "add a compatible" or such. I've yet to find what kaanapali is
> >>> in relation to any other QCom chip. It may be the next SoC for the smart
> >>> toaster market for all I know.
> >>
> >> Perhaps this would be useful to have in bindings commit messages, but
> >> the cover letter of >this< series states that Kaanapali is the newly
> >> announced Snapdragon 8 Elite Gen 5.
> > 
> > Patches should stand on their own. I'm talking about patches in other series.
> 
> We can add the soc introduction information to the each patches series's
> change log and resend after merge window October 12th. Like:
> Kaanapali is the newly announced Snapdragon 8 Elite Gen 5, and here is
> the document link for reference [1]:
> [1]https://www.qualcomm.com/products/mobile/snapdragon/smartphones/snapdragon-8-series-mobile-platforms/snapdragon-8-elite-gen-5

Note, you have been asked to explain the details regarding IP block
compatibility, rather than just marketing details regarding the
platform. A proper commit message might say something like 'Block foo on
Kaapanali is compatible with the same block on the MSM8960, use
qcom,msm8960-foo as a fallback compatible string'. Or 'Block foo on
Kaanapali has different register stride between ADC channels, as such it
requires a new compatible string'.

> 
> > 
> >> The product page states at the very bottom of the spec sheet that
> >> SM8850 is another name for it (although the shift to codenames
> >> happened precisely to disconnect from specific SKU numbers,
> >> because e.g. both SA8775P and QCS9100 are 'lemans' silicon)
> > 
> > Sorry, I'm not going to go read your product pages...
> 
> 
> Feel free to let me know whether the above suggested updated statements
> in the patch change log address your comments.>

2c from my side: please work on declaring the _actual_ dependencies for
the series (as in 'patches in the series won't even build without
changes a, b, c). Use b4 --edit-deps to declare those dependencies.
Remove unnecessary dependencies between DT bindings (by using ephemeral
nodes instead of actual platform indices). Drop long lists of
pseudo-dependencies from cover letters. They do more harm than help.
Work with Bjorn and Konrad on how to better structure the DT patches.
Make sure that all your commit messages describe the _reason_ for the
change rather than the patch contents.

> > Rob
> -- 
> Thx and BRs,
> Aiqun(Maria) Yu

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists