[PATCH v2 0/3] Exynos5 DMC minor fixes

[PATCH v2 0/3] Exynos5 DMC minor fixes

[Lukasz Luba]

Hi all,

This is a follow up patch set for the Exynos5 Dynamic Memory Controller
driver v13 [1]. The patches are based on Krzysztof's 'for-next' branch [2].
There are a few minor fixes captured during static analysis and a new
binding for 'samsung,K3QF2F20DB' LPDDR3 memory.

Regards,
Lukasz Luba

[1] https://lkml.org/lkml/2019/8/21/283
[2] https://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux.git/log/?h=for-next

Lukasz Luba (3):
  memory: Exynos5422: minor fixes in DMC
  ARM: dts: exynos: fix too long line in memory device
  dt-bindings: ddr: Add bindings for Samsung LPDDR3 memories

 Documentation/devicetree/bindings/ddr/lpddr3.txt | 9 ++++++---
 arch/arm/boot/dts/exynos5422-odroid-core.dtsi    | 3 ++-
 drivers/memory/samsung/exynos5422-dmc.c          | 4 ++--
 3 files changed, 10 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH v2 1/3] memory: Exynos5422: minor fixes in DMC

[Lukasz Luba]

Small fixes for issues captured by static analyzes:
used kfree() insead of devm_kfree() and missing 'static' in the private
function.
Checks which show the issues:
- drivers/memory/samsung/exynos5422-dmc.c:272 exynos5_init_freq_table()
warn: passing devm_ allocated variable to kfree. 'dmc->opp'
- drivers/memory/samsung/exynos5422-dmc.c:736:1: warning: symbol
'exynos5_dmc_align_init_freq' was not declared.

Reported-by: Krzysztof Kozlowski <krzk@kernel.org>
Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
---
 drivers/memory/samsung/exynos5422-dmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/memory/samsung/exynos5422-dmc.c b/drivers/memory/samsung/exynos5422-dmc.c
index 8c2ec29a7d57..a809fa997c03 100644
--- a/drivers/memory/samsung/exynos5422-dmc.c
+++ b/drivers/memory/samsung/exynos5422-dmc.c
@@ -269,7 +269,7 @@ static int exynos5_init_freq_table(struct exynos5_dmc *dmc,
 	return 0;
 
 err_free_tables:
-	kfree(dmc->opp);
+	devm_kfree(dmc->dev, dmc->opp);
 err_opp:
 	dev_pm_opp_of_remove_table(dmc->dev);
 
@@ -732,7 +732,7 @@ static struct devfreq_dev_profile exynos5_dmc_df_profile = {
  * statistics engine which supports only registered values. Thus, some alignment
  * must be made.
  */
-unsigned long
+static unsigned long
 exynos5_dmc_align_init_freq(struct exynos5_dmc *dmc,
 			    unsigned long bootloader_init_freq)
 {
-- 
2.17.1

Re: [PATCH v2 1/3] memory: Exynos5422: minor fixes in DMC

[Krzysztof Kozlowski]

On Mon, Sep 16, 2019 at 12:07:02PM +0200, Lukasz Luba wrote:
> Small fixes for issues captured by static analyzes:
> used kfree() insead of devm_kfree() and missing 'static' in the private
> function.
> Checks which show the issues:
> - drivers/memory/samsung/exynos5422-dmc.c:272 exynos5_init_freq_table()
> warn: passing devm_ allocated variable to kfree. 'dmc->opp'
> - drivers/memory/samsung/exynos5422-dmc.c:736:1: warning: symbol
> 'exynos5_dmc_align_init_freq' was not declared.
> 
> Reported-by: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  drivers/memory/samsung/exynos5422-dmc.c | 4 ++--

Thanks, applied.

Best regards,
Krzysztof

[PATCH v2 2/3] ARM: dts: exynos: fix too long line in memory device

[Lukasz Luba]

Small fix moving the comment to line above making sure the lines do not
exceed 80 characters.

Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
---
 arch/arm/boot/dts/exynos5422-odroid-core.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
index fe885ca969af..059fa32d1a8f 100644
--- a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
+++ b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
@@ -103,7 +103,8 @@
 
 		timings_samsung_K3QF2F20DB_800mhz: lpddr3-timings@800000000 {
 			compatible	= "jedec,lpddr3-timings";
-			reg		= <800000000>; /* workaround: it shows max-freq */
+			/* workaround: 'reg' shows max-freq */
+			reg		= <800000000>;
 			min-freq	= <100000000>;
 			tRFC		= <65000>;
 			tRRD		= <6000>;
-- 
2.17.1

Re: [PATCH v2 2/3] ARM: dts: exynos: fix too long line in memory device

[Krzysztof Kozlowski]

On Mon, Sep 16, 2019 at 12:07:03PM +0200, Lukasz Luba wrote:
> Small fix moving the comment to line above making sure the lines do not
> exceed 80 characters.
> 
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  arch/arm/boot/dts/exynos5422-odroid-core.dtsi | 3 ++-

Thanks, applied (squashed with previous).

Best regards,
Krzysztof

[PATCH v2 3/3] dt-bindings: ddr: Add bindings for Samsung LPDDR3 memories

[Lukasz Luba]

Add compatible for Samsung k3qf2f20db LPDDR3 memory bindings.
Introduce minor fixes in the old documentation.

Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
---
 Documentation/devicetree/bindings/ddr/lpddr3.txt | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/ddr/lpddr3.txt b/Documentation/devicetree/bindings/ddr/lpddr3.txt
index 3b2485b84b3f..49afe794daaa 100644
--- a/Documentation/devicetree/bindings/ddr/lpddr3.txt
+++ b/Documentation/devicetree/bindings/ddr/lpddr3.txt
@@ -1,7 +1,9 @@
 * LPDDR3 SDRAM memories compliant to JEDEC JESD209-3C
 
 Required properties:
-- compatible : Should be  - "jedec,lpddr3"
+- compatible : should be one of the following:
+	Generic default - "jedec,lpddr3".
+	For Samsung 542x SoC - "samsung,K3QF2F20DB", "jedec,lpddr3".
 - density  : <u32> representing density in Mb (Mega bits)
 - io-width : <u32> representing bus width. Possible values are 8, 16, 32, 64
 - #address-cells: Must be set to 1
@@ -43,7 +45,7 @@ Child nodes:
 Example:
 
 samsung_K3QF2F20DB: lpddr3 {
-	compatible	= "Samsung,K3QF2F20DB", "jedec,lpddr3";
+	compatible	= "samsung,K3QF2F20DB", "jedec,lpddr3";
 	density		= <16384>;
 	io-width	= <32>;
 	#address-cells	= <1>;
@@ -73,7 +75,8 @@ samsung_K3QF2F20DB: lpddr3 {
 
 	timings_samsung_K3QF2F20DB_800mhz: lpddr3-timings@800000000 {
 		compatible	= "jedec,lpddr3-timings";
-		reg		= <800000000>; /* workaround: it shows max-freq */
+		/* workaround: 'reg' shows max-freq */
+		reg		= <800000000>;
 		min-freq	= <100000000>;
 		tRFC		= <65000>;
 		tRRD		= <6000>;
-- 
2.17.1

Re: [PATCH v2 3/3] dt-bindings: ddr: Add bindings for Samsung LPDDR3 memories

[Krzysztof Kozlowski]

On Mon, 16 Sep 2019 at 12:07, Lukasz Luba <l.luba@partner.samsung.com> wrote:
>
> Add compatible for Samsung k3qf2f20db LPDDR3 memory bindings.
> Introduce minor fixes in the old documentation.
>
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  Documentation/devicetree/bindings/ddr/lpddr3.txt | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/ddr/lpddr3.txt b/Documentation/devicetree/bindings/ddr/lpddr3.txt
> index 3b2485b84b3f..49afe794daaa 100644
> --- a/Documentation/devicetree/bindings/ddr/lpddr3.txt
> +++ b/Documentation/devicetree/bindings/ddr/lpddr3.txt
> @@ -1,7 +1,9 @@
>  * LPDDR3 SDRAM memories compliant to JEDEC JESD209-3C
>
>  Required properties:
> -- compatible : Should be  - "jedec,lpddr3"
> +- compatible : should be one of the following:
> +       Generic default - "jedec,lpddr3".

The convention is first compatible, then description. I gave you the
example to base on - at25. Why making it different?

Best regards,
Krzysztof

Re: [PATCH v2 3/3] dt-bindings: ddr: Add bindings for Samsung LPDDR3 memories

[Lukasz Luba]

Hi Krzysztof,

On 9/18/19 8:51 PM, Krzysztof Kozlowski wrote:
> On Mon, 16 Sep 2019 at 12:07, Lukasz Luba <l.luba@partner.samsung.com> wrote:
>>
>> Add compatible for Samsung k3qf2f20db LPDDR3 memory bindings.
>> Introduce minor fixes in the old documentation.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>   Documentation/devicetree/bindings/ddr/lpddr3.txt | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/ddr/lpddr3.txt b/Documentation/devicetree/bindings/ddr/lpddr3.txt
>> index 3b2485b84b3f..49afe794daaa 100644
>> --- a/Documentation/devicetree/bindings/ddr/lpddr3.txt
>> +++ b/Documentation/devicetree/bindings/ddr/lpddr3.txt
>> @@ -1,7 +1,9 @@
>>   * LPDDR3 SDRAM memories compliant to JEDEC JESD209-3C
>>
>>   Required properties:
>> -- compatible : Should be  - "jedec,lpddr3"
>> +- compatible : should be one of the following:
>> +       Generic default - "jedec,lpddr3".
> 
> The convention is first compatible, then description. I gave you the
> example to base on - at25. Why making it different?

I have checked at25 that you pointed me to and also checked at24, which
has a bit longer "compatible" section.

I found that there are many "jedec,spi-nor" compatible devices, which I
thought would be a better example for my "jedec,lpddr3".
For example, two configurations, where you have a single labels or dual
(with specific device)
arch/arm/boot/dts/imx6dl-rex-basic.dts:
compatible = "sst,sst25vf016b", "jedec,spi-nor";
arch/arm/boot/dts/imx6q-ba16.dtsi:
compatible = "jedec,spi-nor";

The 'compatible' in documentation for the "jedec,spi-nor" is slightly
different (similar to at24).
Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
It has a long explanation, which is also OK. So I thought that it is
quite flexible what you put in there.

I have also checked Cadance QSPI controller.
Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
The controller might be built-in into different vendor SoC's
and the "compatible" is ready to reflect it in similar fashion but
with a short explanation in this section.

Therefore, what you see in the patch draw heavily on Cadence's qspi,
with a bit of inspiration from jedec,spi-nor usage.

Should I change it to at25 "compatible" style and send next patch?

PS. Thank you for taking the other two patches. I will not respond in
their threads to keep the traffic low.

Regards,
Lukasz

Re: [PATCH v2 3/3] dt-bindings: ddr: Add bindings for Samsung LPDDR3 memories

[Krzysztof Kozlowski]

On Thu, 19 Sep 2019 at 08:49, Lukasz Luba <l.luba@partner.samsung.com> wrote:
>
> Hi Krzysztof,
>
> On 9/18/19 8:51 PM, Krzysztof Kozlowski wrote:
> > On Mon, 16 Sep 2019 at 12:07, Lukasz Luba <l.luba@partner.samsung.com> wrote:
> >>
> >> Add compatible for Samsung k3qf2f20db LPDDR3 memory bindings.
> >> Introduce minor fixes in the old documentation.
> >>
> >> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> >> ---
> >>   Documentation/devicetree/bindings/ddr/lpddr3.txt | 9 ++++++---
> >>   1 file changed, 6 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/ddr/lpddr3.txt b/Documentation/devicetree/bindings/ddr/lpddr3.txt
> >> index 3b2485b84b3f..49afe794daaa 100644
> >> --- a/Documentation/devicetree/bindings/ddr/lpddr3.txt
> >> +++ b/Documentation/devicetree/bindings/ddr/lpddr3.txt
> >> @@ -1,7 +1,9 @@
> >>   * LPDDR3 SDRAM memories compliant to JEDEC JESD209-3C
> >>
> >>   Required properties:
> >> -- compatible : Should be  - "jedec,lpddr3"
> >> +- compatible : should be one of the following:
> >> +       Generic default - "jedec,lpddr3".
> >
> > The convention is first compatible, then description. I gave you the
> > example to base on - at25. Why making it different?
>
> I have checked at25 that you pointed me to and also checked at24, which
> has a bit longer "compatible" section.
>
> I found that there are many "jedec,spi-nor" compatible devices, which I
> thought would be a better example for my "jedec,lpddr3".
> For example, two configurations, where you have a single labels or dual
> (with specific device)
> arch/arm/boot/dts/imx6dl-rex-basic.dts:
> compatible = "sst,sst25vf016b", "jedec,spi-nor";
> arch/arm/boot/dts/imx6q-ba16.dtsi:
> compatible = "jedec,spi-nor";
>
> The 'compatible' in documentation for the "jedec,spi-nor" is slightly
> different (similar to at24).
> Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
> It has a long explanation, which is also OK. So I thought that it is
> quite flexible what you put in there.

It is flexible but I see clear pattern in existing sources:
  jedec,spi-nor.txt
  compatible : May include a device-specific ..
  ...
  Supported chip names:
    at25df321a
    ...

  at25.txt:
  - compatible : Should be "<vendor>,<type>", and generic value "atmel,at25".
    Example "<vendor>,<type>" values:
      "anvo,anv32e61w"
      "microchip,25lc040"

In these cases the doc says that "compatible should be" and then you
have the list of values. Your example says that the compatible should
be "Generic default" or "For Samsung 542x SoC"... :) The difference is
slight but putting the value first is a simple and elegant solution.
In your case one has to go to the end of sentence to find the most
important information - the compatible value.

> I have also checked Cadance QSPI controller.
> Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
> The controller might be built-in into different vendor SoC's
> and the "compatible" is ready to reflect it in similar fashion but
> with a short explanation in this section.

I see. I do not find this pattern as much readable as jedec-spi-nor or
at25 therefore I mentioned them as an example to base on ("Exactly the
same as AT24 or AT25 EEPROM bindings."). We can avoid also this entire
discussion with YAML (which also follows approach of at25 - value
first).

> Therefore, what you see in the patch draw heavily on Cadence's qspi,
> with a bit of inspiration from jedec,spi-nor usage.
>
> Should I change it to at25 "compatible" style and send next patch?

Yes, please. Or go to YAML and make entire discussion obsolete.

Best regards,
Krzysztof

Re: [PATCH v2 3/3] dt-bindings: ddr: Add bindings for Samsung LPDDR3 memories

[Lukasz Luba]

On 9/19/19 9:28 AM, Krzysztof Kozlowski wrote:
> On Thu, 19 Sep 2019 at 08:49, Lukasz Luba <l.luba@partner.samsung.com> wrote:
>>
>> Hi Krzysztof,
>>
>> On 9/18/19 8:51 PM, Krzysztof Kozlowski wrote:
>>> On Mon, 16 Sep 2019 at 12:07, Lukasz Luba <l.luba@partner.samsung.com> wrote:
>>>>
>>>> Add compatible for Samsung k3qf2f20db LPDDR3 memory bindings.
>>>> Introduce minor fixes in the old documentation.
>>>>
>>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/ddr/lpddr3.txt | 9 ++++++---
>>>>    1 file changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/ddr/lpddr3.txt b/Documentation/devicetree/bindings/ddr/lpddr3.txt
>>>> index 3b2485b84b3f..49afe794daaa 100644
>>>> --- a/Documentation/devicetree/bindings/ddr/lpddr3.txt
>>>> +++ b/Documentation/devicetree/bindings/ddr/lpddr3.txt
>>>> @@ -1,7 +1,9 @@
>>>>    * LPDDR3 SDRAM memories compliant to JEDEC JESD209-3C
>>>>
>>>>    Required properties:
>>>> -- compatible : Should be  - "jedec,lpddr3"
>>>> +- compatible : should be one of the following:
>>>> +       Generic default - "jedec,lpddr3".
>>>
>>> The convention is first compatible, then description. I gave you the
>>> example to base on - at25. Why making it different?
>>
>> I have checked at25 that you pointed me to and also checked at24, which
>> has a bit longer "compatible" section.
>>
>> I found that there are many "jedec,spi-nor" compatible devices, which I
>> thought would be a better example for my "jedec,lpddr3".
>> For example, two configurations, where you have a single labels or dual
>> (with specific device)
>> arch/arm/boot/dts/imx6dl-rex-basic.dts:
>> compatible = "sst,sst25vf016b", "jedec,spi-nor";
>> arch/arm/boot/dts/imx6q-ba16.dtsi:
>> compatible = "jedec,spi-nor";
>>
>> The 'compatible' in documentation for the "jedec,spi-nor" is slightly
>> different (similar to at24).
>> Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
>> It has a long explanation, which is also OK. So I thought that it is
>> quite flexible what you put in there.
> 
> It is flexible but I see clear pattern in existing sources:
>    jedec,spi-nor.txt
>    compatible : May include a device-specific ..
>    ...
>    Supported chip names:
>      at25df321a
>      ...
> 
>    at25.txt:
>    - compatible : Should be "<vendor>,<type>", and generic value "atmel,at25".
>      Example "<vendor>,<type>" values:
>        "anvo,anv32e61w"
>        "microchip,25lc040"
> 
> In these cases the doc says that "compatible should be" and then you
> have the list of values. Your example says that the compatible should
> be "Generic default" or "For Samsung 542x SoC"... :) The difference is
> slight but putting the value first is a simple and elegant solution.
> In your case one has to go to the end of sentence to find the most
> important information - the compatible value.
> 
>> I have also checked Cadance QSPI controller.
>> Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
>> The controller might be built-in into different vendor SoC's
>> and the "compatible" is ready to reflect it in similar fashion but
>> with a short explanation in this section.
> 
> I see. I do not find this pattern as much readable as jedec-spi-nor or
> at25 therefore I mentioned them as an example to base on ("Exactly the
> same as AT24 or AT25 EEPROM bindings."). We can avoid also this entire
> discussion with YAML (which also follows approach of at25 - value
> first).
> 
>> Therefore, what you see in the patch draw heavily on Cadence's qspi,
>> with a bit of inspiration from jedec,spi-nor usage.
>>
>> Should I change it to at25 "compatible" style and send next patch?
> 
> Yes, please. Or go to YAML and make entire discussion obsolete.

OK I will change it to at25 style.

Regards,
Lukasz