Re: [patch] video: support for VGA hoses on alpha TITAN, TSUNAMI systems (ES45, DS25)

[Andrew Morton <akpm@osdl.org>]

On Thu, 2 Nov 2006 00:37:20 -0800
Steve Langasek <vorlon@debian.org> wrote:

> From: Jay Estabrook <jay.estabrook@hp.com>
> 
> Add working support for VGA hoses on Alpha, which is required in order to
> use a VGA console on TITAN- and TSUNAMI-class Alpha systems.  This also
> generalizes support for non-standard VGA offsets on Alpha to cover the
> Marvel class of alphas without special-casing.  Includes recognizing an
> additional model of TITAN-class Alpha.
> 
> Tested successfully on an ES45 and a DS25 with a vanilla 2.6.18 kernel,
> also tested on an LX164 with the Debian 2.6.18 kernel with no regressions. 
> Included in Alpha Core distribution for some time.
> 

It would be appropriate for both yourself and Jay to provide signoffs for
this work, as per section 11 of Documentation/SubmittingPatches, please.

What is a "hose"?


Some trivia:


> +	if (!hose || (conswitchp == &vga_con && pci_vga_hose == hose)) return;

This is missing a newline.

> +static int titan_pchip1_present = 0;

Unneeded (and undesirable) initialisation.

> +++ source-es45/arch/alpha/kernel/proto.h	2006-09-30 03:14:44.000000000 -0700
> @@ -106,6 +106,11 @@
>  extern unsigned long wildfire_node_mem_start(int);
>  extern unsigned long wildfire_node_mem_size(int);
>  
> +/* console.c */
> +#ifdef CONFIG_VGA_HOSE
> +  extern void locate_and_init_vga(void *(*)(void *, void *));
> +#endif

The ifdef isn't really needed - we usually leave it out.

> diff -uNr source/arch/alpha/kernel/sys_dp264.c source-es45/arch/alpha/kernel/sys_dp264.c
> --- source/arch/alpha/kernel/sys_dp264.c	2006-09-19 20:42:06.000000000 -0700
> +++ source-es45/arch/alpha/kernel/sys_dp264.c	2006-09-30 03:14:44.000000000 -0700
> @@ -543,6 +543,9 @@
>  {
>  	common_init_pci();
>  	SMC669_Init(0);
> +#ifdef CONFIG_VGA_HOSE
> +	locate_and_init_vga(NULL);
> +#endif
>  }
>  
>  static void __init
> @@ -551,6 +554,18 @@
>  	common_init_pci();
>  	SMC669_Init(1);
>  	es1888_init();
> +#ifdef CONFIG_VGA_HOSE
> +	locate_and_init_vga(NULL);
> +#endif
> +}
> +
> +static void __init
> +clipper_init_pci(void)
> +{
> +	common_init_pci();
> +#ifdef CONFIG_VGA_HOSE
> +	locate_and_init_vga(NULL);
> +#endif
>  }

We normally do:

#ifdef CONFIG_VGA_HOSE
extern void locate_and_init_vga(void *(*handler)(void *, void *));
#else
static inline void locate_and_init_vga(void *(*handler)(void *, void *))
{
}
#endif

so that all those ifdefs go away and so that the new code gets
compile-checked regardless of config settings.

>  
> +/* wait until after includes to test for this, to allow arch-specific mod. */
> +#ifndef vga_request_resource
> +# define vga_request_resource request_resource
> +#endif
> +

erk.

> +#define vga_request_resource alpha_vga_request_resource
> +
> +static int __inline__
> +alpha_vga_request_resource(struct resource *root, struct resource *new)
> +{
> +	/* First, fixup the VGA resource bounds WRT the hose it is on. */
> +	if (pci_vga_hose) {
> +		new->start += pci_vga_hose->io_space->start;
> +		new->end += pci_vga_hose->io_space->start;
> +	}
> +
> +	/* Finally, do a normal request_resource(). */
> +	return request_resource(root, new);
> +}
> 

So if the resource is being requested by vga we adjust the caller's
resource before actually requesting it.

Isn't this a bit hacky?  How come vgacon ended up requesting the wrong
addresses in the first place?

(And please don't use __inline__ or __inline - plain old `inline' works fine)

>  static DEFINE_SPINLOCK(vga_lock);
>  static int cursor_size_lastfrom;
>  static int cursor_size_lastto;
> @@ -393,7 +398,7 @@
>  			vga_video_type = VIDEO_TYPE_EGAM;
>  			vga_vram_size = 0x8000;
>  			display_desc = "EGA+";
> -			request_resource(&ioport_resource,
> +			vga_request_resource(&ioport_resource,
>  					 &ega_console_resource);
>
> ..
>
> diff -uNr source/include/asm-alpha/core_titan.h source-es45/include/asm-alpha/core_titan.h
> --- source/include/asm-alpha/core_titan.h	2006-09-19 20:42:06.000000000 -0700
> +++ source-es45/include/asm-alpha/core_titan.h	2006-09-30 03:14:44.000000000 -0700
> @@ -3,6 +3,7 @@
>  
>  #include <linux/types.h>
>  #include <linux/pci.h>
> +#include <asm/pci.h>
>  #include <asm/compiler.h>
>  
> ...
>
> diff -uNr source/include/asm-alpha/core_tsunami.h source-es45/include/asm-alpha/core_tsunami.h
> --- source/include/asm-alpha/core_tsunami.h	2006-09-19 20:42:06.000000000 -0700
> +++ source-es45/include/asm-alpha/core_tsunami.h	2006-09-30 03:14:44.000000000 -0700
> @@ -2,6 +2,8 @@
>  #define __ALPHA_TSUNAMI__H__
>  
>  #include <linux/types.h>
> +#include <linux/pci.h>
> +#include <asm/pci.h>
>  #include <asm/compiler.h>

So we actually need to include asm/pci.h in these files?  linux/pci.h
already did that?