[PATCH v2 0/2] SRAM dt binding and fix

[PATCH v2 0/2] SRAM dt binding and fix

[Linus Walleij]

This adds a DT binding for the U8500 SRAMs.

This patch series also fixes an issue with pool labels that I
saw on U8500.

I suppose SRAM patches will go in through the SoC tree, I kind
of feel that SRAM is a SoC concept and the driver should
actually be in drivers/soc...

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes in v2:
- Change to just use dev_name() for naming the SRAM partition
  when no label is passed.
- Link to v1: https://lore.kernel.org/r/20230417-ux500-sram-v1-0-5924988bb835@linaro.org

---
Linus Walleij (2):
      dt-bindings: sram: Add compatible for ST-Ericsson U8500 eSRAM
      misc: sram: Generate unique names for subpools

 Documentation/devicetree/bindings/sram/sram.yaml | 1 +
 drivers/misc/sram.c                              | 9 +++++----
 2 files changed, 6 insertions(+), 4 deletions(-)
---
base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
change-id: 20230417-ux500-sram-961796726db4

Best regards,
-- 
Linus Walleij <linus.walleij@linaro.org>

[PATCH v2 1/2] dt-bindings: sram: Add compatible for ST-Ericsson U8500 eSRAM

[Linus Walleij]

This adds an SoC-specific binding for the banks of eSRAM
available in the ST-Ericsson U8500.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 Documentation/devicetree/bindings/sram/sram.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
index 993430be355b..0922d1f71ba8 100644
--- a/Documentation/devicetree/bindings/sram/sram.yaml
+++ b/Documentation/devicetree/bindings/sram/sram.yaml
@@ -94,6 +94,7 @@ patternProperties:
             - samsung,exynos4210-sysram
             - samsung,exynos4210-sysram-ns
             - socionext,milbeaut-smp-sram
+            - stericsson,u8500-esram
 
       reg:
         description:

-- 
2.40.0

[PATCH v2 2/2] misc: sram: Generate unique names for subpools

[Linus Walleij]

The current code will, if we do not specify unique labels
for the SRAM subnodes, fail to register several nodes named
the same.

Example:

sram@40020000 {
  (...)
  sram@0 {
    (...)
  };
  sram@1000 {
    (...)
  };
};

Since the child->name in both cases will be "sram" the
gen_pool_create() will fail because the name is not unique.

Use dev_name() for the device as this will have bus ID
set to the fully translated address for the node, and that
will always be unique.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Stop complicating things and just use dev_name()
---
 drivers/misc/sram.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index f0e7f02605eb..f80c3adddf0b 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -240,10 +240,11 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
 				goto err_chunks;
 			}
 			if (!label)
-				label = child->name;
-
-			block->label = devm_kstrdup(sram->dev,
-						    label, GFP_KERNEL);
+				block->label = devm_kasprintf(sram->dev, GFP_KERNEL,
+							      "%s", dev_name(sram->dev));
+			else
+				block->label = devm_kstrdup(sram->dev,
+							    label, GFP_KERNEL);
 			if (!block->label) {
 				ret = -ENOMEM;
 				goto err_chunks;

-- 
2.40.0

Re: [PATCH v2 2/2] misc: sram: Generate unique names for subpools

[Dmitry Osipenko]

21.04.2023 00:17, Linus Walleij пишет:
> The current code will, if we do not specify unique labels
> for the SRAM subnodes, fail to register several nodes named
> the same.
> 
> Example:
> 
> sram@40020000 {
>   (...)
>   sram@0 {
>     (...)
>   };
>   sram@1000 {
>     (...)
>   };
> };
> 
> Since the child->name in both cases will be "sram" the
> gen_pool_create() will fail because the name is not unique.
> 
> Use dev_name() for the device as this will have bus ID
> set to the fully translated address for the node, and that
> will always be unique.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Stop complicating things and just use dev_name()
> ---
>  drivers/misc/sram.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> index f0e7f02605eb..f80c3adddf0b 100644
> --- a/drivers/misc/sram.c
> +++ b/drivers/misc/sram.c
> @@ -240,10 +240,11 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
>  				goto err_chunks;
>  			}
>  			if (!label)
> -				label = child->name;
> -
> -			block->label = devm_kstrdup(sram->dev,
> -						    label, GFP_KERNEL);
> +				block->label = devm_kasprintf(sram->dev, GFP_KERNEL,
> +							      "%s", dev_name(sram->dev));

This broke device-trees that have no label property. The SRAM DT binding
says:

"
label:
description:
	The name for the reserved partition, if omitted, the label is taken
	from the node name excluding the unit address.
"

Not sure whether breakage was on purpose, otherwise doc needs to be
updated or there should be explicit check for the duplicated node names.

Secondly, AFAICS, the dev_name(sram->dev) is the name of the parent SRAM
device and not of the children sub-nodes, hence it's now always the same
dev_name(sram->dev) for all sub-nodes.

Re: [PATCH v2 2/2] misc: sram: Generate unique names for subpools

[Linus Walleij]

On Sun, Jun 18, 2023 at 11:33 PM Dmitry Osipenko <digetx@gmail.com> wrote:

> >                       if (!label)
> > -                             label = child->name;
> > -
> > -                     block->label = devm_kstrdup(sram->dev,
> > -                                                 label, GFP_KERNEL);
> > +                             block->label = devm_kasprintf(sram->dev, GFP_KERNEL,
> > +                                                           "%s", dev_name(sram->dev));
>
> This broke device-trees that have no label property.

Which system is affected? Asking so I can inspect the DTS file
and figure out how this needs to work.

>  The SRAM DT binding says:
>
> "
> label:
> description:
>         The name for the reserved partition, if omitted, the label is taken
>         from the node name excluding the unit address.
> "
>
> Not sure whether breakage was on purpose, otherwise doc needs to be
> updated or there should be explicit check for the duplicated node names.
>
> Secondly, AFAICS, the dev_name(sram->dev) is the name of the parent SRAM
> device and not of the children sub-nodes, hence it's now always the same
> dev_name(sram->dev) for all sub-nodes.

Sounds like I should go back to the original approach in patch v1:
https://lore.kernel.org/linux-devicetree/20230417-ux500-sram-v1-2-5924988bb835@linaro.org/

and also augment the DTS binding text to say it uses the full node name
including the address.

Does that look OK to you, or will this regress your system as well?

Yours,
Linus Walleij

Re: [PATCH v2 2/2] misc: sram: Generate unique names for subpools

[Dmitry Osipenko]

19.06.2023 10:11, Linus Walleij пишет:
> On Sun, Jun 18, 2023 at 11:33 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> 
>>>                       if (!label)
>>> -                             label = child->name;
>>> -
>>> -                     block->label = devm_kstrdup(sram->dev,
>>> -                                                 label, GFP_KERNEL);
>>> +                             block->label = devm_kasprintf(sram->dev, GFP_KERNEL,
>>> +                                                           "%s", dev_name(sram->dev));
>>
>> This broke device-trees that have no label property.
> 
> Which system is affected? Asking so I can inspect the DTS file
> and figure out how this needs to work.

NVIDIA Tegra2/3 video decoder driver fails to probe with this change.

https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/nvidia/tegra-vde/vde.c#L312

https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/tegra20.dtsi#L347

>>  The SRAM DT binding says:
>>
>> "
>> label:
>> description:
>>         The name for the reserved partition, if omitted, the label is taken
>>         from the node name excluding the unit address.
>> "
>>
>> Not sure whether breakage was on purpose, otherwise doc needs to be
>> updated or there should be explicit check for the duplicated node names.
>>
>> Secondly, AFAICS, the dev_name(sram->dev) is the name of the parent SRAM
>> device and not of the children sub-nodes, hence it's now always the same
>> dev_name(sram->dev) for all sub-nodes.
> 
> Sounds like I should go back to the original approach in patch v1:
> https://lore.kernel.org/linux-devicetree/20230417-ux500-sram-v1-2-5924988bb835@linaro.org/
> 
> and also augment the DTS binding text to say it uses the full node name
> including the address.
> 
> Does that look OK to you, or will this regress your system as well?

That may work, but then seems you'll also need to update
of_gen_pool_get() to use np_pool->full_name instead of np_pool->name.

https://elixir.bootlin.com/linux/latest/source/lib/genalloc.c#L898