Re: [PATCH 14/16] powerpc: add ps3 platform OS params support

[Michael Ellerman <michael@ellerman.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2442 bytes --]

On Sun, 2006-11-12 at 19:19 -0800, Geoff Levand wrote:
> > Is it an offset (from something) or a segment number?
> > 
> >> + * @ldr_format: ldr_format flag.
> >> + * @ldr_size: Size of bootloader image in bytes.
> > 
> > If these three all describe the same thing, the bootloader, it'd be good
> > if the names were similar, eg: bootloader_offset, bootloader_format,
> > bootloader_size.
> 
> 
> These names came from the docs.  I think it would be best to keep them the
> same to avoid confusion.

I'm not sure I like that argument, if we followed it throughout the
kernel we'd have one unholy mess, but I'm not that fussed.

> 
> It is not so convenient to view, but the other os area is documented in the
> iso image CELL-Linux-CL_20061110-ADDON.iso at
> http://ftp.uk.linux.org/pub/linux/Sony-PS3/.

Any chance we can get that in some sort of non-insane format? Like a
PDF? :)
 
> >> +static int __init verify_header(const struct header *header)
> >> +{
> >> +	if (memcmp(header->magic_num, "cell_ext_os_area", 16)) {
> >> +		pr_debug("%s:%d magic_num failed\n", __func__, __LINE__);
> >> +		return -1;
> >> +	}
> >> +
> >> +	if (header->hdr_version != 1) {
> >> +		pr_debug("%s:%d hdr_version failed\n", __func__, __LINE__);
> >> +		return -1;
> >> +	}
> > 
> > Is version 2 not going to be backward compatible? Could it be >= 1 ?
> 
> 
> I have absolutely no clue what the next version number will be, nor the
> compatibility, etc.  I'll set this when the version changes.

Except you'll have trouble changing it on the installed base of millions
of PS3s running Linux :) - At the moment if firmware bump the version on
you, you've got an unbootable system until the user does a kernel
upgrade. But it's your baby.

> 
> 
> >> +	dump_header(header);
> >> +	dump_params(params);
> >> +
> >> +	os_params.rtc_diff = params->rtc_diff;
> >> +	os_params.av_multi_out = params->av_multi_out;
> >> +	if (0) { /* currently not used */
> > 
> > Why not?
> 
> 
> The drivers aren't ported yet.  I suppose I could take this out though.

Not fussed, but if you leave it in, expand the comment so no one else
needs to ask :)

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]