mbox series

[v3,0/3] Add Broadcom STB memory controller driver

Message ID 20220801220931.181531-1-f.fainelli@gmail.com
Headers show
Series Add Broadcom STB memory controller driver | expand

Message

Florian Fainelli Aug. 1, 2022, 10:09 p.m. UTC 
Hi Krzysztof,

This small patch series adds basic support for controlling self-refresh
power down on Broadcom STB memory controllers. We might be able to
contribute more features to the memory controller driver in the future
like accurate reporting of the memory type, timings, and possibly some
performance counters.

Changes in v3:

- made 'frequency' property optional to avoid introducing warnings for
  existing BMIPS and BCM7445 DTS files
- updated sysfs document to use a shorter and universal path

Changes in v2:

- merged the v1 first two patches
- added a sysfs document describing attributes exposed
- addressed feedback from Krzysztof regarding style and API usage


Florian Fainelli (3):
  dt-bindings: memory-controller: Document Broadcom STB MEMC
  Documentation: sysfs: Document Broadcom STB memc sysfs knobs
  memory: Add Broadcom STB memory controller driver

 .../ABI/testing/sysfs-platform-brcmstb-memc   |  15 +
 .../bindings/arm/bcm/brcm,brcmstb.txt         |  11 +-
 .../brcm,brcmstb-memc-ddr.yaml                |  52 +++
 drivers/memory/Kconfig                        |   9 +
 drivers/memory/Makefile                       |   1 +
 drivers/memory/brcmstb_memc.c                 | 302 ++++++++++++++++++
 6 files changed, 381 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-brcmstb-memc
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/brcm,brcmstb-memc-ddr.yaml
 create mode 100644 drivers/memory/brcmstb_memc.c

Comments

Krzysztof Kozlowski Aug. 9, 2022, 9:58 a.m. UTC | #1 
On 02/08/2022 01:09, Florian Fainelli wrote:
> Add support for configuring the Self Refresh Power Down (SRPD)
> inactivity timeout on Broadcom STB chips. This is used to conserve power
> when the DRAM activity is reduced.
> 


> +static int __maybe_unused brcmstb_memc_resume(struct device *dev)
> +{
> +	struct brcmstb_memc *memc = dev_get_drvdata(dev);
> +
> +	if (memc->timeout_cycles == 0)
> +		return 0;
> +
> +	return brcmstb_memc_srpd_config(memc, memc->timeout_cycles);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(brcmstb_memc_pm_ops, brcmstb_memc_suspend,
> +			 brcmstb_memc_resume);
> +
> +static struct platform_driver brcmstb_memc_driver = {
> +	.probe = brcmstb_memc_probe,
> +	.remove = brcmstb_memc_remove,
> +	.driver = {
> +		.name		= "brcmstb_memc",
> +		.owner		= THIS_MODULE,

No need, run coccinelle.

> +		.of_match_table	= brcmstb_memc_of_match,
> +		.pm		= &brcmstb_memc_pm_ops,

Shouldn't this be pm_ptr()? and then no need for __maybe_unused in
brcmstb_memc_resume/suspend.

> +	},
> +};
> +module_platform_driver(brcmstb_memc_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Broadcom");
> +MODULE_DESCRIPTION("DDR SRPD driver for Broadcom STB chips");


Best regards,
Krzysztof
Florian Fainelli Aug. 12, 2022, 5:29 p.m. UTC | #2 
On 8/9/22 02:58, Krzysztof Kozlowski wrote:
> On 02/08/2022 01:09, Florian Fainelli wrote:
>> Add support for configuring the Self Refresh Power Down (SRPD)
>> inactivity timeout on Broadcom STB chips. This is used to conserve power
>> when the DRAM activity is reduced.
>>
> 
> 
>> +static int __maybe_unused brcmstb_memc_resume(struct device *dev)
>> +{
>> +	struct brcmstb_memc *memc = dev_get_drvdata(dev);
>> +
>> +	if (memc->timeout_cycles == 0)
>> +		return 0;
>> +
>> +	return brcmstb_memc_srpd_config(memc, memc->timeout_cycles);
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(brcmstb_memc_pm_ops, brcmstb_memc_suspend,
>> +			 brcmstb_memc_resume);
>> +
>> +static struct platform_driver brcmstb_memc_driver = {
>> +	.probe = brcmstb_memc_probe,
>> +	.remove = brcmstb_memc_remove,
>> +	.driver = {
>> +		.name		= "brcmstb_memc",
>> +		.owner		= THIS_MODULE,
> 
> No need, run coccinelle.
> 
>> +		.of_match_table	= brcmstb_memc_of_match,
>> +		.pm		= &brcmstb_memc_pm_ops,
> 
> Shouldn't this be pm_ptr()? and then no need for __maybe_unused in
> brcmstb_memc_resume/suspend.

How can one can remove __maybe_unused without causing a warning for the 
CONFIG_PM=n case, not that I needed to build to convince myself, but 
still did anyway:

drivers/memory/brcmstb_memc.c:275:12: warning: 'brcmstb_memc_resume' 
defined but not used [-Wunused-function]
  static int brcmstb_memc_resume(struct device *dev)
             ^~~~~~~~~~~~~~~~~~~
drivers/memory/brcmstb_memc.c:252:12: warning: 'brcmstb_memc_suspend' 
defined but not used [-Wunused-function]
  static int brcmstb_memc_suspend(struct device *dev)
             ^~~~~~~~~~~~~~~~~~~~

unless you also implied enclosing those functions under an #if 
IS_ENABLED(CONFIG_PM) or something which is IMHO less preferable.
Krzysztof Kozlowski Aug. 12, 2022, 5:36 p.m. UTC | #3 
On 12/08/2022 20:29, Florian Fainelli wrote:
> On 8/9/22 02:58, Krzysztof Kozlowski wrote:
>> On 02/08/2022 01:09, Florian Fainelli wrote:
>>> Add support for configuring the Self Refresh Power Down (SRPD)
>>> inactivity timeout on Broadcom STB chips. This is used to conserve power
>>> when the DRAM activity is reduced.
>>>
>>
>>
>>> +static int __maybe_unused brcmstb_memc_resume(struct device *dev)
>>> +{
>>> +	struct brcmstb_memc *memc = dev_get_drvdata(dev);
>>> +
>>> +	if (memc->timeout_cycles == 0)
>>> +		return 0;
>>> +
>>> +	return brcmstb_memc_srpd_config(memc, memc->timeout_cycles);
>>> +}
>>> +
>>> +static SIMPLE_DEV_PM_OPS(brcmstb_memc_pm_ops, brcmstb_memc_suspend,
>>> +			 brcmstb_memc_resume);
>>> +
>>> +static struct platform_driver brcmstb_memc_driver = {
>>> +	.probe = brcmstb_memc_probe,
>>> +	.remove = brcmstb_memc_remove,
>>> +	.driver = {
>>> +		.name		= "brcmstb_memc",
>>> +		.owner		= THIS_MODULE,
>>
>> No need, run coccinelle.
>>
>>> +		.of_match_table	= brcmstb_memc_of_match,
>>> +		.pm		= &brcmstb_memc_pm_ops,
>>
>> Shouldn't this be pm_ptr()? and then no need for __maybe_unused in
>> brcmstb_memc_resume/suspend.
> 
> How can one can remove __maybe_unused without causing a warning for the 
> CONFIG_PM=n case, not that I needed to build to convince myself, but 
> still did anyway:
> 
> drivers/memory/brcmstb_memc.c:275:12: warning: 'brcmstb_memc_resume' 
> defined but not used [-Wunused-function]
>   static int brcmstb_memc_resume(struct device *dev)
>              ^~~~~~~~~~~~~~~~~~~
> drivers/memory/brcmstb_memc.c:252:12: warning: 'brcmstb_memc_suspend' 
> defined but not used [-Wunused-function]
>   static int brcmstb_memc_suspend(struct device *dev)
>              ^~~~~~~~~~~~~~~~~~~~
> 
> unless you also implied enclosing those functions under an #if 
> IS_ENABLED(CONFIG_PM) or something which is IMHO less preferable.

Are you sure you added also pm_ptr()? I don't see such warnings with W=1
and final object does not have the functions (for a different driver but
same principle).

Best regards,
Krzysztof
Florian Fainelli Aug. 12, 2022, 5:52 p.m. UTC | #4 
On 8/12/22 10:36, Krzysztof Kozlowski wrote:
> On 12/08/2022 20:29, Florian Fainelli wrote:
>> On 8/9/22 02:58, Krzysztof Kozlowski wrote:
>>> On 02/08/2022 01:09, Florian Fainelli wrote:
>>>> Add support for configuring the Self Refresh Power Down (SRPD)
>>>> inactivity timeout on Broadcom STB chips. This is used to conserve power
>>>> when the DRAM activity is reduced.
>>>>
>>>
>>>
>>>> +static int __maybe_unused brcmstb_memc_resume(struct device *dev)
>>>> +{
>>>> +	struct brcmstb_memc *memc = dev_get_drvdata(dev);
>>>> +
>>>> +	if (memc->timeout_cycles == 0)
>>>> +		return 0;
>>>> +
>>>> +	return brcmstb_memc_srpd_config(memc, memc->timeout_cycles);
>>>> +}
>>>> +
>>>> +static SIMPLE_DEV_PM_OPS(brcmstb_memc_pm_ops, brcmstb_memc_suspend,
>>>> +			 brcmstb_memc_resume);
>>>> +
>>>> +static struct platform_driver brcmstb_memc_driver = {
>>>> +	.probe = brcmstb_memc_probe,
>>>> +	.remove = brcmstb_memc_remove,
>>>> +	.driver = {
>>>> +		.name		= "brcmstb_memc",
>>>> +		.owner		= THIS_MODULE,
>>>
>>> No need, run coccinelle.
>>>
>>>> +		.of_match_table	= brcmstb_memc_of_match,
>>>> +		.pm		= &brcmstb_memc_pm_ops,
>>>
>>> Shouldn't this be pm_ptr()? and then no need for __maybe_unused in
>>> brcmstb_memc_resume/suspend.
>>
>> How can one can remove __maybe_unused without causing a warning for the
>> CONFIG_PM=n case, not that I needed to build to convince myself, but
>> still did anyway:
>>
>> drivers/memory/brcmstb_memc.c:275:12: warning: 'brcmstb_memc_resume'
>> defined but not used [-Wunused-function]
>>    static int brcmstb_memc_resume(struct device *dev)
>>               ^~~~~~~~~~~~~~~~~~~
>> drivers/memory/brcmstb_memc.c:252:12: warning: 'brcmstb_memc_suspend'
>> defined but not used [-Wunused-function]
>>    static int brcmstb_memc_suspend(struct device *dev)
>>               ^~~~~~~~~~~~~~~~~~~~
>>
>> unless you also implied enclosing those functions under an #if
>> IS_ENABLED(CONFIG_PM) or something which is IMHO less preferable.
> 
> Are you sure you added also pm_ptr()? I don't see such warnings with W=1
> and final object does not have the functions (for a different driver but
> same principle).

Yes I am sure I added pm_ptr() see the v4 I just submitted. I don't see 
how the compiler cannot warn about the functions being unused the day 
they stop being referenced by the pm_ops structure which is eliminated?
Krzysztof Kozlowski Aug. 12, 2022, 6:41 p.m. UTC | #5 
On 12/08/2022 20:52, Florian Fainelli wrote:

>>> unless you also implied enclosing those functions under an #if
>>> IS_ENABLED(CONFIG_PM) or something which is IMHO less preferable.
>>
>> Are you sure you added also pm_ptr()? I don't see such warnings with W=1
>> and final object does not have the functions (for a different driver but
>> same principle).
> 
> Yes I am sure I added pm_ptr() see the v4 I just submitted. I don't see 
> how the compiler cannot warn about the functions being unused the day 
> they stop being referenced by the pm_ops structure which is eliminated?

I don't have the answer how it exactly works (or which gcc version
introduced it), but I tested it and the functions were not present in
the object file, thus of course no warnings.

The driver change I am referring to is:
https://lore.kernel.org/all/20220808174107.38676-15-paul@crapouillou.net/

I think the only difference against your v4 is:
DEFINE_SIMPLE_DEV_PM_OPS
and lack of __maybe_unused on the functions.

The DEFINE_SIMPLE_DEV_PM_OPS itself adds __maybe_unused for !CONFIG_PM
case, but I don't think it is relevant.

Best regards,
Krzysztof
Florian Fainelli Aug. 12, 2022, 10:22 p.m. UTC | #6 
On 8/12/22 11:41, Krzysztof Kozlowski wrote:
> On 12/08/2022 20:52, Florian Fainelli wrote:
> 
>>>> unless you also implied enclosing those functions under an #if
>>>> IS_ENABLED(CONFIG_PM) or something which is IMHO less preferable.
>>>
>>> Are you sure you added also pm_ptr()? I don't see such warnings with W=1
>>> and final object does not have the functions (for a different driver but
>>> same principle).
>>
>> Yes I am sure I added pm_ptr() see the v4 I just submitted. I don't see
>> how the compiler cannot warn about the functions being unused the day
>> they stop being referenced by the pm_ops structure which is eliminated?
> 
> I don't have the answer how it exactly works (or which gcc version
> introduced it), but I tested it and the functions were not present in
> the object file, thus of course no warnings.
> 
> The driver change I am referring to is:
> https://lore.kernel.org/all/20220808174107.38676-15-paul@crapouillou.net/
> 
> I think the only difference against your v4 is:
> DEFINE_SIMPLE_DEV_PM_OPS
> and lack of __maybe_unused on the functions.
> 
> The DEFINE_SIMPLE_DEV_PM_OPS itself adds __maybe_unused for !CONFIG_PM
> case, but I don't think it is relevant.

It definitively is relevant here. SIMPLE_DEV_PM_OPS without 
__maybe_unused results in the following pre-processed code:

static int brcmstb_memc_suspend(struct device *dev)
{
  struct brcmstb_memc *memc = dev_get_drvdata(dev);
  void *cfg = memc->ddr_ctrl + memc->srpd_offset;
  u32 val;

  if (memc->timeout_cycles == 0)
   return 0;






  val = ({ u32 __r = (( __u32)(__le32)(( __le32) __raw_readl(cfg))); 
__r; });
  val &= ~((((1UL))) << (16));
  __raw_writel(( u32) (( __le32)(__u32)(val)),cfg);

  (void)({ u32 __r = (( __u32)(__le32)(( __le32) __raw_readl(cfg))); 
__r; });

  return 0;
}

static int brcmstb_memc_resume(struct device *dev)
{
  struct brcmstb_memc *memc = dev_get_drvdata(dev);

  if (memc->timeout_cycles == 0)
   return 0;

  return brcmstb_memc_srpd_config(memc, memc->timeout_cycles);
}

static const struct dev_pm_ops __attribute__((__unused__)) 
brcmstb_memc_pm_ops = { }
                         ;

static struct platform_driver brcmstb_memc_driver = {
  .probe = brcmstb_memc_probe,
  .remove = brcmstb_memc_remove,
  .driver = {
   .name = "brcmstb_memc",
   .of_match_table = brcmstb_memc_of_match,
   .pm = ((1) ? ((&brcmstb_memc_pm_ops)) : ((void *)0)),
  },
};

Now with DEFINE_SIMPLE_PM_OPS, we get the following instead:

static const struct dev_pm_ops brcmstb_memc_pm_ops = { .suspend = ((0) ? 
((brcmstb_memc_suspend)) : ((void *)0)), .resume = ((0) ? 
((brcmstb_memc_resume)) : ((void *)0)), .freeze = ((0) ? 
((brcmstb_memc_suspend)) : ((void *)0)), .thaw = ((0) ? 
((brcmstb_memc_resume)) : ((void *)0)), .poweroff = ((0) ? 
((brcmstb_memc_suspend)) : ((void *)0)), .restore = ((0) ? 
((brcmstb_memc_resume)) : ((void *)0)), .runtime_suspend = ((void *)0), 
.runtime_resume = ((void *)0), .runtime_idle = ((void *)0), }
                         ;

static struct platform_driver brcmstb_memc_driver = {
  .probe = brcmstb_memc_probe,
  .remove = brcmstb_memc_remove,
  .driver = {
   .name = "brcmstb_memc",
   .of_match_table = brcmstb_memc_of_match,
   .pm = ((1) ? ((&brcmstb_memc_pm_ops)) : ((void *)0)),
  },
};

so we will continue to reference the functions, but eventually we will 
eliminate those at the optimization stage by figuring out that this is 
dead code, therefore it can be eliminated. Note that in both cases the 
object does not contain brcmstb_memc_{resume,suspend} which makes sense 
because the warnings are generated at the time the code is processed, 
and the dead code elimination at one of the optimization stages.

I can spin a v5 with DEFINE_SIMPLE_PM_OPS if you prefer to get rid of 
the __maybe_unused.