RE: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Michel Pollet]

On 07 June 2018 16:55, Rob wrote:
>
> On Thu, Jun 7, 2018 at 1:59 AM, Michel Pollet
> <michel.pollet@xxxxxxxxxxxxxx> 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?


Well not too late, but read further on...

>
> >
> > + 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.

So actually I didn't use 32 bits by plain chance, I read the cpu.txt file which says
that 64 bits systems use 64 bits property, concluded that in my case I ought to
use 32 bits, then grepped around and found other systems using 32 bits, therefore
I went forward and used it..

Nothing said here that it should be 64 bits everywhere -- So the documentation
needs fixing somehow. Right now it certainly led me wrong.

>
> Rob

Michel




Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.