| Message ID | 1321556259-4459-2-git-send-email-timur@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kumar Gala |
| Headers | show |
Hi Timur, On Thu, 17 Nov 2011 12:57:39 -0600 Timur Tabi <timur@freescale.com> wrote: > > @@ -129,44 +144,117 @@ static void p1022ds_set_gamma_table(enum fsl_diu_monitor_port port, > */ > static void p1022ds_set_monitor_port(enum fsl_diu_monitor_port port) > { > - struct device_node *np; > - void __iomem *pixis; > - u8 __iomem *brdcfg1; > + struct device_node *guts_node = NULL; There is no point in initialising this as it is assigned before being used. > + struct device_node *indirect_node = NULL; > + struct ccsr_guts_85xx __iomem *guts = NULL; > + u8 __iomem *lbc_lcs0_ba = NULL; > + u8 __iomem *lbc_lcs1_ba = NULL; And if you had multiple error path labels, you could avoid these others as well (and the NULL checks on the error path).
On Thu, Nov 17, 2011 at 12:57:39PM -0600, Timur Tabi wrote: > When the P1022's DIU video controller is active, the pixis must be accessed > in "indirect" mode, which uses localbus chip select addresses. > > Switching between the DVI and LVDS monitor ports is handled by the pixis, > so that switching needs to be done via indirect mode. > > This has the side-effect of no longer requiring U-Boot to enable the DIU. > Now Linux can enable the DIU all by itself. Under what circumstances does Linux do this? How does Linux prevent the NOR flash driver from binding to the device when this mode has been or will be used? -Scott
Stephen Rothwell wrote: >> static void p1022ds_set_monitor_port(enum fsl_diu_monitor_port port) >> { >> - struct device_node *np; >> - void __iomem *pixis; >> - u8 __iomem *brdcfg1; >> + struct device_node *guts_node = NULL; > > There is no point in initialising this as it is assigned before being > used. Ok. >> + struct device_node *indirect_node = NULL; >> + struct ccsr_guts_85xx __iomem *guts = NULL; >> + u8 __iomem *lbc_lcs0_ba = NULL; >> + u8 __iomem *lbc_lcs1_ba = NULL; > > And if you had multiple error path labels, you could avoid these others > as well (and the NULL checks on the error path). But I don't want multiple error path labels. The error path could only happen if someone passed in a broken device tree (i.e. pretty much never), so I'm not keen on complicating my code just to optimize something that will never be used.
Scott Wood wrote: >> This has the side-effect of no longer requiring U-Boot to enable the DIU. >> Now Linux can enable the DIU all by itself. > > Under what circumstances does Linux do this? p1022ds_set_monitor_port() is called by the DIU driver when it enables the DIU. This happens on boot, for example, if you enable the framebuffer console. > How does Linux prevent the > NOR flash driver from binding to the device when this mode has been or > will be used? It doesn't. This isn't a simple problem to solve. On the P1022, NOR flash and the DIU are incompatible, and yet that's exactly what we ship on the P1022DS board. We could just remove the NOR flash node from the DTS.
On Thu, Nov 17, 2011 at 04:12:02PM -0600, Timur Tabi wrote: > Scott Wood wrote: > > >> This has the side-effect of no longer requiring U-Boot to enable the DIU. > >> Now Linux can enable the DIU all by itself. > > > > Under what circumstances does Linux do this? > > p1022ds_set_monitor_port() is called by the DIU driver when it enables > the DIU. This happens on boot, for example, if you enable the > framebuffer console. > > > How does Linux prevent the > > NOR flash driver from binding to the device when this mode has been or > > will be used? > > It doesn't. This isn't a simple problem to solve. On the P1022, NOR > flash and the DIU are incompatible, and yet that's exactly what we ship > on the P1022DS board. We could just remove the NOR flash node from the > DTS. As we discussed earlier, you could have a kernel boot parameter that indicates what mode you'd like the localbus to run in. Then, platform code could update the device tree before any drivers bind. Or, have U-boot set up the desired mode before entering the kernel, and provide an appropriate device tree. Letting the kernel bind to localbus devices such as NOR flash, and then taking the devices away without notice just because another device gets initialized, is not the way to go. We've already left "it just works" territory, might as well make the user choose explicitly. -Scott
Scott Wood wrote: > As we discussed earlier, you could have a kernel boot parameter that > indicates what mode you'd like the localbus to run in. Then, platform > code could update the device tree before any drivers bind. How do I update the device tree from platform code? > Or, have U-boot set up the desired mode before entering the kernel, and > provide an appropriate device tree. I can both of these features, but not in this patch.
On Thu, Nov 17, 2011 at 04:28:48PM -0600, Timur Tabi wrote: > Scott Wood wrote: > > > As we discussed earlier, you could have a kernel boot parameter that > > indicates what mode you'd like the localbus to run in. Then, platform > > code could update the device tree before any drivers bind. > > How do I update the device tree from platform code? prom_update_property() Or, since you shouldn't be declaring these devices directly under a simple-bus node, selectively probe the devices that are accessible. > > Or, have U-boot set up the desired mode before entering the kernel, and > > provide an appropriate device tree. > > I can both of these features, but not in this patch. This patch is the one that is adding a switch to indirect mode. You're adding it to the wrong place (it shouldn't be in a function called by the DIU driver), so it is relevant to this patch. -Scott
Scott Wood wrote: >> > How do I update the device tree from platform code? > prom_update_property() I assume this needs to be called after the DT has been unflattened, since it takes a device_node* as a parameter. Is there any way I can modify the tree before it's unflattened? I'd like to fixup the tree in an early_param() function.
On Fri, Nov 18, 2011 at 11:00:04AM -0600, Timur Tabi wrote: > Scott Wood wrote: > >> > How do I update the device tree from platform code? > > > prom_update_property() > > I assume this needs to be called after the DT has been unflattened, > since it takes a device_node* as a parameter. Is there any way I can > modify the tree before it's unflattened? I'd like to fixup the tree in > an early_param() function. What's wrong with doing it in setup_arch()? Modifying a flat device tree is a more complicated task, and the kernel's code is read-only. It's not worth bringing libfdt or similar complexity in for this, when there are other alternatives. If it really needs to be done early, consider doing it from U-Boot. The bootwrapper would be a decent place as well, but we probably shouldn't require its use just for this. -Scott
Scott Wood wrote:
> What's wrong with doing it in setup_arch()?
Well, I was hoping to encapsulate everything into one function -- the early_param() callback function. But I guess that's not going to work.
Hi Timur, On Thu, 17 Nov 2011 16:09:17 -0600 Timur Tabi <timur@freescale.com> wrote: > > Stephen Rothwell wrote: > > >> + struct device_node *indirect_node = NULL; > >> + struct ccsr_guts_85xx __iomem *guts = NULL; > >> + u8 __iomem *lbc_lcs0_ba = NULL; > >> + u8 __iomem *lbc_lcs1_ba = NULL; > > > > And if you had multiple error path labels, you could avoid these others > > as well (and the NULL checks on the error path). > > But I don't want multiple error path labels. The error path could only > happen if someone passed in a broken device tree (i.e. pretty much > never), so I'm not keen on complicating my code just to optimize > something that will never be used. But you are already optimizing for the error path by doing the assignments and NULL checks that are unneeded in the non error path. What I am suggesting is adding a little more code that could end up doing less at run time. How about (not even compile tested): static void p1022ds_set_monitor_port(enum fsl_diu_monitor_port port) { struct device_node *guts_node; struct device_node *indirect_node; struct ccsr_guts_85xx __iomem *guts; u8 __iomem *lbc_lcs0_ba; u8 __iomem *lbc_lcs1_ba; u8 b; /* Map the global utilities registers. */ guts_node = of_find_compatible_node(NULL, NULL, "fsl,p1022-guts"); if (!guts_node) { pr_err("p1022ds: missing global utilties device node\n"); return; } guts = of_iomap(guts_node, 0); of_node_put(guts_node); if (!guts) { pr_err("p1022ds: could not map global utilties device\n"); return; } indirect_node = of_find_compatible_node(NULL, NULL, "fsl,p1022ds-indirect-pixis"); if (!indirect_node) { pr_err("p1022ds: missing pixis indirect mode node\n"); goto exit_guts_iounmap; } lbc_lcs0_ba = of_iomap(indirect_node, 0); if (!lbc_lcs0_ba) { pr_err("p1022ds: could not map localbus chip select 0\n"); goto exit_indirect_node; } lbc_lcs1_ba = of_iomap(indirect_node, 1); if (!lbc_lcs1_ba) { pr_err("p1022ds: could not map localbus chip select 1\n"); goto exit_lcs0_iounmap; } /* Make sure we're in indirect mode first. */ if ((in_be32(&guts->pmuxcr) & PMUXCR_ELBCDIU_MASK) != PMUXCR_ELBCDIU_DIU) { struct device_node *pixis_node; void __iomem *pixis; pixis_node = of_find_compatible_node(NULL, NULL, "fsl,p1022ds-fpga"); if (!pixis_node) { pr_err("p1022ds: missing pixis node\n"); goto exit_lcs1_iounmap; } pixis = of_iomap(pixis_node, 0); of_node_put(pixis_node); if (!pixis) { pr_err("p1022ds: could not map pixis registers\n"); goto exit_lcs1_iounmap; } /* Enable indirect PIXIS mode. */ setbits8(pixis + PX_CTL, PX_CTL_ALTACC); iounmap(pixis); /* Switch the board mux to the DIU */ out_8(lbc_lcs0_ba, PX_BRDCFG0); /* BRDCFG0 */ b = in_8(lbc_lcs1_ba); b |= PX_BRDCFG0_ELBC_DIU; out_8(lbc_lcs1_ba, b); /* Set the chip mux to DIU mode. */ clrsetbits_be32(&guts->pmuxcr, PMUXCR_ELBCDIU_MASK, PMUXCR_ELBCDIU_DIU); in_be32(&guts->pmuxcr); } switch (port) { case FSL_DIU_PORT_DVI: /* Enable the DVI port, disable the DFP and the backlight */ out_8(lbc_lcs0_ba, PX_BRDCFG1); b = in_8(lbc_lcs1_ba); b &= ~(PX_BRDCFG1_DFPEN | PX_BRDCFG1_BACKLIGHT); b |= PX_BRDCFG1_DVIEN; out_8(lbc_lcs1_ba, b); break; case FSL_DIU_PORT_LVDS: /* * LVDS also needs backlight enabled, otherwise the display * will be blank. */ /* Enable the DFP port, disable the DVI and the backlight */ out_8(lbc_lcs0_ba, PX_BRDCFG1); b = in_8(lbc_lcs1_ba); b &= ~PX_BRDCFG1_DVIEN; b |= PX_BRDCFG1_DFPEN | PX_BRDCFG1_BACKLIGHT; out_8(lbc_lcs1_ba, b); break; default: pr_err("p1022ds: unsupported monitor port %i\n", port); } exit_lcs1_iounmap: iounmap(lbc_lcs1_ba); exit_lcs0_iounmap: iounmap(lbc_lcs0_ba); exit_indirect_node: of_node_put(indirect_node); exit_guts_iounmap: iounmap(guts); }
Stephen Rothwell wrote: > exit_lcs1_iounmap: > iounmap(lbc_lcs1_ba); > exit_lcs0_iounmap: > iounmap(lbc_lcs0_ba); > exit_indirect_node: > of_node_put(indirect_node); > exit_guts_iounmap: > iounmap(guts); The point I'm trying to make is that I don't want to have multiple goto exit labels. If I have to rearrange the code or add/delete code, then I probably would need to make similar changes to these labels. I just don't like that sort of thing. I appreciate your taking the time to review my code and provide suggestions. However, considering that I'm modifying my own code, I would hope that you can appreciate my personal coding style preference. And my style is to reduce the number of labels, even if it means superfluous checks in the exit code. So Kumar, if there are no further objections, please apply this patch as-is. Thank you.
diff --git a/arch/powerpc/platforms/85xx/p1022_ds.c b/arch/powerpc/platforms/85xx/p1022_ds.c index fda1571..7bdb9af 100644 --- a/arch/powerpc/platforms/85xx/p1022_ds.c +++ b/arch/powerpc/platforms/85xx/p1022_ds.c @@ -29,6 +29,10 @@ #if defined(CONFIG_FB_FSL_DIU) || defined(CONFIG_FB_FSL_DIU_MODULE) +#define PMUXCR_ELBCDIU_MASK 0xc0000000 +#define PMUXCR_ELBCDIU_NOR16 0x80000000 +#define PMUXCR_ELBCDIU_DIU 0x40000000 + /* * Board-specific initialization of the DIU. This code should probably be * executed when the DIU is opened, rather than in arch code, but the DIU @@ -46,11 +50,22 @@ #define CLKDVDR_PXCLK_MASK 0x00FF0000 /* Some ngPIXIS register definitions */ +#define PX_CTL 3 +#define PX_BRDCFG0 8 +#define PX_BRDCFG1 9 + +#define PX_BRDCFG0_ELBC_SPI_MASK 0xc0 +#define PX_BRDCFG0_ELBC_SPI_ELBC 0x00 +#define PX_BRDCFG0_ELBC_SPI_NULL 0xc0 +#define PX_BRDCFG0_ELBC_DIU 0x02 + #define PX_BRDCFG1_DVIEN 0x80 #define PX_BRDCFG1_DFPEN 0x40 #define PX_BRDCFG1_BACKLIGHT 0x20 #define PX_BRDCFG1_DDCEN 0x10 +#define PX_CTL_ALTACC 0x80 + /* * DIU Area Descriptor * @@ -129,44 +144,117 @@ static void p1022ds_set_gamma_table(enum fsl_diu_monitor_port port, */ static void p1022ds_set_monitor_port(enum fsl_diu_monitor_port port) { - struct device_node *np; - void __iomem *pixis; - u8 __iomem *brdcfg1; + struct device_node *guts_node = NULL; + struct device_node *indirect_node = NULL; + struct ccsr_guts_85xx __iomem *guts = NULL; + u8 __iomem *lbc_lcs0_ba = NULL; + u8 __iomem *lbc_lcs1_ba = NULL; + u8 b; - np = of_find_compatible_node(NULL, NULL, "fsl,p1022ds-fpga"); - if (!np) - /* older device trees used "fsl,p1022ds-pixis" */ - np = of_find_compatible_node(NULL, NULL, "fsl,p1022ds-pixis"); - if (!np) { - pr_err("p1022ds: missing ngPIXIS node\n"); + /* Map the global utilities registers. */ + guts_node = of_find_compatible_node(NULL, NULL, "fsl,p1022-guts"); + if (!guts_node) { + pr_err("p1022ds: missing global utilties device node\n"); return; } - pixis = of_iomap(np, 0); - if (!pixis) { - pr_err("p1022ds: could not map ngPIXIS registers\n"); - return; + guts = of_iomap(guts_node, 0); + if (!guts) { + pr_err("p1022ds: could not map global utilties device\n"); + goto exit; } - brdcfg1 = pixis + 9; /* BRDCFG1 is at offset 9 in the ngPIXIS */ + + indirect_node = of_find_compatible_node(NULL, NULL, + "fsl,p1022ds-indirect-pixis"); + if (!indirect_node) { + pr_err("p1022ds: missing pixis indirect mode node\n"); + goto exit; + } + + lbc_lcs0_ba = of_iomap(indirect_node, 0); + if (!lbc_lcs0_ba) { + pr_err("p1022ds: could not map localbus chip select 0\n"); + goto exit; + } + + lbc_lcs1_ba = of_iomap(indirect_node, 1); + if (!lbc_lcs1_ba) { + pr_err("p1022ds: could not map localbus chip select 1\n"); + goto exit; + } + + /* Make sure we're in indirect mode first. */ + if ((in_be32(&guts->pmuxcr) & PMUXCR_ELBCDIU_MASK) != + PMUXCR_ELBCDIU_DIU) { + struct device_node *pixis_node; + void __iomem *pixis; + + pixis_node = + of_find_compatible_node(NULL, NULL, "fsl,p1022ds-fpga"); + if (!pixis_node) { + pr_err("p1022ds: missing pixis node\n"); + goto exit; + } + + pixis = of_iomap(pixis_node, 0); + of_node_put(pixis_node); + if (!pixis) { + pr_err("p1022ds: could not map pixis registers\n"); + goto exit; + } + + /* Enable indirect PIXIS mode. */ + setbits8(pixis + PX_CTL, PX_CTL_ALTACC); + iounmap(pixis); + + /* Switch the board mux to the DIU */ + out_8(lbc_lcs0_ba, PX_BRDCFG0); /* BRDCFG0 */ + b = in_8(lbc_lcs1_ba); + b |= PX_BRDCFG0_ELBC_DIU; + out_8(lbc_lcs1_ba, b); + + /* Set the chip mux to DIU mode. */ + clrsetbits_be32(&guts->pmuxcr, PMUXCR_ELBCDIU_MASK, + PMUXCR_ELBCDIU_DIU); + in_be32(&guts->pmuxcr); + } + switch (port) { case FSL_DIU_PORT_DVI: - printk(KERN_INFO "%s:%u\n", __func__, __LINE__); /* Enable the DVI port, disable the DFP and the backlight */ - clrsetbits_8(brdcfg1, PX_BRDCFG1_DFPEN | PX_BRDCFG1_BACKLIGHT, - PX_BRDCFG1_DVIEN); + out_8(lbc_lcs0_ba, PX_BRDCFG1); + b = in_8(lbc_lcs1_ba); + b &= ~(PX_BRDCFG1_DFPEN | PX_BRDCFG1_BACKLIGHT); + b |= PX_BRDCFG1_DVIEN; + out_8(lbc_lcs1_ba, b); break; case FSL_DIU_PORT_LVDS: - printk(KERN_INFO "%s:%u\n", __func__, __LINE__); + /* + * LVDS also needs backlight enabled, otherwise the display + * will be blank. + */ /* Enable the DFP port, disable the DVI and the backlight */ - clrsetbits_8(brdcfg1, PX_BRDCFG1_DVIEN | PX_BRDCFG1_BACKLIGHT, - PX_BRDCFG1_DFPEN); + out_8(lbc_lcs0_ba, PX_BRDCFG1); + b = in_8(lbc_lcs1_ba); + b &= ~PX_BRDCFG1_DVIEN; + b |= PX_BRDCFG1_DFPEN | PX_BRDCFG1_BACKLIGHT; + out_8(lbc_lcs1_ba, b); break; default: pr_err("p1022ds: unsupported monitor port %i\n", port); } - iounmap(pixis); +exit: + if (lbc_lcs1_ba) + iounmap(lbc_lcs1_ba); + if (lbc_lcs0_ba) + iounmap(lbc_lcs0_ba); + if (guts) + iounmap(guts); + + of_node_put(indirect_node); + of_node_put(guts_node); } /**
When the P1022's DIU video controller is active, the pixis must be accessed in "indirect" mode, which uses localbus chip select addresses. Switching between the DVI and LVDS monitor ports is handled by the pixis, so that switching needs to be done via indirect mode. This has the side-effect of no longer requiring U-Boot to enable the DIU. Now Linux can enable the DIU all by itself. Signed-off-by: Timur Tabi <timur@freescale.com> --- arch/powerpc/platforms/85xx/p1022_ds.c | 132 ++++++++++++++++++++++++++----- 1 files changed, 110 insertions(+), 22 deletions(-)