Message-ID: <CAL_JsqLn+x0JCkjwABU7XkPsgvyVdiCRq=gZTfUBbbR4-c+1bQ@mail.gmail.com>
Date:   Thu, 7 Jun 2018 10:54:52 -0500
From:   Rob Herring <robh+dt@...nel.org>
To:     Michel Pollet <michel.pollet@...renesas.com>
Cc:     Frank Rowand <frowand.list@...il.com>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>,
        Simon Horman <horms@...ge.net.au>,
        Michel Pollet <buserror+upstream@...il.com>,
        Mark Rutland <mark.rutland@....com>,
        Phil Edworthy <phil.edworthy@...esas.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Stefan Wahren <stefan.wahren@...e.com>,
        Magnus Damm <magnus.damm@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Douglas Anderson <dianders@...omium.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Carlo Caione <carlo@...lessm.com>,
        Andreas Färber <afaerber@...e.de>,
        Frank Rowand <frank.rowand@...y.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

On Thu, Jun 7, 2018 at 1:59 AM, Michel Pollet
<michel.pollet@...renesas.com> wrote:
> On 06 June 2018 22:53, Frank wrote:
>> On 06/06/18 14:48, Frank Rowand wrote:
>> > On 06/05/18 23:36, Michel Pollet wrote:
>> >> Hi Frank,
>> >>
>> >> On 05 June 2018 18:34, Frank wrote:
>> >>> On 06/05/18 04:28, Michel Pollet wrote:
>> >>>> The Renesas R9A06G032 second CA7 is parked in a ROM pen at boot
>> >>>> time, it requires a special enable method to get it started.

[...]

>> >>>> + * The second CPU is parked in ROM at boot time. It requires
>> >>>> +waking it after
>> >>>> + * writing an address into the BOOTADDR register of sysctrl.
>> >>>> + *
>> >>>> + * So the default value of the "cpu-release-addr" corresponds to
>> >>> BOOTADDR...
>> >>>> + *
>> >>>> + * *However* the BOOTADDR register is not available when the
>> >>>> +kernel
>> >>>> + * starts in NONSEC mode.
>> >>>> + *
>> >>>> + * So for NONSEC mode, the bootloader re-parks the second CPU into
>> >>>> +a pen
>> >>>> + * in SRAM, and changes the "cpu-release-addr" of linux's DT to a
>> >>>> +SRAM address,
>> >>>> + * which is not restricted.
>> >>>
>> >>> The binding document for cpu-release-addr does not have a definition
>> >>> for 32 bit arm.  The existing definition is only 64 bit arm.  Please
>> >>> add the definition for 32 bit arm to patch 1.
>> >>
>> >> Hmmm I do find a definition in
>> >> Documentation/devicetree/bindings/arm/cpus.txt -- just under where I
>> >> added my 'enable-method' -- And it is already used as 32 bits in at
>> >> least arch/arm/boot/dts/stih407-family.dtsi.
>> >
>> > If the correct answer is for cpu-release-addr to be 64 bits in certain
>> > cases (that discussion is ongoing further downthread) then one
>> > approach to maintain compatibility _and_ to fix the devicetree source
>> > files is to change the source code that currently gets
>> > cpu-release-addr as a
>> > 32 bit object to check the size of the property and get it as either a
>> > 32 bit or 64 bit object, based on the actual size of the property in
>> > the device tree and then change the value in the devicetree source
>> > files to be two cells.  BUT this does not consider the bootloader
>> > complication.  arch/arm/boot/dts/axm5516-cpus.dtsi has a note "//
>> > Fixed by the boot loader", so the boot loader also has to be modified
>> > to be able to handle the possibility that the property could be either
>> > 32 bits or 64 bits.  I don't know how to maintain compatibility with
>> > the boot loader since we can't force it to change synchronously with
>> > changes in the kernel.
>> >
>> > You can consider this comment to be a drive-by observation.  I think
>> > Rob and Geert and people like that are likely to be more helpful with
>> > what to actually do, and you can treat my comment more as pointing out
>> > the issue than as providing the perfect solution.
>>
>> Darn it, hit <send> too quickly.
>>
>> I meant to mention that there are several devicetree source files that have a
>> single cell value for cpu-release-addr, and thus potentially face the same
>> situation, depending on what the final decision is on the proper size for cpu-
>> release-addr. As of v4.17, a git grep shows one cell values in:
>>
>>   arch/arm/boot/dts/axm5516-cpus.dtsi
>>   arch/arm/boot/dts/stih407-family.dtsi
>>   arch/arm/boot/dts/stih418.dtsi
>
> Yes, I had grepped before I used 32 bits on mine...
>
> Now, what is the decision here? Our bootloader is already modified to set it to 32 bits, so I propose that

And too late to fix the bootloader?

>
> + I change the driver to handle 32 and 64 bits properties

That's fine if you can't fix the bootloader.

> + I add this to the cpu.txt, as a separate patch:
> # On other systems, the property can be either
>   32 bits or 64 bits, it is the driver's responsibility
>   to deal with either sizes.

That is definitely not what we want to say. Use of 32-bit should be
considered out of spec. Yes, we have a few platforms in that category,
but they already handle that themselves. Would be nice to fix them,
but at least the STi platforms don't seem too active.

IMO, we should delete whatever text we can here and at most just refer
to the spec.

Rob

Powered by blists - more mailing lists