Since commit 3af5a67c86a3 ("MIPS: Fix early CM probing") the MT7621
has not been able to boot.
This patched caused mips_cm_probe() to be called before
mt7621.c::proc_soc_init().
prom_soc_init() has a comment explaining that mips_cm_probe()
"wipes out the bootloader config" and means that configuration
registers are no longer available. It has some code to re-enable
this config.
Before this re-enable code is run, the sysc register cannot be
read, so when SYSC_REG_CHIP_NAME0 is read, a garbage value
is returned and panic() is called.
If we move the config-repair code to the top of prom_soc_init(),
the registers can be read and boot can proceed.
Fixes: 3af5a67c86a3 ("MIPS: Fix early CM probing")
Signed-off-by: NeilBrown <[email protected]>
---
arch/mips/ralink/mt7621.c | 41 +++++++++++++++++++++--------------------
1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/arch/mips/ralink/mt7621.c b/arch/mips/ralink/mt7621.c
index 1b274742077d..5a1b19bebd5b 100644
--- a/arch/mips/ralink/mt7621.c
+++ b/arch/mips/ralink/mt7621.c
@@ -170,6 +170,27 @@ void prom_soc_init(struct ralink_soc_info *soc_info)
u32 n1;
u32 rev;
+ /* Early detection of CMP support */
+ mips_cm_probe();
+ mips_cpc_probe();
+
+ if (mips_cps_numiocu(0)) {
+ /*
+ * mips_cm_probe() wipes out bootloader
+ * config for CM regions and we have to configure them
+ * again. This SoC cannot talk to pamlbus devices
+ * witout proper iocu region set up.
+ *
+ * FIXME: it would be better to do this with values
+ * from DT, but we need this very early because
+ * without this we cannot talk to pretty much anything
+ * including serial.
+ */
+ write_gcr_reg0_base(MT7621_PALMBUS_BASE);
+ write_gcr_reg0_mask(~MT7621_PALMBUS_SIZE |
+ CM_GCR_REGn_MASK_CMTGT_IOCU0);
+ }
+
n0 = __raw_readl(sysc + SYSC_REG_CHIP_NAME0);
n1 = __raw_readl(sysc + SYSC_REG_CHIP_NAME1);
@@ -194,26 +215,6 @@ void prom_soc_init(struct ralink_soc_info *soc_info)
rt2880_pinmux_data = mt7621_pinmux_data;
- /* Early detection of CMP support */
- mips_cm_probe();
- mips_cpc_probe();
-
- if (mips_cps_numiocu(0)) {
- /*
- * mips_cm_probe() wipes out bootloader
- * config for CM regions and we have to configure them
- * again. This SoC cannot talk to pamlbus devices
- * witout proper iocu region set up.
- *
- * FIXME: it would be better to do this with values
- * from DT, but we need this very early because
- * without this we cannot talk to pretty much anything
- * including serial.
- */
- write_gcr_reg0_base(MT7621_PALMBUS_BASE);
- write_gcr_reg0_mask(~MT7621_PALMBUS_SIZE |
- CM_GCR_REGn_MASK_CMTGT_IOCU0);
- }
if (!register_cps_smp_ops())
return;
--
2.14.0.rc0.dirty
Further testing showed that the original version of this
patch wasn't 100% reliable. Very occasionally the read
of SYSC_REG_CHIP_NAME0 returns garbage. Repeating the
read seems to be reliable, but it hasn't happened enough
for me to be completely confident.
So this version repeats that first read.
Thanks,
NeilBrown
----------------8<--------------------
Since commit 3af5a67c86a3 ("MIPS: Fix early CM probing") the MT7621
has not been able to boot.
This patched caused mips_cm_probe() to be called before
mt7621.c::proc_soc_init().
prom_soc_init() has a comment explaining that mips_cm_probe()
"wipes out the bootloader config" and means that configuration
registers are no longer available. It has some code to re-enable
this config.
Before this re-enable code is run, the sysc register cannot be
read, so when SYSC_REG_CHIP_NAME0 is read, a garbage value
is returned and panic() is called.
If we move the config-repair code to the top of prom_soc_init(),
the registers can be read and boot can proceed.
Very occasionally, the first register read after the reconfiguration
returns garbage. So repeat that read to be on the safe side.
Fixes: 3af5a67c86a3 ("MIPS: Fix early CM probing")
Signed-off-by: NeilBrown <[email protected]>
---
arch/mips/ralink/mt7621.c | 43 +++++++++++++++++++++++--------------------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/arch/mips/ralink/mt7621.c b/arch/mips/ralink/mt7621.c
index 1b274742077d..c37716407fbe 100644
--- a/arch/mips/ralink/mt7621.c
+++ b/arch/mips/ralink/mt7621.c
@@ -170,6 +170,29 @@ void prom_soc_init(struct ralink_soc_info *soc_info)
u32 n1;
u32 rev;
+ /* Early detection of CMP support */
+ mips_cm_probe();
+ mips_cpc_probe();
+
+ if (mips_cps_numiocu(0)) {
+ /*
+ * mips_cm_probe() wipes out bootloader
+ * config for CM regions and we have to configure them
+ * again. This SoC cannot talk to pamlbus devices
+ * witout proper iocu region set up.
+ *
+ * FIXME: it would be better to do this with values
+ * from DT, but we need this very early because
+ * without this we cannot talk to pretty much anything
+ * including serial.
+ */
+ write_gcr_reg0_base(MT7621_PALMBUS_BASE);
+ write_gcr_reg0_mask(~MT7621_PALMBUS_SIZE |
+ CM_GCR_REGn_MASK_CMTGT_IOCU0);
+ }
+
+ n0 = __raw_readl(sysc + SYSC_REG_CHIP_NAME0);
+ /* Sometimes first read returns garbage, so try again to be safe */
n0 = __raw_readl(sysc + SYSC_REG_CHIP_NAME0);
n1 = __raw_readl(sysc + SYSC_REG_CHIP_NAME1);
@@ -194,26 +217,6 @@ void prom_soc_init(struct ralink_soc_info *soc_info)
rt2880_pinmux_data = mt7621_pinmux_data;
- /* Early detection of CMP support */
- mips_cm_probe();
- mips_cpc_probe();
-
- if (mips_cps_numiocu(0)) {
- /*
- * mips_cm_probe() wipes out bootloader
- * config for CM regions and we have to configure them
- * again. This SoC cannot talk to pamlbus devices
- * witout proper iocu region set up.
- *
- * FIXME: it would be better to do this with values
- * from DT, but we need this very early because
- * without this we cannot talk to pretty much anything
- * including serial.
- */
- write_gcr_reg0_base(MT7621_PALMBUS_BASE);
- write_gcr_reg0_mask(~MT7621_PALMBUS_SIZE |
- CM_GCR_REGn_MASK_CMTGT_IOCU0);
- }
if (!register_cps_smp_ops())
return;
--
2.14.0.rc0.dirty
Hi Neil,
On 20/03/18 08:22, NeilBrown wrote:
>
> Further testing showed that the original version of this
> patch wasn't 100% reliable. Very occasionally the read
> of SYSC_REG_CHIP_NAME0 returns garbage. Repeating the
> read seems to be reliable, but it hasn't happened enough
> for me to be completely confident.
> So this version repeats that first read.
You almost certainly need a sync() to ensure that the write to gcr_reg0
has completed before attempting to read sysc + SYSC_REG_CHIP_NAME0.
>
> Thanks,
> NeilBrown
>
>
> ----------------8<--------------------
> Since commit 3af5a67c86a3 ("MIPS: Fix early CM probing") the MT7621
> has not been able to boot.
>
> This patched caused mips_cm_probe() to be called before
> mt7621.c::proc_soc_init().
>
> prom_soc_init() has a comment explaining that mips_cm_probe()
> "wipes out the bootloader config" and means that configuration
> registers are no longer available. It has some code to re-enable
> this config.
>
> Before this re-enable code is run, the sysc register cannot be
> read, so when SYSC_REG_CHIP_NAME0 is read, a garbage value
> is returned and panic() is called.
>
> If we move the config-repair code to the top of prom_soc_init(),
> the registers can be read and boot can proceed.
>
> Very occasionally, the first register read after the reconfiguration
> returns garbage. So repeat that read to be on the safe side.
>
> Fixes: 3af5a67c86a3 ("MIPS: Fix early CM probing")
> Signed-off-by: NeilBrown <[email protected]>
> ---
> arch/mips/ralink/mt7621.c | 43 +++++++++++++++++++++++--------------------
> 1 file changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/arch/mips/ralink/mt7621.c b/arch/mips/ralink/mt7621.c
> index 1b274742077d..c37716407fbe 100644
> --- a/arch/mips/ralink/mt7621.c
> +++ b/arch/mips/ralink/mt7621.c
> @@ -170,6 +170,29 @@ void prom_soc_init(struct ralink_soc_info *soc_info)
> u32 n1;
> u32 rev;
>
> + /* Early detection of CMP support */
> + mips_cm_probe();
> + mips_cpc_probe();
> +
> + if (mips_cps_numiocu(0)) {
> + /*
> + * mips_cm_probe() wipes out bootloader
> + * config for CM regions and we have to configure them
> + * again. This SoC cannot talk to pamlbus devices
> + * witout proper iocu region set up.
> + *
> + * FIXME: it would be better to do this with values
> + * from DT, but we need this very early because
> + * without this we cannot talk to pretty much anything
> + * including serial.
> + */
> + write_gcr_reg0_base(MT7621_PALMBUS_BASE);
> + write_gcr_reg0_mask(~MT7621_PALMBUS_SIZE |
> + CM_GCR_REGn_MASK_CMTGT_IOCU0);
i.e. Try putting a sync() here.
> + }
> +
> + n0 = __raw_readl(sysc + SYSC_REG_CHIP_NAME0);
> + /* Sometimes first read returns garbage, so try again to be safe */
Rather than doing this, which is a bit of a hack and there's no
guarantee the second read won't also read garbage without the barrier.
Thanks,
Matt
> n0 = __raw_readl(sysc + SYSC_REG_CHIP_NAME0);
> n1 = __raw_readl(sysc + SYSC_REG_CHIP_NAME1);
>
> @@ -194,26 +217,6 @@ void prom_soc_init(struct ralink_soc_info *soc_info)
>
> rt2880_pinmux_data = mt7621_pinmux_data;
>
> - /* Early detection of CMP support */
> - mips_cm_probe();
> - mips_cpc_probe();
> -
> - if (mips_cps_numiocu(0)) {
> - /*
> - * mips_cm_probe() wipes out bootloader
> - * config for CM regions and we have to configure them
> - * again. This SoC cannot talk to pamlbus devices
> - * witout proper iocu region set up.
> - *
> - * FIXME: it would be better to do this with values
> - * from DT, but we need this very early because
> - * without this we cannot talk to pretty much anything
> - * including serial.
> - */
> - write_gcr_reg0_base(MT7621_PALMBUS_BASE);
> - write_gcr_reg0_mask(~MT7621_PALMBUS_SIZE |
> - CM_GCR_REGn_MASK_CMTGT_IOCU0);
> - }
>
> if (!register_cps_smp_ops())
> return;
>
On Tue, Mar 20 2018, Matt Redfearn wrote:
> Hi Neil,
>
>
> On 20/03/18 08:22, NeilBrown wrote:
>>
>> Further testing showed that the original version of this
>> patch wasn't 100% reliable. Very occasionally the read
>> of SYSC_REG_CHIP_NAME0 returns garbage. Repeating the
>> read seems to be reliable, but it hasn't happened enough
>> for me to be completely confident.
>> So this version repeats that first read.
>
> You almost certainly need a sync() to ensure that the write to gcr_reg0
> has completed before attempting to read sysc + SYSC_REG_CHIP_NAME0.
That sound like exactly the right sort of thing to do, though
I assume you mean __sync().
I tried to reproduce the problem so I could test the fix, and of course
I failed. Over 700 reboot cycles and never read any garbage from
SYSC_REG_CHIP_NAME0.
So I cannot test that this works, but I have tested that it doesn't
cause any obvious regression.
I'll send the v3 patch separately.
Thanks a lot,
NeilBrown
Since commit 3af5a67c86a3 ("MIPS: Fix early CM probing") the MT7621
has not been able to boot.
This patched caused mips_cm_probe() to be called before
mt7621.c::proc_soc_init().
prom_soc_init() has a comment explaining that mips_cm_probe()
"wipes out the bootloader config" and means that configuration
registers are no longer available. It has some code to re-enable
this config.
Before this re-enable code is run, the sysc register cannot be
read, so when SYSC_REG_CHIP_NAME0 is read, a garbage value
is returned and panic() is called.
If we move the config-repair code to the top of prom_soc_init(),
the registers can be read and boot can proceed.
Very occasionally, the first register read after the reconfiguration
returns garbage. So I added a call to __sync().
Fixes: 3af5a67c86a3 ("MIPS: Fix early CM probing")
Signed-off-by: NeilBrown <[email protected]>
---
arch/mips/ralink/mt7621.c | 42 ++++++++++++++++++++++--------------------
1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/arch/mips/ralink/mt7621.c b/arch/mips/ralink/mt7621.c
index 1b274742077d..d2718de60b9b 100644
--- a/arch/mips/ralink/mt7621.c
+++ b/arch/mips/ralink/mt7621.c
@@ -170,6 +170,28 @@ void prom_soc_init(struct ralink_soc_info *soc_info)
u32 n1;
u32 rev;
+ /* Early detection of CMP support */
+ mips_cm_probe();
+ mips_cpc_probe();
+
+ if (mips_cps_numiocu(0)) {
+ /*
+ * mips_cm_probe() wipes out bootloader
+ * config for CM regions and we have to configure them
+ * again. This SoC cannot talk to pamlbus devices
+ * witout proper iocu region set up.
+ *
+ * FIXME: it would be better to do this with values
+ * from DT, but we need this very early because
+ * without this we cannot talk to pretty much anything
+ * including serial.
+ */
+ write_gcr_reg0_base(MT7621_PALMBUS_BASE);
+ write_gcr_reg0_mask(~MT7621_PALMBUS_SIZE |
+ CM_GCR_REGn_MASK_CMTGT_IOCU0);
+ __sync();
+ }
+
n0 = __raw_readl(sysc + SYSC_REG_CHIP_NAME0);
n1 = __raw_readl(sysc + SYSC_REG_CHIP_NAME1);
@@ -194,26 +216,6 @@ void prom_soc_init(struct ralink_soc_info *soc_info)
rt2880_pinmux_data = mt7621_pinmux_data;
- /* Early detection of CMP support */
- mips_cm_probe();
- mips_cpc_probe();
-
- if (mips_cps_numiocu(0)) {
- /*
- * mips_cm_probe() wipes out bootloader
- * config for CM regions and we have to configure them
- * again. This SoC cannot talk to pamlbus devices
- * witout proper iocu region set up.
- *
- * FIXME: it would be better to do this with values
- * from DT, but we need this very early because
- * without this we cannot talk to pretty much anything
- * including serial.
- */
- write_gcr_reg0_base(MT7621_PALMBUS_BASE);
- write_gcr_reg0_mask(~MT7621_PALMBUS_SIZE |
- CM_GCR_REGn_MASK_CMTGT_IOCU0);
- }
if (!register_cps_smp_ops())
return;
--
2.14.0.rc0.dirty
Hi Neil,
On 21/03/18 03:00, NeilBrown wrote:
> On Tue, Mar 20 2018, Matt Redfearn wrote:
>
>> Hi Neil,
>>
>>
>> On 20/03/18 08:22, NeilBrown wrote:
>>>
>>> Further testing showed that the original version of this
>>> patch wasn't 100% reliable. Very occasionally the read
>>> of SYSC_REG_CHIP_NAME0 returns garbage. Repeating the
>>> read seems to be reliable, but it hasn't happened enough
>>> for me to be completely confident.
>>> So this version repeats that first read.
>>
>> You almost certainly need a sync() to ensure that the write to gcr_reg0
>> has completed before attempting to read sysc + SYSC_REG_CHIP_NAME0.
>
> That sound like exactly the right sort of thing to do, though
> I assume you mean __sync().
Indeed I did :-)
>
> I tried to reproduce the problem so I could test the fix, and of course
> I failed. Over 700 reboot cycles and never read any garbage from
> SYSC_REG_CHIP_NAME0.
Funny how things conspire like that :-) __sync() is definitely the
correct barrier required to ensure the write completes before the read
begins and will guarantee that the memory operations are ordered.
Thanks,
Matt
>
> So I cannot test that this works, but I have tested that it doesn't
> cause any obvious regression.
> I'll send the v3 patch separately.
>
> Thanks a lot,
> NeilBrown
>
On 21/03/18 03:02, NeilBrown wrote:
>
> Since commit 3af5a67c86a3 ("MIPS: Fix early CM probing") the MT7621
> has not been able to boot.
>
> This patched caused mips_cm_probe() to be called before
> mt7621.c::proc_soc_init().
>
> prom_soc_init() has a comment explaining that mips_cm_probe()
> "wipes out the bootloader config" and means that configuration
> registers are no longer available. It has some code to re-enable
> this config.
>
> Before this re-enable code is run, the sysc register cannot be
> read, so when SYSC_REG_CHIP_NAME0 is read, a garbage value
> is returned and panic() is called.
>
> If we move the config-repair code to the top of prom_soc_init(),
> the registers can be read and boot can proceed.
>
> Very occasionally, the first register read after the reconfiguration
> returns garbage. So I added a call to __sync().
>
> Fixes: 3af5a67c86a3 ("MIPS: Fix early CM probing")
> Signed-off-by: NeilBrown <[email protected]>
Looks good to me
Reviewed-by: Matt Redfearn <[email protected]>
> ---
> arch/mips/ralink/mt7621.c | 42 ++++++++++++++++++++++--------------------
> 1 file changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/arch/mips/ralink/mt7621.c b/arch/mips/ralink/mt7621.c
> index 1b274742077d..d2718de60b9b 100644
> --- a/arch/mips/ralink/mt7621.c
> +++ b/arch/mips/ralink/mt7621.c
> @@ -170,6 +170,28 @@ void prom_soc_init(struct ralink_soc_info *soc_info)
> u32 n1;
> u32 rev;
>
> + /* Early detection of CMP support */
> + mips_cm_probe();
> + mips_cpc_probe();
> +
> + if (mips_cps_numiocu(0)) {
> + /*
> + * mips_cm_probe() wipes out bootloader
> + * config for CM regions and we have to configure them
> + * again. This SoC cannot talk to pamlbus devices
> + * witout proper iocu region set up.
> + *
> + * FIXME: it would be better to do this with values
> + * from DT, but we need this very early because
> + * without this we cannot talk to pretty much anything
> + * including serial.
> + */
> + write_gcr_reg0_base(MT7621_PALMBUS_BASE);
> + write_gcr_reg0_mask(~MT7621_PALMBUS_SIZE |
> + CM_GCR_REGn_MASK_CMTGT_IOCU0);
> + __sync();
> + }
> +
> n0 = __raw_readl(sysc + SYSC_REG_CHIP_NAME0);
> n1 = __raw_readl(sysc + SYSC_REG_CHIP_NAME1);
>
> @@ -194,26 +216,6 @@ void prom_soc_init(struct ralink_soc_info *soc_info)
>
> rt2880_pinmux_data = mt7621_pinmux_data;
>
> - /* Early detection of CMP support */
> - mips_cm_probe();
> - mips_cpc_probe();
> -
> - if (mips_cps_numiocu(0)) {
> - /*
> - * mips_cm_probe() wipes out bootloader
> - * config for CM regions and we have to configure them
> - * again. This SoC cannot talk to pamlbus devices
> - * witout proper iocu region set up.
> - *
> - * FIXME: it would be better to do this with values
> - * from DT, but we need this very early because
> - * without this we cannot talk to pretty much anything
> - * including serial.
> - */
> - write_gcr_reg0_base(MT7621_PALMBUS_BASE);
> - write_gcr_reg0_mask(~MT7621_PALMBUS_SIZE |
> - CM_GCR_REGn_MASK_CMTGT_IOCU0);
> - }
>
> if (!register_cps_smp_ops())
> return;
>
On Wed, Mar 21, 2018 at 02:02:10PM +1100, NeilBrown wrote:
>
> Since commit 3af5a67c86a3 ("MIPS: Fix early CM probing") the MT7621
> has not been able to boot.
>
> This patched caused mips_cm_probe() to be called before
> mt7621.c::proc_soc_init().
>
> prom_soc_init() has a comment explaining that mips_cm_probe()
> "wipes out the bootloader config" and means that configuration
> registers are no longer available. It has some code to re-enable
> this config.
>
> Before this re-enable code is run, the sysc register cannot be
> read, so when SYSC_REG_CHIP_NAME0 is read, a garbage value
> is returned and panic() is called.
>
> If we move the config-repair code to the top of prom_soc_init(),
> the registers can be read and boot can proceed.
>
> Very occasionally, the first register read after the reconfiguration
> returns garbage. So I added a call to __sync().
>
> Fixes: 3af5a67c86a3 ("MIPS: Fix early CM probing")
> Signed-off-by: NeilBrown <[email protected]>
Looks good. I've cosmetically tweaked commit message (mainly reflow),
added stable tag for 4.5+, and applied for 4.16.
Thanks
James