RE: [PATCH v4 1/1] MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync for BCM47XX PCIe erratum

[IKEGAMI Tokunori <ikegami@xxxxxxxxxxxxxxxxxxxx>]

Hi Hauke-san,

Thanks for your reviewing and comments.

> You do it now unconditionally on all MIPS SoCs using bcma.
> 
> There are multiple different silicons from the Broadcom BCM47xx and
> BCM53xx line with a MIPS 74K core, see
> https://wikidevi.com/wiki/Broadcom I haven't tested if this fix is ok
> for all of them, but I think so.

  I see and yes I think so from the CFE implementation.

> Acked-by: Hauke Mehrtens <hauke@xxxxxxxxxx>

  Just sent the v5 version patch to change your name tag as Acked-by.

Regards,
Ikegami

> -----Original Message-----
> From: Hauke Mehrtens [mailto:hauke@xxxxxxxxxx]
> Sent: Saturday, June 02, 2018 6:54 PM
> To: IKEGAMI Tokunori; James Hogan
> Cc: PACKHAM Chris; Rafał Miłecki; linux-mips@xxxxxxxxxxxxxx
> Subject: Re: [PATCH v4 1/1] MIPS: BCM47XX: Enable MIPS32 74K Core
> ExternalSync for BCM47XX PCIe erratum
> 
> On 06/01/2018 12:19 AM, Tokunori Ikegami wrote:
> > The erratum and workaround are described by BCM5300X-ES300-RDS.pdf as
> below.
> >
> >   R10: PCIe Transactions Periodically Fail
> >
> >     Description: The BCM5300X PCIe does not maintain transaction
> ordering.
> >                  This may cause PCIe transaction failure.
> >     Fix Comment: Add a dummy PCIe configuration read after a PCIe
> >                  configuration write to ensure PCIe configuration
> access
> >                  ordering. Set ES bit of CP0 configu7 register to enable
> >                  sync function so that the sync instruction is
> functional.
> >     Resolution:  hndpci.c: extpci_write_config()
> >                  hndmips.c: si_mips_init()
> >                  mipsinc.h CONF7_ES
> >
> > This is fixed by the CFE MIPS bcmsi chipset driver also for BCM47XX.
> > Also the dummy PCIe configuration read is already implemented in the Linux
> > BCMA driver.
> > Enable ExternalSync in Config7 when CONFIG_BCMA_DRIVER_PCI_HOSTMODE=y
> > too so that the sync instruction is externalised.
> 
> You do it now unconditionally on all MIPS SoCs using bcma.
> 
> There are multiple different silicons from the Broadcom BCM47xx and
> BCM53xx line with a MIPS 74K core, see
> https://wikidevi.com/wiki/Broadcom I haven't tested if this fix is ok
> for all of them, but I think so.
> 
> > Signed-off-by: Tokunori Ikegami <ikegami@xxxxxxxxxxxxxxxxxxxx>
> > Reviewed-by: Paul Burton <paul.burton@xxxxxxxx>
> > Cc: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
> > Cc: Hauke Mehrtens <hauke@xxxxxxxxxx>
> > Cc: Rafał Miłecki <zajec5@xxxxxxxxx>
> > Cc: linux-mips@xxxxxxxxxxxxxx
> 
> Acked-by: Hauke Mehrtens <hauke@xxxxxxxxxx>
> 
> > ---
> > Changes since v3:
> > - Add Reviewed-by: Paul Burton tag.
> > - Remove pr_info().
> >
> > Changes since v2:
> > - Move the change into platform-specific code bcm47xx_cpu_fixes()
> function from in generic code.
> >
> > Changes since v1 resent:
> > - None.
> >
> > Changes since v1 original:
> > - Change to use set_c0_config7 instead of write_c0_config7.
> >
> >  arch/mips/bcm47xx/setup.c        | 6 ++++++
> >  arch/mips/include/asm/mipsregs.h | 3 +++
> >  2 files changed, 9 insertions(+)
> >
> > diff --git a/arch/mips/bcm47xx/setup.c b/arch/mips/bcm47xx/setup.c
> > index 6054d49e608e..8c9cbf13d32a 100644
> > --- a/arch/mips/bcm47xx/setup.c
> > +++ b/arch/mips/bcm47xx/setup.c
> > @@ -212,6 +212,12 @@ static int __init bcm47xx_cpu_fixes(void)
> >  		 */
> >  		if (bcm47xx_bus.bcma.bus.chipinfo.id ==
> BCMA_CHIP_ID_BCM4706)
> >  			cpu_wait = NULL;
> > +
> > +		/*
> > +		 * BCM47XX Erratum "R10: PCIe Transactions Periodically
> Fail"
> > +		 * Enable ExternalSync for sync instruction to take effect
> > +		 */
> > +		set_c0_config7(MIPS_CONF7_ES);
> >  		break;
> >  #endif
> >  	}
> > diff --git a/arch/mips/include/asm/mipsregs.h
> b/arch/mips/include/asm/mipsregs.h
> > index 858752dac337..0f94acf60144 100644
> > --- a/arch/mips/include/asm/mipsregs.h
> > +++ b/arch/mips/include/asm/mipsregs.h
> > @@ -680,6 +680,8 @@
> >  #define MIPS_CONF7_WII		(_ULCAST_(1) << 31)
> >
> >  #define MIPS_CONF7_RPS		(_ULCAST_(1) << 2)
> > +/* ExternalSync */
> > +#define MIPS_CONF7_ES		(_ULCAST_(1) << 8)
> >
> >  #define MIPS_CONF7_IAR		(_ULCAST_(1) << 10)
> >  #define MIPS_CONF7_AR		(_ULCAST_(1) << 16)
> > @@ -2759,6 +2761,7 @@ __BUILD_SET_C0(status)
> >  __BUILD_SET_C0(cause)
> >  __BUILD_SET_C0(config)
> >  __BUILD_SET_C0(config5)
> > +__BUILD_SET_C0(config7)
> >  __BUILD_SET_C0(intcontrol)
> >  __BUILD_SET_C0(intctl)
> >  __BUILD_SET_C0(srsmap)
> >