Re: Linux 6.4.4 on m68k - Q40 - pata_falcon causes oops at boot time

[Michael Schmitz <schmitzmic@gmail.com>]

Hi William,

thanks for running these tests!

On 14/08/23 10:24, William R Sowerbutts wrote:
> Hello
>
> Thank you for your interest and patience!
>
> Sorry for the delay in providing more information.  The only method I had to
> transfer test kernels onto the Q40 was painfully slow and involved swapping
> ISA cards which is itself problematic as the pins in one of the ISA slots are
> breaking.
>
> In the end I decided my best option was to dispose of the standard operating
> system (SMSQ/E) and write a new boot ROM for the machine with support for
> FAT32 filesystems on an IDE disk and TFTP transfers using an NE2000 card.
> It's still in development but is now working well enough to use. The source
> code is at https://github.com/willsowerbutts/q40boot
>
> During the process I learned a bit more about the Q40 hardware, and
> transferring a kernel now takes 7 seconds instead of over 20 minutes.
>
> I've run the various tests as requested;
>
> * 5.13.19 -- IDE works (but the data on disk is byte-swapped, see below)
>
> * 5.13.19+44b1fbc0f5f30e66a56d29575349f0b1ebe2b0ee -- IDE fails (crash)
>
> * 5.14.21 -- IDE fails (crash)
>
> * 6.4.10 + Michael's RFC patch -- IDE fails (no crash, but "Bad IO access")

That might be fixed by my second RFC patch, just gone out today.

This one still byte-swaps the data from disk though. I understood that 
was always the case for Q40, but I may have been mistaken there.

>
> * 6.4.10 + my patch -- IDE works, data on disk is NOT byte-swapped
>
> Copies of the boot output for each kernel are attached to this email.
>
> I've also attached the patch I am currently using on my Q40 kernel. It's
> based in part on Michael's patch. It bodges up "ioport_map" to apply the
> offset required to convert raw ISA address into "cookies" and thus allow the
> IDE to work without "Bad IO" complaints. To be crystal clear I am NOT
> proposing this as a solution, it's obviously a hack job, but I wanted to
> illustrate at least one way that *does* work on the Q40.
Yep, my patch is also just meant to find a way to make the driver work 
at all, and then do it again properly (perhaps using 
CONFIG_HAS_IOPORT_MAP).
>
> I think one proper solution might involve enabling CONFIG_HAS_IOPORT_MAP but
> I've not yet looked properly into the implications of doing so. I'm very open
> to alternative solutions too.
>
> As a side note, the NE2000 driver, which is also using 8- and 16-bit ISA I/O,
> works correctly with both 5.13 and 6.4. If one allows it to auto-probe the
> card's interrupt it locks up the machine due to limitations of the interrupt
> controller, but I understand this was always the case; I've made a tiny patch
> to ne.c to prevent it trying this.
I've had autoprobe lockups (rather, interrupts coming in before 
interrupt handling was set up) with ne.c and the ROM port adapter, which 
does not use interrupts. Might be easier to load this driver as a 
module. This may not be an option if you boot the kernel from the 
network card.
>
>
> On Tue, Jul 25, 2023 at 08:26:28AM +1200, Michael Schmitz wrote:
>> Note that this also means that the address hardcoded for IDE may be
>> incorrect (AFAIR, IDE cards could have their IO base reassigned by
>> firmware).
> The W83757 on my ISA IO card, and the W83787IF on the ISA IO card that
> shipped with the machine, are configurable only using jumpers. They can only
> use the two standard base addresses (0x1F0 or 0x170).
>
>
> On Fri, Jul 28, 2023 at 9:52 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> Yes, but Q40 and Falcon are impossible to support any other way, and
>> byte swapping the IDE interface in hardware was never repeated anywhere
>> else, fortunately. We will have to keep pata_falcon around for that
>> cause in any event.
> I wonder if this is a misunderstanding about Q40 IDE.
>
> The Q40 does not have an on-board IDE controller.  IDE is accessed via a
> common ISA PC Super-IO card.  When you do a 16-bit transfer from the ISA I/O
> address the data comes out backwards and the CPU needs to byteswap it.
>
> I found it interesting and quite unexpected that 5.13 read data from the IDE
> drive byte-swapped.  I had prepared a drive for the machine with an MBR
> partition table and an ext4 filesystem but the kernel did not recognise
> either.  After a while I realised I had to byte swap the entire drive with
> 'dd conv=swab', and then it worked.
>
> For my 6.4 patch I enabled byte swapping in the pata_falcon_data_xfer()
> function -- this allows me to use an IDE drive with everything in the
> normal/compatible byte ordering, which makes life much easier. I assume this
> is the intended behaviour.
That would be the sane behaviour, but the designers of both the Falcon 
and apparently the TiVo decided otherwise, wired up the IDE data bus 
byte-swapped and saved the byteswap operations in the driver. I'm unsure 
how this was intended to work on Q40.
> I wonder if any existing Q40 users are actually storing all the data on their
> drives byte-swapped just to avoid the overhead of byte-swapping on every
> read/write. I suppose this would work just fine until you need to connect
> that drive to another machine.

Yes, it does work just fine with that little inconvenience. (When I had 
to fix a corrupted partition table an FAT, I had to hook the Falcon disk 
up to a PC and run ARAnyM on it to get byte-swapped access to the data.)

Now the question is how data on legacy Q40 IDE disks have been stored. 
If it's byte-swapped, we'd better keep that byte order in the current 
driver (meaning your changes to pata_falcon_data_xfer() won't be needed, 
but you would have to swap back data on your disk). If it's always been 
in PC compatible byte order, all data (not just the identify data) must 
be swapped.

I'd like to have Richard's opinion on this (or hear from any other 
former Q40 user).

>
> For my Q40boot ROM I wrote an IDE driver. It just treats the IDE interface as
> a regular legacy PC style IDE interface (which is exactly what it is!) plus
> byte swapping of 16-bit transfers. See the functions q40_ide_read_sector_data
> and q40_ide_write_sector_data in the IDE driver (q40ide.c, q40isa.h in the
> q40boot github repository linked above).
>
> I am going to look at the pata_legacy driver and see if it might be usable on
> the Q40 instead of pata_falcon.
>
> As a long term fix I think I need to read up on CONFIG_HAS_IOPORT_MAP to try
> and understand how I might enable this.  I expect enabling it will have
> implications for all M68K machines, so maybe someone with more experience
> should be making that decision.
>
> I would be very happy to run further tests or to rewrite my hacky patch in
> response to pointers on how to improve it.

If you test my RFC v2 patch, do remember to change 
pata_falcon_data_xfer() to swap all data. I'll have a look at your patch 
to see whether there's anything else I missed or misunderstood.

If you want to handle patch submission yourself, please by all means do 
so. I cannot test any of the Q40 changes, only make sure none of the 
changes modify driver behaviour on Falcon.

Cheers,

     Michael


>
> Thanks
>
> Will
>
>
>
> On Sun, Aug 13, 2023 at 05:38:00PM +1000, Finn Thain wrote:
>> Michael, William,
>>
>> On Sun, 13 Aug 2023, Michael Schmitz wrote:
>>
>>> Am 28.07.23 um 11:47 schrieb Finn Thain:
>>>> On Thu, 27 Jul 2023, Michael Schmitz wrote:
>>>>
>>>>>>> And yes, I'm painfully aware the m68k low level IO code is becoming
>>>>>>> a bit of a maintainance legacy, in no small part due to my hacks
>>>>>>> around Atari EtherNEC.
>>>>>>>
>>>>>> I guess you and I both can share the blame for 44b1fbc0f5f30e66...
>>>>>>
>>>>>> Anyway, you make a good point about on-going maintenance. Do you
>>>>>> think that by supporting standard ISA drivers we might actually
>>>>>> reduce the ideosyncracies in m68k IO code?
>>>>> You and DaveM ought to have a chat about that - abstracting the
>>>>> legacy drivers from the ISA constraints was his preferred option when
>>>>> I last attempted to get the Gayle network driver patches merged. When
>>>>> I say 'preferred', I'm probably understating a little.
>>>>>
>>>> A discussion with maintainers probably won't get far without working
>>>> code to look at. Perhaps William will send an RFC patch to illustrate
>>>> his approach.
>>> Haven't seen anything yet, so I've just sent a patch switching
>>> pata_falcon.c to use the IO resources instead of the memory resources.
>> Thanks for sending that.
>>
>>> Survived compile and ARAnyM boot tests only so far. I've checked and
>>> confirmed the entire 0xffxxxxx range is mapped transparent in head.S for
>>> Q40 so I don't see what else might be missing.
>>>
>> In the message that Geert originally forwarded, William was quoted as
>> saying it was necessary to "fix up the pata_falcon_data_xfer function". He
>> also said that using the IO port resources "causes the ioread8/iowrite8
>> functions to complain loudly".
>> https://lore.kernel.org/linux-m68k/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagVA@mail.gmail.com/
>>
>> I think your patch is taking the same approach and may run into the same
>> issues... I guess we shall see when William tests it.
>>
>>> Please have a look and test if possible. Haven't yet bothered
>>> linux-block or linux-ide... the patch still needs a Fixes: and other
>>> trimmings so isn't ready for submission anyway.
>>>
>> Right. You'll want to run checkpatch.pl on it at some stage.
> _________________________________________________________________________
> William R Sowerbutts                                  will@sowerbutts.com
> "Carpe post meridiem"                               http://sowerbutts.com
>           main(){char*s=">#=0> ^#X@#@^7=",c=0,m;for(;c<15;c++)for
>           (m=-1;m<7;putchar(m++/6&c%3/2?10:s[c]-31&1<<m?42:32));}
>