[PATCH 1/2] dt-bindings: mmc: sdhci-iproc: Add brcm,bcm7211a0-sdhci

[PATCH 1/2] dt-bindings: mmc: sdhci-iproc: Add brcm,bcm7211a0-sdhci

[Al Cooper]

Add new compatible string for the legacy sdhci controller on the
BCM7211 family of SoC's.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 Documentation/devicetree/bindings/mmc/brcm,iproc-sdhci.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mmc/brcm,iproc-sdhci.yaml b/Documentation/devicetree/bindings/mmc/brcm,iproc-sdhci.yaml
index 6f569fbfa134..2f63f2cdeb71 100644
--- a/Documentation/devicetree/bindings/mmc/brcm,iproc-sdhci.yaml
+++ b/Documentation/devicetree/bindings/mmc/brcm,iproc-sdhci.yaml
@@ -21,6 +21,7 @@ properties:
       - brcm,bcm2711-emmc2
       - brcm,sdhci-iproc-cygnus
       - brcm,sdhci-iproc
+      - brcm,bcm7211a0-sdhci
 
   reg:
     minItems: 1

base-commit: 97e5bf604b7a0d6e1b3e00fe31d5fd4b9bffeaae
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] mmc: sdhci-iproc: Add support for the legacy sdhci controller on the BCM7211

[Al Cooper]

Add support for the legacy Arasan sdhci controller on the BCM7211 and
related SoC's. This includes adding a .shutdown callback to increase
the power savings during S5.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 drivers/mmc/host/Kconfig       |  2 +-
 drivers/mmc/host/sdhci-iproc.c | 30 ++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.h       |  2 ++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index a4d4c757eea0..561184fa7eb9 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -412,7 +412,7 @@ config MMC_SDHCI_MILBEAUT
 
 config MMC_SDHCI_IPROC
 	tristate "SDHCI support for the BCM2835 & iProc SD/MMC Controller"
-	depends on ARCH_BCM2835 || ARCH_BCM_IPROC || COMPILE_TEST
+	depends on ARCH_BCM2835 || ARCH_BCM_IPROC || ARCH_BRCMSTB || COMPILE_TEST
 	depends on MMC_SDHCI_PLTFM
 	depends on OF || ACPI
 	default ARCH_BCM_IPROC
diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
index ddeaf8e1f72f..cce390fe9cf3 100644
--- a/drivers/mmc/host/sdhci-iproc.c
+++ b/drivers/mmc/host/sdhci-iproc.c
@@ -286,11 +286,35 @@ static const struct sdhci_iproc_data bcm2711_data = {
 	.mmc_caps = MMC_CAP_3_3V_DDR,
 };
 
+static const struct sdhci_pltfm_data sdhci_bcm7211a0_pltfm_data = {
+	.quirks = SDHCI_QUIRK_MISSING_CAPS |
+		SDHCI_QUIRK_BROKEN_TIMEOUT_VAL |
+		SDHCI_QUIRK_BROKEN_DMA |
+		SDHCI_QUIRK_BROKEN_ADMA,
+	.ops = &sdhci_iproc_ops,
+};
+
+#define BCM7211A0_BASE_CLK_MHZ 100
+static const struct sdhci_iproc_data bcm7211a0_data = {
+	.pdata = &sdhci_bcm7211a0_pltfm_data,
+	.caps = ((BCM7211A0_BASE_CLK_MHZ / 2) << SDHCI_TIMEOUT_CLK_SHIFT) |
+		(BCM7211A0_BASE_CLK_MHZ << SDHCI_CLOCK_BASE_SHIFT) |
+		((0x2 << SDHCI_MAX_BLOCK_SHIFT)
+			& SDHCI_MAX_BLOCK_MASK) |
+		SDHCI_CAN_VDD_330 |
+		SDHCI_CAN_VDD_180 |
+		SDHCI_CAN_DO_SUSPEND |
+		SDHCI_CAN_DO_HISPD,
+	.caps1 = SDHCI_DRIVER_TYPE_C |
+		 SDHCI_DRIVER_TYPE_D,
+};
+
 static const struct of_device_id sdhci_iproc_of_match[] = {
 	{ .compatible = "brcm,bcm2835-sdhci", .data = &bcm2835_data },
 	{ .compatible = "brcm,bcm2711-emmc2", .data = &bcm2711_data },
 	{ .compatible = "brcm,sdhci-iproc-cygnus", .data = &iproc_cygnus_data},
 	{ .compatible = "brcm,sdhci-iproc", .data = &iproc_data },
+	{ .compatible = "brcm,bcm7211a0-sdhci", .data = &bcm7211a0_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sdhci_iproc_of_match);
@@ -384,6 +408,11 @@ static int sdhci_iproc_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static void sdhci_iproc_shutdown(struct platform_device *pdev)
+{
+	sdhci_pltfm_suspend(&pdev->dev);
+}
+
 static struct platform_driver sdhci_iproc_driver = {
 	.driver = {
 		.name = "sdhci-iproc",
@@ -394,6 +423,7 @@ static struct platform_driver sdhci_iproc_driver = {
 	},
 	.probe = sdhci_iproc_probe,
 	.remove = sdhci_pltfm_unregister,
+	.shutdown = sdhci_iproc_shutdown,
 };
 module_platform_driver(sdhci_iproc_driver);
 
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 0770c036e2ff..c35ed4be75b7 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -201,8 +201,10 @@
 
 #define SDHCI_CAPABILITIES	0x40
 #define  SDHCI_TIMEOUT_CLK_MASK		GENMASK(5, 0)
+#define  SDHCI_TIMEOUT_CLK_SHIFT 0
 #define  SDHCI_TIMEOUT_CLK_UNIT	0x00000080
 #define  SDHCI_CLOCK_BASE_MASK		GENMASK(13, 8)
+#define  SDHCI_CLOCK_BASE_SHIFT	8
 #define  SDHCI_CLOCK_V3_BASE_MASK	GENMASK(15, 8)
 #define  SDHCI_MAX_BLOCK_MASK	0x00030000
 #define  SDHCI_MAX_BLOCK_SHIFT  16
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] mmc: sdhci-iproc: Add support for the legacy sdhci controller on the BCM7211

[Ulf Hansson]

On Wed, 2 Jun 2021 at 21:28, Al Cooper <alcooperx@gmail.com> wrote:
>
> Add support for the legacy Arasan sdhci controller on the BCM7211 and
> related SoC's. This includes adding a .shutdown callback to increase
> the power savings during S5.

Please split this into two separate changes.

May I also ask about the ->shutdown() callback and in relation to S5.
What makes the ->shutdown callback only being invoked for S5?

Kind regards
Uffe

>
> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> ---
>  drivers/mmc/host/Kconfig       |  2 +-
>  drivers/mmc/host/sdhci-iproc.c | 30 ++++++++++++++++++++++++++++++
>  drivers/mmc/host/sdhci.h       |  2 ++
>  3 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index a4d4c757eea0..561184fa7eb9 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -412,7 +412,7 @@ config MMC_SDHCI_MILBEAUT
>
>  config MMC_SDHCI_IPROC
>         tristate "SDHCI support for the BCM2835 & iProc SD/MMC Controller"
> -       depends on ARCH_BCM2835 || ARCH_BCM_IPROC || COMPILE_TEST
> +       depends on ARCH_BCM2835 || ARCH_BCM_IPROC || ARCH_BRCMSTB || COMPILE_TEST
>         depends on MMC_SDHCI_PLTFM
>         depends on OF || ACPI
>         default ARCH_BCM_IPROC
> diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
> index ddeaf8e1f72f..cce390fe9cf3 100644
> --- a/drivers/mmc/host/sdhci-iproc.c
> +++ b/drivers/mmc/host/sdhci-iproc.c
> @@ -286,11 +286,35 @@ static const struct sdhci_iproc_data bcm2711_data = {
>         .mmc_caps = MMC_CAP_3_3V_DDR,
>  };
>
> +static const struct sdhci_pltfm_data sdhci_bcm7211a0_pltfm_data = {
> +       .quirks = SDHCI_QUIRK_MISSING_CAPS |
> +               SDHCI_QUIRK_BROKEN_TIMEOUT_VAL |
> +               SDHCI_QUIRK_BROKEN_DMA |
> +               SDHCI_QUIRK_BROKEN_ADMA,
> +       .ops = &sdhci_iproc_ops,
> +};
> +
> +#define BCM7211A0_BASE_CLK_MHZ 100
> +static const struct sdhci_iproc_data bcm7211a0_data = {
> +       .pdata = &sdhci_bcm7211a0_pltfm_data,
> +       .caps = ((BCM7211A0_BASE_CLK_MHZ / 2) << SDHCI_TIMEOUT_CLK_SHIFT) |
> +               (BCM7211A0_BASE_CLK_MHZ << SDHCI_CLOCK_BASE_SHIFT) |
> +               ((0x2 << SDHCI_MAX_BLOCK_SHIFT)
> +                       & SDHCI_MAX_BLOCK_MASK) |
> +               SDHCI_CAN_VDD_330 |
> +               SDHCI_CAN_VDD_180 |
> +               SDHCI_CAN_DO_SUSPEND |
> +               SDHCI_CAN_DO_HISPD,
> +       .caps1 = SDHCI_DRIVER_TYPE_C |
> +                SDHCI_DRIVER_TYPE_D,
> +};
> +
>  static const struct of_device_id sdhci_iproc_of_match[] = {
>         { .compatible = "brcm,bcm2835-sdhci", .data = &bcm2835_data },
>         { .compatible = "brcm,bcm2711-emmc2", .data = &bcm2711_data },
>         { .compatible = "brcm,sdhci-iproc-cygnus", .data = &iproc_cygnus_data},
>         { .compatible = "brcm,sdhci-iproc", .data = &iproc_data },
> +       { .compatible = "brcm,bcm7211a0-sdhci", .data = &bcm7211a0_data },
>         { }
>  };
>  MODULE_DEVICE_TABLE(of, sdhci_iproc_of_match);
> @@ -384,6 +408,11 @@ static int sdhci_iproc_probe(struct platform_device *pdev)
>         return ret;
>  }
>
> +static void sdhci_iproc_shutdown(struct platform_device *pdev)
> +{
> +       sdhci_pltfm_suspend(&pdev->dev);
> +}
> +
>  static struct platform_driver sdhci_iproc_driver = {
>         .driver = {
>                 .name = "sdhci-iproc",
> @@ -394,6 +423,7 @@ static struct platform_driver sdhci_iproc_driver = {
>         },
>         .probe = sdhci_iproc_probe,
>         .remove = sdhci_pltfm_unregister,
> +       .shutdown = sdhci_iproc_shutdown,
>  };
>  module_platform_driver(sdhci_iproc_driver);
>
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 0770c036e2ff..c35ed4be75b7 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -201,8 +201,10 @@
>
>  #define SDHCI_CAPABILITIES     0x40
>  #define  SDHCI_TIMEOUT_CLK_MASK                GENMASK(5, 0)
> +#define  SDHCI_TIMEOUT_CLK_SHIFT 0
>  #define  SDHCI_TIMEOUT_CLK_UNIT        0x00000080
>  #define  SDHCI_CLOCK_BASE_MASK         GENMASK(13, 8)
> +#define  SDHCI_CLOCK_BASE_SHIFT        8
>  #define  SDHCI_CLOCK_V3_BASE_MASK      GENMASK(15, 8)
>  #define  SDHCI_MAX_BLOCK_MASK  0x00030000
>  #define  SDHCI_MAX_BLOCK_SHIFT  16
> --
> 2.17.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] mmc: sdhci-iproc: Add support for the legacy sdhci controller on the BCM7211

[Florian Fainelli]

On 6/8/2021 5:40 AM, Ulf Hansson wrote:
> On Wed, 2 Jun 2021 at 21:28, Al Cooper <alcooperx@gmail.com> wrote:
>>
>> Add support for the legacy Arasan sdhci controller on the BCM7211 and
>> related SoC's. This includes adding a .shutdown callback to increase
>> the power savings during S5.
> 
> Please split this into two separate changes.
> 
> May I also ask about the ->shutdown() callback and in relation to S5.
> What makes the ->shutdown callback only being invoked for S5?

It is not only called for S5 (entered via poweroff on a prompt) but also
during kexec or reboot. The poweroff path is via:

kernel_power_off() -> kernel_shutdown_prepare() -> device_shutdown() ->
.shutdown()

For kexec or reboot we do not really care about power savings since we
are about to load a new image anyway, however for S5/poweroff we do care
about quiescing the eMMC controller in a way that its clocks and the
eMMC device can be put into low power mode since we will stay in that
mode for seconds/hours/days until someone presses a button on their
remote (or other wake-up sources).
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] mmc: sdhci-iproc: Add support for the legacy sdhci controller on the BCM7211

[Ulf Hansson]

On Wed, 9 Jun 2021 at 05:07, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 6/8/2021 5:40 AM, Ulf Hansson wrote:
> > On Wed, 2 Jun 2021 at 21:28, Al Cooper <alcooperx@gmail.com> wrote:
> >>
> >> Add support for the legacy Arasan sdhci controller on the BCM7211 and
> >> related SoC's. This includes adding a .shutdown callback to increase
> >> the power savings during S5.
> >
> > Please split this into two separate changes.
> >
> > May I also ask about the ->shutdown() callback and in relation to S5.
> > What makes the ->shutdown callback only being invoked for S5?
>
> It is not only called for S5 (entered via poweroff on a prompt) but also
> during kexec or reboot. The poweroff path is via:
>
> kernel_power_off() -> kernel_shutdown_prepare() -> device_shutdown() ->
> .shutdown()
>
> For kexec or reboot we do not really care about power savings since we
> are about to load a new image anyway, however for S5/poweroff we do care
> about quiescing the eMMC controller in a way that its clocks and the
> eMMC device can be put into low power mode since we will stay in that
> mode for seconds/hours/days until someone presses a button on their
> remote (or other wake-up sources).

Hmm, I am not sure I understand correctly. At shutdown we don't care
about wake-up sources from the kernel point of view, instead we treat
everything as if it will be powered off.

We put devices into low power state at system suspend and potentially
also during some of the hibernation phases.

Graceful shutdown of the eMMC is also managed by the mmc core.

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] mmc: sdhci-iproc: Add support for the legacy sdhci controller on the BCM7211

[Florian Fainelli]

On 6/9/2021 2:22 AM, Ulf Hansson wrote:
> On Wed, 9 Jun 2021 at 05:07, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>
>>
>> On 6/8/2021 5:40 AM, Ulf Hansson wrote:
>>> On Wed, 2 Jun 2021 at 21:28, Al Cooper <alcooperx@gmail.com> wrote:
>>>>
>>>> Add support for the legacy Arasan sdhci controller on the BCM7211 and
>>>> related SoC's. This includes adding a .shutdown callback to increase
>>>> the power savings during S5.
>>>
>>> Please split this into two separate changes.
>>>
>>> May I also ask about the ->shutdown() callback and in relation to S5.
>>> What makes the ->shutdown callback only being invoked for S5?
>>
>> It is not only called for S5 (entered via poweroff on a prompt) but also
>> during kexec or reboot. The poweroff path is via:
>>
>> kernel_power_off() -> kernel_shutdown_prepare() -> device_shutdown() ->
>> .shutdown()
>>
>> For kexec or reboot we do not really care about power savings since we
>> are about to load a new image anyway, however for S5/poweroff we do care
>> about quiescing the eMMC controller in a way that its clocks and the
>> eMMC device can be put into low power mode since we will stay in that
>> mode for seconds/hours/days until someone presses a button on their
>> remote (or other wake-up sources).
> 
> Hmm, I am not sure I understand correctly. At shutdown we don't care
> about wake-up sources from the kernel point of view, instead we treat
> everything as if it will be powered off.

The same .shutdown() path is used whether you kexec, reboot or poweroff,
but for poweroff we do care about allowing specific wake-up sources
configured as such to wake-up the system at a later time, like GPIOs,
RTC, etc.

> 
> We put devices into low power state at system suspend and potentially
> also during some of the hibernation phases.
> 
> Graceful shutdown of the eMMC is also managed by the mmc core.

AFAICT that calls mmc_blk_shutdown() but that is pretty much it, the
SDHCI platform_driver still needs to do something in order to conserve
power including disabling host->clk, otherwise we would not have done
that for sdhci-brcmstb.c.
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] mmc: sdhci-iproc: Add support for the legacy sdhci controller on the BCM7211

[Ulf Hansson]

On Thu, 10 Jun 2021 at 01:59, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 6/9/2021 2:22 AM, Ulf Hansson wrote:
> > On Wed, 9 Jun 2021 at 05:07, Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>
> >>
> >>
> >> On 6/8/2021 5:40 AM, Ulf Hansson wrote:
> >>> On Wed, 2 Jun 2021 at 21:28, Al Cooper <alcooperx@gmail.com> wrote:
> >>>>
> >>>> Add support for the legacy Arasan sdhci controller on the BCM7211 and
> >>>> related SoC's. This includes adding a .shutdown callback to increase
> >>>> the power savings during S5.
> >>>
> >>> Please split this into two separate changes.
> >>>
> >>> May I also ask about the ->shutdown() callback and in relation to S5.
> >>> What makes the ->shutdown callback only being invoked for S5?
> >>
> >> It is not only called for S5 (entered via poweroff on a prompt) but also
> >> during kexec or reboot. The poweroff path is via:
> >>
> >> kernel_power_off() -> kernel_shutdown_prepare() -> device_shutdown() ->
> >> .shutdown()
> >>
> >> For kexec or reboot we do not really care about power savings since we
> >> are about to load a new image anyway, however for S5/poweroff we do care
> >> about quiescing the eMMC controller in a way that its clocks and the
> >> eMMC device can be put into low power mode since we will stay in that
> >> mode for seconds/hours/days until someone presses a button on their
> >> remote (or other wake-up sources).
> >
> > Hmm, I am not sure I understand correctly. At shutdown we don't care
> > about wake-up sources from the kernel point of view, instead we treat
> > everything as if it will be powered off.
>
> The same .shutdown() path is used whether you kexec, reboot or poweroff,
> but for poweroff we do care about allowing specific wake-up sources
> configured as such to wake-up the system at a later time, like GPIOs,
> RTC, etc.

That's true, but using the ->shutdown() callbacks in this way would
certainly be a new use case.

Most subsystems/drivers don't care about power management in those
callbacks, but rather just about managing a graceful shutdown.

It sounds to me like you should have a look at the hibernation
path/callbacks instead - or perhaps even the system suspend
path/callback. Normally, that's where we care about power management.

I have looped in Rafael, to allow him to share his opinion on this.

>
> >
> > We put devices into low power state at system suspend and potentially
> > also during some of the hibernation phases.
> >
> > Graceful shutdown of the eMMC is also managed by the mmc core.
>
> AFAICT that calls mmc_blk_shutdown() but that is pretty much it, the
> SDHCI platform_driver still needs to do something in order to conserve
> power including disabling host->clk, otherwise we would not have done
> that for sdhci-brcmstb.c.

That's not entirely correct. When mmc_bus_shutdown() is called for the
struct device* that belongs to an eMMC card, two actions are taken.

*) We call mmc_blk_shutdown(), to suspend the block device queue from
receiving new I/O requests.
**) We call host->bus_ops->shutdown(), which is an eMMC specific
callback set to mmc_shutdown(). In this step, we do a graceful
shutdown/power-off of the eMMC card.

When it comes to controller specific resources, like clocks and PM
domains, for example, those may very well stay turned on. Do deal with
these, then yes, you would need to implement the ->shutdown()
callback. But as I said above, I am not sure it's the right thing to
do.

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] mmc: sdhci-iproc: Add support for the legacy sdhci controller on the BCM7211

[Florian Fainelli]

On 6/10/2021 1:49 AM, Ulf Hansson wrote:
> On Thu, 10 Jun 2021 at 01:59, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>
>>
>> On 6/9/2021 2:22 AM, Ulf Hansson wrote:
>>> On Wed, 9 Jun 2021 at 05:07, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> On 6/8/2021 5:40 AM, Ulf Hansson wrote:
>>>>> On Wed, 2 Jun 2021 at 21:28, Al Cooper <alcooperx@gmail.com> wrote:
>>>>>>
>>>>>> Add support for the legacy Arasan sdhci controller on the BCM7211 and
>>>>>> related SoC's. This includes adding a .shutdown callback to increase
>>>>>> the power savings during S5.
>>>>>
>>>>> Please split this into two separate changes.
>>>>>
>>>>> May I also ask about the ->shutdown() callback and in relation to S5.
>>>>> What makes the ->shutdown callback only being invoked for S5?
>>>>
>>>> It is not only called for S5 (entered via poweroff on a prompt) but also
>>>> during kexec or reboot. The poweroff path is via:
>>>>
>>>> kernel_power_off() -> kernel_shutdown_prepare() -> device_shutdown() ->
>>>> .shutdown()
>>>>
>>>> For kexec or reboot we do not really care about power savings since we
>>>> are about to load a new image anyway, however for S5/poweroff we do care
>>>> about quiescing the eMMC controller in a way that its clocks and the
>>>> eMMC device can be put into low power mode since we will stay in that
>>>> mode for seconds/hours/days until someone presses a button on their
>>>> remote (or other wake-up sources).
>>>
>>> Hmm, I am not sure I understand correctly. At shutdown we don't care
>>> about wake-up sources from the kernel point of view, instead we treat
>>> everything as if it will be powered off.
>>
>> The same .shutdown() path is used whether you kexec, reboot or poweroff,
>> but for poweroff we do care about allowing specific wake-up sources
>> configured as such to wake-up the system at a later time, like GPIOs,
>> RTC, etc.
> 
> That's true, but using the ->shutdown() callbacks in this way would
> certainly be a new use case.
> 
> Most subsystems/drivers don't care about power management in those
> callbacks, but rather just about managing a graceful shutdown.
> 
> It sounds to me like you should have a look at the hibernation
> path/callbacks instead - or perhaps even the system suspend
> path/callback. Normally, that's where we care about power management.

The platforms we use do not support hibernation, keep in mind that these
are embedded SoCs that support the S2 (standby), S3 (mem) and poweroff
suspend states, hibernation is not something that we can support.

> 
> I have looped in Rafael, to allow him to share his opinion on this.
> 
>>
>>>
>>> We put devices into low power state at system suspend and potentially
>>> also during some of the hibernation phases.
>>>
>>> Graceful shutdown of the eMMC is also managed by the mmc core.
>>
>> AFAICT that calls mmc_blk_shutdown() but that is pretty much it, the
>> SDHCI platform_driver still needs to do something in order to conserve
>> power including disabling host->clk, otherwise we would not have done
>> that for sdhci-brcmstb.c.
> 
> That's not entirely correct. When mmc_bus_shutdown() is called for the
> struct device* that belongs to an eMMC card, two actions are taken.
> 
> *) We call mmc_blk_shutdown(), to suspend the block device queue from
> receiving new I/O requests.
> **) We call host->bus_ops->shutdown(), which is an eMMC specific
> callback set to mmc_shutdown(). In this step, we do a graceful
> shutdown/power-off of the eMMC card.
> 
> When it comes to controller specific resources, like clocks and PM
> domains, for example, those may very well stay turned on. Do deal with
> these, then yes, you would need to implement the ->shutdown()
> callback. But as I said above, I am not sure it's the right thing to
> do.

As explained before, we can enter S5 for an indefinite amount of time
until a wake-up source wakes us up so we must conserve power, even if we
happen to wake up the next second, we don't know that ahead of time. The
point of calling sdhci_pltfm_suspend() here is to ensure that host->clk
is turned off which cuts the eMMC controller digital clock, I forgot how
much power we save by doing so, but every 10s of mW counts for us.
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] mmc: sdhci-iproc: Add support for the legacy sdhci controller on the BCM7211

[Ulf Hansson]

On Thu, 10 Jun 2021 at 17:59, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 6/10/2021 1:49 AM, Ulf Hansson wrote:
> > On Thu, 10 Jun 2021 at 01:59, Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>
> >>
> >>
> >> On 6/9/2021 2:22 AM, Ulf Hansson wrote:
> >>> On Wed, 9 Jun 2021 at 05:07, Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 6/8/2021 5:40 AM, Ulf Hansson wrote:
> >>>>> On Wed, 2 Jun 2021 at 21:28, Al Cooper <alcooperx@gmail.com> wrote:
> >>>>>>
> >>>>>> Add support for the legacy Arasan sdhci controller on the BCM7211 and
> >>>>>> related SoC's. This includes adding a .shutdown callback to increase
> >>>>>> the power savings during S5.
> >>>>>
> >>>>> Please split this into two separate changes.
> >>>>>
> >>>>> May I also ask about the ->shutdown() callback and in relation to S5.
> >>>>> What makes the ->shutdown callback only being invoked for S5?
> >>>>
> >>>> It is not only called for S5 (entered via poweroff on a prompt) but also
> >>>> during kexec or reboot. The poweroff path is via:
> >>>>
> >>>> kernel_power_off() -> kernel_shutdown_prepare() -> device_shutdown() ->
> >>>> .shutdown()
> >>>>
> >>>> For kexec or reboot we do not really care about power savings since we
> >>>> are about to load a new image anyway, however for S5/poweroff we do care
> >>>> about quiescing the eMMC controller in a way that its clocks and the
> >>>> eMMC device can be put into low power mode since we will stay in that
> >>>> mode for seconds/hours/days until someone presses a button on their
> >>>> remote (or other wake-up sources).
> >>>
> >>> Hmm, I am not sure I understand correctly. At shutdown we don't care
> >>> about wake-up sources from the kernel point of view, instead we treat
> >>> everything as if it will be powered off.
> >>
> >> The same .shutdown() path is used whether you kexec, reboot or poweroff,
> >> but for poweroff we do care about allowing specific wake-up sources
> >> configured as such to wake-up the system at a later time, like GPIOs,
> >> RTC, etc.
> >
> > That's true, but using the ->shutdown() callbacks in this way would
> > certainly be a new use case.
> >
> > Most subsystems/drivers don't care about power management in those
> > callbacks, but rather just about managing a graceful shutdown.
> >
> > It sounds to me like you should have a look at the hibernation
> > path/callbacks instead - or perhaps even the system suspend
> > path/callback. Normally, that's where we care about power management.
>
> The platforms we use do not support hibernation, keep in mind that these
> are embedded SoCs that support the S2 (standby), S3 (mem) and poweroff
> suspend states, hibernation is not something that we can support.
>
> >
> > I have looped in Rafael, to allow him to share his opinion on this.
> >
> >>
> >>>
> >>> We put devices into low power state at system suspend and potentially
> >>> also during some of the hibernation phases.
> >>>
> >>> Graceful shutdown of the eMMC is also managed by the mmc core.
> >>
> >> AFAICT that calls mmc_blk_shutdown() but that is pretty much it, the
> >> SDHCI platform_driver still needs to do something in order to conserve
> >> power including disabling host->clk, otherwise we would not have done
> >> that for sdhci-brcmstb.c.
> >
> > That's not entirely correct. When mmc_bus_shutdown() is called for the
> > struct device* that belongs to an eMMC card, two actions are taken.
> >
> > *) We call mmc_blk_shutdown(), to suspend the block device queue from
> > receiving new I/O requests.
> > **) We call host->bus_ops->shutdown(), which is an eMMC specific
> > callback set to mmc_shutdown(). In this step, we do a graceful
> > shutdown/power-off of the eMMC card.
> >
> > When it comes to controller specific resources, like clocks and PM
> > domains, for example, those may very well stay turned on. Do deal with
> > these, then yes, you would need to implement the ->shutdown()
> > callback. But as I said above, I am not sure it's the right thing to
> > do.
>
> As explained before, we can enter S5 for an indefinite amount of time
> until a wake-up source wakes us up so we must conserve power, even if we
> happen to wake up the next second, we don't know that ahead of time. The
> point of calling sdhci_pltfm_suspend() here is to ensure that host->clk
> is turned off which cuts the eMMC controller digital clock, I forgot how
> much power we save by doing so, but every 10s of mW counts for us.

I fully understand that you want to avoid draining energy, every
single uA certainly counts in cases like these.

What puzzles me, is that your platform seems to keep some resources
powered on (like device clocks) when entering the system wide low
power state, S5.

In principle, I am wondering if it would be possible to use S5 as the
system-wide low power state for the system suspend path, rather than
S3, for example? In this way, we would be able to re-use already
implemented ->suspend|resume callbacks from most subsystems/drivers, I
believe. Or is there a problem with that?

I think we need an opinion from Rafel to move forward.

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] mmc: sdhci-iproc: Add support for the legacy sdhci controller on the BCM7211

[Florian Fainelli]

On 6/11/2021 3:23 AM, Ulf Hansson wrote:
> On Thu, 10 Jun 2021 at 17:59, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>
>>
>> On 6/10/2021 1:49 AM, Ulf Hansson wrote:
>>> On Thu, 10 Jun 2021 at 01:59, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> On 6/9/2021 2:22 AM, Ulf Hansson wrote:
>>>>> On Wed, 9 Jun 2021 at 05:07, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 6/8/2021 5:40 AM, Ulf Hansson wrote:
>>>>>>> On Wed, 2 Jun 2021 at 21:28, Al Cooper <alcooperx@gmail.com> wrote:
>>>>>>>>
>>>>>>>> Add support for the legacy Arasan sdhci controller on the BCM7211 and
>>>>>>>> related SoC's. This includes adding a .shutdown callback to increase
>>>>>>>> the power savings during S5.
>>>>>>>
>>>>>>> Please split this into two separate changes.
>>>>>>>
>>>>>>> May I also ask about the ->shutdown() callback and in relation to S5.
>>>>>>> What makes the ->shutdown callback only being invoked for S5?
>>>>>>
>>>>>> It is not only called for S5 (entered via poweroff on a prompt) but also
>>>>>> during kexec or reboot. The poweroff path is via:
>>>>>>
>>>>>> kernel_power_off() -> kernel_shutdown_prepare() -> device_shutdown() ->
>>>>>> .shutdown()
>>>>>>
>>>>>> For kexec or reboot we do not really care about power savings since we
>>>>>> are about to load a new image anyway, however for S5/poweroff we do care
>>>>>> about quiescing the eMMC controller in a way that its clocks and the
>>>>>> eMMC device can be put into low power mode since we will stay in that
>>>>>> mode for seconds/hours/days until someone presses a button on their
>>>>>> remote (or other wake-up sources).
>>>>>
>>>>> Hmm, I am not sure I understand correctly. At shutdown we don't care
>>>>> about wake-up sources from the kernel point of view, instead we treat
>>>>> everything as if it will be powered off.
>>>>
>>>> The same .shutdown() path is used whether you kexec, reboot or poweroff,
>>>> but for poweroff we do care about allowing specific wake-up sources
>>>> configured as such to wake-up the system at a later time, like GPIOs,
>>>> RTC, etc.
>>>
>>> That's true, but using the ->shutdown() callbacks in this way would
>>> certainly be a new use case.
>>>
>>> Most subsystems/drivers don't care about power management in those
>>> callbacks, but rather just about managing a graceful shutdown.
>>>
>>> It sounds to me like you should have a look at the hibernation
>>> path/callbacks instead - or perhaps even the system suspend
>>> path/callback. Normally, that's where we care about power management.
>>
>> The platforms we use do not support hibernation, keep in mind that these
>> are embedded SoCs that support the S2 (standby), S3 (mem) and poweroff
>> suspend states, hibernation is not something that we can support.
>>
>>>
>>> I have looped in Rafael, to allow him to share his opinion on this.
>>>
>>>>
>>>>>
>>>>> We put devices into low power state at system suspend and potentially
>>>>> also during some of the hibernation phases.
>>>>>
>>>>> Graceful shutdown of the eMMC is also managed by the mmc core.
>>>>
>>>> AFAICT that calls mmc_blk_shutdown() but that is pretty much it, the
>>>> SDHCI platform_driver still needs to do something in order to conserve
>>>> power including disabling host->clk, otherwise we would not have done
>>>> that for sdhci-brcmstb.c.
>>>
>>> That's not entirely correct. When mmc_bus_shutdown() is called for the
>>> struct device* that belongs to an eMMC card, two actions are taken.
>>>
>>> *) We call mmc_blk_shutdown(), to suspend the block device queue from
>>> receiving new I/O requests.
>>> **) We call host->bus_ops->shutdown(), which is an eMMC specific
>>> callback set to mmc_shutdown(). In this step, we do a graceful
>>> shutdown/power-off of the eMMC card.
>>>
>>> When it comes to controller specific resources, like clocks and PM
>>> domains, for example, those may very well stay turned on. Do deal with
>>> these, then yes, you would need to implement the ->shutdown()
>>> callback. But as I said above, I am not sure it's the right thing to
>>> do.
>>
>> As explained before, we can enter S5 for an indefinite amount of time
>> until a wake-up source wakes us up so we must conserve power, even if we
>> happen to wake up the next second, we don't know that ahead of time. The
>> point of calling sdhci_pltfm_suspend() here is to ensure that host->clk
>> is turned off which cuts the eMMC controller digital clock, I forgot how
>> much power we save by doing so, but every 10s of mW counts for us.
> 
> I fully understand that you want to avoid draining energy, every
> single uA certainly counts in cases like these.
> 
> What puzzles me, is that your platform seems to keep some resources
> powered on (like device clocks) when entering the system wide low
> power state, S5.

More on that below.

> 
> In principle, I am wondering if it would be possible to use S5 as the
> system-wide low power state for the system suspend path, rather than
> S3, for example? In this way, we would be able to re-use already
> implemented ->suspend|resume callbacks from most subsystems/drivers, I
> believe. Or is there a problem with that?

The specific platform this driver is used on (BCM7211) is only capable
of supporting S2 and S5. There is no S3 because we have no provision on
the board to maintain the DRAM supplies on and preserve the DRAM
contents. This is a design choice that is different from the other
Broadcom STB platforms where we offer S2, S3 and S5 and we have an
On/off domain which is shutdown by hardware upon S3 or S5 entry and a
small always on domain which remains on to service wake-up sources
(infrared, timer, gpio, UART, etc.). S2 on this platform is implemented
entirely in software/firmware and does make use of the regular
suspend/resume calls.

S5 is implemented in part in software/firmware and with the help of the
hardware that will turn off external board components. We do need the
help of the various software drivers (PCIe, Ethernet, GPIO, USB, UART,
RTC, eMMC, SPI, etc.) to do their job and conserve power when we enter
S5, hence the reason why all of our drivers implement ->shutdown() (in
addition to needing that for kexec and ensure no DMA is left running).

> 
> I think we need an opinion from Rafel to move forward.

There is already an identical change done for sdhci-brcmstb.c, and the
exact same rationale applied there since both sdhci-iproc.c and
sdhci-brcmstb.c are used on this BCM7211 platform.

In all honesty, I am a bit surprised that the Linux device driver model
does not try to default the absence of a ->shutdown() to a ->suspend()
call since in most cases they are functionally equivalent, or should be,
in that they need to save power and quiesce the hardware, or leave
enough running to support a wake-up event.

Cheers
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] mmc: sdhci-iproc: Add support for the legacy sdhci controller on the BCM7211

[Ulf Hansson]

On Fri, 11 Jun 2021 at 18:54, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 6/11/2021 3:23 AM, Ulf Hansson wrote:
> > On Thu, 10 Jun 2021 at 17:59, Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>
> >>
> >>
> >> On 6/10/2021 1:49 AM, Ulf Hansson wrote:
> >>> On Thu, 10 Jun 2021 at 01:59, Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 6/9/2021 2:22 AM, Ulf Hansson wrote:
> >>>>> On Wed, 9 Jun 2021 at 05:07, Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 6/8/2021 5:40 AM, Ulf Hansson wrote:
> >>>>>>> On Wed, 2 Jun 2021 at 21:28, Al Cooper <alcooperx@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>> Add support for the legacy Arasan sdhci controller on the BCM7211 and
> >>>>>>>> related SoC's. This includes adding a .shutdown callback to increase
> >>>>>>>> the power savings during S5.
> >>>>>>>
> >>>>>>> Please split this into two separate changes.
> >>>>>>>
> >>>>>>> May I also ask about the ->shutdown() callback and in relation to S5.
> >>>>>>> What makes the ->shutdown callback only being invoked for S5?
> >>>>>>
> >>>>>> It is not only called for S5 (entered via poweroff on a prompt) but also
> >>>>>> during kexec or reboot. The poweroff path is via:
> >>>>>>
> >>>>>> kernel_power_off() -> kernel_shutdown_prepare() -> device_shutdown() ->
> >>>>>> .shutdown()
> >>>>>>
> >>>>>> For kexec or reboot we do not really care about power savings since we
> >>>>>> are about to load a new image anyway, however for S5/poweroff we do care
> >>>>>> about quiescing the eMMC controller in a way that its clocks and the
> >>>>>> eMMC device can be put into low power mode since we will stay in that
> >>>>>> mode for seconds/hours/days until someone presses a button on their
> >>>>>> remote (or other wake-up sources).
> >>>>>
> >>>>> Hmm, I am not sure I understand correctly. At shutdown we don't care
> >>>>> about wake-up sources from the kernel point of view, instead we treat
> >>>>> everything as if it will be powered off.
> >>>>
> >>>> The same .shutdown() path is used whether you kexec, reboot or poweroff,
> >>>> but for poweroff we do care about allowing specific wake-up sources
> >>>> configured as such to wake-up the system at a later time, like GPIOs,
> >>>> RTC, etc.
> >>>
> >>> That's true, but using the ->shutdown() callbacks in this way would
> >>> certainly be a new use case.
> >>>
> >>> Most subsystems/drivers don't care about power management in those
> >>> callbacks, but rather just about managing a graceful shutdown.
> >>>
> >>> It sounds to me like you should have a look at the hibernation
> >>> path/callbacks instead - or perhaps even the system suspend
> >>> path/callback. Normally, that's where we care about power management.
> >>
> >> The platforms we use do not support hibernation, keep in mind that these
> >> are embedded SoCs that support the S2 (standby), S3 (mem) and poweroff
> >> suspend states, hibernation is not something that we can support.
> >>
> >>>
> >>> I have looped in Rafael, to allow him to share his opinion on this.
> >>>
> >>>>
> >>>>>
> >>>>> We put devices into low power state at system suspend and potentially
> >>>>> also during some of the hibernation phases.
> >>>>>
> >>>>> Graceful shutdown of the eMMC is also managed by the mmc core.
> >>>>
> >>>> AFAICT that calls mmc_blk_shutdown() but that is pretty much it, the
> >>>> SDHCI platform_driver still needs to do something in order to conserve
> >>>> power including disabling host->clk, otherwise we would not have done
> >>>> that for sdhci-brcmstb.c.
> >>>
> >>> That's not entirely correct. When mmc_bus_shutdown() is called for the
> >>> struct device* that belongs to an eMMC card, two actions are taken.
> >>>
> >>> *) We call mmc_blk_shutdown(), to suspend the block device queue from
> >>> receiving new I/O requests.
> >>> **) We call host->bus_ops->shutdown(), which is an eMMC specific
> >>> callback set to mmc_shutdown(). In this step, we do a graceful
> >>> shutdown/power-off of the eMMC card.
> >>>
> >>> When it comes to controller specific resources, like clocks and PM
> >>> domains, for example, those may very well stay turned on. Do deal with
> >>> these, then yes, you would need to implement the ->shutdown()
> >>> callback. But as I said above, I am not sure it's the right thing to
> >>> do.
> >>
> >> As explained before, we can enter S5 for an indefinite amount of time
> >> until a wake-up source wakes us up so we must conserve power, even if we
> >> happen to wake up the next second, we don't know that ahead of time. The
> >> point of calling sdhci_pltfm_suspend() here is to ensure that host->clk
> >> is turned off which cuts the eMMC controller digital clock, I forgot how
> >> much power we save by doing so, but every 10s of mW counts for us.
> >
> > I fully understand that you want to avoid draining energy, every
> > single uA certainly counts in cases like these.
> >
> > What puzzles me, is that your platform seems to keep some resources
> > powered on (like device clocks) when entering the system wide low
> > power state, S5.
>
> More on that below.
>
> >
> > In principle, I am wondering if it would be possible to use S5 as the
> > system-wide low power state for the system suspend path, rather than
> > S3, for example? In this way, we would be able to re-use already
> > implemented ->suspend|resume callbacks from most subsystems/drivers, I
> > believe. Or is there a problem with that?
>
> The specific platform this driver is used on (BCM7211) is only capable
> of supporting S2 and S5. There is no S3 because we have no provision on
> the board to maintain the DRAM supplies on and preserve the DRAM
> contents. This is a design choice that is different from the other
> Broadcom STB platforms where we offer S2, S3 and S5 and we have an
> On/off domain which is shutdown by hardware upon S3 or S5 entry and a
> small always on domain which remains on to service wake-up sources
> (infrared, timer, gpio, UART, etc.). S2 on this platform is implemented
> entirely in software/firmware and does make use of the regular
> suspend/resume calls.
>
> S5 is implemented in part in software/firmware and with the help of the
> hardware that will turn off external board components. We do need the
> help of the various software drivers (PCIe, Ethernet, GPIO, USB, UART,
> RTC, eMMC, SPI, etc.) to do their job and conserve power when we enter
> S5, hence the reason why all of our drivers implement ->shutdown() (in
> addition to needing that for kexec and ensure no DMA is left running).
>
> >
> > I think we need an opinion from Rafel to move forward.
>
> There is already an identical change done for sdhci-brcmstb.c, and the
> exact same rationale applied there since both sdhci-iproc.c and
> sdhci-brcmstb.c are used on this BCM7211 platform.

Right, thanks for the pointer. Looks like we should have taken this
discussion back then, but better late than never.

>
> In all honesty, I am a bit surprised that the Linux device driver model
> does not try to default the absence of a ->shutdown() to a ->suspend()
> call since in most cases they are functionally equivalent, or should be,
> in that they need to save power and quiesce the hardware, or leave
> enough running to support a wake-up event.

Well, the generall assumption is that the platform is going to be
entirely powered off, thus moving things into a low power state would
just be a waste of execution cycles. Of course, that's not the case
for your platform.

As I have stated earlier, to me it looks a bit questionable to use the
kernel_power_off() path to support the use case you describe. On the
other hand, we may not have a better option at this point.

Just a few things, from the top of my head, that we certainly are
missing to support your use case through kernel_power_off() path
(there are certainly more):
1. In general, subsystems/drivers don't care about moving things into
lower power modes from their ->shutdown() callbacks.
2. System wakeups and devices being affected in the wakeup path, needs
to be respected properly. Additionally, userspace should be able to
decide if system wakeups should be enabled or not.
3. PM domains don't have ->shutdown() callbacks, thus it's likely that
they remain powered on.
4. Etc...

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] mmc: sdhci-iproc: Add support for the legacy sdhci controller on the BCM7211

[Florian Fainelli]

On 6/14/2021 6:19 AM, Ulf Hansson wrote:
> On Fri, 11 Jun 2021 at 18:54, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>
>>
>> On 6/11/2021 3:23 AM, Ulf Hansson wrote:
>>> On Thu, 10 Jun 2021 at 17:59, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> On 6/10/2021 1:49 AM, Ulf Hansson wrote:
>>>>> On Thu, 10 Jun 2021 at 01:59, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 6/9/2021 2:22 AM, Ulf Hansson wrote:
>>>>>>> On Wed, 9 Jun 2021 at 05:07, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 6/8/2021 5:40 AM, Ulf Hansson wrote:
>>>>>>>>> On Wed, 2 Jun 2021 at 21:28, Al Cooper <alcooperx@gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>> Add support for the legacy Arasan sdhci controller on the BCM7211 and
>>>>>>>>>> related SoC's. This includes adding a .shutdown callback to increase
>>>>>>>>>> the power savings during S5.
>>>>>>>>>
>>>>>>>>> Please split this into two separate changes.
>>>>>>>>>
>>>>>>>>> May I also ask about the ->shutdown() callback and in relation to S5.
>>>>>>>>> What makes the ->shutdown callback only being invoked for S5?
>>>>>>>>
>>>>>>>> It is not only called for S5 (entered via poweroff on a prompt) but also
>>>>>>>> during kexec or reboot. The poweroff path is via:
>>>>>>>>
>>>>>>>> kernel_power_off() -> kernel_shutdown_prepare() -> device_shutdown() ->
>>>>>>>> .shutdown()
>>>>>>>>
>>>>>>>> For kexec or reboot we do not really care about power savings since we
>>>>>>>> are about to load a new image anyway, however for S5/poweroff we do care
>>>>>>>> about quiescing the eMMC controller in a way that its clocks and the
>>>>>>>> eMMC device can be put into low power mode since we will stay in that
>>>>>>>> mode for seconds/hours/days until someone presses a button on their
>>>>>>>> remote (or other wake-up sources).
>>>>>>>
>>>>>>> Hmm, I am not sure I understand correctly. At shutdown we don't care
>>>>>>> about wake-up sources from the kernel point of view, instead we treat
>>>>>>> everything as if it will be powered off.
>>>>>>
>>>>>> The same .shutdown() path is used whether you kexec, reboot or poweroff,
>>>>>> but for poweroff we do care about allowing specific wake-up sources
>>>>>> configured as such to wake-up the system at a later time, like GPIOs,
>>>>>> RTC, etc.
>>>>>
>>>>> That's true, but using the ->shutdown() callbacks in this way would
>>>>> certainly be a new use case.
>>>>>
>>>>> Most subsystems/drivers don't care about power management in those
>>>>> callbacks, but rather just about managing a graceful shutdown.
>>>>>
>>>>> It sounds to me like you should have a look at the hibernation
>>>>> path/callbacks instead - or perhaps even the system suspend
>>>>> path/callback. Normally, that's where we care about power management.
>>>>
>>>> The platforms we use do not support hibernation, keep in mind that these
>>>> are embedded SoCs that support the S2 (standby), S3 (mem) and poweroff
>>>> suspend states, hibernation is not something that we can support.
>>>>
>>>>>
>>>>> I have looped in Rafael, to allow him to share his opinion on this.
>>>>>
>>>>>>
>>>>>>>
>>>>>>> We put devices into low power state at system suspend and potentially
>>>>>>> also during some of the hibernation phases.
>>>>>>>
>>>>>>> Graceful shutdown of the eMMC is also managed by the mmc core.
>>>>>>
>>>>>> AFAICT that calls mmc_blk_shutdown() but that is pretty much it, the
>>>>>> SDHCI platform_driver still needs to do something in order to conserve
>>>>>> power including disabling host->clk, otherwise we would not have done
>>>>>> that for sdhci-brcmstb.c.
>>>>>
>>>>> That's not entirely correct. When mmc_bus_shutdown() is called for the
>>>>> struct device* that belongs to an eMMC card, two actions are taken.
>>>>>
>>>>> *) We call mmc_blk_shutdown(), to suspend the block device queue from
>>>>> receiving new I/O requests.
>>>>> **) We call host->bus_ops->shutdown(), which is an eMMC specific
>>>>> callback set to mmc_shutdown(). In this step, we do a graceful
>>>>> shutdown/power-off of the eMMC card.
>>>>>
>>>>> When it comes to controller specific resources, like clocks and PM
>>>>> domains, for example, those may very well stay turned on. Do deal with
>>>>> these, then yes, you would need to implement the ->shutdown()
>>>>> callback. But as I said above, I am not sure it's the right thing to
>>>>> do.
>>>>
>>>> As explained before, we can enter S5 for an indefinite amount of time
>>>> until a wake-up source wakes us up so we must conserve power, even if we
>>>> happen to wake up the next second, we don't know that ahead of time. The
>>>> point of calling sdhci_pltfm_suspend() here is to ensure that host->clk
>>>> is turned off which cuts the eMMC controller digital clock, I forgot how
>>>> much power we save by doing so, but every 10s of mW counts for us.
>>>
>>> I fully understand that you want to avoid draining energy, every
>>> single uA certainly counts in cases like these.
>>>
>>> What puzzles me, is that your platform seems to keep some resources
>>> powered on (like device clocks) when entering the system wide low
>>> power state, S5.
>>
>> More on that below.
>>
>>>
>>> In principle, I am wondering if it would be possible to use S5 as the
>>> system-wide low power state for the system suspend path, rather than
>>> S3, for example? In this way, we would be able to re-use already
>>> implemented ->suspend|resume callbacks from most subsystems/drivers, I
>>> believe. Or is there a problem with that?
>>
>> The specific platform this driver is used on (BCM7211) is only capable
>> of supporting S2 and S5. There is no S3 because we have no provision on
>> the board to maintain the DRAM supplies on and preserve the DRAM
>> contents. This is a design choice that is different from the other
>> Broadcom STB platforms where we offer S2, S3 and S5 and we have an
>> On/off domain which is shutdown by hardware upon S3 or S5 entry and a
>> small always on domain which remains on to service wake-up sources
>> (infrared, timer, gpio, UART, etc.). S2 on this platform is implemented
>> entirely in software/firmware and does make use of the regular
>> suspend/resume calls.
>>
>> S5 is implemented in part in software/firmware and with the help of the
>> hardware that will turn off external board components. We do need the
>> help of the various software drivers (PCIe, Ethernet, GPIO, USB, UART,
>> RTC, eMMC, SPI, etc.) to do their job and conserve power when we enter
>> S5, hence the reason why all of our drivers implement ->shutdown() (in
>> addition to needing that for kexec and ensure no DMA is left running).
>>
>>>
>>> I think we need an opinion from Rafel to move forward.
>>
>> There is already an identical change done for sdhci-brcmstb.c, and the
>> exact same rationale applied there since both sdhci-iproc.c and
>> sdhci-brcmstb.c are used on this BCM7211 platform.
> 
> Right, thanks for the pointer. Looks like we should have taken this
> discussion back then, but better late than never.
> 
>>
>> In all honesty, I am a bit surprised that the Linux device driver model
>> does not try to default the absence of a ->shutdown() to a ->suspend()
>> call since in most cases they are functionally equivalent, or should be,
>> in that they need to save power and quiesce the hardware, or leave
>> enough running to support a wake-up event.
> 
> Well, the generall assumption is that the platform is going to be
> entirely powered off, thus moving things into a low power state would
> just be a waste of execution cycles. Of course, that's not the case
> for your platform.

That assumption may hold true for ACPI-enabled machines but power off is
offered as a general function towards other more flexible and snowflaky
systems (read embedded) as well.

> 
> As I have stated earlier, to me it looks a bit questionable to use the
> kernel_power_off() path to support the use case you describe. On the
> other hand, we may not have a better option at this point.

Correct, there is not really anything better and I am not sure what the
semantics of something better could be anyway.

> 
> Just a few things, from the top of my head, that we certainly are
> missing to support your use case through kernel_power_off() path
> (there are certainly more):
> 1. In general, subsystems/drivers don't care about moving things into
> lower power modes from their ->shutdown() callbacks.
> 2. System wakeups and devices being affected in the wakeup path, needs
> to be respected properly. Additionally, userspace should be able to
> decide if system wakeups should be enabled or not.
> 3. PM domains don't have ->shutdown() callbacks, thus it's likely that
> they remain powered on.
> 4. Etc...

For the particular eMMC driver being discussed here this is a no-brainer
because  it is not a wake-up source, therefore there is no reason not to
power if off if we can. It also seems proper to have it done by the
kernel as opposed to firmware.
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] mmc: sdhci-iproc: Add support for the legacy sdhci controller on the BCM7211

[Ulf Hansson]

[...]

> >
> >>
> >> In all honesty, I am a bit surprised that the Linux device driver model
> >> does not try to default the absence of a ->shutdown() to a ->suspend()
> >> call since in most cases they are functionally equivalent, or should be,
> >> in that they need to save power and quiesce the hardware, or leave
> >> enough running to support a wake-up event.
> >
> > Well, the generall assumption is that the platform is going to be
> > entirely powered off, thus moving things into a low power state would
> > just be a waste of execution cycles. Of course, that's not the case
> > for your platform.
>
> That assumption may hold true for ACPI-enabled machines but power off is
> offered as a general function towards other more flexible and snowflaky
> systems (read embedded) as well.
>
> >
> > As I have stated earlier, to me it looks a bit questionable to use the
> > kernel_power_off() path to support the use case you describe. On the
> > other hand, we may not have a better option at this point.
>
> Correct, there is not really anything better and I am not sure what the
> semantics of something better could be anyway.
>
> >
> > Just a few things, from the top of my head, that we certainly are
> > missing to support your use case through kernel_power_off() path
> > (there are certainly more):
> > 1. In general, subsystems/drivers don't care about moving things into
> > lower power modes from their ->shutdown() callbacks.
> > 2. System wakeups and devices being affected in the wakeup path, needs
> > to be respected properly. Additionally, userspace should be able to
> > decide if system wakeups should be enabled or not.
> > 3. PM domains don't have ->shutdown() callbacks, thus it's likely that
> > they remain powered on.
> > 4. Etc...
>
> For the particular eMMC driver being discussed here this is a no-brainer
 > because  it is not a wake-up source, therefore there is no reason not to
> power if off if we can. It also seems proper to have it done by the
> kernel as opposed to firmware.

Okay, I have applied the $subject patch onto my next branch, along
with patch 1/2 (the DT doc change).

However, I still think we should look for a proper long term solution,
because the kernel_power_off() path does not currently support your
use case, with system wakeups etc.

I guess it could be a topic that is easier to bring up at the Linux
Plumbers Conf, for example.

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] mmc: sdhci-iproc: Add support for the legacy sdhci controller on the BCM7211

[Florian Fainelli]

On 6/15/2021 8:30 AM, Ulf Hansson wrote:
> [...]
> 
>>>
>>>>
>>>> In all honesty, I am a bit surprised that the Linux device driver model
>>>> does not try to default the absence of a ->shutdown() to a ->suspend()
>>>> call since in most cases they are functionally equivalent, or should be,
>>>> in that they need to save power and quiesce the hardware, or leave
>>>> enough running to support a wake-up event.
>>>
>>> Well, the generall assumption is that the platform is going to be
>>> entirely powered off, thus moving things into a low power state would
>>> just be a waste of execution cycles. Of course, that's not the case
>>> for your platform.
>>
>> That assumption may hold true for ACPI-enabled machines but power off is
>> offered as a general function towards other more flexible and snowflaky
>> systems (read embedded) as well.
>>
>>>
>>> As I have stated earlier, to me it looks a bit questionable to use the
>>> kernel_power_off() path to support the use case you describe. On the
>>> other hand, we may not have a better option at this point.
>>
>> Correct, there is not really anything better and I am not sure what the
>> semantics of something better could be anyway.
>>
>>>
>>> Just a few things, from the top of my head, that we certainly are
>>> missing to support your use case through kernel_power_off() path
>>> (there are certainly more):
>>> 1. In general, subsystems/drivers don't care about moving things into
>>> lower power modes from their ->shutdown() callbacks.
>>> 2. System wakeups and devices being affected in the wakeup path, needs
>>> to be respected properly. Additionally, userspace should be able to
>>> decide if system wakeups should be enabled or not.
>>> 3. PM domains don't have ->shutdown() callbacks, thus it's likely that
>>> they remain powered on.
>>> 4. Etc...
>>
>> For the particular eMMC driver being discussed here this is a no-brainer
>  > because  it is not a wake-up source, therefore there is no reason not to
>> power if off if we can. It also seems proper to have it done by the
>> kernel as opposed to firmware.
> 
> Okay, I have applied the $subject patch onto my next branch, along
> with patch 1/2 (the DT doc change).
> 
> However, I still think we should look for a proper long term solution,
> because the kernel_power_off() path does not currently support your
> use case, with system wakeups etc.

Not really, it does work fine, some drivers like gpio-keys.c or
gpio-brcmstb.c will ensure that the GPIOs that are enabled as wake-up
interrupts are configured that way during kernel_power_off() and the
various interrupt controllers like irq-brcmstb-l2.c will make sure they
don't mask wake-up interrupts.

> 
> I guess it could be a topic that is easier to bring up at the Linux
> Plumbers Conf, for example.

OK, not sure if I will be able to attend, but would definitively try to.
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] dt-bindings: mmc: sdhci-iproc: Add brcm,bcm7211a0-sdhci

[Rob Herring]

On Wed, 02 Jun 2021 15:27:57 -0400, Al Cooper wrote:
> Add new compatible string for the legacy sdhci controller on the
> BCM7211 family of SoC's.
> 
> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> ---
>  Documentation/devicetree/bindings/mmc/brcm,iproc-sdhci.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel