On Mon, 19 Aug 2019, Ralf Ramsauer wrote:
> @@ -11,6 +11,7 @@
> #include <linux/acpi_pmtmr.h>
> #include <linux/kernel.h>
> #include <linux/reboot.h>
> +#include <linux/serial_8250.h>
> #include <asm/apic.h>
> #include <asm/cpu.h>
> #include <asm/hypervisor.h>
> @@ -20,8 +21,13 @@
> #include <asm/reboot.h>
> #include <asm/setup.h>
>
> +#define SETUP_DATA_FLAGS_PERMIT_PCUART(n) (1 << (n))
> +#define SETUP_DATA_FLAGS_HAS_PCUART(flags, n) \
> + !!(flags & SETUP_DATA_FLAGS_PERMIT_PCUART(n))
TBH, this is unreadable obfuscation and there is absolutely no reason for
this to be implemented as macros. What's wrong with proper helper
functions?
static bool uart_enabled(unsigned int uartnr)
{
return setup_data.v2.flags & BIT(uartnr);
}
That would be too simple and not enough UPPERCASELETTERS, right?
> +#ifdef CONFIG_SERIAL_8250
> struct mpc_intsrc mp_irq = {
> .type = MP_INTSRC,
> .irqtype = mp_INT,
> .irqflag = MP_IRQPOL_ACTIVE_HIGH | MP_IRQTRIG_EDGE,
> };
> +#endif
Errm. Why are you not moving this into the other conditional section?
> unsigned int cpu;
>
> jailhouse_x2apic_init();
> @@ -99,12 +107,16 @@ static void __init jailhouse_get_smp_config(unsigned int
> early)
> if (setup_data.v1.standard_ioapic) {
> mp_register_ioapic(0, 0xfec00000, gsi_top, &ioapic_cfg);
>
> - /* Register 1:1 mapping for legacy UART IRQs 3 and 4 */
> - mp_irq.srcbusirq = mp_irq.dstirq = 3;
> - mp_save_irq(&mp_irq);
> +#ifdef CONFIG_SERIAL_8250
> + if (setup_data.hdr.version < 2) {
> + /* Register 1:1 mapping for legacy UART IRQs 3 and 4 */
> + mp_irq.srcbusirq = mp_irq.dstirq = 3;
> + mp_save_irq(&mp_irq);
>
> - mp_irq.srcbusirq = mp_irq.dstirq = 4;
> - mp_save_irq(&mp_irq);
> + mp_irq.srcbusirq = mp_irq.dstirq = 4;
> + mp_save_irq(&mp_irq);
> + }
> +#endif
This #ifdeffery can be avoided and that setup of the interrupts can be
split out into a helper.
static void uart_setup_irq(unsigned int irq)
{
struct mpc_intsrc mp_irq = {
.type = MP_INTSRC,
.irqtype = mp_INT,
.irqflag = MP_IRQPOL_ACTIVE_HIGH | MP_IRQTRIG_EDGE,
.srcbusirq = irq,
.dstirq = irq,
};
mp_save_irq(&mp_irq);
}
And then that whole thing becomes
if (IS_ENABLED(CONFIG_SERIAL_8250) && hdr.version < 2) {
/* Register 1:1 mapping for legacy UART IRQs 3 and 4 */
uart_setup_irq(3);
uart_setup_irq(4);
}
And of course the serial fixup can use exactly the same helper.
> @@ -137,6 +149,42 @@ static int __init jailhouse_pci_arch_init(void)
> return 0;
> }
>
> +#ifdef CONFIG_SERIAL_8250
> +static const u16 pcuart_base[] = {
> + 0x3f8,
> + 0x2f8,
> + 0x3e8,
> + 0x2e8,
> +};
Fits nicely in a single line and can just be inside of the function.
> +static void jailhouse_serial_fixup(int port, struct uart_port *up,
> + u32 *capabilites)
> +{
> + struct mpc_intsrc mp_irq = {
> + .type = MP_INTSRC,
> + .irqtype = mp_INT,
> + .irqflag = MP_IRQPOL_ACTIVE_HIGH | MP_IRQTRIG_EDGE,
> + };
Can go away
> + unsigned int n;
> +
> + for (n = 0; n < ARRAY_SIZE(pcuart_base); n++) {
> + if (pcuart_base[n] != up->iobase)
> + continue;
> +
> + if (SETUP_DATA_FLAGS_HAS_PCUART(setup_data.v2.flags, n)) {
> + pr_info("Enabling UART%u (port 0x%lx)\n", n,
> + up->iobase);
> + mp_irq.srcbusirq = mp_irq.dstirq = up->irq;
> + mp_save_irq(&mp_irq);
> + } else {
> + /* Deactivate UART if access isn't allowed */
> + up->iobase = 0;
> + }
> + break;
> + }
> +}
> +#endif
> +
> static void __init jailhouse_init_platform(void)
> {
> u64 pa_data = boot_params.hdr.setup_data;
> @@ -187,7 +235,8 @@ static void __init jailhouse_init_platform(void)
> if (setup_data.hdr.version == 0 ||
> setup_data.hdr.compatible_version !=
> JAILHOUSE_SETUP_REQUIRED_VERSION ||
> - (setup_data.hdr.version >= 1 && header.len < SETUP_DATA_V1_LEN))
> + (setup_data.hdr.version == 1 && header.len < SETUP_DATA_V1_LEN) ||
> + (setup_data.hdr.version >= 2 && header.len < SETUP_DATA_V2_LEN))
> goto unsupported;
>
> pmtmr_ioport = setup_data.v1.pm_timer_address;
> @@ -203,6 +252,20 @@ static void __init jailhouse_init_platform(void)
> * are none in a non-root cell.
> */
> disable_acpi();
> +
> +#ifdef CONFIG_SERIAL_8250
> + /*
> + * There are flags inside setup_data that indicate availability of
> + * platform UARTs since setup data version 2.
> + *
> + * In case of version 1, we don't know which UARTs belong Linux. In
> + * this case, unconditionally register 1:1 mapping for legacy UART IRQs
> + * 3 and 4.
> + */
> + if (setup_data.hdr.version > 1)
> + serial8250_set_isa_configurator(jailhouse_serial_fixup);
If you put that into a helper and stick that into the above #ifdef with a
stub for the CONFIG...0=n case then you keep that init function readable
and not plastered with that UART stuff.
Thanks,
tglx
--
You received this message because you are subscribed to the Google Groups
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to jailhouse-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/jailhouse-dev/alpine.DEB.2.21.1908212158350.1983%40nanos.tec.linutronix.de.
