Re: [RFC PATCH 0/5] Add smp booting support for Qualcomm ARMv8 SoCs

[Kumar Gala]

> On Apr 13, 2015, at 4:41 AM, Catalin Marinas <catalin.marinas@xxxxxxx> wrote:
> 
> On Fri, Apr 10, 2015 at 02:06:33PM -0500, Kumar Gala wrote:
>> On Apr 10, 2015, at 11:10 AM, Catalin Marinas <catalin.marinas@xxxxxxx> wrote:
>>> On Fri, Apr 10, 2015 at 10:24:46AM -0500, Kumar Gala wrote:
>>>> On Apr 10, 2015, at 5:05 AM, Catalin Marinas <catalin.marinas@xxxxxxx> wrote:
>>>>> On Thu, Apr 09, 2015 at 12:37:06PM -0500, Kumar Gala wrote:
>>>>>> This patch set adds support for SMP boot on the MSM8x16 family of Qualcomm SoCs.
>>>>>> 
>>>>>> To support SMP on the MSM8x16 SoCs we need to add ARMv8/64-bit SCM interfaces to
>>>>>> setup the boot/release addresses for the secondary CPUs.  In addition we need
>>>>>> a uniquie set of cpu ops.  I'm aware the desired methods for booting secondary
>>>>>> CPUs is either via spintable or PSCI.  However, these SoCs are shipping with a
>>>>>> firmware that does not support those methods.
>>>>> 
>>>>> And the reason is? Some guesses:
>>>>> 
>>>>> a) QC doesn't think boot interface (and cpuidle) standardisation is
>>>>> worth the effort (to put it nicely)
>>>>> b) The hardware was available before we even mentioned PSCI
>>>>> c) PSCI is not suitable for the QC's SCM interface
>>>>> d) Any combination of the above
>>>>> 
>>>>> I strongly suspect it's point (a). Should we expect future QC hardware
>>>>> to do the same?
>>>>> 
>>>>> You could argue the reason was (b), though we've been discussing PSCI
>>>>> for at least two years and, according to QC press releases, MSM8916
>>>>> started sampling in 2014.
>>>>> 
>>>>> The only valid reason is (c) and if that's the case, I would expect a
>>>>> proposal for a new firmware interface protocol (it could be PSCI-based),
>>>>> well documented, that can be shared with others that may encounter the
>>>>> same shortcomings.
>>>> 
>>>> Does it matter?  Iâve always felt the kernel was a place of inclusion.
>>>> Qualcomm choose for whatever reason to not use PSCI or spin table.
>>>> You donât like it, I might not like it, but it is what it is.
>>> 
>>> Yes, it matters, but only if Qualcomm wants the SoC support in mainline.
>>> Just because Qualcomm Inc does not want to invest in implementing a
>>> standard firmware interface is not a good enough reason to merge the
>>> kernel code.
>> 
>> The reason to merge the code upstream it expands functionality for a
>> platform.
> 
> This alone has never been a good enough reason. Code (both design and
> style) needs to pass the review.

You havenât really made any comments about the code itself, other than just not liking the firmware interface.

> There is nothing that says when someone licenses an ARM core that they
>> must also implement this standard.
> 
> Just to be perfectly clear: this has nothing to do with ARM Ltd nor the
> ARM hardware licensing terms. ARM Ltd doesn't even require you to use
> Linux, that's your choice. But when you make an OS choice, you have to
> abide by its rules.

But Linux has tended to support hardware as broadly as it can.  Yes it tries to get firmware to improve but there are numerous devices with embedded firmware interfaces that Linux probably would love to change but deals with because they are fixed.

> Qualcomm choose for whatever reasons to not implement it.  There are
>> examples on other architectures supporting non-standard platforms all
>> the time (x86 supported Voyager and SGI VIS for a long time).  As far
>> as I can tell you are just wanting uniformity to impose this rule.
> 
> Don't confuse non-standard hardware (which has always been acceptable on
> ARM) with non-standard ways of entering the kernel from firmware,
> whether it's a primary or secondary CPU. Just look at how many smp_ops
> structures are defined on x86.

When Voyager was supported it had a unique means for SMP bring up.  In the 4.0 kernel MIPS supports 13 different means, PowerPC has 14 different means, ARM has 36 different means.

>>> What if Qualcomm decides that it doesn't like DT, nor ACPI but comes up
>>> with yet another way to describe hardware because that's what the
>>> firmware provides? Should the kernel community take it? You could argue
>>> that this is a significant change but it's about the principle. And each
>>> SoC with its own non-standard boot protocol for no good reason is
>>> significant.
>> 
>> I wouldnât argue that because we are talking about something that has
>> an extremely small impact on the maintainability or changes to how the
>> kernel actually functions.
> 
> It's not about one particular case but about where to draw the line.
> Just multiply this change by the number of SoC variants and you'll no
> longer see this as "extremely small impact". Why would we accept it for
> Qualcomm and reject it for others? It's either giving up trying to
> standardise this altogether or enforcing rules. Since you only care
> about Qualcomm hardware, you don't care about the overall picture.
> 
> By your reasoning, Qualcomm may solely decide to change the booting for
> the primary CPU as well, ignore single Image requirements (well, why
> would Qualcomm care about other SoCs) and so on. The kernel community
> should just accept such changes without questioning because they expand
> platform functionality. I am not asking for *hardware* standardisation
> here, but common software interfaces.
> 
>> Also, if Qualcomm did come up with some other means to replace DT or
>> ACPI and felt it was worth trying to get accepted upstream, I would
>> hope the upstream would look at it before just saying it was not using
>> some standard.
> 
> We would still expect proper technical arguments. That's exactly what
> I'm asking here; is PSCI not suitable for Qualcomm? If not, can it be
> improved? If you have completely different needs, can you come up with a
> standard firmware interface, which may only be used by Qualcomm for the
> time being, but at least guarantee consistency between subsequent
> Qualcomm SoC revisions? But the message you give is pretty much
> "Qualcomm cannot be bothered to explain itself to the kernel community".
> 
> And what makes you think no-one looked at your code (there's an
> unanswered question, asked twice, from Arnd already)? One of the first
> steps in a review is looking at the overall design and asking questions
> if the choices made are not clear. The review is not just about coding
> style or spotting bugs.

Agreed, I need to address Arndâs comment.  However, if there isnât any likehood of getting this code accepted at all to support this device than there really isnât any point.

> Looking beyond this set of patches, I can foresee that you won't care
> about the generic arm64 cpuidle driver either, or more precisely the
> separation between cpuidle subsystem+driver and the SoC-specific
> back-end (cpu_operations).

Thatâs probably true for what I guess are a number of reasons.  Iâm guessing the arm64 cpuidle driver expects PSCI.

>>> It's not Qualcomm Inc maintaining the kernel code but individuals like
>>> you and me who may or may not be sponsored by Qualcomm. And being
>>> sponsored by a company to do kernel maintenance work does not mean that
>>> said company decides what gets merged (on my side, ARM Ltd management is
>>> aware of this, though it's not always easy for the parties involved).
>> 
>> Fair enough, but youâve not given any reasons the code isnât
>> maintainable.  Youâve only said you donât like it because it doesnât
>> use one of the defacto âstandardsâ.
> 
> See above about maintainability and how it no longer scales to tens of
> SoCs on the long term.
> 
>> As you say, its individual doing the maintenance, so those individuals
>> are not likely to have access to change firmware on a given device.
>> So saying go change firmware is pretty much saying we donât want to
>> support your platform or code upstream.
> 
> I don't blame you here for the lack of standard firmware interfaces. But
> it's a message you should have given back to Qualcomm 2-3 years ago.
> Maybe you did and Qualcomm ignored it, which kind of makes it worse.
> 
>>> We haven't really asked for anything difficult like hardware changes,
>>> such decisions are left with Qualcomm. We asked for a standard secure
>>> firmware interface, either an existing one or, if not suitable (with
>>> good arguments), to come up with a proposal for an alternative
>>> _standard_ interface. I don't even have the certitude that the firmware
>>> interface used in these patches would be consistent across Qualcomm
>>> SoCs, let alone being properly documented.
>> 
>> If and when those issues arise we can accept or reject that code.
>> Right now when I look at the impact to supporting this to generic
>> arch/arm64 kernel it is either non-existant if we use the
>> CPU_OF_TABLES or extremely minor if we just add an entry to the
>> supported_cpu_ops struct.
> 
> Again, see above about how such change is no longer small when each SoC
> does the same.

There are so many places that we already deal with per SoC uniqueness.  We setup a software interface in the kernel and people develop towards it.

Are you saying that going forward all SoCs should have the same clocking programming interface to ease portability?  Do you expect them all to have the same pin control programming interface if someone defines a firmware abstraction?

>>> So please come up with proper technical arguments rather than the kernel
>>> should take whatever SoC vendors dreamt of.
>> 
>> There is no technical argument to be made.  This is about the
>> community and you as maintainer wanting to accept code that complies
>> to your decision or not.
> 
> If you are not willing to make technical arguments, I don't have to
> provide any further reasons in this thread. It's your choice. In the
> meantime, the short answer is NAK.

>From my understanding for this given device when the firmware was developed PSCI wasnât available.  Iâm not really familiar with the details.

- k

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/