[v4] powerpc/pci: Assign fixed PHB number based on device-tree properties

[Guilherme G. Piccoli]

On 05/25/2016 02:45 AM, Michael Ellerman wrote:
> Hi Guilherme,
>
> Sorry for the very late reply, this got lost in my email filters.

No problem Michael, thanks for replying!


> On Mon, 2016-03-28 at 09:36 -0300, Guilherme G. Piccoli wrote:
>> On 03/25/2016 06:33 AM, Michael Ellerman wrote:
>
>>>> +static int get_phb_number(struct device_node *dn)
>>>> +{
>>>> +	const __be64 *prop64;
>>>> +	const __be32 *regs;
>>>> +	int phb_id = 0;
>>>> +
>>>> +	/* try fixed PHB numbering first, by checking archs and reading
>>>> +	 * the respective device-tree property. */
>>>> +	if (machine_is(pseries)) {
>>>
>>> Firstly I don't see why this check needs to be conditional on pseries. Any
>>> machine where the PHB has a 'reg' property should be able to use 'reg' for
>>> numbering.
>>
>> This is something I'm not sure for all the powerpc sub-architectures,
>> like Cell - that's the reason of the check. If you are sure about this,
>> I'll gladly remove this check =)
>
> Please do.
>
> I'll test on Cell & other platforms. If there are bugs we can fix them. Maybe
> if we can't get it to work on eg. Cell then we need a machine_is() check, but
> that should be the last resort.
>
>>>> +		regs = of_get_property(dn, "reg", NULL);
>>>> +		if (regs)
>>>> +			return (int)(be32_to_cpu(regs[1]) & 0xFFFF);
>>>
>>> This should use of_property_read_u32().
>
> You missed this in v5 ^
>
>>> And finally in either case above, where you get a number from the device tree,
>>> you must check that it's not already allocated. Otherwise if you have a system
>>> where some PHBs have a property but others don't, you may give out the same
>>> number twice. Also you could have firmware give you the same number twice
>>> (which would be a firmware bug, but those happen).
>>>
>>> If the number is allocated you fall back to dynamic numbering.
>>>
>>> If it's not allocated you must mark it as allocated in the bitmap.
>>
>> Hmm..interesting. I thought in performing such check, but I wasn't able
>> to imagine a system in which we can have some PHBs indexed by
>> device-tree properties and others don't, seemed impossible to me. The
>> buggy fw case is an example, I can implement this modification if you
>> think it's valid.
>>
>> But, notice that for consistency in implementation, I'll might need to
>> increase the MAX_PHBS value to 65536, otherwise we won't cover all the
>> possible wrong cases, since I'm performing an AND with 0xFFFF mask
>> (imagine if we can have a buggy fw exposing same value for two different
>> PHBs, and this value is higher than 8192). What do you think about this?
>
> Yeah please increase the bitmap size to 65536. It will only take 8KB of memory,
> which is negligible.

Well, since I sent a v6 and you replied there too, I guess we can 
continue our iterations there - mostly suggestions (all except one) you 
gave here were implemented in v6.

Thanks,


Guilherme

>
> cheers
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>