| Message ID | 20200508105304.14065-3-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dmaengine: dw: Take Baikal-T1 SoC DW DMAC peculiarities into account | expand |
On Fri, May 08, 2020 at 01:53:00PM +0300, Serge Semin wrote: > This array property is used to indicate the maximum burst transaction > length supported by each DMA channel. > + snps,max-burst-len: > + $ref: /schemas/types.yaml#/definitions/uint32-array > + description: | > + Maximum length of burst transactions supported by hardware. > + It's an array property with one cell per channel in units of > + CTLx register SRC_TR_WIDTH/DST_TR_WIDTH (data-width) field. > + items: > + maxItems: 8 > + items: > + enum: [4, 8, 16, 32, 64, 128, 256] Isn't 1 allowed? > + default: 256
On Fri, May 08, 2020 at 02:12:42PM +0300, Andy Shevchenko wrote: > On Fri, May 08, 2020 at 01:53:00PM +0300, Serge Semin wrote: > > This array property is used to indicate the maximum burst transaction > > length supported by each DMA channel. > > > + snps,max-burst-len: > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > + description: | > > + Maximum length of burst transactions supported by hardware. > > + It's an array property with one cell per channel in units of > > + CTLx register SRC_TR_WIDTH/DST_TR_WIDTH (data-width) field. > > + items: > > + maxItems: 8 > > + items: > > > + enum: [4, 8, 16, 32, 64, 128, 256] > > Isn't 1 allowed? Burst length of 1 unit is supported, but in accordance with Data Book the MAX burst length is limited to be equal to a value from the set I submitted. So the max value can be either 4, or 8, or 16 and so on. -Sergey > > > + default: 256 > > -- > With Best Regards, > Andy Shevchenko > >
On Mon, May 11, 2020 at 11:05:28PM +0300, Serge Semin wrote: > On Fri, May 08, 2020 at 02:12:42PM +0300, Andy Shevchenko wrote: > > On Fri, May 08, 2020 at 01:53:00PM +0300, Serge Semin wrote: > > > This array property is used to indicate the maximum burst transaction > > > length supported by each DMA channel. > > > > > + snps,max-burst-len: > > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > > + description: | > > > + Maximum length of burst transactions supported by hardware. > > > + It's an array property with one cell per channel in units of > > > + CTLx register SRC_TR_WIDTH/DST_TR_WIDTH (data-width) field. > > > + items: > > > + maxItems: 8 > > > + items: > > > > > + enum: [4, 8, 16, 32, 64, 128, 256] > > > > Isn't 1 allowed? > > Burst length of 1 unit is supported, but in accordance with Data Book the MAX > burst length is limited to be equal to a value from the set I submitted. So the > max value can be either 4, or 8, or 16 and so on. Hmm... It seems you mistakenly took here DMAH_CHx_MAX_MULT_SIZE pre-silicon configuration parameter instead of runtime as described in Table 26: CTLx.SRC_MSIZE and DEST_MSIZE Decoding.
On Tue, May 12, 2020 at 12:01:38AM +0300, Andy Shevchenko wrote: > On Mon, May 11, 2020 at 11:05:28PM +0300, Serge Semin wrote: > > On Fri, May 08, 2020 at 02:12:42PM +0300, Andy Shevchenko wrote: > > > On Fri, May 08, 2020 at 01:53:00PM +0300, Serge Semin wrote: > > > > This array property is used to indicate the maximum burst transaction > > > > length supported by each DMA channel. > > > > > > > + snps,max-burst-len: > > > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > > > + description: | > > > > + Maximum length of burst transactions supported by hardware. > > > > + It's an array property with one cell per channel in units of > > > > + CTLx register SRC_TR_WIDTH/DST_TR_WIDTH (data-width) field. > > > > + items: > > > > + maxItems: 8 > > > > + items: > > > > > > > + enum: [4, 8, 16, 32, 64, 128, 256] > > > > > > Isn't 1 allowed? > > > > Burst length of 1 unit is supported, but in accordance with Data Book the MAX > > burst length is limited to be equal to a value from the set I submitted. So the > > max value can be either 4, or 8, or 16 and so on. > > Hmm... It seems you mistakenly took here DMAH_CHx_MAX_MULT_SIZE pre-silicon > configuration parameter instead of runtime as described in Table 26: > CTLx.SRC_MSIZE and DEST_MSIZE Decoding. No. You misunderstood what I meant. We shouldn't use a runtime parameters values here. Why would we? Property "snps,max-burst-len" matches DMAH_CHx_MAX_MULT_SIZE config parameter. See a comment to the "SRC_MSIZE" and "DEST_MSIZE" fields of the registers. You'll find out that their maximum value is determined by the DMAH_CHx_MAX_MULT_SIZE parameter, which must belong to the set [4, 8, 16, 32, 64, 128, 256]. So no matter how you synthesize the DW DMAC block you'll have at least 4x max burst length supported. -Sergey > > -- > With Best Regards, > Andy Shevchenko > >
On Tue, May 12, 2020 at 12:35:31AM +0300, Serge Semin wrote: > On Tue, May 12, 2020 at 12:01:38AM +0300, Andy Shevchenko wrote: > > On Mon, May 11, 2020 at 11:05:28PM +0300, Serge Semin wrote: > > > On Fri, May 08, 2020 at 02:12:42PM +0300, Andy Shevchenko wrote: > > > > On Fri, May 08, 2020 at 01:53:00PM +0300, Serge Semin wrote: > > > > > This array property is used to indicate the maximum burst transaction > > > > > length supported by each DMA channel. > > > > > > > > > + snps,max-burst-len: > > > > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > > > > + description: | > > > > > + Maximum length of burst transactions supported by hardware. > > > > > + It's an array property with one cell per channel in units of > > > > > + CTLx register SRC_TR_WIDTH/DST_TR_WIDTH (data-width) field. > > > > > + items: > > > > > + maxItems: 8 > > > > > + items: > > > > > > > > > + enum: [4, 8, 16, 32, 64, 128, 256] > > > > > > > > Isn't 1 allowed? > > > > > > Burst length of 1 unit is supported, but in accordance with Data Book the MAX > > > burst length is limited to be equal to a value from the set I submitted. So the > > > max value can be either 4, or 8, or 16 and so on. > > > > Hmm... It seems you mistakenly took here DMAH_CHx_MAX_MULT_SIZE pre-silicon > > configuration parameter instead of runtime as described in Table 26: > > CTLx.SRC_MSIZE and DEST_MSIZE Decoding. > > No. You misunderstood what I meant. We shouldn't use a runtime parameters values > here. Why would we? Because what we describe in the DTS is what user may do to the hardware. In some cases user might want to limit this to 1, how to achieve that? Rob, is there any clarification that schema describes only synthesized values? Or i.o.w. shall we allow user to setup whatever hardware supports at run time? > Property "snps,max-burst-len" matches DMAH_CHx_MAX_MULT_SIZE > config parameter. Why? User should have a possibility to ask whatever hardware supports at run time. > See a comment to the "SRC_MSIZE" and "DEST_MSIZE" fields of the > registers. You'll find out that their maximum value is determined by the > DMAH_CHx_MAX_MULT_SIZE parameter, which must belong to the set [4, 8, 16, 32, 64, > 128, 256]. So no matter how you synthesize the DW DMAC block you'll have at least > 4x max burst length supported. That's true.
On Tue, May 12, 2020 at 12:08:04PM +0300, Andy Shevchenko wrote: > On Tue, May 12, 2020 at 12:35:31AM +0300, Serge Semin wrote: > > On Tue, May 12, 2020 at 12:01:38AM +0300, Andy Shevchenko wrote: > > > On Mon, May 11, 2020 at 11:05:28PM +0300, Serge Semin wrote: > > > > On Fri, May 08, 2020 at 02:12:42PM +0300, Andy Shevchenko wrote: > > > > > On Fri, May 08, 2020 at 01:53:00PM +0300, Serge Semin wrote: > > > > > > This array property is used to indicate the maximum burst transaction > > > > > > length supported by each DMA channel. > > > > > > > > > > > + snps,max-burst-len: > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > > > > > + description: | > > > > > > + Maximum length of burst transactions supported by hardware. > > > > > > + It's an array property with one cell per channel in units of > > > > > > + CTLx register SRC_TR_WIDTH/DST_TR_WIDTH (data-width) field. > > > > > > + items: > > > > > > + maxItems: 8 > > > > > > + items: > > > > > > > > > > > + enum: [4, 8, 16, 32, 64, 128, 256] > > > > > > > > > > Isn't 1 allowed? > > > > > > > > Burst length of 1 unit is supported, but in accordance with Data Book the MAX > > > > burst length is limited to be equal to a value from the set I submitted. So the > > > > max value can be either 4, or 8, or 16 and so on. > > > > > > Hmm... It seems you mistakenly took here DMAH_CHx_MAX_MULT_SIZE pre-silicon > > > configuration parameter instead of runtime as described in Table 26: > > > CTLx.SRC_MSIZE and DEST_MSIZE Decoding. > > > > No. You misunderstood what I meant. We shouldn't use a runtime parameters values > > here. Why would we? > > Because what we describe in the DTS is what user may do to the hardware. In > some cases user might want to limit this to 1, how to achieve that? No, dts isn't about hardware configuration, it's about hardware description. It's not what user want, it's about what hardware can and can't. If a developer wants to limit it to 1, one need to do this in software. The IP-core just can't be synthesized with such limitation. No matter what, it must be no less than 4 as I described in the enum setting. > > Rob, is there any clarification that schema describes only synthesized values? > Or i.o.w. shall we allow user to setup whatever hardware supports at run time? One more time. max-burst-len set to 1 wouldn't describe the real hardware capability because the Dw DMAC IP-core simply can't be synthesized with such max-burst-len. In this patch I submitted the "max-burst-len" property, not just "burst-len" setting. > > > Property "snps,max-burst-len" matches DMAH_CHx_MAX_MULT_SIZE > > config parameter. > > Why? User should have a possibility to ask whatever hardware supports at run time. Because the run time parameter is limited with DMAH_CHx_MAX_MULT_SIZE value, you agreed with that further and "snps,max-burst-len" is about hardware limitation. For the same reason the dma-channels property is limited to belong the segment 1 - 8, dma-masters number must be limited with 1 - 4, block_size should be one of the set [3, 7, 15, 31, 63, 127, 255, 511, 1023, 2047, 4095] and so on. For instance, the block-size can be set any but not greater than a value of the "block-size" property found in the dt node or retrieved from the corresponding IP param register. It's not what user want, but what hardware can support. -Sergey > > > See a comment to the "SRC_MSIZE" and "DEST_MSIZE" fields of the > > registers. You'll find out that their maximum value is determined by the > > DMAH_CHx_MAX_MULT_SIZE parameter, which must belong to the set [4, 8, 16, 32, 64, > > 128, 256]. So no matter how you synthesize the DW DMAC block you'll have at least > > 4x max burst length supported. > > That's true. > > > -- > With Best Regards, > Andy Shevchenko > >
On Tue, May 12, 2020 at 02:49:46PM +0300, Serge Semin wrote: > On Tue, May 12, 2020 at 12:08:04PM +0300, Andy Shevchenko wrote: > > On Tue, May 12, 2020 at 12:35:31AM +0300, Serge Semin wrote: > > > On Tue, May 12, 2020 at 12:01:38AM +0300, Andy Shevchenko wrote: > > > > On Mon, May 11, 2020 at 11:05:28PM +0300, Serge Semin wrote: > > > > > On Fri, May 08, 2020 at 02:12:42PM +0300, Andy Shevchenko wrote: > > > > > > On Fri, May 08, 2020 at 01:53:00PM +0300, Serge Semin wrote: ... I leave it to Rob and Vinod. It won't break our case, so, feel free with your approach. P.S. Perhaps at some point we need to 1) convert properties to be u32 (it will simplify things); 2) convert legacy ones to proper format ('-' instead of '_', vendor prefix added); 3) parse them in core with device property API.
On 12-05-20, 15:38, Andy Shevchenko wrote: > On Tue, May 12, 2020 at 02:49:46PM +0300, Serge Semin wrote: > > On Tue, May 12, 2020 at 12:08:04PM +0300, Andy Shevchenko wrote: > > > On Tue, May 12, 2020 at 12:35:31AM +0300, Serge Semin wrote: > > > > On Tue, May 12, 2020 at 12:01:38AM +0300, Andy Shevchenko wrote: > > > > > On Mon, May 11, 2020 at 11:05:28PM +0300, Serge Semin wrote: > > > > > > On Fri, May 08, 2020 at 02:12:42PM +0300, Andy Shevchenko wrote: > > > > > > > On Fri, May 08, 2020 at 01:53:00PM +0300, Serge Semin wrote: > > ... > > I leave it to Rob and Vinod. > It won't break our case, so, feel free with your approach. I agree the DT is about describing the hardware and looks like value of 1 is not allowed. If allowed it should be added.. > P.S. Perhaps at some point we need to > 1) convert properties to be u32 (it will simplify things); > 2) convert legacy ones to proper format ('-' instead of '_', vendor prefix added); > 3) parse them in core with device property API. These suggestions are good and should be done.
On Fri, May 15, 2020 at 11:39:11AM +0530, Vinod Koul wrote: > On 12-05-20, 15:38, Andy Shevchenko wrote: > > On Tue, May 12, 2020 at 02:49:46PM +0300, Serge Semin wrote: > > > On Tue, May 12, 2020 at 12:08:04PM +0300, Andy Shevchenko wrote: > > > > On Tue, May 12, 2020 at 12:35:31AM +0300, Serge Semin wrote: > > > > > On Tue, May 12, 2020 at 12:01:38AM +0300, Andy Shevchenko wrote: > > > > > > On Mon, May 11, 2020 at 11:05:28PM +0300, Serge Semin wrote: > > > > > > > On Fri, May 08, 2020 at 02:12:42PM +0300, Andy Shevchenko wrote: > > > > > > > > On Fri, May 08, 2020 at 01:53:00PM +0300, Serge Semin wrote: ... > > I leave it to Rob and Vinod. > > It won't break our case, so, feel free with your approach. > > I agree the DT is about describing the hardware and looks like value of > 1 is not allowed. If allowed it should be added.. It's allowed at *run time*, it's illegal in *pre-silicon stage* when synthesizing the IP.
On 15-05-20, 13:51, Andy Shevchenko wrote: > On Fri, May 15, 2020 at 11:39:11AM +0530, Vinod Koul wrote: > > On 12-05-20, 15:38, Andy Shevchenko wrote: > > > On Tue, May 12, 2020 at 02:49:46PM +0300, Serge Semin wrote: > > > > On Tue, May 12, 2020 at 12:08:04PM +0300, Andy Shevchenko wrote: > > > > > On Tue, May 12, 2020 at 12:35:31AM +0300, Serge Semin wrote: > > > > > > On Tue, May 12, 2020 at 12:01:38AM +0300, Andy Shevchenko wrote: > > > > > > > On Mon, May 11, 2020 at 11:05:28PM +0300, Serge Semin wrote: > > > > > > > > On Fri, May 08, 2020 at 02:12:42PM +0300, Andy Shevchenko wrote: > > > > > > > > > On Fri, May 08, 2020 at 01:53:00PM +0300, Serge Semin wrote: > > ... > > > > I leave it to Rob and Vinod. > > > It won't break our case, so, feel free with your approach. > > > > I agree the DT is about describing the hardware and looks like value of > > 1 is not allowed. If allowed it should be added.. > > It's allowed at *run time*, it's illegal in *pre-silicon stage* when > synthesizing the IP. Then it should be added ..
On Fri, May 15, 2020 at 04:26:58PM +0530, Vinod Koul wrote: > On 15-05-20, 13:51, Andy Shevchenko wrote: > > On Fri, May 15, 2020 at 11:39:11AM +0530, Vinod Koul wrote: > > > On 12-05-20, 15:38, Andy Shevchenko wrote: > > > > On Tue, May 12, 2020 at 02:49:46PM +0300, Serge Semin wrote: > > > > > On Tue, May 12, 2020 at 12:08:04PM +0300, Andy Shevchenko wrote: > > > > > > On Tue, May 12, 2020 at 12:35:31AM +0300, Serge Semin wrote: > > > > > > > On Tue, May 12, 2020 at 12:01:38AM +0300, Andy Shevchenko wrote: > > > > > > > > On Mon, May 11, 2020 at 11:05:28PM +0300, Serge Semin wrote: > > > > > > > > > On Fri, May 08, 2020 at 02:12:42PM +0300, Andy Shevchenko wrote: > > > > > > > > > > On Fri, May 08, 2020 at 01:53:00PM +0300, Serge Semin wrote: > > > > ... > > > > > > I leave it to Rob and Vinod. > > > > It won't break our case, so, feel free with your approach. > > > > > > I agree the DT is about describing the hardware and looks like value of > > > 1 is not allowed. If allowed it should be added.. > > > > It's allowed at *run time*, it's illegal in *pre-silicon stage* when > > synthesizing the IP. > > Then it should be added .. Vinod, max-burst-len is "MAXimum" burst length not "run-time or current or any other" burst length. It's a constant defined at the IP-core synthesis stage and according to the Data Book, MAX burst length can't be 1. The allowed values are exactly as I described in the binding [4, 8, 16, 32, ...]. MAX burst length defines the upper limit of the run-time burst length. So setting it to 1 isn't about describing a hardware, but using DT for the software convenience. -Sergey > > -- > ~Vinod
On Fri, May 15, 2020 at 02:11:13PM +0300, Serge Semin wrote: > On Fri, May 15, 2020 at 04:26:58PM +0530, Vinod Koul wrote: > > On 15-05-20, 13:51, Andy Shevchenko wrote: > > > On Fri, May 15, 2020 at 11:39:11AM +0530, Vinod Koul wrote: > > > > On 12-05-20, 15:38, Andy Shevchenko wrote: > > > > > On Tue, May 12, 2020 at 02:49:46PM +0300, Serge Semin wrote: > > > > > > On Tue, May 12, 2020 at 12:08:04PM +0300, Andy Shevchenko wrote: > > > > > > > On Tue, May 12, 2020 at 12:35:31AM +0300, Serge Semin wrote: > > > > > > > > On Tue, May 12, 2020 at 12:01:38AM +0300, Andy Shevchenko wrote: > > > > > > > > > On Mon, May 11, 2020 at 11:05:28PM +0300, Serge Semin wrote: > > > > > > > > > > On Fri, May 08, 2020 at 02:12:42PM +0300, Andy Shevchenko wrote: > > > > > > > > > > > On Fri, May 08, 2020 at 01:53:00PM +0300, Serge Semin wrote: > > > > > > ... > > > > > > > > I leave it to Rob and Vinod. > > > > > It won't break our case, so, feel free with your approach. > > > > > > > > I agree the DT is about describing the hardware and looks like value of > > > > 1 is not allowed. If allowed it should be added.. > > > > > > It's allowed at *run time*, it's illegal in *pre-silicon stage* when > > > synthesizing the IP. > > > > Then it should be added .. > > Vinod, max-burst-len is "MAXimum" burst length not "run-time or current or any > other" burst length. It's a constant defined at the IP-core synthesis stage and > according to the Data Book, MAX burst length can't be 1. The allowed values are > exactly as I described in the binding [4, 8, 16, 32, ...]. MAX burst length > defines the upper limit of the run-time burst length. So setting it to 1 isn't > about describing a hardware, but using DT for the software convenience. > > -Sergey Vinod, to make this completely clear. According to the DW DMAC data book: - In general, run-time parameter of the DMA transaction burst length (set in the SRC_MSIZE/DST_MSIZE fields of the channel control register) may belong to the set [1, 4, 8, 16, 32, 64, 128, 256]. - Actual upper limit of the burst length run-time parameter is limited by a constant defined at the IP-synthesize stage (it's called DMAH_CHx_MAX_MULT_SIZE) and this constant belongs to the set [4, 8, 16, 32, 64, 128, 256]. (See, no 1 in this set). So the run-time burst length in a case of particular DW DMA controller belongs to the range: 1 <= SRC_MSIZE <= DMAH_CHx_MAX_MULT_SIZE and 1 <= DST_MSIZE <= DMAH_CHx_MAX_MULT_SIZE See. No mater which DW DMA controller we get each of them will at least support the burst length of 1 and 4 transfer words. This is determined by design of the DW DMA controller IP since DMAH_CHx_MAX_MULT_SIZE constant set starts with 4. In this patch I suggest to add the max-burst-len property, which specifies the upper limit for the run-time burst length. Since the maximum burst length capable to be set to the SRC_MSIZE/DST_MSIZE fields of the DMA channel control register is determined by the DMAH_CHx_MAX_MULT_SIZE constant (which can't be 1 by the DW DMA IP design), max-burst-len property as being also responsible for the maximum burst length setting should be associated with DMAH_CHx_MAX_MULT_SIZE thus should belong to the same set [4, 8, 16, 32, 64, 128, 256]. So 1 shouldn't be in the enum of the max-burst-len property constraint, because hardware doesn't support such limitation by design, while setting 1 as max-burst-len would mean incorrect description of the DMA controller. Vinod, could you take a look at the info I provided above and say your final word whether 1 should be really allowed to be in the max-burst-len enum constraints? I'll do as you say in the next version of the patchset. Regards, -Sergey > > > > > -- > > ~Vinod
On Sun, May 17, 2020 at 08:47:39PM +0300, Serge Semin wrote: > On Fri, May 15, 2020 at 02:11:13PM +0300, Serge Semin wrote: > > On Fri, May 15, 2020 at 04:26:58PM +0530, Vinod Koul wrote: > > > On 15-05-20, 13:51, Andy Shevchenko wrote: > > > > On Fri, May 15, 2020 at 11:39:11AM +0530, Vinod Koul wrote: > > > > > On 12-05-20, 15:38, Andy Shevchenko wrote: > > > > > > On Tue, May 12, 2020 at 02:49:46PM +0300, Serge Semin wrote: > > > > > > > On Tue, May 12, 2020 at 12:08:04PM +0300, Andy Shevchenko wrote: > > > > > > > > On Tue, May 12, 2020 at 12:35:31AM +0300, Serge Semin wrote: > > > > > > > > > On Tue, May 12, 2020 at 12:01:38AM +0300, Andy Shevchenko wrote: > > > > > > > > > > On Mon, May 11, 2020 at 11:05:28PM +0300, Serge Semin wrote: > > > > > > > > > > > On Fri, May 08, 2020 at 02:12:42PM +0300, Andy Shevchenko wrote: > > > > > > > > > > > > On Fri, May 08, 2020 at 01:53:00PM +0300, Serge Semin wrote: > > > > > > > > ... > > > > > > > > > > I leave it to Rob and Vinod. > > > > > > It won't break our case, so, feel free with your approach. > > > > > > > > > > I agree the DT is about describing the hardware and looks like value of > > > > > 1 is not allowed. If allowed it should be added.. > > > > > > > > It's allowed at *run time*, it's illegal in *pre-silicon stage* when > > > > synthesizing the IP. > > > > > > Then it should be added .. > > > > Vinod, max-burst-len is "MAXimum" burst length not "run-time or current or any > > other" burst length. It's a constant defined at the IP-core synthesis stage and > > according to the Data Book, MAX burst length can't be 1. The allowed values are > > exactly as I described in the binding [4, 8, 16, 32, ...]. MAX burst length > > defines the upper limit of the run-time burst length. So setting it to 1 isn't > > about describing a hardware, but using DT for the software convenience. > > > > -Sergey > > Vinod, to make this completely clear. According to the DW DMAC data book: > - In general, run-time parameter of the DMA transaction burst length (set in > the SRC_MSIZE/DST_MSIZE fields of the channel control register) may belong > to the set [1, 4, 8, 16, 32, 64, 128, 256]. > - Actual upper limit of the burst length run-time parameter is limited by a > constant defined at the IP-synthesize stage (it's called DMAH_CHx_MAX_MULT_SIZE) > and this constant belongs to the set [4, 8, 16, 32, 64, 128, 256]. (See, no 1 > in this set). > > So the run-time burst length in a case of particular DW DMA controller belongs > to the range: > 1 <= SRC_MSIZE <= DMAH_CHx_MAX_MULT_SIZE > and > 1 <= DST_MSIZE <= DMAH_CHx_MAX_MULT_SIZE > > See. No mater which DW DMA controller we get each of them will at least support > the burst length of 1 and 4 transfer words. This is determined by design of the > DW DMA controller IP since DMAH_CHx_MAX_MULT_SIZE constant set starts with 4. > > In this patch I suggest to add the max-burst-len property, which specifies > the upper limit for the run-time burst length. Since the maximum burst length > capable to be set to the SRC_MSIZE/DST_MSIZE fields of the DMA channel control > register is determined by the DMAH_CHx_MAX_MULT_SIZE constant (which can't be 1 > by the DW DMA IP design), max-burst-len property as being also responsible for > the maximum burst length setting should be associated with DMAH_CHx_MAX_MULT_SIZE > thus should belong to the same set [4, 8, 16, 32, 64, 128, 256]. > > So 1 shouldn't be in the enum of the max-burst-len property constraint, because > hardware doesn't support such limitation by design, while setting 1 as > max-burst-len would mean incorrect description of the DMA controller. > > Vinod, could you take a look at the info I provided above and say your final word > whether 1 should be really allowed to be in the max-burst-len enum constraints? > I'll do as you say in the next version of the patchset. I generally think the synthesis time IP configuration should be implied by the compatible string which is why we have SoC specific compatible strings (Of course I dream for IP vendors to make all that discoverable which is only occasionally the case). There are exceptions to this. If one SoC has the same IP configured in different ways, then we'd probably have properties for the differences. As to whether h/w configuration is okay in DT, the answer is yes. The question is whether it is determined by SoC, board, OS and also who would set it and how often. Something tuned per board and independent of the OS/user is the ideal example. Rob
On Mon, May 18, 2020 at 11:30:03AM -0600, Rob Herring wrote: > On Sun, May 17, 2020 at 08:47:39PM +0300, Serge Semin wrote: > > On Fri, May 15, 2020 at 02:11:13PM +0300, Serge Semin wrote: > > > On Fri, May 15, 2020 at 04:26:58PM +0530, Vinod Koul wrote: > > > > On 15-05-20, 13:51, Andy Shevchenko wrote: > > > > > On Fri, May 15, 2020 at 11:39:11AM +0530, Vinod Koul wrote: > > > > > > On 12-05-20, 15:38, Andy Shevchenko wrote: > > > > > > > On Tue, May 12, 2020 at 02:49:46PM +0300, Serge Semin wrote: > > > > > > > > On Tue, May 12, 2020 at 12:08:04PM +0300, Andy Shevchenko wrote: > > > > > > > > > On Tue, May 12, 2020 at 12:35:31AM +0300, Serge Semin wrote: > > > > > > > > > > On Tue, May 12, 2020 at 12:01:38AM +0300, Andy Shevchenko wrote: > > > > > > > > > > > On Mon, May 11, 2020 at 11:05:28PM +0300, Serge Semin wrote: > > > > > > > > > > > > On Fri, May 08, 2020 at 02:12:42PM +0300, Andy Shevchenko wrote: > > > > > > > > > > > > > On Fri, May 08, 2020 at 01:53:00PM +0300, Serge Semin wrote: > > > > > > > > > > ... > > > > > > > > > > > > I leave it to Rob and Vinod. > > > > > > > It won't break our case, so, feel free with your approach. > > > > > > > > > > > > I agree the DT is about describing the hardware and looks like value of > > > > > > 1 is not allowed. If allowed it should be added.. > > > > > > > > > > It's allowed at *run time*, it's illegal in *pre-silicon stage* when > > > > > synthesizing the IP. > > > > > > > > Then it should be added .. > > > > > > Vinod, max-burst-len is "MAXimum" burst length not "run-time or current or any > > > other" burst length. It's a constant defined at the IP-core synthesis stage and > > > according to the Data Book, MAX burst length can't be 1. The allowed values are > > > exactly as I described in the binding [4, 8, 16, 32, ...]. MAX burst length > > > defines the upper limit of the run-time burst length. So setting it to 1 isn't > > > about describing a hardware, but using DT for the software convenience. > > > > > > -Sergey > > > > Vinod, to make this completely clear. According to the DW DMAC data book: > > - In general, run-time parameter of the DMA transaction burst length (set in > > the SRC_MSIZE/DST_MSIZE fields of the channel control register) may belong > > to the set [1, 4, 8, 16, 32, 64, 128, 256]. > > - Actual upper limit of the burst length run-time parameter is limited by a > > constant defined at the IP-synthesize stage (it's called DMAH_CHx_MAX_MULT_SIZE) > > and this constant belongs to the set [4, 8, 16, 32, 64, 128, 256]. (See, no 1 > > in this set). > > > > So the run-time burst length in a case of particular DW DMA controller belongs > > to the range: > > 1 <= SRC_MSIZE <= DMAH_CHx_MAX_MULT_SIZE > > and > > 1 <= DST_MSIZE <= DMAH_CHx_MAX_MULT_SIZE > > > > See. No mater which DW DMA controller we get each of them will at least support > > the burst length of 1 and 4 transfer words. This is determined by design of the > > DW DMA controller IP since DMAH_CHx_MAX_MULT_SIZE constant set starts with 4. > > > > In this patch I suggest to add the max-burst-len property, which specifies > > the upper limit for the run-time burst length. Since the maximum burst length > > capable to be set to the SRC_MSIZE/DST_MSIZE fields of the DMA channel control > > register is determined by the DMAH_CHx_MAX_MULT_SIZE constant (which can't be 1 > > by the DW DMA IP design), max-burst-len property as being also responsible for > > the maximum burst length setting should be associated with DMAH_CHx_MAX_MULT_SIZE > > thus should belong to the same set [4, 8, 16, 32, 64, 128, 256]. > > > > So 1 shouldn't be in the enum of the max-burst-len property constraint, because > > hardware doesn't support such limitation by design, while setting 1 as > > max-burst-len would mean incorrect description of the DMA controller. > > > > Vinod, could you take a look at the info I provided above and say your final word > > whether 1 should be really allowed to be in the max-burst-len enum constraints? > > I'll do as you say in the next version of the patchset. > > I generally think the synthesis time IP configuration should be implied > by the compatible string which is why we have SoC specific compatible > strings (Of course I dream for IP vendors to make all that discoverable > which is only occasionally the case). There are exceptions to this. If > one SoC has the same IP configured in different ways, then we'd probably > have properties for the differences. Hm, AFAIU from what you said the IP configuration specific to a particular SoC must be determined by the compatible string and that configuration parameters should be hidden somewhere in the driver internals for instance in the platform data structure. In case if there are several versions of the same IP are embedded into the SoC, then the differences can be described by the DT properties. Right? If I am right, then that's weird. A lot of the currently available platforms (and drivers) don't follow that rule and just specify the generic IP compatible string and describe their IP synthesis parameters by the DT properties. > > As to whether h/w configuration is okay in DT, the answer is yes. The > question is whether it is determined by SoC, board, OS and also who > would set it and how often. Something tuned per board and independent of > the OS/user is the ideal example. So does this mean that I have to allow the max-burst-len property to be 1 even though in accordance with the DW DMA Data Book the upper limit of the burst-length will never be 1, but will always start with 4? By allowing the upper limit to be 1 we wouldn't provide the h/w configuration (hardware has already been configured with maximum burst length parameter DMAH_CHx_MAX_MULT_SIZE on the IP synthesis stage), but would setup an artificial constraints on the maximum allowed burst length. Are you ok with this and 1 should be permitted anyway? -Sergey > > Rob
On 17-05-20, 20:47, Serge Semin wrote: > On Fri, May 15, 2020 at 02:11:13PM +0300, Serge Semin wrote: > > On Fri, May 15, 2020 at 04:26:58PM +0530, Vinod Koul wrote: > > > On 15-05-20, 13:51, Andy Shevchenko wrote: > > > > On Fri, May 15, 2020 at 11:39:11AM +0530, Vinod Koul wrote: > > > > > On 12-05-20, 15:38, Andy Shevchenko wrote: > > > > > > On Tue, May 12, 2020 at 02:49:46PM +0300, Serge Semin wrote: > > > > > > > On Tue, May 12, 2020 at 12:08:04PM +0300, Andy Shevchenko wrote: > > > > > > > > On Tue, May 12, 2020 at 12:35:31AM +0300, Serge Semin wrote: > > > > > > > > > On Tue, May 12, 2020 at 12:01:38AM +0300, Andy Shevchenko wrote: > > > > > > > > > > On Mon, May 11, 2020 at 11:05:28PM +0300, Serge Semin wrote: > > > > > > > > > > > On Fri, May 08, 2020 at 02:12:42PM +0300, Andy Shevchenko wrote: > > > > > > > > > > > > On Fri, May 08, 2020 at 01:53:00PM +0300, Serge Semin wrote: > > > > > > > > ... > > > > > > > > > > I leave it to Rob and Vinod. > > > > > > It won't break our case, so, feel free with your approach. > > > > > > > > > > I agree the DT is about describing the hardware and looks like value of > > > > > 1 is not allowed. If allowed it should be added.. > > > > > > > > It's allowed at *run time*, it's illegal in *pre-silicon stage* when > > > > synthesizing the IP. > > > > > > Then it should be added .. > > > > Vinod, max-burst-len is "MAXimum" burst length not "run-time or current or any > > other" burst length. It's a constant defined at the IP-core synthesis stage and > > according to the Data Book, MAX burst length can't be 1. The allowed values are > > exactly as I described in the binding [4, 8, 16, 32, ...]. MAX burst length > > defines the upper limit of the run-time burst length. So setting it to 1 isn't > > about describing a hardware, but using DT for the software convenience. > > > > -Sergey > > Vinod, to make this completely clear. According to the DW DMAC data book: > - In general, run-time parameter of the DMA transaction burst length (set in > the SRC_MSIZE/DST_MSIZE fields of the channel control register) may belong > to the set [1, 4, 8, 16, 32, 64, 128, 256]. so 1 is valid value for msize > - Actual upper limit of the burst length run-time parameter is limited by a > constant defined at the IP-synthesize stage (it's called DMAH_CHx_MAX_MULT_SIZE) > and this constant belongs to the set [4, 8, 16, 32, 64, 128, 256]. (See, no 1 > in this set). maximum can be 4 onwards, but in my configuration I can choose 1 as value for msize > So the run-time burst length in a case of particular DW DMA controller belongs > to the range: > 1 <= SRC_MSIZE <= DMAH_CHx_MAX_MULT_SIZE > and > 1 <= DST_MSIZE <= DMAH_CHx_MAX_MULT_SIZE > > See. No mater which DW DMA controller we get each of them will at least support > the burst length of 1 and 4 transfer words. This is determined by design of the > DW DMA controller IP since DMAH_CHx_MAX_MULT_SIZE constant set starts with 4. > > In this patch I suggest to add the max-burst-len property, which specifies > the upper limit for the run-time burst length. Since the maximum burst length > capable to be set to the SRC_MSIZE/DST_MSIZE fields of the DMA channel control > register is determined by the DMAH_CHx_MAX_MULT_SIZE constant (which can't be 1 > by the DW DMA IP design), max-burst-len property as being also responsible for > the maximum burst length setting should be associated with DMAH_CHx_MAX_MULT_SIZE > thus should belong to the same set [4, 8, 16, 32, 64, 128, 256]. > > So 1 shouldn't be in the enum of the max-burst-len property constraint, because > hardware doesn't support such limitation by design, while setting 1 as > max-burst-len would mean incorrect description of the DMA controller. > > Vinod, could you take a look at the info I provided above and say your final word > whether 1 should be really allowed to be in the max-burst-len enum constraints? > I'll do as you say in the next version of the patchset. You are specifying the parameter which will be used to pick, i think starting with 4 makes sense as we are specifying maximum allowed values for msize. Values lesser than or equal to this would be allowed, I guess that should be added to documentation. thanks
On Tue, May 19, 2020 at 10:43:04PM +0530, Vinod Koul wrote: > On 17-05-20, 20:47, Serge Semin wrote: > > On Fri, May 15, 2020 at 02:11:13PM +0300, Serge Semin wrote: > > > On Fri, May 15, 2020 at 04:26:58PM +0530, Vinod Koul wrote: > > > > On 15-05-20, 13:51, Andy Shevchenko wrote: > > > > > On Fri, May 15, 2020 at 11:39:11AM +0530, Vinod Koul wrote: > > > > > > On 12-05-20, 15:38, Andy Shevchenko wrote: > > > > > > > On Tue, May 12, 2020 at 02:49:46PM +0300, Serge Semin wrote: > > > > > > > > On Tue, May 12, 2020 at 12:08:04PM +0300, Andy Shevchenko wrote: > > > > > > > > > On Tue, May 12, 2020 at 12:35:31AM +0300, Serge Semin wrote: > > > > > > > > > > On Tue, May 12, 2020 at 12:01:38AM +0300, Andy Shevchenko wrote: > > > > > > > > > > > On Mon, May 11, 2020 at 11:05:28PM +0300, Serge Semin wrote: > > > > > > > > > > > > On Fri, May 08, 2020 at 02:12:42PM +0300, Andy Shevchenko wrote: > > > > > > > > > > > > > On Fri, May 08, 2020 at 01:53:00PM +0300, Serge Semin wrote: > > > > > > > > > > ... > > > > > > > > > > > > I leave it to Rob and Vinod. > > > > > > > It won't break our case, so, feel free with your approach. > > > > > > > > > > > > I agree the DT is about describing the hardware and looks like value of > > > > > > 1 is not allowed. If allowed it should be added.. > > > > > > > > > > It's allowed at *run time*, it's illegal in *pre-silicon stage* when > > > > > synthesizing the IP. > > > > > > > > Then it should be added .. > > > > > > Vinod, max-burst-len is "MAXimum" burst length not "run-time or current or any > > > other" burst length. It's a constant defined at the IP-core synthesis stage and > > > according to the Data Book, MAX burst length can't be 1. The allowed values are > > > exactly as I described in the binding [4, 8, 16, 32, ...]. MAX burst length > > > defines the upper limit of the run-time burst length. So setting it to 1 isn't > > > about describing a hardware, but using DT for the software convenience. > > > > > > -Sergey > > > > Vinod, to make this completely clear. According to the DW DMAC data book: > > - In general, run-time parameter of the DMA transaction burst length (set in > > the SRC_MSIZE/DST_MSIZE fields of the channel control register) may belong > > to the set [1, 4, 8, 16, 32, 64, 128, 256]. > > so 1 is valid value for msize Right. > > > - Actual upper limit of the burst length run-time parameter is limited by a > > constant defined at the IP-synthesize stage (it's called DMAH_CHx_MAX_MULT_SIZE) > > and this constant belongs to the set [4, 8, 16, 32, 64, 128, 256]. (See, no 1 > > in this set). > > maximum can be 4 onwards, but in my configuration I can choose 1 as > value for msize It's true for all configurations. msize can be at least 0 or 1, which correspond to 1 and 4 burst length respectively. > > > So the run-time burst length in a case of particular DW DMA controller belongs > > to the range: > > 1 <= SRC_MSIZE <= DMAH_CHx_MAX_MULT_SIZE > > and > > 1 <= DST_MSIZE <= DMAH_CHx_MAX_MULT_SIZE > > > > See. No mater which DW DMA controller we get each of them will at least support > > the burst length of 1 and 4 transfer words. This is determined by design of the > > DW DMA controller IP since DMAH_CHx_MAX_MULT_SIZE constant set starts with 4. > > > > In this patch I suggest to add the max-burst-len property, which specifies > > the upper limit for the run-time burst length. Since the maximum burst length > > capable to be set to the SRC_MSIZE/DST_MSIZE fields of the DMA channel control > > register is determined by the DMAH_CHx_MAX_MULT_SIZE constant (which can't be 1 > > by the DW DMA IP design), max-burst-len property as being also responsible for > > the maximum burst length setting should be associated with DMAH_CHx_MAX_MULT_SIZE > > thus should belong to the same set [4, 8, 16, 32, 64, 128, 256]. > > > > So 1 shouldn't be in the enum of the max-burst-len property constraint, because > > hardware doesn't support such limitation by design, while setting 1 as > > max-burst-len would mean incorrect description of the DMA controller. > > > > Vinod, could you take a look at the info I provided above and say your final word > > whether 1 should be really allowed to be in the max-burst-len enum constraints? > > I'll do as you say in the next version of the patchset. > > You are specifying the parameter which will be used to pick, i think > starting with 4 makes sense as we are specifying maximum allowed values > for msize. Values lesser than or equal to this would be allowed, I guess > that should be added to documentation. Right. Thanks. I'll a proper description to the property in the binding file. -Sergey > > thanks > -- > ~Vinod
diff --git a/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml b/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml index e7611840a7cf..7df4f9ad418a 100644 --- a/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml +++ b/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml @@ -120,6 +120,18 @@ properties: enum: [0, 1] default: 1 + snps,max-burst-len: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: | + Maximum length of burst transactions supported by hardware. + It's an array property with one cell per channel in units of + CTLx register SRC_TR_WIDTH/DST_TR_WIDTH (data-width) field. + items: + maxItems: 8 + items: + enum: [4, 8, 16, 32, 64, 128, 256] + default: 256 + snps,dma-protection-control: $ref: /schemas/types.yaml#definitions/uint32 description: |
This array property is used to indicate the maximum burst transaction length supported by each DMA channel. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Cc: Paul Burton <paulburton@kernel.org> Cc: Ralf Baechle <ralf@linux-mips.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Viresh Kumar <vireshk@kernel.org> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Dan Williams <dan.j.williams@intel.com> Cc: linux-mips@vger.kernel.org --- Changelog v2: - Rearrange SoBs. - Move $ref to the root level of the properties. So do with the constraints. - Set default max-burst-len to 256 TR-WIDTH words. --- .../devicetree/bindings/dma/snps,dma-spear1340.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+)