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

[Geoff Levand <geoffrey.levand@am.sony.com>]

Michael Ellerman wrote:
>> +++ cell--common--6/arch/powerpc/platforms/ps3pf/Makefile
>> @@ -1,2 +1,2 @@
>>  obj-$(CONFIG_PS3PF) += setup.o mm.o smp.o time.o hvcall.o htab.o repository.o
>> -obj-$(CONFIG_PS3PF) += interrupt.o exports.o
>> +obj-$(CONFIG_PS3PF) += interrupt.o exports.o os-area.o
> 
> You don't need CONFIG_PS3PF in this Makefile, it'll only be built if
> CONFIG_PS3PF is already set, just add to obj-y. See the other platform
> Makefiles for example.


Good point, thanks.


>> +enum {
>> +	segment_size = 0x200,
>> +	ldr_format_raw = 0,
>> +	ldr_format_gzip = 1,
>> +	boot_flag_game_os = 0,
>> +	boot_flag_other_os = 1,
>> +	av_multi_out_ntsc = 0,
>> +	av_multi_out_pal_rgb = 1,
>> +	av_multi_out_pal_ycbcr = 2,
>> +	av_multi_out_secam = 3,
>> +	ctrl_button_o_is_yes = 0,
>> +	ctrl_button_x_is_yes = 1,
>> +};
> 
> I know you've got a lot of code to get reviewed and merged, but this
> still irks me. These aren't related constants, so I don't think they
> belong in an enum together, ctrl_button_o_is_yes == boot_flag_game_os ?
> 
> CodingStyle says the labels should be capitalised. 


I'll do that.


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

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


>> + */
>> +
>> +struct header {
>> +	s8 magic_num[16];
> 
> You compare this later to "cell_ext_os_area", so it's not really a magic
> number at all, it's a string?


Same thing, the name from the docs.


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


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


-Geoff