| Message ID | 1399977032-26469-3-git-send-email-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Peter, On Tuesday 13 May 2014 04:00 PM, Peter Ujfalusi wrote: > From CCCFG register of eDMA3 we can get all the needed information for the > driver about the IP: > Number of channels: NUM_DMACH > Number of regions: NUM_REGN > Number of slots (PaRAM sets): NUM_PAENTRY > Number of TC/EQ: NUM_EVQUE > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > --- > arch/arm/common/edma.c | 128 ++++++++++++++++++++++++++++++------------------- > 1 file changed, 79 insertions(+), 49 deletions(-) > > diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c > index fade9ada81f8..1a98f3cd4cd9 100644 > --- a/arch/arm/common/edma.c > +++ b/arch/arm/common/edma.c > @@ -102,7 +102,16 @@ > #define PARM_OFFSET(param_no) (EDMA_PARM + ((param_no) << 5)) > > #define EDMA_DCHMAP 0x0100 /* 64 registers */ > -#define CHMAP_EXIST BIT(24) > + > +/* CCCFG register */ > +#define GET_NUM_DMACH(x) (x & 0x7) /* bits 0-2 */ > +#define GET_NUM_QDMACH(x) ((x & 0x70) >> 4) /* bits 4-6 */ > +#define GET_NUM_INTCH(x) ((x & 0x700) >> 8) /* bits 8-10 */ > +#define GET_NUM_PAENTRY(x) ((x & 0x7000) >> 12) /* bits 12-14 */ > +#define GET_NUM_EVQUE(x) ((x & 0x70000) >> 16) /* bits 16-18 */ > +#define GET_NUM_REGN(x) ((x & 0x300000) >> 20) /* bits 20-21 */ > +#define CHMAP_EXIST BIT(24) > +#define MP_EXIST BIT(25) Of these new defines, you do not use GET_NUM_QDMACH(), GET_NUM_INTCH() and MP_EXIST (at least in this patch). Can you please get rid of them if not needed? May be its just me but its pretty annoying to search for a define only to find its never used :) > > #define EDMA_MAX_DMACH 64 > #define EDMA_MAX_PARAMENTRY 512 > @@ -1415,6 +1424,68 @@ void edma_clear_event(unsigned channel) > } > EXPORT_SYMBOL(edma_clear_event); > > +static int edma_setup_info_from_hw(struct device *dev, > + struct edma_soc_info *pdata) > +{ > + int i; > + u32 value, cccfg, n_tc; > + s8 (*queue_tc_map)[2], (*queue_priority_map)[2]; > + > + /* Decode the eDMA3 configuration from CCCFG register */ > + cccfg = edma_read(0, EDMA_CCCFG); You do not handle the second controller here, but its pretty straight forward to add that by passing the controller number to this function. I was wondering why we never read the hardware for this information before, and strangely enough cannot find any SoC where the CCCFG register does not publish this information correctly. I have tested on DA850, DA830, DM365, DM355 and DM6467. Instead of keeping this specific to OF case, the code can be simplified much better if we read from hardware all the time. We can directly populate the equivalent variables in the internal structure 'struct edma' instead of populating them in 'struct edma_soc_info' and then copying then over. > + > + value = GET_NUM_DMACH(cccfg); > + pdata->n_channel = BIT(value + 1); > + > + value = GET_NUM_REGN(cccfg); > + pdata->n_region = BIT(value); > + > + value = GET_NUM_PAENTRY(cccfg); > + pdata->n_slot = BIT(value + 4); > + > + value = GET_NUM_EVQUE(cccfg); > + n_tc = value + 1; > + > + dev_dbg(dev, "eDMA3 HW configuration (cccfg: 0x%08x):\n", cccfg); > + dev_dbg(dev, "n_channel: %u\n", pdata->n_channel); > + dev_dbg(dev, "n_region: %u\n", pdata->n_region); > + dev_dbg(dev, "n_slot: %u\n", pdata->n_slot); > + dev_dbg(dev, "n_tc: %u\n", n_tc); > + [snip] > + pdata->n_cc = 1; > + > + queue_tc_map = devm_kzalloc(dev, (n_tc + 1) * sizeof(s8), GFP_KERNEL); > + if (!queue_tc_map) > + return -ENOMEM; > + > + for (i = 0; i < n_tc; i++) { > + queue_tc_map[i][0] = i; > + queue_tc_map[i][1] = i; > + } > + queue_tc_map[i][0] = -1; > + queue_tc_map[i][1] = -1; > + > + pdata->queue_tc_mapping = queue_tc_map; > + > + queue_priority_map = devm_kzalloc(dev, (n_tc + 1) * sizeof(s8), > + GFP_KERNEL); > + if (!queue_priority_map) > + return -ENOMEM; > + > + for (i = 0; i < n_tc; i++) { > + queue_priority_map[i][0] = i; > + queue_priority_map[i][1] = i; > + } > + queue_priority_map[i][0] = -1; > + queue_priority_map[i][1] = -1; > + > + pdata->queue_priority_mapping = queue_priority_map; > + > + pdata->default_queue = 0; This is part is not really setting up from hardware (rather falling back to some sane defaults for the DT case). Could you leave them in edma_of_parse_dt()? > @@ -1655,6 +1679,12 @@ static int edma_probe(struct platform_device *pdev) > if (IS_ERR(edmacc_regs_base[j])) > return PTR_ERR(edmacc_regs_base[j]); > > + if (node) { > + /* Get eDMA3 configuration from IP */ > + ret = edma_setup_info_from_hw(dev, info[j]); > + if (ret) > + return ret; No need to do this only for the DT case, I think. Also, once we get rid of the edma_soc_info dependency, just pass edma_cc[] directly edma_setup_info_from_hw(dev, j, edma_cc[j]); Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sekhar, On 05/15/2014 11:53 AM, Sekhar Nori wrote: > Hi Peter, > > On Tuesday 13 May 2014 04:00 PM, Peter Ujfalusi wrote: >> From CCCFG register of eDMA3 we can get all the needed information for the >> driver about the IP: >> Number of channels: NUM_DMACH >> Number of regions: NUM_REGN >> Number of slots (PaRAM sets): NUM_PAENTRY >> Number of TC/EQ: NUM_EVQUE >> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >> --- >> arch/arm/common/edma.c | 128 ++++++++++++++++++++++++++++++------------------- >> 1 file changed, 79 insertions(+), 49 deletions(-) >> >> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c >> index fade9ada81f8..1a98f3cd4cd9 100644 >> --- a/arch/arm/common/edma.c >> +++ b/arch/arm/common/edma.c >> @@ -102,7 +102,16 @@ >> #define PARM_OFFSET(param_no) (EDMA_PARM + ((param_no) << 5)) >> >> #define EDMA_DCHMAP 0x0100 /* 64 registers */ >> -#define CHMAP_EXIST BIT(24) >> + >> +/* CCCFG register */ >> +#define GET_NUM_DMACH(x) (x & 0x7) /* bits 0-2 */ >> +#define GET_NUM_QDMACH(x) ((x & 0x70) >> 4) /* bits 4-6 */ >> +#define GET_NUM_INTCH(x) ((x & 0x700) >> 8) /* bits 8-10 */ >> +#define GET_NUM_PAENTRY(x) ((x & 0x7000) >> 12) /* bits 12-14 */ >> +#define GET_NUM_EVQUE(x) ((x & 0x70000) >> 16) /* bits 16-18 */ >> +#define GET_NUM_REGN(x) ((x & 0x300000) >> 20) /* bits 20-21 */ >> +#define CHMAP_EXIST BIT(24) >> +#define MP_EXIST BIT(25) > > Of these new defines, you do not use GET_NUM_QDMACH(), GET_NUM_INTCH() > and MP_EXIST (at least in this patch). Can you please get rid of them if > not needed? May be its just me but its pretty annoying to search for a > define only to find its never used :) Sure, I can remove the ones we are not using in the code. I usually prefer to have full description if the register even if we only use one bit from the register. > >> >> #define EDMA_MAX_DMACH 64 >> #define EDMA_MAX_PARAMENTRY 512 >> @@ -1415,6 +1424,68 @@ void edma_clear_event(unsigned channel) >> } >> EXPORT_SYMBOL(edma_clear_event); >> >> +static int edma_setup_info_from_hw(struct device *dev, >> + struct edma_soc_info *pdata) >> +{ >> + int i; >> + u32 value, cccfg, n_tc; >> + s8 (*queue_tc_map)[2], (*queue_priority_map)[2]; >> + >> + /* Decode the eDMA3 configuration from CCCFG register */ >> + cccfg = edma_read(0, EDMA_CCCFG); > > You do not handle the second controller here, but its pretty straight > forward to add that by passing the controller number to this function. The second controller is not handled because in DT boot we only handle 1 cc as far as I know. I don't know why, but this is how the DT support has been written and used. > I was wondering why we never read the hardware for this information > before, and strangely enough cannot find any SoC where the CCCFG > register does not publish this information correctly. I have tested on > DA850, DA830, DM365, DM355 and DM6467. Good question. I was also puzzled about this. > Instead of keeping this specific to OF case, the code can be simplified > much better if we read from hardware all the time. We can directly > populate the equivalent variables in the internal structure 'struct > edma' instead of populating them in 'struct edma_soc_info' and then > copying then over. Yes, we can switch the non DT boot to this mode as well, true. At first I wanted to change code which I can test easily. For non DT boot I would need to set up my old daVinci board ;) >> + >> + value = GET_NUM_DMACH(cccfg); >> + pdata->n_channel = BIT(value + 1); >> + >> + value = GET_NUM_REGN(cccfg); >> + pdata->n_region = BIT(value); >> + >> + value = GET_NUM_PAENTRY(cccfg); >> + pdata->n_slot = BIT(value + 4); >> + >> + value = GET_NUM_EVQUE(cccfg); >> + n_tc = value + 1; >> + >> + dev_dbg(dev, "eDMA3 HW configuration (cccfg: 0x%08x):\n", cccfg); >> + dev_dbg(dev, "n_channel: %u\n", pdata->n_channel); >> + dev_dbg(dev, "n_region: %u\n", pdata->n_region); >> + dev_dbg(dev, "n_slot: %u\n", pdata->n_slot); >> + dev_dbg(dev, "n_tc: %u\n", n_tc); >> + > > [snip] > >> + pdata->n_cc = 1; >> + >> + queue_tc_map = devm_kzalloc(dev, (n_tc + 1) * sizeof(s8), GFP_KERNEL); >> + if (!queue_tc_map) >> + return -ENOMEM; >> + >> + for (i = 0; i < n_tc; i++) { >> + queue_tc_map[i][0] = i; >> + queue_tc_map[i][1] = i; >> + } >> + queue_tc_map[i][0] = -1; >> + queue_tc_map[i][1] = -1; >> + >> + pdata->queue_tc_mapping = queue_tc_map; >> + >> + queue_priority_map = devm_kzalloc(dev, (n_tc + 1) * sizeof(s8), >> + GFP_KERNEL); >> + if (!queue_priority_map) >> + return -ENOMEM; >> + >> + for (i = 0; i < n_tc; i++) { >> + queue_priority_map[i][0] = i; >> + queue_priority_map[i][1] = i; >> + } >> + queue_priority_map[i][0] = -1; >> + queue_priority_map[i][1] = -1; >> + >> + pdata->queue_priority_mapping = queue_priority_map; >> + >> + pdata->default_queue = 0; > > This is part is not really setting up from hardware (rather falling back > to some sane defaults for the DT case). Could you leave them in > edma_of_parse_dt()? Not really since the number of tc is not know as early as edma_of_parse_dt(), we used to a magic number of 3 in place for n_tc previously. We are doing similar things as previously done in edma_of_parse_dt() but with 'correct' number of tc. >> @@ -1655,6 +1679,12 @@ static int edma_probe(struct platform_device *pdev) >> if (IS_ERR(edmacc_regs_base[j])) >> return PTR_ERR(edmacc_regs_base[j]); >> >> + if (node) { >> + /* Get eDMA3 configuration from IP */ >> + ret = edma_setup_info_from_hw(dev, info[j]); >> + if (ret) >> + return ret; > > No need to do this only for the DT case, I think. Also, once we get rid > of the edma_soc_info dependency, just pass edma_cc[] directly > > edma_setup_info_from_hw(dev, j, edma_cc[j]); Yes, let's do this for DT and not DT boot as well. I will keep the queue_tc_map and queue_priority_map setup in there I think to be done in case of DT boot. I'll try to craft v3 as soon as I can. Thanks, Péter -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 15 May 2014 06:00 PM, Peter Ujfalusi wrote: > The second controller is not handled because in DT boot we only handle 1 cc as > far as I know. I don't know why, but this is how the DT support has been > written and used. Its just because none of the platforms under heavy development use two controllers. Joel promised to work on it at some point ;) > >> I was wondering why we never read the hardware for this information >> before, and strangely enough cannot find any SoC where the CCCFG >> register does not publish this information correctly. I have tested on >> DA850, DA830, DM365, DM355 and DM6467. > > Good question. I was also puzzled about this. > >> Instead of keeping this specific to OF case, the code can be simplified >> much better if we read from hardware all the time. We can directly >> populate the equivalent variables in the internal structure 'struct >> edma' instead of populating them in 'struct edma_soc_info' and then >> copying then over. > > Yes, we can switch the non DT boot to this mode as well, true. At first I > wanted to change code which I can test easily. For non DT boot I would need to > set up my old daVinci board ;) I can help testing (and even with writing the DaVinci platform specific patches). >>> + pdata->n_cc = 1; >>> + >>> + queue_tc_map = devm_kzalloc(dev, (n_tc + 1) * sizeof(s8), GFP_KERNEL); >>> + if (!queue_tc_map) >>> + return -ENOMEM; >>> + >>> + for (i = 0; i < n_tc; i++) { >>> + queue_tc_map[i][0] = i; >>> + queue_tc_map[i][1] = i; >>> + } >>> + queue_tc_map[i][0] = -1; >>> + queue_tc_map[i][1] = -1; >>> + >>> + pdata->queue_tc_mapping = queue_tc_map; >>> + >>> + queue_priority_map = devm_kzalloc(dev, (n_tc + 1) * sizeof(s8), >>> + GFP_KERNEL); >>> + if (!queue_priority_map) >>> + return -ENOMEM; >>> + >>> + for (i = 0; i < n_tc; i++) { >>> + queue_priority_map[i][0] = i; >>> + queue_priority_map[i][1] = i; >>> + } >>> + queue_priority_map[i][0] = -1; >>> + queue_priority_map[i][1] = -1; >>> + >>> + pdata->queue_priority_mapping = queue_priority_map; >>> + >>> + pdata->default_queue = 0; >> >> This is part is not really setting up from hardware (rather falling back >> to some sane defaults for the DT case). Could you leave them in >> edma_of_parse_dt()? > > Not really since the number of tc is not know as early as edma_of_parse_dt(), > we used to a magic number of 3 in place for n_tc previously. > We are doing similar things as previously done in edma_of_parse_dt() but with > 'correct' number of tc. Okay. I did not notice the n_tc hardcoding. In that case to make this function usable on non-DT case we will have to do something like: /* Default to 1 if nothing passed */ if (!pdata->n_cc) pdata->n_cc = 1; if (!pdata->queue_priority_mapping) { queue_priority_map = devm_kzalloc(...) } I was hoping we could avoid that. >>> @@ -1655,6 +1679,12 @@ static int edma_probe(struct platform_device *pdev) >>> if (IS_ERR(edmacc_regs_base[j])) >>> return PTR_ERR(edmacc_regs_base[j]); >>> >>> + if (node) { >>> + /* Get eDMA3 configuration from IP */ >>> + ret = edma_setup_info_from_hw(dev, info[j]); >>> + if (ret) >>> + return ret; >> >> No need to do this only for the DT case, I think. Also, once we get rid >> of the edma_soc_info dependency, just pass edma_cc[] directly >> >> edma_setup_info_from_hw(dev, j, edma_cc[j]); > > Yes, let's do this for DT and not DT boot as well. > I will keep the queue_tc_map and queue_priority_map setup in there I think to > be done in case of DT boot. Right. > > I'll try to craft v3 as soon as I can. Thanks. Regards, Sekhar -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/15/2014 07:48 AM, Sekhar Nori wrote: > On Thursday 15 May 2014 06:00 PM, Peter Ujfalusi wrote: > >> The second controller is not handled because in DT boot we only handle 1 cc as >> far as I know. I don't know why, but this is how the DT support has been >> written and used. > > Its just because none of the platforms under heavy development use two > controllers. Joel promised to work on it at some point ;) Yeah, often things like that go down in priority when there aren't users, and when other things on fire need to be tended to :-D I think the next on-fire thing as far as EDMA goes would be to move out into drivers/edma. I like Peter's series in that it probably is a step forward there. thanks, -Joel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c index fade9ada81f8..1a98f3cd4cd9 100644 --- a/arch/arm/common/edma.c +++ b/arch/arm/common/edma.c @@ -102,7 +102,16 @@ #define PARM_OFFSET(param_no) (EDMA_PARM + ((param_no) << 5)) #define EDMA_DCHMAP 0x0100 /* 64 registers */ -#define CHMAP_EXIST BIT(24) + +/* CCCFG register */ +#define GET_NUM_DMACH(x) (x & 0x7) /* bits 0-2 */ +#define GET_NUM_QDMACH(x) ((x & 0x70) >> 4) /* bits 4-6 */ +#define GET_NUM_INTCH(x) ((x & 0x700) >> 8) /* bits 8-10 */ +#define GET_NUM_PAENTRY(x) ((x & 0x7000) >> 12) /* bits 12-14 */ +#define GET_NUM_EVQUE(x) ((x & 0x70000) >> 16) /* bits 16-18 */ +#define GET_NUM_REGN(x) ((x & 0x300000) >> 20) /* bits 20-21 */ +#define CHMAP_EXIST BIT(24) +#define MP_EXIST BIT(25) #define EDMA_MAX_DMACH 64 #define EDMA_MAX_PARAMENTRY 512 @@ -1415,6 +1424,68 @@ void edma_clear_event(unsigned channel) } EXPORT_SYMBOL(edma_clear_event); +static int edma_setup_info_from_hw(struct device *dev, + struct edma_soc_info *pdata) +{ + int i; + u32 value, cccfg, n_tc; + s8 (*queue_tc_map)[2], (*queue_priority_map)[2]; + + /* Decode the eDMA3 configuration from CCCFG register */ + cccfg = edma_read(0, EDMA_CCCFG); + + value = GET_NUM_DMACH(cccfg); + pdata->n_channel = BIT(value + 1); + + value = GET_NUM_REGN(cccfg); + pdata->n_region = BIT(value); + + value = GET_NUM_PAENTRY(cccfg); + pdata->n_slot = BIT(value + 4); + + value = GET_NUM_EVQUE(cccfg); + n_tc = value + 1; + + dev_dbg(dev, "eDMA3 HW configuration (cccfg: 0x%08x):\n", cccfg); + dev_dbg(dev, "n_channel: %u\n", pdata->n_channel); + dev_dbg(dev, "n_region: %u\n", pdata->n_region); + dev_dbg(dev, "n_slot: %u\n", pdata->n_slot); + dev_dbg(dev, "n_tc: %u\n", n_tc); + + pdata->n_cc = 1; + + queue_tc_map = devm_kzalloc(dev, (n_tc + 1) * sizeof(s8), GFP_KERNEL); + if (!queue_tc_map) + return -ENOMEM; + + for (i = 0; i < n_tc; i++) { + queue_tc_map[i][0] = i; + queue_tc_map[i][1] = i; + } + queue_tc_map[i][0] = -1; + queue_tc_map[i][1] = -1; + + pdata->queue_tc_mapping = queue_tc_map; + + queue_priority_map = devm_kzalloc(dev, (n_tc + 1) * sizeof(s8), + GFP_KERNEL); + if (!queue_priority_map) + return -ENOMEM; + + for (i = 0; i < n_tc; i++) { + queue_priority_map[i][0] = i; + queue_priority_map[i][1] = i; + } + queue_priority_map[i][0] = -1; + queue_priority_map[i][1] = -1; + + pdata->queue_priority_mapping = queue_priority_map; + + pdata->default_queue = 0; + + return 0; +} + #if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_DMADEVICES) static int edma_of_read_u32_to_s16_array(const struct device_node *np, @@ -1483,63 +1554,16 @@ static int edma_of_parse_dt(struct device *dev, struct device_node *node, struct edma_soc_info *pdata) { - int ret = 0, i; - u32 value; + int ret = 0; struct property *prop; size_t sz; struct edma_rsv_info *rsv_info; - s8 (*queue_tc_map)[2], (*queue_priority_map)[2]; - - ret = of_property_read_u32(node, "dma-channels", &value); - if (ret < 0) - return ret; - pdata->n_channel = value; - - ret = of_property_read_u32(node, "ti,edma-regions", &value); - if (ret < 0) - return ret; - pdata->n_region = value; - - ret = of_property_read_u32(node, "ti,edma-slots", &value); - if (ret < 0) - return ret; - pdata->n_slot = value; - - pdata->n_cc = 1; rsv_info = devm_kzalloc(dev, sizeof(struct edma_rsv_info), GFP_KERNEL); if (!rsv_info) return -ENOMEM; pdata->rsv = rsv_info; - queue_tc_map = devm_kzalloc(dev, 8*sizeof(s8), GFP_KERNEL); - if (!queue_tc_map) - return -ENOMEM; - - for (i = 0; i < 3; i++) { - queue_tc_map[i][0] = i; - queue_tc_map[i][1] = i; - } - queue_tc_map[i][0] = -1; - queue_tc_map[i][1] = -1; - - pdata->queue_tc_mapping = queue_tc_map; - - queue_priority_map = devm_kzalloc(dev, 8*sizeof(s8), GFP_KERNEL); - if (!queue_priority_map) - return -ENOMEM; - - for (i = 0; i < 3; i++) { - queue_priority_map[i][0] = i; - queue_priority_map[i][1] = i; - } - queue_priority_map[i][0] = -1; - queue_priority_map[i][1] = -1; - - pdata->queue_priority_mapping = queue_priority_map; - - pdata->default_queue = 0; - prop = of_find_property(node, "ti,edma-xbar-event-map", &sz); if (prop) ret = edma_xbar_event_map(dev, node, pdata, sz); @@ -1655,6 +1679,12 @@ static int edma_probe(struct platform_device *pdev) if (IS_ERR(edmacc_regs_base[j])) return PTR_ERR(edmacc_regs_base[j]); + if (node) { + /* Get eDMA3 configuration from IP */ + ret = edma_setup_info_from_hw(dev, info[j]); + if (ret) + return ret; + } edma_cc[j] = devm_kzalloc(&pdev->dev, sizeof(struct edma), GFP_KERNEL); if (!edma_cc[j])
From CCCFG register of eDMA3 we can get all the needed information for the driver about the IP: Number of channels: NUM_DMACH Number of regions: NUM_REGN Number of slots (PaRAM sets): NUM_PAENTRY Number of TC/EQ: NUM_EVQUE Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- arch/arm/common/edma.c | 128 ++++++++++++++++++++++++++++++------------------- 1 file changed, 79 insertions(+), 49 deletions(-)