Re: [PATCH v4 4/6] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 3/21/2018 11:20 AM, Stephen Boyd wrote:
> Quoting Karthik Ramasubramanian (2018-03-20 15:53:25)
>>
>>
>> On 3/20/2018 9:37 AM, Stephen Boyd wrote:
>>> Quoting Karthikeyan Ramasubramanian (2018-03-14 16:58:49)
>>>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>>>> new file mode 100644
>>>> index 0000000..1442777
>>>> --- /dev/null
>>>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>>>> @@ -0,0 +1,1158 @@
>>>> +
> 
>>>
>>> Can you also support the OF_EARLYCON_DECLARE method of console writing
>>> so we can get an early printk style debug console?
>> Do you prefer that as part of this patch itself or is it ok if I upload
>> the earlycon support once this gets merged.
> 
> I think this already got merged? So just split it out into another patch
> would be fine. I see the config is already selecting the earlycon
> support so it must be planned.
Yes it is definitely in the plan. Since the serial driver got merged, I
will address the pending comments in this patch series. I will upload
the early console support in another patch.
> 
>>>
>>>
>>>> +
>>>> +static int __maybe_unused qcom_geni_serial_sys_resume_noirq(struct device *dev)
>>>> +{
>>>> +       struct platform_device *pdev = to_platform_device(dev);
>>>> +       struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
>>>> +       struct uart_port *uport = &port->uport;
>>>> +
>>>> +       if (console_suspend_enabled && uport->suspended) {
>>>> +               uart_resume_port(uport->private_data, uport);
>>>> +               disable_irq(uport->irq);
>>>
>>> I missed the enable_irq() part. Is this still necessary?
>> Suspending the uart console port invokes the uart port shutdown
>> operation. The shutdown operation disables and frees the concerned IRQ.
>> Resuming the uart console port invokes the uart port startup operation
>> which requests for the IRQ. The request_irq operation auto-enables the
>> IRQ. In addition, resume_noirq implicitly enables the IRQ. This leads to
>> an unbalanced IRQ enable warning.
>>
>> This disable_irq() helps with suppressing that warning.
> 
> That's not obvious so we need a big comment here.
> 
> I thought we would move this to the normal suspend/resume callbacks and
> skip the noirq variants. That would make this disable_irq() unnecessary
> then?
For a non-console UART(eg. 4-wire UART), to reduce the wakeup latency
_noirq variant is used so that the resources can be turned on as soon as
possible. The idea is to use the same suspend and resume operation for
both debug-uart and regular uart. Hence using the _noirq variant.

I will add a detailed comment regarding why we are disabling the IRQ.
> 
> BTW, free_irq() should disable the irq itself, so it looks odd to have a
> disable_irq() followed directly by a free_irq().
I will update to just free_irq.
> 
Regards,
Karthik.
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux