Re: [PATCH v2 18/20] mips: csrc-r4k: Decrease r4k-clocksource rating if CPU_FREQ enabled

[Serge Semin]

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
>