| Message ID | 20180605060125.9518-4-nm@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 05, 2018 at 01:01:22AM -0500, Nishanth Menon wrote: > AM654 uses a UART controller that is compatible (partially) with > existing 8250 UART, however, has a few differences with respect to DMA > support and control paths. Introduce a base definition that allows us > to build up the differences in follow on patches. > > Cc: Sekhar Nori <nsekhar@ti.com> > Cc: Vignesh R <vigneshr@ti.com> > Signed-off-by: Nishanth Menon <nm@ti.com> > --- > Documentation/devicetree/bindings/serial/omap_serial.txt | 1 + > drivers/tty/serial/8250/8250_omap.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/serial/omap_serial.txt b/Documentation/devicetree/bindings/serial/omap_serial.txt > index 4b0f05adb228..c35d5ece1156 100644 > --- a/Documentation/devicetree/bindings/serial/omap_serial.txt > +++ b/Documentation/devicetree/bindings/serial/omap_serial.txt > @@ -1,6 +1,7 @@ > OMAP UART controller > > Required properties: > +- compatible : should be "ti,am654-uart" for AM654 controllers Not compatible with any existing TI 8250 UARTs? > - compatible : should be "ti,omap2-uart" for OMAP2 controllers > - compatible : should be "ti,omap3-uart" for OMAP3 controllers > - compatible : should be "ti,omap4-uart" for OMAP4 controllers > diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c > index 1b337fee07ed..a019286f8bb6 100644 > --- a/drivers/tty/serial/8250/8250_omap.c > +++ b/drivers/tty/serial/8250/8250_omap.c > @@ -1115,6 +1115,7 @@ static const u8 am3352_habit = OMAP_DMA_TX_KICK | UART_ERRATA_CLOCK_DISABLE; > static const u8 dra742_habit = UART_ERRATA_CLOCK_DISABLE; > > static const struct of_device_id omap8250_dt_ids[] = { > + { .compatible = "ti,am654-uart" }, > { .compatible = "ti,omap2-uart" }, > { .compatible = "ti,omap3-uart" }, > { .compatible = "ti,omap4-uart", .data = &omap4_habit, }, > -- > 2.15.1 >
On 21:06-20180612, Rob Herring wrote: > > diff --git a/Documentation/devicetree/bindings/serial/omap_serial.txt b/Documentation/devicetree/bindings/serial/omap_serial.txt > > index 4b0f05adb228..c35d5ece1156 100644 > > --- a/Documentation/devicetree/bindings/serial/omap_serial.txt > > +++ b/Documentation/devicetree/bindings/serial/omap_serial.txt > > @@ -1,6 +1,7 @@ > > OMAP UART controller > > > > Required properties: > > +- compatible : should be "ti,am654-uart" for AM654 controllers > > Not compatible with any existing TI 8250 UARTs? Base is compatible, however there are differences in DMA operation and few additional bits. omap4-uart is sufficient for a basic PIO mode of operation given initialization a bootloader might do for base console. I will split the bindings off into it's own patch to keep the confusion to a minimum and allowing serial driver change to go in independently.
Hi Rob, On Wednesday 13 June 2018 02:36 AM, Rob Herring wrote: > On Tue, Jun 05, 2018 at 01:01:22AM -0500, Nishanth Menon wrote: >> AM654 uses a UART controller that is compatible (partially) with >> existing 8250 UART, however, has a few differences with respect to DMA >> support and control paths. Introduce a base definition that allows us >> to build up the differences in follow on patches. >> >> Cc: Sekhar Nori <nsekhar@ti.com> >> Cc: Vignesh R <vigneshr@ti.com> >> Signed-off-by: Nishanth Menon <nm@ti.com> >> --- >> Documentation/devicetree/bindings/serial/omap_serial.txt | 1 + >> drivers/tty/serial/8250/8250_omap.c | 1 + >> 2 files changed, 2 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/serial/omap_serial.txt b/Documentation/devicetree/bindings/serial/omap_serial.txt >> index 4b0f05adb228..c35d5ece1156 100644 >> --- a/Documentation/devicetree/bindings/serial/omap_serial.txt >> +++ b/Documentation/devicetree/bindings/serial/omap_serial.txt >> @@ -1,6 +1,7 @@ >> OMAP UART controller >> >> Required properties: >> +- compatible : should be "ti,am654-uart" for AM654 controllers > > Not compatible with any existing TI 8250 UARTs? Curious on why you asked about this. Are you suggesting why not: "ti,<new-soc>-uart", "ti,<old-soc>-uart" or you are asking why introduce "ti,<new-soc>-uart" unless there is clear demonstrable need for using it in driver code. In general, I think "ti,<new-soc>-uart", "ti,<old-soc>-uart" in device-tree (and by extension in binding document) is better even in there are no _known_ incompatibilities at the time of initial driver submission. The reason is silicon integration and process differences many times spill over into driver. Of course, the idea is not to go postal and create a new compatible for every pin-compatible part number that gets created, but probably a new compatible should be created for a new silicon die. We have just started introducing support for this SoC, and since it reuses many IPs, this question is likely to come up again. In this particular case though, Nishanth is perfectly right in not saying compatible : should be "ti,am654-uart", "ti,omap4-uart" Because we *know* UART DMA integration is different and a match against omap4 would result in non-working UART once DMA is enabled by default. Thanks, Sekhar
On Fri, Jun 15, 2018 at 11:17 AM, Sekhar Nori <nsekhar@ti.com> wrote: > Hi Rob, > > On Wednesday 13 June 2018 02:36 AM, Rob Herring wrote: >> On Tue, Jun 05, 2018 at 01:01:22AM -0500, Nishanth Menon wrote: >>> AM654 uses a UART controller that is compatible (partially) with >>> existing 8250 UART, however, has a few differences with respect to DMA >>> support and control paths. Introduce a base definition that allows us >>> to build up the differences in follow on patches. >>> >>> Cc: Sekhar Nori <nsekhar@ti.com> >>> Cc: Vignesh R <vigneshr@ti.com> >>> Signed-off-by: Nishanth Menon <nm@ti.com> >>> --- >>> Documentation/devicetree/bindings/serial/omap_serial.txt | 1 + >>> drivers/tty/serial/8250/8250_omap.c | 1 + >>> 2 files changed, 2 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/serial/omap_serial.txt b/Documentation/devicetree/bindings/serial/omap_serial.txt >>> index 4b0f05adb228..c35d5ece1156 100644 >>> --- a/Documentation/devicetree/bindings/serial/omap_serial.txt >>> +++ b/Documentation/devicetree/bindings/serial/omap_serial.txt >>> @@ -1,6 +1,7 @@ >>> OMAP UART controller >>> >>> Required properties: >>> +- compatible : should be "ti,am654-uart" for AM654 controllers >> >> Not compatible with any existing TI 8250 UARTs? > > Curious on why you asked about this. Are you suggesting why not: > > "ti,<new-soc>-uart", "ti,<old-soc>-uart" Correct. > or you are asking why introduce "ti,<new-soc>-uart" unless there is > clear demonstrable need for using it in driver code. > > In general, I think "ti,<new-soc>-uart", "ti,<old-soc>-uart" in > device-tree (and by extension in binding document) is better even in > there are no _known_ incompatibilities at the time of initial driver > submission. The reason is silicon integration and process differences > many times spill over into driver. Yes, and chip designers can't be trusted. ;) > Of course, the idea is not to go postal and create a new compatible for > every pin-compatible part number that gets created, but probably a new > compatible should be created for a new silicon die. Yes, that's the criteria I would use too. That's sometimes hard if it's not the chip vendor doing the DT bindings. > We have just started introducing support for this SoC, and since it > reuses many IPs, this question is likely to come up again. > > In this particular case though, Nishanth is perfectly right in not saying > > compatible : should be "ti,am654-uart", "ti,omap4-uart" > > Because we *know* UART DMA integration is different and a match against > omap4 would result in non-working UART once DMA is enabled by default. Okay, makes sense. I'd suggest rewording the commit message to include this. The "compatible to 8250 except for DMA" part I would have applied to all TI UARTs rather than DMA differences with other TI UARTs. Rob
diff --git a/Documentation/devicetree/bindings/serial/omap_serial.txt b/Documentation/devicetree/bindings/serial/omap_serial.txt index 4b0f05adb228..c35d5ece1156 100644 --- a/Documentation/devicetree/bindings/serial/omap_serial.txt +++ b/Documentation/devicetree/bindings/serial/omap_serial.txt @@ -1,6 +1,7 @@ OMAP UART controller Required properties: +- compatible : should be "ti,am654-uart" for AM654 controllers - compatible : should be "ti,omap2-uart" for OMAP2 controllers - compatible : should be "ti,omap3-uart" for OMAP3 controllers - compatible : should be "ti,omap4-uart" for OMAP4 controllers diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c index 1b337fee07ed..a019286f8bb6 100644 --- a/drivers/tty/serial/8250/8250_omap.c +++ b/drivers/tty/serial/8250/8250_omap.c @@ -1115,6 +1115,7 @@ static const u8 am3352_habit = OMAP_DMA_TX_KICK | UART_ERRATA_CLOCK_DISABLE; static const u8 dra742_habit = UART_ERRATA_CLOCK_DISABLE; static const struct of_device_id omap8250_dt_ids[] = { + { .compatible = "ti,am654-uart" }, { .compatible = "ti,omap2-uart" }, { .compatible = "ti,omap3-uart" }, { .compatible = "ti,omap4-uart", .data = &omap4_habit, },
AM654 uses a UART controller that is compatible (partially) with existing 8250 UART, however, has a few differences with respect to DMA support and control paths. Introduce a base definition that allows us to build up the differences in follow on patches. Cc: Sekhar Nori <nsekhar@ti.com> Cc: Vignesh R <vigneshr@ti.com> Signed-off-by: Nishanth Menon <nm@ti.com> --- Documentation/devicetree/bindings/serial/omap_serial.txt | 1 + drivers/tty/serial/8250/8250_omap.c | 1 + 2 files changed, 2 insertions(+)