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

Commit Message

Serge Semin May 6, 2020, 5:42 p.m. UTC

From: Serge Semin <[e-mail]>

Commit 07d69579e7fe ("MIPS: Don't register r4k sched clock when
CPUFREQ enabled") disabled the r4k-clock usage for scheduler ticks
counting due to the scheduler being non-tolerant for unstable
clocks sources. For the same reason the clock should be used
in the system clocksource framework only as a last resort if CPU
frequency may change.

Signed-off-by: Serge Semin <[e-mail]>
Cc: Alexey Malahov <[e-mail]>
Cc: Thomas Bogendoerfer <[e-mail]>
Cc: Paul Burton <[e-mail]>
Cc: Ralf Baechle <[e-mail]>
Cc: Greg Kroah-Hartman <[e-mail]>
Cc: Arnd Bergmann <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
---
 arch/mips/kernel/csrc-r4k.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Thomas Bogendoerfer May 8, 2020, 3:41 p.m. UTC | #1

On Wed, May 06, 2020 at 08:42:36PM +0300, [e-mail] wrote:
> From: Serge Semin <[e-mail]>
> 
> Commit 07d69579e7fe ("MIPS: Don't register r4k sched clock when
> CPUFREQ enabled") disabled the r4k-clock usage for scheduler ticks
> counting due to the scheduler being non-tolerant for unstable
> clocks sources. For the same reason the clock should be used
> in the system clocksource framework only as a last resort if CPU
> frequency may change.
> 
> Signed-off-by: Serge Semin <[e-mail]>
> Cc: Alexey Malahov <[e-mail]>
> Cc: Thomas Bogendoerfer <[e-mail]>
> Cc: Paul Burton <[e-mail]>
> Cc: Ralf Baechle <[e-mail]>
> Cc: Greg Kroah-Hartman <[e-mail]>
> Cc: Arnd Bergmann <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  arch/mips/kernel/csrc-r4k.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/mips/kernel/csrc-r4k.c b/arch/mips/kernel/csrc-r4k.c
> index 437dda64fd7a..d81fb374f477 100644
> --- a/arch/mips/kernel/csrc-r4k.c
> +++ b/arch/mips/kernel/csrc-r4k.c
> @@ -71,7 +71,11 @@ int __init init_r4k_clocksource(void)
>  		return -ENXIO;
>  
>  	/* Calculate a somewhat reasonable rating value */
> +#ifndef CONFIG_CPU_FREQ
>  	clocksource_mips.rating = 200 + mips_hpt_frequency / 10000000;
> +#else
> +	clocksource_mips.rating = 99;
> +#endif

I dislike this patch. Assuming you have an other clocksource, why not
simply disable csrc-r4k, if CPU_FREQ is enabled ?

Thomas.

Serge Semin May 11, 2020, 1:31 p.m. UTC | #2

On Fri, May 08, 2020 at 05:41:50PM +0200, Thomas Bogendoerfer wrote:
> On Wed, May 06, 2020 at 08:42:36PM +0300, [e-mail] wrote:
> > From: Serge Semin <[e-mail]>
> > 
> > Commit 07d69579e7fe ("MIPS: Don't register r4k sched clock when
> > CPUFREQ enabled") disabled the r4k-clock usage for scheduler ticks
> > counting due to the scheduler being non-tolerant for unstable
> > clocks sources. For the same reason the clock should be used
> > in the system clocksource framework only as a last resort if CPU
> > frequency may change.
> > 
> > Signed-off-by: Serge Semin <[e-mail]>
> > Cc: Alexey Malahov <[e-mail]>
> > Cc: Thomas Bogendoerfer <[e-mail]>
> > Cc: Paul Burton <[e-mail]>
> > Cc: Ralf Baechle <[e-mail]>
> > Cc: Greg Kroah-Hartman <[e-mail]>
> > Cc: Arnd Bergmann <[e-mail]>
> > Cc: Rob Herring <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > ---
> >  arch/mips/kernel/csrc-r4k.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/arch/mips/kernel/csrc-r4k.c b/arch/mips/kernel/csrc-r4k.c
> > index 437dda64fd7a..d81fb374f477 100644
> > --- a/arch/mips/kernel/csrc-r4k.c
> > +++ b/arch/mips/kernel/csrc-r4k.c
> > @@ -71,7 +71,11 @@ int __init init_r4k_clocksource(void)
> >  		return -ENXIO;
> >  
> >  	/* Calculate a somewhat reasonable rating value */
> > +#ifndef CONFIG_CPU_FREQ
> >  	clocksource_mips.rating = 200 + mips_hpt_frequency / 10000000;
> > +#else
> > +	clocksource_mips.rating = 99;
> > +#endif
> 
> I dislike this patch. Assuming you have an other clocksource, why not
> simply disable csrc-r4k, if CPU_FREQ is enabled ?

Me neither and the best way would be to update the clocksource frequency
dynamically the same way it's done for cevt-r4k and MIPS GIC timers. Alas the
clocksource doesn't support it. Due to this together with CPU-freq facility
enabled we have to use a very slow DW APB Timer instead of the fast embedded
into the CPU core r4k and MIPS GIC timers. Just note the difference: it takes
220 ns to read the counter from DW APB Timer in comparison to a few nanoseconds
reading from MIPS GIC and R4K. So IMO disabling the timer as you suggest isn't
the best option. By making the CPUFREQ and CSRC_R4K mutual exclusive we'd
assume a use-case that the system will always use the CPU-freq facility changing
the CPU reference frequency. This is obviously not true. Noone prevents the
system administrator to leave the default setting of the CPU-freq with fixed
frequency and select a faster, more accurate timer like in our case.

My idea was not to try to predict how the system would be used, but to let the
system administration to choose which timer is applicable in particular usecase
enabling a safest one by default. So if CPUFREQ is available, then we fallback
to the external timer as safest one. If the system user wouldn't need to have
the CPUFREQ facility utilized, then the system administrator would want to
leave the default CPU-freq governor with pre-defined CPU frequency and
select either R4K (MIPS) or MIPS GIC timer just by writing its name into
/sys/bus/clocksource/devices/clocksource0/current_clocksource .
 
I should note, that currently CPU_FREQ won't be available if there is no
MIPS_EXTERNAL_TIMER available for the platform. It's prohibited by means of the
conditional kbuild config inclusion declared in the arch/mips/Kconfig:
+ if CPU_SUPPORTS_CPUFREQ && MIPS_EXTERNAL_TIMER
+ source "drivers/cpufreq/Kconfig"
+ endif
So if there is no external timer working independently from the CPU core clock
source, the CPUFREQ won't be available to select for the kernel. Though currently
this limitation is supposed to be applicable for the R4K/MIPS GIC clocksource
timers only since clockevents must work fine in unstable reference clock conditions.

So what can we do to improve the patch? First one is a solution I suggested in
this patch but it could be a bit altered by using IS_ENABLED() macro to:
+ clocksource_mips.rating = !IS_ENABLED(CONFIG_CPU_FREQ) ?
+			    200 + mips_hpt_frequency / 10000000 : 99;

Another idea I discovered when have been searching through the x86 arch code.
x86's got the same problem with TSC timer, but it doesn't disable it if
CPU-frequency is switched on. Instead it just marks it as unstable by calling
the clocksource_mark_unstable() method if CPU frequency changes. I suggest to
implement the same approach in our case of MIPS GIC (another patchset
I've sent, see "clocksource: Fix MIPS GIC and DW APB Timer for Baikal-T1 SoC
support" in your email client) and R4K timers. We'll subscribe to the CPU
frequency change and if it changes we'll call clocksource_mark_unstable() with
MIPS GIC and R4K clocksource handlers passed. This shall reduce their rating and
cause selecting a clocksource with better one. BTW I suppose it won't be
necessary to initially lower the rating of the MIPS GIC and R4K clocksource
timers if this is implemented.

So, what do you think?

-Sergey

> 
> Thomas.
> 
> -- 
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

Serge Semin May 15, 2020, 7:48 a.m. UTC | #3

Thomas,
Could you take a look at my comment below so I could proceed with the
patchset v3 development?

-Sergey

On Mon, May 11, 2020 at 04:31:21PM +0300, Serge Semin wrote:
> On Fri, May 08, 2020 at 05:41:50PM +0200, Thomas Bogendoerfer wrote:
> > On Wed, May 06, 2020 at 08:42:36PM +0300, [e-mail] wrote:
> > > From: Serge Semin <[e-mail]>
> > > 
> > > Commit 07d69579e7fe ("MIPS: Don't register r4k sched clock when
> > > CPUFREQ enabled") disabled the r4k-clock usage for scheduler ticks
> > > counting due to the scheduler being non-tolerant for unstable
> > > clocks sources. For the same reason the clock should be used
> > > in the system clocksource framework only as a last resort if CPU
> > > frequency may change.
> > > 
> > > Signed-off-by: Serge Semin <[e-mail]>
> > > Cc: Alexey Malahov <[e-mail]>
> > > Cc: Thomas Bogendoerfer <[e-mail]>
> > > Cc: Paul Burton <[e-mail]>
> > > Cc: Ralf Baechle <[e-mail]>
> > > Cc: Greg Kroah-Hartman <[e-mail]>
> > > Cc: Arnd Bergmann <[e-mail]>
> > > Cc: Rob Herring <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: [e-mail]
> > > ---
> > >  arch/mips/kernel/csrc-r4k.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/arch/mips/kernel/csrc-r4k.c b/arch/mips/kernel/csrc-r4k.c
> > > index 437dda64fd7a..d81fb374f477 100644
> > > --- a/arch/mips/kernel/csrc-r4k.c
> > > +++ b/arch/mips/kernel/csrc-r4k.c
> > > @@ -71,7 +71,11 @@ int __init init_r4k_clocksource(void)
> > >  		return -ENXIO;
> > >  
> > >  	/* Calculate a somewhat reasonable rating value */
> > > +#ifndef CONFIG_CPU_FREQ
> > >  	clocksource_mips.rating = 200 + mips_hpt_frequency / 10000000;
> > > +#else
> > > +	clocksource_mips.rating = 99;
> > > +#endif
> > 
> > I dislike this patch. Assuming you have an other clocksource, why not
> > simply disable csrc-r4k, if CPU_FREQ is enabled ?
> 
> Me neither and the best way would be to update the clocksource frequency
> dynamically the same way it's done for cevt-r4k and MIPS GIC timers. Alas the
> clocksource doesn't support it. Due to this together with CPU-freq facility
> enabled we have to use a very slow DW APB Timer instead of the fast embedded
> into the CPU core r4k and MIPS GIC timers. Just note the difference: it takes
> 220 ns to read the counter from DW APB Timer in comparison to a few nanoseconds
> reading from MIPS GIC and R4K. So IMO disabling the timer as you suggest isn't
> the best option. By making the CPUFREQ and CSRC_R4K mutual exclusive we'd
> assume a use-case that the system will always use the CPU-freq facility changing
> the CPU reference frequency. This is obviously not true. Noone prevents the
> system administrator to leave the default setting of the CPU-freq with fixed
> frequency and select a faster, more accurate timer like in our case.
> 
> My idea was not to try to predict how the system would be used, but to let the
> system administration to choose which timer is applicable in particular usecase
> enabling a safest one by default. So if CPUFREQ is available, then we fallback
> to the external timer as safest one. If the system user wouldn't need to have
> the CPUFREQ facility utilized, then the system administrator would want to
> leave the default CPU-freq governor with pre-defined CPU frequency and
> select either R4K (MIPS) or MIPS GIC timer just by writing its name into
> /sys/bus/clocksource/devices/clocksource0/current_clocksource .
>  
> I should note, that currently CPU_FREQ won't be available if there is no
> MIPS_EXTERNAL_TIMER available for the platform. It's prohibited by means of the
> conditional kbuild config inclusion declared in the arch/mips/Kconfig:
> + if CPU_SUPPORTS_CPUFREQ && MIPS_EXTERNAL_TIMER
> + source "drivers/cpufreq/Kconfig"
> + endif
> So if there is no external timer working independently from the CPU core clock
> source, the CPUFREQ won't be available to select for the kernel. Though currently
> this limitation is supposed to be applicable for the R4K/MIPS GIC clocksource
> timers only since clockevents must work fine in unstable reference clock conditions.
> 
> So what can we do to improve the patch? First one is a solution I suggested in
> this patch but it could be a bit altered by using IS_ENABLED() macro to:
> + clocksource_mips.rating = !IS_ENABLED(CONFIG_CPU_FREQ) ?
> +			    200 + mips_hpt_frequency / 10000000 : 99;
> 
> Another idea I discovered when have been searching through the x86 arch code.
> x86's got the same problem with TSC timer, but it doesn't disable it if
> CPU-frequency is switched on. Instead it just marks it as unstable by calling
> the clocksource_mark_unstable() method if CPU frequency changes. I suggest to
> implement the same approach in our case of MIPS GIC (another patchset
> I've sent, see "clocksource: Fix MIPS GIC and DW APB Timer for Baikal-T1 SoC
> support" in your email client) and R4K timers. We'll subscribe to the CPU
> frequency change and if it changes we'll call clocksource_mark_unstable() with
> MIPS GIC and R4K clocksource handlers passed. This shall reduce their rating and
> cause selecting a clocksource with better one. BTW I suppose it won't be
> necessary to initially lower the rating of the MIPS GIC and R4K clocksource
> timers if this is implemented.
> 
> So, what do you think?
> 
> -Sergey
> 
> > 
> > Thomas.
> > 
> > -- 
> > Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> > good idea.                                                [ RFC1925, 2.3 ]

Thomas Bogendoerfer May 15, 2020, 9:06 p.m. UTC | #4

On Fri, May 15, 2020 at 10:48:27AM +0300, Serge Semin wrote:
> Thomas,
> Could you take a look at my comment below so I could proceed with the
> patchset v3 development?

I can't help, but using r4k clocksource with changing frequency is
probaly only usefull as a random generator. So IMHO the only two
options are disabling it or implement what arch/x86/kernel/tsc.c does.

Thomas.

Serge Semin May 16, 2020, 11:55 a.m. UTC | #5

On Fri, May 15, 2020 at 11:06:47PM +0200, Thomas Bogendoerfer wrote:
> On Fri, May 15, 2020 at 10:48:27AM +0300, Serge Semin wrote:
> > Thomas,
> > Could you take a look at my comment below so I could proceed with the
> > patchset v3 development?
> 
> I can't help, but using r4k clocksource with changing frequency is
> probaly only usefull as a random generator. So IMHO the only two
> options are disabling it or implement what arch/x86/kernel/tsc.c does.

Then it's settled. I'll resend the series with csrc-r4k updated to have the
tsc-like design implemented.

-Sergey

> 
> Thomas.
> 
> -- 
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

Serge Semin May 18, 2020, 1:48 p.m. UTC | #6

On Fri, May 15, 2020 at 11:06:47PM +0200, Thomas Bogendoerfer wrote:
> On Fri, May 15, 2020 at 10:48:27AM +0300, Serge Semin wrote:
> > Thomas,
> > Could you take a look at my comment below so I could proceed with the
> > patchset v3 development?
> 
> I can't help, but using r4k clocksource with changing frequency is
> probaly only usefull as a random generator. So IMHO the only two
> options are disabling it or implement what arch/x86/kernel/tsc.c does.
> 
> Thomas.

Thomas, could you proceed with the rest of the patches review?
├─>[PATCH v2 16/20] bus: cdmm: Add MIPS R5 arch support
├─>[PATCH v2 15/20] mips: cdmm: Add mti,mips-cdmm dtb node support
├─>[PATCH v2 13/20] mips: early_printk_8250: Use offset-sized IO-mem accessors
├─>[PATCH v2 12/20] mips: MAAR: Add XPA mode support
├─>[PATCH v2 10/20] mips: Add CONFIG/CONFIG6/Cause reg fields macro
└─>[PATCH v2 09/20] mips: Add CP0 Write Merge config support

It would be great if I fixed more comments in the next patchset version.

-Sergey

> 
> -- 
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

Thomas Bogendoerfer May 18, 2020, 4:32 p.m. UTC | #7

On Mon, May 18, 2020 at 04:48:20PM +0300, Serge Semin wrote:
> On Fri, May 15, 2020 at 11:06:47PM +0200, Thomas Bogendoerfer wrote:
> > On Fri, May 15, 2020 at 10:48:27AM +0300, Serge Semin wrote:
> > > Thomas,
> > > Could you take a look at my comment below so I could proceed with the
> > > patchset v3 development?
> > 
> > I can't help, but using r4k clocksource with changing frequency is
> > probaly only usefull as a random generator. So IMHO the only two
> > options are disabling it or implement what arch/x86/kernel/tsc.c does.
> > 
> > Thomas.
> 
> Thomas, could you proceed with the rest of the patches review?
> ├─>[PATCH v2 16/20] bus: cdmm: Add MIPS R5 arch support
> ├─>[PATCH v2 15/20] mips: cdmm: Add mti,mips-cdmm dtb node support

both are not my call, but look ok to me.

> ├─>[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.

> ├─>[PATCH v2 12/20] mips: MAAR: Add XPA mode support

looks ok so far.

> ├─>[PATCH v2 10/20] mips: Add CONFIG/CONFIG6/Cause reg fields macro

that is fine

> └─>[PATCH v2 09/20] mips: Add CP0 Write Merge config support

this is IMHO a dangerous change. Enabling write merging for any
CPU supporting it might triggers bugs. Do it in your board bringup
code and at the moment I don't see a reason for the rest of that
patch.

Thomas.

Serge Semin May 18, 2020, 8:57 p.m. UTC | #8

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:
> > On Fri, May 15, 2020 at 11:06:47PM +0200, Thomas Bogendoerfer wrote:
> > > On Fri, May 15, 2020 at 10:48:27AM +0300, Serge Semin wrote:
> > > > Thomas,
> > > > Could you take a look at my comment below so I could proceed with the
> > > > patchset v3 development?
> > > 
> > > I can't help, but using r4k clocksource with changing frequency is
> > > probaly only usefull as a random generator. So IMHO the only two
> > > options are disabling it or implement what arch/x86/kernel/tsc.c does.
> > > 
> > > Thomas.
> > 
> > Thomas, could you proceed with the rest of the patches review?
> > ├─>[PATCH v2 16/20] bus: cdmm: Add MIPS R5 arch support
> > ├─>[PATCH v2 15/20] mips: cdmm: Add mti,mips-cdmm dtb node support
> 
> both are not my call, but look ok to me.

Can I add your Reviewed-by tag there then?

> 
> > ├─>[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
the currently available setup_8250_early_printk_port() calls so they would pass
the reg_with as 1 to activate the normal readb/writeb IO methods.

What do you think about this?

> 
> > ├─>[PATCH v2 12/20] mips: MAAR: Add XPA mode support
> 
> looks ok so far.

Can I add your Reviewed-by tag there then?

> 
> > ├─>[PATCH v2 10/20] mips: Add CONFIG/CONFIG6/Cause reg fields macro
> 
> that is fine

Can I add your Reviewed-by tag there then?

> 
> > └─>[PATCH v2 09/20] mips: Add CP0 Write Merge config support
> 
> this is IMHO a dangerous change. Enabling write merging for any
> CPU supporting it might triggers bugs. Do it in your board bringup
> code and at the moment I don't see a reason for the rest of that
> patch.

Let's at least leave the mm_config() implementation but without the write-merge
enabling by default. Providing features availability macro
cpu_has_mm_sysad/cpu_has_mm_full and c0 config fields
MIPS_CPU_MM_SYSAD/MIPS_CPU_MM_FULL defined will be useful in the platform-specific
Write-Merge enable/disable procedure. For instance, in the my prom_init() method
I could use them to implement a code pattern like:

+	if (cpu_has_mm_full) {
+		unsigned int config0 = read_c0_config();
+               config0 = (config0 & ~MIPS_CONF_MM) | MIPS_CONF_MM_FULL;
+               write_c0_config(config0);
+	}

By doing so I can manually enable/disable the MM feature in the
cpu-feature-overrides.h. Without that I'd have to locally define these macro,
which isn't good seeing they are in fact generic and can be useful for other
platforms with SYSAD and FULL MM feature available. What do you think?

-Sergey

> 
> Thomas.
> 
> -- 
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

Thomas Bogendoerfer May 19, 2020, 3:50 p.m. UTC | #9

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:
> > > On Fri, May 15, 2020 at 11:06:47PM +0200, Thomas Bogendoerfer wrote:
> > > > On Fri, May 15, 2020 at 10:48:27AM +0300, Serge Semin wrote:
> > > > > Thomas,
> > > > > Could you take a look at my comment below so I could proceed with the
> > > > > patchset v3 development?
> > > > 
> > > > I can't help, but using r4k clocksource with changing frequency is
> > > > probaly only usefull as a random generator. So IMHO the only two
> > > > options are disabling it or implement what arch/x86/kernel/tsc.c does.
> > > > 
> > > > Thomas.
> > > 
> > > Thomas, could you proceed with the rest of the patches review?
> > > ├─>[PATCH v2 16/20] bus: cdmm: Add MIPS R5 arch support
> > > ├─>[PATCH v2 15/20] mips: cdmm: Add mti,mips-cdmm dtb node support
> > 
> > both are not my call, but look ok to me.
> 
> Can I add your Reviewed-by tag there then?

only for 16/20. 15/20 looks ok to me, but I have not enough insides
on the hardware to say this is good.

> > > ├─>[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.


> > 
> > > ├─>[PATCH v2 12/20] mips: MAAR: Add XPA mode support
> > 
> > looks ok so far.
> 
> Can I add your Reviewed-by tag there then?

As I'm the maintainer of the part, I've simply applied it.

> > 
> > > ├─>[PATCH v2 10/20] mips: Add CONFIG/CONFIG6/Cause reg fields macro
> > 
> > that is fine
> 
> Can I add your Reviewed-by tag there then?

As this didn't apply cleanly, I'll apply it after you've resent it.
IMHO no need for a Reviewed-by.

> > > └─>[PATCH v2 09/20] mips: Add CP0 Write Merge config support
> > 
> > this is IMHO a dangerous change. Enabling write merging for any
> > CPU supporting it might triggers bugs. Do it in your board bringup
> > code and at the moment I don't see a reason for the rest of that
> > patch.
> 
> Let's at least leave the mm_config() implementation but without the write-merge
> enabling by default. Providing features availability macro
> cpu_has_mm_sysad/cpu_has_mm_full and c0 config fields

do you have a user of that ? I'm not introducing code nobody uses.

> I could use them to implement a code pattern like:
> 
> +	if (cpu_has_mm_full) {
> +		unsigned int config0 = read_c0_config();
> +               config0 = (config0 & ~MIPS_CONF_MM) | MIPS_CONF_MM_FULL;
> +               write_c0_config(config0);
> +	}

you know you are running on a R5 core, so you know you have MM_FULL.
No need to check this.

> By doing so I can manually enable/disable the MM feature in the
> cpu-feature-overrides.h. Without that I'd have to locally define these macro,
> which isn't good seeing they are in fact generic and can be useful for other
> platforms with SYSAD and FULL MM feature available. What do you think?

To me this is a hardware feature I expect to be done by firmware and
Linux shouldn't care about it, if it doesn't have any software
implications.

Thomas.

Serge Semin May 20, 2020, 11:59 a.m. UTC | #10

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:
> > > > On Fri, May 15, 2020 at 11:06:47PM +0200, Thomas Bogendoerfer wrote:
> > > > > On Fri, May 15, 2020 at 10:48:27AM +0300, Serge Semin wrote:
> > > > > > Thomas,
> > > > > > Could you take a look at my comment below so I could proceed with the
> > > > > > patchset v3 development?
> > > > > 
> > > > > I can't help, but using r4k clocksource with changing frequency is
> > > > > probaly only usefull as a random generator. So IMHO the only two
> > > > > options are disabling it or implement what arch/x86/kernel/tsc.c does.
> > > > > 
> > > > > Thomas.
> > > > 
> > > > Thomas, could you proceed with the rest of the patches review?
> > > > ├─>[PATCH v2 16/20] bus: cdmm: Add MIPS R5 arch support
> > > > ├─>[PATCH v2 15/20] mips: cdmm: Add mti,mips-cdmm dtb node support
> > > 
> > > both are not my call, but look ok to me.
> > 
> > Can I add your Reviewed-by tag there then?
> 
> only for 16/20. 15/20 looks ok to me, but I have not enough insides
> on the hardware to say this is good.

Ok.

> 
> > > > ├─>[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.
> 

Ok.

> 
> > > 
> > > > ├─>[PATCH v2 12/20] mips: MAAR: Add XPA mode support
> > > 
> > > looks ok so far.
> > 
> > Can I add your Reviewed-by tag there then?
> 
> As I'm the maintainer of the part, I've simply applied it.
> 
> > > 
> > > > ├─>[PATCH v2 10/20] mips: Add CONFIG/CONFIG6/Cause reg fields macro
> > > 
> > > that is fine
> > 
> > Can I add your Reviewed-by tag there then?
> 
> As this didn't apply cleanly, I'll apply it after you've resent it.
> IMHO no need for a Reviewed-by.

Ok.

> 
> > > > └─>[PATCH v2 09/20] mips: Add CP0 Write Merge config support
> > > 
> > > this is IMHO a dangerous change. Enabling write merging for any
> > > CPU supporting it might triggers bugs. Do it in your board bringup
> > > code and at the moment I don't see a reason for the rest of that
> > > patch.
> > 
> > Let's at least leave the mm_config() implementation but without the write-merge
> > enabling by default. Providing features availability macro
> > cpu_has_mm_sysad/cpu_has_mm_full and c0 config fields
> 
> do you have a user of that ? I'm not introducing code nobody uses.
> 

See my comment below.

> > I could use them to implement a code pattern like:
> > 
> > +	if (cpu_has_mm_full) {
> > +		unsigned int config0 = read_c0_config();
> > +               config0 = (config0 & ~MIPS_CONF_MM) | MIPS_CONF_MM_FULL;
> > +               write_c0_config(config0);
> > +	}
> 
> you know you are running on a R5 core, so you know you have MM_FULL.
> No need to check this.
> 
> > By doing so I can manually enable/disable the MM feature in the
> > cpu-feature-overrides.h. Without that I'd have to locally define these macro,
> > which isn't good seeing they are in fact generic and can be useful for other
> > platforms with SYSAD and FULL MM feature available. What do you think?
> 
> To me this is a hardware feature I expect to be done by firmware and
> Linux shouldn't care about it, if it doesn't have any software
> implications.

I think there is a misunderstanding here. In this patch I am not enabling
Write-Merge feature for any memory range. I am enabling the UCA Cache Coherency
attribute to be available for utilization. See the user-manual info regarding
the CP0.CONFIG.MM field:
	Write Merge.This bit indicates whether write-through merging is enabled
	in the 32-byte collapsing write buffer.
	0: No merging allowed
	1: Merging allowed

In order to have the Write-merging really enabled for a particular PFN one have
to mark its TLB entry with UCA (EntryLoX.C[3:5] = 7) attribute. So in this patch
I am attempting to detect whether the feature is either already enabled or if
available to enable it for utilization.

If there is no misunderstanding and you said what you said, that even enabling
the feature for utilization might be dangerous, let's at least leave the
MIPS_CONF_MM, MIPS_CONF_MM_FULL and MIPS_CONF_MM_SYS_SYSAD fields
definition in the "arch/mips/include/asm/mipsregs.h" header. I'll use
them to enable the write-merge in my platform code.

What do you think?

-Sergey

> 
> Thomas.
> 
> -- 
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

Serge Semin May 20, 2020, 12:12 p.m. UTC | #11

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:
> > > > On Fri, May 15, 2020 at 11:06:47PM +0200, Thomas Bogendoerfer wrote:
> > > > > On Fri, May 15, 2020 at 10:48:27AM +0300, Serge Semin wrote:
> > > > > > Thomas,
> > > > > > Could you take a look at my comment below so I could proceed with the
> > > > > > patchset v3 development?
> > > > > 
> > > > > I can't help, but using r4k clocksource with changing frequency is
> > > > > probaly only usefull as a random generator. So IMHO the only two
> > > > > options are disabling it or implement what arch/x86/kernel/tsc.c does.
> > > > > 
> > > > > Thomas.
> > > > 
> > > > Thomas, could you proceed with the rest of the patches review?
> > > > ├─>[PATCH v2 16/20] bus: cdmm: Add MIPS R5 arch support
> > > > ├─>[PATCH v2 15/20] mips: cdmm: Add mti,mips-cdmm dtb node support
> > > 
> > > both are not my call, but look ok to me.
> > 
> > Can I add your Reviewed-by tag there then?
> 
> only for 16/20. 15/20 looks ok to me, but I have not enough insides
> on the hardware to say this is good.
> 
> > > > ├─>[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.)

-Sergey

> 
> 
> > > 
> > > > ├─>[PATCH v2 12/20] mips: MAAR: Add XPA mode support
> > > 
> > > looks ok so far.
> > 
> > Can I add your Reviewed-by tag there then?
> 
> As I'm the maintainer of the part, I've simply applied it.
> 
> > > 
> > > > ├─>[PATCH v2 10/20] mips: Add CONFIG/CONFIG6/Cause reg fields macro
> > > 
> > > that is fine
> > 
> > Can I add your Reviewed-by tag there then?
> 
> As this didn't apply cleanly, I'll apply it after you've resent it.
> IMHO no need for a Reviewed-by.
> 
> > > > └─>[PATCH v2 09/20] mips: Add CP0 Write Merge config support
> > > 
> > > this is IMHO a dangerous change. Enabling write merging for any
> > > CPU supporting it might triggers bugs. Do it in your board bringup
> > > code and at the moment I don't see a reason for the rest of that
> > > patch.
> > 
> > Let's at least leave the mm_config() implementation but without the write-merge
> > enabling by default. Providing features availability macro
> > cpu_has_mm_sysad/cpu_has_mm_full and c0 config fields
> 
> do you have a user of that ? I'm not introducing code nobody uses.
> 
> > I could use them to implement a code pattern like:
> > 
> > +	if (cpu_has_mm_full) {
> > +		unsigned int config0 = read_c0_config();
> > +               config0 = (config0 & ~MIPS_CONF_MM) | MIPS_CONF_MM_FULL;
> > +               write_c0_config(config0);
> > +	}
> 
> you know you are running on a R5 core, so you know you have MM_FULL.
> No need to check this.
> 
> > By doing so I can manually enable/disable the MM feature in the
> > cpu-feature-overrides.h. Without that I'd have to locally define these macro,
> > which isn't good seeing they are in fact generic and can be useful for other
> > platforms with SYSAD and FULL MM feature available. What do you think?
> 
> To me this is a hardware feature I expect to be done by firmware and
> Linux shouldn't care about it, if it doesn't have any software
> implications.
> 
> Thomas.
> 
> -- 
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

Serge Semin May 20, 2020, 12:21 p.m. UTC | #12

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
>

Thomas Bogendoerfer May 20, 2020, 1:38 p.m. UTC | #13

On Wed, May 20, 2020 at 03:12:01PM +0300, Serge Semin wrote:
> 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?

to be on the safe side it's probably the best thing. But I don't know
enough about CPS_NS16550 to judge whether shift value correlates with
possible access width.

Thomas.

Serge Semin May 20, 2020, 1:48 p.m. UTC | #14

On Wed, May 20, 2020 at 03:38:27PM +0200, Thomas Bogendoerfer wrote:
> On Wed, May 20, 2020 at 03:12:01PM +0300, Serge Semin wrote:
> > 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?
> 
> to be on the safe side it's probably the best thing. But I don't know
> enough about CPS_NS16550 to judge whether shift value correlates with
> possible access width.

The base address passed to the _mips_cps_putc() leaf is UART-base address. It
has nothing to do with CPS. See:
/**
 * _mips_cps_putc() - write a character to the UART
 * @a0: ASCII character to write
 * @t9: UART base address
 */
LEAF(_mips_cps_putc)
1:      lw              t0, UART_LSR_OFS(t9)
        andi            t0, t0, UART_LSR_TEMT
        beqz            t0, 1b
        sb              a0, UART_TX_OFS(t9)
        jr              ra
        END(_mips_cps_putc)

So it's base address must be accessed with proper alignment. On our case it's
lw/sw instructions. Regarding using lw in the first line of the function. That's
must be a bug, since further in the same function they use sb to access the UART
Tx register. So reading a data from UART_LSR register should be also byte-sized
by using lb.

-Sergey

> 
> Thomas.
> 
> -- 
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

Serge Semin May 20, 2020, 2:03 p.m. UTC | #15

On Wed, May 20, 2020 at 02:59:27PM +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:
> > > > > On Fri, May 15, 2020 at 11:06:47PM +0200, Thomas Bogendoerfer wrote:

[nip]

> > > > > └─>[PATCH v2 09/20] mips: Add CP0 Write Merge config support
> > > > 
> > > > this is IMHO a dangerous change. Enabling write merging for any
> > > > CPU supporting it might triggers bugs. Do it in your board bringup
> > > > code and at the moment I don't see a reason for the rest of that
> > > > patch.
> > > 
> > > Let's at least leave the mm_config() implementation but without the write-merge
> > > enabling by default. Providing features availability macro
> > > cpu_has_mm_sysad/cpu_has_mm_full and c0 config fields
> > 
> > do you have a user of that ? I'm not introducing code nobody uses.
> > 
> 
> See my comment below.
> 
> > > I could use them to implement a code pattern like:
> > > 
> > > +	if (cpu_has_mm_full) {
> > > +		unsigned int config0 = read_c0_config();
> > > +               config0 = (config0 & ~MIPS_CONF_MM) | MIPS_CONF_MM_FULL;
> > > +               write_c0_config(config0);
> > > +	}
> > 
> > you know you are running on a R5 core, so you know you have MM_FULL.
> > No need to check this.
> > 
> > > By doing so I can manually enable/disable the MM feature in the
> > > cpu-feature-overrides.h. Without that I'd have to locally define these macro,
> > > which isn't good seeing they are in fact generic and can be useful for other
> > > platforms with SYSAD and FULL MM feature available. What do you think?
> > 
> > To me this is a hardware feature I expect to be done by firmware and
> > Linux shouldn't care about it, if it doesn't have any software
> > implications.
> 
> I think there is a misunderstanding here. In this patch I am not enabling
> Write-Merge feature for any memory range. I am enabling the UCA Cache Coherency
> attribute to be available for utilization. See the user-manual info regarding
> the CP0.CONFIG.MM field:
> 	Write Merge.This bit indicates whether write-through merging is enabled
> 	in the 32-byte collapsing write buffer.
> 	0: No merging allowed
> 	1: Merging allowed
> 
> In order to have the Write-merging really enabled for a particular PFN one have
> to mark its TLB entry with UCA (EntryLoX.C[3:5] = 7) attribute. So in this patch
> I am attempting to detect whether the feature is either already enabled or if
> available to enable it for utilization.
> 
> If there is no misunderstanding and you said what you said, that even enabling
> the feature for utilization might be dangerous, let's at least leave the
> MIPS_CONF_MM, MIPS_CONF_MM_FULL and MIPS_CONF_MM_SYS_SYSAD fields
> definition in the "arch/mips/include/asm/mipsregs.h" header. I'll use
> them to enable the write-merge in my platform code.
> 
> What do you think?
> 

Thomas,
Could you also give me your comment on the above, so to make sure that we
understood each other correctly in this question?

-Sergey

Thomas Bogendoerfer May 20, 2020, 6:30 p.m. UTC | #16

On Wed, May 20, 2020 at 04:48:26PM +0300, Serge Semin wrote:
> On Wed, May 20, 2020 at 03:38:27PM +0200, Thomas Bogendoerfer wrote:
> > On Wed, May 20, 2020 at 03:12:01PM +0300, Serge Semin wrote:
> > > 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?
> > 
> > to be on the safe side it's probably the best thing. But I don't know
> > enough about CPS_NS16550 to judge whether shift value correlates with
> > possible access width.
> 
> The base address passed to the _mips_cps_putc() leaf is UART-base address. It
> has nothing to do with CPS. See:

ok, I'm confused. So this isn't an uart inside CPS hardware, but an uart used
by CPS code for debug output, right ? 

To solve the issued please add CONFIG_MIPS_CPS_NS16550_WIDTH to select the
correct access width.

Thomas.

Thomas Bogendoerfer May 20, 2020, 6:40 p.m. UTC | #17

On Wed, May 20, 2020 at 02:59:26PM +0300, Serge Semin wrote:
> I think there is a misunderstanding here. In this patch I am not enabling

you are right, I've missed the fact, that this also needs to be enabled
in TLB entries. Strange that MIPS added the enable bit while R10k simply
do uncached acclerated, whenever TLB entry selects it.

> If there is no misunderstanding and you said what you said, that even enabling
> the feature for utilization might be dangerous, let's at least leave the
> MIPS_CONF_MM, MIPS_CONF_MM_FULL and MIPS_CONF_MM_SYS_SYSAD fields
> definition in the "arch/mips/include/asm/mipsregs.h" header. I'll use
> them to enable the write-merge in my platform code.
> 
> What do you think?

I withdraw my concerns and will apply the patch as is.

Thomas.

Serge Semin May 20, 2020, 9:12 p.m. UTC | #18

On Wed, May 20, 2020 at 08:30:57PM +0200, Thomas Bogendoerfer wrote:
> On Wed, May 20, 2020 at 04:48:26PM +0300, Serge Semin wrote:
> > On Wed, May 20, 2020 at 03:38:27PM +0200, Thomas Bogendoerfer wrote:
> > > On Wed, May 20, 2020 at 03:12:01PM +0300, Serge Semin wrote:
> > > > 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?
> > > 
> > > to be on the safe side it's probably the best thing. But I don't know
> > > enough about CPS_NS16550 to judge whether shift value correlates with
> > > possible access width.
> > 
> > The base address passed to the _mips_cps_putc() leaf is UART-base address. It
> > has nothing to do with CPS. See:
> 
> ok, I'm confused. So this isn't an uart inside CPS hardware, but an uart used
> by CPS code for debug output, right ? 

Right. It's not CPS, but just UART available on the system. See a comment in the
arch/mips/kernel/cps-vec-ns16550.S:
/**
 * mips_cps_bev_dump() - dump relevant exception state to UART
 * @a0: pointer to NULL-terminated ASCII string naming the exception
 *
 * Write information that may be useful in debugging an exception to the
 * UART configured by CONFIG_MIPS_CPS_NS16550_*.
 *...
 */
LEAF(mips_cps_bev_dump)
        move            s0, ra
        move            s1, a0

        li              t9, CKSEG1ADDR(CONFIG_MIPS_CPS_NS16550_BASE)
        ...

See the base is just loaded to the t9 register.

> 
> To solve the issued please add CONFIG_MIPS_CPS_NS16550_WIDTH to select the
> correct access width.

Ok. Thanks.

-Sergey

> 
> Thomas.
> 
> -- 
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

Serge Semin May 20, 2020, 9:13 p.m. UTC | #19

On Wed, May 20, 2020 at 08:40:24PM +0200, Thomas Bogendoerfer wrote:
> On Wed, May 20, 2020 at 02:59:26PM +0300, Serge Semin wrote:
> > I think there is a misunderstanding here. In this patch I am not enabling
> 
> you are right, I've missed the fact, that this also needs to be enabled
> in TLB entries. Strange that MIPS added the enable bit while R10k simply
> do uncached acclerated, whenever TLB entry selects it.
> 
> > If there is no misunderstanding and you said what you said, that even enabling
> > the feature for utilization might be dangerous, let's at least leave the
> > MIPS_CONF_MM, MIPS_CONF_MM_FULL and MIPS_CONF_MM_SYS_SYSAD fields
> > definition in the "arch/mips/include/asm/mipsregs.h" header. I'll use
> > them to enable the write-merge in my platform code.
> > 
> > What do you think?
> 
> I withdraw my concerns and will apply the patch as is.

Great! Thanks.

-Sergey

> 
> Thomas.
> 
> -- 
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

Patch

diff --git a/arch/mips/kernel/csrc-r4k.c b/arch/mips/kernel/csrc-r4k.c
index 437dda64fd7a..d81fb374f477 100644
--- a/arch/mips/kernel/csrc-r4k.c
+++ b/arch/mips/kernel/csrc-r4k.c
@@ -71,7 +71,11 @@  int __init init_r4k_clocksource(void)
 		return -ENXIO;
 
 	/* Calculate a somewhat reasonable rating value */
+#ifndef CONFIG_CPU_FREQ
 	clocksource_mips.rating = 200 + mips_hpt_frequency / 10000000;
+#else
+	clocksource_mips.rating = 99;
+#endif
 
 	/*
 	 * R2 onwards makes the count accessible to user mode so it can be used