Here is a set of fixes discovered during my work on adding virtio-blk support to OpenBIOS.
Patch 1 changes the way in which the IDE controller DT nodes are generated so that they use the controller index as the address, and not the value programmed in the PCI BAR. Otherwise it is impossible for QEMU to determine the fw path for IDE boot devices, since the PCI BARs are programmed after the machine has been initialised.
Patches 2 and 3 contain fixes for bugs that were discovered whilst trying to initialise the virtio-blk device for both PPC and SPARC64 architectures.
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk
Mark Cave-Ayland (3): ide: don't use ioports as part of the controller node address libopenbios: don't allow find_package_method() to push a NULL for empty strings pci: use absolute PCI IO addresses when programming bridge IO limits
config/examples/ppc64_config.xml | 2 +- config/examples/ppc_config.xml | 2 +- drivers/ide.c | 18 ++++++++---------- drivers/pci.c | 2 +- libopenbios/bindings.c | 7 ++++++- 5 files changed, 17 insertions(+), 14 deletions(-)
The problem with using the ioport as part of the controller node address is that the address cannot be determined until after the PCI BARs have been programmed.
This causes a problem when trying to generate fw bootpaths because by definition they must be passed to the firmware before PCI initialisation.
Instead of using the controller ioport address, use the controller index for the node address to provide a consistent device node regardless of how the PCI BARs are programmed.
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk --- config/examples/ppc64_config.xml | 2 +- config/examples/ppc_config.xml | 2 +- drivers/ide.c | 18 ++++++++---------- 3 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/config/examples/ppc64_config.xml b/config/examples/ppc64_config.xml index 5f79c21..bcb605e 100644 --- a/config/examples/ppc64_config.xml +++ b/config/examples/ppc64_config.xml @@ -66,7 +66,7 @@ <option name="CONFIG_DRIVER_IDE" type="boolean" value="true"/> <option name="CONFIG_IDE_NUM_CHANNELS" type="integer" value="2"/> <option name="CONFIG_IDE_FIRST_UNIT" type="integer" value="1"/> - <option name="CONFIG_IDE_DEV_NAME" type="string" value="ata-%d"/> + <option name="CONFIG_IDE_DEV_NAME" type="string" value="ata"/> <option name="CONFIG_IDE_DEV_TYPE" type="string" value="ata"/> <option name="CONFIG_DEBUG_IDE" type="boolean" value="false"/> <option name="CONFIG_DRIVER_ADB" type="boolean" value="true"/> diff --git a/config/examples/ppc_config.xml b/config/examples/ppc_config.xml index 3112ea5..43277a0 100644 --- a/config/examples/ppc_config.xml +++ b/config/examples/ppc_config.xml @@ -72,7 +72,7 @@ <option name="CONFIG_DRIVER_IDE" type="boolean" value="true"/> <option name="CONFIG_IDE_NUM_CHANNELS" type="integer" value="2"/> <option name="CONFIG_IDE_FIRST_UNIT" type="integer" value="1"/> - <option name="CONFIG_IDE_DEV_NAME" type="string" value="ata-%d"/> + <option name="CONFIG_IDE_DEV_NAME" type="string" value="ata"/> <option name="CONFIG_IDE_DEV_TYPE" type="string" value="ata"/> <option name="CONFIG_DEBUG_IDE" type="boolean" value="false"/> <option name="CONFIG_DRIVER_ADB" type="boolean" value="true"/> diff --git a/drivers/ide.c b/drivers/ide.c index 274236f..edbd287 100644 --- a/drivers/ide.c +++ b/drivers/ide.c @@ -58,7 +58,7 @@ DECLARE_UNNAMED_NODE( ob_ide_ctrl, INSTALL_OPEN, sizeof(int)); #endif
#ifndef CONFIG_IDE_DEV_NAME -#define DEV_NAME "ide%d" +#define DEV_NAME "ide" #else #define DEV_NAME CONFIG_IDE_DEV_NAME #endif @@ -1457,8 +1457,7 @@ int ob_ide_init(const char *path, uint32_t io_port0, uint32_t ctl_port0,
ob_ide_identify_drives(chan);
- snprintf(nodebuff, sizeof(nodebuff), "%s/" DEV_NAME, path, - current_channel); + snprintf(nodebuff, sizeof(nodebuff), "%s/" DEV_NAME, path); REGISTER_NAMED_NODE_PHANDLE(ob_ide_ctrl, nodebuff, dnode);
chan->ph = dnode; @@ -1469,11 +1468,10 @@ int ob_ide_init(const char *path, uint32_t io_port0, uint32_t ctl_port0, (char *)&props, 2*sizeof(props[0])); #endif
- props[0] = __cpu_to_be32(chan->io_regs[0]); - props[1] = __cpu_to_be32(1); props[2] = __cpu_to_be32(8); - props[3] = __cpu_to_be32(chan->io_regs[8]); - props[4] = __cpu_to_be32(1); props[5] = __cpu_to_be32(2); - set_property(dnode, "reg", (char *)&props, 6*sizeof(props[0])); + props[0] = __cpu_to_be32(current_channel); + props[1] = __cpu_to_be32(0); + props[2] = 0; + set_property(dnode, "reg", (char *)&props, 3*sizeof(props[0]));
IDE_DPRINTF(DEV_NAME": [io ports 0x%x-0x%x,0x%x]\n", current_channel, chan->io_regs[0], @@ -1624,8 +1622,8 @@ int macio_ide_init(const char *path, uint32_t addr, int nb_channels)
ob_ide_identify_drives(chan);
- snprintf(nodebuff, sizeof(nodebuff), "%s/" DEV_NAME, path, - current_channel); + snprintf(nodebuff, sizeof(nodebuff), "%s/" DEV_NAME "-%d", path, + current_channel); REGISTER_NAMED_NODE_PHANDLE(ob_ide_ctrl, nodebuff, dnode);
chan->ph = dnode;
Instead push a valid address with zero length otherwise the Forth find-method word can reference a NULL pointer.
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk --- libopenbios/bindings.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/libopenbios/bindings.c b/libopenbios/bindings.c index 4f7a993..926944d 100644 --- a/libopenbios/bindings.c +++ b/libopenbios/bindings.c @@ -161,7 +161,12 @@ my_self( void ) xt_t find_package_method( const char *method, phandle_t ph ) { - push_str( method ); + if (method == NULL) { + push_str(""); + } else { + push_str( method ); + } + PUSH_ph( ph ); fword("find-method"); if( POP() )
The upper IO bridge limit was being calculated relative to the lower IO bridge limit, rather than as an absolute address.
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk --- drivers/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci.c b/drivers/pci.c index e674cfb..7a174b4 100644 --- a/drivers/pci.c +++ b/drivers/pci.c @@ -1599,7 +1599,7 @@ static void ob_configure_pci_bridge(pci_addr addr, Some drivers (e.g. IDE) will attempt ioport access as part of the configuration process, so we allow them during the secondary bus scan and then set the correct IO limit below. */ - io_scan_limit = *io_base + (0xffff - *io_base); + io_scan_limit = *io_base + 0xffff; pci_config_write16(addr, PCI_IO_LIMIT_UPPER, (io_scan_limit >> 16)); pci_config_write8(addr, PCI_IO_LIMIT, (io_scan_limit >> 8) & ~(0xf));
On Sun, Aug 12, 2018 at 01:55:10PM +0100, Mark Cave-Ayland wrote:
The problem with using the ioport as part of the controller node address is that the address cannot be determined until after the PCI BARs have been programmed.
If it is a PCI device, it should have the configuration space address as its node address. See https://www.openfirmware.info/data/docs/bus.pci.pdf 4.1.1 "reg".
Or is this some weird thing that does not have a PCI function for the IDE?
Segher
On 12/08/18 18:30, Segher Boessenkool wrote:
On Sun, Aug 12, 2018 at 01:55:10PM +0100, Mark Cave-Ayland wrote:
The problem with using the ioport as part of the controller node address is that the address cannot be determined until after the PCI BARs have been programmed.
If it is a PCI device, it should have the configuration space address as its node address. See https://www.openfirmware.info/data/docs/bus.pci.pdf 4.1.1 "reg".
Or is this some weird thing that does not have a PCI function for the IDE?
Ah no, this isn't the PCI IDE device itself, it's the IDE controller nodes that live underneath the PCI IDE device node.
ATB,
Mark.
On 12/08/18 13:55, Mark Cave-Ayland wrote:
Here is a set of fixes discovered during my work on adding virtio-blk support to OpenBIOS.
Patch 1 changes the way in which the IDE controller DT nodes are generated so that they use the controller index as the address, and not the value programmed in the PCI BAR. Otherwise it is impossible for QEMU to determine the fw path for IDE boot devices, since the PCI BARs are programmed after the machine has been initialised.
Patches 2 and 3 contain fixes for bugs that were discovered whilst trying to initialise the virtio-blk device for both PPC and SPARC64 architectures.
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk
Mark Cave-Ayland (3): ide: don't use ioports as part of the controller node address libopenbios: don't allow find_package_method() to push a NULL for empty strings pci: use absolute PCI IO addresses when programming bridge IO limits
config/examples/ppc64_config.xml | 2 +- config/examples/ppc_config.xml | 2 +- drivers/ide.c | 18 ++++++++---------- drivers/pci.c | 2 +- libopenbios/bindings.c | 7 ++++++- 5 files changed, 17 insertions(+), 14 deletions(-)
No further comments, so pushed to master.
ATB,
Mark.
-
Mark Cave-Ayland -
Segher Boessenkool