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.
