From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eugeniy Paltsev Subject: Re: [RFC 0/2] dw_mmc: add multislot support Date: Fri, 20 Apr 2018 15:53:30 +0000 Message-ID: <1524239609.10138.47.camel@synopsys.com> References: <20180417121130.25281-1-Eugeniy.Paltsev@synopsys.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: Content-Language: en-US Content-ID: <0E79426BD2A16D41A85A936916640BBD@internal.synopsys.com> Sender: linux-kernel-owner@vger.kernel.org To: "ulf.hansson@linaro.org" , "Eugeniy.Paltsev@synopsys.com" Cc: "jh80.chung@samsung.com" , "linux-kernel@vger.kernel.org" , "Alexey.Brodkin@synopsys.com" , "linux-mmc@vger.kernel.org" , "krzk@kernel.org" , "linux-snps-arc@lists.infradead.org" , "kgene@kernel.org" , "shawn.lin@rock-chips.com" , "linux-arm-kernel@lists.infradead.org" , "linux-samsung-soc@vger.kernel.org" List-Id: linux-mmc@vger.kernel.org SGkgVWxmLA0KDQpPbiBGcmksIDIwMTgtMDQtMjAgYXQgMDk6MzUgKzAyMDAsIFVsZiBIYW5zc29u IHdyb3RlOg0KPiBbLi4uXQ0KPiANCj4gPiANCj4gPiAyLiBBZGQgbWlzc2luZyBzdHVmZiB0byBz dXBwb3J0IG11bHRpc2xvdCBtb2RlIGluIERlc2lnbldhcmUgTU1DIGRyaXZlci4NCj4gPiAgKiBB ZGQgbWlzc2luZyBzbG90IHN3aXRjaCB0byBfX2R3X21jaV9zdGFydF9yZXF1ZXN0KCkgZnVuY3Rp b24uDQo+ID4gICogUmVmYWN0b3Igc2V0X2lvcyBmdW5jdGlvbjoNCj4gPiAgICBhKSBDYWxjdWxh dGUgY29tbW9uIGNsb2NrIHdoaWNoIGlzDQo+ID4gICAgICAgc3VpdGFibGUgZm9yIGFsbCBzbG90 cyBpbnN0ZWFkIG9mIGRpcmVjdGx5IHVzZSBjbG9jayB2YWx1ZQ0KPiA+ICAgICAgIHByb3ZpZGVk IGJ5IG1tYyBjb3JlLiBXZSBjYWxjdWxhdGUgY29tbW9uIGNsb2NrIGFzIHRoZSBtaW5pbXVtDQo+ ID4gICAgICAgYW1vbmcgZWFjaCB1c2VkIHNsb3QgY2xvY2tzLiBUaGlzIGNsb2NrIGlzIGNhbGN1 bGF0ZWQgaW4NCj4gPiAgICAgICBkd19tY2lfY2FsY19jb21tb25fY2xvY2soKSBmdW5jdGlvbiB3 aGljaCBpcyBjYWxsZWQNCj4gPiAgICAgICBmcm9tIHNldF9pb3MoKQ0KPiA+ICAgIGIpIERpc2Fi bGUgY2xvY2sgb25seSBpZiBubyBvdGhlciBzbG90cyBhcmUgT04uDQo+ID4gICAgYykgU2V0dXAg Y2xvY2sgZGlyZWN0bHkgaW4gc2V0X2lvcygpIG9ubHkgaWYgbm8gb3RoZXIgc2xvdHMNCj4gPiAg ICAgICBhcmUgT04uIE90aGVyd2lzZSBhZGp1c3QgY2xvY2sgaW4gX19kd19tY2lfc3RhcnRfcmVx dWVzdCgpDQo+ID4gICAgICAgZnVuY3Rpb24gYmVmb3JlIHNsb3Qgc3dpdGNoLg0KPiA+ICAgIGQp IE1vdmUgdGltaW5ncyBhbmQgYnVzX3dpZHRoIHNldHVwIHRvIHNlcGFyYXRlIGZ1bmNpb25zLg0K PiA+ICAqIFVzZSB0aW1pbmcgZmllbGQgaW4gZWFjaCBzbG90IHN0cnVjdHVyZSBpbnN0ZWFkIG9m IGNvbW1vbiBmaWVsZCBpbg0KPiA+ICAgIGhvc3Qgc3RydWN0dXJlLg0KPiA+ICAqIEFkZCBsb2Nr cyB0byBzZXJpYWxpemUgYWNjZXNzIHRvIHJlZ2lzdGVycy4NCj4gDQo+IFNvcnJ5LCBidXQgdGhp cyBpcyBhIGhhY2sgdG8gKnRyeSogdG8gbWFrZSBtdWx0aS1zbG90IHdvcmsgYW5kIHRoaXMNCj4g aXNuJ3Qgc3VmZmljaWVudC4gVGhlcmUgd2VyZSBnb29kIHJlYXNvbnMgdG8gd2h5IHRoZSBlYXJs aWVyDQo+IG5vbi13b3JraW5nIG11bHRpIHNsb3Qgc3VwcG9ydCB3YXMgcmVtb3ZlZCBmcm9tIGR3 X21tYy4NCg0KUHJldmlvdXMgbXVsdGkgc2xvdCBpbXBsZW1lbnRhdGlvbiB3YXMgcmVtb3ZlZCBh cyBub2JvZHkgdXNlZCBpdCBhbmQNCm5vYm9keSB0ZXN0ZWQgaXQuIFRoZXJlIGFyZSBsb3RzIG9m IG1pc3Rha2VzIGluIHByZXZpb3VzIGltcGxlbWVudGF0aW9uDQp3aGljaCBhcmUgbm90IHJlbGF0 ZWQgdG8gcmVxdWVzdCBzZXJpYWxpemF0aW9uDQpsaWtlIGxhY2sgb2Ygc2xvdCBzd2l0Y2ggLyBs YWNrIG9mIGFkZGluZyBzbG90IGlkIHRvIENJVSBjb21tYW5kcyAvIGV0cy4uLg0KU28gb2J2aW91 c2x5IGl0IHdhcyBuZXZlciB0ZXN0ZWQgYW5kIG5ldmVyIHVzZWQgYXQgcmVhbCBtdWx0aSBzbG90 IGhhcmR3YXJlLg0KDQo+IExldCBtZSBlbGFib3JhdGUgYSBiaXQgZm9yIHlvdXIgdW5kZXJzdGFu ZGluZy4gVGhlIGNvcmUgdXNlcyBhIGhvc3QNCj4gbG9jayAobW1jX2NsYWltfHJlbGVhc2VfaG9z dCgpKSB0byBzZXJpYWxpemUgb3BlcmF0aW9ucyBhbmQgY29tbWFuZHMsDQo+IGFzIHRvIGNvbmZp cm0gdG8gdGhlIFNEL1NESU8vKGUpTU1DIHNwZWNzLiBUaGUgYWJvdmUgY2hhbmdlcyBnaXZlcyBu bw0KPiBndWFyYW50ZWVzIGZvciB0aGlzLiBUbyBtYWtlIHRoYXQgd29yaywgd2Ugd291bGQgbmVl ZCBhICJtbWMgYnVzIGxvY2siDQo+IHRvIGJlIG1hbmFnZWQgYnkgdGhlIGNvcmUuDQoNCkluIGN1 cnJlbnQgaW1wbGVtZW50YXRpb24gZGF0YSB0cmFuc2ZlcnMgYW5kIGNvbW1hbmRzIHRvIGRpZmZl cmVudA0KaG9zdHMgKHNsb3RzKSBhcmUgc2VyaWFsaXplZCBpbnRlcm5hbGx5IGluIHRoZSBkd19t bWMgZHJpdmVyLiBXZSBoYXZlDQpyZXF1ZXN0IHF1ZXVlIGFuZCB3aGVuIC5yZXF1ZXN0KCkgaXMg Y2FsbGVkIHdlIGFkZCBuZXcgcmVxdWVzdCB0byB0aGUNCnF1ZXVlLiBXZSB0YWtlIG5ldyByZXF1 ZXN0IGZyb20gdGhlIHF1ZXVlIG9ubHkgaWYgdGhlIHByZXZpb3VzIG9uZQ0KaGFzIGFscmVhZHkg ZmluaXNoZWQuDQoNClNvIGFsdGhvdWdoIGhvc3RzIChzbG90cykgaGF2ZSBzZXBhcmF0ZSBsb2Nr cyAobW1jX2NsYWltfHJlbGVhc2VfaG9zdCgpKQ0KdGhlIHJlcXVlc3RzIHRvIGRpZmZlcmVudCBz bG90cyBhcmUgc2VyaWFsaXplZCBieSBkcml2ZXIuDQoNCklzbid0IHRoYXQgZW5vdWdoPw0KSSdt IG5vdCB2ZXJ5IGZhbWlsaWFyIHdpdGggU0QvU0RJTy8oZSlNTUMgc3BlY3Mgc28gbXkgYXNzdW1w dGlvbnMgbWlnaHQgYmUgd3JvbmcNCmluIHRoYXQgY2FzZSBwbGVhc2UgY29ycmVjdCBtZS4NCg0K PiBIb3dldmVyLCBpbnZlbnRpbmcgYSAibW1jIGJ1cyBsb2NrIiB3b3VsZCBsZWFkIHRvIG90aGVy IHByb2JsZW1zDQo+IHJlbGF0ZWQgdG8gSS9PIHNjaGVkdWxpbmcgZm9yIHVwcGVyIGxheWVycyAt IGl0IHNpbXBseSBicmVha3MuIEZvcg0KPiBleGFtcGxlLCBJL08gcmVxdWVzdHMgZm9yIG9uZSBj YXJkL3Nsb3QgY2FuIHRoZW4gc3RhcnZlIEkvTyByZXF1ZXN0cw0KPiByZWFjaGluZyBhbm90aGVy IGNhcmQvc2xvdC4NCj4gDQoNCk5ldmVydGhlbGVzcyB3ZSBoYWQgdG8gZGVhbCBzb21laG93IHdp dGggZXhpc3RpbmcgaGFyZHdhcmUgd2hpY2gNCmhhcyBtdWx0aXNsb3QgZHcgbW1jIGNvbnRyb2xs ZXIgYW5kIGJvdGggc2xvdHMgYXJlIHVzZWQuLi4NCg0KVGhpcyBwYXRjaCBhdCBsZWFzdCBzaG91 bGRuJ3QgYnJlYWsgYW55dGhpbmcgZm9yIGN1cnJlbnQgdXNlcnMgKHdoaWNoIHVzZQ0KaXQgaW4g c2luZ2xlIHNsb3QgbW9kZSkNCg0KTW9yZW92ZXIgd2UgdGVzdGVkIHRoaXMgZHVhbC1zbG90IGlt cGxlbWVudGF0aW9uIGFuZCBkb24ndCBjYXRjaCBhbnkgcHJvYmxlbXMNCihwcm9iYWJseSB5ZXQp IGV4Y2VwdCBidXMgcGVyZm9ybWFuY2UgZGVjcmVhc2UgaW4gZHVhbC1zbG90IG1vZGUgKHdoaWNo IGlzDQpxdWl0ZSBleHBlY3RlZCkuDQoNCj4gDQo+IEtpbmQgcmVnYXJkcw0KPiBVZmZlDQotLSAN CiBFdWdlbml5IFBhbHRzZXY= From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eugeniy.Paltsev@synopsys.com (Eugeniy Paltsev) Date: Fri, 20 Apr 2018 15:53:30 +0000 Subject: [RFC 0/2] dw_mmc: add multislot support In-Reply-To: References: <20180417121130.25281-1-Eugeniy.Paltsev@synopsys.com> List-ID: Message-ID: <1524239609.10138.47.camel@synopsys.com> To: linux-snps-arc@lists.infradead.org Hi Ulf, On Fri, 2018-04-20@09:35 +0200, Ulf Hansson wrote: > [...] > > > > > 2. Add missing stuff to support multislot mode in DesignWare MMC driver. > > * Add missing slot switch to __dw_mci_start_request() function. > > * Refactor set_ios function: > > a) Calculate common clock which is > > suitable for all slots instead of directly use clock value > > provided by mmc core. We calculate common clock as the minimum > > among each used slot clocks. This clock is calculated in > > dw_mci_calc_common_clock() function which is called > > from set_ios() > > b) Disable clock only if no other slots are ON. > > c) Setup clock directly in set_ios() only if no other slots > > are ON. Otherwise adjust clock in __dw_mci_start_request() > > function before slot switch. > > d) Move timings and bus_width setup to separate funcions. > > * Use timing field in each slot structure instead of common field in > > host structure. > > * Add locks to serialize access to registers. > > Sorry, but this is a hack to *try* to make multi-slot work and this > isn't sufficient. There were good reasons to why the earlier > non-working multi slot support was removed from dw_mmc. Previous multi slot implementation was removed as nobody used it and nobody tested it. There are lots of mistakes in previous implementation which are not related to request serialization like lack of slot switch / lack of adding slot id to CIU commands / ets... So obviously it was never tested and never used at real multi slot hardware. > Let me elaborate a bit for your understanding. The core uses a host > lock (mmc_claim|release_host()) to serialize operations and commands, > as to confirm to the SD/SDIO/(e)MMC specs. The above changes gives no > guarantees for this. To make that work, we would need a "mmc bus lock" > to be managed by the core. In current implementation data transfers and commands to different hosts (slots) are serialized internally in the dw_mmc driver. We have request queue and when .request() is called we add new request to the queue. We take new request from the queue only if the previous one has already finished. So although hosts (slots) have separate locks (mmc_claim|release_host()) the requests to different slots are serialized by driver. Isn't that enough? I'm not very familiar with SD/SDIO/(e)MMC specs so my assumptions might be wrong in that case please correct me. > However, inventing a "mmc bus lock" would lead to other problems > related to I/O scheduling for upper layers - it simply breaks. For > example, I/O requests for one card/slot can then starve I/O requests > reaching another card/slot. > Nevertheless we had to deal somehow with existing hardware which has multislot dw mmc controller and both slots are used... This patch at least shouldn't break anything for current users (which use it in single slot mode) Moreover we tested this dual-slot implementation and don't catch any problems (probably yet) except bus performance decrease in dual-slot mode (which is quite expected). > > Kind regards > Uffe -- Eugeniy Paltsev From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eugeniy.Paltsev@synopsys.com (Eugeniy Paltsev) Date: Fri, 20 Apr 2018 15:53:30 +0000 Subject: [RFC 0/2] dw_mmc: add multislot support In-Reply-To: References: <20180417121130.25281-1-Eugeniy.Paltsev@synopsys.com> Message-ID: <1524239609.10138.47.camel@synopsys.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Ulf, On Fri, 2018-04-20 at 09:35 +0200, Ulf Hansson wrote: > [...] > > > > > 2. Add missing stuff to support multislot mode in DesignWare MMC driver. > > * Add missing slot switch to __dw_mci_start_request() function. > > * Refactor set_ios function: > > a) Calculate common clock which is > > suitable for all slots instead of directly use clock value > > provided by mmc core. We calculate common clock as the minimum > > among each used slot clocks. This clock is calculated in > > dw_mci_calc_common_clock() function which is called > > from set_ios() > > b) Disable clock only if no other slots are ON. > > c) Setup clock directly in set_ios() only if no other slots > > are ON. Otherwise adjust clock in __dw_mci_start_request() > > function before slot switch. > > d) Move timings and bus_width setup to separate funcions. > > * Use timing field in each slot structure instead of common field in > > host structure. > > * Add locks to serialize access to registers. > > Sorry, but this is a hack to *try* to make multi-slot work and this > isn't sufficient. There were good reasons to why the earlier > non-working multi slot support was removed from dw_mmc. Previous multi slot implementation was removed as nobody used it and nobody tested it. There are lots of mistakes in previous implementation which are not related to request serialization like lack of slot switch / lack of adding slot id to CIU commands / ets... So obviously it was never tested and never used at real multi slot hardware. > Let me elaborate a bit for your understanding. The core uses a host > lock (mmc_claim|release_host()) to serialize operations and commands, > as to confirm to the SD/SDIO/(e)MMC specs. The above changes gives no > guarantees for this. To make that work, we would need a "mmc bus lock" > to be managed by the core. In current implementation data transfers and commands to different hosts (slots) are serialized internally in the dw_mmc driver. We have request queue and when .request() is called we add new request to the queue. We take new request from the queue only if the previous one has already finished. So although hosts (slots) have separate locks (mmc_claim|release_host()) the requests to different slots are serialized by driver. Isn't that enough? I'm not very familiar with SD/SDIO/(e)MMC specs so my assumptions might be wrong in that case please correct me. > However, inventing a "mmc bus lock" would lead to other problems > related to I/O scheduling for upper layers - it simply breaks. For > example, I/O requests for one card/slot can then starve I/O requests > reaching another card/slot. > Nevertheless we had to deal somehow with existing hardware which has multislot dw mmc controller and both slots are used... This patch at least shouldn't break anything for current users (which use it in single slot mode) Moreover we tested this dual-slot implementation and don't catch any problems (probably yet) except bus performance decrease in dual-slot mode (which is quite expected). > > Kind regards > Uffe -- Eugeniy Paltsev