From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Per_F=F6rlin?= Subject: Re: [PATCH] mmc: mmci: Support non-power-of-two block sizes for ux500v2 variant Date: Mon, 26 Nov 2012 11:52:03 +0100 Message-ID: <50B349D3.2050806@stericsson.com> References: <1350050522-8852-1-git-send-email-ulf.hansson@stericsson.com> <20121121153811.GN3290@n2100.arm.linux.org.uk> <20121121165048.GO3290@n2100.arm.linux.org.uk> <20121122173708.GJ5764@n2100.arm.linux.org.uk> <50B34270.7070403@stericsson.com> <20121126102712.GC19440@n2100.arm.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from eu1sys200aog114.obsmtp.com ([207.126.144.137]:47732 "EHLO eu1sys200aog114.obsmtp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754867Ab2KZKwY (ORCPT ); Mon, 26 Nov 2012 05:52:24 -0500 In-Reply-To: <20121126102712.GC19440@n2100.arm.linux.org.uk> Sender: linux-mmc-owner@vger.kernel.org List-Id: linux-mmc@vger.kernel.org To: Russell King - ARM Linux Cc: Per Forlin , Ulf HANSSON , "linux-mmc@vger.kernel.org" , Chris Ball , "linux-arm-kernel@lists.infradead.org" , Linus Walleij , Ulf Hansson On 11/26/2012 11:27 AM, Russell King - ARM Linux wrote: > On Mon, Nov 26, 2012 at 11:20:32AM +0100, Per F=F6rlin wrote: >> On 11/22/2012 06:37 PM, Russell King - ARM Linux wrote: >>> On Thu, Nov 22, 2012 at 06:28:30PM +0100, Per Forlin wrote: >>>> On Wed, Nov 21, 2012 at 5:50 PM, Russell King - ARM Linux >>>> wrote: >>>>> On Wed, Nov 21, 2012 at 05:13:55PM +0100, Per Forlin wrote: >>>>>> On Wed, Nov 21, 2012 at 4:38 PM, Russell King - ARM Linux >>>>>> wrote: >>>>>>> On Fri, Oct 12, 2012 at 04:02:02PM +0200, Ulf Hansson wrote: >>>>>>>> /* >>>>>>>> + * Validate mmc prerequisites >>>>>>>> + */ >>>>>>>> +static int mmci_validate_data(struct mmci_host *host, >>>>>>>> + struct mmc_data *data) >>>>>>>> +{ >>>>>>>> + if (!data) >>>>>>>> + return 0; >>>>>>>> + >>>>>>>> + if (!host->variant->non_power_of_2_blksize && >>>>>>>> + !is_power_of_2(data->blksz)) { >>>>>>>> + dev_err(mmc_dev(host->mmc), >>>>>>>> + "unsupported block size (%d bytes)\n", d= ata->blksz); >>>>>>>> + return -EINVAL; >>>>>>>> + } >>>>>>>> + >>>>>>>> + if (data->sg->offset & 3) { >>>>>>>> + dev_err(mmc_dev(host->mmc), >>>>>>>> + "unsupported alginment (0x%x)\n", data->= sg->offset); >>>>>>>> + return -EINVAL; >>>>>>>> + } >>>>>>> >>>>>>> Why? What's the reasoning behind this suddenly introduced rest= riction? >>>>>>> readsl()/writesl() copes just fine with non-aligned pointers. = It may be >>>>>>> that your DMA engine can not, but that's no business interferin= g with >>>>>>> non-DMA transfers, and no reason to fail such transfers. >>>>>>> >>>>>>> If your DMA engine can't do that then its your DMA engine code = which >>>>>>> should refuse to prepare the transfer. >>>>>>> >>>>>>> Yes, that means problems with the way things are ordered - or i= t needs a >>>>>>> proper API where DMA engine can export these kinds of propertie= s. >>>>>> The alignment constraint is related to PIO, sg_miter and that FI= =46O >>>>>> access must be done with 4 bytes. >>>>> >>>>> Total claptrap. No it isn't. >>>>> >>>>> - sg_miter just deals with bytes, and number of bytes transferred= ; there >>>>> is no word assumptions in that code. Indeed many ATA disks tra= nsfer >>>>> by half-word accesses so such a restriction would be insane. >>>>> >>>>> - the FIFO access itself needs to be 32-bit words, so readsl or w= ritesl >>>>> (or their io* equivalents must be used). >>>>> >>>>> - but - and this is the killer item to your argument as I said ab= ove - >>>>> readsl and writesl _can_ take misaligned pointers and cope with= them >>>>> fine. >>>>> >>>>> The actual alignment of the buffer address is totally irrelevant = here. >>>>> >>>>> What isn't irrelevant is the _number_ of bytes to be transferred,= but >>>>> that's something totally different and completely unrelated from >>>>> data->sg->offset. >>>> Let's try again :) >>>> >>>> Keep in mind that the mmc -block layer is aligned so from that asp= ect >>>> everything is fine. >>>> SDIO may have any length or alignment but sg-len is always 1. >>>> >>>> There is just one sg-element and one continues buffer. >>>> >>>> sg-miter splits the continues buffer in chunks that may not be ali= gned >>>> with 4 byte size. It depends on the start address alignment of the >>>> buffer. >>>> >>>> Is it more clear now? >>> >>> Is this more clear: you may be passed a single buffer which is misa= ligned. >>> We cope with that just fine. There is *no* reason to reject that t= ransfer >>> because readsl/writesl cope just fine with it. >>> >> The MMCI driver doesn't support alignment smaller than 4 bytes (it m= ay >> result in data corruption). >=20 > Explain yourself. That's what's lacking here. I'm explaining why I > think you're wrong, but you're just asserting all the time that I'm > wrong without giving any real details. >=20 >> There are two options >> 1. Either one should fix the driver to support it. Currently the dri= ver >> only supports miss-alignment of the last sg-miter buffer. >> 2. Or be kind to inform the user that the alignment is not supported= =2E >=20 > Look, it's very very simple. >=20 > If you have a multi-sg transfer, and the pointer starts off being > misaligned, the first transfer to the end of the page _MAY_ be a > non-word aligned number of bytes. _THAT_ is what you should be check= ing. > _THAT_ is what the limitation is in the driver. _NOT_ that the point= er > is misaligned. > There will be no multi-sg transfer that is misaligned because SDIO-fwk = set the sg-length to 1. Still the transfer may be multi-PAGE with sg-le= ngth 1 which is something that mmci driver cannot handle. The intention of "data->sg->offset & 3" is to check for misaligned data= =2E What would you replace this check with? Thanks Per