diff mbox

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

Message ID 494a7bc044457240a302d92b3a50b8e5@agner.ch
State Not Applicable
Headers show

Commit Message

Stefan Agner April 21, 2017, 4:19 p.m. UTC
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@agner.ch>
>>>> ---
>>>>  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?

 #endif

--
Stefan

Comments

Marek Vasut April 21, 2017, 5:22 p.m. UTC | #1
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@agner.ch>
>>>>> ---
>>>>>  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 ...

> --- 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
>
Stefan Agner April 21, 2017, 5:38 p.m. UTC | #2
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@agner.ch>
>>>>>> ---
>>>>>>  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...

--
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
>>
Han Xu April 21, 2017, 6:29 p.m. UTC | #3
On Fri, Apr 21, 2017 at 12:38 PM, Stefan Agner <stefan@agner.ch> 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@agner.ch>
>>>>>>> ---
>>>>>>>  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/
Boris Brezillon May 2, 2017, 9:17 a.m. UTC | #4
Hi Han,

On Fri, 21 Apr 2017 13:29:16 -0500
Han Xu <xhnjupt@gmail.com> wrote:

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

Not sure what this means. Are you okay with Stefan's v2?

Regards,

Boris
Marek Vasut May 2, 2017, 11:18 a.m. UTC | #5
On 05/02/2017 11:17 AM, Boris Brezillon wrote:
> Hi Han,
> 
> On Fri, 21 Apr 2017 13:29:16 -0500
> Han Xu <xhnjupt@gmail.com> wrote:
> 
>>>>  
>>>>>>> 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...  

I missed the DT bit, sorry. the DT bindings say:
  - compatible : should be "fsl,<chip>-gpmi-nand"
so if FSL invented their own buggy bindings, they need to get them
through Rob :) IMO for MX7, this should be "imx7-gpmi-nand" , unless
there's some incentive to discern the solo/dual chips and/or there
is a future imx7 coming up with different GPMI NAND block version.

>> 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.
> 
> Not sure what this means. Are you okay with Stefan's v2?

IMO the GPMI controller in solo and dual should be the same, so there's
no need to have quirks for it.
Stefan Agner May 3, 2017, 9:24 p.m. UTC | #6
On 2017-05-02 04:18, Marek Vasut wrote:
> On 05/02/2017 11:17 AM, Boris Brezillon wrote:
>> Hi Han,
>>
>> On Fri, 21 Apr 2017 13:29:16 -0500
>> Han Xu <xhnjupt@gmail.com> wrote:
>>
>>>>>
>>>>>>>> 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...
> 
> I missed the DT bit, sorry. the DT bindings say:
>   - compatible : should be "fsl,<chip>-gpmi-nand"
> so if FSL invented their own buggy bindings, they need to get them
> through Rob :) IMO for MX7, this should be "imx7-gpmi-nand" , unless
> there's some incentive to discern the solo/dual chips and/or there
> is a future imx7 coming up with different GPMI NAND block version.
> 

There is no incentive to discern solo/dual, afaict this are the same
chips, only some IP's "fused away"... From GPMI NAND perspective, they
are definitely the same.

So that leaves us with "imx7-gpmi-nand" vs. "imx7d-gpmi-nand".

All device tree compatible strings as well as Kconfig symbol,
preprocessor defines and function prefixes have been named "imx7d" or
IMX7D so far... Although they work just fine for i.MX7 Solo too... Not
sure why that exactly ended up to be that way, but I guess the reason
was that NXP started to upstream with the dual...

For the sake of continuity, I would prefer if we stick to that (as my
current patchsets do)... It is like imx6q, which also works for dual...

$ grep -r -w compatible.*imx7d  arch/arm/boot/dts/*.dts* | wc -l
68
$ grep -r -w compatible.*imx7^d  arch/arm/boot/dts/*.dts* | wc -l
0


>>> 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.
>>
>> Not sure what this means. Are you okay with Stefan's v2?
> 
> IMO the GPMI controller in solo and dual should be the same, so there's
> no need to have quirks for it.

Agreed.

--
Stefan
diff mbox

Patch

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