All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kevin Hilman <khilman@baylibre.com>
To: Rob Herring <robh@kernel.org>
Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
	yixun.lan@amlogic.com, linux-mtd@lists.infradead.org,
	boris.brezillon@bootlin.com, richard@nod.at,
	Neil Armstrong <narmstrong@baylibre.com>,
	linux-kernel@vger.kernel.org, marek.vasut@gmail.com,
	devicetree@vger.kernel.org, liang.yang@amlogic.com,
	jian.hu@amlogic.com, carlo@caione.org,
	linux-amlogic@lists.infradead.org, computersforpeace@gmail.com,
	dwmw2@infradead.org, linux-arm-kernel@lists.infradead.org,
	jbrunet@baylibre.com
Subject: Re: [PATCH 1/2] dt-bindings: nand: meson: add Amlogic NAND controller driver
Date: Wed, 27 Jun 2018 16:40:45 -0700	[thread overview]
Message-ID: <7hvaa3hk76.fsf@baylibre.com> (raw)
In-Reply-To: <20180626183003.GA3019@rob-hp-laptop> (Rob Herring's message of "Tue, 26 Jun 2018 12:30:03 -0600")

Rob Herring <robh@kernel.org> writes:

> On Sun, Jun 24, 2018 at 12:46:59AM +0200, Martin Blumenstingl wrote:
>> Hello Yixun, Hello Liang,
>> 
>> I have a few small comments inline below
>> additionally I tried to explain the reason behind
>> "amlogic,mmc-syscon", clkin0 and clkin1 so Rob (or the devicetree
>> maintainers in general) can give feedback. feel free to correct me
>> wherever I'm wrong or provide additional notes in case I missed
>> something!
>> 
>> On Wed, Jun 13, 2018 at 10:17 AM Yixun Lan <yixun.lan@amlogic.com> wrote:
>> >
>> > From: Liang Yang <liang.yang@amlogic.com>
>> >
>> > Add Amlogic NAND controller dt-bindings for Meson SoC,
>> > Current this driver support GXBB/GXL/AXG platform.
>> >
>> > Signed-off-by: Liang Yang <liang.yang@amlogic.com>
>> > Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
>> > ---
>> >  .../bindings/mtd/amlogic,meson-nand.txt       | 118 ++++++++++++++++++
>> >  1 file changed, 118 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>> > new file mode 100644
>> > index 000000000000..eac9f9433d5d
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>> > @@ -0,0 +1,118 @@
>> > +Amlogic NAND Flash Controller (NFC) for GXBB/GXL/AXG family SoCs
>> > +
>> > +This file documents the properties in addition to those available in
>> > +the MTD NAND bindings.
>> > +
>> > +Required properties:
>> > +- compatible : contains one of:
>> > +  - "amlogic,meson-gxl-nfc"
>> > +  - "amlogic,meson-axg-nfc"
>> the patch description states that GXBB/GXL/AXG are supported
>> shouldn't you add a compatible string for GXBB as well?
>> 
>> > +- clocks     :
>> > +       A list of phandle + clock-specifier pairs for the clocks listed
>> > +       in clock-names.
>> > +
>> > +- clock-names: Should contain the following:
>> > +       "core" - NFC module gate clock
>> > +       "clkin0" - Parent clock of internal mux
>> > +       "clkin1" - Other parent clock of internal mux
>> to give the devicetree maintainers some context on clkin0 and clkin1:
>> 
>> older SoCs (Meson8, Meson8b - not supported by this binding/driver
>> yet) had a dedicated NAND clock. there neither clkin0 or clkin1 would
>> be used, instead we just had a "nand" or "interface" clock (I'm not
>> aware of the actual naming in Amlogic's internal datasheets)
>> 
>> newer SoCs do NOT have a dedicated NAND "interface" clock anymore.
>> instead they are sharing the clock with the "sd_emmc_c" controller (I
>> *believe* the reason for this is because sd_emmc_c and the NAND
>> controller use the same pads on the SoC, pinctrl muxing controls where
>> these pads are routed -> NAND and sd_emmc_c cannot be used at the same
>> time, so SoC designers probably decided to re-use the clock)
>> 
>> unfortunately the sd_emmc_c clock is not provided by the "main" clock
>> controller on these newer SoCs
>> instead the clock is part of the MMC controller's register space (see
>> the SD_EMMC_CLOCK register in drivers/mmc/host/meson-gx-mmc.c)
>> even worse: the SD_EMMC_CLOCK contains more than just clock settings
>> (bit 25 enables the SDIO interrupt, which is currently not supported
>> by the meson-gx-mmc driver though)
>> 
>> the SD_EMMC_CLOCK register has a mux (CLK_SRC_MASK) to choose from
>> clkin0 and clkin1 which are passed here
>> the "amlogic,mmc-syscon" property is used to get a phandle to the
>> sd_emmc_c syscon register space
>> thus there is a bit of code duplication in the MMC and NAND drivers
>> with this binding (because both need to configure the SD_EMMC_CLOCK
>> register)
>
> Well, that's ugly. Really, the SD controller should be modeled as a 
> clock provider. But then you would have to always have a driver 
> instantiated for it. Maybe you need that anyway if accessing this 
> register is dependent on some other clock or reset to the module being 
> enabled (which you may not hit if you only access the reg during boot)?

On some earlier rounds of off-lis review, we did consider making the
SD/MMC controller a clock provider.  But forcing it to be instantiated
was kinda ugly too, especailly because there are several instances of
the MMC IP, and only one of them shares the clock with the NAND, so only
one of them needs to be a clock provider. :(

> But if you really want to do it this way, I guess that is fine.

Thanks.  We did consider a few options, and found this one to be the
least worst of a few options.

Kevin

WARNING: multiple messages have this Message-ID (diff)
From: khilman@baylibre.com (Kevin Hilman)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/2] dt-bindings: nand: meson: add Amlogic NAND controller driver
Date: Wed, 27 Jun 2018 16:40:45 -0700	[thread overview]
Message-ID: <7hvaa3hk76.fsf@baylibre.com> (raw)
In-Reply-To: <20180626183003.GA3019@rob-hp-laptop> (Rob Herring's message of "Tue, 26 Jun 2018 12:30:03 -0600")

Rob Herring <robh@kernel.org> writes:

> On Sun, Jun 24, 2018 at 12:46:59AM +0200, Martin Blumenstingl wrote:
>> Hello Yixun, Hello Liang,
>> 
>> I have a few small comments inline below
>> additionally I tried to explain the reason behind
>> "amlogic,mmc-syscon", clkin0 and clkin1 so Rob (or the devicetree
>> maintainers in general) can give feedback. feel free to correct me
>> wherever I'm wrong or provide additional notes in case I missed
>> something!
>> 
>> On Wed, Jun 13, 2018 at 10:17 AM Yixun Lan <yixun.lan@amlogic.com> wrote:
>> >
>> > From: Liang Yang <liang.yang@amlogic.com>
>> >
>> > Add Amlogic NAND controller dt-bindings for Meson SoC,
>> > Current this driver support GXBB/GXL/AXG platform.
>> >
>> > Signed-off-by: Liang Yang <liang.yang@amlogic.com>
>> > Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
>> > ---
>> >  .../bindings/mtd/amlogic,meson-nand.txt       | 118 ++++++++++++++++++
>> >  1 file changed, 118 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>> > new file mode 100644
>> > index 000000000000..eac9f9433d5d
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>> > @@ -0,0 +1,118 @@
>> > +Amlogic NAND Flash Controller (NFC) for GXBB/GXL/AXG family SoCs
>> > +
>> > +This file documents the properties in addition to those available in
>> > +the MTD NAND bindings.
>> > +
>> > +Required properties:
>> > +- compatible : contains one of:
>> > +  - "amlogic,meson-gxl-nfc"
>> > +  - "amlogic,meson-axg-nfc"
>> the patch description states that GXBB/GXL/AXG are supported
>> shouldn't you add a compatible string for GXBB as well?
>> 
>> > +- clocks     :
>> > +       A list of phandle + clock-specifier pairs for the clocks listed
>> > +       in clock-names.
>> > +
>> > +- clock-names: Should contain the following:
>> > +       "core" - NFC module gate clock
>> > +       "clkin0" - Parent clock of internal mux
>> > +       "clkin1" - Other parent clock of internal mux
>> to give the devicetree maintainers some context on clkin0 and clkin1:
>> 
>> older SoCs (Meson8, Meson8b - not supported by this binding/driver
>> yet) had a dedicated NAND clock. there neither clkin0 or clkin1 would
>> be used, instead we just had a "nand" or "interface" clock (I'm not
>> aware of the actual naming in Amlogic's internal datasheets)
>> 
>> newer SoCs do NOT have a dedicated NAND "interface" clock anymore.
>> instead they are sharing the clock with the "sd_emmc_c" controller (I
>> *believe* the reason for this is because sd_emmc_c and the NAND
>> controller use the same pads on the SoC, pinctrl muxing controls where
>> these pads are routed -> NAND and sd_emmc_c cannot be used at the same
>> time, so SoC designers probably decided to re-use the clock)
>> 
>> unfortunately the sd_emmc_c clock is not provided by the "main" clock
>> controller on these newer SoCs
>> instead the clock is part of the MMC controller's register space (see
>> the SD_EMMC_CLOCK register in drivers/mmc/host/meson-gx-mmc.c)
>> even worse: the SD_EMMC_CLOCK contains more than just clock settings
>> (bit 25 enables the SDIO interrupt, which is currently not supported
>> by the meson-gx-mmc driver though)
>> 
>> the SD_EMMC_CLOCK register has a mux (CLK_SRC_MASK) to choose from
>> clkin0 and clkin1 which are passed here
>> the "amlogic,mmc-syscon" property is used to get a phandle to the
>> sd_emmc_c syscon register space
>> thus there is a bit of code duplication in the MMC and NAND drivers
>> with this binding (because both need to configure the SD_EMMC_CLOCK
>> register)
>
> Well, that's ugly. Really, the SD controller should be modeled as a 
> clock provider. But then you would have to always have a driver 
> instantiated for it. Maybe you need that anyway if accessing this 
> register is dependent on some other clock or reset to the module being 
> enabled (which you may not hit if you only access the reg during boot)?

On some earlier rounds of off-lis review, we did consider making the
SD/MMC controller a clock provider.  But forcing it to be instantiated
was kinda ugly too, especailly because there are several instances of
the MMC IP, and only one of them shares the clock with the NAND, so only
one of them needs to be a clock provider. :(

> But if you really want to do it this way, I guess that is fine.

Thanks.  We did consider a few options, and found this one to be the
least worst of a few options.

Kevin

WARNING: multiple messages have this Message-ID (diff)
From: khilman@baylibre.com (Kevin Hilman)
To: linus-amlogic@lists.infradead.org
Subject: [PATCH 1/2] dt-bindings: nand: meson: add Amlogic NAND controller driver
Date: Wed, 27 Jun 2018 16:40:45 -0700	[thread overview]
Message-ID: <7hvaa3hk76.fsf@baylibre.com> (raw)
In-Reply-To: <20180626183003.GA3019@rob-hp-laptop> (Rob Herring's message of "Tue, 26 Jun 2018 12:30:03 -0600")

Rob Herring <robh@kernel.org> writes:

> On Sun, Jun 24, 2018 at 12:46:59AM +0200, Martin Blumenstingl wrote:
>> Hello Yixun, Hello Liang,
>> 
>> I have a few small comments inline below
>> additionally I tried to explain the reason behind
>> "amlogic,mmc-syscon", clkin0 and clkin1 so Rob (or the devicetree
>> maintainers in general) can give feedback. feel free to correct me
>> wherever I'm wrong or provide additional notes in case I missed
>> something!
>> 
>> On Wed, Jun 13, 2018 at 10:17 AM Yixun Lan <yixun.lan@amlogic.com> wrote:
>> >
>> > From: Liang Yang <liang.yang@amlogic.com>
>> >
>> > Add Amlogic NAND controller dt-bindings for Meson SoC,
>> > Current this driver support GXBB/GXL/AXG platform.
>> >
>> > Signed-off-by: Liang Yang <liang.yang@amlogic.com>
>> > Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
>> > ---
>> >  .../bindings/mtd/amlogic,meson-nand.txt       | 118 ++++++++++++++++++
>> >  1 file changed, 118 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>> > new file mode 100644
>> > index 000000000000..eac9f9433d5d
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>> > @@ -0,0 +1,118 @@
>> > +Amlogic NAND Flash Controller (NFC) for GXBB/GXL/AXG family SoCs
>> > +
>> > +This file documents the properties in addition to those available in
>> > +the MTD NAND bindings.
>> > +
>> > +Required properties:
>> > +- compatible : contains one of:
>> > +  - "amlogic,meson-gxl-nfc"
>> > +  - "amlogic,meson-axg-nfc"
>> the patch description states that GXBB/GXL/AXG are supported
>> shouldn't you add a compatible string for GXBB as well?
>> 
>> > +- clocks     :
>> > +       A list of phandle + clock-specifier pairs for the clocks listed
>> > +       in clock-names.
>> > +
>> > +- clock-names: Should contain the following:
>> > +       "core" - NFC module gate clock
>> > +       "clkin0" - Parent clock of internal mux
>> > +       "clkin1" - Other parent clock of internal mux
>> to give the devicetree maintainers some context on clkin0 and clkin1:
>> 
>> older SoCs (Meson8, Meson8b - not supported by this binding/driver
>> yet) had a dedicated NAND clock. there neither clkin0 or clkin1 would
>> be used, instead we just had a "nand" or "interface" clock (I'm not
>> aware of the actual naming in Amlogic's internal datasheets)
>> 
>> newer SoCs do NOT have a dedicated NAND "interface" clock anymore.
>> instead they are sharing the clock with the "sd_emmc_c" controller (I
>> *believe* the reason for this is because sd_emmc_c and the NAND
>> controller use the same pads on the SoC, pinctrl muxing controls where
>> these pads are routed -> NAND and sd_emmc_c cannot be used at the same
>> time, so SoC designers probably decided to re-use the clock)
>> 
>> unfortunately the sd_emmc_c clock is not provided by the "main" clock
>> controller on these newer SoCs
>> instead the clock is part of the MMC controller's register space (see
>> the SD_EMMC_CLOCK register in drivers/mmc/host/meson-gx-mmc.c)
>> even worse: the SD_EMMC_CLOCK contains more than just clock settings
>> (bit 25 enables the SDIO interrupt, which is currently not supported
>> by the meson-gx-mmc driver though)
>> 
>> the SD_EMMC_CLOCK register has a mux (CLK_SRC_MASK) to choose from
>> clkin0 and clkin1 which are passed here
>> the "amlogic,mmc-syscon" property is used to get a phandle to the
>> sd_emmc_c syscon register space
>> thus there is a bit of code duplication in the MMC and NAND drivers
>> with this binding (because both need to configure the SD_EMMC_CLOCK
>> register)
>
> Well, that's ugly. Really, the SD controller should be modeled as a 
> clock provider. But then you would have to always have a driver 
> instantiated for it. Maybe you need that anyway if accessing this 
> register is dependent on some other clock or reset to the module being 
> enabled (which you may not hit if you only access the reg during boot)?

On some earlier rounds of off-lis review, we did consider making the
SD/MMC controller a clock provider.  But forcing it to be instantiated
was kinda ugly too, especailly because there are several instances of
the MMC IP, and only one of them shares the clock with the NAND, so only
one of them needs to be a clock provider. :(

> But if you really want to do it this way, I guess that is fine.

Thanks.  We did consider a few options, and found this one to be the
least worst of a few options.

Kevin

  reply	other threads:[~2018-06-27 23:40 UTC|newest]

Thread overview: 63+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-13 16:13 [PATCH 0/2] mtd: rawnand: meson: add Amlogic NAND driver support Yixun Lan
2018-06-13 16:13 ` Yixun Lan
2018-06-13 16:13 ` Yixun Lan
2018-06-13 16:13 ` Yixun Lan
2018-06-13 16:13 ` [PATCH 1/2] dt-bindings: nand: meson: add Amlogic NAND controller driver Yixun Lan
2018-06-13 16:13   ` Yixun Lan
2018-06-13 16:13   ` Yixun Lan
2018-06-13 16:13   ` Yixun Lan
2018-06-23 22:46   ` Martin Blumenstingl
2018-06-23 22:46     ` Martin Blumenstingl
2018-06-23 22:46     ` Martin Blumenstingl
2018-06-26 18:30     ` Rob Herring
2018-06-26 18:30       ` Rob Herring
2018-06-26 18:30       ` Rob Herring
2018-06-27 23:40       ` Kevin Hilman [this message]
2018-06-27 23:40         ` Kevin Hilman
2018-06-27 23:40         ` Kevin Hilman
2018-06-24 13:57   ` Boris Brezillon
2018-06-24 13:57     ` Boris Brezillon
2018-06-24 13:57     ` Boris Brezillon
2018-06-24 13:57     ` Boris Brezillon
2018-06-13 16:13 ` [PATCH 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller Yixun Lan
2018-06-13 16:13   ` Yixun Lan
2018-06-13 16:13   ` Yixun Lan
2018-06-13  9:07   ` kbuild test robot
2018-06-13  9:07     ` kbuild test robot
2018-06-13  9:07     ` kbuild test robot
2018-06-13  9:33   ` kbuild test robot
2018-06-13  9:33     ` kbuild test robot
2018-06-13  9:33     ` kbuild test robot
2018-06-24 19:38   ` Boris Brezillon
2018-06-24 19:38     ` Boris Brezillon
2018-06-24 19:38     ` Boris Brezillon
2018-06-27 23:33     ` Kevin Hilman
2018-06-27 23:33       ` Kevin Hilman
2018-06-27 23:33       ` Kevin Hilman
2018-06-28  7:00       ` Miquel Raynal
2018-06-28  7:00         ` Miquel Raynal
2018-06-28  7:00         ` Miquel Raynal
2018-06-28 23:45         ` Kevin Hilman
2018-06-28 23:45           ` Kevin Hilman
2018-06-28 23:45           ` Kevin Hilman
2018-06-29  7:14           ` Neil Armstrong
2018-06-29  7:14             ` Neil Armstrong
2018-06-29  7:14             ` Neil Armstrong
2018-07-02  7:17           ` Yixun Lan
2018-07-02  7:17             ` Yixun Lan
2018-07-02  7:17             ` Yixun Lan
2018-07-18  9:38     ` Yixun Lan
2018-07-18  9:38       ` Yixun Lan
2018-07-18  9:38       ` Yixun Lan
2018-07-18 19:08       ` Boris Brezillon
2018-07-18 19:08         ` Boris Brezillon
2018-07-18 19:08         ` Boris Brezillon
2018-07-19  8:13         ` Yixun Lan
2018-07-19  8:13           ` Yixun Lan
2018-07-19  8:13           ` Yixun Lan
2018-07-19  8:39           ` Boris Brezillon
2018-07-19  8:39             ` Boris Brezillon
2018-07-19  8:39             ` Boris Brezillon
2018-07-19  9:53             ` Yixun Lan
2018-07-19  9:53               ` Yixun Lan
2018-07-19  9:53               ` Yixun Lan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7hvaa3hk76.fsf@baylibre.com \
    --to=khilman@baylibre.com \
    --cc=boris.brezillon@bootlin.com \
    --cc=carlo@caione.org \
    --cc=computersforpeace@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dwmw2@infradead.org \
    --cc=jbrunet@baylibre.com \
    --cc=jian.hu@amlogic.com \
    --cc=liang.yang@amlogic.com \
    --cc=linux-amlogic@lists.infradead.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=marek.vasut@gmail.com \
    --cc=martin.blumenstingl@googlemail.com \
    --cc=narmstrong@baylibre.com \
    --cc=richard@nod.at \
    --cc=robh@kernel.org \
    --cc=yixun.lan@amlogic.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.