Re: [PATCH] RISC-V: Make port I/O string accessors actually work

[Maciej W. Rozycki]

On Fri, 23 Sep 2022, Arnd Bergmann wrote:

> > Fix port I/O string accessors such as `insb', `outsb', etc. which use 
> > the physical PCI port I/O address rather than the corresponding memory 
> > mapping to get at the requested location, which in turn breaks at least 
> > accesses made by our parport driver to a PCIe parallel port such as:
> >
> > PCI parallel port detected: 1415:c118, I/O at 0x1000(0x1008), IRQ 20
> > parport0: PC-style at 0x1000 (0x1008), irq 20, using FIFO 
> > [PCSPP,TRISTATE,COMPAT,EPP,ECP]
> 
> The patch looks correct to me, but I'm curious: Are you actually
> using a parport device on your system, or just testing it to
> make it work?

 I do, e.g. I have a MIPS Malta development board that uses an otherwise 
standard Super I/O parallel port for firmware downloads and have had a 
need recently to use it in the course of addressing a firmware bug.  This 
is how I actually triggered this bug; the Oops is from a download attempt.

 For the time being I resorted to using the only other parallel port I 
have in my lab, which is in my 25 years old x86 box, but I'd rather rely 
on a fast modern machine that is subject to less risk to fail.  And I 
found my HiFive Unmatched board a perfect fit for this purpose.

 I have a pair of Grammar Engine ROM emulators too that have both a serial 
and a parallel port for the purpose of image downloads, either of which 
can be used, except that the parallel interface is lightning fast compared 
to the serial one (so possibly the best arrangement is to use the serial 
port for control and the parallel one for data).  I haven't used them for 
a while now, but there's another firmware bug I plan to get at sometime, 
with my Broadcom SWARM development board.

 So yes, I have an ongoing interest in working parallel ports.

> > +#define outsb(addr, buffer, count) __outsb(PCI_IOBASE + (addr), 
> > buffer, count)
> > +#define outsw(addr, buffer, count) __outsw(PCI_IOBASE + (addr), 
> > buffer, count)
> > +#define outsl(addr, buffer, count) __outsl(PCI_IOBASE + (addr), 
> > buffer, count)
> > 
> 
> I don't see anything actually risc-v specific in these definitions,
> and it would be great to make the asm-generic version do the
> right thing here. As far as I can tell, the only difference is
> the barriers in the risc-v version, and we should really have the
> same in the generic code anyway.

 Fine with me as the next step, but we need to fix the bug at hand first 
and get the fix backported with the RISC-V port.

 Thanks for your review!

  Maciej