Re: Re: [PATCH 1/5] ARM: sunxi: Add driver for SD/MMC hosts found on Allwinner sunxi SoCs

[Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

Hi,

On 12/15/2013 05:21 PM, Maxime Ripard wrote:
> On Sun, Dec 15, 2013 at 03:20:19PM +0100, Hans de Goede wrote:
<snip>

>>>> +static const struct of_device_id sunxi_mmc_of_match[] = {
>>>> +	{ .compatible = "allwinner,sun4i-mmc", },
>>>> +	{ .compatible = "allwinner,sun5i-mmc", },
>>>
>>> Please use sun5i-a13-mmc as your compatible.
>>
>> Can you explain a bit why? In essence currently we have
>> 2 versions of the mmc controller, those found on sun4i
>> and those found on sun5i and sun7i. I thought that the
>> norm was to use the oldest soc version in which a revision
>> of an ip-block first appears as the compatible string ?
>
> Indeed.
>
>> Note I've tested this with both a13 and a10s SOCs, if we
>> add a sun5i-a13-mmc we should also add a sun5i-a10s-mmc
>> and a sun5i-a20-mmc, or would that then be sun7i-a20-mmc?
>
> And the A13 has been the first SoC in the sun5i family, hence why we
> should use sun5i-a13 as the prefix here. If the A10s and A20 would not
> have been compatible with the A13 MMC controller, we would have used
> sun5i-a10s-mmc and sun7i-a20-mmc, respectively.

Ah I see, you meant s/allwinner,sun5i-mmc/allwinner,sun5i-a13-mmc/

I interpreted your remark as adding an extra allwinner,sun5i-a13-mmc
compatible string. Using allwinner,sun5i-a13-mmc instead of
allwinner,sun5i-mmc makes sense. David having us both editing the
driver at the same time seems counter-productive, can you take care
of this too?

>
>> To me just having sun5i-mmc for sun5i+ socs seems simpler.
>
> And if we ever find out that A10s or A13 differs in some way, we will
> end up introducing a compatible that will be sun5i-a10s-mmc, along
> with the sun5i-mmc we already have, which is not really the more
> consistent thing we would have done.
>
>> <snip>
>>
>>>> +	mmc->ops		= &sunxi_mmc_ops;
>>>> +	mmc->max_blk_count	= 8192;
>>>> +	mmc->max_blk_size	= 4096;
>>>> +	mmc->max_segs		= PAGE_SIZE / sizeof(struct sunxi_idma_des);
>>>> +	mmc->max_seg_size	= (1 << host->idma_des_size_bits);
>>>> +	mmc->max_req_size	= mmc->max_seg_size * mmc->max_segs;
>>>> +	/* 400kHz ~ 50MHz */
>>>> +	mmc->f_min		=   400000;
>>>> +	mmc->f_max		= 50000000;
>>>
>>> Hmmm, the tables earlier seem to suggest it can do much more than that.
>>
>> I know, but this is what the allwinner android kernels are using, actually
>> in case of sdc3 they are putting 200000000 in f_max (as that is often
>> used for sdio cards) but then later in set_ios they clamp the passed
>> in clock to 47000000 Mhz, so I seriously doubt that 200Mhz has actually
>> worked. Hence I've simply gone for a safe range for now. If someone has
>> cards capable of doing 200 MHz we could certainly run various tests and
>> try to improve this, but for now this seems a sane range to start with.
>
> That's probably something that you should mention in your comment then :)

Good point, David ?

<snip>

>>> Hmmm, I see no implementation for this function. Didn't you forget
>>> a file here? (and it should probably be a separate patch anyway).
>>
>> The implementation is part of Emilio's clk branch, but he forgot
>> to add a header for it, so I'm fixing that up here, I guess this
>> should go through Emlio's tree as a separate patch.
>
> As far as I know, this work has never been posted, let alone
> merged.

I don't know if it has been posted, but it has been in his tree for a
while now. Anyways I'll send him a standalone patch for the clk/sunxi.h
file.

 > Anyway, the dependencies you have is something that you should
 > mention in your cover letter, so that we know what to merge, in which
 > order, and when to merge it.

Right, my bad, sorry. I was so happy I was finally ready to send the patch
upstream (I finally had fixed everything on my todo list), I rushed the
cover letter a bit. I was planning on putting things like this in there,
as well as why we didn't try to extend the mmc-dw driver, but I forgot.

Thanks & Regards,

Hans