From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eugeniy Paltsev Subject: Re: [RFC 0/2] dw_mmc: add multislot support Date: Wed, 25 Apr 2018 13:53:38 +0000 Message-ID: <1524664417.18209.16.camel@synopsys.com> References: <20180417121130.25281-1-Eugeniy.Paltsev@synopsys.com> <1524239609.10138.47.camel@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: Sender: linux-kernel-owner@vger.kernel.org To: "ulf.hansson@linaro.org" 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 T24gTW9uLCAyMDE4LTA0LTIzIGF0IDA4OjQ3ICswMjAwLCBVbGYgSGFuc3NvbiB3cm90ZToNCj4g T24gMjAgQXByaWwgMjAxOCBhdCAxNzo1MywgRXVnZW5peSBQYWx0c2V2IDxFdWdlbml5LlBhbHRz ZXZAc3lub3BzeXMuY29tPiB3cm90ZToNCj4gPiBIaSBVbGYsDQo+ID4gDQo+ID4gT24gRnJpLCAy MDE4LTA0LTIwIGF0IDA5OjM1ICswMjAwLCBVbGYgSGFuc3NvbiB3cm90ZToNCj4gPiA+IFsuLi5d DQo+ID4gPiANCj4gPiA+ID4gDQo+ID4gPiA+IDIuIEFkZCBtaXNzaW5nIHN0dWZmIHRvIHN1cHBv cnQgbXVsdGlzbG90IG1vZGUgaW4gRGVzaWduV2FyZSBNTUMgZHJpdmVyLg0KPiA+ID4gPiAgKiBB ZGQgbWlzc2luZyBzbG90IHN3aXRjaCB0byBfX2R3X21jaV9zdGFydF9yZXF1ZXN0KCkgZnVuY3Rp b24uDQo+ID4gPiA+ICAqIFJlZmFjdG9yIHNldF9pb3MgZnVuY3Rpb246DQo+ID4gPiA+ICAgIGEp IENhbGN1bGF0ZSBjb21tb24gY2xvY2sgd2hpY2ggaXMNCj4gPiA+ID4gICAgICAgc3VpdGFibGUg Zm9yIGFsbCBzbG90cyBpbnN0ZWFkIG9mIGRpcmVjdGx5IHVzZSBjbG9jayB2YWx1ZQ0KPiA+ID4g PiAgICAgICBwcm92aWRlZCBieSBtbWMgY29yZS4gV2UgY2FsY3VsYXRlIGNvbW1vbiBjbG9jayBh cyB0aGUgbWluaW11bQ0KPiA+ID4gPiAgICAgICBhbW9uZyBlYWNoIHVzZWQgc2xvdCBjbG9ja3Mu IFRoaXMgY2xvY2sgaXMgY2FsY3VsYXRlZCBpbg0KPiA+ID4gPiAgICAgICBkd19tY2lfY2FsY19j b21tb25fY2xvY2soKSBmdW5jdGlvbiB3aGljaCBpcyBjYWxsZWQNCj4gPiA+ID4gICAgICAgZnJv bSBzZXRfaW9zKCkNCj4gPiA+ID4gICAgYikgRGlzYWJsZSBjbG9jayBvbmx5IGlmIG5vIG90aGVy IHNsb3RzIGFyZSBPTi4NCj4gPiA+ID4gICAgYykgU2V0dXAgY2xvY2sgZGlyZWN0bHkgaW4gc2V0 X2lvcygpIG9ubHkgaWYgbm8gb3RoZXIgc2xvdHMNCj4gPiA+ID4gICAgICAgYXJlIE9OLiBPdGhl cndpc2UgYWRqdXN0IGNsb2NrIGluIF9fZHdfbWNpX3N0YXJ0X3JlcXVlc3QoKQ0KPiA+ID4gPiAg ICAgICBmdW5jdGlvbiBiZWZvcmUgc2xvdCBzd2l0Y2guDQo+ID4gPiA+ICAgIGQpIE1vdmUgdGlt aW5ncyBhbmQgYnVzX3dpZHRoIHNldHVwIHRvIHNlcGFyYXRlIGZ1bmNpb25zLg0KPiA+ID4gPiAg KiBVc2UgdGltaW5nIGZpZWxkIGluIGVhY2ggc2xvdCBzdHJ1Y3R1cmUgaW5zdGVhZCBvZiBjb21t b24gZmllbGQgaW4NCj4gPiA+ID4gICAgaG9zdCBzdHJ1Y3R1cmUuDQo+ID4gPiA+ICAqIEFkZCBs b2NrcyB0byBzZXJpYWxpemUgYWNjZXNzIHRvIHJlZ2lzdGVycy4NCj4gPiA+IA0KPiA+ID4gU29y cnksIGJ1dCB0aGlzIGlzIGEgaGFjayB0byAqdHJ5KiB0byBtYWtlIG11bHRpLXNsb3Qgd29yayBh bmQgdGhpcw0KPiA+ID4gaXNuJ3Qgc3VmZmljaWVudC4gVGhlcmUgd2VyZSBnb29kIHJlYXNvbnMg dG8gd2h5IHRoZSBlYXJsaWVyDQo+ID4gPiBub24td29ya2luZyBtdWx0aSBzbG90IHN1cHBvcnQg d2FzIHJlbW92ZWQgZnJvbSBkd19tbWMuDQo+ID4gDQo+ID4gUHJldmlvdXMgbXVsdGkgc2xvdCBp bXBsZW1lbnRhdGlvbiB3YXMgcmVtb3ZlZCBhcyBub2JvZHkgdXNlZCBpdCBhbmQNCj4gPiBub2Jv ZHkgdGVzdGVkIGl0LiBUaGVyZSBhcmUgbG90cyBvZiBtaXN0YWtlcyBpbiBwcmV2aW91cyBpbXBs ZW1lbnRhdGlvbg0KPiA+IHdoaWNoIGFyZSBub3QgcmVsYXRlZCB0byByZXF1ZXN0IHNlcmlhbGl6 YXRpb24NCj4gPiBsaWtlIGxhY2sgb2Ygc2xvdCBzd2l0Y2ggLyBsYWNrIG9mIGFkZGluZyBzbG90 IGlkIHRvIENJVSBjb21tYW5kcyAvIGV0cy4uLg0KPiA+IFNvIG9idmlvdXNseSBpdCB3YXMgbmV2 ZXIgdGVzdGVkIGFuZCBuZXZlciB1c2VkIGF0IHJlYWwgbXVsdGkgc2xvdCBoYXJkd2FyZS4NCj4g PiANCj4gPiA+IExldCBtZSBlbGFib3JhdGUgYSBiaXQgZm9yIHlvdXIgdW5kZXJzdGFuZGluZy4g VGhlIGNvcmUgdXNlcyBhIGhvc3QNCj4gPiA+IGxvY2sgKG1tY19jbGFpbXxyZWxlYXNlX2hvc3Qo KSkgdG8gc2VyaWFsaXplIG9wZXJhdGlvbnMgYW5kIGNvbW1hbmRzLA0KPiA+ID4gYXMgdG8gY29u ZmlybSB0byB0aGUgU0QvU0RJTy8oZSlNTUMgc3BlY3MuIFRoZSBhYm92ZSBjaGFuZ2VzIGdpdmVz IG5vDQo+ID4gPiBndWFyYW50ZWVzIGZvciB0aGlzLiBUbyBtYWtlIHRoYXQgd29yaywgd2Ugd291 bGQgbmVlZCBhICJtbWMgYnVzIGxvY2siDQo+ID4gPiB0byBiZSBtYW5hZ2VkIGJ5IHRoZSBjb3Jl Lg0KPiA+IA0KPiA+IEluIGN1cnJlbnQgaW1wbGVtZW50YXRpb24gZGF0YSB0cmFuc2ZlcnMgYW5k IGNvbW1hbmRzIHRvIGRpZmZlcmVudA0KPiA+IGhvc3RzIChzbG90cykgYXJlIHNlcmlhbGl6ZWQg aW50ZXJuYWxseSBpbiB0aGUgZHdfbW1jIGRyaXZlci4gV2UgaGF2ZQ0KPiA+IHJlcXVlc3QgcXVl dWUgYW5kIHdoZW4gLnJlcXVlc3QoKSBpcyBjYWxsZWQgd2UgYWRkIG5ldyByZXF1ZXN0IHRvIHRo ZQ0KPiA+IHF1ZXVlLiBXZSB0YWtlIG5ldyByZXF1ZXN0IGZyb20gdGhlIHF1ZXVlIG9ubHkgaWYg dGhlIHByZXZpb3VzIG9uZQ0KPiA+IGhhcyBhbHJlYWR5IGZpbmlzaGVkLg0KPiANCj4gVGhhdCBp c24ndCBzdWZmaWNpZW50LiBUaGUgY29yZSBleHBlY3RzIGFsbCBjYWxscyB0byAqYW55KiBvZiB0 aGUgaG9zdA0KPiBvcHMgdG8gYmUgc2VyaWFsaXplZCBmb3Igb25lIGhvc3QuIEl0IGRvZXMgc28g dG8gY29uZm9ybSB0byB0aGUgc3BlY3MuDQo+IA0KPiBGb3IgZXhhbXBsZSBpdCBtYXkgY2FsbDoN Cj4gIC0+c2V0X2lvcygpDQo+IC0+cmVxdWVzdCgpDQo+IC0+c2V0X2lvcygpDQo+IC0+cmVxdWVz dCgpDQo+IC0+cmVxdWVzdCgpDQo+IA0KDQpBIGJpdCByZW1hcmsgZm9yIGJldHRlciB1bmRlcnN0 YW5kaW5nOg0KDQpBbGwgY2FyZCBzZXR0aW5ncyBjaGFuZ2UgYXJlIHNlcmlhbGl6ZWQgdG9vLiBU aGVzZSBzZXR0aW5ncyBhcmUgYXBwbGllZA0KYWZ0ZXIgc2xvdCBzd2l0Y2ggYmVmb3JlIGV4ZWN1 dGlvbiBvZiBuZXcgcmVxdWVzdCBmb3IgdGhpcyBzbG90Lg0KDQpTbyBzaXR1YXRpb25zIGxpa2Ug Y2FsbGluZyBhbnkgaG9zdF8wIG9wcyB3aGlsZSBhbm90aGVyIGhvc3QgKGhvc3RfMSkgaXMgYWN0 aXZlDQphcmUgaGFuZGxlZCBieSBjdXJyZW50IGNvZGUuDQoNClRoaXMgaXMgZXhhbXBsZSBvZiBz aW11bHRhbmVvdXMgb3BzIGNhbGxzIGZvciBib3RoIHNsb3RzOg0KDQpob3N0IChzbG90KSAwICAg IHwgaG9zdCAoc2xvdCkgMQ0KLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0NCmgw LT5zZXRfaW9zKCkgICAgfCAgICBoMS0+c2V0X2lvcygpDQpoMC0+cmVxdWVzdCgpICAgIHwgICAg aDEtPnJlcXVlc3QoKQ0KaDAtPnNldF9pb3MoKSAgICB8ICAgIGgxLT5zZXRfaW9zKCkNCmgwLT5y ZXF1ZXN0KCkgICAgfCAgICBoMS0+cmVxdWVzdCgpDQpoMC0+cmVxdWVzdCgpICAgIHwNCmgwLT5y ZXF1ZXN0KCkgICAgfA0KaDAtPnJlcXVlc3QoKSAgICB8DQoNCkhvdyBpdCB3aWxsIGJlIHNlcmlh bGl6ZWQgaW4gdGhlIG1tYyBkcml2ZXI6DQoNCmgwLT5zZXRfaW9zKCkgICAvLyBoMCBzZXR0aW5n cyBzYXZlDQpoMS0+c2V0X2lvcygpICAgLy8gaDEgc2V0dGluZ3Mgc2F2ZQ0KaDAtPnJlcXVlc3Qo KSAgIC8vIGFwcGx5IHNldHRpbmdzIGZvciBoMCBhbmQgZG8gcmVxdWVzdA0KLS0tLS0tIHNsb3Qg c3dpdGNoIHRvIGgxIC0tLS0tLQ0KaDEtPnJlcXVlc3QoKSAgIC8vIGFwcGx5IHNldHRpbmdzIGZv ciBoMSBhbmQgZG8gcmVxdWVzdA0KaDAtPnNldF9pb3MoKSAgIC8vIGgwIHNldHRpbmdzIHNhdmUN CmgxLT5zZXRfaW9zKCkgICAvLyBoMSBzZXR0aW5ncyBzYXZlDQotLS0tLS0gc2xvdCBzd2l0Y2gg dG8gaDAgLS0tLS0tDQpoMC0+cmVxdWVzdCgpICAgLy8gYXBwbHkgc2V0dGluZ3MgZm9yIGgwIGFu ZCBkbyByZXF1ZXN0DQotLS0tLS0gc2xvdCBzd2l0Y2ggdG8gaDEgLS0tLS0tDQpoMS0+cmVxdWVz dCgpICAgLy8gYXBwbHkgc2V0dGluZ3MgZm9yIGgxIGFuZCBkbyByZXF1ZXN0DQotLS0tLS0gc2xv dCBzd2l0Y2ggdG8gaDAgLS0tLS0tDQpoMC0+cmVxdWVzdCgpICAgLy8gZG8gcmVxdWVzdCAobm8g bmV3IHNldHRpbmdzIHRvIGFwcGx5KQ0KaDAtPnJlcXVlc3QoKSAgIC8vIGRvIHJlcXVlc3QgKG5v IG5ldyBzZXR0aW5ncyB0byBhcHBseSkNCmgwLT5yZXF1ZXN0KCkgICAvLyBkbyByZXF1ZXN0IChu byBuZXcgc2V0dGluZ3MgdG8gYXBwbHkpDQoNCi0tIA0KIEV1Z2VuaXkgUGFsdHNldg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eugeniy.Paltsev@synopsys.com (Eugeniy Paltsev) Date: Wed, 25 Apr 2018 13:53:38 +0000 Subject: [RFC 0/2] dw_mmc: add multislot support In-Reply-To: References: <20180417121130.25281-1-Eugeniy.Paltsev@synopsys.com> <1524239609.10138.47.camel@synopsys.com> List-ID: Message-ID: <1524664417.18209.16.camel@synopsys.com> To: linux-snps-arc@lists.infradead.org On Mon, 2018-04-23@08:47 +0200, Ulf Hansson wrote: > On 20 April 2018@17:53, Eugeniy Paltsev wrote: > > 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. > > That isn't sufficient. The core expects all calls to *any* of the host > ops to be serialized for one host. It does so to conform to the specs. > > For example it may call: > ->set_ios() > ->request() > ->set_ios() > ->request() > ->request() > A bit remark for better understanding: All card settings change are serialized too. These settings are applied after slot switch before execution of new request for this slot. So situations like calling any host_0 ops while another host (host_1) is active are handled by current code. This is example of simultaneous ops calls for both slots: host (slot) 0 | host (slot) 1 ----------------------------------- h0->set_ios() | h1->set_ios() h0->request() | h1->request() h0->set_ios() | h1->set_ios() h0->request() | h1->request() h0->request() | h0->request() | h0->request() | How it will be serialized in the mmc driver: h0->set_ios() // h0 settings save h1->set_ios() // h1 settings save h0->request() // apply settings for h0 and do request ------ slot switch to h1 ------ h1->request() // apply settings for h1 and do request h0->set_ios() // h0 settings save h1->set_ios() // h1 settings save ------ slot switch to h0 ------ h0->request() // apply settings for h0 and do request ------ slot switch to h1 ------ h1->request() // apply settings for h1 and do request ------ slot switch to h0 ------ h0->request() // do request (no new settings to apply) h0->request() // do request (no new settings to apply) h0->request() // do request (no new settings to apply) -- Eugeniy Paltsev From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eugeniy.Paltsev@synopsys.com (Eugeniy Paltsev) Date: Wed, 25 Apr 2018 13:53:38 +0000 Subject: [RFC 0/2] dw_mmc: add multislot support In-Reply-To: References: <20180417121130.25281-1-Eugeniy.Paltsev@synopsys.com> <1524239609.10138.47.camel@synopsys.com> Message-ID: <1524664417.18209.16.camel@synopsys.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, 2018-04-23 at 08:47 +0200, Ulf Hansson wrote: > On 20 April 2018 at 17:53, Eugeniy Paltsev wrote: > > 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. > > That isn't sufficient. The core expects all calls to *any* of the host > ops to be serialized for one host. It does so to conform to the specs. > > For example it may call: > ->set_ios() > ->request() > ->set_ios() > ->request() > ->request() > A bit remark for better understanding: All card settings change are serialized too. These settings are applied after slot switch before execution of new request for this slot. So situations like calling any host_0 ops while another host (host_1) is active are handled by current code. This is example of simultaneous ops calls for both slots: host (slot) 0 | host (slot) 1 ----------------------------------- h0->set_ios() | h1->set_ios() h0->request() | h1->request() h0->set_ios() | h1->set_ios() h0->request() | h1->request() h0->request() | h0->request() | h0->request() | How it will be serialized in the mmc driver: h0->set_ios() // h0 settings save h1->set_ios() // h1 settings save h0->request() // apply settings for h0 and do request ------ slot switch to h1 ------ h1->request() // apply settings for h1 and do request h0->set_ios() // h0 settings save h1->set_ios() // h1 settings save ------ slot switch to h0 ------ h0->request() // apply settings for h0 and do request ------ slot switch to h1 ------ h1->request() // apply settings for h1 and do request ------ slot switch to h0 ------ h0->request() // do request (no new settings to apply) h0->request() // do request (no new settings to apply) h0->request() // do request (no new settings to apply) -- Eugeniy Paltsev