Re: [PATCH] mmc: mmci: Support non-power-of-two block sizes for ux500v2 variant

["Per Förlin" <per.forlin@stericsson.com>]

On 11/26/2012 11:27 AM, Russell King - ARM Linux wrote:
> On Mon, Nov 26, 2012 at 11:20:32AM +0100, Per Förlin 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
>>>> <linux@arm.linux.org.uk> 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
>>>>>> <linux@arm.linux.org.uk> 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", data->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 restriction?
>>>>>>> readsl()/writesl() copes just fine with non-aligned pointers.  It may be
>>>>>>> that your DMA engine can not, but that's no business interfering 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 it needs a
>>>>>>> proper API where DMA engine can export these kinds of properties.
>>>>>> The alignment constraint is related to PIO, sg_miter and that FIFO
>>>>>> 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 transfer
>>>>>   by half-word accesses so such a restriction would be insane.
>>>>>
>>>>> - the FIFO access itself needs to be 32-bit words, so readsl or writesl
>>>>>   (or their io* equivalents must be used).
>>>>>
>>>>> - but - and this is the killer item to your argument as I said above -
>>>>>   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 aspect
>>>> 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 aligned
>>>> 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 misaligned.
>>> We cope with that just fine.  There is *no* reason to reject that transfer
>>> because readsl/writesl cope just fine with it.
>>>
>> The MMCI driver doesn't support alignment smaller than 4 bytes (it may
>> result in data corruption).
> 
> 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.
> 
>> There are two options
>> 1. Either one should fix the driver to support it. Currently the driver
>> only supports miss-alignment of the last sg-miter buffer.
>> 2. Or be kind to inform the user that the alignment is not supported.
> 
> Look, it's very very simple.
> 
> 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 checking.
> _THAT_ is what the limitation is in the driver.  _NOT_ that the pointer
> 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-length 1 which is something that mmci driver cannot handle.

The intention of "data->sg->offset & 3" is to check for misaligned data. What would you replace this check with?

Thanks
Per