From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [v5,2/3] dmaengine: mediatek: Add MediaTek High-Speed DMA controller for MT7622 and MT7623 SoC From: sean.wang@mediatek.com Message-Id: <1519973271.8089.166.camel@mtkswgap22> Date: Fri, 2 Mar 2018 14:47:51 +0800 To: Vinod Koul Cc: dan.j.williams@intel.com, robh+dt@kernel.org, mark.rutland@arm.com, dmaengine@vger.kernel.org, devicetree@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Randy Dunlap , Fengguang Wu , Julia Lawall List-ID: SGksIFZpbm9kCgpPbiBUaHUsIDIwMTgtMDMtMDEgYXQgMTg6MjYgKzA1MzAsIFZpbm9kIEtvdWwg d3JvdGU6Cj4gT24gVGh1LCBNYXIgMDEsIDIwMTggYXQgMDY6Mjc6MDFQTSArMDgwMCwgU2VhbiBX YW5nIHdyb3RlOgo+ID4gT24gVGh1LCAyMDE4LTAzLTAxIGF0IDEzOjUzICswNTMwLCBWaW5vZCBL b3VsIHdyb3RlOgo+ID4gPiBPbiBTdW4sIEZlYiAxOCwgMjAxOCBhdCAwMzowODozMEFNICswODAw LCBzZWFuLndhbmdAbWVkaWF0ZWsuY29tIHdyb3RlOgo+ID4gPiAKPiA+ID4gPiBAQCAtMCwwICsx LDEwNTQgQEAKPiA+ID4gPiArLy8gU1BEWC1MaWNlbnNlLUlkZW50aWZpZXI6IEdQTC0yLjAKPiA+ ID4gLy8gQ29weXJpZ2h0IC4uLgo+ID4gPiAKPiA+ID4gVGhlIGNvcHlyaWdodCBsaW5lIG5lZWRz IHRvIGZvbGxvdyBTUERYIHRhZyBsaW5lCj4gPiA+IAo+ID4gCj4gPiBva2F5LCBJIHdpbGwgbWFr ZSBpdCByZW9yZGVyIGFuZCBiZSBzb21ldGhpbmcgbGlrZSB0aGF0Cj4gPiAKPiA+IC8vIFNQRFgt TGljZW5zZS1JZGVudGlmaWVyOiBHUEwtMi4wCj4gPiAvKgo+ID4gICogQ29weXJpZ2h0IChjKSAy MDE3LTIwMTggTWVkaWFUZWsgSW5jLgo+ID4gICogQXV0aG9yOiBTZWFuIFdhbmcgPHNlYW4ud2Fu Z0BtZWRpYXRlay5jb20+Cj4gPiAgKgo+ID4gICogRHJpdmVyIGZvciBNZWRpYVRlayBIaWdoLVNw ZWVkIERNQSBDb250cm9sbGVyCj4gPiAgKgo+ID4gICovCj4gCj4gSXQgbmVlZHMgdG8gYmU6Cj4g Cj4gLy8gU1BEWC1MaWNlbnNlLUlkZW50aWZpZXI6IEdQTC0yLjAKPiAvLyBDb3B5cmlnaHQgKGMp IDIwMTctMjAxOCBNZWRpYVRlayBJbmMuCj4gCj4gLyoKPiAgKiB3aGF0ZXZlciBlbHNlIHlvdSB3 YW50Cj4gICovCj4gCj4gVGhlIGZpcnN0IHR3byBsaW5lcyBhcmUgaW4gQzk5IHN0eWxlIGNvbW1l bnQgYW5kIG5lZWQgdG8gaGF2ZSBTUERYIHRhZyBhbmQKPiBDb3B5cmlnaHQgaW5mbwoKU3VyZSwg SSBjYW4gZG8gaXQgdXNpbmcgQzk5IHN0eWxlIGNvbW1lbnRzIGF0IHRoZSBmaXJzdCB0d28gbGlu ZXMuCgpJbiBhZGRpdGlvbiwgSSdtIHJlYWxseSBjdXJpb3VzIHdoZXJlIHdlIGNhbiBmaW5kIGEg cmVmZXJlbmNlIHRvIHRoZQpydWxlIGFuZCBpZiBpdCAncyBhIHN0cmljdCBydWxlIGZvciBhbGwg dGhlIGRyaXZlcnMuCgpCZWNhdXNlIEknbSBjb25zaWRlcmluZyB3aGV0aGVyIEkgc2hvdWxkIHR1 cm4gb3RoZXIgZHJpdmVyIGludG8gdXNpbmcKdGhlIHNhbWUgcnVsZS4KCj4gPiB0aGUgcG9pbnQg aXMgSSBsZWFybmVkIGZyb20gb3RoZXIgc3Vic3lzdGVtIG1ha2VzIHRoZSBkcml2ZXIgbmFtZSBi ZQo+ID4gc2FtZSB3aXRoIHRoZSBtb2R1bGUgbmFtZSB3aXRoIEtCVUlMRF9NT0ROQU1FLgo+ID4g Cj4gPiBJZiB5b3UgcmVhbGx5IGRvbid0IGxpa2UgaXQsIEkgY2FuIGp1c3QgY2hhbmdlIGl0IGlu dG8gCj4gPiAKPiA+ICNkZWZpbmUgTVRLX0RNQV9ERVYgIm10ay1oc2RtYSIKPiAKPiBJdCBpcyB1 c2VkIG9ubHkgb25jZSwgd2h5IG5vdCB1c2UgS0JVSUxEX01PRE5BTUUgZGlyZWN0bHk/Cj4gCgpZ dXAsIGl0IGNhbiB1c2UgS0JVSUxEX01PRE5BTUUgZGlyZWN0bHkuCgo+ID4gCj4gPiA+ID4gKwo+ ID4gPiA+ICsjZGVmaW5lIE1US19IU0RNQV9VU0VDX1BPTEwJCTIwCj4gPiA+ID4gKyNkZWZpbmUg TVRLX0hTRE1BX1RJTUVPVVRfUE9MTAkJMjAwMDAwCj4gPiA+ID4gKyNkZWZpbmUgTVRLX0hTRE1B X0RNQV9CVVNXSURUSFMJCUJJVChETUFfU0xBVkVfQlVTV0lEVEhfVU5ERUZJTkVEKQo+ID4gPiAK PiA+ID4gVW5kZWZpbmVkIGJ1c3dpZHRoPz8KPiAKPiA/PwoKU29ycnkgZm9yIEkgZGlkbid0IGFu c3dlciB0aGUgcXVlc3Rpb24gaW4gdGhlIHNob3J0IHRpbWUuCgpBZnRlciBzcGVuZGluZyBzb21l IHRpbWUgb24gYSBjb25maXJtYXRpb24gd2l0aCBkZXNpZ24sIGl0IGlzCkRNQV9TTEFWRV9CVVNX SURUSF80X0JZVEVTIGFuZCBub3QgYmUgY29uZmlndXJhYmxlLiAKCj4gCj4gPiA+ID4gKy8qKgo+ ID4gPiA+ICsgKiBzdHJ1Y3QgbXRrX2hzZG1hX3BkZXNjIC0gVGhpcyBpcyB0aGUgc3RydWN0IGhv bGRpbmcgaW5mbyBkZXNjcmliaW5nIHBoeXNpY2FsCj4gPiA+ID4gKyAqCQkJICAgIGRlc2NyaXB0 b3IgKFBEKSBhbmQgaXRzIHBsYWNlbWVudCBtdXN0IGJlIGtlcHQgYXQKPiA+ID4gPiArICoJCQkg ICAgNC1ieXRlcyBhbGlnbm1lbnQgaW4gbGl0dGxlIGVuZGlhbiBvcmRlci4KPiA+ID4gPiArICog QGRlc2NbMS00XToJCSAgICBUaGUgY29udHJvbCBwYWQgdXNlZCB0byBpbmRpY2F0ZSBoYXJkd2Fy ZSBob3cgdG8KPiA+ID4gCj4gPiA+IHBscyBhbGlnbiB0byA4MGNoYXIgb3IgbGVzc2VyCj4gPiA+ IAo+ID4gCj4gPiB3ZWlyZCwgaXQgc2VlbXMgdGhlIGxpbmUgaXMgYWxyZWFkeSB3aXRoIDgwIGNo YXIgYW5kIHBhc3MgdGhlCj4gPiBjaGVja3BhdGNoLnBsLiBvciBkbyBJIG1pc3VuZGVyc3RhbmQg c29tZXRoaW5nID8KPiAKPiBPa2F5IHBsZWFzZSBjaGVjay4gV2l0aCB0ZXh0IGl0IGhlbHBzIHRv IHdyYXAgYmVmb3JlIHRoYXQKPiAKCkFmdGVyIGNoZWNrIGFnYWluLCB0aGVzZSBsaW5lcyBhcmUg YWxsIGFscmVhZHkgYWxpZ25lZCB0byA4MCBjaGFycwoKPiA+ID4gPiArCS8qCj4gPiA+ID4gKwkg KiBVcGRhdGluZyBpbnRvIGhhcmR3YXJlIHRoZSBwb2ludGVyIG9mIFRYIHJpbmcgbGV0cyBIU0RN QSB0byB0YWtlCj4gPiA+ID4gKwkgKiBhY3Rpb24gZm9yIHRob3NlIHBlbmRpbmcgUERzLgo+ID4g PiA+ICsJICovCj4gPiA+ID4gKwltdGtfZG1hX3dyaXRlKGhzZG1hLCBNVEtfSFNETUFfVFhfQ1BV LCByaW5nLT5jdXJfdHB0cik7Cj4gPiA+ID4gKwo+ID4gPiA+ICsJc3Bpbl91bmxvY2tfaXJxcmVz dG9yZSgmaHNkbWEtPmxvY2ssIGZsYWdzKTsKPiA+ID4gPiArCj4gPiA+ID4gKwlyZXR1cm4gIWh2 ZC0+bGVuID8gMCA6IC1FTk9TUEM7Cj4gPiA+IAo+ID4gPiB5b3UgYWxyZWFkeSB3cm90ZSBhbmQg c3RhcnRlZCB0eG4sIHNvIHdoeSB0aGlzPwo+ID4gPiAKPiA+IAo+ID4gaXQncyBwb3NzaWJsZSBq dXN0IHBhcnRpYWwgdmlydHVhbCBkZXNjcmlwdG9yIGZpdHMgaW50byBoYXJkd2FyZSBhbmQKPiA+ IHRoZW4gcmV0dXJuIC1FTk9TUEMuIEFuZCBpdCB3aWxsIHN0YXJ0IGl0IHRvIGNvbXBsZXRlIHRo ZSByZW1haW5pbmcgcGFydAo+ID4gYXMgc29vbiBhcyBwb3NzaWJsZSB3aGVuIHNvbWUgcm9vbXMg aXMgYmVpbmcgZnJlZWQuCj4gCj4gRWl0aGVyIHdheXMgeW91IGhhdmUgaXNzdWVkIHRoZSBkZXNj cmlwdG9yLCBzbyB5b3Ugc3VjY2VlZCByaWdodD8KPiAKCkkgdGhpbmsgSSBzaG91bGQgZ2V0IHlv dXIgcG9pbnRzLgoKSSBndWVzc2VkIHdoYXQgeW91IG1lYW50IGlzIHRoYXQgaXQgc2hvdWxkIGJl IHJldHVybmluZyAwIGluc3RlYWQgb2YKLUVOT1NQQyBmb3IgYWxsIHN1Y2Nlc3NmdWwgZGVzY3Jp cHRvciBpc3N1aW5nIGVpdGhlciBpbiBwYXJ0IG9yIGluIGZ1bGwKCkkgd2lsbCByZWZpbmUgdGhp cyBmbG93IGJhc2VkIG9uIHRoZSB0aG91Z2h0LgoKPiA+ID4gc2hvdWxkbid0IHdlIGNoZWNrIGlm IG5leHQgaXMgaW4gcmFuZ2UsIHdlIGNhbiBjcmFzaCBpZiB3ZSBnZXQgYmFkIHZhbHVlCj4gPiA+ IGZyb20gaGFyZHdhcmUuLgo+ID4gCj4gPiBva2F5LCB0aGVyZSBhcmUgY2hlY2tzIGZvciBuZXh0 IHdpdGggZGRvbmUgYml0IGNoZWNrIGFuZCBudWxsIGNoZWNrIGluCj4gPiB0aGUgY29ycmVzcG9u ZGluZyBkZXNjcmlwdG9yIGFzIHRoZSBmb2xsb3dpbmcuCj4gCj4gd2hhdCBpZiB5b3UgZ2V0IGJh ZCBuZXh0IHZhbHVlCj4gCgpuZXh0IGlzIG5vdCBoYXJkd2FyZSB2YWx1ZS4gaXQncyBtYWludGFp bmVkIGJ5IHNvZnR3YXJlIHdoaWNoIGlzIGFsd2F5cwpiZXR3ZWVuIDAgdG8gTVRLX0RNQV9TSVpF IC0gMSwgYW5kIGRlZmluaXRlbHkgZG9lc24ndCBnZXQgYSBiYWQgdmFsdWUuCgo+ID4gCj4gPiA+ ID4gKwkJcnhkID0gJnBjLT5yaW5nLnJ4ZFtuZXh0XTsKPiAKPiByZXN1bHRpbmcgaW4gYmFkIHJl ZiBoZXJlCgpyeGQgaXMgYWxzbyBkZWZpbml0ZWx5IGEgZ29vZCByZWYKCgo+Ci0tLQpUbyB1bnN1 YnNjcmliZSBmcm9tIHRoaXMgbGlzdDogc2VuZCB0aGUgbGluZSAidW5zdWJzY3JpYmUgZG1hZW5n aW5lIiBpbgp0aGUgYm9keSBvZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21vQHZnZXIua2VybmVsLm9y ZwpNb3JlIG1ham9yZG9tbyBpbmZvIGF0ICBodHRwOi8vdmdlci5rZXJuZWwub3JnL21ham9yZG9t by1pbmZvLmh0bWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Wang Subject: Re: [PATCH v5 2/3] dmaengine: mediatek: Add MediaTek High-Speed DMA controller for MT7622 and MT7623 SoC Date: Fri, 2 Mar 2018 14:47:51 +0800 Message-ID: <1519973271.8089.166.camel@mtkswgap22> References: <20180301082329.GD15443@localhost> <1519900021.8089.136.camel@mtkswgap22> <20180301125649.GH15443@localhost> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20180301125649.GH15443@localhost> Sender: linux-kernel-owner@vger.kernel.org To: Vinod Koul Cc: dan.j.williams@intel.com, robh+dt@kernel.org, mark.rutland@arm.com, dmaengine@vger.kernel.org, devicetree@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Randy Dunlap , Fengguang Wu , Julia Lawall List-Id: linux-mediatek@lists.infradead.org Hi, Vinod On Thu, 2018-03-01 at 18:26 +0530, Vinod Koul wrote: > On Thu, Mar 01, 2018 at 06:27:01PM +0800, Sean Wang wrote: > > On Thu, 2018-03-01 at 13:53 +0530, Vinod Koul wrote: > > > On Sun, Feb 18, 2018 at 03:08:30AM +0800, sean.wang@mediatek.com wrote: > > > > > > > @@ -0,0 +1,1054 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > // Copyright ... > > > > > > The copyright line needs to follow SPDX tag line > > > > > > > okay, I will make it reorder and be something like that > > > > // SPDX-License-Identifier: GPL-2.0 > > /* > > * Copyright (c) 2017-2018 MediaTek Inc. > > * Author: Sean Wang > > * > > * Driver for MediaTek High-Speed DMA Controller > > * > > */ > > It needs to be: > > // SPDX-License-Identifier: GPL-2.0 > // Copyright (c) 2017-2018 MediaTek Inc. > > /* > * whatever else you want > */ > > The first two lines are in C99 style comment and need to have SPDX tag and > Copyright info Sure, I can do it using C99 style comments at the first two lines. In addition, I'm really curious where we can find a reference to the rule and if it 's a strict rule for all the drivers. Because I'm considering whether I should turn other driver into using the same rule. > > the point is I learned from other subsystem makes the driver name be > > same with the module name with KBUILD_MODNAME. > > > > If you really don't like it, I can just change it into > > > > #define MTK_DMA_DEV "mtk-hsdma" > > It is used only once, why not use KBUILD_MODNAME directly? > Yup, it can use KBUILD_MODNAME directly. > > > > > > + > > > > +#define MTK_HSDMA_USEC_POLL 20 > > > > +#define MTK_HSDMA_TIMEOUT_POLL 200000 > > > > +#define MTK_HSDMA_DMA_BUSWIDTHS BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED) > > > > > > Undefined buswidth?? > > ?? Sorry for I didn't answer the question in the short time. After spending some time on a confirmation with design, it is DMA_SLAVE_BUSWIDTH_4_BYTES and not be configurable. > > > > > +/** > > > > + * struct mtk_hsdma_pdesc - This is the struct holding info describing physical > > > > + * descriptor (PD) and its placement must be kept at > > > > + * 4-bytes alignment in little endian order. > > > > + * @desc[1-4]: The control pad used to indicate hardware how to > > > > > > pls align to 80char or lesser > > > > > > > weird, it seems the line is already with 80 char and pass the > > checkpatch.pl. or do I misunderstand something ? > > Okay please check. With text it helps to wrap before that > After check again, these lines are all already aligned to 80 chars > > > > + /* > > > > + * Updating into hardware the pointer of TX ring lets HSDMA to take > > > > + * action for those pending PDs. > > > > + */ > > > > + mtk_dma_write(hsdma, MTK_HSDMA_TX_CPU, ring->cur_tptr); > > > > + > > > > + spin_unlock_irqrestore(&hsdma->lock, flags); > > > > + > > > > + return !hvd->len ? 0 : -ENOSPC; > > > > > > you already wrote and started txn, so why this? > > > > > > > it's possible just partial virtual descriptor fits into hardware and > > then return -ENOSPC. And it will start it to complete the remaining part > > as soon as possible when some rooms is being freed. > > Either ways you have issued the descriptor, so you succeed right? > I think I should get your points. I guessed what you meant is that it should be returning 0 instead of -ENOSPC for all successful descriptor issuing either in part or in full I will refine this flow based on the thought. > > > shouldn't we check if next is in range, we can crash if we get bad value > > > from hardware.. > > > > okay, there are checks for next with ddone bit check and null check in > > the corresponding descriptor as the following. > > what if you get bad next value > next is not hardware value. it's maintained by software which is always between 0 to MTK_DMA_SIZE - 1, and definitely doesn't get a bad value. > > > > > > + rxd = &pc->ring.rxd[next]; > > resulting in bad ref here rxd is also definitely a good ref > From mboxrd@z Thu Jan 1 00:00:00 1970 From: sean.wang@mediatek.com (Sean Wang) Date: Fri, 2 Mar 2018 14:47:51 +0800 Subject: [PATCH v5 2/3] dmaengine: mediatek: Add MediaTek High-Speed DMA controller for MT7622 and MT7623 SoC In-Reply-To: <20180301125649.GH15443@localhost> References: <20180301082329.GD15443@localhost> <1519900021.8089.136.camel@mtkswgap22> <20180301125649.GH15443@localhost> Message-ID: <1519973271.8089.166.camel@mtkswgap22> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi, Vinod On Thu, 2018-03-01 at 18:26 +0530, Vinod Koul wrote: > On Thu, Mar 01, 2018 at 06:27:01PM +0800, Sean Wang wrote: > > On Thu, 2018-03-01 at 13:53 +0530, Vinod Koul wrote: > > > On Sun, Feb 18, 2018 at 03:08:30AM +0800, sean.wang at mediatek.com wrote: > > > > > > > @@ -0,0 +1,1054 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > // Copyright ... > > > > > > The copyright line needs to follow SPDX tag line > > > > > > > okay, I will make it reorder and be something like that > > > > // SPDX-License-Identifier: GPL-2.0 > > /* > > * Copyright (c) 2017-2018 MediaTek Inc. > > * Author: Sean Wang > > * > > * Driver for MediaTek High-Speed DMA Controller > > * > > */ > > It needs to be: > > // SPDX-License-Identifier: GPL-2.0 > // Copyright (c) 2017-2018 MediaTek Inc. > > /* > * whatever else you want > */ > > The first two lines are in C99 style comment and need to have SPDX tag and > Copyright info Sure, I can do it using C99 style comments at the first two lines. In addition, I'm really curious where we can find a reference to the rule and if it 's a strict rule for all the drivers. Because I'm considering whether I should turn other driver into using the same rule. > > the point is I learned from other subsystem makes the driver name be > > same with the module name with KBUILD_MODNAME. > > > > If you really don't like it, I can just change it into > > > > #define MTK_DMA_DEV "mtk-hsdma" > > It is used only once, why not use KBUILD_MODNAME directly? > Yup, it can use KBUILD_MODNAME directly. > > > > > > + > > > > +#define MTK_HSDMA_USEC_POLL 20 > > > > +#define MTK_HSDMA_TIMEOUT_POLL 200000 > > > > +#define MTK_HSDMA_DMA_BUSWIDTHS BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED) > > > > > > Undefined buswidth?? > > ?? Sorry for I didn't answer the question in the short time. After spending some time on a confirmation with design, it is DMA_SLAVE_BUSWIDTH_4_BYTES and not be configurable. > > > > > +/** > > > > + * struct mtk_hsdma_pdesc - This is the struct holding info describing physical > > > > + * descriptor (PD) and its placement must be kept at > > > > + * 4-bytes alignment in little endian order. > > > > + * @desc[1-4]: The control pad used to indicate hardware how to > > > > > > pls align to 80char or lesser > > > > > > > weird, it seems the line is already with 80 char and pass the > > checkpatch.pl. or do I misunderstand something ? > > Okay please check. With text it helps to wrap before that > After check again, these lines are all already aligned to 80 chars > > > > + /* > > > > + * Updating into hardware the pointer of TX ring lets HSDMA to take > > > > + * action for those pending PDs. > > > > + */ > > > > + mtk_dma_write(hsdma, MTK_HSDMA_TX_CPU, ring->cur_tptr); > > > > + > > > > + spin_unlock_irqrestore(&hsdma->lock, flags); > > > > + > > > > + return !hvd->len ? 0 : -ENOSPC; > > > > > > you already wrote and started txn, so why this? > > > > > > > it's possible just partial virtual descriptor fits into hardware and > > then return -ENOSPC. And it will start it to complete the remaining part > > as soon as possible when some rooms is being freed. > > Either ways you have issued the descriptor, so you succeed right? > I think I should get your points. I guessed what you meant is that it should be returning 0 instead of -ENOSPC for all successful descriptor issuing either in part or in full I will refine this flow based on the thought. > > > shouldn't we check if next is in range, we can crash if we get bad value > > > from hardware.. > > > > okay, there are checks for next with ddone bit check and null check in > > the corresponding descriptor as the following. > > what if you get bad next value > next is not hardware value. it's maintained by software which is always between 0 to MTK_DMA_SIZE - 1, and definitely doesn't get a bad value. > > > > > > + rxd = &pc->ring.rxd[next]; > > resulting in bad ref here rxd is also definitely a good ref > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1163907AbeCBGr7 (ORCPT ); Fri, 2 Mar 2018 01:47:59 -0500 Received: from mailgw02.mediatek.com ([210.61.82.184]:28971 "EHLO mailgw02.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S935863AbeCBGr5 (ORCPT ); Fri, 2 Mar 2018 01:47:57 -0500 X-UUID: ba102993478f40ecb36aad222d88a6f3-20180302 Message-ID: <1519973271.8089.166.camel@mtkswgap22> Subject: Re: [PATCH v5 2/3] dmaengine: mediatek: Add MediaTek High-Speed DMA controller for MT7622 and MT7623 SoC From: Sean Wang To: Vinod Koul CC: , , , , , , , , Randy Dunlap , Fengguang Wu , Julia Lawall Date: Fri, 2 Mar 2018 14:47:51 +0800 In-Reply-To: <20180301125649.GH15443@localhost> References: <20180301082329.GD15443@localhost> <1519900021.8089.136.camel@mtkswgap22> <20180301125649.GH15443@localhost> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit MIME-Version: 1.0 X-MTK: N Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, Vinod On Thu, 2018-03-01 at 18:26 +0530, Vinod Koul wrote: > On Thu, Mar 01, 2018 at 06:27:01PM +0800, Sean Wang wrote: > > On Thu, 2018-03-01 at 13:53 +0530, Vinod Koul wrote: > > > On Sun, Feb 18, 2018 at 03:08:30AM +0800, sean.wang@mediatek.com wrote: > > > > > > > @@ -0,0 +1,1054 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > // Copyright ... > > > > > > The copyright line needs to follow SPDX tag line > > > > > > > okay, I will make it reorder and be something like that > > > > // SPDX-License-Identifier: GPL-2.0 > > /* > > * Copyright (c) 2017-2018 MediaTek Inc. > > * Author: Sean Wang > > * > > * Driver for MediaTek High-Speed DMA Controller > > * > > */ > > It needs to be: > > // SPDX-License-Identifier: GPL-2.0 > // Copyright (c) 2017-2018 MediaTek Inc. > > /* > * whatever else you want > */ > > The first two lines are in C99 style comment and need to have SPDX tag and > Copyright info Sure, I can do it using C99 style comments at the first two lines. In addition, I'm really curious where we can find a reference to the rule and if it 's a strict rule for all the drivers. Because I'm considering whether I should turn other driver into using the same rule. > > the point is I learned from other subsystem makes the driver name be > > same with the module name with KBUILD_MODNAME. > > > > If you really don't like it, I can just change it into > > > > #define MTK_DMA_DEV "mtk-hsdma" > > It is used only once, why not use KBUILD_MODNAME directly? > Yup, it can use KBUILD_MODNAME directly. > > > > > > + > > > > +#define MTK_HSDMA_USEC_POLL 20 > > > > +#define MTK_HSDMA_TIMEOUT_POLL 200000 > > > > +#define MTK_HSDMA_DMA_BUSWIDTHS BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED) > > > > > > Undefined buswidth?? > > ?? Sorry for I didn't answer the question in the short time. After spending some time on a confirmation with design, it is DMA_SLAVE_BUSWIDTH_4_BYTES and not be configurable. > > > > > +/** > > > > + * struct mtk_hsdma_pdesc - This is the struct holding info describing physical > > > > + * descriptor (PD) and its placement must be kept at > > > > + * 4-bytes alignment in little endian order. > > > > + * @desc[1-4]: The control pad used to indicate hardware how to > > > > > > pls align to 80char or lesser > > > > > > > weird, it seems the line is already with 80 char and pass the > > checkpatch.pl. or do I misunderstand something ? > > Okay please check. With text it helps to wrap before that > After check again, these lines are all already aligned to 80 chars > > > > + /* > > > > + * Updating into hardware the pointer of TX ring lets HSDMA to take > > > > + * action for those pending PDs. > > > > + */ > > > > + mtk_dma_write(hsdma, MTK_HSDMA_TX_CPU, ring->cur_tptr); > > > > + > > > > + spin_unlock_irqrestore(&hsdma->lock, flags); > > > > + > > > > + return !hvd->len ? 0 : -ENOSPC; > > > > > > you already wrote and started txn, so why this? > > > > > > > it's possible just partial virtual descriptor fits into hardware and > > then return -ENOSPC. And it will start it to complete the remaining part > > as soon as possible when some rooms is being freed. > > Either ways you have issued the descriptor, so you succeed right? > I think I should get your points. I guessed what you meant is that it should be returning 0 instead of -ENOSPC for all successful descriptor issuing either in part or in full I will refine this flow based on the thought. > > > shouldn't we check if next is in range, we can crash if we get bad value > > > from hardware.. > > > > okay, there are checks for next with ddone bit check and null check in > > the corresponding descriptor as the following. > > what if you get bad next value > next is not hardware value. it's maintained by software which is always between 0 to MTK_DMA_SIZE - 1, and definitely doesn't get a bad value. > > > > > > + rxd = &pc->ring.rxd[next]; > > resulting in bad ref here rxd is also definitely a good ref >