Re: [PATCH] net: ethernet: ti: davinci_cpdma: fix fixed prio cpdma ctlr configuration
On 11/10/2016 01:52 PM, ivan.khoronzhuk wrote: On 10.11.16 18:37, Grygorii Strashko wrote: On 11/09/2016 05:56 PM, Ivan Khoronzhuk wrote: On 09.11.16 23:09, Grygorii Strashko wrote: On 11/08/2016 07:10 AM, Ivan Khoronzhuk wrote: The dma ctlr is reseted to 0 while cpdma start, thus cpdma ctlr I assume this is because cpdma_ctlr_start() does soft reset. Is it correct? Probably not. I've seen this register doesn't hold any previous settings (just trash) What register CPDMA_DMACONTROL or CPSW_DMACTRL? after cpdma_ctlr_stop(), actually after last channel is stopped (inside of cpdma_ctlr_stop()). So, You are stating that Registers context is changed after stop? Then cpdma_ctlr_start() just reset it to 0. "just trash" or "0". Sry, I do not see how cpdma_ctlr_stop() can affect on registers state :( and I'd very appreciated if can provide more detailed information. I've checked again, it was my mistake. It simply reset to 0 while soft reset. cannot be configured after cpdma is stopped. So, restore content of cpdma ctlr while off/on procedure. Based on net-next/master ^ remove it sure Signed-off-by: Ivan Khoronzhuk--- drivers/net/ethernet/ti/cpsw.c | 6 +- drivers/net/ethernet/ti/davinci_cpdma.c | 103 +--- drivers/net/ethernet/ti/davinci_cpdma.h | 2 + 3 files changed, 58 insertions(+), 53 deletions(-) diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c index b1ddf89..4d04b8e 100644 --- a/drivers/net/ethernet/ti/cpsw.c +++ b/drivers/net/ethernet/ti/cpsw.c @@ -1376,10 +1376,6 @@ static int cpsw_ndo_open(struct net_device *ndev) ALE_ALL_PORTS, ALE_ALL_PORTS, 0, 0); if (!cpsw_common_res_usage_state(cpsw)) { -/* setup tx dma to fixed prio and zero offset */ -cpdma_control_set(cpsw->dma, CPDMA_TX_PRIO_FIXED, 1); -cpdma_control_set(cpsw->dma, CPDMA_RX_BUFFER_OFFSET, 0); - /* disable priority elevation */ __raw_writel(0, >regs->ptype); @@ -2710,6 +2706,8 @@ static int cpsw_probe(struct platform_device *pdev) dma_params.desc_align= 16; dma_params.has_ext_regs= true; dma_params.desc_hw_addr = dma_params.desc_mem_phys; +dma_params.rxbuf_offset= 0; +dma_params.fixed_prio= 1; Do we really need this new parameters? Do you have plans to use other values? I'm ok if this is static (equally as a bunch of rest in dma_params), no see reason to use other values, That's what i wanted to know :) - go static, pls. it at least now. But the issue is not only in these two parameters and not only in cpsw_ndo_open(). It touches cpsw_set_channels() also, where ctlr stop/start is present. In order to not copy cpdma_control_set(cpsw->dma, CPDMA_TX_PRIO_FIXED, 1)... in all such kind places in eth drivers, better to allow the cpdma to control it's parameters... The cpdma ctlr register holds a little more parameters (but only two of them are set in cpsw) Maybe there is reason to save them also. Actually I'd seen this problem when playing with on/off channel shapers, unfortunately the cpdma ctlr holds this info also, and it was lost while on/off (but I'm going to restore it in chan_start()). I understand you change, but I'm note sure about real root cause :( so, are you Ok with current version? No. pls, use constant values and do not introduce additional parameters. Also, pls update commit message with new information. -- regards, -grygorii
Re: [PATCH] net: ethernet: ti: davinci_cpdma: fix fixed prio cpdma ctlr configuration
On 10.11.16 18:37, Grygorii Strashko wrote: On 11/09/2016 05:56 PM, Ivan Khoronzhuk wrote: On 09.11.16 23:09, Grygorii Strashko wrote: On 11/08/2016 07:10 AM, Ivan Khoronzhuk wrote: The dma ctlr is reseted to 0 while cpdma start, thus cpdma ctlr I assume this is because cpdma_ctlr_start() does soft reset. Is it correct? Probably not. I've seen this register doesn't hold any previous settings (just trash) What register CPDMA_DMACONTROL or CPSW_DMACTRL? after cpdma_ctlr_stop(), actually after last channel is stopped (inside of cpdma_ctlr_stop()). So, You are stating that Registers context is changed after stop? Then cpdma_ctlr_start() just reset it to 0. "just trash" or "0". Sry, I do not see how cpdma_ctlr_stop() can affect on registers state :( and I'd very appreciated if can provide more detailed information. I've checked again, it was my mistake. It simply reset to 0 while soft reset. cannot be configured after cpdma is stopped. So, restore content of cpdma ctlr while off/on procedure. Based on net-next/master ^ remove it sure Signed-off-by: Ivan Khoronzhuk--- drivers/net/ethernet/ti/cpsw.c | 6 +- drivers/net/ethernet/ti/davinci_cpdma.c | 103 +--- drivers/net/ethernet/ti/davinci_cpdma.h | 2 + 3 files changed, 58 insertions(+), 53 deletions(-) diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c index b1ddf89..4d04b8e 100644 --- a/drivers/net/ethernet/ti/cpsw.c +++ b/drivers/net/ethernet/ti/cpsw.c @@ -1376,10 +1376,6 @@ static int cpsw_ndo_open(struct net_device *ndev) ALE_ALL_PORTS, ALE_ALL_PORTS, 0, 0); if (!cpsw_common_res_usage_state(cpsw)) { -/* setup tx dma to fixed prio and zero offset */ -cpdma_control_set(cpsw->dma, CPDMA_TX_PRIO_FIXED, 1); -cpdma_control_set(cpsw->dma, CPDMA_RX_BUFFER_OFFSET, 0); - /* disable priority elevation */ __raw_writel(0, >regs->ptype); @@ -2710,6 +2706,8 @@ static int cpsw_probe(struct platform_device *pdev) dma_params.desc_align= 16; dma_params.has_ext_regs= true; dma_params.desc_hw_addr = dma_params.desc_mem_phys; +dma_params.rxbuf_offset= 0; +dma_params.fixed_prio= 1; Do we really need this new parameters? Do you have plans to use other values? I'm ok if this is static (equally as a bunch of rest in dma_params), no see reason to use other values, That's what i wanted to know :) - go static, pls. it at least now. But the issue is not only in these two parameters and not only in cpsw_ndo_open(). It touches cpsw_set_channels() also, where ctlr stop/start is present. In order to not copy cpdma_control_set(cpsw->dma, CPDMA_TX_PRIO_FIXED, 1)... in all such kind places in eth drivers, better to allow the cpdma to control it's parameters... The cpdma ctlr register holds a little more parameters (but only two of them are set in cpsw) Maybe there is reason to save them also. Actually I'd seen this problem when playing with on/off channel shapers, unfortunately the cpdma ctlr holds this info also, and it was lost while on/off (but I'm going to restore it in chan_start()). I understand you change, but I'm note sure about real root cause :( so, are you Ok with current version?
Re: [PATCH] net: ethernet: ti: davinci_cpdma: fix fixed prio cpdma ctlr configuration
On 11/09/2016 05:56 PM, Ivan Khoronzhuk wrote: > > > On 09.11.16 23:09, Grygorii Strashko wrote: >> >> >> On 11/08/2016 07:10 AM, Ivan Khoronzhuk wrote: >>> The dma ctlr is reseted to 0 while cpdma start, thus cpdma ctlr >> >> I assume this is because cpdma_ctlr_start() does soft reset. Is it >> correct? > Probably not. I've seen this register doesn't hold any previous settings > (just trash) What register CPDMA_DMACONTROL or CPSW_DMACTRL? > after cpdma_ctlr_stop(), actually after last channel is stopped (inside > of cpdma_ctlr_stop()). So, You are stating that Registers context is changed after stop? > Then cpdma_ctlr_start() just reset it to 0. "just trash" or "0". Sry, I do not see how cpdma_ctlr_stop() can affect on registers state :( and I'd very appreciated if can provide more detailed information. > >> >>> cannot be configured after cpdma is stopped. So, restore content >>> of cpdma ctlr while off/on procedure. >>> >>> Based on net-next/master >> >> ^ remove it > sure > >> >>> >>> Signed-off-by: Ivan Khoronzhuk>>> --- >>> drivers/net/ethernet/ti/cpsw.c | 6 +- >>> drivers/net/ethernet/ti/davinci_cpdma.c | 103 >>> +--- >>> drivers/net/ethernet/ti/davinci_cpdma.h | 2 + >>> 3 files changed, 58 insertions(+), 53 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/ti/cpsw.c >>> b/drivers/net/ethernet/ti/cpsw.c >>> index b1ddf89..4d04b8e 100644 >>> --- a/drivers/net/ethernet/ti/cpsw.c >>> +++ b/drivers/net/ethernet/ti/cpsw.c >>> @@ -1376,10 +1376,6 @@ static int cpsw_ndo_open(struct net_device *ndev) >>>ALE_ALL_PORTS, ALE_ALL_PORTS, 0, 0); >>> >>> if (!cpsw_common_res_usage_state(cpsw)) { >>> -/* setup tx dma to fixed prio and zero offset */ >>> -cpdma_control_set(cpsw->dma, CPDMA_TX_PRIO_FIXED, 1); >>> -cpdma_control_set(cpsw->dma, CPDMA_RX_BUFFER_OFFSET, 0); >>> - >>> /* disable priority elevation */ >>> __raw_writel(0, >regs->ptype); >>> >>> @@ -2710,6 +2706,8 @@ static int cpsw_probe(struct platform_device >>> *pdev) >>> dma_params.desc_align= 16; >>> dma_params.has_ext_regs= true; >>> dma_params.desc_hw_addr = dma_params.desc_mem_phys; >>> +dma_params.rxbuf_offset= 0; >>> +dma_params.fixed_prio= 1; >> >> Do we really need this new parameters? Do you have plans to use other >> values? > I'm ok if this is static (equally as a bunch of rest in dma_params), no > see reason to use other values, That's what i wanted to know :) - go static, pls. > it at least now. But the issue is not only in these two parameters and > not only in cpsw_ndo_open(). > It touches cpsw_set_channels() also, where ctlr stop/start is present. > In order to not copy cpdma_control_set(cpsw->dma, CPDMA_TX_PRIO_FIXED, > 1)... > in all such kind places in eth drivers, better to allow the cpdma to > control it's parameters... > > The cpdma ctlr register holds a little more parameters (but only two of > them are set in cpsw) > Maybe there is reason to save them also. Actually I'd seen this problem > when playing with > on/off channel shapers, unfortunately the cpdma ctlr holds this info > also, and it was lost > while on/off (but I'm going to restore it in chan_start()). > I understand you change, but I'm note sure about real root cause :( -- regards, -grygorii
Re: [PATCH] net: ethernet: ti: davinci_cpdma: fix fixed prio cpdma ctlr configuration
On 09.11.16 23:09, Grygorii Strashko wrote: On 11/08/2016 07:10 AM, Ivan Khoronzhuk wrote: The dma ctlr is reseted to 0 while cpdma start, thus cpdma ctlr I assume this is because cpdma_ctlr_start() does soft reset. Is it correct? Probably not. I've seen this register doesn't hold any previous settings (just trash) after cpdma_ctlr_stop(), actually after last channel is stopped (inside of cpdma_ctlr_stop()). Then cpdma_ctlr_start() just reset it to 0. cannot be configured after cpdma is stopped. So, restore content of cpdma ctlr while off/on procedure. Based on net-next/master ^ remove it sure Signed-off-by: Ivan Khoronzhuk--- drivers/net/ethernet/ti/cpsw.c | 6 +- drivers/net/ethernet/ti/davinci_cpdma.c | 103 +--- drivers/net/ethernet/ti/davinci_cpdma.h | 2 + 3 files changed, 58 insertions(+), 53 deletions(-) diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c index b1ddf89..4d04b8e 100644 --- a/drivers/net/ethernet/ti/cpsw.c +++ b/drivers/net/ethernet/ti/cpsw.c @@ -1376,10 +1376,6 @@ static int cpsw_ndo_open(struct net_device *ndev) ALE_ALL_PORTS, ALE_ALL_PORTS, 0, 0); if (!cpsw_common_res_usage_state(cpsw)) { - /* setup tx dma to fixed prio and zero offset */ - cpdma_control_set(cpsw->dma, CPDMA_TX_PRIO_FIXED, 1); - cpdma_control_set(cpsw->dma, CPDMA_RX_BUFFER_OFFSET, 0); - /* disable priority elevation */ __raw_writel(0, >regs->ptype); @@ -2710,6 +2706,8 @@ static int cpsw_probe(struct platform_device *pdev) dma_params.desc_align = 16; dma_params.has_ext_regs = true; dma_params.desc_hw_addr = dma_params.desc_mem_phys; + dma_params.rxbuf_offset = 0; + dma_params.fixed_prio = 1; Do we really need this new parameters? Do you have plans to use other values? I'm ok if this is static (equally as a bunch of rest in dma_params), no see reason to use other values, it at least now. But the issue is not only in these two parameters and not only in cpsw_ndo_open(). It touches cpsw_set_channels() also, where ctlr stop/start is present. In order to not copy cpdma_control_set(cpsw->dma, CPDMA_TX_PRIO_FIXED, 1)... in all such kind places in eth drivers, better to allow the cpdma to control it's parameters... The cpdma ctlr register holds a little more parameters (but only two of them are set in cpsw) Maybe there is reason to save them also. Actually I'd seen this problem when playing with on/off channel shapers, unfortunately the cpdma ctlr holds this info also, and it was lost while on/off (but I'm going to restore it in chan_start()). -- Regards, Ivan Khoronzhuk
Re: [PATCH] net: ethernet: ti: davinci_cpdma: fix fixed prio cpdma ctlr configuration
On 11/08/2016 07:10 AM, Ivan Khoronzhuk wrote: > The dma ctlr is reseted to 0 while cpdma start, thus cpdma ctlr I assume this is because cpdma_ctlr_start() does soft reset. Is it correct? > cannot be configured after cpdma is stopped. So, restore content > of cpdma ctlr while off/on procedure. > > Based on net-next/master ^ remove it > > Signed-off-by: Ivan Khoronzhuk> --- > drivers/net/ethernet/ti/cpsw.c | 6 +- > drivers/net/ethernet/ti/davinci_cpdma.c | 103 > +--- > drivers/net/ethernet/ti/davinci_cpdma.h | 2 + > 3 files changed, 58 insertions(+), 53 deletions(-) > > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c > index b1ddf89..4d04b8e 100644 > --- a/drivers/net/ethernet/ti/cpsw.c > +++ b/drivers/net/ethernet/ti/cpsw.c > @@ -1376,10 +1376,6 @@ static int cpsw_ndo_open(struct net_device *ndev) > ALE_ALL_PORTS, ALE_ALL_PORTS, 0, 0); > > if (!cpsw_common_res_usage_state(cpsw)) { > - /* setup tx dma to fixed prio and zero offset */ > - cpdma_control_set(cpsw->dma, CPDMA_TX_PRIO_FIXED, 1); > - cpdma_control_set(cpsw->dma, CPDMA_RX_BUFFER_OFFSET, 0); > - > /* disable priority elevation */ > __raw_writel(0, >regs->ptype); > > @@ -2710,6 +2706,8 @@ static int cpsw_probe(struct platform_device *pdev) > dma_params.desc_align = 16; > dma_params.has_ext_regs = true; > dma_params.desc_hw_addr = dma_params.desc_mem_phys; > + dma_params.rxbuf_offset = 0; > + dma_params.fixed_prio = 1; Do we really need this new parameters? Do you have plans to use other values? -- regards, -grygorii
[PATCH] net: ethernet: ti: davinci_cpdma: fix fixed prio cpdma ctlr configuration
The dma ctlr is reseted to 0 while cpdma start, thus cpdma ctlr cannot be configured after cpdma is stopped. So, restore content of cpdma ctlr while off/on procedure. Based on net-next/master Signed-off-by: Ivan Khoronzhuk--- drivers/net/ethernet/ti/cpsw.c | 6 +- drivers/net/ethernet/ti/davinci_cpdma.c | 103 +--- drivers/net/ethernet/ti/davinci_cpdma.h | 2 + 3 files changed, 58 insertions(+), 53 deletions(-) diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c index b1ddf89..4d04b8e 100644 --- a/drivers/net/ethernet/ti/cpsw.c +++ b/drivers/net/ethernet/ti/cpsw.c @@ -1376,10 +1376,6 @@ static int cpsw_ndo_open(struct net_device *ndev) ALE_ALL_PORTS, ALE_ALL_PORTS, 0, 0); if (!cpsw_common_res_usage_state(cpsw)) { - /* setup tx dma to fixed prio and zero offset */ - cpdma_control_set(cpsw->dma, CPDMA_TX_PRIO_FIXED, 1); - cpdma_control_set(cpsw->dma, CPDMA_RX_BUFFER_OFFSET, 0); - /* disable priority elevation */ __raw_writel(0, >regs->ptype); @@ -2710,6 +2706,8 @@ static int cpsw_probe(struct platform_device *pdev) dma_params.desc_align = 16; dma_params.has_ext_regs = true; dma_params.desc_hw_addr = dma_params.desc_mem_phys; + dma_params.rxbuf_offset = 0; + dma_params.fixed_prio = 1; cpsw->dma = cpdma_ctlr_create(_params); if (!cpsw->dma) { diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c index c3f35f1..05afc05 100644 --- a/drivers/net/ethernet/ti/davinci_cpdma.c +++ b/drivers/net/ethernet/ti/davinci_cpdma.c @@ -124,6 +124,29 @@ struct cpdma_chan { int int_set, int_clear, td; }; +struct cpdma_control_info { + u32 reg; + u32 shift, mask; + int access; +#define ACCESS_RO BIT(0) +#define ACCESS_WO BIT(1) +#define ACCESS_RW (ACCESS_RO | ACCESS_WO) +}; + +static struct cpdma_control_info controls[] = { + [CPDMA_CMD_IDLE] = {CPDMA_DMACONTROL, 3, 1, ACCESS_WO}, + [CPDMA_COPY_ERROR_FRAMES] = {CPDMA_DMACONTROL, 4, 1, ACCESS_RW}, + [CPDMA_RX_OFF_LEN_UPDATE] = {CPDMA_DMACONTROL, 2, 1, ACCESS_RW}, + [CPDMA_RX_OWNERSHIP_FLIP] = {CPDMA_DMACONTROL, 1, 1, ACCESS_RW}, + [CPDMA_TX_PRIO_FIXED] = {CPDMA_DMACONTROL, 0, 1, ACCESS_RW}, + [CPDMA_STAT_IDLE] = {CPDMA_DMASTATUS, 31, 1, ACCESS_RO}, + [CPDMA_STAT_TX_ERR_CODE] = {CPDMA_DMASTATUS, 20, 0xf,ACCESS_RW}, + [CPDMA_STAT_TX_ERR_CHAN] = {CPDMA_DMASTATUS, 16, 0x7,ACCESS_RW}, + [CPDMA_STAT_RX_ERR_CODE] = {CPDMA_DMASTATUS, 12, 0xf,ACCESS_RW}, + [CPDMA_STAT_RX_ERR_CHAN] = {CPDMA_DMASTATUS, 8, 0x7,ACCESS_RW}, + [CPDMA_RX_BUFFER_OFFSET] = {CPDMA_RXBUFFOFS, 0, 0x, ACCESS_RW}, +}; + #define tx_chan_num(chan) (chan) #define rx_chan_num(chan) ((chan) + CPDMA_MAX_CHANNELS) #define is_rx_chan(chan) ((chan)->chan_num >= CPDMA_MAX_CHANNELS) @@ -253,6 +276,31 @@ static void cpdma_desc_free(struct cpdma_desc_pool *pool, gen_pool_free(pool->gen_pool, (unsigned long)desc, pool->desc_size); } +static int _cpdma_control_set(struct cpdma_ctlr *ctlr, int control, int value) +{ + struct cpdma_control_info *info = [control]; + u32 val; + + if (!ctlr->params.has_ext_regs) + return -ENOTSUPP; + + if (ctlr->state != CPDMA_STATE_ACTIVE) + return -EINVAL; + + if (control < 0 || control >= ARRAY_SIZE(controls)) + return -ENOENT; + + if ((info->access & ACCESS_WO) != ACCESS_WO) + return -EPERM; + + val = dma_reg_read(ctlr, info->reg); + val &= ~(info->mask << info->shift); + val |= (value & info->mask) << info->shift; + dma_reg_write(ctlr, info->reg, val); + + return 0; +} + struct cpdma_ctlr *cpdma_ctlr_create(struct cpdma_params *params) { struct cpdma_ctlr *ctlr; @@ -324,6 +372,11 @@ int cpdma_ctlr_start(struct cpdma_ctlr *ctlr) if (ctlr->channels[i]) cpdma_chan_start(ctlr->channels[i]); } + + _cpdma_control_set(ctlr, CPDMA_TX_PRIO_FIXED, ctlr->params.fixed_prio); + _cpdma_control_set(ctlr, CPDMA_RX_BUFFER_OFFSET, + ctlr->params.rxbuf_offset); + spin_unlock_irqrestore(>lock, flags); return 0; } @@ -874,29 +927,6 @@ int cpdma_chan_int_ctrl(struct cpdma_chan *chan, bool enable) return 0; } -struct cpdma_control_info { - u32 reg; - u32 shift, mask; - int access; -#define ACCESS_RO BIT(0) -#define ACCESS_WO BIT(1) -#define ACCESS_RW (ACCESS_RO | ACCESS_WO) -}; -