On Wed, May 20, 2020 at 03:12:02PM +0300, Serge Semin wrote:
> On Tue, May 19, 2020 at 05:50:53PM +0200, Thomas Bogendoerfer wrote:
> > On Mon, May 18, 2020 at 11:57:52PM +0300, Serge Semin wrote:
> > > On Mon, May 18, 2020 at 06:32:06PM +0200, Thomas Bogendoerfer wrote:
> > > > On Mon, May 18, 2020 at 04:48:20PM +0300, Serge Semin wrote:
[nip]
> > > > > ├─>[PATCH v2 13/20] mips: early_printk_8250: Use offset-sized IO-mem accessors
> > > >
> > > > that's broken. A reg shift of 2 doesn't mean we could use 32bit access
> > > > to the registers on other platforms. As I don't think adding some ifdefery
> > > > makes things nicer, just implement the your prom_putchar in board code.
> > >
> > > I thought about that initially, but then I decided to alter the generic
> > > early_printk_8250 code instead. My version of prom_putchar() would be almost
> > > the same as one implemented in the early_printk_8250 module except minor
> > > modification of replacing readb/writeb methods with readl/writel. So I didn't
> > > want to duplicate the code, but wanted to provide a general way to fix the
> > > problem potentially also for another platforms.
> > >
> > > Since you don't like this fix alternatively I'd suggest to add the reg_width
> > > parameter passed to the setup_8250_early_printk_port() method like this:
> > > -setup_8250_early_printk_port(unsigned long base, unsigned int reg_shift,
> > > - unsigned int timeout)
> > > +setup_8250_early_printk_port(unsigned long base, unsigned int reg_shift,
> > > + unsigned int reg_width, unsigned int timeout)
> > >
> > > By reg_width parameter we could determine the actual width of the register:
> > > static inline u8 serial_in(int offset)
> > > {
> > > - return readb(serial8250_base + (offset << serial8250_reg_shift));
> > > + u8 ret = 0xFF;
> > > +
> > > + offset <<= serial8250_reg_shift;
> > > + switch (serial8250_reg_width) {
> > > + case 1:
> > > + ret = readb(serial8250_base + offset);
> > > + break;
> > > + case 2:
> > > + ret = readw(serial8250_base + offset);
> > > + break;
> > > + case 4:
> > > + ret = readl(serial8250_base + offset);
> > > + break;
> > > + default:
> > > + break;
> > > + }
> > > +
> > > + return ret;
> > > }
> > >
> > > The similar modification will be implemented for serial_out(). I'll also modify
> >
> > look at the lines of code you are adding. Doing your own prom_putchar will
> > probably have less lines.
> >
> > > What do you think about this?
> >
> > please do your own prom_putchar.
>
> One more time regarding this problem but in appliance to another part of the
> MIPS code. I've missed the patch to draw your attention to:
> [PATCH v2 14/20] mips: Use offset-sized IO-mem accessors in CPS debug printout
>
> There I've applied the same fix as in the patch:
> [PATCH v2 13/20] mips: early_printk_8250: Use offset-sized IO-mem accessors
>
> Since you don't like the way I initially fixed it, suppose there we don't have
> another way but to introduce something like CONFIG_MIPS_CPS_NS16550_WIDTH
> parameter to select a proper accessors, like sw in our case, and sb by defaul).
> Right?
>
> (Note UART_L is incorrectly created in that patch, I'll remove that macro in
> v3.)
Hm, actually no. The macro is correct. According to the code _mips_cps_putc()
always perform lw from the UART MMIO registers. This must be a bug. Don't you
think?
-Sergey
>
> -Sergey
>
