| Message ID | alpine.LNX.2.00.1507141725480.851@nippy.intranet (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2015-07-14 at 17:58 +1000, Finn Thain wrote: > Make use of arch_nvram_ops in device drivers so that the nvram_* function > exports can be removed. > > Since they are no longer global symbols, rename the PPC32 nvram_* > functions appropriately. > > Add the missing CONFIG_NVRAM test to imsttfb to avoid a build failure. > > Add a CONFIG_PPC32 test to matroxfb because PPC64 doesn't implement the > read_byte() method. This is a bit fishy in a way because some of that nvram stuff is really about powermac/apple nvram offsets, ie, "XPRAM". Maybe we should have a dedicated accessor for "mac_xpram" and NULL-check it rather than using ifdef's ? > Signed-off-by: Finn Thain <fthain@telegraphics.com.au> > > --- > > Changed since v4: > - Added CONFIG_PPC32 test to matroxfb. > > --- > arch/powerpc/kernel/setup_32.c | 8 ++++---- > drivers/char/generic_nvram.c | 4 ++-- > drivers/video/fbdev/controlfb.c | 4 ++-- > drivers/video/fbdev/imsttfb.c | 7 +++---- > drivers/video/fbdev/matrox/matroxfb_base.c | 4 ++-- > drivers/video/fbdev/platinumfb.c | 4 ++-- > drivers/video/fbdev/valkyriefb.c | 4 ++-- > 7 files changed, 17 insertions(+), 18 deletions(-) > > Index: linux/arch/powerpc/kernel/setup_32.c > =================================================================== > --- linux.orig/arch/powerpc/kernel/setup_32.c 2015-07-13 21:33:01.000000000 +1000 > +++ linux/arch/powerpc/kernel/setup_32.c 2015-07-13 21:33:02.000000000 +1000 > @@ -170,20 +170,18 @@ __setup("l3cr=", ppc_setup_l3cr); > > #ifdef CONFIG_GENERIC_NVRAM > > -unsigned char nvram_read_byte(int addr) > +static unsigned char ppc_nvram_read_byte(int addr) > { > if (ppc_md.nvram_read_val) > return ppc_md.nvram_read_val(addr); > return 0xff; > } > -EXPORT_SYMBOL(nvram_read_byte); > > -void nvram_write_byte(unsigned char val, int addr) > +static void ppc_nvram_write_byte(unsigned char val, int addr) > { > if (ppc_md.nvram_write_val) > ppc_md.nvram_write_val(addr, val); > } > -EXPORT_SYMBOL(nvram_write_byte); > > static ssize_t ppc_nvram_get_size(void) > { > @@ -200,6 +198,8 @@ static long ppc_nvram_sync(void) > } > > const struct nvram_ops arch_nvram_ops = { > + .read_byte = ppc_nvram_read_byte, > + .write_byte = ppc_nvram_write_byte, > .get_size = ppc_nvram_get_size, > .sync = ppc_nvram_sync, > }; > Index: linux/drivers/char/generic_nvram.c > =================================================================== > --- linux.orig/drivers/char/generic_nvram.c 2015-07-13 21:33:01.000000000 +1000 > +++ linux/drivers/char/generic_nvram.c 2015-07-13 21:33:02.000000000 +1000 > @@ -64,7 +64,7 @@ static ssize_t read_nvram(struct file *f > if (*ppos >= nvram_len) > return 0; > for (i = *ppos; count > 0 && i < nvram_len; ++i, ++p, --count) > - if (__put_user(nvram_read_byte(i), p)) > + if (__put_user(arch_nvram_ops.read_byte(i), p)) > return -EFAULT; > *ppos = i; > return p - buf; > @@ -84,7 +84,7 @@ static ssize_t write_nvram(struct file * > for (i = *ppos; count > 0 && i < nvram_len; ++i, ++p, --count) { > if (__get_user(c, p)) > return -EFAULT; > - nvram_write_byte(c, i); > + arch_nvram_ops.write_byte(c, i); > } > *ppos = i; > return p - buf; > Index: linux/drivers/video/fbdev/controlfb.c > =================================================================== > --- linux.orig/drivers/video/fbdev/controlfb.c 2015-07-13 21:32:43.000000000 +1000 > +++ linux/drivers/video/fbdev/controlfb.c 2015-07-13 21:33:02.000000000 +1000 > @@ -415,7 +415,7 @@ static int __init init_control(struct fb > /* Try to pick a video mode out of NVRAM if we have one. */ > #ifdef CONFIG_NVRAM > if (default_cmode == CMODE_NVRAM) { > - cmode = nvram_read_byte(NV_CMODE); > + cmode = arch_nvram_ops.read_byte(NV_CMODE); > if(cmode < CMODE_8 || cmode > CMODE_32) > cmode = CMODE_8; > } else > @@ -423,7 +423,7 @@ static int __init init_control(struct fb > cmode=default_cmode; > #ifdef CONFIG_NVRAM > if (default_vmode == VMODE_NVRAM) { > - vmode = nvram_read_byte(NV_VMODE); > + vmode = arch_nvram_ops.read_byte(NV_VMODE); > if (vmode < 1 || vmode > VMODE_MAX || > control_mac_modes[vmode - 1].m[full] < cmode) { > sense = read_control_sense(p); > Index: linux/drivers/video/fbdev/matrox/matroxfb_base.c > =================================================================== > --- linux.orig/drivers/video/fbdev/matrox/matroxfb_base.c 2015-07-13 21:32:57.000000000 +1000 > +++ linux/drivers/video/fbdev/matrox/matroxfb_base.c 2015-07-13 21:33:02.000000000 +1000 > @@ -1886,9 +1886,9 @@ static int initMatrox2(struct matrox_fb_ > struct fb_var_screeninfo var; > if (default_vmode <= 0 || default_vmode > VMODE_MAX) > default_vmode = VMODE_640_480_60; > -#ifdef CONFIG_NVRAM > +#if defined(CONFIG_NVRAM) && defined(CONFIG_PPC32) > if (default_cmode == CMODE_NVRAM) > - default_cmode = nvram_read_byte(NV_CMODE); > + default_cmode = arch_nvram_ops.read_byte(NV_CMODE); > #endif > if (default_cmode < CMODE_8 || default_cmode > CMODE_32) > default_cmode = CMODE_8; > Index: linux/drivers/video/fbdev/platinumfb.c > =================================================================== > --- linux.orig/drivers/video/fbdev/platinumfb.c 2015-07-13 21:32:43.000000000 +1000 > +++ linux/drivers/video/fbdev/platinumfb.c 2015-07-13 21:33:02.000000000 +1000 > @@ -349,7 +349,7 @@ static int platinum_init_fb(struct fb_in > printk(KERN_INFO "platinumfb: Monitor sense value = 0x%x, ", sense); > if (default_vmode == VMODE_NVRAM) { > #ifdef CONFIG_NVRAM > - default_vmode = nvram_read_byte(NV_VMODE); > + default_vmode = arch_nvram_ops.read_byte(NV_VMODE); > if (default_vmode <= 0 || default_vmode > VMODE_MAX || > !platinum_reg_init[default_vmode-1]) > #endif > @@ -362,7 +362,7 @@ static int platinum_init_fb(struct fb_in > default_vmode = VMODE_640_480_60; > #ifdef CONFIG_NVRAM > if (default_cmode == CMODE_NVRAM) > - default_cmode = nvram_read_byte(NV_CMODE); > + default_cmode = arch_nvram_ops.read_byte(NV_CMODE); > #endif > if (default_cmode < CMODE_8 || default_cmode > CMODE_32) > default_cmode = CMODE_8; > Index: linux/drivers/video/fbdev/valkyriefb.c > =================================================================== > --- linux.orig/drivers/video/fbdev/valkyriefb.c 2015-07-13 21:32:43.000000000 +1000 > +++ linux/drivers/video/fbdev/valkyriefb.c 2015-07-13 21:33:02.000000000 +1000 > @@ -287,7 +287,7 @@ static void __init valkyrie_choose_mode( > /* Try to pick a video mode out of NVRAM if we have one. */ > #if !defined(CONFIG_MAC) && defined(CONFIG_NVRAM) > if (default_vmode == VMODE_NVRAM) { > - default_vmode = nvram_read_byte(NV_VMODE); > + default_vmode = arch_nvram_ops.read_byte(NV_VMODE); > if (default_vmode <= 0 > || default_vmode > VMODE_MAX > || !valkyrie_reg_init[default_vmode - 1]) > @@ -300,7 +300,7 @@ static void __init valkyrie_choose_mode( > default_vmode = VMODE_640_480_67; > #if !defined(CONFIG_MAC) && defined(CONFIG_NVRAM) > if (default_cmode == CMODE_NVRAM) > - default_cmode = nvram_read_byte(NV_CMODE); > + default_cmode = arch_nvram_ops.read_byte(NV_CMODE); > #endif > > /* > Index: linux/drivers/video/fbdev/imsttfb.c > =================================================================== > --- linux.orig/drivers/video/fbdev/imsttfb.c 2015-07-13 21:32:43.000000000 +1000 > +++ linux/drivers/video/fbdev/imsttfb.c 2015-07-13 21:33:02.000000000 +1000 > @@ -328,7 +328,6 @@ enum { > TVP = 1 > }; > > -#define USE_NV_MODES 1 > #define INIT_BPP 8 > #define INIT_XRES 640 > #define INIT_YRES 480 > @@ -1391,17 +1390,17 @@ static void init_imstt(struct fb_info *i > } > } > > -#if USE_NV_MODES && defined(CONFIG_PPC32) > +#if defined(CONFIG_NVRAM) && defined(CONFIG_PPC32) > { > int vmode = init_vmode, cmode = init_cmode; > > if (vmode == -1) { > - vmode = nvram_read_byte(NV_VMODE); > + vmode = arch_nvram_ops.read_byte(NV_VMODE); > if (vmode <= 0 || vmode > VMODE_MAX) > vmode = VMODE_640_480_67; > } > if (cmode == -1) { > - cmode = nvram_read_byte(NV_CMODE); > + cmode = arch_nvram_ops.read_byte(NV_CMODE); > if (cmode < CMODE_8 || cmode > CMODE_32) > cmode = CMODE_8; > } -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 14 Jul 2015, Benjamin Herrenschmidt wrote: > On Tue, 2015-07-14 at 17:58 +1000, Finn Thain wrote: > > Make use of arch_nvram_ops in device drivers so that the nvram_* > > function exports can be removed. > > > > Since they are no longer global symbols, rename the PPC32 nvram_* > > functions appropriately. > > > > Add the missing CONFIG_NVRAM test to imsttfb to avoid a build failure. > > > > Add a CONFIG_PPC32 test to matroxfb because PPC64 doesn't implement > > the read_byte() method. > > This is a bit fishy in a way because some of that nvram stuff is really > about powermac/apple nvram offsets, ie, "XPRAM". Yes, the generalization that PPC64 does not have XPRAM is wrong, but that wasn't originally my doing. If we were to address that issue, this patch series may not be the best place to do so. The situation presently is that CONFIG_NVRAM cannot be enabled on PPC64. I took advantage of that simplification, despite the corner cases where it fails. The corner cases are found among PPC64 systems with Matrox cards. The other PowerMac video drivers are not really relevant here due to "depends on PPC32" or "#if defined(CONFIG_PPC32)", meaning that nvram_read_byte() isn't a problem there. Perhaps only dual-boot systems are at issue because AFAIK only Mac OS offers a user friendly way to edit XPRAM settings (?) Further, does the video mode setting in XPRAM relate only to the MacOS main screen and not to other devices? That is, are we concerned here only with dual-boot PPC64 machines with one matrox card, as the main screen, and no Linux desktop environment and no video mode settings on the kernel command line? > Maybe we should have a dedicated accessor for "mac_xpram" and NULL-check > it rather than using ifdef's ? I wanted arch_nvram_ops to be const data, which means a NULL check won't work, because defined(CONFIG_PPC_PMAC) does not imply availability of XPRAM at run-time. There is a similar situation in the m68k portion of this patch series: a multi-platform kernel binary might run on an Atari or a Mac. On m68k I resolved this with MACH_IS_MAC(), which is analogous to machine_is(powermac). So I can see how to implement XPRAM for matroxfb and imsttfb on PPC64. But this is an enhancement that I would defer unless the present limitation is already problematic.
On Wed, 15 Jul 2015, I wrote: > On Tue, 14 Jul 2015, Benjamin Herrenschmidt wrote: > > > Maybe we should have a dedicated accessor for "mac_xpram" ... > > ... I can see how to implement XPRAM for matroxfb and imsttfb I'll have to retract that. The video mode and color mode settings used by the PowerMac framebuffer drivers don't exist in the PRAM portion of NVRAM. Addresses 0x140F and 0x1410 are found in the partition reserved by Apple for "Name Registry properties", according to Designing PCI Cards and Drivers for Power Macintosh Computers. There is no equivalent on m68k Macs, AFAIK. This is NVRAM partition 2 on my beige g3, which begins at 0x1400. I'm not sure that this is true on New World PowerMacs, and I suspect that the framebuffer drivers should be calling pmac_get_partition() to determine the offset of the beginning of the Name Registry partition. The arch_nvram_ops methods don't deal with structures like partitions. They treat the entire 8 KiB as unstructured, because that's how /dev/nvram treats it.
Hi Ben, On Thu, 16 Jul 2015, I wrote: > On Wed, 15 Jul 2015, I wrote: > > > On Tue, 14 Jul 2015, Benjamin Herrenschmidt wrote: > > > > > Maybe we should have a dedicated accessor for "mac_xpram" ... > > > ... > > The arch_nvram_ops methods don't deal with structures like partitions ... Instead of the accessor you suggested, perhaps it would be better to add a method like arch_nvram_ops.get_partition, to replace the pmac_get_partition() exported function? The call sites for pmac_get_partition() are in the implementation of the IOC_NVRAM_GET_OFFSET ioctl that's used with /dev/nvram, and in pmac_xpram_read(). pmac_xpram_write() has no caller and could be removed. But this doesn't have much to do with linux-fbdev. I think the old NV_CMODE/NV_VMODE issues*, which this patch avoids, are irrelevant to the problem of nvram module re-use, which is the aim of this patch series. But if those issues really are relevant then we should move the discussion to the revised patch, that is, [RFC v6 16/25] powerpc, fbdev: Use NV_CMODE and NV_VMODE only when CONFIG_PPC32 and CONFIG_PPC_PMAC and CONFIG_NVRAM. (There was no response to any patch in RFC v6 from any PowerPC maintainers, which is why I've revived this thread.) * https://lists.ozlabs.org/pipermail/linuxppc-dev/2001-November/012662.html
Index: linux/arch/powerpc/kernel/setup_32.c =================================================================== --- linux.orig/arch/powerpc/kernel/setup_32.c 2015-07-13 21:33:01.000000000 +1000 +++ linux/arch/powerpc/kernel/setup_32.c 2015-07-13 21:33:02.000000000 +1000 @@ -170,20 +170,18 @@ __setup("l3cr=", ppc_setup_l3cr); #ifdef CONFIG_GENERIC_NVRAM -unsigned char nvram_read_byte(int addr) +static unsigned char ppc_nvram_read_byte(int addr) { if (ppc_md.nvram_read_val) return ppc_md.nvram_read_val(addr); return 0xff; } -EXPORT_SYMBOL(nvram_read_byte); -void nvram_write_byte(unsigned char val, int addr) +static void ppc_nvram_write_byte(unsigned char val, int addr) { if (ppc_md.nvram_write_val) ppc_md.nvram_write_val(addr, val); } -EXPORT_SYMBOL(nvram_write_byte); static ssize_t ppc_nvram_get_size(void) { @@ -200,6 +198,8 @@ static long ppc_nvram_sync(void) } const struct nvram_ops arch_nvram_ops = { + .read_byte = ppc_nvram_read_byte, + .write_byte = ppc_nvram_write_byte, .get_size = ppc_nvram_get_size, .sync = ppc_nvram_sync, }; Index: linux/drivers/char/generic_nvram.c =================================================================== --- linux.orig/drivers/char/generic_nvram.c 2015-07-13 21:33:01.000000000 +1000 +++ linux/drivers/char/generic_nvram.c 2015-07-13 21:33:02.000000000 +1000 @@ -64,7 +64,7 @@ static ssize_t read_nvram(struct file *f if (*ppos >= nvram_len) return 0; for (i = *ppos; count > 0 && i < nvram_len; ++i, ++p, --count) - if (__put_user(nvram_read_byte(i), p)) + if (__put_user(arch_nvram_ops.read_byte(i), p)) return -EFAULT; *ppos = i; return p - buf; @@ -84,7 +84,7 @@ static ssize_t write_nvram(struct file * for (i = *ppos; count > 0 && i < nvram_len; ++i, ++p, --count) { if (__get_user(c, p)) return -EFAULT; - nvram_write_byte(c, i); + arch_nvram_ops.write_byte(c, i); } *ppos = i; return p - buf; Index: linux/drivers/video/fbdev/controlfb.c =================================================================== --- linux.orig/drivers/video/fbdev/controlfb.c 2015-07-13 21:32:43.000000000 +1000 +++ linux/drivers/video/fbdev/controlfb.c 2015-07-13 21:33:02.000000000 +1000 @@ -415,7 +415,7 @@ static int __init init_control(struct fb /* Try to pick a video mode out of NVRAM if we have one. */ #ifdef CONFIG_NVRAM if (default_cmode == CMODE_NVRAM) { - cmode = nvram_read_byte(NV_CMODE); + cmode = arch_nvram_ops.read_byte(NV_CMODE); if(cmode < CMODE_8 || cmode > CMODE_32) cmode = CMODE_8; } else @@ -423,7 +423,7 @@ static int __init init_control(struct fb cmode=default_cmode; #ifdef CONFIG_NVRAM if (default_vmode == VMODE_NVRAM) { - vmode = nvram_read_byte(NV_VMODE); + vmode = arch_nvram_ops.read_byte(NV_VMODE); if (vmode < 1 || vmode > VMODE_MAX || control_mac_modes[vmode - 1].m[full] < cmode) { sense = read_control_sense(p); Index: linux/drivers/video/fbdev/matrox/matroxfb_base.c =================================================================== --- linux.orig/drivers/video/fbdev/matrox/matroxfb_base.c 2015-07-13 21:32:57.000000000 +1000 +++ linux/drivers/video/fbdev/matrox/matroxfb_base.c 2015-07-13 21:33:02.000000000 +1000 @@ -1886,9 +1886,9 @@ static int initMatrox2(struct matrox_fb_ struct fb_var_screeninfo var; if (default_vmode <= 0 || default_vmode > VMODE_MAX) default_vmode = VMODE_640_480_60; -#ifdef CONFIG_NVRAM +#if defined(CONFIG_NVRAM) && defined(CONFIG_PPC32) if (default_cmode == CMODE_NVRAM) - default_cmode = nvram_read_byte(NV_CMODE); + default_cmode = arch_nvram_ops.read_byte(NV_CMODE); #endif if (default_cmode < CMODE_8 || default_cmode > CMODE_32) default_cmode = CMODE_8; Index: linux/drivers/video/fbdev/platinumfb.c =================================================================== --- linux.orig/drivers/video/fbdev/platinumfb.c 2015-07-13 21:32:43.000000000 +1000 +++ linux/drivers/video/fbdev/platinumfb.c 2015-07-13 21:33:02.000000000 +1000 @@ -349,7 +349,7 @@ static int platinum_init_fb(struct fb_in printk(KERN_INFO "platinumfb: Monitor sense value = 0x%x, ", sense); if (default_vmode == VMODE_NVRAM) { #ifdef CONFIG_NVRAM - default_vmode = nvram_read_byte(NV_VMODE); + default_vmode = arch_nvram_ops.read_byte(NV_VMODE); if (default_vmode <= 0 || default_vmode > VMODE_MAX || !platinum_reg_init[default_vmode-1]) #endif @@ -362,7 +362,7 @@ static int platinum_init_fb(struct fb_in default_vmode = VMODE_640_480_60; #ifdef CONFIG_NVRAM if (default_cmode == CMODE_NVRAM) - default_cmode = nvram_read_byte(NV_CMODE); + default_cmode = arch_nvram_ops.read_byte(NV_CMODE); #endif if (default_cmode < CMODE_8 || default_cmode > CMODE_32) default_cmode = CMODE_8; Index: linux/drivers/video/fbdev/valkyriefb.c =================================================================== --- linux.orig/drivers/video/fbdev/valkyriefb.c 2015-07-13 21:32:43.000000000 +1000 +++ linux/drivers/video/fbdev/valkyriefb.c 2015-07-13 21:33:02.000000000 +1000 @@ -287,7 +287,7 @@ static void __init valkyrie_choose_mode( /* Try to pick a video mode out of NVRAM if we have one. */ #if !defined(CONFIG_MAC) && defined(CONFIG_NVRAM) if (default_vmode == VMODE_NVRAM) { - default_vmode = nvram_read_byte(NV_VMODE); + default_vmode = arch_nvram_ops.read_byte(NV_VMODE); if (default_vmode <= 0 || default_vmode > VMODE_MAX || !valkyrie_reg_init[default_vmode - 1]) @@ -300,7 +300,7 @@ static void __init valkyrie_choose_mode( default_vmode = VMODE_640_480_67; #if !defined(CONFIG_MAC) && defined(CONFIG_NVRAM) if (default_cmode == CMODE_NVRAM) - default_cmode = nvram_read_byte(NV_CMODE); + default_cmode = arch_nvram_ops.read_byte(NV_CMODE); #endif /* Index: linux/drivers/video/fbdev/imsttfb.c =================================================================== --- linux.orig/drivers/video/fbdev/imsttfb.c 2015-07-13 21:32:43.000000000 +1000 +++ linux/drivers/video/fbdev/imsttfb.c 2015-07-13 21:33:02.000000000 +1000 @@ -328,7 +328,6 @@ enum { TVP = 1 }; -#define USE_NV_MODES 1 #define INIT_BPP 8 #define INIT_XRES 640 #define INIT_YRES 480 @@ -1391,17 +1390,17 @@ static void init_imstt(struct fb_info *i } } -#if USE_NV_MODES && defined(CONFIG_PPC32) +#if defined(CONFIG_NVRAM) && defined(CONFIG_PPC32) { int vmode = init_vmode, cmode = init_cmode; if (vmode == -1) { - vmode = nvram_read_byte(NV_VMODE); + vmode = arch_nvram_ops.read_byte(NV_VMODE); if (vmode <= 0 || vmode > VMODE_MAX) vmode = VMODE_640_480_67; } if (cmode == -1) { - cmode = nvram_read_byte(NV_CMODE); + cmode = arch_nvram_ops.read_byte(NV_CMODE); if (cmode < CMODE_8 || cmode > CMODE_32) cmode = CMODE_8; }
Make use of arch_nvram_ops in device drivers so that the nvram_* function exports can be removed. Since they are no longer global symbols, rename the PPC32 nvram_* functions appropriately. Add the missing CONFIG_NVRAM test to imsttfb to avoid a build failure. Add a CONFIG_PPC32 test to matroxfb because PPC64 doesn't implement the read_byte() method. Signed-off-by: Finn Thain <fthain@telegraphics.com.au> --- Changed since v4: - Added CONFIG_PPC32 test to matroxfb. --- arch/powerpc/kernel/setup_32.c | 8 ++++---- drivers/char/generic_nvram.c | 4 ++-- drivers/video/fbdev/controlfb.c | 4 ++-- drivers/video/fbdev/imsttfb.c | 7 +++---- drivers/video/fbdev/matrox/matroxfb_base.c | 4 ++-- drivers/video/fbdev/platinumfb.c | 4 ++-- drivers/video/fbdev/valkyriefb.c | 4 ++-- 7 files changed, 17 insertions(+), 18 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html