Hi Krzysztof,
On 6/14/19 2:58 PM, Krzysztof Kozlowski wrote:
> On Fri, 14 Jun 2019 at 11:53, Lukasz Luba <l.luba@xxxxxxxxxxxxxxxxxxx> wrote:
>>
>> This patch adds driver for Exynos5422 Dynamic Memory Controller.
>> The driver provides support for dynamic frequency and voltage scaling for
>> DMC and DRAM. It supports changing timings of DRAM running with different
>> frequency. There is also an algorithm to calculate timigns based on
>> memory description provided in DT.
>> The patch also contains needed MAINTAINERS file update.
>>
>> Signed-off-by: Lukasz Luba <l.luba@xxxxxxxxxxxxxxxxxxx>
>> ---
>> MAINTAINERS | 8 +
>> drivers/memory/samsung/Kconfig | 17 +
>> drivers/memory/samsung/Makefile | 1 +
>> drivers/memory/samsung/exynos5422-dmc.c | 1262 +++++++++++++++++++++++
>> 4 files changed, 1288 insertions(+)
>> create mode 100644 drivers/memory/samsung/exynos5422-dmc.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 57f496cff999..6ffccfd95351 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -3470,6 +3470,14 @@ S: Maintained
>> F: drivers/devfreq/exynos-bus.c
>> F: Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>>
>> +DMC FREQUENCY DRIVER FOR SAMSUNG EXYNOS5422
>
> Eh, more comments from my side.
> "For the hard of thinking, this list is meant to remain in alphabetical order."
OK
>
>> +M: Lukasz Luba <l.luba@xxxxxxxxxxxxxxxxxxx>
>> +L: linux-pm@xxxxxxxxxxxxxxx
>> +L: linux-samsung-soc@xxxxxxxxxxxxxxx
>> +S: Maintained
>> +F: drivers/memory/samsung/exynos5422-dmc.c
>> +F: Documentation/devicetree/bindings/memory-controllers/exynos5422-dmc.txt
>> +
>> BUSLOGIC SCSI DRIVER
>> M: Khalid Aziz <khalid@xxxxxxxxxxxxxx>
>> L: linux-scsi@xxxxxxxxxxxxxxx
>> diff --git a/drivers/memory/samsung/Kconfig b/drivers/memory/samsung/Kconfig
>> index 79ce7ea58903..c93baa029654 100644
>> --- a/drivers/memory/samsung/Kconfig
>> +++ b/drivers/memory/samsung/Kconfig
>> @@ -5,6 +5,23 @@ config SAMSUNG_MC
>> Support for the Memory Controller (MC) devices found on
>> Samsung Exynos SoCs.
>>
>> +config ARM_EXYNOS5422_DMC
>
> Why you added prefix of ARM to CONFIG think none of other Exynos
> Kconfig options follow such convention (except devfreq).
In the previous versions the driver was in drivers/devfreq/,
that's why they have this prefix.
>
>> + tristate "ARM EXYNOS5422 Dynamic Memory Controller driver"
>> + depends on ARCH_EXYNOS
>> + select DDR
>
> In general select should be used only for non-visible symbols and DDR
> is visible. Use depends.
OK
>
>> + select PM_DEVFREQ
>
> This definitely cannot be select. You do not select entire subsystem
> because some similar driver was chosen.
So I will use depends int this case
>
>> + select DEVFREQ_GOV_SIMPLE_ONDEMAND
>> + select DEVFREQ_GOV_USERSPACE
>
> I think only simple_ondemand is needed. Is userspace governor
> necessary for working? Or actually maybe both could be skipped?
>
Actually we can skip both governors from here.
>> + select PM_DEVFREQ_EVENT
>
> Again, depends.
OK
There are these 4 options which the driver depends on:
depends on ARCH_EXYNOS
depends on DDR
depends on PM_DEVFREQ
depends on PM_DEVFREQ_EVENT
Should I merged them into one, two lines? like below:
a)
depends on (ARCH_EXYNOS && DDR && PM_DEVFREQ && PM_DEVFREQ_EVENT)
b) grouped into two sets
depends on (ARCH_EXYNOS && DDR)
depends on (PM_DEVFREQ && PM_DEVFREQ_EVENT)
c) grouped by pm_devfreq only
depends on ARCH_EXYNOS
depends on DDR
depends on (PM_DEVFREQ && PM_DEVFREQ_EVENT)
>
>> + select PM_OPP
This option can be used here IIUC
>> + help
>> + This adds driver for Exynos5422 DMC (Dynamic Memory Controller).
>> + The driver provides support for Dynamic Voltage and Frequency Scaling in
>> + DMC and DRAM. It also supports changing timings of DRAM running with
>> + different frequency. The timings are calculated based on DT memory
>> + information.
>> +
>> +
>> if SAMSUNG_MC
>
> Why this is outside of SAMSUNG_MC?
Good question, I will move it below this 'if' statement.
Regards,
Lukasz
>
> Best regards,
> Krzysztof
>
>
