diff mbox series

[V2,05/10] mmc: sdhci-iproc: Set SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN on BCM2711

Message ID 1628334401-6577-6-git-send-email-stefan.wahren@i2se.com (mailing list archive)
State New, archived
Headers show
Series ARM: dts: Add Raspberry Pi CM4 & CM4 IO Board support | expand

Commit Message

Stefan Wahren Aug. 7, 2021, 11:06 a.m. UTC 
From: Nicolas Saenz Julienne <nsaenz@kernel.org>

The controller doesn't seem to pick-up on clock changes, so set the
SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN flag to query the clock frequency
directly from the clock.

Fixes: f84e411c85be ("mmc: sdhci-iproc: Add support for emmc2 of the BCM2711")
Signed-off-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/mmc/host/sdhci-iproc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Ulf Hansson Aug. 16, 2021, 1:59 p.m. UTC | #1 
On Sat, 7 Aug 2021 at 13:07, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>
> From: Nicolas Saenz Julienne <nsaenz@kernel.org>
>
> The controller doesn't seem to pick-up on clock changes, so set the
> SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN flag to query the clock frequency
> directly from the clock.
>
> Fixes: f84e411c85be ("mmc: sdhci-iproc: Add support for emmc2 of the BCM2711")
> Signed-off-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>

Applied for fixes and by adding a stable tag, thanks!

Kind regards
Uffe


> ---
>  drivers/mmc/host/sdhci-iproc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
> index 032bf85..e7565c6 100644
> --- a/drivers/mmc/host/sdhci-iproc.c
> +++ b/drivers/mmc/host/sdhci-iproc.c
> @@ -295,7 +295,8 @@ static const struct sdhci_ops sdhci_iproc_bcm2711_ops = {
>  };
>
>  static const struct sdhci_pltfm_data sdhci_bcm2711_pltfm_data = {
> -       .quirks = SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
> +       .quirks = SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 |
> +                 SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
>         .ops = &sdhci_iproc_bcm2711_ops,
>  };
>
> --
> 2.7.4
>
Jeremy Linton Aug. 26, 2021, 6:36 a.m. UTC | #2 
Hi,


On 8/7/21 6:06 AM, Stefan Wahren wrote:
> From: Nicolas Saenz Julienne <nsaenz@kernel.org>
> 
> The controller doesn't seem to pick-up on clock changes, so set the
> SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN flag to query the clock frequency
> directly from the clock.
> 
> Fixes: f84e411c85be ("mmc: sdhci-iproc: Add support for emmc2 of the BCM2711")
> Signed-off-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
>   drivers/mmc/host/sdhci-iproc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
> index 032bf85..e7565c6 100644
> --- a/drivers/mmc/host/sdhci-iproc.c
> +++ b/drivers/mmc/host/sdhci-iproc.c
> @@ -295,7 +295,8 @@ static const struct sdhci_ops sdhci_iproc_bcm2711_ops = {
>   };
>   
>   static const struct sdhci_pltfm_data sdhci_bcm2711_pltfm_data = {
> -	.quirks = SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
> +	.quirks = SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 |
> +		  SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
>   	.ops = &sdhci_iproc_bcm2711_ops,
>   };

I just noticed that this got merged to rc7, and it breaks the ACPI based 
rpi's because it causes the 100Mhz max clock to be overridden to the 
return from sdhci_iproc_get_max_clock() which is 0 because there isn't a 
OF/DT based clock device.
Ulf Hansson Aug. 26, 2021, 9:22 a.m. UTC | #3 
On Thu, 26 Aug 2021 at 08:36, Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> Hi,
>
>
> On 8/7/21 6:06 AM, Stefan Wahren wrote:
> > From: Nicolas Saenz Julienne <nsaenz@kernel.org>
> >
> > The controller doesn't seem to pick-up on clock changes, so set the
> > SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN flag to query the clock frequency
> > directly from the clock.
> >
> > Fixes: f84e411c85be ("mmc: sdhci-iproc: Add support for emmc2 of the BCM2711")
> > Signed-off-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > ---
> >   drivers/mmc/host/sdhci-iproc.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
> > index 032bf85..e7565c6 100644
> > --- a/drivers/mmc/host/sdhci-iproc.c
> > +++ b/drivers/mmc/host/sdhci-iproc.c
> > @@ -295,7 +295,8 @@ static const struct sdhci_ops sdhci_iproc_bcm2711_ops = {
> >   };
> >
> >   static const struct sdhci_pltfm_data sdhci_bcm2711_pltfm_data = {
> > -     .quirks = SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
> > +     .quirks = SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 |
> > +               SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> >       .ops = &sdhci_iproc_bcm2711_ops,
> >   };
>
> I just noticed that this got merged to rc7, and it breaks the ACPI based
> rpi's because it causes the 100Mhz max clock to be overridden to the
> return from sdhci_iproc_get_max_clock() which is 0 because there isn't a
> OF/DT based clock device.

Thanks for reporting! I allow Stefan to respond in a day or two,
before I do a revert of it.

Kind regards
Uffe
Stefan Wahren Aug. 26, 2021, 11:18 a.m. UTC | #4 
Am 26.08.21 um 11:22 schrieb Ulf Hansson:
> On Thu, 26 Aug 2021 at 08:36, Jeremy Linton <jeremy.linton@arm.com> wrote:
>> Hi,
>>
>>
>> On 8/7/21 6:06 AM, Stefan Wahren wrote:
>>> From: Nicolas Saenz Julienne <nsaenz@kernel.org>
>>>
>>> The controller doesn't seem to pick-up on clock changes, so set the
>>> SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN flag to query the clock frequency
>>> directly from the clock.
>>>
>>> Fixes: f84e411c85be ("mmc: sdhci-iproc: Add support for emmc2 of the BCM2711")
>>> Signed-off-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>>> ---
>>>   drivers/mmc/host/sdhci-iproc.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
>>> index 032bf85..e7565c6 100644
>>> --- a/drivers/mmc/host/sdhci-iproc.c
>>> +++ b/drivers/mmc/host/sdhci-iproc.c
>>> @@ -295,7 +295,8 @@ static const struct sdhci_ops sdhci_iproc_bcm2711_ops = {
>>>   };
>>>
>>>   static const struct sdhci_pltfm_data sdhci_bcm2711_pltfm_data = {
>>> -     .quirks = SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
>>> +     .quirks = SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 |
>>> +               SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
>>>       .ops = &sdhci_iproc_bcm2711_ops,
>>>   };
>> I just noticed that this got merged to rc7, and it breaks the ACPI based
>> rpi's because it causes the 100Mhz max clock to be overridden to the
>> return from sdhci_iproc_get_max_clock() which is 0 because there isn't a
>> OF/DT based clock device.
> Thanks for reporting! I allow Stefan to respond in a day or two,
> before I do a revert of it.

I'm fine with a revert.

Thanks

>
> Kind regards
> Uffe
Ulf Hansson Aug. 27, 2021, 2:44 p.m. UTC | #5 
On Thu, 26 Aug 2021 at 13:18, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>
> Am 26.08.21 um 11:22 schrieb Ulf Hansson:
> > On Thu, 26 Aug 2021 at 08:36, Jeremy Linton <jeremy.linton@arm.com> wrote:
> >> Hi,
> >>
> >>
> >> On 8/7/21 6:06 AM, Stefan Wahren wrote:
> >>> From: Nicolas Saenz Julienne <nsaenz@kernel.org>
> >>>
> >>> The controller doesn't seem to pick-up on clock changes, so set the
> >>> SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN flag to query the clock frequency
> >>> directly from the clock.
> >>>
> >>> Fixes: f84e411c85be ("mmc: sdhci-iproc: Add support for emmc2 of the BCM2711")
> >>> Signed-off-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
> >>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> >>> ---
> >>>   drivers/mmc/host/sdhci-iproc.c | 3 ++-
> >>>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
> >>> index 032bf85..e7565c6 100644
> >>> --- a/drivers/mmc/host/sdhci-iproc.c
> >>> +++ b/drivers/mmc/host/sdhci-iproc.c
> >>> @@ -295,7 +295,8 @@ static const struct sdhci_ops sdhci_iproc_bcm2711_ops = {
> >>>   };
> >>>
> >>>   static const struct sdhci_pltfm_data sdhci_bcm2711_pltfm_data = {
> >>> -     .quirks = SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
> >>> +     .quirks = SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 |
> >>> +               SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> >>>       .ops = &sdhci_iproc_bcm2711_ops,
> >>>   };
> >> I just noticed that this got merged to rc7, and it breaks the ACPI based
> >> rpi's because it causes the 100Mhz max clock to be overridden to the
> >> return from sdhci_iproc_get_max_clock() which is 0 because there isn't a
> >> OF/DT based clock device.
> > Thanks for reporting! I allow Stefan to respond in a day or two,
> > before I do a revert of it.
>
> I'm fine with a revert.
>
> Thanks

Patch reverted and applied for fixes, thanks!

Kind regards
Uffe
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
index 032bf85..e7565c6 100644
--- a/drivers/mmc/host/sdhci-iproc.c
+++ b/drivers/mmc/host/sdhci-iproc.c
@@ -295,7 +295,8 @@  static const struct sdhci_ops sdhci_iproc_bcm2711_ops = {
 };
 
 static const struct sdhci_pltfm_data sdhci_bcm2711_pltfm_data = {
-	.quirks = SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
+	.quirks = SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 |
+		  SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
 	.ops = &sdhci_iproc_bcm2711_ops,
 };