Re: [PATCH] mmc: sdhci-brcmstb: Add SDHCI_QUIRK2_BROKEN_64_BIT_DMA

Re: [PATCH] mmc: sdhci-brcmstb: Add SDHCI_QUIRK2_BROKEN_64_BIT_DMA

[Jaedon Shin]

Hi Ulf,

> On Aug 19, 2016, at 10:44 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> 
> On 19 August 2016 at 04:25, Jaedon Shin <jaedon.shin@gmail.com> wrote:
>> Hi Alan,
>> 
>> On Aug 18, 2016, at 11:27 PM, Alan Cooper <alcooperx@gmail.com> wrote:
>>> 
>>> It would be better to make this a MIPS only setting because this issue
>>> only exists for MIPS chips and some newer ARM chips will support 64
>>> bit DMA.
>>> Also, since there's been a general effort to reduce the use QUIRKs,
>>> you could clear the SDHCI_CAN_64BIT in CAPS1 instead of using the
>>> QUIRK.
>>> 
>>> @@ -101,6 +101,9 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>>>       host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
>>>       host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_SDR104 |
>>>                       SDHCI_SUPPORT_DDR50);
>>> +#if defined(CONFIG_MIPS)
>>> +       host->caps1 &= ~SDHCI_CAN_64BIT;
>>> +#endif
>>>       host->quirks |= SDHCI_QUIRK_MISSING_CAPS |
>>>               SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
>> 
>> It's better to me, but we should use host->cap instead of host->cap1. I will update
>> patch with your comment.
> 
> Please, then also send this to the public linux-mmc list.
> 
> Kind regards
> Uffe
> 

I'm sorry I could not add the public linux-mmc list this mail thread, but 
I have already sent the updated patch with linux-mmc.

https://patchwork.kernel.org/patch/9289189/

Thanks,
Jaedon

>> 
>> Thanks,
>> Jaedon
>> 
>>> 
>>> 
>>> Al
>>> 
>>> On Thu, Aug 18, 2016 at 4:24 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> On 08/08/16 04:58, Jaedon Shin wrote:
>>>>> Add SDHCI_QUIRK2_BROKEN_64_BIT_DMA quirk for Broadcom BRCMSTB SoCs. The
>>>>> Broadcom ARM and MIPS based BRCMSTB SDHCI host controllers are using
>>>>> ADMA, but the several chipsets have a incorrect capability about ADMA
>>>>> 64-bit.
>>>>> 
>>>>> Signed-off-by: Jaedon Shin <jaedon.shin@gmail.com>
>>>> 
>>>> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
>>>> 
>>>>> ---
>>>>> drivers/mmc/host/sdhci-brcmstb.c | 1 +
>>>>> 1 file changed, 1 insertion(+)
>>>>> 
>>>>> diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
>>>>> index cce10fe3e19e..704267000ef0 100644
>>>>> --- a/drivers/mmc/host/sdhci-brcmstb.c
>>>>> +++ b/drivers/mmc/host/sdhci-brcmstb.c
>>>>> @@ -103,6 +103,7 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>>>>>                     SDHCI_SUPPORT_DDR50);
>>>>>     host->quirks |= SDHCI_QUIRK_MISSING_CAPS |
>>>>>             SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
>>>>> +     host->quirks2 |= SDHCI_QUIRK2_BROKEN_64_BIT_DMA;
>>>>> 
>>>>>     res = sdhci_add_host(host);
>>>>>     if (res)

Re: [PATCH] mmc: sdhci-brcmstb: Add SDHCI_QUIRK2_BROKEN_64_BIT_DMA

[Florian Fainelli]

On 08/19/2016 07:05 AM, Jaedon Shin wrote:
> Hi Ulf,
> 
>> On Aug 19, 2016, at 10:44 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>> On 19 August 2016 at 04:25, Jaedon Shin <jaedon.shin@gmail.com> wrote:
>>> Hi Alan,
>>>
>>> On Aug 18, 2016, at 11:27 PM, Alan Cooper <alcooperx@gmail.com> wrote:
>>>>
>>>> It would be better to make this a MIPS only setting because this issue
>>>> only exists for MIPS chips and some newer ARM chips will support 64
>>>> bit DMA.
>>>> Also, since there's been a general effort to reduce the use QUIRKs,
>>>> you could clear the SDHCI_CAN_64BIT in CAPS1 instead of using the
>>>> QUIRK.
>>>>
>>>> @@ -101,6 +101,9 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>>>>       host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
>>>>       host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_SDR104 |
>>>>                       SDHCI_SUPPORT_DDR50);
>>>> +#if defined(CONFIG_MIPS)
>>>> +       host->caps1 &= ~SDHCI_CAN_64BIT;
>>>> +#endif
>>>>       host->quirks |= SDHCI_QUIRK_MISSING_CAPS |
>>>>               SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
>>>
>>> It's better to me, but we should use host->cap instead of host->cap1. I will update
>>> patch with your comment.
>>
>> Please, then also send this to the public linux-mmc list.
>>
>> Kind regards
>> Uffe
>>
> 
> I'm sorry I could not add the public linux-mmc list this mail thread, but 
> I have already sent the updated patch with linux-mmc.
> 
> https://patchwork.kernel.org/patch/9289189/

Humm, is not this one of these cases where we would expect the
compatible string to dictacte whether enabling 64_BIT_DMA makes sense or
not?

The patch is technically correct though.
-- 
Florian

Re: [PATCH] mmc: sdhci-brcmstb: Add SDHCI_QUIRK2_BROKEN_64_BIT_DMA

[Ulf Hansson]

On 25 August 2016 at 18:41, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 08/19/2016 07:05 AM, Jaedon Shin wrote:
>> Hi Ulf,
>>
>>> On Aug 19, 2016, at 10:44 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>
>>> On 19 August 2016 at 04:25, Jaedon Shin <jaedon.shin@gmail.com> wrote:
>>>> Hi Alan,
>>>>
>>>> On Aug 18, 2016, at 11:27 PM, Alan Cooper <alcooperx@gmail.com> wrote:
>>>>>
>>>>> It would be better to make this a MIPS only setting because this issue
>>>>> only exists for MIPS chips and some newer ARM chips will support 64
>>>>> bit DMA.
>>>>> Also, since there's been a general effort to reduce the use QUIRKs,
>>>>> you could clear the SDHCI_CAN_64BIT in CAPS1 instead of using the
>>>>> QUIRK.
>>>>>
>>>>> @@ -101,6 +101,9 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>>>>>       host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
>>>>>       host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_SDR104 |
>>>>>                       SDHCI_SUPPORT_DDR50);
>>>>> +#if defined(CONFIG_MIPS)
>>>>> +       host->caps1 &= ~SDHCI_CAN_64BIT;
>>>>> +#endif
>>>>>       host->quirks |= SDHCI_QUIRK_MISSING_CAPS |
>>>>>               SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
>>>>
>>>> It's better to me, but we should use host->cap instead of host->cap1. I will update
>>>> patch with your comment.
>>>
>>> Please, then also send this to the public linux-mmc list.
>>>
>>> Kind regards
>>> Uffe
>>>
>>
>> I'm sorry I could not add the public linux-mmc list this mail thread, but
>> I have already sent the updated patch with linux-mmc.
>>
>> https://patchwork.kernel.org/patch/9289189/
>
> Humm, is not this one of these cases where we would expect the
> compatible string to dictacte whether enabling 64_BIT_DMA makes sense or
> not?

Yes!

Jaedon, can you please send an updated patch. Please also bump the
version number of the patch!

Kind regards
Uffe

>
> The patch is technically correct though.
> --
> Florian

Re: [PATCH] mmc: sdhci-brcmstb: Add SDHCI_QUIRK2_BROKEN_64_BIT_DMA

[Arnd Bergmann]

On Friday 26 August 2016, Ulf Hansson wrote:
> On 25 August 2016 at 18:41, Florian Fainelli <f.fainelli@gmail.com> wrote:
> > On 08/19/2016 07:05 AM, Jaedon Shin wrote:
> >> Hi Ulf,
> >>
> >>> On Aug 19, 2016, at 10:44 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >>>
> >>> On 19 August 2016 at 04:25, Jaedon Shin <jaedon.shin@gmail.com> wrote:
> >>>> Hi Alan,
> >>>>
> >>>> On Aug 18, 2016, at 11:27 PM, Alan Cooper <alcooperx@gmail.com> wrote:
> >>>>>
> >>>>> It would be better to make this a MIPS only setting because this issue
> >>>>> only exists for MIPS chips and some newer ARM chips will support 64
> >>>>> bit DMA.
> >>>>> Also, since there's been a general effort to reduce the use QUIRKs,
> >>>>> you could clear the SDHCI_CAN_64BIT in CAPS1 instead of using the
> >>>>> QUIRK.
> >>>>>
> >>>>> @@ -101,6 +101,9 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
> >>>>>       host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
> >>>>>       host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_SDR104 |
> >>>>>                       SDHCI_SUPPORT_DDR50);
> >>>>> +#if defined(CONFIG_MIPS)
> >>>>> +       host->caps1 &= ~SDHCI_CAN_64BIT;
> >>>>> +#endif
> >>>>>       host->quirks |= SDHCI_QUIRK_MISSING_CAPS |
> >>>>>               SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
> >>>>
> >>>> It's better to me, but we should use host->cap instead of host->cap1. I will update
> >>>> patch with your comment.
> >>>
> >>> Please, then also send this to the public linux-mmc list.
> >>>
> >>> Kind regards
> >>> Uffe
> >>>
> >>
> >> I'm sorry I could not add the public linux-mmc list this mail thread, but
> >> I have already sent the updated patch with linux-mmc.
> >>
> >> https://patchwork.kernel.org/patch/9289189/
> >
> > Humm, is not this one of these cases where we would expect the
> > compatible string to dictacte whether enabling 64_BIT_DMA makes sense or
> > not?
> 
> Yes!
> 
> Jaedon, can you please send an updated patch. Please also bump the
> version number of the patch!
> 

Sorry for jumping in late in the thread, but if I understand it right that the
problem is an SDHCI controller claiming to support 64-bit DMA that is connected
to a bus that only supports 32-bit addressing, this should be handled by
interpreting the dma-ranges property of the parent bus instead.

	Arnd

Re: [PATCH] mmc: sdhci-brcmstb: Add SDHCI_QUIRK2_BROKEN_64_BIT_DMA

[Jaedon Shin]

Hi Florian,

On Aug 26, 2016, at 1:41 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> 
> On 08/19/2016 07:05 AM, Jaedon Shin wrote:
>> Hi Ulf,
>> 
>>> On Aug 19, 2016, at 10:44 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> 
>>> On 19 August 2016 at 04:25, Jaedon Shin <jaedon.shin@gmail.com> wrote:
>>>> Hi Alan,
>>>> 
>>>> On Aug 18, 2016, at 11:27 PM, Alan Cooper <alcooperx@gmail.com> wrote:
>>>>> 
>>>>> It would be better to make this a MIPS only setting because this issue
>>>>> only exists for MIPS chips and some newer ARM chips will support 64
>>>>> bit DMA.
>>>>> Also, since there's been a general effort to reduce the use QUIRKs,
>>>>> you could clear the SDHCI_CAN_64BIT in CAPS1 instead of using the
>>>>> QUIRK.
>>>>> 
>>>>> @@ -101,6 +101,9 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>>>>>      host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
>>>>>      host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_SDR104 |
>>>>>                      SDHCI_SUPPORT_DDR50);
>>>>> +#if defined(CONFIG_MIPS)
>>>>> +       host->caps1 &= ~SDHCI_CAN_64BIT;
>>>>> +#endif
>>>>>      host->quirks |= SDHCI_QUIRK_MISSING_CAPS |
>>>>>              SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
>>>> 
>>>> It's better to me, but we should use host->cap instead of host->cap1. I will update
>>>> patch with your comment.
>>> 
>>> Please, then also send this to the public linux-mmc list.
>>> 
>>> Kind regards
>>> Uffe
>>> 
>> 
>> I'm sorry I could not add the public linux-mmc list this mail thread, but 
>> I have already sent the updated patch with linux-mmc.
>> 
>> https://patchwork.kernel.org/patch/9289189/
> 
> Humm, is not this one of these cases where we would expect the
> compatible string to dictacte whether enabling 64_BIT_DMA makes sense or
> not?
> 
> The patch is technically correct though.

Yes, It's right way that uses host->cap according to the previous discussion
for this driver and commit 5eaa7476f937 ("mmc: sdhci: Allow CAPS check for 
SDHCI_CAN_64BIT to use overridden caps").

If the 64bit ARM chipsets have own compatible string, the patch like as below

	if (of_device_is_compatible(pdev->dev.of_node, "brcm,bcm7425-sdhci"))
		host->caps &= ~SDHCI_CAN_64BIT;

Could you tell me the some newer 64bit ARM chipsets have possible own compatible
string?

Thanks,
Jaedon

> -- 
> Florian

Re: [PATCH] mmc: sdhci-brcmstb: Add SDHCI_QUIRK2_BROKEN_64_BIT_DMA

[Florian Fainelli]

Le 26/08/2016 à 21:02, Jaedon Shin a écrit :
> Hi Florian,
> 
> On Aug 26, 2016, at 1:41 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> On 08/19/2016 07:05 AM, Jaedon Shin wrote:
>>> Hi Ulf,
>>>
>>>> On Aug 19, 2016, at 10:44 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>
>>>> On 19 August 2016 at 04:25, Jaedon Shin <jaedon.shin@gmail.com> wrote:
>>>>> Hi Alan,
>>>>>
>>>>> On Aug 18, 2016, at 11:27 PM, Alan Cooper <alcooperx@gmail.com> wrote:
>>>>>>
>>>>>> It would be better to make this a MIPS only setting because this issue
>>>>>> only exists for MIPS chips and some newer ARM chips will support 64
>>>>>> bit DMA.
>>>>>> Also, since there's been a general effort to reduce the use QUIRKs,
>>>>>> you could clear the SDHCI_CAN_64BIT in CAPS1 instead of using the
>>>>>> QUIRK.
>>>>>>
>>>>>> @@ -101,6 +101,9 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>>>>>>      host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
>>>>>>      host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_SDR104 |
>>>>>>                      SDHCI_SUPPORT_DDR50);
>>>>>> +#if defined(CONFIG_MIPS)
>>>>>> +       host->caps1 &= ~SDHCI_CAN_64BIT;
>>>>>> +#endif
>>>>>>      host->quirks |= SDHCI_QUIRK_MISSING_CAPS |
>>>>>>              SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
>>>>>
>>>>> It's better to me, but we should use host->cap instead of host->cap1. I will update
>>>>> patch with your comment.
>>>>
>>>> Please, then also send this to the public linux-mmc list.
>>>>
>>>> Kind regards
>>>> Uffe
>>>>
>>>
>>> I'm sorry I could not add the public linux-mmc list this mail thread, but 
>>> I have already sent the updated patch with linux-mmc.
>>>
>>> https://patchwork.kernel.org/patch/9289189/
>>
>> Humm, is not this one of these cases where we would expect the
>> compatible string to dictacte whether enabling 64_BIT_DMA makes sense or
>> not?
>>
>> The patch is technically correct though.

Hi Jaedon,

> 
> Yes, It's right way that uses host->cap according to the previous discussion
> for this driver and commit 5eaa7476f937 ("mmc: sdhci: Allow CAPS check for 
> SDHCI_CAN_64BIT to use overridden caps").
> 
> If the 64bit ARM chipsets have own compatible string, the patch like as below
> 
> 	if (of_device_is_compatible(pdev->dev.of_node, "brcm,bcm7425-sdhci"))
> 		host->caps &= ~SDHCI_CAN_64BIT;
> 
> Could you tell me the some newer 64bit ARM chipsets have possible own compatible
> string?

All ARM 32-bit brcmstb chips are LPAE capable, which means that the
SDHCI controller may have to deal with bus addresses larger than
32-bits, so we always need SDHCI_CAN_64BIT to be set for that to happen
and work correctly.

On ARM 64-bit brcmstb chips, we may not have enough memory such that the
SDHCI controller needs to deal with > 32-bits bus addresses, but same
thing, this may happen and the controller is fully capable of, so we
also need SDHCI_CAN_64BIT.

In both cases, the controller should be fully operational with > 32-bits
physical addresses.

On BMIPS chips, we should probably clear SDHCI_CAN_64BIT because AFAIR,
it really is broken (Al, can you confirm?), but at the same time, the
DMA-API should never hand us buffers which exceed the 32-bits bus
address boundary because of the processor and chip memory map
limitations anyway, is that what you encountered though?

At the moment, brcm,bcm7425-sdhci is used across all 3 types of SoCs
(BMIPS, ARM and ARM64) while we should probably allocate a new one for
ARM and newer and then we could reliably base the clearing of
SDHCI_CAN_64BIT based on brcm,bcm7425-sdhci.

Finally, Arnd's suggestions of using "dma-ranges" is fine, but I do not
think we quite need this here because we really need to advertise the
right set of capabilities based on the generation/version of the
controller deployed in specific chips.

I would like to have Al's feedback on this, since he wrote the driver ;)

Thanks!
-- 
Florian

Re: [PATCH] mmc: sdhci-brcmstb: Add SDHCI_QUIRK2_BROKEN_64_BIT_DMA

[Jaedon Shin]

On Aug 28, 2016, at 4:56 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> 
> Le 26/08/2016 à 21:02, Jaedon Shin a écrit :
>> Hi Florian,
>> 
>> On Aug 26, 2016, at 1:41 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> 
>>> On 08/19/2016 07:05 AM, Jaedon Shin wrote:
>>>> Hi Ulf,
>>>> 
>>>>> On Aug 19, 2016, at 10:44 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>> 
>>>>> On 19 August 2016 at 04:25, Jaedon Shin <jaedon.shin@gmail.com> wrote:
>>>>>> Hi Alan,
>>>>>> 
>>>>>> On Aug 18, 2016, at 11:27 PM, Alan Cooper <alcooperx@gmail.com> wrote:
>>>>>>> 
>>>>>>> It would be better to make this a MIPS only setting because this issue
>>>>>>> only exists for MIPS chips and some newer ARM chips will support 64
>>>>>>> bit DMA.
>>>>>>> Also, since there's been a general effort to reduce the use QUIRKs,
>>>>>>> you could clear the SDHCI_CAN_64BIT in CAPS1 instead of using the
>>>>>>> QUIRK.
>>>>>>> 
>>>>>>> @@ -101,6 +101,9 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>>>>>>>     host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
>>>>>>>     host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_SDR104 |
>>>>>>>                     SDHCI_SUPPORT_DDR50);
>>>>>>> +#if defined(CONFIG_MIPS)
>>>>>>> +       host->caps1 &= ~SDHCI_CAN_64BIT;
>>>>>>> +#endif
>>>>>>>     host->quirks |= SDHCI_QUIRK_MISSING_CAPS |
>>>>>>>             SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
>>>>>> 
>>>>>> It's better to me, but we should use host->cap instead of host->cap1. I will update
>>>>>> patch with your comment.
>>>>> 
>>>>> Please, then also send this to the public linux-mmc list.
>>>>> 
>>>>> Kind regards
>>>>> Uffe
>>>>> 
>>>> 
>>>> I'm sorry I could not add the public linux-mmc list this mail thread, but 
>>>> I have already sent the updated patch with linux-mmc.
>>>> 
>>>> https://patchwork.kernel.org/patch/9289189/
>>> 
>>> Humm, is not this one of these cases where we would expect the
>>> compatible string to dictacte whether enabling 64_BIT_DMA makes sense or
>>> not?
>>> 
>>> The patch is technically correct though.
> 
> Hi Jaedon,
> 
>> 
>> Yes, It's right way that uses host->cap according to the previous discussion
>> for this driver and commit 5eaa7476f937 ("mmc: sdhci: Allow CAPS check for 
>> SDHCI_CAN_64BIT to use overridden caps").
>> 
>> If the 64bit ARM chipsets have own compatible string, the patch like as below
>> 
>> 	if (of_device_is_compatible(pdev->dev.of_node, "brcm,bcm7425-sdhci"))
>> 		host->caps &= ~SDHCI_CAN_64BIT;
>> 
>> Could you tell me the some newer 64bit ARM chipsets have possible own compatible
>> string?
> 
> All ARM 32-bit brcmstb chips are LPAE capable, which means that the
> SDHCI controller may have to deal with bus addresses larger than
> 32-bits, so we always need SDHCI_CAN_64BIT to be set for that to happen
> and work correctly.
> 
> On ARM 64-bit brcmstb chips, we may not have enough memory such that the
> SDHCI controller needs to deal with > 32-bits bus addresses, but same
> thing, this may happen and the controller is fully capable of, so we
> also need SDHCI_CAN_64BIT.
> 
> In both cases, the controller should be fully operational with > 32-bits
> physical addresses.
> 
> On BMIPS chips, we should probably clear SDHCI_CAN_64BIT because AFAIR,
> it really is broken (Al, can you confirm?), but at the same time, the
> DMA-API should never hand us buffers which exceed the 32-bits bus
> address boundary because of the processor and chip memory map
> limitations anyway, is that what you encountered though?
> 
> At the moment, brcm,bcm7425-sdhci is used across all 3 types of SoCs
> (BMIPS, ARM and ARM64) while we should probably allocate a new one for
> ARM and newer and then we could reliably base the clearing of
> SDHCI_CAN_64BIT based on brcm,bcm7425-sdhci.
> 
> Finally, Arnd's suggestions of using "dma-ranges" is fine, but I do not
> think we quite need this here because we really need to advertise the
> right set of capabilities based on the generation/version of the
> controller deployed in specific chips.
> 
> I would like to have Al's feedback on this, since he wrote the driver ;)
> 
> Thanks!
> -- 
> Florian

Thanks for your detailed replay, and I understand why it must use a compatible
string for all brcmstb chips. I'm sending the bumped patch for BMIPS quickly.

Thanks,
Jaedon

Re: [PATCH] mmc: sdhci-brcmstb: Add SDHCI_QUIRK2_BROKEN_64_BIT_DMA

[Arnd Bergmann]

On Saturday, August 27, 2016 12:56:10 PM CEST Florian Fainelli wrote:
> 
> Finally, Arnd's suggestions of using "dma-ranges" is fine, but I do not
> think we quite need this here because we really need to advertise the
> right set of capabilities based on the generation/version of the
> controller deployed in specific chips.

To be more specific here, I think that without the dma-ranges property
you should never be able to set a dma-mask larger than the 32-bit
mask, so if you have machines that are capable of high DMA, you
should definitely add the property in the bus, even if that is
currently ignored.

I've suggested a patch before, but I believe both ARM and MIPS ignore
this at the moment, and just allow drivers to set arbitrary masks
even when the bus does not have a dma-ranges property, and that
is a bug.

	Arnd

Re: [PATCH] mmc: sdhci-brcmstb: Add SDHCI_QUIRK2_BROKEN_64_BIT_DMA

[Alan Cooper]

On Mon, Aug 29, 2016 at 5:44 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Saturday, August 27, 2016 12:56:10 PM CEST Florian Fainelli wrote:
>>
>> Finally, Arnd's suggestions of using "dma-ranges" is fine, but I do not
>> think we quite need this here because we really need to advertise the
>> right set of capabilities based on the generation/version of the
>> controller deployed in specific chips.
>
> To be more specific here, I think that without the dma-ranges property
> you should never be able to set a dma-mask larger than the 32-bit
> mask, so if you have machines that are capable of high DMA, you
> should definitely add the property in the bus, even if that is
> currently ignored.
>
> I've suggested a patch before, but I believe both ARM and MIPS ignore
> this at the moment, and just allow drivers to set arbitrary masks
> even when the bus does not have a dma-ranges property, and that
> is a bug.


I think there's some confusion over the core issue so let be summarize.
The SDHCI host controller CAPABILITIES register bit 28 indicates if
the host controller hardware can support 64 bit addresses using the
larger 96 bit DMA descriptors. We currently don't have any SoCs that
have this support, even our current 64bit ARM SoCs don't support this,
though there are future ARM SoCs that will have this support. The bug
is that a few MIPS based SoCs had this bit incorrectly set even though
the hardware did not support it and this caused the driver to use the
larger DMA descriptors which crashed the driver. All we're trying to
fix here is a simple SDHCI host controller hardware bug where a CAPs
bit is a 1 where it should be a 0. Our future 64 ARM chips that do
support 64bit addressing should be the only chips with this bit set.


Al

Re: [PATCH] mmc: sdhci-brcmstb: Add SDHCI_QUIRK2_BROKEN_64_BIT_DMA

[Arnd Bergmann]

On Monday 29 August 2016, Alan Cooper wrote:
> I think there's some confusion over the core issue so let be summarize.
> The SDHCI host controller CAPABILITIES register bit 28 indicates if
> the host controller hardware can support 64 bit addresses using the
> larger 96 bit DMA descriptors. We currently don't have any SoCs that
> have this support, even our current 64bit ARM SoCs don't support this,
> though there are future ARM SoCs that will have this support. The bug
> is that a few MIPS based SoCs had this bit incorrectly set even though
> the hardware did not support it and this caused the driver to use the
> larger DMA descriptors which crashed the driver. All we're trying to
> fix here is a simple SDHCI host controller hardware bug where a CAPs
> bit is a 1 where it should be a 0. Our future 64 ARM chips that do
> support 64bit addressing should be the only chips with this bit set.

Ok, got it. Thanks for the clarification.

So the SDHCI device claims to have a capability that doesn't work,
rather than correctly listing a property that works in principle
but is prevented from working by something outside of the device
as we discussed another time [1]

If this happens in other chips as well, I guess we could also
solve this by changing the code to only enable the 64-bit ADMA
feature when dma_get_required_mask() returns something larger
than a 32-bit mask. This would also be (very slightly) more
efficient because we have to access fewer registers per transfer.

	Arnd

[1] http://marc.info/?l=linux-mmc&m=141457290627820&w=2