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

[Per Forlin]

On Mon, Nov 26, 2012 at 11:52 AM, Per Förlin <per.forlin at stericsson.com> wrote:
> 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 at 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 at 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
>
I have tried to work out an if-statement to check this properly. Here
is my conclusion,
This only works if sg len is 1 (in the SDIO case)

if (WRITE)
  align = sg->offset & 3
else if (READ)
  align = 0

if (sg->offset & 3 && (PAGE_SIZE - (sg->offset + align) < host->size))
   error; /* cannot be handled by mmci driver */

Is this if-statement align with your view of the alignment issue ?

BR
Per