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

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

[Pali Rohár]

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@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);
+		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 */
-- 
2.20.1

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

[Stefan Roese]

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@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?

Other than this:

Reviewed-by: Stefan Roese <sr@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@denx.de

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

[Pali Rohár]

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

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

> Other than this:
> 
> Reviewed-by: Stefan Roese <sr@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@denx.de

Re: [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@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@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@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@denx.de

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

[Tom Rini]

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

On Thu, Nov 25, 2021 at 11:32:43AM +0100, 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@kernel.org>
> Reviewed-by: Stefan Roese <sr@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]