[PATCH v3 07/38] drm: handle HAS_IOPORT dependencies

[PATCH v3 07/38] drm: handle HAS_IOPORT dependencies

[Niklas Schnelle]

In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
not being declared. We thus need to add HAS_IOPORT as dependency for
those drivers using them. In the bochs driver there is optional MMIO
support detected at runtime, warn if this isn't taken when
HAS_IOPORT is not defined.

There is also a direct and hard coded use in cirrus.c which according to
the comment is only necessary during resume.  Let's just skip this as
for example s390 which doesn't have I/O port support also doesen't
support suspend/resume.

Co-developed-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 drivers/gpu/drm/qxl/Kconfig   |  1 +
 drivers/gpu/drm/tiny/bochs.c  | 19 +++++++++++++++++++
 drivers/gpu/drm/tiny/cirrus.c |  2 ++
 3 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/qxl/Kconfig b/drivers/gpu/drm/qxl/Kconfig
index ca3f51c2a8fe..d0e0d440c8d9 100644
--- a/drivers/gpu/drm/qxl/Kconfig
+++ b/drivers/gpu/drm/qxl/Kconfig
@@ -2,6 +2,7 @@
 config DRM_QXL
 	tristate "QXL virtual GPU"
 	depends on DRM && PCI && MMU
+	depends on HAS_IOPORT
 	select DRM_KMS_HELPER
 	select DRM_TTM
 	select DRM_TTM_HELPER
diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
index 024346054c70..da4a5d53b003 100644
--- a/drivers/gpu/drm/tiny/bochs.c
+++ b/drivers/gpu/drm/tiny/bochs.c
@@ -2,6 +2,7 @@
 
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <asm/bug.h>
 
 #include <drm/drm_aperture.h>
 #include <drm/drm_atomic_helper.h>
@@ -105,7 +106,11 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val)
 
 		writeb(val, bochs->mmio + offset);
 	} else {
+#ifdef HAS_IOPORT
 		outb(val, ioport);
+#else
+		WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT");
+#endif
 	}
 }
 
@@ -119,7 +124,12 @@ static u8 bochs_vga_readb(struct bochs_device *bochs, u16 ioport)
 
 		return readb(bochs->mmio + offset);
 	} else {
+#ifdef HAS_IOPORT
 		return inb(ioport);
+#else
+		WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT");
+		return 0xff;
+#endif
 	}
 }
 
@@ -132,8 +142,13 @@ static u16 bochs_dispi_read(struct bochs_device *bochs, u16 reg)
 
 		ret = readw(bochs->mmio + offset);
 	} else {
+#ifdef HAS_IOPORT
 		outw(reg, VBE_DISPI_IOPORT_INDEX);
 		ret = inw(VBE_DISPI_IOPORT_DATA);
+#else
+		WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT");
+		ret = 0xffff;
+#endif
 	}
 	return ret;
 }
@@ -145,8 +160,12 @@ static void bochs_dispi_write(struct bochs_device *bochs, u16 reg, u16 val)
 
 		writew(val, bochs->mmio + offset);
 	} else {
+#ifdef HAS_IOPORT
 		outw(reg, VBE_DISPI_IOPORT_INDEX);
 		outw(val, VBE_DISPI_IOPORT_DATA);
+#else
+		WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT");
+#endif
 	}
 }
 
diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
index accfa52e78c5..9da89732c5ac 100644
--- a/drivers/gpu/drm/tiny/cirrus.c
+++ b/drivers/gpu/drm/tiny/cirrus.c
@@ -308,8 +308,10 @@ static int cirrus_mode_set(struct cirrus_device *cirrus,
 
 	cirrus_set_start_address(cirrus, 0);
 
+#ifdef CONFIG_HAS_IOPORT
 	/* Unblank (needed on S3 resume, vgabios doesn't do it then) */
 	outb(0x20, 0x3c0);
+#endif
 
 	drm_dev_exit(idx);
 	return 0;
-- 
2.37.2

Re: [PATCH v3 07/38] drm: handle HAS_IOPORT dependencies

[Jani Nikula]

On Tue, 14 Mar 2023, Niklas Schnelle <schnelle@linux.ibm.com> wrote:
> In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> not being declared. We thus need to add HAS_IOPORT as dependency for
> those drivers using them. In the bochs driver there is optional MMIO
> support detected at runtime, warn if this isn't taken when
> HAS_IOPORT is not defined.

Not that I care a whole lot, but there should really only be one warning
or even failure to probe at bochs_hw_init() for !bochs->mmio &&
!IS_ENABLED(CONFIG_HAS_IOPORT), not warnings all over the place.

Moreover, the config macro is CONFIG_HAS_IOPORT instead of HAS_IOPORT
that you check for below.

BR,
Jani.

> There is also a direct and hard coded use in cirrus.c which according to
> the comment is only necessary during resume.  Let's just skip this as
> for example s390 which doesn't have I/O port support also doesen't
> support suspend/resume.
>
> Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
>  drivers/gpu/drm/qxl/Kconfig   |  1 +
>  drivers/gpu/drm/tiny/bochs.c  | 19 +++++++++++++++++++
>  drivers/gpu/drm/tiny/cirrus.c |  2 ++
>  3 files changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/qxl/Kconfig b/drivers/gpu/drm/qxl/Kconfig
> index ca3f51c2a8fe..d0e0d440c8d9 100644
> --- a/drivers/gpu/drm/qxl/Kconfig
> +++ b/drivers/gpu/drm/qxl/Kconfig
> @@ -2,6 +2,7 @@
>  config DRM_QXL
>  	tristate "QXL virtual GPU"
>  	depends on DRM && PCI && MMU
> +	depends on HAS_IOPORT
>  	select DRM_KMS_HELPER
>  	select DRM_TTM
>  	select DRM_TTM_HELPER
> diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
> index 024346054c70..da4a5d53b003 100644
> --- a/drivers/gpu/drm/tiny/bochs.c
> +++ b/drivers/gpu/drm/tiny/bochs.c
> @@ -2,6 +2,7 @@
>  
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <asm/bug.h>
>  
>  #include <drm/drm_aperture.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -105,7 +106,11 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val)
>  
>  		writeb(val, bochs->mmio + offset);
>  	} else {
> +#ifdef HAS_IOPORT
>  		outb(val, ioport);
> +#else
> +		WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT");
> +#endif
>  	}
>  }
>  
> @@ -119,7 +124,12 @@ static u8 bochs_vga_readb(struct bochs_device *bochs, u16 ioport)
>  
>  		return readb(bochs->mmio + offset);
>  	} else {
> +#ifdef HAS_IOPORT
>  		return inb(ioport);
> +#else
> +		WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT");
> +		return 0xff;
> +#endif
>  	}
>  }
>  
> @@ -132,8 +142,13 @@ static u16 bochs_dispi_read(struct bochs_device *bochs, u16 reg)
>  
>  		ret = readw(bochs->mmio + offset);
>  	} else {
> +#ifdef HAS_IOPORT
>  		outw(reg, VBE_DISPI_IOPORT_INDEX);
>  		ret = inw(VBE_DISPI_IOPORT_DATA);
> +#else
> +		WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT");
> +		ret = 0xffff;
> +#endif
>  	}
>  	return ret;
>  }
> @@ -145,8 +160,12 @@ static void bochs_dispi_write(struct bochs_device *bochs, u16 reg, u16 val)
>  
>  		writew(val, bochs->mmio + offset);
>  	} else {
> +#ifdef HAS_IOPORT
>  		outw(reg, VBE_DISPI_IOPORT_INDEX);
>  		outw(val, VBE_DISPI_IOPORT_DATA);
> +#else
> +		WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT");
> +#endif
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
> index accfa52e78c5..9da89732c5ac 100644
> --- a/drivers/gpu/drm/tiny/cirrus.c
> +++ b/drivers/gpu/drm/tiny/cirrus.c
> @@ -308,8 +308,10 @@ static int cirrus_mode_set(struct cirrus_device *cirrus,
>  
>  	cirrus_set_start_address(cirrus, 0);
>  
> +#ifdef CONFIG_HAS_IOPORT
>  	/* Unblank (needed on S3 resume, vgabios doesn't do it then) */
>  	outb(0x20, 0x3c0);
> +#endif
>  
>  	drm_dev_exit(idx);
>  	return 0;

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v3 07/38] drm: handle HAS_IOPORT dependencies

[Niklas Schnelle]

On Wed, 2023-03-15 at 13:54 +0200, Jani Nikula wrote:
> On Tue, 14 Mar 2023, Niklas Schnelle <schnelle@linux.ibm.com> wrote:
> > In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> > not being declared. We thus need to add HAS_IOPORT as dependency for
> > those drivers using them. In the bochs driver there is optional MMIO
> > support detected at runtime, warn if this isn't taken when
> > HAS_IOPORT is not defined.
> 
> Not that I care a whole lot, but there should really only be one warning
> or even failure to probe at bochs_hw_init() for !bochs->mmio &&
> !IS_ENABLED(CONFIG_HAS_IOPORT), not warnings all over the place.
> 
> Moreover, the config macro is CONFIG_HAS_IOPORT instead of HAS_IOPORT
> that you check for below.
> 
> BR,
> Jani.
> 

Good Find! Yes the #ifdefs need CONFIG_HAS_IOPORT I even had it
correctly for cirrus in the same patch. For v4 I remove the separate
warnings and instead went with the below:

@@ -229,6 +242,10 @@ static int bochs_hw_init(struct drm_device *dev)
                        return -ENOMEM;
                }
        } else {
+               if (!IS_ENABLED(CONFIG_HAS_IOPORT)) {
+                       DRM_ERROR("I/O ports are not supported\n");
+                       return -EIO;
+               }
                ioaddr = VBE_DISPI_IOPORT_INDEX;
                iosize = 2;
                if (!request_region(ioaddr, iosize, "bochs-drm")) {


Thanks,
Niklas

[PATCH v3 35/38] video: handle HAS_IOPORT dependencies

[Niklas Schnelle]

In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
not being declared. We thus need to add HAS_IOPORT as dependency for
those drivers using them and guard inline code in headers.

Co-developed-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 drivers/video/console/Kconfig |  1 +
 drivers/video/fbdev/Kconfig   | 25 +++++++++++++------------
 include/video/vga.h           |  8 ++++++++
 3 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
index 22cea5082ac4..64974eaa3ac5 100644
--- a/drivers/video/console/Kconfig
+++ b/drivers/video/console/Kconfig
@@ -10,6 +10,7 @@ config VGA_CONSOLE
 	depends on !4xx && !PPC_8xx && !SPARC && !M68K && !PARISC &&  !SUPERH && \
 		(!ARM || ARCH_FOOTBRIDGE || ARCH_INTEGRATOR || ARCH_NETWINDER) && \
 		!ARM64 && !ARC && !MICROBLAZE && !OPENRISC && !S390 && !UML
+	depends on HAS_IOPORT
 	select APERTURE_HELPERS if (DRM || FB || VFIO_PCI_CORE)
 	default y
 	help
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index ff3646c30d0d..b21a37497d22 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -338,7 +338,7 @@ config FB_IMX
 
 config FB_CYBER2000
 	tristate "CyberPro 2000/2010/5000 support"
-	depends on FB && PCI && (BROKEN || !SPARC64)
+	depends on FB && PCI && HAS_IOPORT && (BROKEN || !SPARC64)
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
@@ -432,6 +432,7 @@ config FB_FM2
 config FB_ARC
 	tristate "Arc Monochrome LCD board support"
 	depends on FB && (X86 || COMPILE_TEST)
+	depends on HAS_IOPORT
 	select FB_SYS_FILLRECT
 	select FB_SYS_COPYAREA
 	select FB_SYS_IMAGEBLIT
@@ -1260,7 +1261,7 @@ config FB_RADEON_DEBUG
 
 config FB_ATY128
 	tristate "ATI Rage128 display support"
-	depends on FB && PCI
+	depends on FB && PCI && HAS_IOPORT
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
@@ -1284,7 +1285,7 @@ config FB_ATY128_BACKLIGHT
 
 config FB_ATY
 	tristate "ATI Mach64 display support" if PCI || ATARI
-	depends on FB && !SPARC32
+	depends on FB && HAS_IOPORT && !SPARC32
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
@@ -1335,7 +1336,7 @@ config FB_ATY_BACKLIGHT
 
 config FB_S3
 	tristate "S3 Trio/Virge support"
-	depends on FB && PCI
+	depends on FB && PCI && HAS_IOPORT
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
@@ -1396,7 +1397,7 @@ config FB_SAVAGE_ACCEL
 
 config FB_SIS
 	tristate "SiS/XGI display support"
-	depends on FB && PCI
+	depends on FB && PCI && HAS_IOPORT
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
@@ -1427,7 +1428,7 @@ config FB_SIS_315
 
 config FB_VIA
 	tristate "VIA UniChrome (Pro) and Chrome9 display support"
-	depends on FB && PCI && GPIOLIB && I2C && (X86 || COMPILE_TEST)
+	depends on FB && PCI && GPIOLIB && I2C && HAS_IOPORT && (X86 || COMPILE_TEST)
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
@@ -1466,7 +1467,7 @@ endif
 
 config FB_NEOMAGIC
 	tristate "NeoMagic display support"
-	depends on FB && PCI
+	depends on FB && PCI && HAS_IOPORT
 	select FB_MODE_HELPERS
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
@@ -1496,7 +1497,7 @@ config FB_KYRO
 
 config FB_3DFX
 	tristate "3Dfx Banshee/Voodoo3/Voodoo5 display support"
-	depends on FB && PCI
+	depends on FB && PCI && HAS_IOPORT
 	select FB_CFB_IMAGEBLIT
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
@@ -1546,7 +1547,7 @@ config FB_VOODOO1
 
 config FB_VT8623
 	tristate "VIA VT8623 support"
-	depends on FB && PCI
+	depends on FB && PCI && HAS_IOPORT
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
@@ -1561,7 +1562,7 @@ config FB_VT8623
 
 config FB_TRIDENT
 	tristate "Trident/CyberXXX/CyberBlade support"
-	depends on FB && PCI
+	depends on FB && PCI && HAS_IOPORT
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
@@ -1584,7 +1585,7 @@ config FB_TRIDENT
 
 config FB_ARK
 	tristate "ARK 2000PV support"
-	depends on FB && PCI
+	depends on FB && PCI && HAS_IOPORT
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
@@ -2198,7 +2199,7 @@ config FB_SSD1307
 
 config FB_SM712
 	tristate "Silicon Motion SM712 framebuffer support"
-	depends on FB && PCI
+	depends on FB && PCI && HAS_IOPORT
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
diff --git a/include/video/vga.h b/include/video/vga.h
index 947c0abd04ef..f4b806b85c86 100644
--- a/include/video/vga.h
+++ b/include/video/vga.h
@@ -203,18 +203,26 @@ extern int restore_vga(struct vgastate *state);
 
 static inline unsigned char vga_io_r (unsigned short port)
 {
+#ifdef CONFIG_HAS_IOPORT
 	return inb_p(port);
+#else
+	return 0xff;
+#endif
 }
 
 static inline void vga_io_w (unsigned short port, unsigned char val)
 {
+#ifdef CONFIG_HAS_IOPORT
 	outb_p(val, port);
+#endif
 }
 
 static inline void vga_io_w_fast (unsigned short port, unsigned char reg,
 				  unsigned char val)
 {
+#ifdef CONFIG_HAS_IOPORT
 	outw(VGA_OUT16VAL (val, reg), port);
+#endif
 }
 
 static inline unsigned char vga_mm_r (void __iomem *regbase, unsigned short port)
-- 
2.37.2

Re: [PATCH v3 35/38] video: handle HAS_IOPORT dependencies

[Geert Uytterhoeven]

Hi Niklas,

On Tue, Mar 14, 2023 at 1:13 PM Niklas Schnelle <schnelle@linux.ibm.com> wrote:
> In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> not being declared. We thus need to add HAS_IOPORT as dependency for
> those drivers using them and guard inline code in headers.
>
> Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>

Thanks for your patch!

> --- a/drivers/video/fbdev/Kconfig
> +++ b/drivers/video/fbdev/Kconfig

> @@ -1284,7 +1285,7 @@ config FB_ATY128_BACKLIGHT
>
>  config FB_ATY
>         tristate "ATI Mach64 display support" if PCI || ATARI
> -       depends on FB && !SPARC32
> +       depends on FB && HAS_IOPORT && !SPARC32

On Atari, this works without ATARI_ROM_ISA, hence it must not depend
on HAS_IOPORT.
The only call to inb() is inside a section protected by #ifdef
CONFIG_PCI. So:

    depends on FB && !SPARC32
    depends on ATARI || HAS_IOPORT

>         select FB_CFB_FILLRECT
>         select FB_CFB_COPYAREA
>         select FB_CFB_IMAGEBLIT

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 35/38] video: handle HAS_IOPORT dependencies

[Ville Syrjälä]

On Wed, Mar 15, 2023 at 09:16:50AM +0100, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Tue, Mar 14, 2023 at 1:13 PM Niklas Schnelle <schnelle@linux.ibm.com> wrote:
> > In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> > not being declared. We thus need to add HAS_IOPORT as dependency for
> > those drivers using them and guard inline code in headers.
> >
> > Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/video/fbdev/Kconfig
> > +++ b/drivers/video/fbdev/Kconfig
> 
> > @@ -1284,7 +1285,7 @@ config FB_ATY128_BACKLIGHT
> >
> >  config FB_ATY
> >         tristate "ATI Mach64 display support" if PCI || ATARI
> > -       depends on FB && !SPARC32
> > +       depends on FB && HAS_IOPORT && !SPARC32
> 
> On Atari, this works without ATARI_ROM_ISA, hence it must not depend
> on HAS_IOPORT.
> The only call to inb() is inside a section protected by #ifdef
> CONFIG_PCI. So:

That piece of code is a nop anyway. We immediately overwrite
clk_wr_offset with a hardcoded selection after the register reads.
So if you nuke that nop code then no IOPORT dependency required
at all.

> 
>     depends on FB && !SPARC32
>     depends on ATARI || HAS_IOPORT
> 
> >         select FB_CFB_FILLRECT
> >         select FB_CFB_COPYAREA
> >         select FB_CFB_IMAGEBLIT
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

-- 
Ville Syrjälä
Intel

Re: [PATCH v3 35/38] video: handle HAS_IOPORT dependencies

[Niklas Schnelle]

On Wed, 2023-03-15 at 12:19 +0200, Ville Syrjälä wrote:
> On Wed, Mar 15, 2023 at 09:16:50AM +0100, Geert Uytterhoeven wrote:
> > Hi Niklas,
> > 
> > On Tue, Mar 14, 2023 at 1:13 PM Niklas Schnelle <schnelle@linux.ibm.com> wrote:
> > > In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> > > not being declared. We thus need to add HAS_IOPORT as dependency for
> > > those drivers using them and guard inline code in headers.
> > > 
> > > Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > 
> > Thanks for your patch!
> > 
> > > --- a/drivers/video/fbdev/Kconfig
> > > +++ b/drivers/video/fbdev/Kconfig
> > 
> > > @@ -1284,7 +1285,7 @@ config FB_ATY128_BACKLIGHT
> > > 
> > >  config FB_ATY
> > >         tristate "ATI Mach64 display support" if PCI || ATARI
> > > -       depends on FB && !SPARC32
> > > +       depends on FB && HAS_IOPORT && !SPARC32
> > 
> > On Atari, this works without ATARI_ROM_ISA, hence it must not depend
> > on HAS_IOPORT.
> > The only call to inb() is inside a section protected by #ifdef
> > CONFIG_PCI. So:
> 
> That piece of code is a nop anyway. We immediately overwrite
> clk_wr_offset with a hardcoded selection after the register reads.
> So if you nuke that nop code then no IOPORT dependency required
> at all.
> 

I agree this "looks" like a nop but are we sure the inb() doesn't have
side effects? 
(for reference drivers/video/fbdev/aty/aty/atyfb_base.c:
atyfb_setup_generc() towards the end)

It does feel a bit out of scope for this series but if it's really a
nop nuking it surely is the cleaner solution.

Thanks,
Niklas

Re: [PATCH v3 35/38] video: handle HAS_IOPORT dependencies

[Ville Syrjälä]

On Thu, Mar 23, 2023 at 03:17:38PM +0100, Niklas Schnelle wrote:
> On Wed, 2023-03-15 at 12:19 +0200, Ville Syrjälä wrote:
> > On Wed, Mar 15, 2023 at 09:16:50AM +0100, Geert Uytterhoeven wrote:
> > > Hi Niklas,
> > > 
> > > On Tue, Mar 14, 2023 at 1:13 PM Niklas Schnelle <schnelle@linux.ibm.com> wrote:
> > > > In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> > > > not being declared. We thus need to add HAS_IOPORT as dependency for
> > > > those drivers using them and guard inline code in headers.
> > > > 
> > > > Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> > > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > > 
> > > Thanks for your patch!
> > > 
> > > > --- a/drivers/video/fbdev/Kconfig
> > > > +++ b/drivers/video/fbdev/Kconfig
> > > 
> > > > @@ -1284,7 +1285,7 @@ config FB_ATY128_BACKLIGHT
> > > > 
> > > >  config FB_ATY
> > > >         tristate "ATI Mach64 display support" if PCI || ATARI
> > > > -       depends on FB && !SPARC32
> > > > +       depends on FB && HAS_IOPORT && !SPARC32
> > > 
> > > On Atari, this works without ATARI_ROM_ISA, hence it must not depend
> > > on HAS_IOPORT.
> > > The only call to inb() is inside a section protected by #ifdef
> > > CONFIG_PCI. So:
> > 
> > That piece of code is a nop anyway. We immediately overwrite
> > clk_wr_offset with a hardcoded selection after the register reads.
> > So if you nuke that nop code then no IOPORT dependency required
> > at all.
> > 
> 
> I agree this "looks" like a nop but are we sure the inb() doesn't have
> side effects? 

Yes. It's just trying to check which PLL dividers/etc. are currently
used. In VGA mode it gets it from a the GENMO and in non-VGA mode from
CLOCK_CNTL. And then it says "screw that" and just uses index 3 instead.

Though I must say that mach64 GX seems to use that clk_wr_offset
very differently so I'm not sure the PCI+GX combo is even working
currently, assuming those even exist. I don't think I have anything
older than a PCI mach64 VT myself.

> (for reference drivers/video/fbdev/aty/aty/atyfb_base.c:
> atyfb_setup_generc() towards the end)
> 
> It does feel a bit out of scope for this series but if it's really a
> nop nuking it surely is the cleaner solution.
> 
> Thanks,
> Niklas

-- 
Ville Syrjälä
Intel

Re: [PATCH v3 35/38] video: handle HAS_IOPORT dependencies

[Niklas Schnelle]

On Thu, 2023-03-23 at 18:08 +0200, Ville Syrjälä wrote:
> On Thu, Mar 23, 2023 at 03:17:38PM +0100, Niklas Schnelle wrote:
> > On Wed, 2023-03-15 at 12:19 +0200, Ville Syrjälä wrote:
> > > On Wed, Mar 15, 2023 at 09:16:50AM +0100, Geert Uytterhoeven wrote:
> > > > Hi Niklas,
> > > > 
> > > > On Tue, Mar 14, 2023 at 1:13 PM Niklas Schnelle <schnelle@linux.ibm.com> wrote:
> > > > > In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> > > > > not being declared. We thus need to add HAS_IOPORT as dependency for
> > > > > those drivers using them and guard inline code in headers.
> > > > > 
> > > > > Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> > > > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > > > 
> > > > Thanks for your patch!
> > > > 
> > > > > --- a/drivers/video/fbdev/Kconfig
> > > > > +++ b/drivers/video/fbdev/Kconfig
> > > > 
> > > > > @@ -1284,7 +1285,7 @@ config FB_ATY128_BACKLIGHT
> > > > > 
> > > > >  config FB_ATY
> > > > >         tristate "ATI Mach64 display support" if PCI || ATARI
> > > > > -       depends on FB && !SPARC32
> > > > > +       depends on FB && HAS_IOPORT && !SPARC32
> > > > 
> > > > On Atari, this works without ATARI_ROM_ISA, hence it must not depend
> > > > on HAS_IOPORT.
> > > > The only call to inb() is inside a section protected by #ifdef
> > > > CONFIG_PCI. So:
> > > 
> > > That piece of code is a nop anyway. We immediately overwrite
> > > clk_wr_offset with a hardcoded selection after the register reads.
> > > So if you nuke that nop code then no IOPORT dependency required
> > > at all.
> > > 
> > 
> > I agree this "looks" like a nop but are we sure the inb() doesn't have
> > side effects? 
> 
> Yes. It's just trying to check which PLL dividers/etc. are currently
> used. In VGA mode it gets it from a the GENMO and in non-VGA mode from
> CLOCK_CNTL. And then it says "screw that" and just uses index 3 instead.
> 

Ok, I've added a patch to remove this part of the code and with that
the driver actually builds on s390 (no HAS_IOPORT) so I also removed
the HAS_IOPORT dependency. Both will be in my v4.

Thanks,
Niklas