LKML Archive on lore.kernel.org
 help / color / Atom feed
* Regression: commit c9712e333809 breaks xilinx_uartps
@ 2019-09-11 17:04 Paul Thomas
  2019-09-12  5:53 ` Michal Simek
  0 siblings, 1 reply; 4+ messages in thread
From: Paul Thomas @ 2019-09-11 17:04 UTC (permalink / raw)
  To: Shubhrajyoti Datta, Michal Simek, Kroah-Hartman, Sasha Levin,
	linux-kernel

Hello,

As I was working with a recent 5.2 kernel with a Zynq Ultrascale+
board I found that the serial console wasn't working with a message
like:
 xuartps: probe of ff000000.serial failed with error -16

I did a git bisect and found that this came from:
 commit: c9712e3338098359a82c3f5d198c92688fa6cd26
 serial: uartps: Use the same dynamic major number for all ports

One difference I might have is in the device-tree, I'm using a proper
clock config (zynqmp-clk-ccf.dtsi) using the firmware clock interface.
This is absolutely necessary, for instance, with the Ethernet
negotiation where the macb driver needs to change the clock rate. In
any case I believe this pushes my case to the -EPROBE_DEFER when
devm_clk_get() fails the first time, this might not have been tested
in with the original submission. I'm not sure this makes everything
completely correct, but the patch below does fix the issue for me.

thanks,
Paul

---
 drivers/tty/serial/xilinx_uartps.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/xilinx_uartps.c
b/drivers/tty/serial/xilinx_uartps.c
index 9dcc4d855ddd..ece7f6caa994 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1565,6 +1565,8 @@ static int cdns_uart_probe(struct platform_device *pdev)

        cdns_uart_data->pclk = devm_clk_get(&pdev->dev, "pclk");
        if (PTR_ERR(cdns_uart_data->pclk) == -EPROBE_DEFER) {
+               /* If we end up defering then set uartps_major back to 0 */
+               uartps_major = 0;
                rc = PTR_ERR(cdns_uart_data->pclk);
                goto err_out_unregister_driver;
        }
-- 
2.17.1

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Regression: commit c9712e333809 breaks xilinx_uartps
  2019-09-11 17:04 Regression: commit c9712e333809 breaks xilinx_uartps Paul Thomas
@ 2019-09-12  5:53 ` Michal Simek
  2019-09-12 16:38   ` Paul Thomas
  0 siblings, 1 reply; 4+ messages in thread
From: Michal Simek @ 2019-09-12  5:53 UTC (permalink / raw)
  To: Paul Thomas, Shubhrajyoti Datta, Michal Simek, Kroah-Hartman,
	Sasha Levin, linux-kernel

On 11. 09. 19 19:04, Paul Thomas wrote:
> Hello,
> 
> As I was working with a recent 5.2 kernel with a Zynq Ultrascale+
> board I found that the serial console wasn't working with a message
> like:
>  xuartps: probe of ff000000.serial failed with error -16
> 
> I did a git bisect and found that this came from:
>  commit: c9712e3338098359a82c3f5d198c92688fa6cd26
>  serial: uartps: Use the same dynamic major number for all ports
> 
> One difference I might have is in the device-tree, I'm using a proper
> clock config (zynqmp-clk-ccf.dtsi) using the firmware clock interface.
> This is absolutely necessary, for instance, with the Ethernet
> negotiation where the macb driver needs to change the clock rate. In
> any case I believe this pushes my case to the -EPROBE_DEFER when
> devm_clk_get() fails the first time, this might not have been tested
> in with the original submission. I'm not sure this makes everything
> completely correct, but the patch below does fix the issue for me.
> 
> thanks,
> Paul
> 
> ---
>  drivers/tty/serial/xilinx_uartps.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/tty/serial/xilinx_uartps.c
> b/drivers/tty/serial/xilinx_uartps.c
> index 9dcc4d855ddd..ece7f6caa994 100644
> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -1565,6 +1565,8 @@ static int cdns_uart_probe(struct platform_device *pdev)
> 
>         cdns_uart_data->pclk = devm_clk_get(&pdev->dev, "pclk");
>         if (PTR_ERR(cdns_uart_data->pclk) == -EPROBE_DEFER) {
> +               /* If we end up defering then set uartps_major back to 0 */
> +               uartps_major = 0;
>                 rc = PTR_ERR(cdns_uart_data->pclk);
>                 goto err_out_unregister_driver;
>         }
> 

I expect that this can be problematic for all failures in probe.
What about this? Just setting up global major number if first instance
pass.
Also cleanup should be likely done in remove function too.


 diff --git a/drivers/tty/serial/xilinx_uartps.c
b/drivers/tty/serial/xilinx_uartps.c
 index f145946f659b..c1550b45d59b 100644
 --- a/drivers/tty/serial/xilinx_uartps.c
 +++ b/drivers/tty/serial/xilinx_uartps.c
 @@ -1550,7 +1550,6 @@ static int cdns_uart_probe(struct platform_device
*pdev)
                 goto err_out_id;
         }

 -       uartps_major = cdns_uart_uart_driver->tty_driver->major;
         cdns_uart_data->cdns_uart_driver = cdns_uart_uart_driver;

         /*
 @@ -1680,6 +1679,7 @@ static int cdns_uart_probe(struct platform_device
*pdev)
                 console_port = NULL;
  #endif

 +       uartps_major = cdns_uart_uart_driver->tty_driver->major;
         cdns_uart_data->cts_override =
of_property_read_bool(pdev->dev.of_node,

"cts-override");
         return 0;

Thanks,
Michal

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Regression: commit c9712e333809 breaks xilinx_uartps
  2019-09-12  5:53 ` Michal Simek
@ 2019-09-12 16:38   ` Paul Thomas
  2019-09-13  6:03     ` Michal Simek
  0 siblings, 1 reply; 4+ messages in thread
From: Paul Thomas @ 2019-09-12 16:38 UTC (permalink / raw)
  To: Michal Simek; +Cc: Shubhrajyoti Datta, Kroah-Hartman, Sasha Levin, linux-kernel

> > ---
> >  drivers/tty/serial/xilinx_uartps.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/tty/serial/xilinx_uartps.c
> > b/drivers/tty/serial/xilinx_uartps.c
> > index 9dcc4d855ddd..ece7f6caa994 100644
> > --- a/drivers/tty/serial/xilinx_uartps.c
> > +++ b/drivers/tty/serial/xilinx_uartps.c
> > @@ -1565,6 +1565,8 @@ static int cdns_uart_probe(struct platform_device *pdev)
> >
> >         cdns_uart_data->pclk = devm_clk_get(&pdev->dev, "pclk");
> >         if (PTR_ERR(cdns_uart_data->pclk) == -EPROBE_DEFER) {
> > +               /* If we end up defering then set uartps_major back to 0 */
> > +               uartps_major = 0;
> >                 rc = PTR_ERR(cdns_uart_data->pclk);
> >                 goto err_out_unregister_driver;
> >         }
> >
>
> I expect that this can be problematic for all failures in probe.
> What about this?
Makes sense, this worked for me. Although, I think the patch is
malformed by the email line lengths.

-Paul

> Just setting up global major number if first instance
> pass.
> Also cleanup should be likely done in remove function too.
>
>
>  diff --git a/drivers/tty/serial/xilinx_uartps.c
> b/drivers/tty/serial/xilinx_uartps.c
>  index f145946f659b..c1550b45d59b 100644
>  --- a/drivers/tty/serial/xilinx_uartps.c
>  +++ b/drivers/tty/serial/xilinx_uartps.c
>  @@ -1550,7 +1550,6 @@ static int cdns_uart_probe(struct platform_device
> *pdev)
>                  goto err_out_id;
>          }
>
>  -       uartps_major = cdns_uart_uart_driver->tty_driver->major;
>          cdns_uart_data->cdns_uart_driver = cdns_uart_uart_driver;
>
>          /*
>  @@ -1680,6 +1679,7 @@ static int cdns_uart_probe(struct platform_device
> *pdev)
>                  console_port = NULL;
>   #endif
>
>  +       uartps_major = cdns_uart_uart_driver->tty_driver->major;
>          cdns_uart_data->cts_override =
> of_property_read_bool(pdev->dev.of_node,
>
> "cts-override");
>          return 0;
>
> Thanks,
> Michal

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Regression: commit c9712e333809 breaks xilinx_uartps
  2019-09-12 16:38   ` Paul Thomas
@ 2019-09-13  6:03     ` Michal Simek
  0 siblings, 0 replies; 4+ messages in thread
From: Michal Simek @ 2019-09-13  6:03 UTC (permalink / raw)
  To: Paul Thomas, Michal Simek
  Cc: Shubhrajyoti Datta, Kroah-Hartman, Sasha Levin, linux-kernel

On 12. 09. 19 18:38, Paul Thomas wrote:
>>> ---
>>>  drivers/tty/serial/xilinx_uartps.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/tty/serial/xilinx_uartps.c
>>> b/drivers/tty/serial/xilinx_uartps.c
>>> index 9dcc4d855ddd..ece7f6caa994 100644
>>> --- a/drivers/tty/serial/xilinx_uartps.c
>>> +++ b/drivers/tty/serial/xilinx_uartps.c
>>> @@ -1565,6 +1565,8 @@ static int cdns_uart_probe(struct platform_device *pdev)
>>>
>>>         cdns_uart_data->pclk = devm_clk_get(&pdev->dev, "pclk");
>>>         if (PTR_ERR(cdns_uart_data->pclk) == -EPROBE_DEFER) {
>>> +               /* If we end up defering then set uartps_major back to 0 */
>>> +               uartps_major = 0;
>>>                 rc = PTR_ERR(cdns_uart_data->pclk);
>>>                 goto err_out_unregister_driver;
>>>         }
>>>
>>
>> I expect that this can be problematic for all failures in probe.
>> What about this?
> Makes sense, this worked for me. Although, I think the patch is
> malformed by the email line lengths.

I just c&p. Let me send proper patch with description.

Thanks,
Michal


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-11 17:04 Regression: commit c9712e333809 breaks xilinx_uartps Paul Thomas
2019-09-12  5:53 ` Michal Simek
2019-09-12 16:38   ` Paul Thomas
2019-09-13  6:03     ` Michal Simek

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox