| Message ID | 1313688345-17699-5-git-send-email-davidb@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thursday 18 August 2011, David Brown wrote: > +static void __init msm8660_surf_fpga_init(void __iomem *fpga_mem) > +{ > + /* Advanced mode */ > + writew(0xFFFF, fpga_mem + 0x15C); > + /* FPGA_UART_SEL */ > + writew(0, fpga_mem + 0x172); > + /* FPGA_GPIO_CONFIG_117 */ > + writew(1, fpga_mem + 0xEA); > + /* FPGA_GPIO_CONFIG_118 */ > + writew(1, fpga_mem + 0xEC); > + dmb(); > +} Does the dmb() do the right thing here? It seems strange to combine a strictly ordered I/O instruction with another ordering instruction, and I think it would be better to use writew_relaxed for the first one, followed by a 'wmb()'. > +#ifdef CONFIG_OF > +static void __init msm8660_surf_fpga_init_dt(void) > +{ > + struct device_node *node; > + void __iomem *fpga_mem; > + > + node = of_find_compatible_node(NULL, NULL, "qcom,msm8660-surf-fpga"); > + if (!node) > + return; > + > + fpga_mem = of_iomap(node, 0); > + of_node_put(node); > + if (!fpga_mem) { > + printk(KERN_ERR "%s: Can't map fpga registers\n", __func__); > + return; > + } > + > + msm8660_surf_fpga_init(fpga_mem); > + iounmap(fpga_mem); > +} > +#endif Is the serial port connected through the FPGA or just configured by it? In the former case, I think it would be better to make this a proper device driver that binds to the qcom,msm8660-surf-fpga device, configures it and then creates the platform_devices for the child nodes (the serial port, possibly others) by calling of_platform_bus_probe. Arnd
On Thu, Aug 25, 2011 at 01:27:12PM +0200, Arnd Bergmann wrote: > On Thursday 18 August 2011, David Brown wrote: > > +static void __init msm8660_surf_fpga_init(void __iomem *fpga_mem) > > +{ > > + /* Advanced mode */ > > + writew(0xFFFF, fpga_mem + 0x15C); > > + /* FPGA_UART_SEL */ > > + writew(0, fpga_mem + 0x172); > > + /* FPGA_GPIO_CONFIG_117 */ > > + writew(1, fpga_mem + 0xEA); > > + /* FPGA_GPIO_CONFIG_118 */ > > + writew(1, fpga_mem + 0xEC); > > + dmb(); > > +} > > Does the dmb() do the right thing here? It seems strange to combine a strictly > ordered I/O instruction with another ordering instruction, and I think it would > be better to use writew_relaxed for the first one, followed by a 'wmb()'. I guess I didn't really think about that, I just kind of kept the code. I'll ask Stepan why he did it that way, and come up with a cleaner solution. > > +#ifdef CONFIG_OF > > +static void __init msm8660_surf_fpga_init_dt(void) > > +{ > > + struct device_node *node; > > + void __iomem *fpga_mem; > > + > > + node = of_find_compatible_node(NULL, NULL, "qcom,msm8660-surf-fpga"); > > + if (!node) > > + return; > > + > > + fpga_mem = of_iomap(node, 0); > > + of_node_put(node); > > + if (!fpga_mem) { > > + printk(KERN_ERR "%s: Can't map fpga registers\n", __func__); > > + return; > > + } > > + > > + msm8660_surf_fpga_init(fpga_mem); > > + iounmap(fpga_mem); > > +} > > +#endif > > Is the serial port connected through the FPGA or just configured by it? The FPGA controls how the UART pins are connected on the development board. The serial port itself is in the MSM, not the FPGA, and on other dev boards this isn't needed for the serial port to work. > In the former case, I think it would be better to make this a proper > device driver that binds to the qcom,msm8660-surf-fpga device, > configures it and then creates the platform_devices for the child > nodes (the serial port, possibly others) by calling > of_platform_bus_probe. It might make sense to have the FPGA as a driver. I believe it was done early to make sure that the pins were configured correctly before the serial driver came up. As far as I can tell, the output pin is already configured correctly, so this can actually happen completely independently, since early usage of the UART is really only for console messages. I don't think it makes sense for the serial to be a child node, this FPGA configuration is more along the lines of pinmux. Most configurations of this SOC don't have or need this fpga. So, if I made it a separate driver, where would it go? Since this board still has platform device support, I suspect the platform data needed to describe this device would end up being larger than the driver itself. David
On Thursday 25 August 2011, David Brown wrote: > On Thu, Aug 25, 2011 at 01:27:12PM +0200, Arnd Bergmann wrote: > > On Thursday 18 August 2011, David Brown wrote: > > > +static void __init msm8660_surf_fpga_init(void __iomem *fpga_mem) > > > +{ > > > + /* Advanced mode */ > > > + writew(0xFFFF, fpga_mem + 0x15C); > > > + /* FPGA_UART_SEL */ > > > + writew(0, fpga_mem + 0x172); > > > + /* FPGA_GPIO_CONFIG_117 */ > > > + writew(1, fpga_mem + 0xEA); > > > + /* FPGA_GPIO_CONFIG_118 */ > > > + writew(1, fpga_mem + 0xEC); > > > + dmb(); > > > +} > > > > Does the dmb() do the right thing here? It seems strange to combine a strictly > > ordered I/O instruction with another ordering instruction, and I think it would > > be better to use writew_relaxed for the first one, followed by a 'wmb()'. > > I guess I didn't really think about that, I just kind of kept the > code. I'll ask Stepan why he did it that way, and come up with a > cleaner solution. Yes, no worries. I saw later that the code already exists in similar form, so it is not urgent to change. > > > +#ifdef CONFIG_OF > > > +static void __init msm8660_surf_fpga_init_dt(void) > > > +{ > > > + struct device_node *node; > > > + void __iomem *fpga_mem; > > > + > > > + node = of_find_compatible_node(NULL, NULL, "qcom,msm8660-surf-fpga"); > > > + if (!node) > > > + return; > > > + > > > + fpga_mem = of_iomap(node, 0); > > > + of_node_put(node); > > > + if (!fpga_mem) { > > > + printk(KERN_ERR "%s: Can't map fpga registers\n", __func__); > > > + return; > > > + } > > > + > > > + msm8660_surf_fpga_init(fpga_mem); > > > + iounmap(fpga_mem); > > > +} > > > +#endif > > > > Is the serial port connected through the FPGA or just configured by it? > > The FPGA controls how the UART pins are connected on the development > board. The serial port itself is in the MSM, not the FPGA, and on > other dev boards this isn't needed for the serial port to work. ok. > > In the former case, I think it would be better to make this a proper > > device driver that binds to the qcom,msm8660-surf-fpga device, > > configures it and then creates the platform_devices for the child > > nodes (the serial port, possibly others) by calling > > of_platform_bus_probe. > > It might make sense to have the FPGA as a driver. I believe it was > done early to make sure that the pins were configured correctly before > the serial driver came up. As far as I can tell, the output pin is > already configured correctly, so this can actually happen completely > independently, since early usage of the UART is really only for > console messages. > > I don't think it makes sense for the serial to be a child node, this > FPGA configuration is more along the lines of pinmux. Most > configurations of this SOC don't have or need this fpga. Agreed. > So, if I made it a separate driver, where would it go? Since this > board still has platform device support, I suspect the platform data > needed to describe this device would end up being larger than the > driver itself. Excellent question ;-) When the driver is really small, I would just leave it in the board file for now, although that might not be a good long-term strategy. Do we have any similar cases that we can group together with the fpga to make a subsystem? Maybe it could be a small driver in the pinmux subsystem when that is established. Arnd
> > So, if I made it a separate driver, where would it go? Since this > > board still has platform device support, I suspect the platform data > > needed to describe this device would end up being larger than the > > driver itself. > > Excellent question ;-) > > When the driver is really small, I would just leave it in the board > file for now, although that might not be a good long-term strategy. > Do we have any similar cases that we can group together with the > fpga to make a subsystem? Maybe it could be a small driver in the > pinmux subsystem when that is established. At this point, I think I'm just going to leave this FPGA initialization out. It seems that newer versions of the bootloader have made the FPGA entirely inaccessible. This has the unfortunate consequence of making the UART unusable on the MSM8660 SURF. I'll see if I can get this fixed at a bootloader level, or come up with another solution. So, meanwhile, I'll include the other patches, which work fine on the MSM 8660 Fluid target, and I'll just make an additional dsb file for this. Patch soon... The FLUID has the advantage of being a device that is technically available to people (although a bit pricey) http://www.bsquare.com/snapdragon-mobile-development-platform.aspx David
diff --git a/arch/arm/boot/dts/msm8660-surf.dts b/arch/arm/boot/dts/msm8660-surf.dts index 15ded0d..3591c94 100644 --- a/arch/arm/boot/dts/msm8660-surf.dts +++ b/arch/arm/boot/dts/msm8660-surf.dts @@ -21,4 +21,9 @@ <0x19c00000 0x1000>; interrupts = <195>; }; + + qcom,fpga@1d000000 { + compatible = "qcom,msm8660-surf-fpga"; + reg = < 0x1d000000 0x1000 >; + }; }; diff --git a/arch/arm/mach-msm/board-msm8x60.c b/arch/arm/mach-msm/board-msm8x60.c index 10fa8f6..f2dacfe 100644 --- a/arch/arm/mach-msm/board-msm8x60.c +++ b/arch/arm/mach-msm/board-msm8x60.c @@ -58,8 +58,52 @@ static void __init msm8x60_init_irq(void) } } +static void __init msm8660_surf_fpga_init(void __iomem *fpga_mem) +{ + /* Advanced mode */ + writew(0xFFFF, fpga_mem + 0x15C); + /* FPGA_UART_SEL */ + writew(0, fpga_mem + 0x172); + /* FPGA_GPIO_CONFIG_117 */ + writew(1, fpga_mem + 0xEA); + /* FPGA_GPIO_CONFIG_118 */ + writew(1, fpga_mem + 0xEC); + dmb(); +} + +static void __init msm8660_surf_fpga_init_platform(void) +{ + /* 0x1D000000 now belongs to EBI2:CS3 i.e. USB ISP Controller */ + void __iomem *fpga_mem = ioremap(0x1D000000, SZ_4K); + msm8660_surf_fpga_init(fpga_mem); + iounmap(fpga_mem); +} + +#ifdef CONFIG_OF +static void __init msm8660_surf_fpga_init_dt(void) +{ + struct device_node *node; + void __iomem *fpga_mem; + + node = of_find_compatible_node(NULL, NULL, "qcom,msm8660-surf-fpga"); + if (!node) + return; + + fpga_mem = of_iomap(node, 0); + of_node_put(node); + if (!fpga_mem) { + printk(KERN_ERR "%s: Can't map fpga registers\n", __func__); + return; + } + + msm8660_surf_fpga_init(fpga_mem); + iounmap(fpga_mem); +} +#endif + static void __init msm8x60_init(void) { + msm8660_surf_fpga_init_platform(); } #ifdef CONFIG_OF @@ -81,10 +125,7 @@ static void __init msm8x60_dt_init(void) if (node) irq_domain_add_simple(node, GIC_SPI_START); - if (of_machine_is_compatible("qcom,msm8660-surf")) { - printk(KERN_INFO "Init surf UART registers\n"); - msm8x60_init_uart12dm(); - } + msm8660_surf_fpga_init_dt(); of_platform_populate(NULL, of_default_bus_match_table, msm_auxdata_lookup, NULL);
The MSM 8660 SURF development board contains a register-accessible FPGA. By default, this FPGA configures the first UART in output-only mode. On this target, reconfigure this FPGA to enable the UART to be bidirectional. Signed-off-by: David Brown <davidb@codeaurora.org> --- arch/arm/boot/dts/msm8660-surf.dts | 5 +++ arch/arm/mach-msm/board-msm8x60.c | 49 +++++++++++++++++++++++++++++++++--- 2 files changed, 50 insertions(+), 4 deletions(-)