Re: [PATCH v2 2/2] mmc: sdhci-tegra: Specify valid DMA mask

[Alexandre Courbot]

On Wed, Mar 2, 2016 at 8:25 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Wednesday 02 March 2016 19:36:23 Alexandre Courbot wrote:
>> On Wed, Mar 2, 2016 at 6:34 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
>> > On Tuesday 01 March 2016 13:32:44 Alexandre Courbot wrote:
>> >> On T210, the sdhci controller can address more than 32 bits of address
>> >> space. Failing to express this fact results in the use of bounce
>> >> buffers and affects performance.
>> >>
>> >> Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
>> >
>> > I don't get this one. Why don't you just set the (SDHCI_USE_SDMA | SDHCI_USE_ADMA)
>> > flags that are checked in the first patch?
>>
>> The test is against (!(host->flags & (SDHCI_USE_SDMA |
>> SDHCI_USE_ADMA))), (see the '!') so it will be true (and the DMA mask
>> will be set) if both flags are *not* set (why we set the mask to 64
>> bits here in that case, I don't know).
>>
>> T210 is capable of SDMA, so we cannot use this condition for that
>> purpose (maybe you missed the '!', in which case I understand why you
>> were surprised).
>
> Ok, I see now that this code was just setting a fake mask in case
> of PIO mode, which doesn't apply here. That in turn means that
> your first patch is wrong though:
>
> For a device that is not DMA capable (neither SDMA nor ADMA
> claimed to be supported), we should not call dma_set_mask_and_coherent()
> because that is likely to fail as well. It's an ugly hack to
> just override the mask in that case, and I'd say it requires
> a comment explaining what is going on.

Yeah, I'm not too sure what is the point of setting the fake mask to
be honest, but you are definitely right that it is a contradiction to
call a DMA function on a device that is not DMA-capable.

>
>> >> @@ -289,6 +291,7 @@ static const struct sdhci_tegra_soc_data soc_data_tegra20 = {
>> >>       .pdata = &sdhci_tegra20_pdata,
>> >>       .nvquirks = NVQUIRK_FORCE_SDHCI_SPEC_200 |
>> >>                   NVQUIRK_ENABLE_BLOCK_GAP_DET,
>> >> +     .dma_mask = DMA_BIT_MASK(32),
>> >>  };
>> >
>> > Can you describe what the specific bug is in these controllers? Do you mean they
>> > support SDHCI_USE_SDMA or SDHCI_USE_ADMA in theory but you still have to prevent
>> > them from using high addresses?
>>
>> Ok, I think you probably missed the '!' then. :)
>
> I missed the larger context of that check too, but I think I've got it now.
>
>> >> @@ -353,6 +358,7 @@ static const struct sdhci_pltfm_data sdhci_tegra210_pdata = {
>> >>
>> >>  static const struct sdhci_tegra_soc_data soc_data_tegra210 = {
>> >>       .pdata = &sdhci_tegra210_pdata,
>> >> +     .dma_mask = DMA_BIT_MASK(34),
>> >>  };
>> >>
>> >>  static const struct of_device_id sdhci_tegra_dt_match[] = {
>> >
>> > This one still completely weirds me out. What kind of odd limitation does
>> > the controller have in Tegra 210?
>> >
>> > Are there actually any machines with more than 16GB?
>>
>> It is not a limitation of the controller - I am just limiting the mask
>> to the range of physical memory we can ever access on T210. My intent
>> here is to overcome the default 32-bit mask, not to limit the range,
>> so I could have set a 64-bit mask if not for my OCD. :P
>>
>> But actually looking at how the various flags are interpreted in
>> sdhci_add_host(), I see the following:
>>
>>     /*
>>      * It is assumed that a 64-bit capable device has set a 64-bit DMA mask
>>      * and *must* do 64-bit DMA.  A driver has the opportunity to change
>>      * that during the first call to ->enable_dma().  Similarly
>>      * SDHCI_QUIRK2_BROKEN_64_BIT_DMA must be left to the drivers to
>>      * implement.
>>      */
>>     if (caps[0] & SDHCI_CAN_64BIT)
>>         host->flags |= SDHCI_USE_64_BIT_DMA;
>>
>> Since this relies on what the hardware declares being capable of and
>> is set on T210, I am very tempted to set a 64-bit dma_mask here and
>> call it a day, but the above comment seems to suggest that this should
>> have been done before.
>
> Right, that sounds good, that also makes it independent of the specific
> Tegra SoC, right?

Yes - it would just be a 2-liner that should set things properly for
all 64-bit capable controllers.

>> If you think this is cool though, I will just do that and in
>> conjunction with patch 1 this will do the job nicely.
>
> as mentioned above, I now have doubts about patch 1, why do you still
> need this now?

We would not need patch 1 anymore.

Let me send this and see if it looks better.

Thanks,
Alex.