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

Krzysztof Kozlowski
2019-06-14T13:46:28Z

On Fri, 14 Jun 2019 at 15:41, Lukasz Luba <l.luba@partner.samsung.com> 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@partner.samsung.com> 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@partner.samsung.com>
> >> ---
> >> 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@partner.samsung.com>
> >> +L: linux-pm@vger.kernel.org
> >> +L: linux-samsung-soc@vger.kernel.org
> >> +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@gonehiking.org>
> >> L: linux-scsi@vger.kernel.org
> >> 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