diff mbox series

[RESEND,1/1] x86/of: Add support for boot time interrupt mode config

Message ID 37cc31242d0edda1bb0900cc62bba87954a7e892.1668403214.git.rtanwar@maxlinear.com
State New
Headers
Series x86/of: Fix a bug in x86 arch OF support |

Commit Message

Rahul Tanwar Nov. 14, 2022, 9:20 a.m. UTC 
  Presently, init/boot time interrupt delivery mode is enumerated only
for ACPI enabled systems by parsing MADT table or for older systems
by parsing MP table. But for OF based x86 systems, it is assumed &
fixed to legacy PIC mode.

Add support for configuration of init time interrupt delivery mode for
x86 OF based systems by introducing a new optional boolean property
'intel,no-imcr' for interrupt-controller node of local APIC. This
property emulates IMCRP Bit 7 of MP feature info byte 2 of MP
floating pointer structure.

Defaults to legacy PIC mode if absent. Configures it to virtual wire
compatibility mode if present.

Fixes: 3879a6f329483 ("x86: dtb: Add early parsing of IO_APIC")
Signed-off-by: Rahul Tanwar <rtanwar@maxlinear.com>
---
 arch/x86/kernel/devicetree.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Andy Shevchenko Nov. 14, 2022, 9:44 a.m. UTC | #1 
On Mon, Nov 14, 2022 at 05:20:06PM +0800, Rahul Tanwar wrote:
> Presently, init/boot time interrupt delivery mode is enumerated only
> for ACPI enabled systems by parsing MADT table or for older systems
> by parsing MP table. But for OF based x86 systems, it is assumed &
> fixed to legacy PIC mode.
> 
> Add support for configuration of init time interrupt delivery mode for
> x86 OF based systems by introducing a new optional boolean property
> 'intel,no-imcr' for interrupt-controller node of local APIC. This
> property emulates IMCRP Bit 7 of MP feature info byte 2 of MP
> floating pointer structure.
> 
> Defaults to legacy PIC mode if absent. Configures it to virtual wire
> compatibility mode if present.

...

> +	if (of_property_read_bool(dn, "intel,no-imcr")) {

I can't find this property in the Documentation/devicetree/bindings.

Moreover, I prefer to see positive one, something like:

	intel,virtual-wire-bla-bla-bla

Please consult with DT people on how properly name it.

> +		pr_info("    Virtual Wire compatibility mode.\n");
> +		pic_mode = 0;
> +	} else {
> +		pr_info("    IMCR and PIC compatibility mode.\n");
> +		pic_mode = 1;
> +	}
Rahul Tanwar Nov. 14, 2022, 10 a.m. UTC | #2 
Hi Andy,

Thanks for response.

On 14/11/2022 5:45 pm, Andy Shevchenko wrote:
> This email was sent from outside of MaxLinear.
> 
> 
> On Mon, Nov 14, 2022 at 05:20:06PM +0800, Rahul Tanwar wrote:
>> Presently, init/boot time interrupt delivery mode is enumerated only
>> for ACPI enabled systems by parsing MADT table or for older systems
>> by parsing MP table. But for OF based x86 systems, it is assumed &
>> fixed to legacy PIC mode.
>>
>> Add support for configuration of init time interrupt delivery mode for
>> x86 OF based systems by introducing a new optional boolean property
>> 'intel,no-imcr' for interrupt-controller node of local APIC. This
>> property emulates IMCRP Bit 7 of MP feature info byte 2 of MP
>> floating pointer structure.
>>
>> Defaults to legacy PIC mode if absent. Configures it to virtual wire
>> compatibility mode if present.
> 
> ...
> 
>> +     if (of_property_read_bool(dn, "intel,no-imcr")) {
> 
> I can't find this property in the Documentation/devicetree/bindings.
> 
> Moreover, I prefer to see positive one, something like:
> 
>          intel,virtual-wire-bla-bla-bla
> 
> Please consult with DT people on how properly name it.


Yes, agree. Need to add it in bindings doc after finalizing the property 
name. I chose "intel,no-imcr" to have a direct correlation with the MPS
spec defined data field for the same purpose. It reads below bit in 
mpparse code to detect PIC mode or virtual wire mode.

Bit 7: IMCRP. When the IMCR presence bit is set, the IMCR is present and 
PIC Mode is implemented; otherwise, Virtual Wire Mode is implemented.

Please refer [1]

[1] https://www.manualslib.com/manual/77733/Intel 
Multiprocessor.html?page=40#manual

Regards,
Rahul


> 
>> +             pr_info("    Virtual Wire compatibility mode.\n");
>> +             pic_mode = 0;
>> +     } else {
>> +             pr_info("    IMCR and PIC compatibility mode.\n");
>> +             pic_mode = 1;
>> +     }
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 
>
Andy Shevchenko Nov. 14, 2022, 10:24 a.m. UTC | #3 
On Mon, Nov 14, 2022 at 10:00:02AM +0000, Rahul Tanwar wrote:
> On 14/11/2022 5:45 pm, Andy Shevchenko wrote:
> > On Mon, Nov 14, 2022 at 05:20:06PM +0800, Rahul Tanwar wrote:

...

> >> +     if (of_property_read_bool(dn, "intel,no-imcr")) {
> > 
> > I can't find this property in the Documentation/devicetree/bindings.
> > 
> > Moreover, I prefer to see positive one, something like:
> > 
> >          intel,virtual-wire-bla-bla-bla
> > 
> > Please consult with DT people on how properly name it.
> 
> 
> Yes, agree. Need to add it in bindings doc after finalizing the property 
> name. I chose "intel,no-imcr" to have a direct correlation with the MPS
> spec defined data field for the same purpose.

The problems with it are:
- it's negative
- it's too cryptic to one who doesn't know area well enough

> It reads below bit in 
> mpparse code to detect PIC mode or virtual wire mode.
> 
> Bit 7: IMCRP. When the IMCR presence bit is set, the IMCR is present and 
> PIC Mode is implemented; otherwise, Virtual Wire Mode is implemented.
> 
> Please refer [1]
> 
> [1] https://www.manualslib.com/manual/77733/Intel 
> Multiprocessor.html?page=40#manual

This is good reference for DT people to suggest you a better name.
diff mbox series

Patch

diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
index 5cd51f25f446..1e4ed420478b 100644
--- a/arch/x86/kernel/devicetree.c
+++ b/arch/x86/kernel/devicetree.c
@@ -167,7 +167,14 @@  static void __init dtb_lapic_setup(void)
 			return;
 	}
 	smp_found_config = 1;
-	pic_mode = 1;
+	if (of_property_read_bool(dn, "intel,no-imcr")) {
+		pr_info("    Virtual Wire compatibility mode.\n");
+		pic_mode = 0;
+	} else {
+		pr_info("    IMCR and PIC compatibility mode.\n");
+		pic_mode = 1;
+	}
+
 	register_lapic_address(lapic_addr);
 }