All of lore.kernel.org
 help / color / mirror / Atom feed
* MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync for BCM47XX PCIe erratum
@ 2018-04-24  7:33 IKEGAMI Tokunori
  2018-04-24 11:49 ` James Hogan
  2018-04-24 19:19 ` [PATCH] " smtpuser
  0 siblings, 2 replies; 10+ messages in thread
From: IKEGAMI Tokunori @ 2018-04-24  7:33 UTC (permalink / raw)
  To: linux-mips; +Cc: PACKHAM Chris

Hi,

Let us consult to change MIPS BCM47XX to enable MIPS32 74K Core ExternalSync.
Can we change the MIPS BCM47XX driver like this?
If any comment or concern please let us know.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync for BCM47XX PCIe erratum
  2018-04-24  7:33 MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync for BCM47XX PCIe erratum IKEGAMI Tokunori
@ 2018-04-24 11:49 ` James Hogan
  2018-04-24 12:06   ` Matt Redfearn
  2018-04-24 16:00   ` IKEGAMI Tokunori
  2018-04-24 19:19 ` [PATCH] " smtpuser
  1 sibling, 2 replies; 10+ messages in thread
From: James Hogan @ 2018-04-24 11:49 UTC (permalink / raw)
  To: IKEGAMI Tokunori
  Cc: linux-mips, PACKHAM Chris, Hauke Mehrtens, Rafał Miłecki

[-- Attachment #1: Type: text/plain, Size: 4897 bytes --]

Hi,

On Tue, Apr 24, 2018 at 07:33:51AM +0000, IKEGAMI Tokunori wrote:
> Let us consult to change MIPS BCM47XX to enable MIPS32 74K Core ExternalSync.
> Can we change the MIPS BCM47XX driver like this?
> If any comment or concern please let us know.

Thanks for the patch.

Please use git-send-email to send patches in future if possible (you can
put additional comments that aren't part of the commit message after a
"---" separator). That should help format it correctly and will put a
[PATCH] prefix etc.

I've also Cc'd Hauke and Rafał who maintain the BCM47XX platform.
Running scripts/get_maintainer.pl on the patch will list some
maintainers who might be worth Cc'ing on a patch.

> 
> 
> From d6904a5fc90aaf205e982755e4d6cda62ad21273 Mon Sep 17 00:00:00 2001
> From: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> Date: Thu, 22 Feb 2018 12:02:16 +0900
> Subject: [PATCH 1/2] MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync for
>  BCM47XX PCIe erratum
> 
> The erratum and workaround are described by BCM5300X-ES300-RDS.pdf as below.

Is that document accessible anywhere publicly?

> 
>   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.

> This patch is just to enable ExternalSync in the Linux driver.

I suggest rewording this line to explain how ES helps, maybe something
along the lines of:
"Enable ExternalSync in Config7 when CONFIG_BCMA_DRIVER_PCI_HOSTMODE=y
too so that the sync instruction is externalised..." 

(Best not to refer to "this patch", just say what it does, and in Linux
terminology this is architecture code, not really a driver).

> 
> Change-Id: Ifc7a0ce46962933731297ad0c82682e7d39328ff

You can drop this from upstream submissions in future.

You also need a signed-off-by line as described in
Documentation/process/submitting-patches.rst to certify the work as
complying with the "Developer's Certificate of Origin".

> Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>  arch/mips/include/asm/mipsregs.h | 2 ++
>  arch/mips/kernel/cpu-probe.c     | 7 +++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/arch/mips/include/asm/mipsregs.h b/arch/mips/include/asm/mipsregs.h
> index 858752dac337..1d1f4416a0f3 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)
> diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c
> index cf3fd549e16d..9171928c40dd 100644
> --- a/arch/mips/kernel/cpu-probe.c
> +++ b/arch/mips/kernel/cpu-probe.c
> @@ -429,6 +429,13 @@ static inline void check_errata(void)
>  		if ((c->processor_id & PRID_REV_MASK) <= PRID_REV_34K_V1_0_2)
>  			write_c0_config7(read_c0_config7() | MIPS_CONF7_RPS);
>  		break;
> +#ifdef CONFIG_BCMA_DRIVER_PCI_HOSTMODE
> +	case CPU_74K:
> +		/* Enable ExternalSync for sync instruction to take effect */

I think it would be helpful to mention the affected device and any
errata number in this comment.

> +		pr_info("ExternalSync has been enabled\n");
> +		write_c0_config7(read_c0_config7() | MIPS_CONF7_ES);
 
(I would have suggested adding:

diff --git a/arch/mips/include/asm/mipsregs.h b/arch/mips/include/asm/mipsregs.h
index f65859784a4c..af6e59cfc763 100644
--- a/arch/mips/include/asm/mipsregs.h
+++ b/arch/mips/include/asm/mipsregs.h
@@ -2760,6 +2760,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)

Then you can just do:

set_c0_config7(MIPS_CONF7_ES);

But I see the write(read() | x) form is already there in that file, so
probably best to remain consistent with that. Using set_c0_config7() can
always be done later as a separate patch.)

Otherwise the change itself looks reasonable to me.

Thanks
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync for BCM47XX PCIe erratum
  2018-04-24 11:49 ` James Hogan
@ 2018-04-24 12:06   ` Matt Redfearn
  2018-04-24 12:32     ` James Hogan
  2018-04-24 16:00   ` IKEGAMI Tokunori
  1 sibling, 1 reply; 10+ messages in thread
From: Matt Redfearn @ 2018-04-24 12:06 UTC (permalink / raw)
  To: James Hogan, IKEGAMI Tokunori
  Cc: linux-mips, PACKHAM Chris, Hauke Mehrtens, Rafał Miłecki

Hi,

On 24/04/18 12:49, James Hogan wrote:
> Hi,
> 
> On Tue, Apr 24, 2018 at 07:33:51AM +0000, IKEGAMI Tokunori wrote:
>> Let us consult to change MIPS BCM47XX to enable MIPS32 74K Core ExternalSync.
>> Can we change the MIPS BCM47XX driver like this?
>> If any comment or concern please let us know.
> 
> Thanks for the patch.
> 
> Please use git-send-email to send patches in future if possible (you can
> put additional comments that aren't part of the commit message after a
> "---" separator). That should help format it correctly and will put a
> [PATCH] prefix etc.
> 
> I've also Cc'd Hauke and Rafał who maintain the BCM47XX platform.
> Running scripts/get_maintainer.pl on the patch will list some
> maintainers who might be worth Cc'ing on a patch.
> 
>>
>>
>>  From d6904a5fc90aaf205e982755e4d6cda62ad21273 Mon Sep 17 00:00:00 2001
>> From: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
>> Date: Thu, 22 Feb 2018 12:02:16 +0900
>> Subject: [PATCH 1/2] MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync for
>>   BCM47XX PCIe erratum
>>
>> The erratum and workaround are described by BCM5300X-ES300-RDS.pdf as below.
> 
> Is that document accessible anywhere publicly?
> 
>>
>>    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.
> 
>> This patch is just to enable ExternalSync in the Linux driver.
> 
> I suggest rewording this line to explain how ES helps, maybe something
> along the lines of:
> "Enable ExternalSync in Config7 when CONFIG_BCMA_DRIVER_PCI_HOSTMODE=y
> too so that the sync instruction is externalised..."
> 
> (Best not to refer to "this patch", just say what it does, and in Linux
> terminology this is architecture code, not really a driver).
> 
>>
>> Change-Id: Ifc7a0ce46962933731297ad0c82682e7d39328ff
> 
> You can drop this from upstream submissions in future.
> 
> You also need a signed-off-by line as described in
> Documentation/process/submitting-patches.rst to certify the work as
> complying with the "Developer's Certificate of Origin".
> 
>> Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>   arch/mips/include/asm/mipsregs.h | 2 ++
>>   arch/mips/kernel/cpu-probe.c     | 7 +++++++
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/arch/mips/include/asm/mipsregs.h b/arch/mips/include/asm/mipsregs.h
>> index 858752dac337..1d1f4416a0f3 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)

Since the config7 register is implementation specific, may I suggest 
changing the MIPS_ prefix to something vendor specific such as
BRCM_CONF7_ES and start a new section with a comment like:

/* Config7 Bits specific to Broadcom implementations */

Thanks,
Matt

>>   
>>   #define MIPS_CONF7_IAR		(_ULCAST_(1) << 10)
>>   #define MIPS_CONF7_AR		(_ULCAST_(1) << 16)
>> diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c
>> index cf3fd549e16d..9171928c40dd 100644
>> --- a/arch/mips/kernel/cpu-probe.c
>> +++ b/arch/mips/kernel/cpu-probe.c
>> @@ -429,6 +429,13 @@ static inline void check_errata(void)
>>   		if ((c->processor_id & PRID_REV_MASK) <= PRID_REV_34K_V1_0_2)
>>   			write_c0_config7(read_c0_config7() | MIPS_CONF7_RPS);
>>   		break;
>> +#ifdef CONFIG_BCMA_DRIVER_PCI_HOSTMODE
>> +	case CPU_74K:
>> +		/* Enable ExternalSync for sync instruction to take effect */
> 
> I think it would be helpful to mention the affected device and any
> errata number in this comment.
> 
>> +		pr_info("ExternalSync has been enabled\n");
>> +		write_c0_config7(read_c0_config7() | MIPS_CONF7_ES);
>   
> (I would have suggested adding:
> 
> diff --git a/arch/mips/include/asm/mipsregs.h b/arch/mips/include/asm/mipsregs.h
> index f65859784a4c..af6e59cfc763 100644
> --- a/arch/mips/include/asm/mipsregs.h
> +++ b/arch/mips/include/asm/mipsregs.h
> @@ -2760,6 +2760,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)
> 
> Then you can just do:
> 
> set_c0_config7(MIPS_CONF7_ES);
> 
> But I see the write(read() | x) form is already there in that file, so
> probably best to remain consistent with that. Using set_c0_config7() can
> always be done later as a separate patch.)
> 
> Otherwise the change itself looks reasonable to me.
> 
> Thanks
> James
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync for BCM47XX PCIe erratum
  2018-04-24 12:06   ` Matt Redfearn
@ 2018-04-24 12:32     ` James Hogan
  2018-04-24 16:39       ` IKEGAMI Tokunori
  0 siblings, 1 reply; 10+ messages in thread
From: James Hogan @ 2018-04-24 12:32 UTC (permalink / raw)
  To: Matt Redfearn
  Cc: IKEGAMI Tokunori, linux-mips, PACKHAM Chris, Hauke Mehrtens,
	Rafał Miłecki

[-- Attachment #1: Type: text/plain, Size: 902 bytes --]

On Tue, Apr 24, 2018 at 01:06:14PM +0100, Matt Redfearn wrote:
> >> +/* ExternalSync */
> >> +#define MIPS_CONF7_ES		(_ULCAST_(1) << 8)
> 
> Since the config7 register is implementation specific, may I suggest 
> changing the MIPS_ prefix to something vendor specific such as
> BRCM_CONF7_ES and start a new section with a comment like:
> 
> /* Config7 Bits specific to Broadcom implementations */

See here:

> >> +	case CPU_74K:

So its MIPS 74K specific, and some other cores have it too (I checked
P5600 and interAptiv manuals, so I'd guess most recentish MIPS cores).

So maybe its worth s/MIPS/MTI/ to clarify it isn't part of the MIPS32
architecture.

Note that the same applies to all the CONF6 and CONF7 definitions around
there, but its worth getting this one right now I think, lets not
unnecessarily add a new definition and have to rename it later.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync for BCM47XX PCIe erratum
  2018-04-24 11:49 ` James Hogan
  2018-04-24 12:06   ` Matt Redfearn
@ 2018-04-24 16:00   ` IKEGAMI Tokunori
  1 sibling, 0 replies; 10+ messages in thread
From: IKEGAMI Tokunori @ 2018-04-24 16:00 UTC (permalink / raw)
  To: James Hogan
  Cc: linux-mips, PACKHAM Chris, Hauke Mehrtens, Rafał Miłecki

Hi James-san,

Thanks for your advice.

> Please use git-send-email to send patches in future if possible (you can
> put additional comments that aren't part of the commit message after a
> "---" separator). That should help format it correctly and will put a
> [PATCH] prefix etc.

  Okay I will do that.

> I've also Cc'd Hauke and Rafał who maintain the BCM47XX platform.
> Running scripts/get_maintainer.pl on the patch will list some
> maintainers who might be worth Cc'ing on a patch.

  Noted it.

> > The erratum and workaround are described by BCM5300X-ES300-RDS.pdf as below.
>
> Is that document accessible anywhere publicly?

  Sorry no the document is not opened to the public.
  We should get the document from the Broadcom docSAFE page.
  I think that it can be accessed from the following page.
    <https://www.broadcom.com/support/download-search>
  But at first myBroadcom Account registration and login are required.
  Also probably it needs to ask to access to the documentation on the docSAFE page after that.
    Note: The Broadcom CFE is also implemented the erratum workaround and it can be downloaded from the page.

> > This patch is just to enable ExternalSync in the Linux driver.
>
> I suggest rewording this line to explain how ES helps, maybe something
> along the lines of:
> "Enable ExternalSync in Config7 when CONFIG_BCMA_DRIVER_PCI_HOSTMODE=y
> too so that the sync instruction is externalised..." 

  Okay I will do try this.

> (Best not to refer to "this patch", just say what it does, and in Linux
> terminology this is architecture code, not really a driver).

  Noted it.

> > Change-Id: Ifc7a0ce46962933731297ad0c82682e7d39328ff
>
> You can drop this from upstream submissions in future.

  I see.

> You also need a signed-off-by line as described in
> Documentation/process/submitting-patches.rst to certify the work as
> complying with the "Developer's Certificate of Origin".

  Noted this also.

> > +#ifdef CONFIG_BCMA_DRIVER_PCI_HOSTMODE
> > +	case CPU_74K:
> > +		/* Enable ExternalSync for sync instruction to take effect */
> 
> I think it would be helpful to mention the affected device and any
> errata number in this comment.

  Yes I will do add to mention them.

> > +		pr_info("ExternalSync has been enabled\n");
> > +		write_c0_config7(read_c0_config7() | MIPS_CONF7_ES);
>
> (I would have suggested adding:
>
> diff --git a/arch/mips/include/asm/mipsregs.h b/arch/mips/include/asm/mipsregs.h
> index f65859784a4c..af6e59cfc763 100644
> --- a/arch/mips/include/asm/mipsregs.h
> +++ b/arch/mips/include/asm/mipsregs.h
> @@ -2760,6 +2760,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)
>
> Then you can just do:
>
> set_c0_config7(MIPS_CONF7_ES);
>
> But I see the write(read() | x) form is already there in that file, so
> probably best to remain consistent with that. Using set_c0_config7() can
> always be done later as a separate patch.)

  Thanks for your advice.
  I will do this.

> Otherwise the change itself looks reasonable to me.

  Thanks for the comment.
  But let me mention our concern additionally as below.
  The change will change the timing on external bus.
  So it may cause any unexpected timing issue but not sure.
  Actually on our products the flash driver was caused the write failure.
  It was investigated as that the failure is caused by the driver error.
  So currently I am trying to fix the mtd cfi_cmdset_0002 driver separately.
  Any other issue is not caused on our environment on my test.
  But if the patch is reviewed as okay we will do more testing on our products.
  Sorry for late to notice this.

Regards,
Ikegami

-----Original Message-----
From: James Hogan [mailto:jhogan@kernel.org] 
Sent: Tuesday, April 24, 2018 8:50 PM
To: IKEGAMI Tokunori
Cc: linux-mips@linux-mips.org; PACKHAM Chris; Hauke Mehrtens; Rafał Miłecki
Subject: Re: MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync for BCM47XX PCIe erratum

Hi,

On Tue, Apr 24, 2018 at 07:33:51AM +0000, IKEGAMI Tokunori wrote:
> Let us consult to change MIPS BCM47XX to enable MIPS32 74K Core ExternalSync.
> Can we change the MIPS BCM47XX driver like this?
> If any comment or concern please let us know.

Thanks for the patch.

Please use git-send-email to send patches in future if possible (you can
put additional comments that aren't part of the commit message after a
"---" separator). That should help format it correctly and will put a
[PATCH] prefix etc.

I've also Cc'd Hauke and Rafał who maintain the BCM47XX platform.
Running scripts/get_maintainer.pl on the patch will list some
maintainers who might be worth Cc'ing on a patch.

> 
> 
> From d6904a5fc90aaf205e982755e4d6cda62ad21273 Mon Sep 17 00:00:00 2001
> From: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> Date: Thu, 22 Feb 2018 12:02:16 +0900
> Subject: [PATCH 1/2] MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync for
>  BCM47XX PCIe erratum
> 
> The erratum and workaround are described by BCM5300X-ES300-RDS.pdf as below.

Is that document accessible anywhere publicly?

> 
>   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.

> This patch is just to enable ExternalSync in the Linux driver.

I suggest rewording this line to explain how ES helps, maybe something
along the lines of:
"Enable ExternalSync in Config7 when CONFIG_BCMA_DRIVER_PCI_HOSTMODE=y
too so that the sync instruction is externalised..." 

(Best not to refer to "this patch", just say what it does, and in Linux
terminology this is architecture code, not really a driver).

> 
> Change-Id: Ifc7a0ce46962933731297ad0c82682e7d39328ff

You can drop this from upstream submissions in future.

You also need a signed-off-by line as described in
Documentation/process/submitting-patches.rst to certify the work as
complying with the "Developer's Certificate of Origin".

> Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>  arch/mips/include/asm/mipsregs.h | 2 ++
>  arch/mips/kernel/cpu-probe.c     | 7 +++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/arch/mips/include/asm/mipsregs.h b/arch/mips/include/asm/mipsregs.h
> index 858752dac337..1d1f4416a0f3 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)
> diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c
> index cf3fd549e16d..9171928c40dd 100644
> --- a/arch/mips/kernel/cpu-probe.c
> +++ b/arch/mips/kernel/cpu-probe.c
> @@ -429,6 +429,13 @@ static inline void check_errata(void)
>  		if ((c->processor_id & PRID_REV_MASK) <= PRID_REV_34K_V1_0_2)
>  			write_c0_config7(read_c0_config7() | MIPS_CONF7_RPS);
>  		break;
> +#ifdef CONFIG_BCMA_DRIVER_PCI_HOSTMODE
> +	case CPU_74K:
> +		/* Enable ExternalSync for sync instruction to take effect */

I think it would be helpful to mention the affected device and any
errata number in this comment.

> +		pr_info("ExternalSync has been enabled\n");
> +		write_c0_config7(read_c0_config7() | MIPS_CONF7_ES);
 
(I would have suggested adding:

diff --git a/arch/mips/include/asm/mipsregs.h b/arch/mips/include/asm/mipsregs.h
index f65859784a4c..af6e59cfc763 100644
--- a/arch/mips/include/asm/mipsregs.h
+++ b/arch/mips/include/asm/mipsregs.h
@@ -2760,6 +2760,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)

Then you can just do:

set_c0_config7(MIPS_CONF7_ES);

But I see the write(read() | x) form is already there in that file, so
probably best to remain consistent with that. Using set_c0_config7() can
always be done later as a separate patch.)

Otherwise the change itself looks reasonable to me.

Thanks
James

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* RE: MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync for BCM47XX PCIe erratum
  2018-04-24 12:32     ` James Hogan
@ 2018-04-24 16:39       ` IKEGAMI Tokunori
  0 siblings, 0 replies; 10+ messages in thread
From: IKEGAMI Tokunori @ 2018-04-24 16:39 UTC (permalink / raw)
  To: James Hogan, Matt Redfearn
  Cc: linux-mips, PACKHAM Chris, Hauke Mehrtens, Rafał Miłecki

Hi Matt-san and James-san,

Thanks for your comments.

> > >> +/* ExternalSync */
> > >> +#define MIPS_CONF7_ES		(_ULCAST_(1) << 8)
> > 
> > Since the config7 register is implementation specific, may I suggest 
> > changing the MIPS_ prefix to something vendor specific such as
> > BRCM_CONF7_ES and start a new section with a comment like:
> > 
> > /* Config7 Bits specific to Broadcom implementations */
>
> See here:
>
> > >> +	case CPU_74K:
>
> So its MIPS 74K specific, and some other cores have it too (I checked
> P5600 and interAptiv manuals, so I'd guess most recentish MIPS cores).

  Yes the ExternalSync is described by the MIPS32 74K Software User Manual MD00519-2B-74K-SUM-01.05.pdf.

> So maybe its worth s/MIPS/MTI/ to clarify it isn't part of the MIPS32
> architecture.
>
> Note that the same applies to all the CONF6 and CONF7 definitions around
> there, but its worth getting this one right now I think, lets not
> unnecessarily add a new definition and have to rename it later.

  Okay if needed I will do change so let me know if needed.

Regards,
Ikegami

-----Original Message-----
From: James Hogan [mailto:jhogan@kernel.org] 
Sent: Tuesday, April 24, 2018 9:32 PM
To: Matt Redfearn
Cc: IKEGAMI Tokunori; linux-mips@linux-mips.org; PACKHAM Chris; Hauke Mehrtens; Rafał Miłecki
Subject: Re: MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync for BCM47XX PCIe erratum

On Tue, Apr 24, 2018 at 01:06:14PM +0100, Matt Redfearn wrote:
> >> +/* ExternalSync */
> >> +#define MIPS_CONF7_ES		(_ULCAST_(1) << 8)
> 
> Since the config7 register is implementation specific, may I suggest 
> changing the MIPS_ prefix to something vendor specific such as
> BRCM_CONF7_ES and start a new section with a comment like:
> 
> /* Config7 Bits specific to Broadcom implementations */

See here:

> >> +	case CPU_74K:

So its MIPS 74K specific, and some other cores have it too (I checked
P5600 and interAptiv manuals, so I'd guess most recentish MIPS cores).

So maybe its worth s/MIPS/MTI/ to clarify it isn't part of the MIPS32
architecture.

Note that the same applies to all the CONF6 and CONF7 definitions around
there, but its worth getting this one right now I think, lets not
unnecessarily add a new definition and have to rename it later.

Cheers
James

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync for BCM47XX PCIe erratum
  2018-04-24  7:33 MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync for BCM47XX PCIe erratum IKEGAMI Tokunori
  2018-04-24 11:49 ` James Hogan
@ 2018-04-24 19:19 ` smtpuser
  2018-04-24 21:42   ` Hauke Mehrtens
  1 sibling, 1 reply; 10+ messages in thread
From: smtpuser @ 2018-04-24 19:19 UTC (permalink / raw)
  To: James Hogan
  Cc: Tokunori Ikegami, Chris Packham, Hauke Mehrtens,
	Rafał Miłecki, linux-mips

From: Tokunori Ikegami <ikegami@allied-telesis.co.jp>

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.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: Hauke Mehrtens <hauke@hauke-m.de>
Cc: Rafał Miłecki <zajec5@gmail.com>
Cc: linux-mips@linux-mips.org
---
I have just fixed your comments.
This patch will be sent by git-send-email.
Also for our company mail system the sender mail address is needed to be set as smtpuser <smtpuser@allied-telesis.co.jp>.
But do not reply to the email address smtpuser <smtpuser@allied-telesis.co.jp>.
Please reply to my email address if any comemnt or problem.
Sorry for inconvinient about this.

 arch/mips/include/asm/mipsregs.h |  3 +++
 arch/mips/kernel/cpu-probe.c     | 12 +++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

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)
diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c
index cf3fd549e16d..75039e89694f 100644
--- a/arch/mips/kernel/cpu-probe.c
+++ b/arch/mips/kernel/cpu-probe.c
@@ -427,8 +427,18 @@ static inline void check_errata(void)
 		 * making use of VPE1 will be responsable for that VPE.
 		 */
 		if ((c->processor_id & PRID_REV_MASK) <= PRID_REV_34K_V1_0_2)
-			write_c0_config7(read_c0_config7() | MIPS_CONF7_RPS);
+			set_c0_config7(MIPS_CONF7_RPS);
 		break;
+#ifdef CONFIG_BCMA_DRIVER_PCI_HOSTMODE
+	case CPU_74K:
+		/*
+		 * BCM47XX Erratum "R10: PCIe Transactions Periodically Fail"
+		 * Enable ExternalSync for sync instruction to take effect
+		 */
+		pr_info("ExternalSync has been enabled\n");
+		set_c0_config7(MIPS_CONF7_ES);
+		break;
+#endif
 	default:
 		break;
 	}
-- 
2.16.1

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync for BCM47XX PCIe erratum
  2018-04-24 19:19 ` [PATCH] " smtpuser
@ 2018-04-24 21:42   ` Hauke Mehrtens
  2018-04-25  1:28     ` IKEGAMI Tokunori
  2018-05-28  0:34     ` IKEGAMI Tokunori
  0 siblings, 2 replies; 10+ messages in thread
From: Hauke Mehrtens @ 2018-04-24 21:42 UTC (permalink / raw)
  To: Tokunori Ikegami
  Cc: James Hogan, Chris Packham, Rafał Miłecki, linux-mips

On 04/24/2018 09:19 PM, smtpuser wrote:
> From: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> 
> 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.
> 
> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Cc: Hauke Mehrtens <hauke@hauke-m.de>
> Cc: Rafał Miłecki <zajec5@gmail.com>
> Cc: linux-mips@linux-mips.org
> ---
> I have just fixed your comments.
> This patch will be sent by git-send-email.
> Also for our company mail system the sender mail address is needed to be set as smtpuser <smtpuser@allied-telesis.co.jp>.
> But do not reply to the email address smtpuser <smtpuser@allied-telesis.co.jp>.
> Please reply to my email address if any comemnt or problem.
> Sorry for inconvinient about this.

Does the CFE boot loader normally apply this workaround? All devices I
have with this SoC use CFE to boot Linux and I am wondering why "only"
the workaround in the driver helped to workaround this problem.

Not all Broadcom MIPS SoCs from the BRCM47xx line with MIPS 74K CPUs are
affected.

See here, we only apply this for the BCM4716 SoCs:
https://git.kernel.org/linus/25c15566635fef86e87f762f73a19f24598e45fa

Here the brcmsmac driver provided by Broadcom, it says that only BCM4716
and BCM4706 are affected:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/broadcom/brcm80211/brcmsmac/types.h#n255

I am not sure if the version of the bcm4706 which is used on most
devices is affected as this workaround is not applied for this SoC in
b43, could be that Broadcom fixed that in a later revision and the
broken revision never made it into mass production.

Do you ses the problem on the BCM53001 I think this is the same silicon
as the bcm4706?

>  arch/mips/include/asm/mipsregs.h |  3 +++
>  arch/mips/kernel/cpu-probe.c     | 12 +++++++++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> 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)
> diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c
> index cf3fd549e16d..75039e89694f 100644
> --- a/arch/mips/kernel/cpu-probe.c
> +++ b/arch/mips/kernel/cpu-probe.c
> @@ -427,8 +427,18 @@ static inline void check_errata(void)
>  		 * making use of VPE1 will be responsable for that VPE.
>  		 */
>  		if ((c->processor_id & PRID_REV_MASK) <= PRID_REV_34K_V1_0_2)
> -			write_c0_config7(read_c0_config7() | MIPS_CONF7_RPS);
> +			set_c0_config7(MIPS_CONF7_RPS);
>  		break;
> +#ifdef CONFIG_BCMA_DRIVER_PCI_HOSTMODE
> +	case CPU_74K:
> +		/*
> +		 * BCM47XX Erratum "R10: PCIe Transactions Periodically Fail"
> +		 * Enable ExternalSync for sync instruction to take effect
> +		 */
> +		pr_info("ExternalSync has been enabled\n");
> +		set_c0_config7(MIPS_CONF7_ES);
> +		break;
> +#endif
>  	default:
>  		break;
>  	}
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync for BCM47XX PCIe erratum
  2018-04-24 21:42   ` Hauke Mehrtens
@ 2018-04-25  1:28     ` IKEGAMI Tokunori
  2018-05-28  0:34     ` IKEGAMI Tokunori
  1 sibling, 0 replies; 10+ messages in thread
From: IKEGAMI Tokunori @ 2018-04-25  1:28 UTC (permalink / raw)
  To: Hauke Mehrtens
  Cc: James Hogan, PACKHAM Chris, Rafał Miłecki, linux-mips



> -----Original Message-----
> From: Hauke Mehrtens [mailto:hauke@hauke-m.de]
> Sent: Wednesday, April 25, 2018 6:42 AM
> To: IKEGAMI Tokunori
> Cc: James Hogan; PACKHAM Chris; Rafał Miłecki; linux-mips@linux-mips.org
> Subject: Re: [PATCH] MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync
> for BCM47XX PCIe erratum
> 
> On 04/24/2018 09:19 PM, smtpuser wrote:
> > From: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> >
> > 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.
> >
> > Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > Cc: Hauke Mehrtens <hauke@hauke-m.de>
> > Cc: Rafał Miłecki <zajec5@gmail.com>
> > Cc: linux-mips@linux-mips.org
> > ---
> > I have just fixed your comments.
> > This patch will be sent by git-send-email.
> > Also for our company mail system the sender mail address is needed to
> be set as smtpuser <smtpuser@allied-telesis.co.jp>.
> > But do not reply to the email address smtpuser
> <smtpuser@allied-telesis.co.jp>.
> > Please reply to my email address if any comemnt or problem.
> > Sorry for inconvinient about this.
> 
> Does the CFE boot loader normally apply this workaround? All devices I
> have with this SoC use CFE to boot Linux and I am wondering why "only"
> the workaround in the driver helped to workaround this problem.
> 
> Not all Broadcom MIPS SoCs from the BRCM47xx line with MIPS 74K CPUs are
> affected.
> 
> See here, we only apply this for the BCM4716 SoCs:
> https://git.kernel.org/linus/25c15566635fef86e87f762f73a19f24598e45fa
> 
> Here the brcmsmac driver provided by Broadcom, it says that only BCM4716
> and BCM4706 are affected:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tre
> e/drivers/net/wireless/broadcom/brcm80211/brcmsmac/types.h#n255
> 
> I am not sure if the version of the bcm4706 which is used on most
> devices is affected as this workaround is not applied for this SoC in
> b43, could be that Broadcom fixed that in a later revision and the
> broken revision never made it into mass production.

Thank you so much for your comments.
The CFE is implemented to enable the ExternalSync on the bcmsi chip set those are defined as mips74k CPU also.
Also for other boards for example bcm94704cpci that I think that this is for BCM4704 the CPU is defined as bcmcore, sb1250 or mpc8245 but not defined as mips74k.
I have just confirmed that BCM5357x family uses ARM CPU so it seems no problem.
I am not sure if there is any other MIPS 74K CPU enabled CONFIG_BCMA_DRIVER_PCI_HOSTMODE option.
But if needed to make sure I can change to use CONFIG_BCM47XX for the change instead of CONFIG_BCMA_DRIVER_PCI_HOSTMODE.
So please let me know if needed to change.
  Note: For our BCM53003 CPU if possible we would like to use CONFIG_BCMA_DRIVER_PCI_HOSTMODE as current patch.

> Do you ses the problem on the BCM53001 I think this is the same silicon
> as the bcm4706?

Yes we are using BCM53003 and the BCM53003 chip ID is same with BCM4706.
So it looks that the BCM53001 is also same.
At this moment I could not confirm the actual erratum error behavior.

> >  arch/mips/include/asm/mipsregs.h |  3 +++
> >  arch/mips/kernel/cpu-probe.c     | 12 +++++++++++-
> >  2 files changed, 14 insertions(+), 1 deletion(-)
> >
> > 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)
> > diff --git a/arch/mips/kernel/cpu-probe.c
> b/arch/mips/kernel/cpu-probe.c
> > index cf3fd549e16d..75039e89694f 100644
> > --- a/arch/mips/kernel/cpu-probe.c
> > +++ b/arch/mips/kernel/cpu-probe.c
> > @@ -427,8 +427,18 @@ static inline void check_errata(void)
> >  		 * making use of VPE1 will be responsable for that VPE.
> >  		 */
> >  		if ((c->processor_id & PRID_REV_MASK) <=
> PRID_REV_34K_V1_0_2)
> > -			write_c0_config7(read_c0_config7() |
> MIPS_CONF7_RPS);
> > +			set_c0_config7(MIPS_CONF7_RPS);
> >  		break;
> > +#ifdef CONFIG_BCMA_DRIVER_PCI_HOSTMODE
> > +	case CPU_74K:
> > +		/*
> > +		 * BCM47XX Erratum "R10: PCIe Transactions Periodically
> Fail"
> > +		 * Enable ExternalSync for sync instruction to take effect
> > +		 */
> > +		pr_info("ExternalSync has been enabled\n");
> > +		set_c0_config7(MIPS_CONF7_ES);
> > +		break;
> > +#endif
> >  	default:
> >  		break;
> >  	}
> >


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync for BCM47XX PCIe erratum
  2018-04-24 21:42   ` Hauke Mehrtens
  2018-04-25  1:28     ` IKEGAMI Tokunori
@ 2018-05-28  0:34     ` IKEGAMI Tokunori
  1 sibling, 0 replies; 10+ messages in thread
From: IKEGAMI Tokunori @ 2018-05-28  0:34 UTC (permalink / raw)
  To: Hauke Mehrtens
  Cc: James Hogan, PACKHAM Chris, Rafał Miłecki, linux-mips

Hi,

I have just sent the v2 version patch with cover letter so please review again.
The patch itself is not changed from previous version that fixed from original version.
But it is not in patchwork so let me send this as the v2 version.
Please review the patch again and if any concern or problem please let me know.
We would like to resolve the issue caused on our products by this patch.

Regards,
Ikegami

> -----Original Message-----
> From: IKEGAMI Tokunori
> Sent: Wednesday, April 25, 2018 10:29 AM
> To: 'Hauke Mehrtens'
> Cc: James Hogan; PACKHAM Chris; Rafał Miłecki; linux-mips@linux-mips.org
> Subject: RE: [PATCH] MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync
> for BCM47XX PCIe erratum
> 
> 
> 
> > -----Original Message-----
> > From: Hauke Mehrtens [mailto:hauke@hauke-m.de]
> > Sent: Wednesday, April 25, 2018 6:42 AM
> > To: IKEGAMI Tokunori
> > Cc: James Hogan; PACKHAM Chris; Rafał Miłecki; linux-mips@linux-mips.org
> > Subject: Re: [PATCH] MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync
> > for BCM47XX PCIe erratum
> >
> > On 04/24/2018 09:19 PM, smtpuser wrote:
> > > From: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> > >
> > > 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.
> > >
> > > Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> > > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > > Cc: Hauke Mehrtens <hauke@hauke-m.de>
> > > Cc: Rafał Miłecki <zajec5@gmail.com>
> > > Cc: linux-mips@linux-mips.org
> > > ---
> > > I have just fixed your comments.
> > > This patch will be sent by git-send-email.
> > > Also for our company mail system the sender mail address is needed to
> > be set as smtpuser <smtpuser@allied-telesis.co.jp>.
> > > But do not reply to the email address smtpuser
> > <smtpuser@allied-telesis.co.jp>.
> > > Please reply to my email address if any comemnt or problem.
> > > Sorry for inconvinient about this.
> >
> > Does the CFE boot loader normally apply this workaround? All devices I
> > have with this SoC use CFE to boot Linux and I am wondering why "only"
> > the workaround in the driver helped to workaround this problem.
> >
> > Not all Broadcom MIPS SoCs from the BRCM47xx line with MIPS 74K CPUs are
> > affected.
> >
> > See here, we only apply this for the BCM4716 SoCs:
> > https://git.kernel.org/linus/25c15566635fef86e87f762f73a19f24598e45fa
> >
> > Here the brcmsmac driver provided by Broadcom, it says that only BCM4716
> > and BCM4706 are affected:
> >
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tre
> > e/drivers/net/wireless/broadcom/brcm80211/brcmsmac/types.h#n255
> >
> > I am not sure if the version of the bcm4706 which is used on most
> > devices is affected as this workaround is not applied for this SoC in
> > b43, could be that Broadcom fixed that in a later revision and the
> > broken revision never made it into mass production.
> 
> Thank you so much for your comments.
> The CFE is implemented to enable the ExternalSync on the bcmsi chip set
> those are defined as mips74k CPU also.
> Also for other boards for example bcm94704cpci that I think that this is
> for BCM4704 the CPU is defined as bcmcore, sb1250 or mpc8245 but not defined
> as mips74k.
> I have just confirmed that BCM5357x family uses ARM CPU so it seems no
> problem.
> I am not sure if there is any other MIPS 74K CPU enabled
> CONFIG_BCMA_DRIVER_PCI_HOSTMODE option.
> But if needed to make sure I can change to use CONFIG_BCM47XX for the change
> instead of CONFIG_BCMA_DRIVER_PCI_HOSTMODE.
> So please let me know if needed to change.
>   Note: For our BCM53003 CPU if possible we would like to use
> CONFIG_BCMA_DRIVER_PCI_HOSTMODE as current patch.
> 
> > Do you ses the problem on the BCM53001 I think this is the same silicon
> > as the bcm4706?
> 
> Yes we are using BCM53003 and the BCM53003 chip ID is same with BCM4706.
> So it looks that the BCM53001 is also same.
> At this moment I could not confirm the actual erratum error behavior.
> 
> > >  arch/mips/include/asm/mipsregs.h |  3 +++
> > >  arch/mips/kernel/cpu-probe.c     | 12 +++++++++++-
> > >  2 files changed, 14 insertions(+), 1 deletion(-)
> > >
> > > 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)
> > > diff --git a/arch/mips/kernel/cpu-probe.c
> > b/arch/mips/kernel/cpu-probe.c
> > > index cf3fd549e16d..75039e89694f 100644
> > > --- a/arch/mips/kernel/cpu-probe.c
> > > +++ b/arch/mips/kernel/cpu-probe.c
> > > @@ -427,8 +427,18 @@ static inline void check_errata(void)
> > >  		 * making use of VPE1 will be responsable for that VPE.
> > >  		 */
> > >  		if ((c->processor_id & PRID_REV_MASK) <=
> > PRID_REV_34K_V1_0_2)
> > > -			write_c0_config7(read_c0_config7() |
> > MIPS_CONF7_RPS);
> > > +			set_c0_config7(MIPS_CONF7_RPS);
> > >  		break;
> > > +#ifdef CONFIG_BCMA_DRIVER_PCI_HOSTMODE
> > > +	case CPU_74K:
> > > +		/*
> > > +		 * BCM47XX Erratum "R10: PCIe Transactions Periodically
> > Fail"
> > > +		 * Enable ExternalSync for sync instruction to take effect
> > > +		 */
> > > +		pr_info("ExternalSync has been enabled\n");
> > > +		set_c0_config7(MIPS_CONF7_ES);
> > > +		break;
> > > +#endif
> > >  	default:
> > >  		break;
> > >  	}
> > >


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-05-28  0:35 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-24  7:33 MIPS: BCM47XX: Enable MIPS32 74K Core ExternalSync for BCM47XX PCIe erratum IKEGAMI Tokunori
2018-04-24 11:49 ` James Hogan
2018-04-24 12:06   ` Matt Redfearn
2018-04-24 12:32     ` James Hogan
2018-04-24 16:39       ` IKEGAMI Tokunori
2018-04-24 16:00   ` IKEGAMI Tokunori
2018-04-24 19:19 ` [PATCH] " smtpuser
2018-04-24 21:42   ` Hauke Mehrtens
2018-04-25  1:28     ` IKEGAMI Tokunori
2018-05-28  0:34     ` IKEGAMI Tokunori

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.