Re: Gayle support reworked

[Radoslaw Kujawa <radoslaw.kujawa%c0ff33.net@localhost>]

Hi.

On 3 Jan 2014, at 13:46, Michael van Elst <mlelstv%serpens.de@localhost> wrote:

> radoslaw.kujawa%c0ff33.net@localhost (Radoslaw Kujawa) writes:
> 
>> On 3 Jan 2014, at 07:21, Michael van Elst <mlelstv%serpens.de@localhost> 
>> wrote:
> 
>>> radoslaw.kujawa%c0ff33.net@localhost (Radoslaw Kujawa) writes:
>>> 
>>>> I’ve reworked[1] the support for Gayle chip. This has implications for:
>>>> - A600/1200 internal IDE (wdc at mainbus)
>>>> - A4000 internal IDE (wdc at mainbus)
>>>> - FastATA 1200 Mk-III/Mk-IV (efa at mainbus)
>>>> - ACA500 Compact Flash (wdc at acafh)
>>> 
>>> I don't think that's the right approach. How many Gayle chips do you count
>>> in an A4000?
> 
>> The A4000 has IDE part of Gayle, it’s essentially the same, but not the 
>> whole Gayle chip.
> 
> The IDE part is just a set of TTL driver chips and the "interrupt status
> register" where the single used bit corresponds to the equivalent status
> bit in Gayle (but which does not require an acknowledgement cycle).

Agreed.

>> I intend to rewrite the wdc at mainbus attachment, to remove the gayle 
>> abstraction layer dependency for A4000 (and therefore simplifying gayle.c 
>> too). Since everything besides IDE part and interrupt status register is not 
>> needed there.
> 
> The WDC attachment did not use an abstraction layer.

It couldn’t use something that didn’t exist. I mean that now I want to remove 
this dependency I introduced myself for A4000, which as you pointed out wasn’t 
that good idea. Though I want to keep it for A600/A1200.

> For A4000 it just
> accessed the A4000 specific interrupt status register, for A1200 it used
> the gayle register (which requires an extra acknowledgement cycle to
> clear the interrupt condition).

True, but as I said, I want to do it this way again (not to touch gayle related 
code for A4000).

>>> Then why pretending that one exists?
> 
>> You can only count one Gayle chip in a given amiga at the time. So (with the 
>> exception above) it’s not “pretending” that one exists, it’s using the chip 
>> from different drivers.
> 
> Every A1200 has the Gayle chip, no A4000 has one.

Maybe it would be a good idea to split wdc_amiga into wdc_a1200 and wdc_a4000 ? 
Especially that I want to add support for IDE doublers (like IDEfix, EIDE ’99), 
which is A600/A1200 specific. This would further complicate the driver.

> The FastATA hardware for A1200 seems to abuse the Gayle chip (no idea why,
> probably some cute compatibility hack).

True, it does so to allow booting from FastATA (which looks like an ordinary 
Gayle IDE until the driver is loaded). And also it uses the interrupt line 
normally tied to on-board IDE (and Gayle interrupt status register).

> You need to prevent the wdc_amiga
> attachment when this hardware exists.

It is prevented by providing a higher driver priority. Knowing that FastATA 
“replaces” on-board IDE I’m matching “wdc” in the efa_probe(). If the probe 
succeeds efa driver returns higher priority than wdc_amiga. A bit of a hack, 
but I don’t see how to do it in a cleaner way.

> The ACA500 (for A500) has nothing to do with Gayle except that
> GAYLE_IDE_BASE is used in the code, but it doesn't even have a
> compatible interrupt status register.

ACA500 emulates Gayle IDE in hardware and even the vendor states so in the 
documentation. It does have compatible status register and acknowledge 
register. See how it is handled in wdc_acafh_intr(). There is a small 
difference that it does have additional channel, and also separate interrupt 
status registers for each channel. 

> But for all these cases the code now pretends that there is a Gayle chip
> that only differs in bus attachment.

With the exception of A4000, there is a Gayle chip in all these cases (though 
emulated in ACA500).

> IMHO a cleanup would:
> 
> - just define ACAFH_IDE_BASE as a constant and remove any reference to gayle
>  from the acafh code.

Removing reference to gayle would also mean duplicating the interrupt related 
code in acafh.

> - make gayle.c also provide the gayle IDE base address
> - make wdc_amiga.c take constant addresses (ide+intreq) for A4000 and those
>  provided by gayle.c for A1200.
> - make efa.c take address (ide+intreq) provided by gayle.c.

These changes make sense, in terms of holding virtual addresses in the drivers, 
instead of in the gayle layer. But I’d keep the appropriate register access 
functions within gayle.c anyway, since otherwise it would again be duplicated 
in all these drivers.

> I don't think it has any benefit to use busspace routines to access
> the intreq register. This is all machine dependent and everything
> is already accessible via ztwomap.

It has advantage of readability and using a known layer, instead of doing some 
magic. 

Previously it was done this way in all drivers that touched gayle:

gayle.this = foo;
gayle.that = bar;

And gayle was a struct casted directly on VA space. As far as I know, this 
isn’t how we usually write drivers in NetBSD ;).

Having an abstraction layer makes code easier to understand and allows to 
easily enable/add debugging code in case of any Gayle related problems.

-- 
Best regards,
Radoslaw Kujawa