Patchwork [6/9] serial: tegra: move to generic dma DT binding

login
register
mail settings
Submitter Richard Zhao
Date July 24, 2013, 4:09 a.m.
Message ID <1374639002-16753-7-git-send-email-rizhao@nvidia.com>
Download mbox | patch
Permalink /patch/261271/
State Superseded, archived
Headers show

Comments

Richard Zhao - July 24, 2013, 4:09 a.m.
- driver: remove use of nvidia,dma-request-selector
	  use dma_request_slave_channel to request channel
- update binding doc

Signed-off-by: Richard Zhao <rizhao@nvidia.com>
---
 .../devicetree/bindings/serial/nvidia,tegra20-hsuart.txt |  8 +++++---
 drivers/tty/serial/serial-tegra.c                        | 16 +---------------
 2 files changed, 6 insertions(+), 18 deletions(-)
Stephen Warren - July 26, 2013, 7:39 p.m.
On 07/23/2013 10:09 PM, Richard Zhao wrote:
> - driver: remove use of nvidia,dma-request-selector
> 	  use dma_request_slave_channel to request channel
> - update binding doc

This patch needs to be amended so that the DMA channel is looked up
during probe() rather than open(), to guarantee that deferred probe
works. It's possible to hold off probe, but not possible AFAIK to hold
off opening the port.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren - July 26, 2013, 7:41 p.m.
On 07/26/2013 01:39 PM, Stephen Warren wrote:
> On 07/23/2013 10:09 PM, Richard Zhao wrote:
>> - driver: remove use of nvidia,dma-request-selector
>> 	  use dma_request_slave_channel to request channel
>> - update binding doc
> 
> This patch needs to be amended so that the DMA channel is looked up
> during probe() rather than open(), to guarantee that deferred probe
> works. It's possible to hold off probe, but not possible AFAIK to hold
> off opening the port.

Hmm. I guess this comment actually applies to all/most of these patches.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Zhao - July 30, 2013, 3:31 a.m.
On Sat, Jul 27, 2013 at 03:39:15AM +0800, Stephen Warren wrote:
> On 07/23/2013 10:09 PM, Richard Zhao wrote:
> > - driver: remove use of nvidia,dma-request-selector
> > 	  use dma_request_slave_channel to request channel
> > - update binding doc
> 
> This patch needs to be amended so that the DMA channel is looked up
> during probe() rather than open(), to guarantee that deferred probe
> works. It's possible to hold off probe, but not possible AFAIK to hold
> off opening the port.
How about avoid return -EPROBE_DEFER at open() time? It can be in
another patch.

Thanks
Richard
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren - July 30, 2013, 4:46 p.m.
On 07/29/2013 09:31 PM, Richard Zhao wrote:
> On Sat, Jul 27, 2013 at 03:39:15AM +0800, Stephen Warren wrote:
>> On 07/23/2013 10:09 PM, Richard Zhao wrote:
>>> - driver: remove use of nvidia,dma-request-selector
>>> 	  use dma_request_slave_channel to request channel
>>> - update binding doc
>>
>> This patch needs to be amended so that the DMA channel is looked up
>> during probe() rather than open(), to guarantee that deferred probe
>> works. It's possible to hold off probe, but not possible AFAIK to hold
>> off opening the port.
>
> How about avoid return -EPROBE_DEFER at open() time? It can be in
> another patch.

I don't really understand what you mean. -EPROBE_DEFER is something that
only makes sense for probe() to return (or functions used by probe()).
It doesn't make sense to return it anywhere else, since probe deferral
is something that can only happen for probe().
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Zhao - July 31, 2013, 3:45 a.m.
On Wed, Jul 31, 2013 at 12:46:42AM +0800, Stephen Warren wrote:
> On 07/29/2013 09:31 PM, Richard Zhao wrote:
> > On Sat, Jul 27, 2013 at 03:39:15AM +0800, Stephen Warren wrote:
> >> On 07/23/2013 10:09 PM, Richard Zhao wrote:
> >>> - driver: remove use of nvidia,dma-request-selector
> >>> 	  use dma_request_slave_channel to request channel
> >>> - update binding doc
> >>
> >> This patch needs to be amended so that the DMA channel is looked up
> >> during probe() rather than open(), to guarantee that deferred probe
> >> works. It's possible to hold off probe, but not possible AFAIK to hold
> >> off opening the port.
> >
> > How about avoid return -EPROBE_DEFER at open() time? It can be in
> > another patch.
> 
> I don't really understand what you mean. -EPROBE_DEFER is something that
> only makes sense for probe() to return (or functions used by probe()).
> It doesn't make sense to return it anywhere else, since probe deferral
> is something that can only happen for probe().
What I meant is, it might not worth to move request channel to probe
only because we need to check error and return -EPROBE_DEFER.

dma_request_slave_channel is better to be called as late as possible to
make better dynamic use of dma channels.

Actually, I searched the kernel code, most drivers don't return EPROBE_DEFER
when dma_request_slave_channel failed. dma_request_slave_channel can
fail in many cases, and dma device not probed is only one case.

Thanks
Richard
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren - July 31, 2013, 9:32 p.m.
On 07/30/2013 09:45 PM, Richard Zhao wrote:
> On Wed, Jul 31, 2013 at 12:46:42AM +0800, Stephen Warren wrote:
>> On 07/29/2013 09:31 PM, Richard Zhao wrote:
>>> On Sat, Jul 27, 2013 at 03:39:15AM +0800, Stephen Warren wrote:
>>>> On 07/23/2013 10:09 PM, Richard Zhao wrote:
>>>>> - driver: remove use of nvidia,dma-request-selector
>>>>> 	  use dma_request_slave_channel to request channel
>>>>> - update binding doc
>>>>
>>>> This patch needs to be amended so that the DMA channel is looked up
>>>> during probe() rather than open(), to guarantee that deferred probe
>>>> works. It's possible to hold off probe, but not possible AFAIK to hold
>>>> off opening the port.
>>>
>>> How about avoid return -EPROBE_DEFER at open() time? It can be in
>>> another patch.
>>
>> I don't really understand what you mean. -EPROBE_DEFER is something that
>> only makes sense for probe() to return (or functions used by probe()).
>> It doesn't make sense to return it anywhere else, since probe deferral
>> is something that can only happen for probe().
>
> What I meant is, it might not worth to move request channel to probe
> only because we need to check error and return -EPROBE_DEFER.

If probe() doesn't attempt to validate that the/a DMA channel provider
exists, then there's no guarantee that the DMA provider has probed
before the DMA client driver either probes or executes any later code.
So, you could quite easily end up with a situation where the serial
driver's open fails because the DMA provider has not yet probed. This is
exactly what deferred probe was intended to avoid. This reason alone is
enough to make it worth fixing this.

> dma_request_slave_channel is better to be called as late as possible to
> make better dynamic use of dma channels.

probe() could validate that the provider exists in a way that doesn't
permanently hold ownership of the DMA channel. For example, request it
then immediately release it, in the absence of a dedicated "validate my
DMA providers exist" API.

> Actually, I searched the kernel code, most drivers don't return EPROBE_DEFER
> when dma_request_slave_channel failed. dma_request_slave_channel can
> fail in many cases, and dma device not probed is only one case.

That sounds like a bug. If a DMA channel /is/ specified, it should be
possible to acquire it. If no DMA channel is specified, and if the
device can do PIO, then it's reasonable to continue.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Zhao - Aug. 1, 2013, 3:30 a.m.
On Thu, Aug 01, 2013 at 05:32:37AM +0800, Stephen Warren wrote:
> On 07/30/2013 09:45 PM, Richard Zhao wrote:
> > On Wed, Jul 31, 2013 at 12:46:42AM +0800, Stephen Warren wrote:
> >> On 07/29/2013 09:31 PM, Richard Zhao wrote:
> >>> On Sat, Jul 27, 2013 at 03:39:15AM +0800, Stephen Warren wrote:
> >>>> On 07/23/2013 10:09 PM, Richard Zhao wrote:
> >>>>> - driver: remove use of nvidia,dma-request-selector
> >>>>> 	  use dma_request_slave_channel to request channel
> >>>>> - update binding doc
> >>>>
> >>>> This patch needs to be amended so that the DMA channel is looked up
> >>>> during probe() rather than open(), to guarantee that deferred probe
> >>>> works. It's possible to hold off probe, but not possible AFAIK to hold
> >>>> off opening the port.
> >>>
> >>> How about avoid return -EPROBE_DEFER at open() time? It can be in
> >>> another patch.
> >>
> >> I don't really understand what you mean. -EPROBE_DEFER is something that
> >> only makes sense for probe() to return (or functions used by probe()).
> >> It doesn't make sense to return it anywhere else, since probe deferral
> >> is something that can only happen for probe().
> >
> > What I meant is, it might not worth to move request channel to probe
> > only because we need to check error and return -EPROBE_DEFER.
> 
> If probe() doesn't attempt to validate that the/a DMA channel provider
> exists, then there's no guarantee that the DMA provider has probed
> before the DMA client driver either probes or executes any later code.

I'm not against defer probe, but dma_request_slave_channel might not be
the right function. Ideally dma channels are allocated at open() time
and dma controller loaded checking is at probe() time. So there's
better some function called check_dma_controller_loaded(), or we could
also use initcalls to let dma driver probe earlier.

This patch intend to be simple to only change the binding.

> So, you could quite easily end up with a situation where the serial
> driver's open fails because the DMA provider has not yet probed. This is
> exactly what deferred probe was intended to avoid. This reason alone is
> enough to make it worth fixing this.
> 
> > dma_request_slave_channel is better to be called as late as possible to
> > make better dynamic use of dma channels.
> 
> probe() could validate that the provider exists in a way that doesn't
> permanently hold ownership of the DMA channel. For example, request it
> then immediately release it, in the absence of a dedicated "validate my
> DMA providers exist" API.

It's ugly but may work. The current driver didn't check dma controller
loaded at probe time. Let's leave the task to another patch?

> 
> > Actually, I searched the kernel code, most drivers don't return EPROBE_DEFER
> > when dma_request_slave_channel failed. dma_request_slave_channel can
> > fail in many cases, and dma device not probed is only one case.
> 
> That sounds like a bug. If a DMA channel /is/ specified, it should be
> possible to acquire it. If no DMA channel is specified, and if the
> device can do PIO, then it's reasonable to continue.
Idealy when dma_request_slave_channel failed at open time, it can also
move to PIO.

Thanks
Richard
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren - Aug. 1, 2013, 3:45 a.m.
On 07/31/2013 09:30 PM, Richard Zhao wrote:
> On Thu, Aug 01, 2013 at 05:32:37AM +0800, Stephen Warren wrote:
>> On 07/30/2013 09:45 PM, Richard Zhao wrote:
>>> On Wed, Jul 31, 2013 at 12:46:42AM +0800, Stephen Warren wrote:
>>>> On 07/29/2013 09:31 PM, Richard Zhao wrote:
>>>>> On Sat, Jul 27, 2013 at 03:39:15AM +0800, Stephen Warren wrote:
>>>>>> On 07/23/2013 10:09 PM, Richard Zhao wrote:
>>>>>>> - driver: remove use of nvidia,dma-request-selector
>>>>>>> 	  use dma_request_slave_channel to request channel
>>>>>>> - update binding doc
>>>>>>
>>>>>> This patch needs to be amended so that the DMA channel is looked up
>>>>>> during probe() rather than open(), to guarantee that deferred probe
>>>>>> works. It's possible to hold off probe, but not possible AFAIK to hold
>>>>>> off opening the port.
>>>>>
>>>>> How about avoid return -EPROBE_DEFER at open() time? It can be in
>>>>> another patch.
>>>>
>>>> I don't really understand what you mean. -EPROBE_DEFER is something that
>>>> only makes sense for probe() to return (or functions used by probe()).
>>>> It doesn't make sense to return it anywhere else, since probe deferral
>>>> is something that can only happen for probe().
>>>
>>> What I meant is, it might not worth to move request channel to probe
>>> only because we need to check error and return -EPROBE_DEFER.
>>
>> If probe() doesn't attempt to validate that the/a DMA channel provider
>> exists, then there's no guarantee that the DMA provider has probed
>> before the DMA client driver either probes or executes any later code.
> 
> I'm not against defer probe, but dma_request_slave_channel might not be
> the right function. Ideally dma channels are allocated at open() time
> and dma controller loaded checking is at probe() time. So there's
> better some function called check_dma_controller_loaded(), or we could
> also use initcalls to let dma driver probe earlier.
> 
> This patch intend to be simple to only change the binding.
> 
>> So, you could quite easily end up with a situation where the serial
>> driver's open fails because the DMA provider has not yet probed. This is
>> exactly what deferred probe was intended to avoid. This reason alone is
>> enough to make it worth fixing this.
>>
>>> dma_request_slave_channel is better to be called as late as possible to
>>> make better dynamic use of dma channels.
>>
>> probe() could validate that the provider exists in a way that doesn't
>> permanently hold ownership of the DMA channel. For example, request it
>> then immediately release it, in the absence of a dedicated "validate my
>> DMA providers exist" API.
> 
> It's ugly but may work. The current driver didn't check dma controller
> loaded at probe time. Let's leave the task to another patch?

I suppose it's fine to solve this later, although it seems quite
dangerous. You'd need to undo all the changes that alter where the DMA
channels are acquired in order to satisfy your argument though.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/devicetree/bindings/serial/nvidia,tegra20-hsuart.txt b/Documentation/devicetree/bindings/serial/nvidia,tegra20-hsuart.txt
index 392a449..1ed2f48 100644
--- a/Documentation/devicetree/bindings/serial/nvidia,tegra20-hsuart.txt
+++ b/Documentation/devicetree/bindings/serial/nvidia,tegra20-hsuart.txt
@@ -4,8 +4,9 @@  Required properties:
 - compatible : should be "nvidia,tegra30-hsuart", "nvidia,tegra20-hsuart".
 - reg: Should contain UART controller registers location and length.
 - interrupts: Should contain UART controller interrupts.
-- nvidia,dma-request-selector : The Tegra DMA controller's phandle and
-  request selector for this UART controller.
+- dmas : The Tegra DMA controller's phandle and request selector for
+  this UART controller.
+- dma-names : Should be "rx-tx";
 
 Optional properties:
 - nvidia,enable-modem-interrupt: Enable modem interrupts. Should be enable
@@ -18,7 +19,8 @@  serial@70006000 {
 	reg = <0x70006000 0x40>;
 	reg-shift = <2>;
 	interrupts = <0 36 0x04>;
-	nvidia,dma-request-selector = <&apbdma 8>;
+	dmas = <&apbdma 8>;
+	dma-names = "rx-tx";
 	nvidia,enable-modem-interrupt;
 	status = "disabled";
 };
diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index ee7c812..c8a7828 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -120,7 +120,6 @@  struct tegra_uart_port {
 	bool					rx_timeout;
 	int					rx_in_progress;
 	int					symb_bit;
-	int					dma_req_sel;
 
 	struct dma_chan				*rx_dma_chan;
 	struct dma_chan				*tx_dma_chan;
@@ -902,11 +901,8 @@  static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
 	dma_addr_t dma_phys;
 	int ret;
 	struct dma_slave_config dma_sconfig;
-	dma_cap_mask_t mask;
 
-	dma_cap_zero(mask);
-	dma_cap_set(DMA_SLAVE, mask);
-	dma_chan = dma_request_channel(mask, NULL, NULL);
+	dma_chan = dma_request_slave_channel(tup->uport.dev, "rx-tx");
 	if (!dma_chan) {
 		dev_err(tup->uport.dev,
 			"Dma channel is not available, will try later\n");
@@ -930,7 +926,6 @@  static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
 		dma_buf = tup->uport.state->xmit.buf;
 	}
 
-	dma_sconfig.slave_id = tup->dma_req_sel;
 	if (dma_to_memory) {
 		dma_sconfig.src_addr = tup->uport.mapbase;
 		dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
@@ -1214,17 +1209,8 @@  static int tegra_uart_parse_dt(struct platform_device *pdev,
 	struct tegra_uart_port *tup)
 {
 	struct device_node *np = pdev->dev.of_node;
-	u32 of_dma[2];
 	int port;
 
-	if (of_property_read_u32_array(np, "nvidia,dma-request-selector",
-				of_dma, 2) >= 0) {
-		tup->dma_req_sel = of_dma[1];
-	} else {
-		dev_err(&pdev->dev, "missing dma requestor in device tree\n");
-		return -EINVAL;
-	}
-
 	port = of_alias_get_id(np, "serial");
 	if (port < 0) {
 		dev_err(&pdev->dev, "failed to get alias id, errno %d\n", port);