| Message ID | 20230314121216.413434-36-schnelle@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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
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
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
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
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
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)
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(-)