[PATCH] pci: Disable I/O forwarding during autoconfiguration if unsupported

[Stefan Roese]

On 12/2/21 00:08, Pali Rohár wrote:
> On Tuesday 30 November 2021 07:09:36 Stefan Roese wrote:
>> On 11/25/21 11:32, Pali Rohár wrote:
>>> If U-Boot does not have any I/O resource for assignment then disable I/O
>>> forwarding in PCI bridge autoconfiguration code. Default initial state of
>>> PCI bridge IO registers is unspecified, therefore they can be in enabled if
>>> U-Boot does not touch them.
>>>
>>> Signed-off-by: Pali Rohár <pali at kernel.org>
>>>
>>> ---
>>>    drivers/pci/pci_auto.c | 8 ++++++++
>>>    1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/pci/pci_auto.c b/drivers/pci/pci_auto.c
>>> index 7e6ee54be087..6e5ed194f247 100644
>>> --- a/drivers/pci/pci_auto.c
>>> +++ b/drivers/pci/pci_auto.c
>>> @@ -265,6 +265,14 @@ void dm_pciauto_prescan_setup_bridge(struct udevice *dev, int sub_bus)
>>>    				      (pci_io->bus_lower & 0xffff0000) >> 16);
>>>    		cmdstat |= PCI_COMMAND_IO;
>>> +	} else {
>>> +		/* Disable I/O if unsupported */
>>> +		dm_pci_write_config8(dev, PCI_IO_BASE, 0xf0 | io_32);
>>> +		dm_pci_write_config8(dev, PCI_IO_LIMIT, 0x0 | io_32);
>>
>> Does it perhaps make sense to add / use a macro for this 0xf0 above?
> 
> For setting range bits we already have macro PCI_IO_RANGE_MASK.
> 
> So for setting all base range bits to one (which is this patch is
> suppose to do) I can write something like this: (~0 & PCI_IO_RANGE_MASK)
> or (0xff & PCI_IO_RANGE_MASK). (write_config8 denotes that we use 8bit
> numbers...)
> 
> I/O forwarding is disabled when base address is higher than limit
> address and it is common to choose base address as the highest possible
> (hence all ones) and limit address to lowest possible (hence all zeros).
> 
> So question is, do we need macro which evaluates to number with all
> zeros and another macro which evaluates to number with all ones?
> 
> I'm not sure. For me is "0xf0 | io_32" more readable as "the verbose
> usage with macro" "(0xff & PCI_IO_RANGE_MASK) | io_32".

I have no hard feeling here to really change this - was just checking
here.

> But this is all just about "naming conventions" and "coding style". If
> you think that for consistency is better to create macro or use another
> construction, please let me know other ideas. I would follow any style
> which is expected here...

You already have my RB tag, so all is fine IMHO.

Thanks,
Stefan

> 
>> Other than this:
>>
>> Reviewed-by: Stefan Roese <sr at denx.de>
>>
>> Thanks,
>> Stefan
>>
>>
>>> +		if (io_32 == PCI_IO_RANGE_TYPE_32) {
>>> +			dm_pci_write_config16(dev, PCI_IO_BASE_UPPER16, 0x0);
>>> +			dm_pci_write_config16(dev, PCI_IO_LIMIT_UPPER16, 0x0);
>>> +		}
>>>    	}
>>>    	/* Enable memory and I/O accesses, enable bus master */
>>>
>>
>> Viele Grüße,
>> Stefan Roese
>>
>> -- 
>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de