Re: [PATCH v10 08/13] drivers: memory: add DMC driver for Exynos5422

[Krzysztof Kozlowski]

On Fri, 14 Jun 2019 at 15:41, Lukasz Luba <l.luba@xxxxxxxxxxxxxxxxxxx> wrote:
>
> 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)

I like the third option... although I would be also happy to see
COMPILE_TEST (probably with IOMEM). You need to check which
dependencies are still necessary for COMPILE_TEST.

Best regards,
Krzysztof