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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJKOXPcsNe+PEszGAS+YyGH3qqQdELXnwhD+x=+9POrZziCOpw@mail.gmail.com>
Date:   Fri, 14 Jun 2019 15:46:11 +0200
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Lukasz Luba <l.luba@...tner.samsung.com>
Cc:     devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org,
        "linux-samsung-soc@...r.kernel.org" 
        <linux-samsung-soc@...r.kernel.org>, linux-clk@...r.kernel.org,
        mturquette@...libre.com, sboyd@...nel.org,
        Bartłomiej Żołnierkiewicz 
        <b.zolnierkie@...sung.com>, kgene@...nel.org,
        Chanwoo Choi <cw00.choi@...sung.com>,
        kyungmin.park@...sung.com,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        s.nawrocki@...sung.com, myungjoo.ham@...sung.com,
        keescook@...omium.org, tony@...mide.com, jroedel@...e.de,
        treding@...dia.com, digetx@...il.com, gregkh@...uxfoundation.org,
        willy.mh.wolff.ml@...il.com
Subject: Re: [PATCH v10 08/13] drivers: memory: add DMC driver for Exynos5422

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