| Message ID | 1295651217-32421-3-git-send-email-twarren@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Tom, On Fri, 2011-01-21 at 16:06 -0700, Tom Warren wrote: > Signed-off-by: Tom Warren <twarren@nvidia.com> > --- > Changes for V2: > - Move serial driver to separate patch > > Changes for V5: > - Move arch/arm/cpu/armv7/uart.c & board.h to drivers/serial and > rename to serial_tegra2.c > - Remove use of uart_num & UART_A/D in serial_tegra2, simplify code > > arch/arm/cpu/armv7/tegra2/Makefile | 2 +- > arch/arm/cpu/armv7/tegra2/board.c | 2 +- > arch/arm/cpu/armv7/tegra2/board.h | 58 ---------- > arch/arm/cpu/armv7/tegra2/uart.c | 216 ------------------------------------ > common/serial.c | 3 +- > drivers/serial/Makefile | 1 + > drivers/serial/serial_tegra2.c | 205 ++++++++++++++++++++++++++++++++++ > drivers/serial/serial_tegra2.h | 49 ++++++++ > include/serial.h | 3 +- > 9 files changed, 261 insertions(+), 278 deletions(-) > delete mode 100644 arch/arm/cpu/armv7/tegra2/board.h > delete mode 100644 arch/arm/cpu/armv7/tegra2/uart.c > create mode 100644 drivers/serial/serial_tegra2.c > create mode 100644 drivers/serial/serial_tegra2.h It looks like arch/arm/cpu/armv7/tegra2/board.h and arch/arm/cpu/armv7/tegra2/uart.c are added in the first patch, then moved in this patch. It'd be ideal to just add them once in the proper location. On a side note, if you pass "git format-patch" the -M and -C options it will make pretty diffs that only show what lines changed during a move. In the case that you do move files in the future its nice to use those options to ease review. <snip> +void uart_init(void) > +{ > + /* Init each UART - there may be more than 1 on a board/build */ > +#if (CONFIG_TEGRA2_ENABLE_UARTA) > + init_uart(); > +#endif > +#if (CONFIG_TEGRA2_ENABLE_UARTD) > + init_uart(); > +#endif > +} How about: #if defined(CONFIG_TEGRA2_ENABLE_UARTA) || defined(CONFIG_TEGRA2_ENABLE_UARTD) init_uart(); #endif Best, Peter
Peter, On Fri, Jan 21, 2011 at 4:46 PM, Peter Tyser <ptyser@xes-inc.com> wrote: > Hi Tom, > > On Fri, 2011-01-21 at 16:06 -0700, Tom Warren wrote: >> Signed-off-by: Tom Warren <twarren@nvidia.com> >> --- >> Changes for V2: >> - Move serial driver to separate patch >> >> Changes for V5: >> - Move arch/arm/cpu/armv7/uart.c & board.h to drivers/serial and >> rename to serial_tegra2.c >> - Remove use of uart_num & UART_A/D in serial_tegra2, simplify code >> >> arch/arm/cpu/armv7/tegra2/Makefile | 2 +- >> arch/arm/cpu/armv7/tegra2/board.c | 2 +- >> arch/arm/cpu/armv7/tegra2/board.h | 58 ---------- >> arch/arm/cpu/armv7/tegra2/uart.c | 216 ------------------------------------ >> common/serial.c | 3 +- >> drivers/serial/Makefile | 1 + >> drivers/serial/serial_tegra2.c | 205 ++++++++++++++++++++++++++++++++++ >> drivers/serial/serial_tegra2.h | 49 ++++++++ >> include/serial.h | 3 +- >> 9 files changed, 261 insertions(+), 278 deletions(-) >> delete mode 100644 arch/arm/cpu/armv7/tegra2/board.h >> delete mode 100644 arch/arm/cpu/armv7/tegra2/uart.c >> create mode 100644 drivers/serial/serial_tegra2.c >> create mode 100644 drivers/serial/serial_tegra2.h > > It looks like arch/arm/cpu/armv7/tegra2/board.h and > arch/arm/cpu/armv7/tegra2/uart.c are added in the first patch, then > moved in this patch. It'd be ideal to just add them once in the proper > location. > > On a side note, if you pass "git format-patch" the -M and -C options it > will make pretty diffs that only show what lines changed during a move. > In the case that you do move files in the future its nice to use those > options to ease review. > I'll use those options in the future (thanks!) to keep things cleaner. Hopefully we can just go with this set of patches so I can get to the other, more interesting code (drivers, A9 CPU init, etc.). > <snip> > > +void uart_init(void) >> +{ >> + /* Init each UART - there may be more than 1 on a board/build */ >> +#if (CONFIG_TEGRA2_ENABLE_UARTA) >> + init_uart(); >> +#endif >> +#if (CONFIG_TEGRA2_ENABLE_UARTD) >> + init_uart(); >> +#endif >> +} > > How about: > #if defined(CONFIG_TEGRA2_ENABLE_UARTA) || defined(CONFIG_TEGRA2_ENABLE_UARTD) > init_uart(); > #endif That won't work for a board like Harmony where the developer wants both UARTs active, since uart_init is only called once. > > Best, > Peter Thanks, Tom > >
<snip> > > It looks like arch/arm/cpu/armv7/tegra2/board.h and > > arch/arm/cpu/armv7/tegra2/uart.c are added in the first patch, then > > moved in this patch. It'd be ideal to just add them once in the proper > > location. > > > > On a side note, if you pass "git format-patch" the -M and -C options it > > will make pretty diffs that only show what lines changed during a move. > > In the case that you do move files in the future its nice to use those > > options to ease review. > > > I'll use those options in the future (thanks!) to keep things cleaner. > Hopefully we can just go with this set of patches so I can get to the > other, more interesting code (drivers, A9 CPU init, etc.). Its up to the ARM maintainer and Wolfgang to decide if adding code in one patch just to move it around in the 2nd is acceptable. I'm guess it won't be acceptable since its considered "bad form", and its unclear what patch in this series contains what functionality. Eg this isn't really meant to "Add Tegra2 serial port support", it moves existing serial port code around? And more? Its not really just adding serial port support as the patch title states in any case. > > <snip> > > > > +void uart_init(void) > >> +{ > >> + /* Init each UART - there may be more than 1 on a board/build */ > >> +#if (CONFIG_TEGRA2_ENABLE_UARTA) > >> + init_uart(); > >> +#endif > >> +#if (CONFIG_TEGRA2_ENABLE_UARTD) > >> + init_uart(); > >> +#endif > >> +} > > > > How about: > > #if defined(CONFIG_TEGRA2_ENABLE_UARTA) || defined(CONFIG_TEGRA2_ENABLE_UARTD) > > init_uart(); > > #endif > That won't work for a board like Harmony where the developer wants > both UARTs active, since uart_init is only called once. Why should init_uart() be called two times? It looks to initialize both ports, meaning it should only be called once, right? Also, just noticed: +static void init_uart(void) +{ +#if CONFIG_TEGRA2_ENABLE_UARTA + uart_ctlr *const uart = (uart_ctlr *)NV_PA_APB_UARTA_BASE; + + uart_clock_init(); + + /* Enable UARTA - uses config 0 */ + pin_mux_uart(); + + setup_uart(uart); +#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ +#if CONFIG_TEGRA2_ENABLE_UARTD + uart_ctlr *const uart = (uart_ctlr *)NV_PA_APB_UARTD_BASE; + Have you compiled with both UARTA and UARTD enabled? Redeclaring 'uart' in the middle of the function should throw an error or warning. + uart_clock_init(); + + /* Enable UARTD - uses config 0 */ + pin_mux_uart(); + + setup_uart(uart); +#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ +} Best, Peter
Peter, On Mon, Jan 24, 2011 at 10:51 AM, Peter Tyser <ptyser@xes-inc.com> wrote: > <snip> > >> > It looks like arch/arm/cpu/armv7/tegra2/board.h and >> > arch/arm/cpu/armv7/tegra2/uart.c are added in the first patch, then >> > moved in this patch. It'd be ideal to just add them once in the proper >> > location. >> > >> > On a side note, if you pass "git format-patch" the -M and -C options it >> > will make pretty diffs that only show what lines changed during a move. >> > In the case that you do move files in the future its nice to use those >> > options to ease review. >> > >> I'll use those options in the future (thanks!) to keep things cleaner. >> Hopefully we can just go with this set of patches so I can get to the >> other, more interesting code (drivers, A9 CPU init, etc.). > > Its up to the ARM maintainer and Wolfgang to decide if adding code in > one patch just to move it around in the 2nd is acceptable. I'm guess it > won't be acceptable since its considered "bad form", and its unclear > what patch in this series contains what functionality. Eg this isn't > really meant to "Add Tegra2 serial port support", it moves existing > serial port code around? And more? Its not really just adding serial > port support as the patch title states in any case. I see what you're talking about now - I've got uart.c in 2 patch files - created in 0001 and then moved in 0002. My bad - that wasn't the intent, just what happened when I applied my V4 patches to a new branch to get the V5 patchset built. I'll fix it and resubmit. As to 0002 not adding serial port support for Tegra2, that's all it does - adds TEGRA2 defines to serial.h/serial.c for the eserial* tables, and then adds code to turn on Tegra2-specific UART HW. If I remove any mention of uart.c in patch 0001 (add basic Tegra2 support), then does patch 0002 make sense to you? > >> > <snip> >> > >> > +void uart_init(void) >> >> +{ >> >> + /* Init each UART - there may be more than 1 on a board/build */ >> >> +#if (CONFIG_TEGRA2_ENABLE_UARTA) >> >> + init_uart(); >> >> +#endif >> >> +#if (CONFIG_TEGRA2_ENABLE_UARTD) >> >> + init_uart(); >> >> +#endif >> >> +} >> > >> > How about: >> > #if defined(CONFIG_TEGRA2_ENABLE_UARTA) || defined(CONFIG_TEGRA2_ENABLE_UARTD) >> > init_uart(); >> > #endif >> That won't work for a board like Harmony where the developer wants >> both UARTs active, since uart_init is only called once. > > Why should init_uart() be called two times? It looks to initialize both > ports, meaning it should only be called once, right? Correct, again (need more coffee!) Your if defined construct wouldn't work as written, though, because CONFIG_TEGRA2_ENABLE_UARTx is always defined (as 0 or 1). I'd have to rework it. > > Also, just noticed: > +static void init_uart(void) > +{ > +#if CONFIG_TEGRA2_ENABLE_UARTA > + uart_ctlr *const uart = (uart_ctlr *)NV_PA_APB_UARTA_BASE; > + > + uart_clock_init(); > + > + /* Enable UARTA - uses config 0 */ > + pin_mux_uart(); > + > + setup_uart(uart); > +#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ > +#if CONFIG_TEGRA2_ENABLE_UARTD > + uart_ctlr *const uart = (uart_ctlr *)NV_PA_APB_UARTD_BASE; > + > > Have you compiled with both UARTA and UARTD enabled? Redeclaring 'uart' > in the middle of the function should throw an error or warning. I'd tested with both enabled earlier, but maybe not since the rewrite. I'll check & resubmit. > > + uart_clock_init(); > + > + /* Enable UARTD - uses config 0 */ > + pin_mux_uart(); > + > + setup_uart(uart); > +#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ > +} > > Best, > Peter Thanks, again, for your thoroughness! Tom > >
Dear Peter Tyser, In message <1295891506.2045.146.camel@petert> you wrote: > > > I'll use those options in the future (thanks!) to keep things cleaner. > > Hopefully we can just go with this set of patches so I can get to the > > other, more interesting code (drivers, A9 CPU init, etc.). > > Its up to the ARM maintainer and Wolfgang to decide if adding code in > one patch just to move it around in the 2nd is acceptable. I'm guess it No, it is not acceptable. This should be fixed. Thanks for pointing out. > Also, just noticed: > +static void init_uart(void) > +{ > +#if CONFIG_TEGRA2_ENABLE_UARTA > + uart_ctlr *const uart = (uart_ctlr *)NV_PA_APB_UARTA_BASE; > + > + uart_clock_init(); > + > + /* Enable UARTA - uses config 0 */ > + pin_mux_uart(); > + > + setup_uart(uart); > +#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ > +#if CONFIG_TEGRA2_ENABLE_UARTD > + uart_ctlr *const uart = (uart_ctlr *)NV_PA_APB_UARTD_BASE; > + > > Have you compiled with both UARTA and UARTD enabled? Redeclaring 'uart' > in the middle of the function should throw an error or warning. Even if the compiler should accept it (which I hpe not), then I will not accept this ;-) Thanks for pointing out. Best regards, Wolfgang Denk
Dear Tom Warren, In message <AANLkTi=uPxZUEeO52EJgQALWL0hznkdGvtdRzMMwibNp@mail.gmail.com> you wrote: > > Correct, again (need more coffee!) Your if defined construct wouldn't work > as written, though, because CONFIG_TEGRA2_ENABLE_UARTx is always > defined (as 0 or 1). I'd have to rework it. Please try and avoid relying on specific values. Rather use "#if defined". Best regards, Wolfgang Denk
<snip> > I see what you're talking about now - I've got uart.c in 2 patch files - created > in 0001 and then moved in 0002. My bad - that wasn't the intent, just what > happened when I applied my V4 patches to a new branch to get the V5 patchset > built. I'll fix it and resubmit. > > As to 0002 not adding serial port support for Tegra2, that's all it does - adds > TEGRA2 defines to serial.h/serial.c for the eserial* tables, and then adds > code to turn on Tegra2-specific UART HW. If I remove any mention of uart.c > in patch 0001 (add basic Tegra2 support), then does patch 0002 make > sense to you? Yeah, that'd make more sense. Patch 2 would just contain: common/serial.c | 3 +- drivers/serial/Makefile | 1 + drivers/serial/serial_tegra2.c | 205 ++++++++++++++++++++++++++++++++++ drivers/serial/serial_tegra2.h | 49 ++++++++ include/serial.h | 3 +- > >> > <snip> > >> > > >> > +void uart_init(void) > >> >> +{ > >> >> + /* Init each UART - there may be more than 1 on a board/build */ > >> >> +#if (CONFIG_TEGRA2_ENABLE_UARTA) > >> >> + init_uart(); > >> >> +#endif > >> >> +#if (CONFIG_TEGRA2_ENABLE_UARTD) > >> >> + init_uart(); > >> >> +#endif > >> >> +} > >> > > >> > How about: > >> > #if defined(CONFIG_TEGRA2_ENABLE_UARTA) || defined(CONFIG_TEGRA2_ENABLE_UARTD) > >> > init_uart(); > >> > #endif > >> That won't work for a board like Harmony where the developer wants > >> both UARTs active, since uart_init is only called once. > > > > Why should init_uart() be called two times? It looks to initialize both > > ports, meaning it should only be called once, right? > Correct, again (need more coffee!) Your if defined construct wouldn't work > as written, though, because CONFIG_TEGRA2_ENABLE_UARTx is always > defined (as 0 or 1). I'd have to rework it. You could also just get rid of uart_init() altogether and rename init_uart() to uart_init(). That would get rid of some idefs and simplify the flow. Best, Peter
Peter, On Mon, Jan 24, 2011 at 12:14 PM, Peter Tyser <ptyser@xes-inc.com> wrote: > <snip> > >> I see what you're talking about now - I've got uart.c in 2 patch files - created >> in 0001 and then moved in 0002. My bad - that wasn't the intent, just what >> happened when I applied my V4 patches to a new branch to get the V5 patchset >> built. I'll fix it and resubmit. >> >> As to 0002 not adding serial port support for Tegra2, that's all it does - adds >> TEGRA2 defines to serial.h/serial.c for the eserial* tables, and then adds >> code to turn on Tegra2-specific UART HW. If I remove any mention of uart.c >> in patch 0001 (add basic Tegra2 support), then does patch 0002 make >> sense to you? > > Yeah, that'd make more sense. Patch 2 would just contain: > common/serial.c | 3 +- > drivers/serial/Makefile | 1 + > drivers/serial/serial_tegra2.c | 205 ++++++++++++++++++++++++++++++++++ > drivers/serial/serial_tegra2.h | 49 ++++++++ > include/serial.h | 3 +- > Exactly what I was thinking. I'll try to get it right in patch V6. >> >> > <snip> >> >> > >> >> > +void uart_init(void) >> >> >> +{ >> >> >> + /* Init each UART - there may be more than 1 on a board/build */ >> >> >> +#if (CONFIG_TEGRA2_ENABLE_UARTA) >> >> >> + init_uart(); >> >> >> +#endif >> >> >> +#if (CONFIG_TEGRA2_ENABLE_UARTD) >> >> >> + init_uart(); >> >> >> +#endif >> >> >> +} >> >> > >> >> > How about: >> >> > #if defined(CONFIG_TEGRA2_ENABLE_UARTA) || defined(CONFIG_TEGRA2_ENABLE_UARTD) >> >> > init_uart(); >> >> > #endif >> >> That won't work for a board like Harmony where the developer wants >> >> both UARTs active, since uart_init is only called once. >> > >> > Why should init_uart() be called two times? It looks to initialize both >> > ports, meaning it should only be called once, right? >> Correct, again (need more coffee!) Your if defined construct wouldn't work >> as written, though, because CONFIG_TEGRA2_ENABLE_UARTx is always >> defined (as 0 or 1). I'd have to rework it. > > You could also just get rid of uart_init() altogether and rename > init_uart() to uart_init(). That would get rid of some idefs and > simplify the flow. Yeah, I saw that as I was cleaning up the indentation & reworking the code to compile with both UARTs defined. I'll get rid of uart_init (renamed to init_uart). Thanks. > > Best, > Peter Thanks, Tom > >
On 01/22/11 01:06, Tom Warren wrote: > Signed-off-by: Tom Warren <twarren@nvidia.com> > --- [ snip ] > +/* > + * Routine: pin_mux_uart > + * Description: setup the pin muxes/tristate values for a UART > + */ > +static void pin_mux_uart(void) > +{ > + pinmux_tri_ctlr *const pmt = (pinmux_tri_ctlr *)NV_PA_APB_MISC_BASE; > + u32 reg; > + > +#if CONFIG_TEGRA2_ENABLE_UARTA > + reg = readl(&pmt->pmt_ctl_c); > + reg &= 0xFFF0FFFF; /* IRRX_/IRTX_SEL [19:16] = 00 UARTA */ > + writel(reg, &pmt->pmt_ctl_c); > + > + reg = readl(&pmt->pmt_tri_a); > + reg &= ~Z_IRRX; /* Z_IRRX = normal (0) */ > + reg &= ~Z_IRTX; /* Z_IRTX = normal (0) */ > + writel(reg, &pmt->pmt_tri_a); > +#endif /* CONFIG_TEGRA2_ENABLE_UARTA */ > +#if CONFIG_TEGRA2_ENABLE_UARTD > + reg = readl(&pmt->pmt_ctl_b); > + reg &= 0xFFFFFFF3; /* GMC_SEL [3:2] = 00, UARTD */ > + writel(reg, &pmt->pmt_ctl_b); > + > + reg = readl(&pmt->pmt_tri_a); > + reg &= ~Z_GMC; /* Z_GMC = normal (0) */ > + writel(reg, &pmt->pmt_tri_a); > +#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ As I've already pointed out (1) this covers only one possibility of UART pin muxing options. I agree that it makes sense when this is a part of the board-specific code. However, forcing specific pins configuration in the generic driver seems problematic to me, even if most Tegra2 boards use the same configuration. As a side note, I'd prefer to have all the pin multiplexing defined in one place in board file rather than scattered among different drivers. (1) http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/92887/focus=93233
Mike, On Tue, Jan 25, 2011 at 1:11 AM, Mike Rapoport <mike@compulab.co.il> wrote: > On 01/22/11 01:06, Tom Warren wrote: >> Signed-off-by: Tom Warren <twarren@nvidia.com> >> --- > > [ snip ] > >> +/* >> + * Routine: pin_mux_uart >> + * Description: setup the pin muxes/tristate values for a UART >> + */ >> +static void pin_mux_uart(void) >> +{ >> + pinmux_tri_ctlr *const pmt = (pinmux_tri_ctlr *)NV_PA_APB_MISC_BASE; >> + u32 reg; >> + >> +#if CONFIG_TEGRA2_ENABLE_UARTA >> + reg = readl(&pmt->pmt_ctl_c); >> + reg &= 0xFFF0FFFF; /* IRRX_/IRTX_SEL [19:16] = 00 UARTA */ >> + writel(reg, &pmt->pmt_ctl_c); >> + >> + reg = readl(&pmt->pmt_tri_a); >> + reg &= ~Z_IRRX; /* Z_IRRX = normal (0) */ >> + reg &= ~Z_IRTX; /* Z_IRTX = normal (0) */ >> + writel(reg, &pmt->pmt_tri_a); >> +#endif /* CONFIG_TEGRA2_ENABLE_UARTA */ >> +#if CONFIG_TEGRA2_ENABLE_UARTD >> + reg = readl(&pmt->pmt_ctl_b); >> + reg &= 0xFFFFFFF3; /* GMC_SEL [3:2] = 00, UARTD */ >> + writel(reg, &pmt->pmt_ctl_b); >> + >> + reg = readl(&pmt->pmt_tri_a); >> + reg &= ~Z_GMC; /* Z_GMC = normal (0) */ >> + writel(reg, &pmt->pmt_tri_a); >> +#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ > > As I've already pointed out (1) this covers only one possibility of UART pin > muxing options. I agree that it makes sense when this is a part of the > board-specific code. However, forcing specific pins configuration in the generic > driver seems problematic to me, even if most Tegra2 boards use the same > configuration. > As a side note, I'd prefer to have all the pin multiplexing defined in one place > in board file rather than scattered among different drivers. > Alright. I'd like to get this wrapped up and accepted before the merge window closes so I can get on with the important bits (drivers, etc.). What would you like here? A generic function like 'pinmux_set_config(reg, val, bit)' that lives in the board files and gets called from the driver code with specifics of that board's/drivers pinmux config? Or do you see the pinmux code living entirely in the board files, and being done as part of board init? That's where it was before, BTW. If there are examples that you approve of in any extant U-Boot code (omap, ti91, davinci, etc.), please point them out. I'd really like to finalize this patch series. > (1) http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/92887/focus=93233 > > > -- > Sincerely yours, > Mike. Thanks, Tom >
On 01/25/11 18:50, Tom Warren wrote: > Mike, > > On Tue, Jan 25, 2011 at 1:11 AM, Mike Rapoport <mike@compulab.co.il> wrote: >> On 01/22/11 01:06, Tom Warren wrote: >>> Signed-off-by: Tom Warren <twarren@nvidia.com> >>> --- >> >> [ snip ] >> >>> +/* >>> + * Routine: pin_mux_uart >>> + * Description: setup the pin muxes/tristate values for a UART >>> + */ >>> +static void pin_mux_uart(void) >>> +{ >>> + pinmux_tri_ctlr *const pmt = (pinmux_tri_ctlr *)NV_PA_APB_MISC_BASE; >>> + u32 reg; >>> + >>> +#if CONFIG_TEGRA2_ENABLE_UARTA >>> + reg = readl(&pmt->pmt_ctl_c); >>> + reg &= 0xFFF0FFFF; /* IRRX_/IRTX_SEL [19:16] = 00 UARTA */ >>> + writel(reg, &pmt->pmt_ctl_c); >>> + >>> + reg = readl(&pmt->pmt_tri_a); >>> + reg &= ~Z_IRRX; /* Z_IRRX = normal (0) */ >>> + reg &= ~Z_IRTX; /* Z_IRTX = normal (0) */ >>> + writel(reg, &pmt->pmt_tri_a); >>> +#endif /* CONFIG_TEGRA2_ENABLE_UARTA */ >>> +#if CONFIG_TEGRA2_ENABLE_UARTD >>> + reg = readl(&pmt->pmt_ctl_b); >>> + reg &= 0xFFFFFFF3; /* GMC_SEL [3:2] = 00, UARTD */ >>> + writel(reg, &pmt->pmt_ctl_b); >>> + >>> + reg = readl(&pmt->pmt_tri_a); >>> + reg &= ~Z_GMC; /* Z_GMC = normal (0) */ >>> + writel(reg, &pmt->pmt_tri_a); >>> +#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ >> >> As I've already pointed out (1) this covers only one possibility of UART pin >> muxing options. I agree that it makes sense when this is a part of the >> board-specific code. However, forcing specific pins configuration in the generic >> driver seems problematic to me, even if most Tegra2 boards use the same >> configuration. >> As a side note, I'd prefer to have all the pin multiplexing defined in one place >> in board file rather than scattered among different drivers. >> > Alright. I'd like to get this wrapped up and accepted before the merge window > closes so I can get on with the important bits (drivers, etc.). What > would you like > here? A generic function like 'pinmux_set_config(reg, val, bit)' that > lives in the board > files and gets called from the driver code with specifics of that > board's/drivers pinmux > config? Or do you see the pinmux code living entirely in the board > files, and being done > as part of board init? That's where it was before, BTW. I think that the pinmux code should live entirely in the board file and performed as part of board init. The drivers than can assume that the pins are configured properly. > If there are examples that you approve of in any extant U-Boot code (omap, > ti91, davinci, etc.), please point them out. I'd really like to > finalize this patch series. I think OMAP is a good example. There's set_muxconf_regs function in each board file that is responsible for configuration of all the pins. >> (1) http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/92887/focus=93233 >> >> >> -- >> Sincerely yours, >> Mike. > Thanks, > > Tom >>
Mike, On Tue, Jan 25, 2011 at 2:12 PM, Mike Rapoport <mike@compulab.co.il> wrote: > On 01/25/11 18:50, Tom Warren wrote: >> Mike, >> >> On Tue, Jan 25, 2011 at 1:11 AM, Mike Rapoport <mike@compulab.co.il> wrote: >>> On 01/22/11 01:06, Tom Warren wrote: >>>> Signed-off-by: Tom Warren <twarren@nvidia.com> >>>> --- >>> >>> [ snip ] >>> >>>> +/* >>>> + * Routine: pin_mux_uart >>>> + * Description: setup the pin muxes/tristate values for a UART >>>> + */ >>>> +static void pin_mux_uart(void) >>>> +{ >>>> + pinmux_tri_ctlr *const pmt = (pinmux_tri_ctlr *)NV_PA_APB_MISC_BASE; >>>> + u32 reg; >>>> + >>>> +#if CONFIG_TEGRA2_ENABLE_UARTA >>>> + reg = readl(&pmt->pmt_ctl_c); >>>> + reg &= 0xFFF0FFFF; /* IRRX_/IRTX_SEL [19:16] = 00 UARTA */ >>>> + writel(reg, &pmt->pmt_ctl_c); >>>> + >>>> + reg = readl(&pmt->pmt_tri_a); >>>> + reg &= ~Z_IRRX; /* Z_IRRX = normal (0) */ >>>> + reg &= ~Z_IRTX; /* Z_IRTX = normal (0) */ >>>> + writel(reg, &pmt->pmt_tri_a); >>>> +#endif /* CONFIG_TEGRA2_ENABLE_UARTA */ >>>> +#if CONFIG_TEGRA2_ENABLE_UARTD >>>> + reg = readl(&pmt->pmt_ctl_b); >>>> + reg &= 0xFFFFFFF3; /* GMC_SEL [3:2] = 00, UARTD */ >>>> + writel(reg, &pmt->pmt_ctl_b); >>>> + >>>> + reg = readl(&pmt->pmt_tri_a); >>>> + reg &= ~Z_GMC; /* Z_GMC = normal (0) */ >>>> + writel(reg, &pmt->pmt_tri_a); >>>> +#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ >>> >>> As I've already pointed out (1) this covers only one possibility of UART pin >>> muxing options. I agree that it makes sense when this is a part of the >>> board-specific code. However, forcing specific pins configuration in the generic >>> driver seems problematic to me, even if most Tegra2 boards use the same >>> configuration. >>> As a side note, I'd prefer to have all the pin multiplexing defined in one place >>> in board file rather than scattered among different drivers. >>> >> Alright. I'd like to get this wrapped up and accepted before the merge window >> closes so I can get on with the important bits (drivers, etc.). What >> would you like >> here? A generic function like 'pinmux_set_config(reg, val, bit)' that >> lives in the board >> files and gets called from the driver code with specifics of that >> board's/drivers pinmux >> config? Or do you see the pinmux code living entirely in the board >> files, and being done >> as part of board init? That's where it was before, BTW. > > I think that the pinmux code should live entirely in the board file and > performed as part of board init. The drivers than can assume that the pins are > configured properly. OK. I'll move the clock/pinmux init code into armv7/tegra/board.c (since it's SoC-centric), and call it during board_init(). > >> If there are examples that you approve of in any extant U-Boot code (omap, >> ti91, davinci, etc.), please point them out. I'd really like to >> finalize this patch series. > > I think OMAP is a good example. There's set_muxconf_regs function in each board > file that is responsible for configuration of all the pins. I'll take a look. Thanks. > >>> (1) http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/92887/focus=93233 >>> >>> >>> -- >>> Sincerely yours, >>> Mike. >> Thanks, >> >> Tom >>> > > > -- > Sincerely yours, > Mike. >
Mike et al, On Tue, Jan 25, 2011 at 2:37 PM, Tom Warren <twarren.nvidia@gmail.com> wrote: > Mike, > > On Tue, Jan 25, 2011 at 2:12 PM, Mike Rapoport <mike@compulab.co.il> wrote: >> On 01/25/11 18:50, Tom Warren wrote: >>> Mike, >>> >>> On Tue, Jan 25, 2011 at 1:11 AM, Mike Rapoport <mike@compulab.co.il> wrote: >>>> On 01/22/11 01:06, Tom Warren wrote: >>>>> Signed-off-by: Tom Warren <twarren@nvidia.com> >>>>> --- >>>> >>>> [ snip ] >>>> >>>>> +/* >>>>> + * Routine: pin_mux_uart >>>>> + * Description: setup the pin muxes/tristate values for a UART >>>>> + */ >>>>> +static void pin_mux_uart(void) >>>>> +{ >>>>> + pinmux_tri_ctlr *const pmt = (pinmux_tri_ctlr *)NV_PA_APB_MISC_BASE; >>>>> + u32 reg; >>>>> + >>>>> +#if CONFIG_TEGRA2_ENABLE_UARTA >>>>> + reg = readl(&pmt->pmt_ctl_c); >>>>> + reg &= 0xFFF0FFFF; /* IRRX_/IRTX_SEL [19:16] = 00 UARTA */ >>>>> + writel(reg, &pmt->pmt_ctl_c); >>>>> + >>>>> + reg = readl(&pmt->pmt_tri_a); >>>>> + reg &= ~Z_IRRX; /* Z_IRRX = normal (0) */ >>>>> + reg &= ~Z_IRTX; /* Z_IRTX = normal (0) */ >>>>> + writel(reg, &pmt->pmt_tri_a); >>>>> +#endif /* CONFIG_TEGRA2_ENABLE_UARTA */ >>>>> +#if CONFIG_TEGRA2_ENABLE_UARTD >>>>> + reg = readl(&pmt->pmt_ctl_b); >>>>> + reg &= 0xFFFFFFF3; /* GMC_SEL [3:2] = 00, UARTD */ >>>>> + writel(reg, &pmt->pmt_ctl_b); >>>>> + >>>>> + reg = readl(&pmt->pmt_tri_a); >>>>> + reg &= ~Z_GMC; /* Z_GMC = normal (0) */ >>>>> + writel(reg, &pmt->pmt_tri_a); >>>>> +#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ >>>> >>>> As I've already pointed out (1) this covers only one possibility of UART pin >>>> muxing options. I agree that it makes sense when this is a part of the >>>> board-specific code. However, forcing specific pins configuration in the generic >>>> driver seems problematic to me, even if most Tegra2 boards use the same >>>> configuration. >>>> As a side note, I'd prefer to have all the pin multiplexing defined in one place >>>> in board file rather than scattered among different drivers. >>>> >>> Alright. I'd like to get this wrapped up and accepted before the merge window >>> closes so I can get on with the important bits (drivers, etc.). What >>> would you like >>> here? A generic function like 'pinmux_set_config(reg, val, bit)' that >>> lives in the board >>> files and gets called from the driver code with specifics of that >>> board's/drivers pinmux >>> config? Or do you see the pinmux code living entirely in the board >>> files, and being done >>> as part of board init? That's where it was before, BTW. >> >> I think that the pinmux code should live entirely in the board file and >> performed as part of board init. The drivers than can assume that the pins are >> configured properly. > OK. I'll move the clock/pinmux init code into armv7/tegra/board.c > (since it's SoC-centric), > and call it during board_init(). Actually, I think it makes more sense to move pinmux_init_uart and clock_init_uart to board/nvidia/common/board.c. These routines are more board-specific than SoC-specific - they depend on how the UART is routed on a board. Let me do that & gen up a new patchset. > >> >>> If there are examples that you approve of in any extant U-Boot code (omap, >>> ti91, davinci, etc.), please point them out. I'd really like to >>> finalize this patch series. >> >> I think OMAP is a good example. There's set_muxconf_regs function in each board >> file that is responsible for configuration of all the pins. > I'll take a look. Thanks. >> >>>> (1) http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/92887/focus=93233 >>>> >>>> >>>> -- >>>> Sincerely yours, >>>> Mike. >>> Thanks, >>> >>> Tom >>>> >> >> >> -- >> Sincerely yours, >> Mike. >> >
> >>>> [ snip ] > >>>> > >>>>> +/* > >>>>> + * Routine: pin_mux_uart > >>>>> + * Description: setup the pin muxes/tristate values for a UART > >>>>> + */ > >>>>> +static void pin_mux_uart(void) > >>>>> +{ > >>>>> + pinmux_tri_ctlr *const pmt = (pinmux_tri_ctlr *)NV_PA_APB_MISC_BASE; > >>>>> + u32 reg; > >>>>> + > >>>>> +#if CONFIG_TEGRA2_ENABLE_UARTA > >>>>> + reg = readl(&pmt->pmt_ctl_c); > >>>>> + reg &= 0xFFF0FFFF; /* IRRX_/IRTX_SEL [19:16] = 00 UARTA */ > >>>>> + writel(reg, &pmt->pmt_ctl_c); > >>>>> + > >>>>> + reg = readl(&pmt->pmt_tri_a); > >>>>> + reg &= ~Z_IRRX; /* Z_IRRX = normal (0) */ > >>>>> + reg &= ~Z_IRTX; /* Z_IRTX = normal (0) */ > >>>>> + writel(reg, &pmt->pmt_tri_a); > >>>>> +#endif /* CONFIG_TEGRA2_ENABLE_UARTA */ > >>>>> +#if CONFIG_TEGRA2_ENABLE_UARTD > >>>>> + reg = readl(&pmt->pmt_ctl_b); > >>>>> + reg &= 0xFFFFFFF3; /* GMC_SEL [3:2] = 00, UARTD */ > >>>>> + writel(reg, &pmt->pmt_ctl_b); > >>>>> + > >>>>> + reg = readl(&pmt->pmt_tri_a); > >>>>> + reg &= ~Z_GMC; /* Z_GMC = normal (0) */ > >>>>> + writel(reg, &pmt->pmt_tri_a); > >>>>> +#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ > >>>> > >>>> As I've already pointed out (1) this covers only one possibility of UART pin > >>>> muxing options. I agree that it makes sense when this is a part of the > >>>> board-specific code. However, forcing specific pins configuration in the generic > >>>> driver seems problematic to me, even if most Tegra2 boards use the same > >>>> configuration. > >>>> As a side note, I'd prefer to have all the pin multiplexing defined in one place > >>>> in board file rather than scattered among different drivers. > >>>> > >>> Alright. I'd like to get this wrapped up and accepted before the merge window > >>> closes so I can get on with the important bits (drivers, etc.). What > >>> would you like > >>> here? A generic function like 'pinmux_set_config(reg, val, bit)' that > >>> lives in the board > >>> files and gets called from the driver code with specifics of that > >>> board's/drivers pinmux > >>> config? Or do you see the pinmux code living entirely in the board > >>> files, and being done > >>> as part of board init? That's where it was before, BTW. > >> > >> I think that the pinmux code should live entirely in the board file and > >> performed as part of board init. The drivers than can assume that the pins are > >> configured properly. > > OK. I'll move the clock/pinmux init code into armv7/tegra/board.c > > (since it's SoC-centric), > > and call it during board_init(). > Actually, I think it makes more sense to move pinmux_init_uart and > clock_init_uart to board/nvidia/common/board.c. > These routines are more board-specific than SoC-specific - they depend > on how the UART is routed on a board. > Let me do that & gen up a new patchset. You previously mentioned that "To date, all of our Tegra boards use these pinmux options for both UARTs. If a board vendor chooses to use different pinmuxes, then they can override these funcs in their board files, or use their own code triggered by their own defines. But according to our HW guys, the vast majority will use these pins" If the vast majority of boards really will use the same pinmuxing, it would be nice to provide that common muxing as a default which can be overridden. Perhaps moving pinmux_init_uart() and uart_clock_init() into armv7/tegra/*, and making them weak functions would make everyone happy. Or could you make default defines that board's could override as needed? eg in pin_mux_uart(): #ifndef CONFIG_TEGRA2_PMT_CTLC_MASK #define CONFIG_TEGRA2_PMC_CTLC_MASK 0xfff0ffff #endif #ifndef CONFIG_TEGRA2_PMT_TRI_A_MASK #define CONFIG_TEGRA2_PMC_TRI_A_MASK ~(Z_IRRX | Z_IRTX) #endif pin_mux_uart() { reg = readl(&pmt->pmt_ctl_c); reg &= CONFIG_TEGRA2_PMT_CTLC_MASK; writel(reg, &pmt->pmt_ctl_c); reg = readl(&pmt->pmt_tri_a); reg &= CONFIG_TEGRA2_PMC_TRI_A_MASK; writel(reg, &pmt->pmt_tri_a); } Or make the functions table driven so each board would define a pinmux/clock configuration table? I think its worthwhile to try and reduce duplicated code whenever possible. Best, Peter
On 01/26/11 00:24, Peter Tyser wrote: >>>>>> As I've already pointed out (1) this covers only one possibility of UART pin >>>>>> muxing options. I agree that it makes sense when this is a part of the >>>>>> board-specific code. However, forcing specific pins configuration in the generic >>>>>> driver seems problematic to me, even if most Tegra2 boards use the same >>>>>> configuration. >>>>>> As a side note, I'd prefer to have all the pin multiplexing defined in one place >>>>>> in board file rather than scattered among different drivers. >>>>>> >>>>> Alright. I'd like to get this wrapped up and accepted before the merge window >>>>> closes so I can get on with the important bits (drivers, etc.). What >>>>> would you like >>>>> here? A generic function like 'pinmux_set_config(reg, val, bit)' that >>>>> lives in the board >>>>> files and gets called from the driver code with specifics of that >>>>> board's/drivers pinmux >>>>> config? Or do you see the pinmux code living entirely in the board >>>>> files, and being done >>>>> as part of board init? That's where it was before, BTW. >>>> >>>> I think that the pinmux code should live entirely in the board file and >>>> performed as part of board init. The drivers than can assume that the pins are >>>> configured properly. >>> OK. I'll move the clock/pinmux init code into armv7/tegra/board.c >>> (since it's SoC-centric), >>> and call it during board_init(). >> Actually, I think it makes more sense to move pinmux_init_uart and >> clock_init_uart to board/nvidia/common/board.c. >> These routines are more board-specific than SoC-specific - they depend >> on how the UART is routed on a board. >> Let me do that & gen up a new patchset. > > You previously mentioned that "To date, all of our Tegra boards use > these pinmux options for both UARTs. If a board vendor chooses to use > different pinmuxes, then they can override these funcs in their board > files, or use their own code triggered by their own defines. But > according to our HW guys, the vast majority will use these pins" > > If the vast majority of boards really will use the same pinmuxing, it > would be nice to provide that common muxing as a default which can be > overridden. Perhaps moving pinmux_init_uart() and uart_clock_init() > into armv7/tegra/*, and making them weak functions would make everyone > happy. My point was that pin muxing belongs to the board code rather than to the driver. Driver should just assume that pins are configured elsewhere and it does not need to deal with pin muxing at all. Moreover, I'd prefer to see pinmux_board_init or something similar that configures all the pins at once rather than collection of pinmux_init_uart, pinmux_init_sdmmc, pinmux_init_gmi etc that will grow as more drivers are added. > Or could you make default defines that board's could override as needed? > eg in pin_mux_uart(): > #ifndef CONFIG_TEGRA2_PMT_CTLC_MASK > #define CONFIG_TEGRA2_PMC_CTLC_MASK 0xfff0ffff > #endif > #ifndef CONFIG_TEGRA2_PMT_TRI_A_MASK > #define CONFIG_TEGRA2_PMC_TRI_A_MASK ~(Z_IRRX | Z_IRTX) > #endif > pin_mux_uart() { > reg = readl(&pmt->pmt_ctl_c); > reg &= CONFIG_TEGRA2_PMT_CTLC_MASK; > writel(reg, &pmt->pmt_ctl_c); > > reg = readl(&pmt->pmt_tri_a); > reg &= CONFIG_TEGRA2_PMC_TRI_A_MASK; > writel(reg, &pmt->pmt_tri_a); > } > > Or make the functions table driven so each board would define a > pinmux/clock configuration table? > > I think its worthwhile to try and reduce duplicated code whenever > possible. > > Best, > Peter >
On Wed, 2011-01-26 at 10:13 +0200, Mike Rapoport wrote: > On 01/26/11 00:24, Peter Tyser wrote: > >>>>>> As I've already pointed out (1) this covers only one possibility of UART pin > >>>>>> muxing options. I agree that it makes sense when this is a part of the > >>>>>> board-specific code. However, forcing specific pins configuration in the generic > >>>>>> driver seems problematic to me, even if most Tegra2 boards use the same > >>>>>> configuration. > >>>>>> As a side note, I'd prefer to have all the pin multiplexing defined in one place > >>>>>> in board file rather than scattered among different drivers. > >>>>>> > >>>>> Alright. I'd like to get this wrapped up and accepted before the merge window > >>>>> closes so I can get on with the important bits (drivers, etc.). What > >>>>> would you like > >>>>> here? A generic function like 'pinmux_set_config(reg, val, bit)' that > >>>>> lives in the board > >>>>> files and gets called from the driver code with specifics of that > >>>>> board's/drivers pinmux > >>>>> config? Or do you see the pinmux code living entirely in the board > >>>>> files, and being done > >>>>> as part of board init? That's where it was before, BTW. > >>>> > >>>> I think that the pinmux code should live entirely in the board file and > >>>> performed as part of board init. The drivers than can assume that the pins are > >>>> configured properly. > >>> OK. I'll move the clock/pinmux init code into armv7/tegra/board.c > >>> (since it's SoC-centric), > >>> and call it during board_init(). > >> Actually, I think it makes more sense to move pinmux_init_uart and > >> clock_init_uart to board/nvidia/common/board.c. > >> These routines are more board-specific than SoC-specific - they depend > >> on how the UART is routed on a board. > >> Let me do that & gen up a new patchset. > > > > You previously mentioned that "To date, all of our Tegra boards use > > these pinmux options for both UARTs. If a board vendor chooses to use > > different pinmuxes, then they can override these funcs in their board > > files, or use their own code triggered by their own defines. But > > according to our HW guys, the vast majority will use these pins" > > > > If the vast majority of boards really will use the same pinmuxing, it > > would be nice to provide that common muxing as a default which can be > > overridden. Perhaps moving pinmux_init_uart() and uart_clock_init() > > into armv7/tegra/*, and making them weak functions would make everyone > > happy. > > My point was that pin muxing belongs to the board code rather than to the > driver. Driver should just assume that pins are configured elsewhere and it does > not need to deal with pin muxing at all. I understand your point, but think if 9/10 boards use the same pin muxing its good to provide a default so we don't have 9 chunks of duplicate code floating around in board/*. What's the harm in providing a default? It can be overridden if needed. > Moreover, I'd prefer to see pinmux_board_init or something similar that > configures all the pins at once rather than collection of pinmux_init_uart, > pinmux_init_sdmmc, pinmux_init_gmi etc that will grow as more drivers are added. I can see that point but its a different discussion. I don't know enough about the Tegra2 to comment on this, but it seems like a good idea based on previous experiences with boards with similar pinmuxing (eg mpc8260). In my last email I mentioned a table-driven approach (similar to the mpc8260 implementation?) which sounds somewhat like you're proposing. Best, Peter
Mike, On Wed, Jan 26, 2011 at 1:13 AM, Mike Rapoport <mike@compulab.co.il> wrote: > On 01/26/11 00:24, Peter Tyser wrote: >>>>>>> As I've already pointed out (1) this covers only one possibility of UART pin >>>>>>> muxing options. I agree that it makes sense when this is a part of the >>>>>>> board-specific code. However, forcing specific pins configuration in the generic >>>>>>> driver seems problematic to me, even if most Tegra2 boards use the same >>>>>>> configuration. >>>>>>> As a side note, I'd prefer to have all the pin multiplexing defined in one place >>>>>>> in board file rather than scattered among different drivers. >>>>>>> >>>>>> Alright. I'd like to get this wrapped up and accepted before the merge window >>>>>> closes so I can get on with the important bits (drivers, etc.). What >>>>>> would you like >>>>>> here? A generic function like 'pinmux_set_config(reg, val, bit)' that >>>>>> lives in the board >>>>>> files and gets called from the driver code with specifics of that >>>>>> board's/drivers pinmux >>>>>> config? Or do you see the pinmux code living entirely in the board >>>>>> files, and being done >>>>>> as part of board init? That's where it was before, BTW. >>>>> >>>>> I think that the pinmux code should live entirely in the board file and >>>>> performed as part of board init. The drivers than can assume that the pins are >>>>> configured properly. >>>> OK. I'll move the clock/pinmux init code into armv7/tegra/board.c >>>> (since it's SoC-centric), >>>> and call it during board_init(). >>> Actually, I think it makes more sense to move pinmux_init_uart and >>> clock_init_uart to board/nvidia/common/board.c. >>> These routines are more board-specific than SoC-specific - they depend >>> on how the UART is routed on a board. >>> Let me do that & gen up a new patchset. >> >> You previously mentioned that "To date, all of our Tegra boards use >> these pinmux options for both UARTs. If a board vendor chooses to use >> different pinmuxes, then they can override these funcs in their board >> files, or use their own code triggered by their own defines. But >> according to our HW guys, the vast majority will use these pins" >> >> If the vast majority of boards really will use the same pinmuxing, it >> would be nice to provide that common muxing as a default which can be >> overridden. Perhaps moving pinmux_init_uart() and uart_clock_init() >> into armv7/tegra/*, and making them weak functions would make everyone >> happy. > > My point was that pin muxing belongs to the board code rather than to the > driver. Driver should just assume that pins are configured elsewhere and it does > not need to deal with pin muxing at all. I understand that point - sorry if I wasn't clear. No objection to having pinmux code in board files. > Moreover, I'd prefer to see pinmux_board_init or something similar that > configures all the pins at once rather than collection of pinmux_init_uart, > pinmux_init_sdmmc, pinmux_init_gmi etc that will grow as more drivers are added. > I see a couple of reasons not to do it that way. First, I don't know at this time what all the pinmux settings will be, since I haven't ported all the periph driver code yet. It's vastly different from what's acceptable in U-Boot, and will all need significant rewrite. It'd take me a week to gather all that info, and I'm not at full BW on this project (one of 4 on my plate right now). Second, I've been chastised before for including code/features in this initial patchset that aren't needed or used. I'm trying to keep the code as simple as possible to make it easier on reviewers and get through the review in as short a time as possible. This has already dragged on far longer than I thought it would. I'm willing to change the pinmux code to make it as generic as possible, but only if there's a consensus on the list that it has to be that way to get accepted & pushed. >> Or could you make default defines that board's could override as needed? >> eg in pin_mux_uart(): >> #ifndef CONFIG_TEGRA2_PMT_CTLC_MASK >> #define CONFIG_TEGRA2_PMC_CTLC_MASK 0xfff0ffff >> #endif >> #ifndef CONFIG_TEGRA2_PMT_TRI_A_MASK >> #define CONFIG_TEGRA2_PMC_TRI_A_MASK ~(Z_IRRX | Z_IRTX) >> #endif >> pin_mux_uart() { >> reg = readl(&pmt->pmt_ctl_c); >> reg &= CONFIG_TEGRA2_PMT_CTLC_MASK; >> writel(reg, &pmt->pmt_ctl_c); >> >> reg = readl(&pmt->pmt_tri_a); >> reg &= CONFIG_TEGRA2_PMC_TRI_A_MASK; >> writel(reg, &pmt->pmt_tri_a); >> } >> >> Or make the functions table driven so each board would define a >> pinmux/clock configuration table? >> >> I think its worthwhile to try and reduce duplicated code whenever >> possible. >> >> Best, >> Peter >> > > -- > Sincerely yours, > Mike. > Thanks, Tom
Tom, On 01/26/11 19:05, Tom Warren wrote: > Mike, > > On Wed, Jan 26, 2011 at 1:13 AM, Mike Rapoport <mike@compulab.co.il> wrote: >> My point was that pin muxing belongs to the board code rather than to the >> driver. Driver should just assume that pins are configured elsewhere and it does >> not need to deal with pin muxing at all. > I understand that point - sorry if I wasn't clear. No objection to > having pinmux code in board files. > >> Moreover, I'd prefer to see pinmux_board_init or something similar that >> configures all the pins at once rather than collection of pinmux_init_uart, >> pinmux_init_sdmmc, pinmux_init_gmi etc that will grow as more drivers are added. >> > I see a couple of reasons not to do it that way. First, I don't know > at this time what all the pinmux settings will be, since I haven't > ported all the periph driver code yet. It's vastly different from > what's acceptable in U-Boot, and will all need significant rewrite. > It'd take me a week to gather all that info, and I'm not at full BW on > this project (one of 4 on my plate right now). > Second, I've been chastised before for including code/features in this > initial patchset that aren't needed or used. I'm trying to keep the > code as simple as possible to make it easier on reviewers and get > through the review in as short a time as possible. This has already > dragged on far longer than I thought it would. > I'm willing to change the pinmux code to make it as generic as > possible, but only if there's a consensus on the list that it has to > be that way to get accepted & pushed. I'm Ok with pinmux_init_uart in the board code for now. I think that the generic pinmux functionality can be added afterwards. >> > Thanks, > Tom
On 01/26/11 17:58, Peter Tyser wrote: > On Wed, 2011-01-26 at 10:13 +0200, Mike Rapoport wrote: >> On 01/26/11 00:24, Peter Tyser wrote: > >> Moreover, I'd prefer to see pinmux_board_init or something similar that >> configures all the pins at once rather than collection of pinmux_init_uart, >> pinmux_init_sdmmc, pinmux_init_gmi etc that will grow as more drivers are added. > > I can see that point but its a different discussion. I don't know > enough about the Tegra2 to comment on this, but it seems like a good > idea based on previous experiences with boards with similar pinmuxing > (eg mpc8260). In my last email I mentioned a table-driven approach > (similar to the mpc8260 implementation?) which sounds somewhat like > you're proposing. Yes, I was thinking about table-driven approach, though slightly more complicated than in mpc8260 :) I think we can mostly reuse the Linux implementation of Tegra2 pinmux (1 and 2): However, the pinmuxing API can be added after the basic Tegra2 support is merged, IMO. (1) http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/mach-tegra/pinmux.c;h=f80d507671bc1b22921ab2821c8a2cde427f1b5f;hb=HEAD (2) http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/mach-tegra/pinmux-t2-tables.c;h=a6ea34e782dc339091b49e54871f953048651ad3;hb=HEAD > Best, > Peter >
Mike, On Thu, Jan 27, 2011 at 12:41 AM, Mike Rapoport <mike@compulab.co.il> wrote: > Tom, > > On 01/26/11 19:05, Tom Warren wrote: >> Mike, >> >> On Wed, Jan 26, 2011 at 1:13 AM, Mike Rapoport <mike@compulab.co.il> wrote: >>> My point was that pin muxing belongs to the board code rather than to the >>> driver. Driver should just assume that pins are configured elsewhere and it does >>> not need to deal with pin muxing at all. >> I understand that point - sorry if I wasn't clear. No objection to >> having pinmux code in board files. >> >>> Moreover, I'd prefer to see pinmux_board_init or something similar that >>> configures all the pins at once rather than collection of pinmux_init_uart, >>> pinmux_init_sdmmc, pinmux_init_gmi etc that will grow as more drivers are added. >>> >> I see a couple of reasons not to do it that way. First, I don't know >> at this time what all the pinmux settings will be, since I haven't >> ported all the periph driver code yet. It's vastly different from >> what's acceptable in U-Boot, and will all need significant rewrite. >> It'd take me a week to gather all that info, and I'm not at full BW on >> this project (one of 4 on my plate right now). >> Second, I've been chastised before for including code/features in this >> initial patchset that aren't needed or used. I'm trying to keep the >> code as simple as possible to make it easier on reviewers and get >> through the review in as short a time as possible. This has already >> dragged on far longer than I thought it would. >> I'm willing to change the pinmux code to make it as generic as >> possible, but only if there's a consensus on the list that it has to >> be that way to get accepted & pushed. > > I'm Ok with pinmux_init_uart in the board code for now. I think that the generic > pinmux functionality can be added afterwards. > Great. My main goal is to get the baseline Tegra2 support in upstream U-Boot. I'm happy to refine the pinmux approach in the next patchset. Either a generic pinmux as you propose, or a set of weak pinmux configs that can be overridden by another board design, etc. as Peter proposes. >>> >> Thanks, >> Tom > > > -- > Sincerely yours, > Mike. > Thanks, Tom
diff --git a/arch/arm/cpu/armv7/tegra2/Makefile b/arch/arm/cpu/armv7/tegra2/Makefile index f5b657b..687c887 100644 --- a/arch/arm/cpu/armv7/tegra2/Makefile +++ b/arch/arm/cpu/armv7/tegra2/Makefile @@ -28,7 +28,7 @@ include $(TOPDIR)/config.mk LIB = $(obj)lib$(SOC).o SOBJS := lowlevel_init.o -COBJS := board.o sys_info.o timer.o uart.o +COBJS := board.o sys_info.o timer.o SRCS := $(SOBJS:.o=.S) $(COBJS:.o=.c) OBJS := $(addprefix $(obj),$(COBJS) $(SOBJS)) diff --git a/arch/arm/cpu/armv7/tegra2/board.c b/arch/arm/cpu/armv7/tegra2/board.c index 816a8cd..1e92d98 100644 --- a/arch/arm/cpu/armv7/tegra2/board.c +++ b/arch/arm/cpu/armv7/tegra2/board.c @@ -25,7 +25,7 @@ #include <asm/io.h> #include <asm/arch/sys_proto.h> #include <asm/arch/tegra2.h> -#include "board.h" +#include <asm/arch/pmc.h> DECLARE_GLOBAL_DATA_PTR; diff --git a/arch/arm/cpu/armv7/tegra2/board.h b/arch/arm/cpu/armv7/tegra2/board.h deleted file mode 100644 index f8f09c0..0000000 --- a/arch/arm/cpu/armv7/tegra2/board.h +++ /dev/null @@ -1,58 +0,0 @@ -/* - * (C) Copyright 2010,2011 - * NVIDIA Corporation <www.nvidia.com> - * - * See file CREDITS for list of people who contributed to this - * project. - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU General Public License as - * published by the Free Software Foundation; either version 2 of - * the License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program; if not, write to the Free Software - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, - * MA 02111-1307 USA - */ - -#ifndef _BOARD_H_ -#define _BOARD_H_ - -#include <asm/arch/clk_rst.h> -#include <asm/arch/pinmux.h> -#include <asm/arch/pmc.h> -#include <asm/arch/uart.h> - -#define NVRM_PLLP_FIXED_FREQ_KHZ 216000 -#define NV_DEFAULT_DEBUG_BAUD 115200 - -#define PLL_BYPASS (1 << 31) -#define PLL_ENABLE (1 << 30) -#define PLL_BASE_OVRRIDE (1 << 28) -#define PLL_DIVP (1 << 20) /* post divider, b22:20 */ -#define PLL_DIVM 0x0C /* input divider, b4:0 */ - -#define SWR_UARTD_RST (1 << 2) -#define CLK_ENB_UARTD (1 << 2) -#define SWR_UARTA_RST (1 << 6) -#define CLK_ENB_UARTA (1 << 6) - -#define Z_GMC (1 << 29) -#define Z_IRRX (1 << 20) -#define Z_IRTX (1 << 19) - -enum { - UART_A = 1, - UART_B, - UART_C, - UART_D, - UART_E -}; - -#endif /* _BOARD_H_ */ diff --git a/arch/arm/cpu/armv7/tegra2/uart.c b/arch/arm/cpu/armv7/tegra2/uart.c deleted file mode 100644 index 5e60bd8..0000000 --- a/arch/arm/cpu/armv7/tegra2/uart.c +++ /dev/null @@ -1,216 +0,0 @@ -/* - * (C) Copyright 2010,2011 - * NVIDIA Corporation <www.nvidia.com> - * - * See file CREDITS for list of people who contributed to this - * project. - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU General Public License as - * published by the Free Software Foundation; either version 2 of - * the License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program; if not, write to the Free Software - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, - * MA 02111-1307 USA - */ - -#include <common.h> -#include <ns16550.h> -#include <asm/io.h> -#include <asm/arch/tegra2.h> -#include "board.h" - -/* - * Routine: uart_clock_init - * Description: init the PLL and clock for the UART in uart_num - */ -static void uart_clock_init(int uart_num) -{ - clk_rst_ctlr *const clkrst = (clk_rst_ctlr *)NV_PA_CLK_RST_BASE; - static int pllp_init_done; - u32 reg; - - if (!pllp_init_done) { - /* Override pllp setup for 216MHz operation. */ - reg = (PLL_BYPASS | PLL_BASE_OVRRIDE | PLL_DIVP); - reg |= (((NVRM_PLLP_FIXED_FREQ_KHZ/500) << 8) | PLL_DIVM); - writel(reg, &clkrst->crc_pllp_base); - - reg |= PLL_ENABLE; - writel(reg, &clkrst->crc_pllp_base); - - reg &= ~PLL_BYPASS; - writel(reg, &clkrst->crc_pllp_base); - - pllp_init_done++; - } - - /* Now do the UART reset/clock enable based on uart_num */ -#if CONFIG_TEGRA2_ENABLE_UARTA - if (uart_num == UART_A) { - /* Assert Reset to UART */ - reg = readl(&clkrst->crc_rst_dev_l); - reg |= SWR_UARTA_RST; /* SWR_UARTA_RST = 1 */ - writel(reg, &clkrst->crc_rst_dev_l); - - /* Enable clk to UART */ - reg = readl(&clkrst->crc_clk_out_enb_l); - reg |= CLK_ENB_UARTA; /* CLK_ENB_UARTA = 1 */ - writel(reg, &clkrst->crc_clk_out_enb_l); - - /* Enable pllp_out0 to UART */ - reg = readl(&clkrst->crc_clk_src_uarta); - reg &= 0x3FFFFFFF; /* UARTA_CLK_SRC = 00, PLLP_OUT0 */ - writel(reg, &clkrst->crc_clk_src_uarta); - - /* wait for 2us */ - udelay(2); - - /* De-assert reset to UART */ - reg = readl(&clkrst->crc_rst_dev_l); - reg &= ~SWR_UARTA_RST; /* SWR_UARTA_RST = 0 */ - writel(reg, &clkrst->crc_rst_dev_l); - } -#endif /* CONFIG_TEGRA2_ENABLE_UARTA */ -#if CONFIG_TEGRA2_ENABLE_UARTD - if (uart_num == UART_D) { - /* Assert Reset to UART */ - reg = readl(&clkrst->crc_rst_dev_u); - reg |= SWR_UARTD_RST; /* SWR_UARTD_RST = 1 */ - writel(reg, &clkrst->crc_rst_dev_u); - - /* Enable clk to UART */ - reg = readl(&clkrst->crc_clk_out_enb_u); - reg |= CLK_ENB_UARTD; /* CLK_ENB_UARTD = 1 */ - writel(reg, &clkrst->crc_clk_out_enb_u); - - /* Enable pllp_out0 to UART */ - reg = readl(&clkrst->crc_clk_src_uartd); - reg &= 0x3FFFFFFF; /* UARTD_CLK_SRC = 00, PLLP_OUT0 */ - writel(reg, &clkrst->crc_clk_src_uartd); - - /* wait for 2us */ - udelay(2); - - /* De-assert reset to UART */ - reg = readl(&clkrst->crc_rst_dev_u); - reg &= ~SWR_UARTD_RST; /* SWR_UARTD_RST = 0 */ - writel(reg, &clkrst->crc_rst_dev_u); - } -#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ -} - -/* - * Routine: pin_mux_uart - * Description: setup the pin muxes/tristate values for UART based on uart_num - */ -static void pin_mux_uart(int uart_num) -{ - pinmux_tri_ctlr *const pmt = (pinmux_tri_ctlr *)NV_PA_APB_MISC_BASE; - u32 reg; - -#if CONFIG_TEGRA2_ENABLE_UARTA - if (uart_num == UART_A) { - reg = readl(&pmt->pmt_ctl_c); - reg &= 0xFFF0FFFF; /* IRRX_/IRTX_SEL [19:16] = 00 UARTA */ - writel(reg, &pmt->pmt_ctl_c); - - reg = readl(&pmt->pmt_tri_a); - reg &= ~Z_IRRX; /* Z_IRRX = normal (0) */ - reg &= ~Z_IRTX; /* Z_IRTX = normal (0) */ - writel(reg, &pmt->pmt_tri_a); - } -#endif /* CONFIG_TEGRA2_ENABLE_UARTA */ -#if CONFIG_TEGRA2_ENABLE_UARTD - if (uart_num == UART_D) { - reg = readl(&pmt->pmt_ctl_b); - reg &= 0xFFFFFFF3; /* GMC_SEL [3:2] = 00, UARTD */ - writel(reg, &pmt->pmt_ctl_b); - - reg = readl(&pmt->pmt_tri_a); - reg &= ~Z_GMC; /* Z_GMC = normal (0) */ - writel(reg, &pmt->pmt_tri_a); - } -#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ -} - -static void setup_uart(uart_ctlr *u) -{ - u32 reg; - - /* Prepare the divisor value */ - reg = NVRM_PLLP_FIXED_FREQ_KHZ * 1000 / NV_DEFAULT_DEBUG_BAUD / 16; - - /* Set up UART parameters */ - writel(UART_LCR_DLAB, &u->uart_lcr); - writel(reg, &u->uart_thr_dlab_0); - writel(0, &u->uart_ier_dlab_0); - writel(0, &u->uart_lcr); /* clear DLAB */ - writel((UART_FCR_TRIGGER_3 | UART_FCR_FIFO_EN | \ - UART_FCR_CLEAR_XMIT | UART_FCR_CLEAR_RCVR), &u->uart_iir_fcr); - writel(0, &u->uart_ier_dlab_0); - writel(UART_LCR_WLS_8, &u->uart_lcr); /* 8N1 */ - writel(UART_MCR_RTS, &u->uart_mcr); - writel(0, &u->uart_msr); - writel(0, &u->uart_spr); - writel(0, &u->uart_irda_csr); - writel(0, &u->uart_asr); - writel((UART_FCR_TRIGGER_3 | UART_FCR_FIFO_EN), &u->uart_iir_fcr); - - /* Flush any old characters out of the RX FIFO */ - reg = readl(&u->uart_lsr); - - while (reg & UART_LSR_DR) { - reg = readl(&u->uart_thr_dlab_0); - reg = readl(&u->uart_lsr); - } -} - -/* - * Routine: init_uart - * Description: init the UART clocks, muxes, and baudrate/parity/etc. - */ -static void init_uart(int uart_num) -{ -#if CONFIG_TEGRA2_ENABLE_UARTA - if (uart_num == UART_A) { - uart_ctlr *const uart = (uart_ctlr *)NV_PA_APB_UARTA_BASE; - - uart_clock_init(UART_A); - - /* Enable UARTA - uses config 0 */ - pin_mux_uart(UART_A); - - setup_uart(uart); - } -#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ -#if CONFIG_TEGRA2_ENABLE_UARTD - if (uart_num == UART_D) { - uart_ctlr *const uart = (uart_ctlr *)NV_PA_APB_UARTD_BASE; - - uart_clock_init(UART_D); - - /* Enable UARTD - uses config 0 */ - pin_mux_uart(UART_D); - - setup_uart(uart); - } -#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ -} - -void uart_init(void) -{ -#if (CONFIG_TEGRA2_ENABLE_UARTA) - init_uart(UART_A); -#endif -#if (CONFIG_TEGRA2_ENABLE_UARTD) - init_uart(UART_D); -#endif -} diff --git a/common/serial.c b/common/serial.c index 051ae4e..8ebf9a5 100644 --- a/common/serial.c +++ b/common/serial.c @@ -41,7 +41,8 @@ struct serial_device *__default_serial_console (void) #elif defined(CONFIG_4xx) \ || defined(CONFIG_MB86R0x) || defined(CONFIG_MPC5xxx) \ || defined(CONFIG_MPC83xx) || defined(CONFIG_MPC85xx) \ - || defined(CONFIG_MPC86xx) || defined(CONFIG_SYS_SC520) + || defined(CONFIG_MPC86xx) || defined(CONFIG_SYS_SC520) \ + || defined(CONFIG_TEGRA2) #if defined(CONFIG_CONS_INDEX) && defined(CONFIG_SYS_NS16550_SERIAL) #if (CONFIG_CONS_INDEX==1) return &eserial1_device; diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile index 7d221fc..5a6011e 100644 --- a/drivers/serial/Makefile +++ b/drivers/serial/Makefile @@ -55,6 +55,7 @@ COBJS-$(CONFIG_S3C24X0_SERIAL) += serial_s3c24x0.o COBJS-$(CONFIG_S3C44B0_SERIAL) += serial_s3c44b0.o COBJS-$(CONFIG_XILINX_UARTLITE) += serial_xuartlite.o COBJS-$(CONFIG_SCIF_CONSOLE) += serial_sh.o +COBJS-$(CONFIG_TEGRA2) += serial_tegra2.o COBJS-$(CONFIG_USB_TTY) += usbtty.o COBJS := $(sort $(COBJS-y)) diff --git a/drivers/serial/serial_tegra2.c b/drivers/serial/serial_tegra2.c new file mode 100644 index 0000000..64dc6b4 --- /dev/null +++ b/drivers/serial/serial_tegra2.c @@ -0,0 +1,205 @@ +/* + * (C) Copyright 2010,2011 + * NVIDIA Corporation <www.nvidia.com> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <common.h> +#include <ns16550.h> +#include <asm/io.h> +#include <asm/arch/tegra2.h> +#include "serial_tegra2.h" + +/* + * Routine: uart_clock_init + * Description: init the PLL and clock for the UART + */ +static void uart_clock_init(void) +{ + clk_rst_ctlr *const clkrst = (clk_rst_ctlr *)NV_PA_CLK_RST_BASE; + static int pllp_init_done; + u32 reg; + + if (!pllp_init_done) { + /* Override pllp setup for 216MHz operation. */ + reg = (PLL_BYPASS | PLL_BASE_OVRRIDE | PLL_DIVP); + reg |= (((NVRM_PLLP_FIXED_FREQ_KHZ/500) << 8) | PLL_DIVM); + writel(reg, &clkrst->crc_pllp_base); + + reg |= PLL_ENABLE; + writel(reg, &clkrst->crc_pllp_base); + + reg &= ~PLL_BYPASS; + writel(reg, &clkrst->crc_pllp_base); + + pllp_init_done++; + } + + /* Now do the UART reset/clock enable */ +#if CONFIG_TEGRA2_ENABLE_UARTA + /* Assert Reset to UART */ + reg = readl(&clkrst->crc_rst_dev_l); + reg |= SWR_UARTA_RST; /* SWR_UARTA_RST = 1 */ + writel(reg, &clkrst->crc_rst_dev_l); + + /* Enable clk to UART */ + reg = readl(&clkrst->crc_clk_out_enb_l); + reg |= CLK_ENB_UARTA; /* CLK_ENB_UARTA = 1 */ + writel(reg, &clkrst->crc_clk_out_enb_l); + + /* Enable pllp_out0 to UART */ + reg = readl(&clkrst->crc_clk_src_uarta); + reg &= 0x3FFFFFFF; /* UARTA_CLK_SRC = 00, PLLP_OUT0 */ + writel(reg, &clkrst->crc_clk_src_uarta); + + /* wait for 2us */ + udelay(2); + + /* De-assert reset to UART */ + reg = readl(&clkrst->crc_rst_dev_l); + reg &= ~SWR_UARTA_RST; /* SWR_UARTA_RST = 0 */ + writel(reg, &clkrst->crc_rst_dev_l); +#endif /* CONFIG_TEGRA2_ENABLE_UARTA */ +#if CONFIG_TEGRA2_ENABLE_UARTD + /* Assert Reset to UART */ + reg = readl(&clkrst->crc_rst_dev_u); + reg |= SWR_UARTD_RST; /* SWR_UARTD_RST = 1 */ + writel(reg, &clkrst->crc_rst_dev_u); + + /* Enable clk to UART */ + reg = readl(&clkrst->crc_clk_out_enb_u); + reg |= CLK_ENB_UARTD; /* CLK_ENB_UARTD = 1 */ + writel(reg, &clkrst->crc_clk_out_enb_u); + + /* Enable pllp_out0 to UART */ + reg = readl(&clkrst->crc_clk_src_uartd); + reg &= 0x3FFFFFFF; /* UARTD_CLK_SRC = 00, PLLP_OUT0 */ + writel(reg, &clkrst->crc_clk_src_uartd); + + /* wait for 2us */ + udelay(2); + + /* De-assert reset to UART */ + reg = readl(&clkrst->crc_rst_dev_u); + reg &= ~SWR_UARTD_RST; /* SWR_UARTD_RST = 0 */ + writel(reg, &clkrst->crc_rst_dev_u); +#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ +} + +/* + * Routine: pin_mux_uart + * Description: setup the pin muxes/tristate values for a UART + */ +static void pin_mux_uart(void) +{ + pinmux_tri_ctlr *const pmt = (pinmux_tri_ctlr *)NV_PA_APB_MISC_BASE; + u32 reg; + +#if CONFIG_TEGRA2_ENABLE_UARTA + reg = readl(&pmt->pmt_ctl_c); + reg &= 0xFFF0FFFF; /* IRRX_/IRTX_SEL [19:16] = 00 UARTA */ + writel(reg, &pmt->pmt_ctl_c); + + reg = readl(&pmt->pmt_tri_a); + reg &= ~Z_IRRX; /* Z_IRRX = normal (0) */ + reg &= ~Z_IRTX; /* Z_IRTX = normal (0) */ + writel(reg, &pmt->pmt_tri_a); +#endif /* CONFIG_TEGRA2_ENABLE_UARTA */ +#if CONFIG_TEGRA2_ENABLE_UARTD + reg = readl(&pmt->pmt_ctl_b); + reg &= 0xFFFFFFF3; /* GMC_SEL [3:2] = 00, UARTD */ + writel(reg, &pmt->pmt_ctl_b); + + reg = readl(&pmt->pmt_tri_a); + reg &= ~Z_GMC; /* Z_GMC = normal (0) */ + writel(reg, &pmt->pmt_tri_a); +#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ +} + +static void setup_uart(uart_ctlr *u) +{ + u32 reg; + + /* Prepare the divisor value */ + reg = NVRM_PLLP_FIXED_FREQ_KHZ * 1000 / NV_DEFAULT_DEBUG_BAUD / 16; + + /* Set up UART parameters */ + writel(UART_LCR_DLAB, &u->uart_lcr); + writel(reg, &u->uart_thr_dlab_0); + writel(0, &u->uart_ier_dlab_0); + writel(0, &u->uart_lcr); /* clear DLAB */ + writel((UART_FCR_TRIGGER_3 | UART_FCR_FIFO_EN | \ + UART_FCR_CLEAR_XMIT | UART_FCR_CLEAR_RCVR), &u->uart_iir_fcr); + writel(0, &u->uart_ier_dlab_0); + writel(UART_LCR_WLS_8, &u->uart_lcr); /* 8N1 */ + writel(UART_MCR_RTS, &u->uart_mcr); + writel(0, &u->uart_msr); + writel(0, &u->uart_spr); + writel(0, &u->uart_irda_csr); + writel(0, &u->uart_asr); + writel((UART_FCR_TRIGGER_3 | UART_FCR_FIFO_EN), &u->uart_iir_fcr); + + /* Flush any old characters out of the RX FIFO */ + reg = readl(&u->uart_lsr); + + while (reg & UART_LSR_DR) { + reg = readl(&u->uart_thr_dlab_0); + reg = readl(&u->uart_lsr); + } +} + +/* + * Routine: init_uart + * Description: init the UART clocks, muxes, and baudrate/parity/etc. + */ +static void init_uart(void) +{ +#if CONFIG_TEGRA2_ENABLE_UARTA + uart_ctlr *const uart = (uart_ctlr *)NV_PA_APB_UARTA_BASE; + + uart_clock_init(); + + /* Enable UARTA - uses config 0 */ + pin_mux_uart(); + + setup_uart(uart); +#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ +#if CONFIG_TEGRA2_ENABLE_UARTD + uart_ctlr *const uart = (uart_ctlr *)NV_PA_APB_UARTD_BASE; + + uart_clock_init(); + + /* Enable UARTD - uses config 0 */ + pin_mux_uart(); + + setup_uart(uart); +#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ +} + +void uart_init(void) +{ + /* Init each UART - there may be more than 1 on a board/build */ +#if (CONFIG_TEGRA2_ENABLE_UARTA) + init_uart(); +#endif +#if (CONFIG_TEGRA2_ENABLE_UARTD) + init_uart(); +#endif +} diff --git a/drivers/serial/serial_tegra2.h b/drivers/serial/serial_tegra2.h new file mode 100644 index 0000000..a8ead9f --- /dev/null +++ b/drivers/serial/serial_tegra2.h @@ -0,0 +1,49 @@ +/* + * (C) Copyright 2010,2011 + * NVIDIA Corporation <www.nvidia.com> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#ifndef _SERIAL_TEGRA_H_ +#define _SERIAL_TEGRA_H_ + +#include <asm/arch/clk_rst.h> +#include <asm/arch/pinmux.h> +#include <asm/arch/uart.h> + +#define NVRM_PLLP_FIXED_FREQ_KHZ 216000 +#define NV_DEFAULT_DEBUG_BAUD 115200 + +#define PLL_BYPASS (1 << 31) +#define PLL_ENABLE (1 << 30) +#define PLL_BASE_OVRRIDE (1 << 28) +#define PLL_DIVP (1 << 20) /* post divider, b22:20 */ +#define PLL_DIVM 0x0C /* input divider, b4:0 */ + +#define SWR_UARTD_RST (1 << 2) +#define CLK_ENB_UARTD (1 << 2) +#define SWR_UARTA_RST (1 << 6) +#define CLK_ENB_UARTA (1 << 6) + +#define Z_GMC (1 << 29) +#define Z_IRRX (1 << 20) +#define Z_IRTX (1 << 19) + +#endif /* _SERIAL_TEGRA_H_ */ diff --git a/include/serial.h b/include/serial.h index 15ab73c..f21d961 100644 --- a/include/serial.h +++ b/include/serial.h @@ -27,7 +27,8 @@ extern struct serial_device * default_serial_console (void); defined(CONFIG_405EP) || defined(CONFIG_405EZ) || defined(CONFIG_405EX) || \ defined(CONFIG_MB86R0x) || defined(CONFIG_MPC5xxx) || \ defined(CONFIG_MPC83xx) || defined(CONFIG_MPC85xx) || \ - defined(CONFIG_MPC86xx) || defined(CONFIG_SYS_SC520) + defined(CONFIG_MPC86xx) || defined(CONFIG_SYS_SC520) || \ + defined(CONFIG_TEGRA2) extern struct serial_device serial0_device; extern struct serial_device serial1_device; #if defined(CONFIG_SYS_NS16550_SERIAL)
Signed-off-by: Tom Warren <twarren@nvidia.com> --- Changes for V2: - Move serial driver to separate patch Changes for V5: - Move arch/arm/cpu/armv7/uart.c & board.h to drivers/serial and rename to serial_tegra2.c - Remove use of uart_num & UART_A/D in serial_tegra2, simplify code arch/arm/cpu/armv7/tegra2/Makefile | 2 +- arch/arm/cpu/armv7/tegra2/board.c | 2 +- arch/arm/cpu/armv7/tegra2/board.h | 58 ---------- arch/arm/cpu/armv7/tegra2/uart.c | 216 ------------------------------------ common/serial.c | 3 +- drivers/serial/Makefile | 1 + drivers/serial/serial_tegra2.c | 205 ++++++++++++++++++++++++++++++++++ drivers/serial/serial_tegra2.h | 49 ++++++++ include/serial.h | 3 +- 9 files changed, 261 insertions(+), 278 deletions(-) delete mode 100644 arch/arm/cpu/armv7/tegra2/board.h delete mode 100644 arch/arm/cpu/armv7/tegra2/uart.c create mode 100644 drivers/serial/serial_tegra2.c create mode 100644 drivers/serial/serial_tegra2.h