| Message ID | 5237314D.7080102@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday 16 September 2013 09:56 PM, Joel Fernandes wrote: > On 09/16/2013 06:48 AM, Sekhar Nori wrote: >> Hi Joel, >> >> On Saturday 14 September 2013 06:27 AM, Joel Fernandes wrote: >>> From: Joel Fernandes <joelf@ti.com> >>> Subject: [PATCH v4] ARM: EDMA: Fix clearing of unused list for DT DMA resources >>> >>> HWMOD removal for MMC is breaking edma_start as the events are being manually >>> triggered due to unused channel list not being clear. >>> >>> This patch fixes the issue, by reading the "dmas" property from the DT node if >>> it exists and clearing the bits in the unused channel list. For this purpose >>> we use the of_* helpers to parse the arguments in the dmas phandle list. >>> >>> Reviewed-by: Sekhar Nori <nsekhar@ti.com> >>> Reported-by: Balaji T K <balajitk@ti.com> >>> Cc: Pantel Antoniou <panto@antoniou-consulting.com> >>> Signed-off-by: Joel Fernandes <joelf@ti.com> >>> --- >>> Changes since v1, in v2 and v3: >>> - Reduced indentation of non-of case by returning from of-case >>> - Using of_* helpers for parsing >>> >>> Note: >>> This patch should go into the merge window as it is a critical bug fix. >> >> I still cannot find any users of edma in the device tree sources either >> in linux-next or linus/master. Why cannot this wait until v3.13? > > I understand this affects only DT users of EDMA. But I get so many private > reports of breakage due to this patch not being there that I think it will save > everyone a lot of pain, specially folks creating integration trees to have this > patch available by default. Well, I do agree that the current DT support for EDMA is incomplete without this patch even if there are no in-kernel users of it. I will try sending this for the next -rc if we get to the final version in time and after that its upto the upstreams to take it. >>> arch/arm/common/edma.c | 23 +++++++++++++++++++++-- >>> 1 file changed, 21 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c >>> index 39ad030..43c7b22 100644 >>> --- a/arch/arm/common/edma.c >>> +++ b/arch/arm/common/edma.c >>> @@ -560,14 +560,33 @@ static int reserve_contiguous_slots(int ctlr, unsigned int >>> id, >>> static int prepare_unused_channel_list(struct device *dev, void *data) >>> { >>> struct platform_device *pdev = to_platform_device(dev); >>> - int i, ctlr; >>> + int i, count, ctlr; >>> + struct of_phandle_args dma_spec; >>> >>> + if (dev->of_node) { >>> + count = of_property_count_strings(dev->of_node, "dma-names"); >>> + if (count < 0) >>> + return 0; >>> + for (i = 0; i < count; i++) { >>> + if (of_parse_phandle_with_args(dev->of_node, "dmas", >>> + "#dma-cells", i, >>> + &dma_spec)) >>> + continue; >> >> This will break for the case where devices on platform bus use non-EDMA >> dma controllers like SDMA or CPPI (DRA7x has both EDMA and SDMA on the >> same chip). You need to do an additional check to make sure the dma >> controller is indeed EDMA. Something like. > > Ok, edma is probed earlier so I could never see any problem. This has got nothing to do with edma probe order. > Thanks for pointing this out, > > Using the below method is more future-proof than using compatible literal > strings directly. The only problem is the matches table has to be defined > earlier in the sources. What do you think? > > if (!of_match_node(edma_of_ids, dma_spec.np) { > of_node_put(dma_spec.np); > continue; > } Looks fine to me. >> if(!of_device_is_compatible(dma_spec.np, "ti,edma3")) >> continue; >> >> Don forget to call of_node_put() on dma_spec.np (something that needs to >> be done even with your current code). > > Ok, will do. > > >>> + >>> + ctlr = EDMA_CTLR(dma_spec.args[0]); >>> + clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]), >>> + edma_cc[ctlr]->edma_unused); >> >> We don't support the second controller when using DT and the controller >> number is not really encoded in the argument to edma phandle. So just >> simplify this to: >> >> clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]), >> edma_cc[0]->edma_unused); > > I think let's not make that assumption just incase in the future we support more > than one EDMA controller for DT-based boot. Is that ok? We don't write future proof code in that _hope_ that it will get used someday. In fact this will confuse the reader into wondering if support for second channel controller is present or not as the code will send mixed messages. In short, we aim for consistency with situation today, not tomorrow. Besides, I can bet that when second CC support does get added, it is very unlikely that the CC number is get encoded into channel number when using DT. Thanks, Sekhar
On 09/17/2013 12:08 AM, Sekhar Nori wrote: [..] >>> I still cannot find any users of edma in the device tree sources either >>> in linux-next or linus/master. Why cannot this wait until v3.13? >> >> I understand this affects only DT users of EDMA. But I get so many private >> reports of breakage due to this patch not being there that I think it will save >> everyone a lot of pain, specially folks creating integration trees to have this >> patch available by default. > > Well, I do agree that the current DT support for EDMA is incomplete > without this patch even if there are no in-kernel users of it. I will > try sending this for the next -rc if we get to the final version in time > and after that its upto the upstreams to take it. Ok, except for the one minor nit below my last scissor patch is good to go. >>>> + >>>> + ctlr = EDMA_CTLR(dma_spec.args[0]); >>>> + clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]), >>>> + edma_cc[ctlr]->edma_unused); >>> >>> We don't support the second controller when using DT and the controller >>> number is not really encoded in the argument to edma phandle. So just >>> simplify this to: >>> >>> clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]), >>> edma_cc[0]->edma_unused); >> >> I think let's not make that assumption just incase in the future we support more >> than one EDMA controller for DT-based boot. Is that ok? > > We don't write future proof code in that _hope_ that it will get used > someday. In fact this will confuse the reader into wondering if support > for second channel controller is present or not as the code will send Nope I don't agree with this at all.. EDMA CC ctrl will not be hardcoded. We need to write future-proof code to make sure sudden regressions don't pop up when say a second EDMA CC is introduced.. further edma_cc[ctrl] pattern is used all through out the code so what you're asking to do doesn't make much sense in this context. There's no reason to break out of this pattern. It actually will confuse the reader more. Second controller can be present in future. I don't want to come back to change the code when we introduce more than 1 CC which is possible in the future. > mixed messages. In short, we aim for consistency with situation today, > not tomorrow. What you're asking to do infact breaks consistency with the rest of the code. > > Besides, I can bet that when second CC support does get added, it is > very unlikely that the CC number is get encoded into channel number when > using DT. Even if it is not encoded, the data structure for edma_cc is an array and what you're asking is to hardcode the controller number to 0 always. No way I'm going to hard code controller number to a single value. Different topic but anyway why wouldn't ctrl number be encoded in the channel? That's clean, and saves variables and extra structures. Better use of the integer bitmap making up the Ctrl and channel number of small ranges. Regards, -Joel
On Tuesday 17 September 2013 11:08 AM, Joel Fernandes wrote: > On 09/17/2013 12:08 AM, Sekhar Nori wrote: > [..] >>>> I still cannot find any users of edma in the device tree sources either >>>> in linux-next or linus/master. Why cannot this wait until v3.13? >>> >>> I understand this affects only DT users of EDMA. But I get so many private >>> reports of breakage due to this patch not being there that I think it will save >>> everyone a lot of pain, specially folks creating integration trees to have this >>> patch available by default. >> >> Well, I do agree that the current DT support for EDMA is incomplete >> without this patch even if there are no in-kernel users of it. I will >> try sending this for the next -rc if we get to the final version in time >> and after that its upto the upstreams to take it. > > Ok, except for the one minor nit below my last scissor patch is good to go. > >>>>> + >>>>> + ctlr = EDMA_CTLR(dma_spec.args[0]); >>>>> + clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]), >>>>> + edma_cc[ctlr]->edma_unused); >>>> >>>> We don't support the second controller when using DT and the controller >>>> number is not really encoded in the argument to edma phandle. So just >>>> simplify this to: >>>> >>>> clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]), >>>> edma_cc[0]->edma_unused); >>> >>> I think let's not make that assumption just incase in the future we support more >>> than one EDMA controller for DT-based boot. Is that ok? >> >> We don't write future proof code in that _hope_ that it will get used >> someday. In fact this will confuse the reader into wondering if support >> for second channel controller is present or not as the code will send > > Nope I don't agree with this at all.. EDMA CC ctrl will not be hardcoded. We > need to write future-proof code to make sure sudden regressions don't pop up > when say a second EDMA CC is introduced.. further edma_cc[ctrl] pattern is used I am not sure why you call this a regression. There is no support for second channel controller. If someone wants to add that support there is a lot to worry about than just these two lines of code. So, no, there wont be "sudden regressions that pop up" like you say. Look at the probe routine. There is no way the second CC is going to get probed. > all through out the code so what you're asking to do doesn't make much sense in > this context. There's no reason to break out of this pattern. It actually will > confuse the reader more. Thats because that code is also used for existing platform data based approach. If you are adding code paths which just get exercised with DT, no point referring to the second channel controller since that does not exist. > > Second controller can be present in future. I don't want to come back to change > the code when we introduce more than 1 CC which is possible in the future. You *will* have to come back and change the code to support the second channel controller. That is true with or without this patch. So lets stop arguing like the second channel controller support is only blocked because of these two lines of code. > >> mixed messages. In short, we aim for consistency with situation today, >> not tomorrow. > > What you're asking to do infact breaks consistency with the rest of the code. Well, ideally we support second CC even with DT to be consistent all around. Since that has not happened, I don't want the code to pretend that it it supports the second channel controller with DT that too only in parts of code. > >> >> Besides, I can bet that when second CC support does get added, it is >> very unlikely that the CC number is get encoded into channel number when >> using DT. > > Even if it is not encoded, the data structure for edma_cc is an array and what > you're asking is to hardcode the controller number to 0 always. No way I'm going > to hard code controller number to a single value. You are missing the point. Look at the code. You are extracting controller number from dma_spec.args[0] which is directly the value present in DT. The binding today does not specify that the value be encoded with controller number. So I will have not have code that interprets it that way. The code follows from bindings, not the other way around. > > Different topic but anyway why wouldn't ctrl number be encoded in the channel? > That's clean, and saves variables and extra structures. Better use of the > integer bitmap making up the Ctrl and channel number of small ranges. Lets have this discussion when you update the bindings to support the second controller. Thanks, Sekhar
diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c index 117f955..8b5c6ed 100644 --- a/arch/arm/common/edma.c +++ b/arch/arm/common/edma.c @@ -269,6 +269,11 @@ static const struct edmacc_param dummy_paramset = { .ccnt = 1, }; +static const struct of_device_id edma_of_ids[] = { + { .compatible = "ti,edma3", }, + {} +}; + /*****************************************************************************/ static void map_dmach_queue(unsigned ctlr, unsigned ch_no, @@ -560,14 +565,39 @@ static int reserve_contiguous_slots(int ctlr, unsigned int id, static int prepare_unused_channel_list(struct device *dev, void *data) { struct platform_device *pdev = to_platform_device(dev); - int i, ctlr; + int i, count, ctlr; + struct of_phandle_args dma_spec; + if (dev->of_node) { + count = of_property_count_strings(dev->of_node, "dma-names"); + if (count < 0) + return 0; + for (i = 0; i < count; i++) { + if (of_parse_phandle_with_args(dev->of_node, "dmas", + "#dma-cells", i, + &dma_spec)) + continue; + + if (!of_match_node(edma_of_ids, dma_spec.np)) { + of_node_put(dma_spec.np); + continue; + } + + ctlr = EDMA_CTLR(dma_spec.args[0]); + clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]), + edma_cc[ctlr]->edma_unused); + of_node_put(dma_spec.np); + } + return 0; + } + + /* For non-OF case */ for (i = 0; i < pdev->num_resources; i++) { if ((pdev->resource[i].flags & IORESOURCE_DMA) && (int)pdev->resource[i].start >= 0) { ctlr = EDMA_CTLR(pdev->resource[i].start); clear_bit(EDMA_CHAN_SLOT(pdev->resource[i].start), - edma_cc[ctlr]->edma_unused); + edma_cc[ctlr]->edma_unused); } } @@ -1762,11 +1792,6 @@ static int edma_probe(struct platform_device *pdev) return 0; } -static const struct of_device_id edma_of_ids[] = { - { .compatible = "ti,edma3", }, - {} -}; - static struct platform_driver edma_driver = { .driver = { .name = "edma",