| Message ID | 20240612-cm_probe-v2-6-a5b55440563c@flygoat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | MIPS: cm: Probe GCR address from devicetree | expand |
Hi Jiaxun, Thomas On Wed, Jun 12, 2024 at 11:08:58AM +0100, Jiaxun Yang wrote: > Traditionally, CM GCR address can be probed from CP0_CMGCRBase. > > However there are chips in wild that do have CM GCR but CP0_CMGCRBase > is not available from CPU point of view. Thus we need to be able to > probe GCR address from DeviceTree. > > It is implemented as: > - If only CP0_CMGCRBase present, trust CP0_CMGCRBase > - If only mti,mips-cm node present, trust mti,mips-cm reg prop > - If both present, remap address space to address specified in dt > > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> > --- > v2: Fix build warning (test bot) > --- > arch/mips/kernel/mips-cm.c | 61 +++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 55 insertions(+), 6 deletions(-) > > diff --git a/arch/mips/kernel/mips-cm.c b/arch/mips/kernel/mips-cm.c > index dddc9428fe58..02afc795ba8a 100644 > --- a/arch/mips/kernel/mips-cm.c > +++ b/arch/mips/kernel/mips-cm.c > @@ -5,6 +5,8 @@ > */ > > #include <linux/errno.h> > +#include <linux/of_address.h> > +#include <linux/of_fdt.h> > #include <linux/percpu.h> > #include <linux/spinlock.h> > > @@ -179,23 +181,70 @@ static char *cm3_causes[32] = { > static DEFINE_PER_CPU_ALIGNED(spinlock_t, cm_core_lock); > static DEFINE_PER_CPU_ALIGNED(unsigned long, cm_core_lock_flags); > > +static int __init mips_cm_fdt_scan(unsigned long node, const char *uname, > + int depth, void *data) > +{ > + u64 addr; > + unsigned long *cmgcr = data; > + > + if (!of_flat_dt_is_compatible(node, "mti,mips-cm")) > + return 0; > + > + addr = of_flat_dt_translate_address(node); > + if (addr == OF_BAD_ADDR || addr >= ULONG_MAX) > + *cmgcr = 0; > + else > + *cmgcr = addr; > + > + return 0; > +} > + > phys_addr_t __init __weak mips_cm_phys_base(void) > { > - unsigned long cmgcr; > + unsigned long gcr_reg = 0, gcr_dt = 0; > + > + if (of_have_populated_dt()) { > + int err; > + struct resource res; > + struct device_node *cm_node; > + > + cm_node = of_find_compatible_node(of_root, NULL, "mti,mips-cm"); > + if (cm_node) { > + err = of_address_to_resource(cm_node, 0, &res); > + of_node_put(cm_node); > + if (!err) > + gcr_dt = res.start; > + } > + } else { > + of_scan_flat_dt(mips_cm_fdt_scan, &gcr_dt); > + } > > /* Check the CMGCRBase register is implemented */ > if (!(read_c0_config() & MIPS_CONF_M)) > - return 0; > + return gcr_dt; > > if (!(read_c0_config2() & MIPS_CONF_M)) > - return 0; > + return gcr_dt; > > if (!(read_c0_config3() & MIPS_CONF3_CMGCR)) > - return 0; > + return gcr_dt; > > /* Read the address from CMGCRBase */ > - cmgcr = read_c0_cmgcrbase(); > - return (cmgcr & MIPS_CMGCRF_BASE) << (36 - 32); > + gcr_reg = read_c0_cmgcrbase(); > + gcr_reg = (gcr_reg & MIPS_CMGCRF_BASE) << (36 - 32); > + > + /* If no of node, return straight away */ > + if (!gcr_dt) > + return gcr_reg; > + > + /* If the CMGCRBase mismatches with dt, remap it */ > + if (gcr_reg != gcr_dt) { > + pr_info("Remapping CMGCRBase from 0x%08lx to 0x%08lx\n", > + gcr_reg, gcr_dt); > + change_gcr_base(CM_GCR_BASE_GCRBASE, gcr_dt); This causes the kernel boot-up procedure to crash/hang-up because the CM GCR base address is supposed to be redefined by means of the already mapped CM GCR address space by accessing the CM_GCR_BASE_GCRBASE register: change_gcr_base() +-> read_gcr_base() +-> addr_gcr_base() +-> return mips_gcr_base + CM_GCR_BASE_GCRBASE By the time of the change_gcr_base() call in mips_cm_phys_base(), the mips_gcr_base variable hasn't been defined. So the IO operations performed in the change_gcr_base() method would be accessing the NULL-based memory space. That's why the kernel crash/hanging-up. In order to fix this we have to first map the CM GCR block at the default base-address, then update the CM GCR-base CSR and after that remap the CM GCR-space. Please also note, the GCR_BASE field might be RO. It depends on the IP-core configuration. So it's possible that the CM_GCR_BASE_GCRBASE field update won't work. Although that will be detected a bit later in the mips_cm_probe() method by comparing the address returned from mips_cm_phys_base() and retrieved from the CM GCR-base CSR. -Serge(y) > + } > + > + return gcr_dt; > } > > phys_addr_t __init __weak mips_cm_l2sync_phys_base(void) > > -- > 2.43.0 >
在2024年9月10日九月 下午1:36，Serge Semin写道： [...] > > This causes the kernel boot-up procedure to crash/hang-up because the > CM GCR base address is supposed to be redefined by means of the > already mapped CM GCR address space by accessing the > CM_GCR_BASE_GCRBASE register: > change_gcr_base() > +-> read_gcr_base() > +-> addr_gcr_base() > +-> return mips_gcr_base + CM_GCR_BASE_GCRBASE > > By the time of the change_gcr_base() call in mips_cm_phys_base(), the > mips_gcr_base variable hasn't been defined. So the IO operations > performed in the change_gcr_base() method would be accessing the > NULL-based memory space. That's why the kernel crash/hanging-up. Thanks for the analysis! This path was not taken on my audience hardware, so I didn't catch this, will fix in next version. > > In order to fix this we have to first map the CM GCR block at the > default base-address, then update the CM GCR-base CSR and after that > remap the CM GCR-space. > > Please also note, the GCR_BASE field might be RO. It depends on the > IP-core configuration. So it's possible that the CM_GCR_BASE_GCRBASE > field update won't work. Although that will be detected a bit later in > the mips_cm_probe() method by comparing the address returned from > mips_cm_phys_base() and retrieved from the CM GCR-base CSR. Hmm, I just checked RTL and RDL for CM2 and CM3 and I didn't see it as a configurable option. It's possible to change hardware reset value but not make it RO. Maybe it was possible on earlier IP release, in this case it's always user's responsibility to write correct address in DeviceTree :-) Thanks > > -Serge(y)
On Tue, Sep 10, 2024 at 08:23:25PM +0100, Jiaxun Yang wrote: > > > 在2024年9月10日九月 下午1:36，Serge Semin写道： > [...] > > > > This causes the kernel boot-up procedure to crash/hang-up because the > > CM GCR base address is supposed to be redefined by means of the > > already mapped CM GCR address space by accessing the > > CM_GCR_BASE_GCRBASE register: > > change_gcr_base() > > +-> read_gcr_base() > > +-> addr_gcr_base() > > +-> return mips_gcr_base + CM_GCR_BASE_GCRBASE > > > > By the time of the change_gcr_base() call in mips_cm_phys_base(), the > > mips_gcr_base variable hasn't been defined. So the IO operations > > performed in the change_gcr_base() method would be accessing the > > NULL-based memory space. That's why the kernel crash/hanging-up. > > Thanks for the analysis! > This path was not taken on my audience hardware, so I didn't catch this, > will fix in next version. > > > > > In order to fix this we have to first map the CM GCR block at the > > default base-address, then update the CM GCR-base CSR and after that > > remap the CM GCR-space. > > > > Please also note, the GCR_BASE field might be RO. It depends on the > > IP-core configuration. So it's possible that the CM_GCR_BASE_GCRBASE > > field update won't work. Although that will be detected a bit later in > > the mips_cm_probe() method by comparing the address returned from > > mips_cm_phys_base() and retrieved from the CM GCR-base CSR. > > Hmm, I just checked RTL and RDL for CM2 and CM3 and I didn't see it as a > configurable option. It's possible to change hardware reset value but not make it RO. Both MIPS P5600 and P6600 databooks define the GCR_BASE field as optionally R/W: GCR_BASE 31:15 This field sets the base address of the 32KB R or R/W GCR block of the P5600 MPS. (IP Config- This register has a fixed value after reset if uration) configured as Read-Only (an IP Configuration Option). > > Maybe it was possible on earlier IP release, I found the text above in the latest MIPS Warrior P-class software manuals downloaded from the ImagTech site. Not sure why your RTL code doesn't have such configs. > in this case it's always > user's responsibility to write correct address in DeviceTree :-) Right. The system just won't work if the CM GCR base address couldn't be updated. -Serge(y) > > Thanks > > > > > -Serge(y) > > -- > - Jiaxun
在2024年9月10日九月 下午9:07，Serge Semin写道： [...] > Both MIPS P5600 and P6600 databooks define the GCR_BASE field as > optionally R/W: > > GCR_BASE 31:15 This field sets the base address of the 32KB R or R/W > GCR block of the P5600 MPS. (IP Config- > This register has a fixed value after reset if uration) > configured as Read-Only (an IP Configuration Option). > Thanks for the pointer, I traced code history and it seems like MIPS decided to not expose this functionality at some point, but documents were not updated. Maybe I should add a read back check here. Thanks
On Tue, Sep 10, 2024 at 09:48:10PM +0100, Jiaxun Yang wrote: > > > 在2024年9月10日九月 下午9:07，Serge Semin写道： > [...] > > Both MIPS P5600 and P6600 databooks define the GCR_BASE field as > > optionally R/W: > > > > GCR_BASE 31:15 This field sets the base address of the 32KB R or R/W > > GCR block of the P5600 MPS. (IP Config- > > This register has a fixed value after reset if uration) > > configured as Read-Only (an IP Configuration Option). > > > > Thanks for the pointer, I traced code history and it seems like MIPS decided > to not expose this functionality at some point, but documents were not updated. Got it. Thanks for clarification. > > Maybe I should add a read back check here. The check is already implemented in the mips_cm_probe() method. Just 10 lines below the mips_cm_phys_base() method call. -Serge(y) > > Thanks > -- > - Jiaxun
diff --git a/arch/mips/kernel/mips-cm.c b/arch/mips/kernel/mips-cm.c index dddc9428fe58..02afc795ba8a 100644 --- a/arch/mips/kernel/mips-cm.c +++ b/arch/mips/kernel/mips-cm.c @@ -5,6 +5,8 @@ */ #include <linux/errno.h> +#include <linux/of_address.h> +#include <linux/of_fdt.h> #include <linux/percpu.h> #include <linux/spinlock.h> @@ -179,23 +181,70 @@ static char *cm3_causes[32] = { static DEFINE_PER_CPU_ALIGNED(spinlock_t, cm_core_lock); static DEFINE_PER_CPU_ALIGNED(unsigned long, cm_core_lock_flags); +static int __init mips_cm_fdt_scan(unsigned long node, const char *uname, + int depth, void *data) +{ + u64 addr; + unsigned long *cmgcr = data; + + if (!of_flat_dt_is_compatible(node, "mti,mips-cm")) + return 0; + + addr = of_flat_dt_translate_address(node); + if (addr == OF_BAD_ADDR || addr >= ULONG_MAX) + *cmgcr = 0; + else + *cmgcr = addr; + + return 0; +} + phys_addr_t __init __weak mips_cm_phys_base(void) { - unsigned long cmgcr; + unsigned long gcr_reg = 0, gcr_dt = 0; + + if (of_have_populated_dt()) { + int err; + struct resource res; + struct device_node *cm_node; + + cm_node = of_find_compatible_node(of_root, NULL, "mti,mips-cm"); + if (cm_node) { + err = of_address_to_resource(cm_node, 0, &res); + of_node_put(cm_node); + if (!err) + gcr_dt = res.start; + } + } else { + of_scan_flat_dt(mips_cm_fdt_scan, &gcr_dt); + } /* Check the CMGCRBase register is implemented */ if (!(read_c0_config() & MIPS_CONF_M)) - return 0; + return gcr_dt; if (!(read_c0_config2() & MIPS_CONF_M)) - return 0; + return gcr_dt; if (!(read_c0_config3() & MIPS_CONF3_CMGCR)) - return 0; + return gcr_dt; /* Read the address from CMGCRBase */ - cmgcr = read_c0_cmgcrbase(); - return (cmgcr & MIPS_CMGCRF_BASE) << (36 - 32); + gcr_reg = read_c0_cmgcrbase(); + gcr_reg = (gcr_reg & MIPS_CMGCRF_BASE) << (36 - 32); + + /* If no of node, return straight away */ + if (!gcr_dt) + return gcr_reg; + + /* If the CMGCRBase mismatches with dt, remap it */ + if (gcr_reg != gcr_dt) { + pr_info("Remapping CMGCRBase from 0x%08lx to 0x%08lx\n", + gcr_reg, gcr_dt); + change_gcr_base(CM_GCR_BASE_GCRBASE, gcr_dt); + } + + return gcr_dt; } phys_addr_t __init __weak mips_cm_l2sync_phys_base(void)
Traditionally, CM GCR address can be probed from CP0_CMGCRBase. However there are chips in wild that do have CM GCR but CP0_CMGCRBase is not available from CPU point of view. Thus we need to be able to probe GCR address from DeviceTree. It is implemented as: - If only CP0_CMGCRBase present, trust CP0_CMGCRBase - If only mti,mips-cm node present, trust mti,mips-cm reg prop - If both present, remap address space to address specified in dt Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> --- v2: Fix build warning (test bot) --- arch/mips/kernel/mips-cm.c | 61 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 55 insertions(+), 6 deletions(-)