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

[Han Xu]

On Fri, Apr 21, 2017 at 12:38 PM, Stefan Agner <stefan@xxxxxxxx> 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 <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."...
>>
>> 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