| Message ID | 1313190008-7551-3-git-send-email-davidb@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday 12 August 2011 16:00:06 David Brown wrote: > +Required properties: > +- compatible : > + - "qcom,msm-uart" > +- reg : offset and length of the register set for the device > + for the hsuart operating in compatible mode, there should be a > + second pair describing the gsbi registers. > +- interrupts : should contain the uart interrupt. > + > +Example: > + > + uart@19c400000 { > + compatible = "qcom,msm-hsuart", "qcom,msm-uart"; > + reg = <0x19c40000 0x1000">, > + <0x19c00000 0x1000">; > + interrupts = <195>; > + }; > @@ -920,11 +928,17 @@ static int __devexit msm_serial_remove(struct platform_device *pdev) > return 0; > } > > +static struct of_device_id msm_match_table[] = { > + { .compatible = "qcom,msm-hsuart-lite" }, > + {} > +}; > + Hi David, It looks like you changed the value for the "compatible" property in the process of making the patch, but did not update all places. Should it be qcom,msm-hsuart-lite or qcom,msm-hsuart? Arnd
On Sat, Aug 13, 2011 at 10:29:00AM +0200, Arnd Bergmann wrote: > On Friday 12 August 2011 16:00:06 David Brown wrote: > > +Required properties: > > +- compatible : > > + - "qcom,msm-uart" > > +- reg : offset and length of the register set for the device > > + for the hsuart operating in compatible mode, there should be a > > + second pair describing the gsbi registers. > > +- interrupts : should contain the uart interrupt. > > + > > +Example: > > + > > + uart@19c400000 { > > + compatible = "qcom,msm-hsuart", "qcom,msm-uart"; > > + reg = <0x19c40000 0x1000">, > > + <0x19c00000 0x1000">; > > + interrupts = <195>; > > + }; > > > > > @@ -920,11 +928,17 @@ static int __devexit msm_serial_remove(struct platform_device *pdev) > > return 0; > > } > > > > +static struct of_device_id msm_match_table[] = { > > + { .compatible = "qcom,msm-hsuart-lite" }, > > + {} > > +}; > > + > > Hi David, > > It looks like you changed the value for the "compatible" property in the > process of making the patch, but did not update all places. > > Should it be qcom,msm-hsuart-lite or qcom,msm-hsuart? I guess I got the documentation to be different than the code. Sadily, I think the documentation is probably what I want instead of the code. I'm not sure actually what is best to use here. I'm thinking that the 'lite' identifier should perhaps go away. MSM's have two UARTS on them, an older "simple" PIO type of UART, and a newer one that can do DMA (called the hsuart for high-speed). The hsuart can also be used in a non-DMA driver in a mostly compatible way with the old UART. For non-high-speed applications, the user will probably just want to use the non-DMA driver. My question is then: if the device tree describes it as compatible = "qcom,msm-hsuart", "qcom,msm-uart"; and one driver matches qcom,msm-hsuart and another matches qcom,msm-uart, which driver will get used. Ideally, it would use the earliest one in the list. If that's the case, I'll get rid of the -lite suffix and just make the non-DMA driver compatible with the plain "qcom,msm-uart". David
On Saturday 13 August 2011 12:46:45 David Brown wrote: > > I'm not sure actually what is best to use here. I'm thinking that the > 'lite' identifier should perhaps go away. MSM's have two UARTS on > them, an older "simple" PIO type of UART, and a newer one that can do > DMA (called the hsuart for high-speed). The hsuart can also be used > in a non-DMA driver in a mostly compatible way with the old UART. > > For non-high-speed applications, the user will probably just want to > use the non-DMA driver. My question is then: if the device tree > describes it as > > compatible = "qcom,msm-hsuart", "qcom,msm-uart"; > > and one driver matches qcom,msm-hsuart and another matches > qcom,msm-uart, which driver will get used. Ideally, it would use the > earliest one in the list. > > If that's the case, I'll get rid of the -lite suffix and just make the > non-DMA driver compatible with the plain "qcom,msm-uart". I believe that unfortunately the answer is that the first driver that matches anything will get used. There are two possible ways that I can see to make it do what you want anyway: 1. In the probe function for the slow driver, you return an error when the device you get passed matches "qcom,msm-hsuart", possibly dependent on whether the other driver also got built. 2. You register one platform driver that handles both names and gives the device to just one of the two drivers. This would probably require linking the two drivers into the same module, or having the non-DMA speed driver just act as a library. Arnd
On Sat, Aug 13, 2011 at 11:34:15PM +0200, Arnd Bergmann wrote: > On Saturday 13 August 2011 12:46:45 David Brown wrote: > > > > I'm not sure actually what is best to use here. I'm thinking that the > > 'lite' identifier should perhaps go away. MSM's have two UARTS on > > them, an older "simple" PIO type of UART, and a newer one that can do > > DMA (called the hsuart for high-speed). The hsuart can also be used > > in a non-DMA driver in a mostly compatible way with the old UART. > > > > For non-high-speed applications, the user will probably just want to > > use the non-DMA driver. My question is then: if the device tree > > describes it as > > > > compatible = "qcom,msm-hsuart", "qcom,msm-uart"; > > > > and one driver matches qcom,msm-hsuart and another matches > > qcom,msm-uart, which driver will get used. Ideally, it would use the > > earliest one in the list. > > > > If that's the case, I'll get rid of the -lite suffix and just make the > > non-DMA driver compatible with the plain "qcom,msm-uart". > > I believe that unfortunately the answer is that the first driver that > matches anything will get used. There are two possible ways that I can > see to make it do what you want anyway: > > 1. In the probe function for the slow driver, you return an error > when the device you get passed matches "qcom,msm-hsuart", possibly > dependent on whether the other driver also got built. > > 2. You register one platform driver that handles both names and > gives the device to just one of the two drivers. This would probably > require linking the two drivers into the same module, or having > the non-DMA speed driver just act as a library. How about if I just keep it simple for now. Since there isn't actually a driver for the DMA version, this driver will handle both UART blocks, so I'll just do the plain thing in the DT. In the future, when a DMA-capable driver exists, we can figure out how to determine which driver should be used. At this point, I'm not even sure what the correct answer will be, since a given configuration may want to use non-DMA for one msm-hsuart device, and the DMA driver for another. It's kind of board/use specific, but beyond just describing what the hardware is. I've just sent new patches with this fixed up. David
On Tuesday 16 August 2011 10:57:02 David Brown wrote: > How about if I just keep it simple for now. Since there isn't > actually a driver for the DMA version, this driver will handle both > UART blocks, so I'll just do the plain thing in the DT. Sounds good to me. > In the future, when a DMA-capable driver exists, we can figure out how > to determine which driver should be used. At this point, I'm not even > sure what the correct answer will be, since a given configuration may > want to use non-DMA for one msm-hsuart device, and the DMA driver for > another. It's kind of board/use specific, but beyond just describing > what the hardware is. In order to be absolutely future-proof, you could mandate that you always list two "compatible" values, one for the generic version and one for the specific implementation (high-speed or low-speed). It's a simple change from what you have now and it allows to change the drivers to bind to just the specific name in case you want to handle them separately in the future, without having to change the device tree files. Arnd
diff --git a/Documentation/devicetree/bindings/tty/serial/msm_serial.txt b/Documentation/devicetree/bindings/tty/serial/msm_serial.txt new file mode 100644 index 0000000..071459c --- /dev/null +++ b/Documentation/devicetree/bindings/tty/serial/msm_serial.txt @@ -0,0 +1,18 @@ +* Qualcomm MSM UART + +Required properties: +- compatible : + - "qcom,msm-uart" +- reg : offset and length of the register set for the device + for the hsuart operating in compatible mode, there should be a + second pair describing the gsbi registers. +- interrupts : should contain the uart interrupt. + +Example: + + uart@19c400000 { + compatible = "qcom,msm-hsuart", "qcom,msm-uart"; + reg = <0x19c40000 0x1000">, + <0x19c00000 0x1000">; + interrupts = <195>; + }; diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c index d9863b2..d5a6db6 100644 --- a/drivers/tty/serial/msm_serial.c +++ b/drivers/tty/serial/msm_serial.c @@ -19,6 +19,7 @@ # define SUPPORT_SYSRQ #endif +#include <linux/atomic.h> #include <linux/hrtimer.h> #include <linux/module.h> #include <linux/io.h> @@ -33,6 +34,8 @@ #include <linux/clk.h> #include <linux/platform_device.h> #include <linux/delay.h> +#include <linux/of.h> +#include <linux/of_device.h> #include "msm_serial.h" @@ -856,6 +859,8 @@ static struct uart_driver msm_uart_driver = { .cons = MSM_CONSOLE, }; +static atomic_t msm_uart_next_id = ATOMIC_INIT(0); + static int __init msm_serial_probe(struct platform_device *pdev) { struct msm_port *msm_port; @@ -863,6 +868,9 @@ static int __init msm_serial_probe(struct platform_device *pdev) struct uart_port *port; int irq; + if (pdev->id == -1) + pdev->id = atomic_inc_return(&msm_uart_next_id) - 1; + if (unlikely(pdev->id < 0 || pdev->id >= UART_NR)) return -ENXIO; @@ -920,11 +928,17 @@ static int __devexit msm_serial_remove(struct platform_device *pdev) return 0; } +static struct of_device_id msm_match_table[] = { + { .compatible = "qcom,msm-hsuart-lite" }, + {} +}; + static struct platform_driver msm_platform_driver = { .remove = msm_serial_remove, .driver = { .name = "msm_serial", .owner = THIS_MODULE, + .of_match_table = msm_match_table, }, };
Add devicetree support to the msm_serial driver. Clocks are still queried by direct name from the driver until device tree clock support is implemented. Signed-off-by: David Brown <davidb@codeaurora.org> --- .../devicetree/bindings/tty/serial/msm_serial.txt | 18 ++++++++++++++++++ drivers/tty/serial/msm_serial.c | 14 ++++++++++++++ 2 files changed, 32 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/tty/serial/msm_serial.txt