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

[Serge Semin]

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