Re: [OpenBIOS] [PATCH 5/5] ppc: fix up IDE device tree and control transfer

[Mark Cave-Ayland]

On 18/08/15 19:17, Cormac O'Brien wrote:
> Mac OS 9 is finicky about devices having the correct properties, so we change
> some IDE data to more closely model a real Mac. We also add ide_exit() to

Possibly make this "more closely model a real New World Mac" to reflect
that this patch also removes the OLDWORLD() guards in a couple of places?
> ciface_quiesce() to properly prepare the drives for transferring control to
> the operating system.
> Signed-off-by: Cormac O'Brien
>
> arch/ppc/qemu/methods.c |  2 ++
>  drivers/ide.c           | 33 +++++++++++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/arch/ppc/qemu/methods.c b/arch/ppc/qemu/methods.c
> index cb72bc1..adf3ea2 100644
> --- a/arch/ppc/qemu/methods.c
> +++ b/arch/ppc/qemu/methods.c
> @@ -109,11 +109,13 @@ NODE_METHODS( tty ) = {
>
> DECLARE_NODE( ciface, 0, 0, "+/openprom/client-services" );
>
> +extern void ide_exit(void);
>  /* ( -- ) */
>  static void
>  ciface_quiesce( unsigned long args[], unsigned long ret[] )
>  {
>    usb_exit();
> ide_exit();
>
> #if 0
>    unsigned long msr;
>    /* This seems to be the correct thing to do - but I'm not sure */
> diff --git a/drivers/ide.c b/drivers/ide.c
> index 327c64a..a064bb8 100644
> --- a/drivers/ide.c
> +++ b/drivers/ide.c
> @@ -1581,6 +1581,11 @@ int macio_ide_init(const char *path, uint32_t addr, int nb_channels)
>    	set_property(dnode, "compatible", (is_oldworld() ?
>    		     "heathrow-ata" : "keylargo-ata"), 13);
> set_property(dnode, "model", ((current_channel == 3) ?
> 	     "ata-3" : "ata-4"), 6);
> set_property(dnode, "AAPL,connector", "ata", 4);

Slight nit here: can we use the same convention as in the earlier patch
for specifying the property length, e.g. strlen("ata-3") + 1 and
strlen("ata") + 1. I should imagine the overhead should be minimal, and
it's a good pattern to use since it helps reduce off-by-one errors,
similar to one that we found earlier in the project.
> props[0] = 0x00000526;
> props[1] = 0x00000085;
> props[2] = 0x00000025;
>
> @@ -1589,8 +1594,8 @@ int macio_ide_init(const char *path, uint32_t addr, int nb_channels)
>    	props[5] = 0x00000000;
>    	props[6] = 0x00000000;
>    	props[7] = 0x00000000;
> OLDWORLD(set_property(dnode, "AAPL,pio-timing",
> 		      (char *)&props, 8*sizeof(props[0])));
> set_property(dnode, "AAPL,pio-timing",
> 		      (char *)&props, 8*sizeof(props[0]));
>
> /* The first interrupt entry is the ide interrupt, the second
>   the dbdma interrupt */
>
> @@ -1634,8 +1639,8 @@ int macio_ide_init(const char *path, uint32_t addr, int nb_channels)
>    			      (char *)&props, 2*sizeof(props[0])));
> props[0] = 0;
> OLDWORLD(set_property(dnode, "AAPL,bus-id", (char*)props,
> 	 1 * sizeof(props[0])));
> set_property(dnode, "AAPL,bus-id", (char*)props,
> 	 1 * sizeof(props[0]));
> IDE_DPRINTF(DEV_NAME": [io ports 0x%lx]\n",
>            current_channel, chan->mmio);
>
> @@ -1683,3 +1688,23 @@ int macio_ide_init(const char *path, uint32_t addr, int nb_channels)
>    return 0;
>  }
>  #endif /* CONFIG_DRIVER_MACIO */
>
> +void ide_exit(void);
> +void ide_exit(void)
> +{
> struct ide_channel *current;
> current = channels;
> while (current) {
> struct ide_drive *drive;
> int i;
> for (i = 0; i < 2; i++) {
> 	drive = &current->drives[i];
> 	ob_ide_select_drive(drive);
> 	ob_ide_software_reset(drive);
> 	ob_ide_device_type_check(drive);
> }
> current = current->next;
> }
>
> +}

ATB,

Mark.