[PATCH] pci: Disable I/O forwarding during autoconfiguration if unsupported
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 */
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
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);}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
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);}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
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!
participants (3)
-
Pali Rohár -
Stefan Roese -
Tom Rini