Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161903AbdDUS3e (ORCPT ); Fri, 21 Apr 2017 14:29:34 -0400 Received: from mail-oi0-f68.google.com ([209.85.218.68]:36137 "EHLO mail-oi0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1037177AbdDUS3S (ORCPT ); Fri, 21 Apr 2017 14:29:18 -0400 MIME-Version: 1.0 In-Reply-To: References: <20170421010755.18025-1-stefan@agner.ch> <20170421010755.18025-3-stefan@agner.ch> <5cf26d2b020392c875464c7504a9fb5b@agner.ch> <8377dadc-043f-5932-cb13-3367db38a6dd@gmail.com> <494a7bc044457240a302d92b3a50b8e5@agner.ch> <57336d7e-7b48-8855-9e87-3eb370facd05@gmail.com> From: Han Xu Date: Fri, 21 Apr 2017 13:29:16 -0500 Message-ID: Subject: Re: [PATCH 2/5] mtd: nand: gpmi: add i.MX 7 SoC support To: Stefan Agner Cc: Marek Vasut , mark.rutland@arm.com, Boris Brezillon , Fabio Estevam , devicetree@vger.kernel.org, Richard Weinberger , linux-kernel@vger.kernel.org, robh+dt@kernel.org, "linux-mtd@lists.infradead.org" , Sascha Hauer , Han Xu , Shawn Guo , Cyrille Pitchen , Brian Norris , David Woodhouse , linux-arm-kernel@lists.infradead.org, LW@karo-electronics.de Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4985 Lines: 135 On Fri, Apr 21, 2017 at 12:38 PM, Stefan Agner wrote: > On 2017-04-21 10:22, Marek Vasut wrote: >> On 04/21/2017 06:19 PM, Stefan Agner wrote: >>> On 2017-04-21 06:08, Marek Vasut wrote: >>>> On 04/21/2017 05:15 AM, Stefan Agner wrote: >>>>> On 2017-04-20 19:03, Marek Vasut wrote: >>>>>> On 04/21/2017 03:07 AM, Stefan Agner wrote: >>>>>>> Add support for i.MX 7 SoC. The i.MX 7 has a slightly different >>>>>>> clock architecture requiring only two clocks to be referenced. >>>>>>> The IP is slightly different compared to i.MX 6SoloX, but currently >>>>>>> none of this differences are in use so there is no detection needed >>>>>>> and the driver can reuse IS_MX6SX. >>>>>>> >>>>>>> Signed-off-by: Stefan Agner >>>>>>> --- >>>>>>> drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 15 +++++++++++++++ >>>>>>> 1 file changed, 15 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c >>>>>>> index c8bbf5da2ab8..4a45d37ddc80 100644 >>>>>>> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c >>>>>>> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c >>>>>>> @@ -127,6 +127,18 @@ static const struct gpmi_devdata gpmi_devdata_imx6sx = { >>>>>>> .clks_count = ARRAY_SIZE(gpmi_clks_for_mx6), >>>>>>> }; >>>>>>> >>>>>>> +static const char * const gpmi_clks_for_mx7d[] = { >>>>>>> + "gpmi_io", "gpmi_bch_apb", >>>>>>> +}; >>>>>>> + >>>>>>> +static const struct gpmi_devdata gpmi_devdata_imx7d = { >>>>>>> + .type = IS_MX6SX, >>>>>> >>>>>> Would it make sense to use IS_MX7 here already to prevent future surprises ? >>>>>> >>>>> >>>>> Yeah I was thinking we can do it once we have an actual reason to >>>>> distinguish. >>>> >>>> So what are the differences anyway ? >>>> >>> >>> I did not check the details, but Han's patchset (link in cover letter) >>> mentions: >>> "add the HW bitflip detection and correction for i.MX6QP and i.MX7D."... >> >> Oh, interesting. >> >>>>> But then, adding the type would only require 2-3 lines of change if I >>>>> add it to the GPMI_IS_MX6 macro... >>>> >>>> Then at least add a comment because using type = IMX6SX right under >>>> gpmi_data_mx7d can trigger some head-scratching. And put my R-B on V2. >>> >>> FWIW, I mentioned it in the commit message. >>> >>> I think rather then adding a comment it is cleaner to just add IS_IMX7D >>> and add it to the GPMI_IS_MX6 macro. That does not need a comment since >>> it implicitly says we have a i.MX 7 but treat it like i.MX 6 and it is a >>> rather small change. Does that sound acceptable? >> >> Sure, that's even better, thanks. >> >> btw isn't there some single-core mx7 (mx7s ?) , maybe we should just go >> with mx7 (without the d suffix). I dunno if it has GPMI NAND though, so >> maybe mx7d is the right thing to do here ... >> > > There is a Solo version yes, and it has GPMI NAND too. However, almost > all i.MX 7 IPs have been named imx7d by NXP for some reason (including > compatible strings, see grep -r -e imx7 Documentation/), so I thought I > stay consistent here... Hi Guys, Yes, there should be a i.MX7 Solo version with one core fused out. IMO, can we use QUIRK to distinguish them rather than SoC name. I know I also sent some patch set with SoC Name but I prefer to use QUIRK now. > > -- > Stefan > >>> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c >>> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c >>> @@ -132,7 +132,7 @@ static const char * const gpmi_clks_for_mx7d[] = { >>> }; >>> >>> static const struct gpmi_devdata gpmi_devdata_imx7d = { >>> - .type = IS_MX6SX, >>> + .type = IS_MX7D, >>> .bch_max_ecc_strength = 62, >>> .max_chain_delay = 12, >>> .clks = gpmi_clks_for_mx7d, >>> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h >>> b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h >>> index 2e584e18d980..f2cc13abc896 100644 >>> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h >>> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h >>> @@ -123,7 +123,8 @@ enum gpmi_type { >>> IS_MX23, >>> IS_MX28, >>> IS_MX6Q, >>> - IS_MX6SX >>> + IS_MX6SX, >>> + IS_MX7D, >>> }; >>> >>> struct gpmi_devdata { >>> @@ -307,6 +308,8 @@ void gpmi_copy_bits(u8 *dst, size_t dst_bit_off, >>> #define GPMI_IS_MX28(x) ((x)->devdata->type == IS_MX28) >>> #define GPMI_IS_MX6Q(x) ((x)->devdata->type == IS_MX6Q) >>> #define GPMI_IS_MX6SX(x) ((x)->devdata->type == IS_MX6SX) >>> +#define GPMI_IS_MX7D(x) ((x)->devdata->type == IS_MX7D) >>> >>> -#define GPMI_IS_MX6(x) (GPMI_IS_MX6Q(x) || GPMI_IS_MX6SX(x)) >>> +#define GPMI_IS_MX6(x) (GPMI_IS_MX6Q(x) || GPMI_IS_MX6SX(x) || >>> \ >>> + GPMI_IS_MX7D(x)) >>> #endif >>> >>> -- >>> Stefan >>> > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ -- Sincerely, Han XU