[OpenBIOS] [PATCH 2/3] fw_cfg: add fw_cfg_find_file() and fw_cfg_read_file() functions

[Mark Cave-Ayland]

On 20/03/17 09:21, Segher Boessenkool wrote:

> On Mon, Mar 20, 2017 at 09:04:49AM +0000, Mark Cave-Ayland wrote:
>>>> +    uint32_t buf, count;
>>>> +
>>>> +    fw_cfg_read(FW_CFG_FILE_DIR, (char *)&buf, sizeof(uint32_t));
>>>> +    count = __be32_to_cpu(buf);
>>>
>>> It's a lot cleaner to use e.g.
>>>
>>> 	count = fw_cfg_read32(FW_CFG_FILE_DIR);
>>
>> Yeah I know what you mean here. The reason for the above was that the
>> existing registers are LE, except for the fw_cfg API which is BE. And
>> since this was the only BE I just added it straight in...
>>
>> I'm not sure I'm too excited about splitting this into a separate
>> function
> 
> You can make it inline if you are worried about performance.

I'm not really worried about the performance, just really trying to
minimise the code churn for something that only has one caller. And if
someone else does need it at a later date, it's fairly easy to refactor out.

>> although I do see that I'm missing a couple of __be*_to_cpu()
>> wrappers in this patch. I'll do a resend later.
> 
> Yeah, you cannot make such mistakes if you do all accesses with
> explicit endianness ;-)

Indeed, although coincidentally both SPARC and PPC are BE which is why I
didn't notice this in my local tests :)

>>> and ideally you wouldn't use byte-swapping things at all:
>>>
>>> uint32_t fw_cfg_read32(unsigned char *p)
>>> {
>>> 	uint32_t x = 0;
>>> 	for (int i = 0; i < 4; i++_
>>> 		x = (x << 8) | p[i];
>>> 	return x;
>>> }
>>
>> This still wouldn't handle the case of mixed LE and BE accesses though?
> 
> Are there both LE and BE things in fw_cfg?  read_be32 etc. then?

Oh yes - this is where it gets really fun. The original fw_cfg interface
(cmd < 0x20) is LE, while the fw_cfg file API uses BE. And to make this
even more fun, SPARC fw_cfg accesses are done via ioport accesses which
are LE, while PPC fw_cfg accesses are done via MMIO which is BE.


ATB,

Mark.