Re: [PATCH 2/5] mtd: nand: gpmi: add i.MX 7 SoC support

[Stefan Agner]

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 <stefan@xxxxxxxx>
>>>> ---
>>>>  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."...

>> 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?

--- 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