| Message ID | 1592745008-17196-4-git-send-email-sagar.kadam@sifive.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | update clock handler and proper cpu features | expand |
Hi Simon, On Sun, Jun 21, 2020 at 9:10 PM Sagar Shrikant Kadam <sagar.kadam at sifive.com> wrote: > > The cmd "cpu detail" fetches uninitialized cpu feature information > and thus displays wrong / inconsitent details as below. FU540-C000 doesn't > have any microcode, yet the cmd display's it. > => cpu detail > 0: cpu at 0 rv64imac > ID = 0, freq = 999.100 MHz: L1 cache, MMU, Microcode, Device ID > Microcode version 0x0 > Device ID 0x0 > 1: cpu at 1 rv64imafdc > ID = 1, freq = 999.100 MHz: L1 cache, MMU, Microcode, Device ID > Microcode version 0x0 > Device ID 0x0 > 2: cpu at 2 rv64imafdc > ID = 2, freq = 999.100 MHz: L1 cache, MMU, Microcode, Device ID > Microcode version 0x0 > Device ID 0x0 > 3: cpu at 3 rv64imafdc > ID = 3, freq = 999.100 MHz: L1 cache, MMU, Microcode, Device ID > Microcode version 0x0 > Device ID 0x0 > 4: cpu at 4 rv64imafdc > ID = 4, freq = 999.100 MHz: L1 cache, MMU, Microcode, Device ID > Microcode version 0x0 > Device ID 0x0 > > The L1 cache or MMU entry seen above is also displayed inconsistently. > So initialize features to zero before fetching from device tree. > Additionally the conditional check to read "mmu-type" from device tree > is not rightly handled due to which the cpu feature doesn't include > CPU_FEAT_MMU even if it's corresponding entry is present in device tree. > > We now see correct features as: > > => cpu detail > 0: cpu at 0 rv64imac > ID = 0, freq = 999.100 MHz > 1: cpu at 1 rv64imafdc > ID = 1, freq = 999.100 MHz: MMU > 2: cpu at 2 rv64imafdc > ID = 2, freq = 999.100 MHz: MMU > 3: cpu at 3 rv64imafdc > ID = 3, freq = 999.100 MHz: MMU > 4: cpu at 4 rv64imafdc > ID = 4, freq = 999.100 MHz: MMU > > Signed-off-by: Sagar Shrikant Kadam <sagar.kadam at sifive.com> > Reviewed-by: Pragnesh Patel <pragnesh.patel at sifive.com> > --- > drivers/cpu/riscv_cpu.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpu/riscv_cpu.c b/drivers/cpu/riscv_cpu.c > index 76b0489..8c4b5e7 100644 > --- a/drivers/cpu/riscv_cpu.c > +++ b/drivers/cpu/riscv_cpu.c > @@ -38,6 +38,8 @@ static int riscv_cpu_get_info(struct udevice *dev, struct cpu_info *info) > > /* Zero out the frequency, in case sizeof(ulong) != sizeof(u32) */ > info->cpu_freq = 0; > + /* Initialise cpu features before updating from device tree */ > + info->features = 0; For this one, do you think we should fix the cpu_get_info() in cpu-uclass driver instead? With fix in the cpu-uclass driver we can avoid similar issue in any single CPU driver. > > /* First try getting the frequency from the assigned clock */ > ret = clk_get_by_index(dev, 0, &clk); > @@ -52,7 +54,7 @@ static int riscv_cpu_get_info(struct udevice *dev, struct cpu_info *info) > dev_read_u32(dev, "clock-frequency", (u32 *)&info->cpu_freq); > > mmu = dev_read_string(dev, "mmu-type"); > - if (!mmu) > + if (mmu) > info->features |= BIT(CPU_FEAT_MMU); > > return 0; Regards, Bin
Hi Bin, On Tue, 23 Jun 2020 at 19:26, Bin Meng <bmeng.cn at gmail.com> wrote: > > Hi Simon, > > On Sun, Jun 21, 2020 at 9:10 PM Sagar Shrikant Kadam > <sagar.kadam at sifive.com> wrote: > > > > The cmd "cpu detail" fetches uninitialized cpu feature information > > and thus displays wrong / inconsitent details as below. FU540-C000 doesn't > > have any microcode, yet the cmd display's it. > > => cpu detail > > 0: cpu at 0 rv64imac > > ID = 0, freq = 999.100 MHz: L1 cache, MMU, Microcode, Device ID > > Microcode version 0x0 > > Device ID 0x0 > > 1: cpu at 1 rv64imafdc > > ID = 1, freq = 999.100 MHz: L1 cache, MMU, Microcode, Device ID > > Microcode version 0x0 > > Device ID 0x0 > > 2: cpu at 2 rv64imafdc > > ID = 2, freq = 999.100 MHz: L1 cache, MMU, Microcode, Device ID > > Microcode version 0x0 > > Device ID 0x0 > > 3: cpu at 3 rv64imafdc > > ID = 3, freq = 999.100 MHz: L1 cache, MMU, Microcode, Device ID > > Microcode version 0x0 > > Device ID 0x0 > > 4: cpu at 4 rv64imafdc > > ID = 4, freq = 999.100 MHz: L1 cache, MMU, Microcode, Device ID > > Microcode version 0x0 > > Device ID 0x0 > > > > The L1 cache or MMU entry seen above is also displayed inconsistently. > > So initialize features to zero before fetching from device tree. > > Additionally the conditional check to read "mmu-type" from device tree > > is not rightly handled due to which the cpu feature doesn't include > > CPU_FEAT_MMU even if it's corresponding entry is present in device tree. > > > > We now see correct features as: > > > > => cpu detail > > 0: cpu at 0 rv64imac > > ID = 0, freq = 999.100 MHz > > 1: cpu at 1 rv64imafdc > > ID = 1, freq = 999.100 MHz: MMU > > 2: cpu at 2 rv64imafdc > > ID = 2, freq = 999.100 MHz: MMU > > 3: cpu at 3 rv64imafdc > > ID = 3, freq = 999.100 MHz: MMU > > 4: cpu at 4 rv64imafdc > > ID = 4, freq = 999.100 MHz: MMU > > > > Signed-off-by: Sagar Shrikant Kadam <sagar.kadam at sifive.com> > > Reviewed-by: Pragnesh Patel <pragnesh.patel at sifive.com> > > --- > > drivers/cpu/riscv_cpu.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/cpu/riscv_cpu.c b/drivers/cpu/riscv_cpu.c > > index 76b0489..8c4b5e7 100644 > > --- a/drivers/cpu/riscv_cpu.c > > +++ b/drivers/cpu/riscv_cpu.c > > @@ -38,6 +38,8 @@ static int riscv_cpu_get_info(struct udevice *dev, struct cpu_info *info) > > > > /* Zero out the frequency, in case sizeof(ulong) != sizeof(u32) */ > > info->cpu_freq = 0; > > + /* Initialise cpu features before updating from device tree */ > > + info->features = 0; > > For this one, do you think we should fix the cpu_get_info() in > cpu-uclass driver instead? With fix in the cpu-uclass driver we can > avoid similar issue in any single CPU driver. Yes it would be best to fix that function to zero the data. Regards, Simon
diff --git a/drivers/cpu/riscv_cpu.c b/drivers/cpu/riscv_cpu.c index 76b0489..8c4b5e7 100644 --- a/drivers/cpu/riscv_cpu.c +++ b/drivers/cpu/riscv_cpu.c @@ -38,6 +38,8 @@ static int riscv_cpu_get_info(struct udevice *dev, struct cpu_info *info) /* Zero out the frequency, in case sizeof(ulong) != sizeof(u32) */ info->cpu_freq = 0; + /* Initialise cpu features before updating from device tree */ + info->features = 0; /* First try getting the frequency from the assigned clock */ ret = clk_get_by_index(dev, 0, &clk); @@ -52,7 +54,7 @@ static int riscv_cpu_get_info(struct udevice *dev, struct cpu_info *info) dev_read_u32(dev, "clock-frequency", (u32 *)&info->cpu_freq); mmu = dev_read_string(dev, "mmu-type"); - if (!mmu) + if (mmu) info->features |= BIT(CPU_FEAT_MMU); return 0;
The cmd "cpu detail" fetches uninitialized cpu feature information and thus displays wrong / inconsitent details as below. FU540-C000 doesn't have any microcode, yet the cmd display's it. => cpu detail 0: cpu at 0 rv64imac ID = 0, freq = 999.100 MHz: L1 cache, MMU, Microcode, Device ID Microcode version 0x0 Device ID 0x0 1: cpu at 1 rv64imafdc ID = 1, freq = 999.100 MHz: L1 cache, MMU, Microcode, Device ID Microcode version 0x0 Device ID 0x0 2: cpu at 2 rv64imafdc ID = 2, freq = 999.100 MHz: L1 cache, MMU, Microcode, Device ID Microcode version 0x0 Device ID 0x0 3: cpu at 3 rv64imafdc ID = 3, freq = 999.100 MHz: L1 cache, MMU, Microcode, Device ID Microcode version 0x0 Device ID 0x0 4: cpu at 4 rv64imafdc ID = 4, freq = 999.100 MHz: L1 cache, MMU, Microcode, Device ID Microcode version 0x0 Device ID 0x0 The L1 cache or MMU entry seen above is also displayed inconsistently. So initialize features to zero before fetching from device tree. Additionally the conditional check to read "mmu-type" from device tree is not rightly handled due to which the cpu feature doesn't include CPU_FEAT_MMU even if it's corresponding entry is present in device tree. We now see correct features as: => cpu detail 0: cpu at 0 rv64imac ID = 0, freq = 999.100 MHz 1: cpu at 1 rv64imafdc ID = 1, freq = 999.100 MHz: MMU 2: cpu at 2 rv64imafdc ID = 2, freq = 999.100 MHz: MMU 3: cpu at 3 rv64imafdc ID = 3, freq = 999.100 MHz: MMU 4: cpu at 4 rv64imafdc ID = 4, freq = 999.100 MHz: MMU Signed-off-by: Sagar Shrikant Kadam <sagar.kadam at sifive.com> Reviewed-by: Pragnesh Patel <pragnesh.patel at sifive.com> --- drivers/cpu/riscv_cpu.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)