Re: [hatari-devel] DSP patch suggestion

[Andreas Grabher <andreas_g86@xxxxxxxxxx>]

> Am 03.12.2023 um 12:32 schrieb Andreas Grabher <andreas_g86@xxxxxxxxxx>:
> 
> 
>> Am 03.12.2023 um 12:09 schrieb Nicolas Pomarède <npomarede@xxxxxxxxxxxx>:
>> 
>> Le 03/12/2023 à 11:09, Andreas Grabher a écrit :
>>> Hello all,
>>> I created a new patch that will cleanly merge with Hatari. It contains these changes:
>>> Changes without (noticeable) effect:
>>> * dsp.c: Changing "P ram" to "P rom“ in spaces[2][1]. This element of
>>>   the array is unused by Hatari and the change therefore has zero
>>>   effect but makes things future proof in case one wants to add the
>>>   bootstrap ROM later.
>>> * dsp.c: Moving the check for program space above the check for
>>>   internal RAM. This change has no functional effect but makes things
>>>   future proof in case one wants to add the bootstrap ROM later. Very
>>>   minor side effect is that one additional condition is checked in
>>>   case of X and Y internal RAM access. Effect on performance is not
>>>   noticeable (if one wants to optimise for performance many other
>>>   things could be improved). No other accesses are affected.
>>> * dsp_core.c: Removing duplicate HSR initialisation. This has zero effect.
>>> * dsp_core.c: Moving dsp_host_interrupt initialisation to a different
>>>   place inside the function. This is just cleaning and has zero effect.
>>> * dsp_core.h: Adding rom[2] (P ROM). This change has no functional
>>>   effect but makes things future proof in case one wants to add the
>>>   bootstrap ROM later.
>>> * dsp_cpu.c: Moving check for program space above check for internal
>>>   RAM. See comments in point 2.
>>> Changes with effect:
>>> * dsp_core.c: Adding host interface initialisation. This function of
>>>   the DSP was not simulated previously but is required for some DSP
>>>   code on the NeXT. I don’t know if any Hatari program makes use of
>>>   this function but I think it should be included anyway to match real
>>>   DSP more closely. I highly recommend this addition.
>>> * dsp_core.c: Adding early abort of the hacked bootstrap loader. The
>>>   real bootstrap loader permanently checks HF0. If HF0 is set, it
>>>   exits early even if a limited amount of bytes have been transferred.
>>>   NeXT makes heavy use of this feature. This is required to accurately
>>>   simulate the function of the bootstrap loader programmed to the real
>>>   DSP. This addition is highly recommended and has zero negative side
>>>   effects.
>>> * dsp_core.c: The real bootstrap loader sets R0 to the amount of bytes
>>>   copied and OMR to 2. This needs to be added to accurately simulate
>>>   the bootstrap program and match real DSP. This change is highly
>>>   recommended and has zero negative side effects.
>>> * dsp_core.c: Adding host received data interrupt. This is missing
>>>   from the current Hatari code. It must be a mistake. This is highly
>>>   recommended to match real DSP.
>>> Changes not included:
>>> * DMA interface: Atari obviously does not use the DMA interface of the
>>>   DSP host port. Therefore I didn’t include my implementation of it.
>>> * Bootstrap ROM: This feature is not necessary. It is more a question
>>>   of wanting to avoid using hacks (dsp_core.running et al). I did not
>>>   include this in my patch because it requires extra CPU power without
>>>   functional benefits.
>>> * Starting with different modes: The DSP has 4 different modes. Hatari
>>>   is hard coded to always use mode 2 (normal expanded mode). Real
>>>   simulation of the modes would be required for using bootstrap ROM.
>>>   Although in theory there could be other programs making use of this,
>>>   there seem to be none on the NeXT and probably also none on Atari.
>>>   Therefore I did not include this.
>>> There seem to be a few white space issues, because tabs are used between code and comments and because changing from SDL types to normal fixed length types messed things a bit. If you want me to fix this, I can provide a patch (replacing tabs with spaces).
>>> Feel free to ask any questions.
>> 
>> Hi
>> 
>> thanks for your patch.
>> since it implies several changes, could it be possible for you to split it at least in two ? one patch for not noticeable effect and one for possible effects ?
>> 
>> I understand these work under Previous, but just in case it breaks sthg in Hatari for some games/demos, it would make bisescting easier if we don't commit one single whole patch.
>> 
>> If not possible, the single patch can be applied anyway as there's no one really working on dsp in hatari at the moment, any improvement/fix should be merged.
>> 
>> Nicolas
>> 
> Yes, of course! I have split it into three patches. And yes, the functional changes (patch „improving“) have been part of Previous for a long time and have not caused any regressions. On the contrary, they were necessary to solve problems. Before making changes I always checked the data sheet of the 56001 to confirm that my changes match the specifications.
> 
> Regards
> Andreas
> 
> <dsp_patch_improving.diff><dsp_patch_cleaning.diff><dsp_patch_futureproofing.diff>

I just recognised that my patches had the wrong level and different directory names. Appended patches are fixed. They can be applied with patch -p1.

Attachment: dsp_patch_cleaning.diff
Description: Binary data

Attachment: dsp_patch_futureproofing.diff
Description: Binary data

Attachment: dsp_patch_improving.diff
Description: Binary data