Olof, > Hi, > > Shortening the email a bit and only including the pieces that still have open questions: > > > On Thu, Feb 04, 2010 at 08:39:10PM +0530, Govindraj.R wrote: >> >> +#define OMAP_SERIAL_NAME ? ? "ttyO" >> >> +#define OMAP_SERIAL_MAJOR ? ?204 >> >> +#define OMAP_SERIAL_MINOR ? ?64 >> > >> > Where did these numbers come from? 204/64 are listed as in use in >> > Documentation/devices.txt. 213 is the first available minor there. (You >> > should also update that file). >> > >> >> I was referring to some custom uart soc driver files. >> Most of them seemed to use in an similar fashion, >> >> References: >> drivers/serial/samsung.c >> drivers/serial/timbuart.c >> drivers/serial/uartlite.c >> >> And many more which co-exist with 8250 driver. >> Use in an similar way. > > Yes, the way you do it is fine, but you need to select a different minor > number. You just copied the one that the samsung driver uses, you have > to select the next available one and update the documentation file I > named above. > Will update the minor number with the one avialable. >> >> + * @baud: baudrate for which divisor needs to be calculated. >> >> + * >> >> + * We have written our own function to get the divisor so as to support >> >> + * 13x mode. >> >> + */ >> > >> > Again, the why, not the how. Why do you need the 13x divisor? What's >> > magic about 3Mbaud? >> > >> >> Refering to TRM UART chapter 17: >> >> Table 17-1. UART Mode Baud Rates, Divisor Values, and Error Rates >> >> referring to oversampling - divisor value >> >> baudrate 460,800 to 3,686,400 all have divisor 13 >> >> except 3,000,000 which has divisor value 16 >> >> thus we are checking if baud != 3000000 > > Ok. It's always useful to have just a bit of information in the driver > so you don't have to search around the manual when trying to figure out > why something was done. > > Maybe something simple: "3Mbaud is unique in that it requires a divisor > of 13. See the TRM for full details". ok. will add that comments. > >> >> + ? ? serial_out(up, UART_LCR, UART_LCR_DLAB); >> >> + ? ? serial_out(up, UART_DLL, 0); >> >> + ? ? serial_out(up, UART_DLM, 0); >> >> + ? ? serial_out(up, UART_LCR, 0); >> >> + >> >> + ? ? serial_out(up, UART_LCR, 0xbf); >> >> + >> >> + ? ? up->efr = serial_in(up, UART_EFR); >> >> + ? ? serial_out(up, UART_EFR, up->efr | UART_EFR_ECB); >> >> + ? ? serial_out(up, UART_LCR, 0x0); ? ? ? ? ?/* Access FCR */ >> > >> > Access FCR by writing LCR? Please explain. Same goes for the ones below. >> >> >> Most of these configuration come straight from TRM: >> >> 7.5 UART/IrDA/CIR Basic Programming Model > > I was mostly referring to the fact that the comment says FCR, but the code says LCR. > Is it the same register with two names, or something else that's not quite correct? > >> >> + ? ? ? ? ? ? serial_out(up, UART_LCR, 0xbf); /* Access EFR */ >> > >> > EFR or LCR?! >> >> writing 0xbf to LCR gives acces to EFR >> >> references : 17.5 UART/IrDA/CIR Basic Programming Model > > Ah. Care to update the comment to mention that? I guess the case above is similar. Before accessing FCR we need to write 0x00 to LCR, I will correct it as, /**** Access to FCR requires writing Ox00 to LCR *****/ > >> > Nearly every OMAP setup seems to prefer 115200 as default baudrate, right? >> >> Yes, but shouldn't we initialize to lowest possible baud? > > I don't see why that should be done. It's easiest if it's initialized > to the most commonly used speed, that way people can drop the annoying > ",115200" from the console bootarg string. > >> >> +static >> >> +int serial_omap_suspend(struct platform_device *pdev, pm_message_t state) >> > >> > No linewrap (checkpatch should have caught this) >> >> I use checkpatch --strict, >> don't know how this passed > > > Yeah, seems like it doesn't check for it. :-) No big deal but since I was already > commenting on other things in the patch I mentioned it. > >> >> + ? ? ? ? ? ? ? ? ? ? up->uart_dma.tx_buf_dma_phys + UART_XMIT_SIZE) >> >> + ? ? ? ? ? ? up->uart_dma.tx_buf_size = >> >> + ? ? ? ? ? ? ? ? ? ? (up->uart_dma.tx_buf_dma_phys + UART_XMIT_SIZE) - start; >> >> + ? ? omap_set_dma_dest_params(up->uart_dma.tx_dma_channel, 0, >> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? OMAP_DMA_AMODE_CONSTANT, >> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? up->uart_dma.uart_base, 0, 0); >> >> + ? ? omap_set_dma_src_params(up->uart_dma.tx_dma_channel, 0, >> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? OMAP_DMA_AMODE_POST_INC, start, 0, 0); >> >> + ? ? omap_set_dma_transfer_params(up->uart_dma.tx_dma_channel, >> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? OMAP_DMA_DATA_TYPE_S8, >> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? up->uart_dma.tx_buf_size, 1, >> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? OMAP_DMA_SYNC_ELEMENT, >> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? up->uart_dma.uart_dma_tx, 0); >> > >> > >> > Isn't there a dma_sync call missing somewhere around here? Same for >> > the similar RX setup (both before DMA is started, as well as once it is >> > completed on the RX side). >> >> Sorry i didn't get you here as don't see any such dma_sync call >> in arch/arm/plat-omap/dma.c. > > It's not part of the OMAP dma driver, but instead it's part of the > DMA mapping API. You have to make sure that the buffers (as well as > descriptors) that the DMA engine are going to access will be written to > the level of coherency (i.e. memory) before the operation begins (for > dma-from-memory), and you have to make sure any caches are invalidated > after the operation completes and you wish to read the resulting data > (for dma-to-memory). > Could you refer me to any omap driver that currently use dma_sync? > > -Olof > -- Regards, Govindraj.R -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
