| Message ID | 1377287613-16491-1-git-send-email-joelf@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Saturday 24 August 2013 01:23 AM, Joel Fernandes wrote: > HWMOD removal for MMC and Crypto is breaking edma_start as the events are > being manually triggered due to unused channel list not being clear. Atleast > breakage has been seen on these peripherals, but it is expected Audio (McASP) > maybe breaking too. > > 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 so that these channels > are not manually triggered. > > v2 changes: > Reduced indendation by returning from if block. Is this a v2 or v3 since you already sent a v2 about a month back? > > 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> > --- > Note: > Patch should go in for -rc cycle as it fixes existing crypto drivers. We agreed the patch is not needed in -rc cycle since there are no current EDMA users in DT-boot? > > arch/arm/common/edma.c | 22 +++++++++++++++++++--- > 1 file changed, 19 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c > index 39ad030..3867e7e 100644 > --- a/arch/arm/common/edma.c > +++ b/arch/arm/common/edma.c > @@ -560,14 +560,30 @@ 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 = 0, ctlr; > + u32 dma_chan; > + const __be32 *dma_chan_p; > + struct property *prop; > + > + if (dev->of_node) { > + of_property_for_each_u32(dev->of_node, "dmas", prop, > + dma_chan_p, dma_chan) { > + if (i++ & 1) { > + ctlr = EDMA_CTLR(dma_chan); > + clear_bit(EDMA_CHAN_SLOT(dma_chan), > + edma_cc[ctlr]->edma_unused); > + } I thought we agreed to do this differently using of_property_count_strings() and of_parse_phandle_with_args(). I seemed to have missed any discussion on why this cannot be done (if such a discussion took place on the list). Thanks, Sekhar
On 08/26/2013 05:46 AM, Sekhar Nori wrote: > On Saturday 24 August 2013 01:23 AM, Joel Fernandes wrote: >> HWMOD removal for MMC and Crypto is breaking edma_start as the events are >> being manually triggered due to unused channel list not being clear. Atleast >> breakage has been seen on these peripherals, but it is expected Audio (McASP) >> maybe breaking too. >> >> 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 so that these channels >> are not manually triggered. >> >> v2 changes: >> Reduced indendation by returning from if block. > > Is this a v2 or v3 since you already sent a v2 about a month back? No, there aren't any changes since v2, I just resubmitted the same patch. >> >> 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> >> --- >> Note: >> Patch should go in for -rc cycle as it fixes existing crypto drivers. > > We agreed the patch is not needed in -rc cycle since there are no > current EDMA users in DT-boot? There is now, crypto and EDMA DT patches are being merged in. >> >> arch/arm/common/edma.c | 22 +++++++++++++++++++--- >> 1 file changed, 19 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c >> index 39ad030..3867e7e 100644 >> --- a/arch/arm/common/edma.c >> +++ b/arch/arm/common/edma.c >> @@ -560,14 +560,30 @@ 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 = 0, ctlr; >> + u32 dma_chan; >> + const __be32 *dma_chan_p; >> + struct property *prop; >> + >> + if (dev->of_node) { >> + of_property_for_each_u32(dev->of_node, "dmas", prop, >> + dma_chan_p, dma_chan) { >> + if (i++ & 1) { >> + ctlr = EDMA_CTLR(dma_chan); >> + clear_bit(EDMA_CHAN_SLOT(dma_chan), >> + edma_cc[ctlr]->edma_unused); >> + } > > I thought we agreed to do this differently using > of_property_count_strings() and of_parse_phandle_with_args(). I seemed > to have missed any discussion on why this cannot be done (if such a > discussion took place on the list). I kind of missed that particular review comment after reading [1]. Because I thought only change left was the indentation. Let me work on that comment and resubmit as v3. Regards, -Joel
On 8/26/2013 10:22 PM, Joel Fernandes wrote: > On 08/26/2013 05:46 AM, Sekhar Nori wrote: >> On Saturday 24 August 2013 01:23 AM, Joel Fernandes wrote: >>> HWMOD removal for MMC and Crypto is breaking edma_start as the events are >>> being manually triggered due to unused channel list not being clear. Atleast >>> breakage has been seen on these peripherals, but it is expected Audio (McASP) >>> maybe breaking too. >>> >>> 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 so that these channels >>> are not manually triggered. >>> >>> v2 changes: >>> Reduced indendation by returning from if block. >> >> Is this a v2 or v3 since you already sent a v2 about a month back? > > No, there aren't any changes since v2, I just resubmitted the same patch. Heh, it does seem to contain some indentation changes per your documentation :) > >>> >>> 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> >>> --- >>> Note: >>> Patch should go in for -rc cycle as it fixes existing crypto drivers. >> >> We agreed the patch is not needed in -rc cycle since there are no >> current EDMA users in DT-boot? > > There is now, crypto and EDMA DT patches are being merged in. They are being merged in v3.12, right? So you meant the -rc cycle of v3.12? If yes, no need to wait for a broken v3.12-rc1 to come out. It is possible to queue fixes during merge window too. Thanks, Sekhar
On 23/08/13 20:53, Joel Fernandes wrote: > HWMOD removal for MMC and Crypto is breaking edma_start as the events are > being manually triggered due to unused channel list not being clear. Atleast > breakage has been seen on these peripherals, but it is expected Audio (McASP) > maybe breaking too. > > 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 so that these channels > are not manually triggered. > > v2 changes: > Reduced indendation by returning from if block. > > 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> > --- > Note: > Patch should go in for -rc cycle as it fixes existing crypto drivers. > > arch/arm/common/edma.c | 22 +++++++++++++++++++--- > 1 file changed, 19 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c > index 39ad030..3867e7e 100644 > --- a/arch/arm/common/edma.c > +++ b/arch/arm/common/edma.c > @@ -560,14 +560,30 @@ 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 = 0, ctlr; > + u32 dma_chan; > + const __be32 *dma_chan_p; > + struct property *prop; > + > + if (dev->of_node) { > + of_property_for_each_u32(dev->of_node, "dmas", prop, > + dma_chan_p, dma_chan) { > + if (i++ & 1) { > + ctlr = EDMA_CTLR(dma_chan); > + clear_bit(EDMA_CHAN_SLOT(dma_chan), > + edma_cc[ctlr]->edma_unused); > + } > + } > + return; This should return something here, otherwise we get:- arch/arm/common/edma.c: In function 'prepare_unused_channel_list': arch/arm/common/edma.c:577:3: warning: 'return' with no value, in function returning non-void [-Wreturn-type] Mark J. > + } > > - for (i = 0; i < pdev->num_resources; i++) { > + /* For non-OF case */ > + for (; 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); > } > }
On 06/09/13 20:13, Mark Jackson wrote: > On 23/08/13 20:53, Joel Fernandes wrote: >> HWMOD removal for MMC and Crypto is breaking edma_start as the events are >> being manually triggered due to unused channel list not being clear. Atleast >> breakage has been seen on these peripherals, but it is expected Audio (McASP) >> maybe breaking too. >> >> 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 so that these channels >> are not manually triggered. >> >> v2 changes: >> Reduced indendation by returning from if block. >> >> 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> >> --- >> Note: >> Patch should go in for -rc cycle as it fixes existing crypto drivers. >> >> arch/arm/common/edma.c | 22 +++++++++++++++++++--- >> 1 file changed, 19 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c >> index 39ad030..3867e7e 100644 >> --- a/arch/arm/common/edma.c >> +++ b/arch/arm/common/edma.c >> @@ -560,14 +560,30 @@ 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 = 0, ctlr; >> + u32 dma_chan; >> + const __be32 *dma_chan_p; >> + struct property *prop; >> + >> + if (dev->of_node) { >> + of_property_for_each_u32(dev->of_node, "dmas", prop, >> + dma_chan_p, dma_chan) { >> + if (i++ & 1) { >> + ctlr = EDMA_CTLR(dma_chan); >> + clear_bit(EDMA_CHAN_SLOT(dma_chan), >> + edma_cc[ctlr]->edma_unused); >> + } >> + } >> + return; > > This should return something here, otherwise we get:- > > arch/arm/common/edma.c: In function 'prepare_unused_channel_list': > arch/arm/common/edma.c:577:3: warning: 'return' with no value, in function returning non-void [-Wreturn-type] Other than that I can confirm it fixes the issue for me ... Tested-by: Mark Jackson <mpfj@newflow.co.uk>
On 09/06/2013 02:15 PM, Mark Jackson wrote: > On 06/09/13 20:13, Mark Jackson wrote: >> On 23/08/13 20:53, Joel Fernandes wrote: >>> HWMOD removal for MMC and Crypto is breaking edma_start as the events are >>> being manually triggered due to unused channel list not being clear. Atleast >>> breakage has been seen on these peripherals, but it is expected Audio (McASP) >>> maybe breaking too. >>> >>> 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 so that these channels >>> are not manually triggered. >>> >>> v2 changes: >>> Reduced indendation by returning from if block. >>> >>> 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> >>> --- >>> Note: >>> Patch should go in for -rc cycle as it fixes existing crypto drivers. >>> >>> arch/arm/common/edma.c | 22 +++++++++++++++++++--- >>> 1 file changed, 19 insertions(+), 3 deletions(-) >>> >>> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c >>> index 39ad030..3867e7e 100644 >>> --- a/arch/arm/common/edma.c >>> +++ b/arch/arm/common/edma.c >>> @@ -560,14 +560,30 @@ 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 = 0, ctlr; >>> + u32 dma_chan; >>> + const __be32 *dma_chan_p; >>> + struct property *prop; >>> + >>> + if (dev->of_node) { >>> + of_property_for_each_u32(dev->of_node, "dmas", prop, >>> + dma_chan_p, dma_chan) { >>> + if (i++ & 1) { >>> + ctlr = EDMA_CTLR(dma_chan); >>> + clear_bit(EDMA_CHAN_SLOT(dma_chan), >>> + edma_cc[ctlr]->edma_unused); >>> + } >>> + } >>> + return; >> >> This should return something here, otherwise we get:- >> >> arch/arm/common/edma.c: In function 'prepare_unused_channel_list': >> arch/arm/common/edma.c:577:3: warning: 'return' with no value, in function returning non-void [-Wreturn-type] > > Other than that I can confirm it fixes the issue for me ... Thanks for pointing that out. Will fix it in the next revision. Regards, -Joel
diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c index 39ad030..3867e7e 100644 --- a/arch/arm/common/edma.c +++ b/arch/arm/common/edma.c @@ -560,14 +560,30 @@ 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 = 0, ctlr; + u32 dma_chan; + const __be32 *dma_chan_p; + struct property *prop; + + if (dev->of_node) { + of_property_for_each_u32(dev->of_node, "dmas", prop, + dma_chan_p, dma_chan) { + if (i++ & 1) { + ctlr = EDMA_CTLR(dma_chan); + clear_bit(EDMA_CHAN_SLOT(dma_chan), + edma_cc[ctlr]->edma_unused); + } + } + return; + } - for (i = 0; i < pdev->num_resources; i++) { + /* For non-OF case */ + for (; 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); } }