We rely on probe order of this driver to determine the line number for
the uart port. This makes it impossible to know the line number
when these devices are populated via DT. Use the DT alias
mechanism to assign the line based on the aliases node.
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/tty/serial/msm_serial.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index 0da0b5474e98..4aba62d00a3f 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -1005,17 +1005,22 @@ static int msm_serial_probe(struct platform_device *pdev)
struct resource *resource;
struct uart_port *port;
const struct of_device_id *id;
- int irq;
+ int irq, line;
if (pdev->id == -1)
pdev->id = atomic_inc_return(&msm_uart_next_id) - 1;
- if (unlikely(pdev->id < 0 || pdev->id >= UART_NR))
+ if (pdev->dev.of_node)
+ line = of_alias_get_id(pdev->dev.of_node, "serial");
+ else
+ line = pdev->id;
+
+ if (unlikely(line < 0 || line >= UART_NR))
return -ENXIO;
- printk(KERN_INFO "msm_serial: detected port #%d\n", pdev->id);
+ printk(KERN_INFO "msm_serial: detected port #%d\n", line);
- port = get_port_from_line(pdev->id);
+ port = get_port_from_line(line);
port->dev = &pdev->dev;
msm_port = UART_TO_MSM(port);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
From: Frank Rowand <[email protected]>
Update devicetree binding for msm_serial to reflect msm_serial_probe()
getting line id from the serial alias.
Signed-off-by: Frank Rowand <[email protected]>
---
Documentation/devicetree/bindings/serial/qcom,msm-uartdm.txt | 15 +++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
Index: b/Documentation/devicetree/bindings/serial/qcom,msm-uartdm.txt
===================================================================
--- a/Documentation/devicetree/bindings/serial/qcom,msm-uartdm.txt
+++ b/Documentation/devicetree/bindings/serial/qcom,msm-uartdm.txt
@@ -27,11 +27,18 @@ Optional properties:
- dmas: Should contain dma specifiers for transmit and receive channels
- dma-names: Should contain "tx" for transmit and "rx" for receive channels
+Additional requirements:
+- Each UART port must have an alias correctly numbered in "aliases" node.
+
Examples:
A uartdm v1.4 device with dma capabilities.
-serial@f991e000 {
+aliases {
+ serial0 = &serial0;
+};
+
+serial0: serial@f991e000 {
compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
reg = <0xf991e000 0x1000>;
interrupts = <0 108 0x0>;
@@ -43,7 +50,11 @@ serial@f991e000 {
A uartdm v1.3 device without dma capabilities and part of a GSBI complex.
-serial@19c40000 {
+aliases {
+ serial0 = &serial0;
+};
+
+serial0: serial@19c40000 {
compatible = "qcom,msm-uartdm-v1.3", "qcom,msm-uartdm";
reg = <0x19c40000 0x1000>,
<0x19c00000 0x1000>;
From: Frank Rowand <[email protected]>
Update msm8974 dtsi for msm_serial to reflect msm_serial_probe()
getting line id from the serial alias.
Signed-off-by: Frank Rowand <[email protected]>
---
arch/arm/boot/dts/qcom-msm8974.dtsi | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
Index: b/arch/arm/boot/dts/qcom-msm8974.dtsi
===================================================================
--- a/arch/arm/boot/dts/qcom-msm8974.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
@@ -9,6 +9,10 @@
compatible = "qcom,msm8974";
interrupt-parent = <&intc>;
+ aliases {
+ serial0 = &serial0;
+ };
+
cpus {
#address-cells = <1>;
#size-cells = <0>;
@@ -189,7 +193,7 @@
reg = <0xfd8c0000 0x6000>;
};
- serial@f991e000 {
+ serial0: serial@f991e000 {
compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
reg = <0xf991e000 0x1000>;
interrupts = <0 108 0x0>;
On 11/6/2014 10:40 PM, Frank Rowand wrote:
> From: Frank Rowand <[email protected]>
>
> Update msm8974 dtsi for msm_serial to reflect msm_serial_probe()
> getting line id from the serial alias.
>
> Signed-off-by: Frank Rowand <[email protected]>
> ---
> arch/arm/boot/dts/qcom-msm8974.dtsi | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> Index: b/arch/arm/boot/dts/qcom-msm8974.dtsi
> ===================================================================
> --- a/arch/arm/boot/dts/qcom-msm8974.dtsi
> +++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
> @@ -9,6 +9,10 @@
> compatible = "qcom,msm8974";
> interrupt-parent = <&intc>;
>
> + aliases {
> + serial0 = &serial0;
> + };
> +
> cpus {
> #address-cells = <1>;
> #size-cells = <0>;
> @@ -189,7 +193,7 @@
> reg = <0xfd8c0000 0x6000>;
> };
>
> - serial@f991e000 {
> + serial0: serial@f991e000 {
> compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
> reg = <0xf991e000 0x1000>;
> interrupts = <0 108 0x0>;
>
This same change is also needed in:
qcom-ipq8064.dtsi
qcom-msm8960.dtsi
qcom-apq8084.dtsi
qcom-apq8064.dtsi
qcom-msm8660.dtsi
but I did not want to just blindly apply those changes without testing.
-Frank
On Thursday 06 November 2014 22:42:47 Frank Rowand wrote:
> This same change is also needed in:
>
> qcom-ipq8064.dtsi
> qcom-msm8960.dtsi
> qcom-apq8084.dtsi
> qcom-apq8064.dtsi
> qcom-msm8660.dtsi
>
> but I did not want to just blindly apply those changes without testing.
>
Is there only one uart on each of these?
If not, it would be better to put the aliases in the board specific file,
pointing to whichever ports are in use, in the order that makes sense
for that board.
Arnd
On 11/7/2014 1:47 AM, Arnd Bergmann wrote:
> On Thursday 06 November 2014 22:42:47 Frank Rowand wrote:
>> This same change is also needed in:
>>
>> qcom-ipq8064.dtsi
>> qcom-msm8960.dtsi
>> qcom-apq8084.dtsi
>> qcom-apq8064.dtsi
>> qcom-msm8660.dtsi
>>
>> but I did not want to just blindly apply those changes without testing.
>>
>
> Is there only one uart on each of these?
>
> If not, it would be better to put the aliases in the board specific file,
> pointing to whichever ports are in use, in the order that makes sense
> for that board.
Good point, thanks for bringing it up.
Your comment made me verify that the board dts files can override the
aliases from the included .dtsi. So not a problem to have a default
set of aliases in the .dtsi files.
-Frank
On Friday 07 November 2014 13:35:45 Frank Rowand wrote:
> On 11/7/2014 1:47 AM, Arnd Bergmann wrote:
> > On Thursday 06 November 2014 22:42:47 Frank Rowand wrote:
> >> This same change is also needed in:
> >>
> >> qcom-ipq8064.dtsi
> >> qcom-msm8960.dtsi
> >> qcom-apq8084.dtsi
> >> qcom-apq8064.dtsi
> >> qcom-msm8660.dtsi
> >>
> >> but I did not want to just blindly apply those changes without testing.
> >>
> >
> > Is there only one uart on each of these?
> >
> > If not, it would be better to put the aliases in the board specific file,
> > pointing to whichever ports are in use, in the order that makes sense
> > for that board.
>
> Good point, thanks for bringing it up.
>
> Your comment made me verify that the board dts files can override the
> aliases from the included .dtsi. So not a problem to have a default
> set of aliases in the .dtsi files.
I would think it's better to keep them in the per-board file out of
principle though.
Arnd
On Wed, Oct 22, 2014 at 5:33 PM, Stephen Boyd <[email protected]> wrote:
> We rely on probe order of this driver to determine the line number for
> the uart port. This makes it impossible to know the line number
> when these devices are populated via DT. Use the DT alias
> mechanism to assign the line based on the aliases node.
>
> Signed-off-by: Stephen Boyd <[email protected]>
FYI... this patch hit linux-next and caused multiple boot failures on
qcom platforms[1] as of next-20141110. I'm assuming this is because
the corresponding DTS changes have not hit linux-next yet.
Kevin
[1] http://status.armcloud.us/boot/?qcom
On 11/10/2014 10:54 AM, Kevin Hilman wrote:
> On Wed, Oct 22, 2014 at 5:33 PM, Stephen Boyd <[email protected]> wrote:
>> We rely on probe order of this driver to determine the line number for
>> the uart port. This makes it impossible to know the line number
>> when these devices are populated via DT. Use the DT alias
>> mechanism to assign the line based on the aliases node.
>>
>> Signed-off-by: Stephen Boyd <[email protected]>
> FYI... this patch hit linux-next and caused multiple boot failures on
> qcom platforms[1] as of next-20141110. I'm assuming this is because
> the corresponding DTS changes have not hit linux-next yet.
>
> Kevin
>
> [1] http://status.armcloud.us/boot/?qcom
Hmm the intention was to make it optional so that dts changes aren't
necessary unless you want deterministic numbering. I screwed that up
badly :/ Thanks for finding this.
Greg, can you also apply this patch or squash it into the bad one?
----8<-----
From: Stephen Boyd <[email protected]>
Subject: [PATCH] tty: serial: msm_serial: Don't required DT aliases
If there isn't a DT alias then of_alias_get_id() will return
-ENODEV. This will cause the msm_serial driver to fail probe,
when we want to keep the previous behavior where we generated a
dynamic line number at probe time. Restore this behavior by
generating a dynamic id if the line number is still negative
after checking for an alias or (in the non-DT case) looking at the
.id field of the platform device.
Reported-by: Kevin Hilman <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/tty/serial/msm_serial.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index 09364dd8cf3a..d1bc6b6cbc70 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -1046,14 +1046,14 @@ static int msm_serial_probe(struct platform_device *pdev)
const struct of_device_id *id;
int irq, line;
- if (pdev->id == -1)
- pdev->id = atomic_inc_return(&msm_uart_next_id) - 1;
-
if (pdev->dev.of_node)
line = of_alias_get_id(pdev->dev.of_node, "serial");
else
line = pdev->id;
+ if (line < 0)
+ line = atomic_inc_return(&msm_uart_next_id) - 1;
+
if (unlikely(line < 0 || line >= UART_NR))
return -ENXIO;
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 11/06/2014 08:44 PM, Frank Rowand wrote:
> From: Frank Rowand <[email protected]>
>
> Update devicetree binding for msm_serial to reflect msm_serial_probe()
> getting line id from the serial alias.
>
> Signed-off-by: Frank Rowand <[email protected]>
> ---
> Documentation/devicetree/bindings/serial/qcom,msm-uartdm.txt | 15 +++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> Index: b/Documentation/devicetree/bindings/serial/qcom,msm-uartdm.txt
> ===================================================================
> --- a/Documentation/devicetree/bindings/serial/qcom,msm-uartdm.txt
> +++ b/Documentation/devicetree/bindings/serial/qcom,msm-uartdm.txt
> @@ -27,11 +27,18 @@ Optional properties:
> - dmas: Should contain dma specifiers for transmit and receive channels
> - dma-names: Should contain "tx" for transmit and "rx" for receive channels
>
> +Additional requirements:
> +- Each UART port must have an alias correctly numbered in "aliases" node.
s/requirements/notes/
s/must/can/
It wasn't the intention to make it a requirement.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 11/10/2014 11:42 AM, Stephen Boyd wrote:
> On 11/10/2014 10:54 AM, Kevin Hilman wrote:
>> On Wed, Oct 22, 2014 at 5:33 PM, Stephen Boyd <[email protected]> wrote:
>>> We rely on probe order of this driver to determine the line number for
>>> the uart port. This makes it impossible to know the line number
>>> when these devices are populated via DT. Use the DT alias
>>> mechanism to assign the line based on the aliases node.
>>>
>>> Signed-off-by: Stephen Boyd <[email protected]>
>> FYI... this patch hit linux-next and caused multiple boot failures on
>> qcom platforms[1] as of next-20141110. I'm assuming this is because
>> the corresponding DTS changes have not hit linux-next yet.
>>
>> Kevin
>>
>> [1] http://status.armcloud.us/boot/?qcom
>
> Hmm the intention was to make it optional so that dts changes aren't
> necessary unless you want deterministic numbering. I screwed that up
> badly :/ Thanks for finding this.
>
> Greg, can you also apply this patch or squash it into the bad one?
>
> ----8<-----
>
> From: Stephen Boyd <[email protected]>
> Subject: [PATCH] tty: serial: msm_serial: Don't required DT aliases
>
> If there isn't a DT alias then of_alias_get_id() will return
> -ENODEV. This will cause the msm_serial driver to fail probe,
> when we want to keep the previous behavior where we generated a
> dynamic line number at probe time. Restore this behavior by
> generating a dynamic id if the line number is still negative
> after checking for an alias or (in the non-DT case) looking at the
> .id field of the platform device.
>
> Reported-by: Kevin Hilman <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> drivers/tty/serial/msm_serial.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
> index 09364dd8cf3a..d1bc6b6cbc70 100644
> --- a/drivers/tty/serial/msm_serial.c
> +++ b/drivers/tty/serial/msm_serial.c
> @@ -1046,14 +1046,14 @@ static int msm_serial_probe(struct platform_device *pdev)
> const struct of_device_id *id;
> int irq, line;
>
> - if (pdev->id == -1)
> - pdev->id = atomic_inc_return(&msm_uart_next_id) - 1;
> -
> if (pdev->dev.of_node)
> line = of_alias_get_id(pdev->dev.of_node, "serial");
> else
> line = pdev->id;
>
> + if (line < 0)
> + line = atomic_inc_return(&msm_uart_next_id) - 1;
> +
> if (unlikely(line < 0 || line >= UART_NR))
Then this original check for "line < 0" can also be removed.
> return -ENXIO;
>
>
On 11/10/2014 05:56 PM, Frank Rowand wrote:
> On 11/10/2014 11:42 AM, Stephen Boyd wrote:
>> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
>> index 09364dd8cf3a..d1bc6b6cbc70 100644
>> --- a/drivers/tty/serial/msm_serial.c
>> +++ b/drivers/tty/serial/msm_serial.c
>> @@ -1046,14 +1046,14 @@ static int msm_serial_probe(struct platform_device *pdev)
>> const struct of_device_id *id;
>> int irq, line;
>>
>> - if (pdev->id == -1)
>> - pdev->id = atomic_inc_return(&msm_uart_next_id) - 1;
>> -
>> if (pdev->dev.of_node)
>> line = of_alias_get_id(pdev->dev.of_node, "serial");
>> else
>> line = pdev->id;
>>
>> + if (line < 0)
>> + line = atomic_inc_return(&msm_uart_next_id) - 1;
>> +
>> if (unlikely(line < 0 || line >= UART_NR))
> Then this original check for "line < 0" can also be removed.
>
>
Well this matches what was there before. It would do atomic_inc_return
if the line was negative and then still check for a negative value. I
don't mind removing it though. Perhaps we should use an ida?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
From: Frank Rowand <[email protected]>
v2, changes from v1:
- The patch this documents was updated to make the serialN alias
optional instead of required.
Update devicetree binding for msm_serial to reflect msm_serial_probe()
getting line id from the serial alias.
Signed-off-by: Frank Rowand <[email protected]>
---
Documentation/devicetree/bindings/serial/qcom,msm-uartdm.txt | 15 +++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
Index: linux/Documentation/devicetree/bindings/serial/qcom,msm-uartdm.txt
===================================================================
--- linux.orig/Documentation/devicetree/bindings/serial/qcom,msm-uartdm.txt
+++ linux/Documentation/devicetree/bindings/serial/qcom,msm-uartdm.txt
@@ -27,11 +27,21 @@ Optional properties:
- dmas: Should contain dma specifiers for transmit and receive channels
- dma-names: Should contain "tx" for transmit and "rx" for receive channels
+Note: Each UART port should have an alias correctly numbered in the
+"aliases" node, according to serialN format, where N is the port number
+(non-negative decimal integer). If the aliases are not present then
+the port numbers will be 0..(number of UARTS - 1), assigned in the driver
+probe order.
+
Examples:
A uartdm v1.4 device with dma capabilities.
-serial@f991e000 {
+aliases {
+ serial0 = &serial0;
+};
+
+serial0: serial@f991e000 {
compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
reg = <0xf991e000 0x1000>;
interrupts = <0 108 0x0>;
@@ -43,7 +53,11 @@ serial@f991e000 {
A uartdm v1.3 device without dma capabilities and part of a GSBI complex.
-serial@19c40000 {
+aliases {
+ serial0 = &serial0;
+};
+
+serial0: serial@19c40000 {
compatible = "qcom,msm-uartdm-v1.3", "qcom,msm-uartdm";
reg = <0x19c40000 0x1000>,
<0x19c00000 0x1000>;
On 11/10/2014 06:20 PM, Frank Rowand wrote:
> From: Frank Rowand <[email protected]>
>
> v2, changes from v1:
> - The patch this documents was updated to make the serialN alias
> optional instead of required.
Not sure we want this v2 vs v1 part, but
>
> Update devicetree binding for msm_serial to reflect msm_serial_probe()
> getting line id from the serial alias.
>
> Signed-off-by: Frank Rowand <[email protected]>
Reviewed-by: Stephen Boyd <[email protected]>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 11/10/2014 6:07 PM, Stephen Boyd wrote:
> On 11/10/2014 05:56 PM, Frank Rowand wrote:
>> On 11/10/2014 11:42 AM, Stephen Boyd wrote:
>>> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
>>> index 09364dd8cf3a..d1bc6b6cbc70 100644
>>> --- a/drivers/tty/serial/msm_serial.c
>>> +++ b/drivers/tty/serial/msm_serial.c
>>> @@ -1046,14 +1046,14 @@ static int msm_serial_probe(struct platform_device *pdev)
>>> const struct of_device_id *id;
>>> int irq, line;
>>>
>>> - if (pdev->id == -1)
>>> - pdev->id = atomic_inc_return(&msm_uart_next_id) - 1;
>>> -
>>> if (pdev->dev.of_node)
>>> line = of_alias_get_id(pdev->dev.of_node, "serial");
>>> else
>>> line = pdev->id;
>>>
>>> + if (line < 0)
>>> + line = atomic_inc_return(&msm_uart_next_id) - 1;
>>> +
>>> if (unlikely(line < 0 || line >= UART_NR))
>> Then this original check for "line < 0" can also be removed.
>>
>>
>
> Well this matches what was there before. It would do atomic_inc_return
> if the line was negative and then still check for a negative value. I
> don't mind removing it though. Perhaps we should use an ida?:
>
OK, you are right. If (pdev->id < -1) and (!pdev->dev.of_node) then
the check is still needed.
You could use an ida. Some drivers use a bit map. I really don't think
this should become a complicated algorithm though. If the rule is that
either all UARTS have an alias, or no UART has an alias, then I think
the patch could be something like the following.
This combines your original patch, plus your fix patch, plus making
the aliases all or nothing. Not tested, not even compiled.
What do you think?
Index: linux/drivers/tty/serial/msm_serial.c
===================================================================
--- linux.orig/drivers/tty/serial/msm_serial.c
+++ linux/drivers/tty/serial/msm_serial.c
@@ -1044,17 +1044,28 @@ static int msm_serial_probe(struct platf
struct resource *resource;
struct uart_port *port;
const struct of_device_id *id;
- int irq;
+ int irq, line;
+ static int no_prev_alias = 1;
- if (pdev->id == -1)
- pdev->id = atomic_inc_return(&msm_uart_next_id) - 1;
+ if (pdev->dev.of_node) {
+ line = of_alias_get_id(pdev->dev.of_node, "serial");
+ if (line < 0 && no_prev_alias)
+ line = atomic_inc_return(&msm_uart_next_id) - 1;
+ else
+ no_prev_alias = 0;
+ } else {
+ if (pdev->id < 0 && no_prev_alias)
+ line = atomic_inc_return(&msm_uart_next_id) - 1;
+ else
+ line = pdev->id;
+ }
- if (unlikely(pdev->id < 0 || pdev->id >= UART_NR))
+ if (unlikely(line < 0 || line >= UART_NR))
return -ENXIO;
- dev_info(&pdev->dev, "msm_serial: detected port #%d\n", pdev->id);
+ dev_info(&pdev->dev, "msm_serial: detected port #%d\n", line);
- port = get_port_from_line(pdev->id);
+ port = get_port_from_line(line);
port->dev = &pdev->dev;
msm_port = UART_TO_MSM(port);
Stephen Boyd <[email protected]> writes:
> On 11/10/2014 10:54 AM, Kevin Hilman wrote:
>> On Wed, Oct 22, 2014 at 5:33 PM, Stephen Boyd <[email protected]> wrote:
>>> We rely on probe order of this driver to determine the line number for
>>> the uart port. This makes it impossible to know the line number
>>> when these devices are populated via DT. Use the DT alias
>>> mechanism to assign the line based on the aliases node.
>>>
>>> Signed-off-by: Stephen Boyd <[email protected]>
>> FYI... this patch hit linux-next and caused multiple boot failures on
>> qcom platforms[1] as of next-20141110. I'm assuming this is because
>> the corresponding DTS changes have not hit linux-next yet.
>>
>> Kevin
>>
>> [1] http://status.armcloud.us/boot/?qcom
>
> Hmm the intention was to make it optional so that dts changes aren't
> necessary unless you want deterministic numbering. I screwed that up
> badly :/ Thanks for finding this.
>
> Greg, can you also apply this patch or squash it into the bad one?
>
> ----8<-----
>
> From: Stephen Boyd <[email protected]>
> Subject: [PATCH] tty: serial: msm_serial: Don't required DT aliases
>
> If there isn't a DT alias then of_alias_get_id() will return
> -ENODEV. This will cause the msm_serial driver to fail probe,
> when we want to keep the previous behavior where we generated a
> dynamic line number at probe time. Restore this behavior by
> generating a dynamic id if the line number is still negative
> after checking for an alias or (in the non-DT case) looking at the
> .id field of the platform device.
>
> Reported-by: Kevin Hilman <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
Tested-by: Kevin Hilman <[email protected]>
I confirm that this patch gets things booting again for the
msm8974/xperia-z1 and the apq8064/ifc6410.
Kevin
On 11/10/2014 7:20 PM, Frank Rowand wrote:
> On 11/10/2014 6:07 PM, Stephen Boyd wrote:
>> On 11/10/2014 05:56 PM, Frank Rowand wrote:
>>> On 11/10/2014 11:42 AM, Stephen Boyd wrote:
>>>> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
>>>> index 09364dd8cf3a..d1bc6b6cbc70 100644
>>>> --- a/drivers/tty/serial/msm_serial.c
>>>> +++ b/drivers/tty/serial/msm_serial.c
>>>> @@ -1046,14 +1046,14 @@ static int msm_serial_probe(struct platform_device *pdev)
>>>> const struct of_device_id *id;
>>>> int irq, line;
>>>>
>>>> - if (pdev->id == -1)
>>>> - pdev->id = atomic_inc_return(&msm_uart_next_id) - 1;
>>>> -
>>>> if (pdev->dev.of_node)
>>>> line = of_alias_get_id(pdev->dev.of_node, "serial");
>>>> else
>>>> line = pdev->id;
>>>>
>>>> + if (line < 0)
>>>> + line = atomic_inc_return(&msm_uart_next_id) - 1;
>>>> +
>>>> if (unlikely(line < 0 || line >= UART_NR))
>>> Then this original check for "line < 0" can also be removed.
>>>
>>>
>>
>> Well this matches what was there before. It would do atomic_inc_return
>> if the line was negative and then still check for a negative value. I
>> don't mind removing it though. Perhaps we should use an ida?:
>>
>
> OK, you are right. If (pdev->id < -1) and (!pdev->dev.of_node) then
> the check is still needed.
>
> You could use an ida. Some drivers use a bit map. I really don't think
> this should become a complicated algorithm though. If the rule is that
> either all UARTS have an alias, or no UART has an alias, then I think
> the patch could be something like the following.
>
> This combines your original patch, plus your fix patch, plus making
> the aliases all or nothing. Not tested, not even compiled.
>
> What do you think?
< snip - previous version of patch >
Previous version of the patch did not protect against no alias, followed
by alias.
From: Frank Rowand <[email protected]>
This patch is intended to show roughly what an implementation of an all or
nothing policy for using serialN aliases would look like. It is intended
simply to help determine whether this policy should be used.
Combines Stephen's first patch, Stephen's fix patch (to make the serialN
alias optional), plus makes use of the serialN alias all or nothing (either
all ports have an alias or none do).
This is not a correct implementation because the atomic_read(&msm_uart_next_id)
does not protect against concurrency; instead a lock should be used.
Not tested, not even compiled.
Not-Signed-off-by: Frank Rowand <[email protected]>
---
Index: linux/drivers/tty/serial/msm_serial.c
===================================================================
--- linux.orig/drivers/tty/serial/msm_serial.c
+++ linux/drivers/tty/serial/msm_serial.c
@@ -1044,17 +1044,31 @@ static int msm_serial_probe(struct platf
struct resource *resource;
struct uart_port *port;
const struct of_device_id *id;
- int irq;
+ int irq, line;
+ static int no_prev_alias = 1;
- if (pdev->id == -1)
- pdev->id = atomic_inc_return(&msm_uart_next_id) - 1;
+ if (pdev->dev.of_node) {
+ line = of_alias_get_id(pdev->dev.of_node, "serial");
+ if (line < 0 && no_prev_alias) {
+ line = atomic_inc_return(&msm_uart_next_id) - 1;
+ } else {
+ no_prev_alias = 0;
+ if (atomic_read(&msm_uart_next_id))
+ line = -ENXIO;
+ }
+ } else {
+ if (pdev->id < 0 && no_prev_alias)
+ line = atomic_inc_return(&msm_uart_next_id) - 1;
+ else
+ line = pdev->id;
+ }
- if (unlikely(pdev->id < 0 || pdev->id >= UART_NR))
+ if (unlikely(line < 0 || line >= UART_NR))
return -ENXIO;
- dev_info(&pdev->dev, "msm_serial: detected port #%d\n", pdev->id);
+ dev_info(&pdev->dev, "msm_serial: detected port #%d\n", line);
- port = get_port_from_line(pdev->id);
+ port = get_port_from_line(line);
port->dev = &pdev->dev;
msm_port = UART_TO_MSM(port);
On 11/12/2014 10:14 AM, Frank Rowand wrote:
> On 11/10/2014 7:20 PM, Frank Rowand wrote:
>> On 11/10/2014 6:07 PM, Stephen Boyd wrote:
>>> On 11/10/2014 05:56 PM, Frank Rowand wrote:
>>>> On 11/10/2014 11:42 AM, Stephen Boyd wrote:
>>>>> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
>>>>> index 09364dd8cf3a..d1bc6b6cbc70 100644
>>>>> --- a/drivers/tty/serial/msm_serial.c
>>>>> +++ b/drivers/tty/serial/msm_serial.c
>>>>> @@ -1046,14 +1046,14 @@ static int msm_serial_probe(struct platform_device *pdev)
>>>>> const struct of_device_id *id;
>>>>> int irq, line;
>>>>>
>>>>> - if (pdev->id == -1)
>>>>> - pdev->id = atomic_inc_return(&msm_uart_next_id) - 1;
>>>>> -
>>>>> if (pdev->dev.of_node)
>>>>> line = of_alias_get_id(pdev->dev.of_node, "serial");
>>>>> else
>>>>> line = pdev->id;
>>>>>
>>>>> + if (line < 0)
>>>>> + line = atomic_inc_return(&msm_uart_next_id) - 1;
>>>>> +
>>>>> if (unlikely(line < 0 || line >= UART_NR))
>>>> Then this original check for "line < 0" can also be removed.
>>>>
>>>>
>>> Well this matches what was there before. It would do atomic_inc_return
>>> if the line was negative and then still check for a negative value. I
>>> don't mind removing it though. Perhaps we should use an ida?:
>>>
>> OK, you are right. If (pdev->id < -1) and (!pdev->dev.of_node) then
>> the check is still needed.
>>
>> You could use an ida. Some drivers use a bit map. I really don't think
>> this should become a complicated algorithm though. If the rule is that
>> either all UARTS have an alias, or no UART has an alias, then I think
>> the patch could be something like the following.
>>
>> This combines your original patch, plus your fix patch, plus making
>> the aliases all or nothing. Not tested, not even compiled.
>>
>> What do you think?
> < snip - previous version of patch >
>
> Previous version of the patch did not protect against no alias, followed
> by alias.
>
>
>
> From: Frank Rowand <[email protected]>
>
> This patch is intended to show roughly what an implementation of an all or
> nothing policy for using serialN aliases would look like. It is intended
> simply to help determine whether this policy should be used.
>
> Combines Stephen's first patch, Stephen's fix patch (to make the serialN
> alias optional), plus makes use of the serialN alias all or nothing (either
> all ports have an alias or none do).
>
> This is not a correct implementation because the atomic_read(&msm_uart_next_id)
> does not protect against concurrency; instead a lock should be used.
>
> Not tested, not even compiled.
Sorry, I'm sort of lost. If there are serial aliases in the dts file,
then we should alias all of the serial ports. If there aren't aliases
then we're backwards compatible with the dts we have now and we'll do
dynamic generation. Putting code into the driver to validate that this
is true is not the job of the driver. If anything, it should validated
when the dts file is created. If one day we screw up and have a dts file
with such a bad configuration we'll have to work around it, but until
that day comes I'd rather not think about it.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 11/13/2014 11:31 AM, Stephen Boyd wrote:
> On 11/12/2014 10:14 AM, Frank Rowand wrote:
>> On 11/10/2014 7:20 PM, Frank Rowand wrote:
>>> On 11/10/2014 6:07 PM, Stephen Boyd wrote:
>>>> On 11/10/2014 05:56 PM, Frank Rowand wrote:
>>>>> On 11/10/2014 11:42 AM, Stephen Boyd wrote:
>>>>>> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
>>>>>> index 09364dd8cf3a..d1bc6b6cbc70 100644
>>>>>> --- a/drivers/tty/serial/msm_serial.c
>>>>>> +++ b/drivers/tty/serial/msm_serial.c
>>>>>> @@ -1046,14 +1046,14 @@ static int msm_serial_probe(struct platform_device *pdev)
>>>>>> const struct of_device_id *id;
>>>>>> int irq, line;
>>>>>>
>>>>>> - if (pdev->id == -1)
>>>>>> - pdev->id = atomic_inc_return(&msm_uart_next_id) - 1;
>>>>>> -
>>>>>> if (pdev->dev.of_node)
>>>>>> line = of_alias_get_id(pdev->dev.of_node, "serial");
>>>>>> else
>>>>>> line = pdev->id;
>>>>>>
>>>>>> + if (line < 0)
>>>>>> + line = atomic_inc_return(&msm_uart_next_id) - 1;
>>>>>> +
>>>>>> if (unlikely(line < 0 || line >= UART_NR))
>>>>> Then this original check for "line < 0" can also be removed.
>>>>>
>>>>>
>>>> Well this matches what was there before. It would do atomic_inc_return
>>>> if the line was negative and then still check for a negative value. I
>>>> don't mind removing it though. Perhaps we should use an ida?:
>>>>
>>> OK, you are right. If (pdev->id < -1) and (!pdev->dev.of_node) then
>>> the check is still needed.
>>>
>>> You could use an ida. Some drivers use a bit map. I really don't think
>>> this should become a complicated algorithm though. If the rule is that
>>> either all UARTS have an alias, or no UART has an alias, then I think
>>> the patch could be something like the following.
>>>
>>> This combines your original patch, plus your fix patch, plus making
>>> the aliases all or nothing. Not tested, not even compiled.
>>>
>>> What do you think?
>> < snip - previous version of patch >
>>
>> Previous version of the patch did not protect against no alias, followed
>> by alias.
>>
>>
>>
>> From: Frank Rowand <[email protected]>
>>
>> This patch is intended to show roughly what an implementation of an all or
>> nothing policy for using serialN aliases would look like. It is intended
>> simply to help determine whether this policy should be used.
>>
>> Combines Stephen's first patch, Stephen's fix patch (to make the serialN
>> alias optional), plus makes use of the serialN alias all or nothing (either
>> all ports have an alias or none do).
>>
>> This is not a correct implementation because the atomic_read(&msm_uart_next_id)
>> does not protect against concurrency; instead a lock should be used.
>>
>> Not tested, not even compiled.
>
> Sorry, I'm sort of lost. If there are serial aliases in the dts file,
> then we should alias all of the serial ports. If there aren't aliases
> then we're backwards compatible with the dts we have now and we'll do
> dynamic generation. Putting code into the driver to validate that this
> is true is not the job of the driver. If anything, it should validated
> when the dts file is created. If one day we screw up and have a dts file
> with such a bad configuration we'll have to work around it, but until
> that day comes I'd rather not think about it.
Maybe I did not understand when you said "Perhaps we should use an ida".
That sentence led me to think the driver should check for misconfiguration.
The case I was trying to handle was if there was at least one serialN
alias and at least one UART without an alias. For example, if there
are three UARTs (serial_a, serial_b, serial_c, probed in that order)
and one alias (serial0 = &serial_c;) then the result would be:
serial_a line 0 (from msm_uart_next_id)
serial_b line 1 (from msm_uart_next_id)
serial_c line 0 (from the alias)
Two UARTs probed with line == 0. This is an error.
Most of the serial drivers don't check for this type of bad configuration.
Some drivers keep a bit map of which lines have been used. I'm not sure
what they do in case of a conflict (I did not read to that level of detail).
I thought you were suggesting the driver check for the bad configuration,
so I was proposing a somewhat simple way of forcing a boot error for the
bad configuration.
Since you are not suggesting the driver check for the bad configuration,
you can ignore my proposal. I agree that it is ok for the driver to
expect the board dts to be correct. The problem should be detected by
the dts author on first boot as part of normal bring up testing, and
then corrected.
-Frank
On 11/13/2014 04:46 PM, Frank Rowand wrote:
> On 11/13/2014 11:31 AM, Stephen Boyd wrote:
>> Sorry, I'm sort of lost. If there are serial aliases in the dts file,
>> then we should alias all of the serial ports. If there aren't aliases
>> then we're backwards compatible with the dts we have now and we'll do
>> dynamic generation. Putting code into the driver to validate that
>> this is true is not the job of the driver. If anything, it should
>> validated when the dts file is created. If one day we screw up and
>> have a dts file with such a bad configuration we'll have to work
>> around it, but until that day comes I'd rather not think about it.
> Maybe I did not understand when you said "Perhaps we should use an ida".
> That sentence led me to think the driver should check for misconfiguration.
> The case I was trying to handle was if there was at least one serialN
> alias and at least one UART without an alias. For example, if there
> are three UARTs (serial_a, serial_b, serial_c, probed in that order)
> and one alias (serial0 = &serial_c;) then the result would be:
>
> serial_a line 0 (from msm_uart_next_id)
> serial_b line 1 (from msm_uart_next_id)
> serial_c line 0 (from the alias)
>
> Two UARTs probed with line == 0. This is an error.
>
> Most of the serial drivers don't check for this type of bad configuration.
> Some drivers keep a bit map of which lines have been used. I'm not sure
> what they do in case of a conflict (I did not read to that level of detail).
>
> I thought you were suggesting the driver check for the bad configuration,
> so I was proposing a somewhat simple way of forcing a boot error for the
> bad configuration.
>
> Since you are not suggesting the driver check for the bad configuration,
> you can ignore my proposal. I agree that it is ok for the driver to
> expect the board dts to be correct. The problem should be detected by
> the dts author on first boot as part of normal bring up testing, and
> then corrected.
>
Ah ok. I was just saying we could use an ida instead of an atomic
increment so that this driver works properly with driver
binding/unbinding, otherwise the line number keeps increasing and
quickly goes beyond the static array of ports (which I still don't
understand why we have at all btw).
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Stephen Boyd <[email protected]> writes:
> On 11/13/2014 04:46 PM, Frank Rowand wrote:
>> On 11/13/2014 11:31 AM, Stephen Boyd wrote:
>>> Sorry, I'm sort of lost. If there are serial aliases in the dts file,
>>> then we should alias all of the serial ports. If there aren't aliases
>>> then we're backwards compatible with the dts we have now and we'll do
>>> dynamic generation. Putting code into the driver to validate that
>>> this is true is not the job of the driver. If anything, it should
>>> validated when the dts file is created. If one day we screw up and
>>> have a dts file with such a bad configuration we'll have to work
>>> around it, but until that day comes I'd rather not think about it.
>> Maybe I did not understand when you said "Perhaps we should use an ida".
>> That sentence led me to think the driver should check for misconfiguration.
>> The case I was trying to handle was if there was at least one serialN
>> alias and at least one UART without an alias. For example, if there
>> are three UARTs (serial_a, serial_b, serial_c, probed in that order)
>> and one alias (serial0 = &serial_c;) then the result would be:
>>
>> serial_a line 0 (from msm_uart_next_id)
>> serial_b line 1 (from msm_uart_next_id)
>> serial_c line 0 (from the alias)
>>
>> Two UARTs probed with line == 0. This is an error.
>>
>> Most of the serial drivers don't check for this type of bad configuration.
>> Some drivers keep a bit map of which lines have been used. I'm not sure
>> what they do in case of a conflict (I did not read to that level of detail).
>>
>> I thought you were suggesting the driver check for the bad configuration,
>> so I was proposing a somewhat simple way of forcing a boot error for the
>> bad configuration.
>>
>> Since you are not suggesting the driver check for the bad configuration,
>> you can ignore my proposal. I agree that it is ok for the driver to
>> expect the board dts to be correct. The problem should be detected by
>> the dts author on first boot as part of normal bring up testing, and
>> then corrected.
>>
>
> Ah ok. I was just saying we could use an ida instead of an atomic
> increment so that this driver works properly with driver
> binding/unbinding, otherwise the line number keeps increasing and
> quickly goes beyond the static array of ports (which I still don't
> understand why we have at all btw).
Due to the length of the thread, I haven't followed all the details, and
I suspect Greg hasn't either, so I'm not sure if you're discssuing what
the right fix is for what's in -next (still broken[1], or what should be done
with the device board files.
If the fix from earlier in this thread is still the right one for fixing
-next, could you repost it separately for Greg to queue/squash and for
me to re-test (if needed.)
Thanks,
Kevin
[1] http://lists.linaro.org/pipermail/kernel-build-reports/2014-November/006298.html
On 11/14, Kevin Hilman wrote:
>
> Due to the length of the thread, I haven't followed all the details, and
> I suspect Greg hasn't either, so I'm not sure if you're discssuing what
> the right fix is for what's in -next (still broken[1], or what should be done
> with the device board files.
>
> If the fix from earlier in this thread is still the right one for fixing
> -next, could you repost it separately for Greg to queue/squash and for
> me to re-test (if needed.)
>
No problem. I'll pick up your tested-by and resend.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project