x86: Add missing DECLARE_GLOBAL_DATA_PTR for mtrr.c

PATCH Superseded Bin Meng 2 participants 9 messages Jan 20, 2015 – Jan 21, 2015 x86

lore

Share & tools

lore

Bin Meng

author

2015-01-20 05:01 UTC #0 lore raw

PATCH 1/3 x86: Add missing DECLARE_GLOBAL_DATA_PTR for mtrr.c

arch/x86/cpu/mtrr.c has access to the U-Boot global data thus DECLARE_GLOBAL_DATA_PTR is needed.

Signed-off-by Bin Meng <[email protected]>

---

 arch/x86/cpu/mtrr.c | 2 ++
 1 file changed, 2 insertions(+)

arch/x86/cpu/mtrr.c +2 -0

index d5a825d..ac8765f 100644
@@ -17,6 +17,8 @@
 #include <asm/msr.h>
 #include <asm/mtrr.h>
 
+DECLARE_GLOBAL_DATA_PTR;
+
 /* Prepare to adjust MTRRs */
 void mtrr_open(struct mtrr_state *state)
 {

Signature · 1 line

1.8.2.1

Bin Meng

author

2015-01-20 05:01 UTC #1 lore raw

PATCH 2/3 x86: Save mtrr support flag in global data

CPUID (EAX 01H) returns MTRR support flag in EDX bit 12. Probe this flag in x86_cpu_init_f() and save it in global data.

Signed-off-by Bin Meng <[email protected]>

---

 arch/x86/cpu/cpu.c                 | 7 +++++++
 arch/x86/include/asm/global_data.h | 3 ++-
 2 files changed, 9 insertions(+), 1 deletion(-)

arch/x86/cpu/cpu.c +7 -0

index 30e5069..ed7905c 100644
@@ -223,6 +223,11 @@ static bool has_cpuid(void)
 	return flag_is_changeable_p(X86_EFLAGS_ID);
 }
 
+static bool has_mtrr(void)
+{
+	return cpuid_edx(0x00000001) & (1 << 12) ? true : false;
+}
+
 static int build_vendor_name(char *vendor_name)
 {
 	struct cpuid_result result;

@@ -318,6 +323,8 @@ int x86_cpu_init_f(void)
 		gd->arch.x86_model = c.x86_model;
 		gd->arch.x86_mask = c.x86_mask;
 		gd->arch.x86_device = cpu.device;
+
+		gd->arch.has_mtrr = has_mtrr();
 	}
 
 	return 0;

arch/x86/include/asm/global_data.h +2 -1

index 24e3052..6499b9e 100644
@@ -63,7 +63,8 @@ struct arch_global_data {
 	void	*hob_list;		/* FSP HOB list */
 #endif
 	struct mtrr_request mtrr_req[MAX_MTRR_REQUESTS];
-	int mtrr_req_count;
+	int	mtrr_req_count;
+	int	has_mtrr;
 };
 
 #endif

Signature · 1 line

1.8.2.1

Simon Glass

2015-01-21 16:05 UTC #2 lore raw

PATCH 2/3 x86: Save mtrr support flag in global data

Quoted 9 lines · Bin Meng

CPUID (EAX 01H) returns MTRR support flag in EDX bit 12. Probe this flag in x86_cpu_init_f() and save it in global data.
Signed-off-by Bin Meng <[email protected]>
---
 arch/x86/cpu/cpu.c                 | 7 +++++++
 arch/x86/include/asm/global_data.h | 3 ++-
 2 files changed, 9 insertions(+), 1 deletion(-)

Acked-by Simon Glass <[email protected]>

arch/x86/include/asm/global_data.h struct arch_global_data { · line 67

Quoted 3 lines

arch/x86/include/asm/global_data.h +2 -1

-       int mtrr_req_count;
+       int     mtrr_req_count;
+       int     has_mtrr;

But I don't think we need the tabs after 'int'.

Regards,
Simon

Bin Meng

author

2015-01-21 16:17 UTC #3 lore raw

PATCH 2/3 x86: Save mtrr support flag in global data

Hi Simon,

Quoted 11 lines · Simon Glass

Quoted 9 lines · Bin Meng
CPUID (EAX 01H) returns MTRR support flag in EDX bit 12. Probe this flag in x86_cpu_init_f() and save it in global data.
Signed-off-by Bin Meng <[email protected]>
---
 arch/x86/cpu/cpu.c                 | 7 +++++++
 arch/x86/include/asm/global_data.h | 3 ++-
 2 files changed, 9 insertions(+), 1 deletion(-)

Quoted 8 lines

Acked-by Simon Glass <[email protected]>
arch/x86/include/asm/global_data.h struct arch_global_data { · line 67
Quoted 3 lines
arch/x86/include/asm/global_data.h +2 -1
-       int mtrr_req_count;
+       int     mtrr_req_count;
+       int     has_mtrr;
But I don't think we need the tabs after 'int'.

Looks that there is a tab before hob_list, so I chose tab to indent these two. I will send a v2 patch to fix this.

Regards,
Bin

Bin Meng

author

2015-01-20 05:01 UTC #4 lore raw

PATCH 3/3 x86: Test mtrr support flag before accessing mtrr msr

In reply to Bin Meng

On some x86 processors (like Intel Quark) the MTRR registers are not supported. This is reflected by the CPUID (EAX 01H) result EDX[12]. Accessing the MTRR registers on such processors will cause #GP so we must test the support flag before accessing MTRR MSRs.

Signed-off-by Bin Meng <[email protected]>

---

 arch/x86/cpu/mtrr.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

arch/x86/cpu/mtrr.c +12 -0

index ac8765f..68f1b04 100644
@@ -22,6 +22,9 @@ DECLARE_GLOBAL_DATA_PTR;
 /* Prepare to adjust MTRRs */
 void mtrr_open(struct mtrr_state *state)
 {
+	if (!gd->arch.has_mtrr)
+		return;
+
 	state->enable_cache = dcache_status();
 
 	if (state->enable_cache)

@@ -33,6 +36,9 @@ void mtrr_open(struct mtrr_state *state)
 /* Clean up after adjusting MTRRs, and enable them */
 void mtrr_close(struct mtrr_state *state)
 {
+	if (!gd->arch.has_mtrr)
+		return;
+
 	wrmsrl(MTRR_DEF_TYPE_MSR, state->deftype | MTRR_DEF_TYPE_EN);
 	if (state->enable_cache)
 		enable_caches();

@@ -45,6 +51,9 @@ int mtrr_commit(bool do_caches)
 	uint64_t mask;
 	int i;
 
+	if (!gd->arch.has_mtrr)
+		return 0;
+
 	mtrr_open(&state);
 	for (i = 0; i < gd->arch.mtrr_req_count; i++, req++) {
 		mask = ~(req->size - 1);

@@ -66,6 +75,9 @@ int mtrr_add_request(int type, uint64_t start, uint64_t size)
 	struct mtrr_request *req;
 	uint64_t mask;
 
+	if (!gd->arch.has_mtrr)
+		return 0;
+
 	if (gd->arch.mtrr_req_count == MAX_MTRR_REQUESTS)
 		return -ENOSPC;
 	req = &gd->arch.mtrr_req[gd->arch.mtrr_req_count++];

Signature · 1 line

1.8.2.1

Simon Glass

2015-01-21 16:06 UTC #5 lore raw

PATCH 3/3 x86: Test mtrr support flag before accessing mtrr msr

Hi Bin,

Quoted 46 lines · Bin Meng

Signed-off-by Bin Meng <[email protected]>

---

 arch/x86/cpu/mtrr.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

arch/x86/cpu/mtrr.c +9 -0

index ac8765f..68f1b04 100644
@@ -22,6 +22,9 @@ DECLARE_GLOBAL_DATA_PTR;
 /* Prepare to adjust MTRRs */
 void mtrr_open(struct mtrr_state *state)
 {
+       if (!gd->arch.has_mtrr)
+               return;
+
        state->enable_cache = dcache_status();
 
        if (state->enable_cache)

@@ -33,6 +36,9 @@ void mtrr_open(struct mtrr_state *state)
 /* Clean up after adjusting MTRRs, and enable them */
 void mtrr_close(struct mtrr_state *state)
 {
+       if (!gd->arch.has_mtrr)
+               return;
+
        wrmsrl(MTRR_DEF_TYPE_MSR, state->deftype | MTRR_DEF_TYPE_EN);
        if (state->enable_cache)
                enable_caches();

@@ -45,6 +51,9 @@ int mtrr_commit(bool do_caches)
        uint64_t mask;
        int i;
 
+       if (!gd->arch.has_mtrr)
+               return 0;
+
        mtrr_open(&state);
        for (i = 0; i < gd->arch.mtrr_req_count; i++, req++) {
                mask = ~(req->size - 1);

arch/x86/cpu/mtrr.c int mtrr_add_request(int type, uint64_t start, uint64_t size) · line 79

Quoted 5 lines

arch/x86/cpu/mtrr.c +2 -0

@@ -66,6 +75,9 @@ int mtrr_add_request(int type, uint64_t start, uint64_t size)
        struct mtrr_request *req;
        uint64_t mask;
 
+       if (!gd->arch.has_mtrr)
+               return 0;

Shouldn't this (and the above) return -ENOSYS?

Quoted 7 lines

arch/x86/cpu/mtrr.c +1 -1

+
        if (gd->arch.mtrr_req_count == MAX_MTRR_REQUESTS)
                return -ENOSPC;
        req = &gd->arch.mtrr_req[gd->arch.mtrr_req_count++];
--

1.8.2.1

Regards,
Simon

Bin Meng

author

2015-01-21 16:19 UTC #6 lore raw

PATCH 3/3 x86: Test mtrr support flag before accessing mtrr msr

Hi Simon,

Quoted 52 lines · Simon Glass

Hi Bin,

Quoted 49 lines · Bin Meng

Signed-off-by Bin Meng <[email protected]>

---

 arch/x86/cpu/mtrr.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

arch/x86/cpu/mtrr.c +9 -0

index ac8765f..68f1b04 100644
@@ -22,6 +22,9 @@ DECLARE_GLOBAL_DATA_PTR;
 /* Prepare to adjust MTRRs */
 void mtrr_open(struct mtrr_state *state)
 {
+       if (!gd->arch.has_mtrr)
+               return;
+
        state->enable_cache = dcache_status();
 
        if (state->enable_cache)

@@ -33,6 +36,9 @@ void mtrr_open(struct mtrr_state *state)
 /* Clean up after adjusting MTRRs, and enable them */
 void mtrr_close(struct mtrr_state *state)
 {
+       if (!gd->arch.has_mtrr)
+               return;
+
        wrmsrl(MTRR_DEF_TYPE_MSR, state->deftype | MTRR_DEF_TYPE_EN);
        if (state->enable_cache)
                enable_caches();

@@ -45,6 +51,9 @@ int mtrr_commit(bool do_caches)
        uint64_t mask;
        int i;
 
+       if (!gd->arch.has_mtrr)
+               return 0;
+
        mtrr_open(&state);
        for (i = 0; i < gd->arch.mtrr_req_count; i++, req++) {
                mask = ~(req->size - 1);

@@ -66,6 +75,9 @@ int mtrr_add_request(int type, uint64_t start, uint64_t size)
        struct mtrr_request *req;
        uint64_t mask;

Quoted 4 lines

arch/x86/cpu/mtrr.c int mtrr_add_request(int type, uint64_t start, uint64_t size) · line 79
Quoted 2 lines
arch/x86/cpu/mtrr.c +2 -0
+       if (!gd->arch.has_mtrr)
+               return 0;
Shouldn't this (and the above) return -ENOSYS?

If returning non-zero, the init_cache_f_r() will fail as it checks the return value. Do you think we should ignore the return value there? But if ignored, there is no meaning of returning error codes here.

Quoted 10 lines

Quoted 7 lines

arch/x86/cpu/mtrr.c +1 -1

+
        if (gd->arch.mtrr_req_count == MAX_MTRR_REQUESTS)
                return -ENOSPC;
        req = &gd->arch.mtrr_req[gd->arch.mtrr_req_count++];
--

1.8.2.1

Regards,
Simon

Regards,
Bin

Simon Glass

2015-01-21 16:23 UTC #7 lore raw

PATCH 3/3 x86: Test mtrr support flag before accessing mtrr msr

Hi Bin,

Quoted 55 lines · Bin Meng

Hi Simon,

Quoted 52 lines · Simon Glass

Hi Bin,

Quoted 49 lines · Bin Meng

Signed-off-by Bin Meng <[email protected]>

---

 arch/x86/cpu/mtrr.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

arch/x86/cpu/mtrr.c +9 -0

index ac8765f..68f1b04 100644
@@ -22,6 +22,9 @@ DECLARE_GLOBAL_DATA_PTR;
 /* Prepare to adjust MTRRs */
 void mtrr_open(struct mtrr_state *state)
 {
+       if (!gd->arch.has_mtrr)
+               return;
+
        state->enable_cache = dcache_status();
 
        if (state->enable_cache)

@@ -33,6 +36,9 @@ void mtrr_open(struct mtrr_state *state)
 /* Clean up after adjusting MTRRs, and enable them */
 void mtrr_close(struct mtrr_state *state)
 {
+       if (!gd->arch.has_mtrr)
+               return;
+
        wrmsrl(MTRR_DEF_TYPE_MSR, state->deftype | MTRR_DEF_TYPE_EN);
        if (state->enable_cache)
                enable_caches();

@@ -45,6 +51,9 @@ int mtrr_commit(bool do_caches)
        uint64_t mask;
        int i;
 
+       if (!gd->arch.has_mtrr)
+               return 0;
+
        mtrr_open(&state);
        for (i = 0; i < gd->arch.mtrr_req_count; i++, req++) {
                mask = ~(req->size - 1);

@@ -66,6 +75,9 @@ int mtrr_add_request(int type, uint64_t start, uint64_t size)
        struct mtrr_request *req;
        uint64_t mask;

Quoted 8 lines

Quoted 4 lines
arch/x86/cpu/mtrr.c int mtrr_add_request(int type, uint64_t start, uint64_t size) · line 79
Quoted 2 lines
arch/x86/cpu/mtrr.c +2 -0
+       if (!gd->arch.has_mtrr)
+               return 0;
Shouldn't this (and the above) return -ENOSYS?
If returning non-zero, the init_cache_f_r() will fail as it checks the return value. Do you think we should ignore the return value there? But if ignored, there is no meaning of returning error codes here.

Yes, I understand, but I think it is better to ignore (just the -ENOSYS) return value in the caller (add a comment in the code if you like) than return an incorrect value indicating that the action was taken.

Quoted 7 lines

arch/x86/cpu/mtrr.c +1 -1

+
        if (gd->arch.mtrr_req_count == MAX_MTRR_REQUESTS)
                return -ENOSPC;
        req = &gd->arch.mtrr_req[gd->arch.mtrr_req_count++];
--

1.8.2.1

Regards,
Simon

Simon Glass

2015-01-21 16:04 UTC #8 lore raw

PATCH 1/3 x86: Add missing DECLARE_GLOBAL_DATA_PTR for mtrr.c

In reply to Bin Meng

Quoted 8 lines · Bin Meng

arch/x86/cpu/mtrr.c has access to the U-Boot global data thus DECLARE_GLOBAL_DATA_PTR is needed.
Signed-off-by Bin Meng <[email protected]>
---
 arch/x86/cpu/mtrr.c | 2 ++
 1 file changed, 2 insertions(+)

Acked-by Simon Glass <[email protected]>