diff mbox series

[v4,2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

Message ID 20200429104205.18780-3-vadivel.muruganx.ramuthevar@linux.intel.com
State Changes Requested
Delegated to: Miquel Raynal
Headers show
Series mtd: rawnand: Add NAND controller support on Intel LGM SoC | expand

Commit Message

Ramuthevar, Vadivel MuruganX April 29, 2020, 10:42 a.m. UTC
From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>

This patch adds the new IP of Nand Flash Controller(NFC) support
on Intel's Lightning Mountain(LGM) SoC.

DMA is used for burst data transfer operation, also DMA HW supports
aligned 32bit memory address and aligned data access by default.
DMA burst of 8 supported. Data register used to support the read/write
operation from/to device.

NAND controller driver implements ->exec_op() to replace legacy hooks,
these specific call-back method to execute NAND operations.

Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
---
 drivers/mtd/nand/raw/Kconfig                 |   8 +
 drivers/mtd/nand/raw/Makefile                |   1 +
 drivers/mtd/nand/raw/intel-nand-controller.c | 750 +++++++++++++++++++++++++++
 3 files changed, 759 insertions(+)
 create mode 100644 drivers/mtd/nand/raw/intel-nand-controller.c

Comments

Boris Brezillon April 29, 2020, 11:33 a.m. UTC | #1
On Wed, 29 Apr 2020 18:42:05 +0800
"Ramuthevar,Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:

> +#define NAND_WRITE_CMD		(EBU_CON_CS_P_LOW | HSNAND_CLE_OFFS)
> +#define NAND_WRITE_ADDR		(EBU_CON_CS_P_LOW | HSNAND_ALE_OFFS)
> +

I thought we agreed on dropping those definitions.
Ramuthevar, Vadivel MuruganX April 29, 2020, 1:29 p.m. UTC | #2
Hi Boris,

    Thank you very much for the review comments and your time..

On 29/4/2020 7:33 pm, Boris Brezillon wrote:
> On Wed, 29 Apr 2020 18:42:05 +0800
> "Ramuthevar,Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> 
>> +#define NAND_WRITE_CMD		(EBU_CON_CS_P_LOW | HSNAND_CLE_OFFS)
>> +#define NAND_WRITE_ADDR		(EBU_CON_CS_P_LOW | HSNAND_ALE_OFFS)
>> +
> 
> I thought we agreed on dropping those definitions.

Yes , we agreed upon it, due to assertion/de-assertion of CS kept it.
Thanks!

Regards
Vadivel
>
Boris Brezillon April 29, 2020, 1:32 p.m. UTC | #3
On Wed, 29 Apr 2020 21:29:40 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:

> Hi Boris,
> 
>     Thank you very much for the review comments and your time..
> 
> On 29/4/2020 7:33 pm, Boris Brezillon wrote:
> > On Wed, 29 Apr 2020 18:42:05 +0800
> > "Ramuthevar,Vadivel MuruganX"
> > <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> >   
> >> +#define NAND_WRITE_CMD		(EBU_CON_CS_P_LOW | HSNAND_CLE_OFFS)
> >> +#define NAND_WRITE_ADDR		(EBU_CON_CS_P_LOW | HSNAND_ALE_OFFS)
> >> +  
> > 
> > I thought we agreed on dropping those definitions.  
> 
> Yes , we agreed upon it, due to assertion/de-assertion of CS kept it.

And I keep thinking we don't need that, just pass
'HSNAND_CLE_OFFS | HSNAND_CS_OFFS' directly.
Boris Brezillon April 29, 2020, 2:22 p.m. UTC | #4
On Wed, 29 Apr 2020 18:42:05 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:

> +
> +#define EBU_ADDR_SEL(n)		(0x20 + (n) * 4)
> +#define EBU_ADDR_MASK		(5 << 4)

It's still unclear what ADDR_MASK is for. Can you add a comment
explaining what it does?

> +#define EBU_ADDR_SEL_REGEN	0x1


> +
> +	writel(lower_32_bits(ebu_host->cs[ebu_host->cs_num].nand_pa) |
> +	       EBU_ADDR_SEL_REGEN | EBU_ADDR_MASK,
> +	       ebu_host->ebu + EBU_ADDR_SEL(reg));
> +
> +	writel(EBU_MEM_BASE_CS_0 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN,
> +	       ebu_host->ebu + EBU_ADDR_SEL(0));
> +	writel(EBU_MEM_BASE_CS_1 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN,
> +	       ebu_host->ebu + EBU_ADDR_SEL(reg));

That's super weird. You seem to set EBU_ADDR_SEL(reg) twice. Are you
sure that's needed, and are we setting EBU_ADDR_SEL(0) here?
Ramuthevar, Vadivel MuruganX April 29, 2020, 2:26 p.m. UTC | #5
Hi Boris,

   Thank you very much for the review comments and your time...

On 29/4/2020 9:32 pm, Boris Brezillon wrote:
> On Wed, 29 Apr 2020 21:29:40 +0800
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> 
>> Hi Boris,
>>
>>      Thank you very much for the review comments and your time..
>>
>> On 29/4/2020 7:33 pm, Boris Brezillon wrote:
>>> On Wed, 29 Apr 2020 18:42:05 +0800
>>> "Ramuthevar,Vadivel MuruganX"
>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>>    
>>>> +#define NAND_WRITE_CMD		(EBU_CON_CS_P_LOW | HSNAND_CLE_OFFS)
>>>> +#define NAND_WRITE_ADDR		(EBU_CON_CS_P_LOW | HSNAND_ALE_OFFS)
>>>> +
>>>
>>> I thought we agreed on dropping those definitions.
>>
>> Yes , we agreed upon it, due to assertion/de-assertion of CS kept it.
> 
> And I keep thinking we don't need that, just pass
> 'HSNAND_CLE_OFFS | HSNAND_CS_OFFS' directly.

Agreed!, will update in the next-patch version, Thanks!

Regards
Vadivel

>
Ramuthevar, Vadivel MuruganX April 29, 2020, 2:33 p.m. UTC | #6
Hi Boris,

On 29/4/2020 10:22 pm, Boris Brezillon wrote:
> On Wed, 29 Apr 2020 18:42:05 +0800
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> 
>> +
>> +#define EBU_ADDR_SEL(n)		(0x20 + (n) * 4)
>> +#define EBU_ADDR_MASK		(5 << 4)
> 
> It's still unclear what ADDR_MASK is for. Can you add a comment
> explaining what it does?

Thank you Boris, keep review and giving inputs, will update.
> 
>> +#define EBU_ADDR_SEL_REGEN	0x1
> 
> 
>> +
>> +	writel(lower_32_bits(ebu_host->cs[ebu_host->cs_num].nand_pa) |
>> +	       EBU_ADDR_SEL_REGEN | EBU_ADDR_MASK,
>> +	       ebu_host->ebu + EBU_ADDR_SEL(reg));
>> +
>> +	writel(EBU_MEM_BASE_CS_0 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN,
>> +	       ebu_host->ebu + EBU_ADDR_SEL(0));
>> +	writel(EBU_MEM_BASE_CS_1 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN,
>> +	       ebu_host->ebu + EBU_ADDR_SEL(reg));
> 
> That's super weird. You seem to set EBU_ADDR_SEL(reg) twice. Are you
> sure that's needed, and are we setting EBU_ADDR_SEL(0) here?

You are right, its weird only, but we need it, since different chip 
select has different memory region access address.

Yes , we are setting both CS0 and CS1 memory access region, if you have 
any concern to optimize, please suggest me, Thanks!

Regards
Vadivel
>
Boris Brezillon April 29, 2020, 2:48 p.m. UTC | #7
On Wed, 29 Apr 2020 22:33:37 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:

> Hi Boris,
> 
> On 29/4/2020 10:22 pm, Boris Brezillon wrote:
> > On Wed, 29 Apr 2020 18:42:05 +0800
> > "Ramuthevar, Vadivel MuruganX"
> > <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> >   
> >> +
> >> +#define EBU_ADDR_SEL(n)		(0x20 + (n) * 4)
> >> +#define EBU_ADDR_MASK		(5 << 4)  
> > 
> > It's still unclear what ADDR_MASK is for. Can you add a comment
> > explaining what it does?  
> 
> Thank you Boris, keep review and giving inputs, will update.

Can you please explain it here before sending a new version?

> >   
> >> +#define EBU_ADDR_SEL_REGEN	0x1  
> > 
> >   
> >> +
> >> +	writel(lower_32_bits(ebu_host->cs[ebu_host->cs_num].nand_pa) |
> >> +	       EBU_ADDR_SEL_REGEN | EBU_ADDR_MASK,
> >> +	       ebu_host->ebu + EBU_ADDR_SEL(reg));
> >> +
> >> +	writel(EBU_MEM_BASE_CS_0 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN,
> >> +	       ebu_host->ebu + EBU_ADDR_SEL(0));
> >> +	writel(EBU_MEM_BASE_CS_1 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN,
> >> +	       ebu_host->ebu + EBU_ADDR_SEL(reg));  
> > 
> > That's super weird. You seem to set EBU_ADDR_SEL(reg) twice. Are you
> > sure that's needed, and are we setting EBU_ADDR_SEL(0) here?  
> 
> You are right, its weird only, but we need it, since different chip 
> select has different memory region access address.

Well, that doesn't make any sense, the second write to
EBU_ADDR_SEL(reg) overrides the first one, meaning that nand_pa is
actually never written to ADDR_SEL(reg).

> 
> Yes , we are setting both CS0 and CS1 memory access region, if you have 
> any concern to optimize, please suggest me, Thanks!

If you want to setup both CS, and the address written in EBU_ADDR_SEL(x)
is really related to the nand_pa address, then retrieve resources for
all CS ranges. If it's not related, please explain what those
EBU_MEM_BASE_CS_X values encode.
Ramuthevar, Vadivel MuruganX April 29, 2020, 3:18 p.m. UTC | #8
Hi Boris,

On 29/4/2020 10:48 pm, Boris Brezillon wrote:
> On Wed, 29 Apr 2020 22:33:37 +0800
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> 
>> Hi Boris,
>>
>> On 29/4/2020 10:22 pm, Boris Brezillon wrote:
>>> On Wed, 29 Apr 2020 18:42:05 +0800
>>> "Ramuthevar, Vadivel MuruganX"
>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>>    
>>>> +
>>>> +#define EBU_ADDR_SEL(n)		(0x20 + (n) * 4)
>>>> +#define EBU_ADDR_MASK		(5 << 4)
>>>
>>> It's still unclear what ADDR_MASK is for. Can you add a comment
>>> explaining what it does?
>>
>> Thank you Boris, keep review and giving inputs, will update.
> 
> Can you please explain it here before sending a new version?

Memory Region Address Mask:
Specifies the number of right-most bits in the base address that should 
be included in the address comparison. bits positions(7:4).

>>>    
>>>> +#define EBU_ADDR_SEL_REGEN	0x1
>>>
>>>    
>>>> +
>>>> +	writel(lower_32_bits(ebu_host->cs[ebu_host->cs_num].nand_pa) |
>>>> +	       EBU_ADDR_SEL_REGEN | EBU_ADDR_MASK,
>>>> +	       ebu_host->ebu + EBU_ADDR_SEL(reg));
>>>> +
>>>> +	writel(EBU_MEM_BASE_CS_0 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN,
>>>> +	       ebu_host->ebu + EBU_ADDR_SEL(0));
>>>> +	writel(EBU_MEM_BASE_CS_1 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN,
>>>> +	       ebu_host->ebu + EBU_ADDR_SEL(reg));
>>>
>>> That's super weird. You seem to set EBU_ADDR_SEL(reg) twice. Are you
>>> sure that's needed, and are we setting EBU_ADDR_SEL(0) here?
>>
>> You are right, its weird only, but we need it, since different chip
>> select has different memory region access address.
> 
> Well, that doesn't make any sense, the second write to
> EBU_ADDR_SEL(reg) overrides the first one, meaning that nand_pa is
> actually never written to ADDR_SEL(reg).

it will not overwrite the first one, since two different registers
EBU_ADDR_SEL_0 EBU_ADDR_SEL  20H
EBU_ADDR_SEL_1 EBU_ADDR_SEL  24H

it is an internal address selection w.r.t chip select for nand physical 
address update.


> 
>>
>> Yes , we are setting both CS0 and CS1 memory access region, if you have
>> any concern to optimize, please suggest me, Thanks!
> 
> If you want to setup both CS, and the address written in EBU_ADDR_SEL(x)
> is really related to the nand_pa address, then retrieve resources for
> all CS ranges. 
If it's not related, please explain what those
> EBU_MEM_BASE_CS_X values encode.

Memory Region Base Address
FPI Bus addresses are compared to this base address in conjunction with 
the mask control(EBU_ADDR_MASK). Driver need to program this field!

Regards
Vadivel

>
Ramuthevar, Vadivel MuruganX April 29, 2020, 3:29 p.m. UTC | #9
Hi Boris,

On 29/4/2020 11:18 pm, Ramuthevar, Vadivel MuruganX wrote:
> +    writel(lower_32_bits(ebu_host->cs[ebu_host->cs_num].nand_pa) |
> +           EBU_ADDR_SEL_REGEN | EBU_ADDR_MASK,
> +           ebu_host->ebu + EBU_ADDR_SEL(reg));
> +
> +    writel(EBU_MEM_BASE_CS_0 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN,
> +           ebu_host->ebu + EBU_ADDR_SEL(0));

Good catch!, one will be removed , Thanks !

Regards
Vadivel
Boris Brezillon April 29, 2020, 3:31 p.m. UTC | #10
On Wed, 29 Apr 2020 23:18:31 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:

> Hi Boris,
> 
> On 29/4/2020 10:48 pm, Boris Brezillon wrote:
> > On Wed, 29 Apr 2020 22:33:37 +0800
> > "Ramuthevar, Vadivel MuruganX"
> > <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> >   
> >> Hi Boris,
> >>
> >> On 29/4/2020 10:22 pm, Boris Brezillon wrote:  
> >>> On Wed, 29 Apr 2020 18:42:05 +0800
> >>> "Ramuthevar, Vadivel MuruganX"
> >>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> >>>      
> >>>> +
> >>>> +#define EBU_ADDR_SEL(n)		(0x20 + (n) * 4)
> >>>> +#define EBU_ADDR_MASK		(5 << 4)  
> >>>
> >>> It's still unclear what ADDR_MASK is for. Can you add a comment
> >>> explaining what it does?  
> >>
> >> Thank you Boris, keep review and giving inputs, will update.  
> > 
> > Can you please explain it here before sending a new version?  
> 
> Memory Region Address Mask:
> Specifies the number of right-most bits in the base address that should 
> be included in the address comparison. bits positions(7:4).

Okay, then the macro should be 

#define EBU_ADDR_MASK(x)	((x) << 4)

And now I'd like you to explain why 5 is the right value for that field
(I guess that has to do with the position of the CS/ALE/CLE pins).

> 
> >>>      
> >>>> +#define EBU_ADDR_SEL_REGEN	0x1  
> >>>
> >>>      
> >>>> +
> >>>> +	writel(lower_32_bits(ebu_host->cs[ebu_host->cs_num].nand_pa) |
> >>>> +	       EBU_ADDR_SEL_REGEN | EBU_ADDR_MASK,
> >>>> +	       ebu_host->ebu + EBU_ADDR_SEL(reg));

You set EBU_ADDR_SEL(reg) once here...

> >>>> +
> >>>> +	writel(EBU_MEM_BASE_CS_0 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN,
> >>>> +	       ebu_host->ebu + EBU_ADDR_SEL(0));
> >>>> +	writel(EBU_MEM_BASE_CS_1 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN,
> >>>> +	       ebu_host->ebu + EBU_ADDR_SEL(reg));  

... and a second time here. That sounds like overwriting the
EBU_ADDR_SEL(reg) register to me.

> >>>
> >>> That's super weird. You seem to set EBU_ADDR_SEL(reg) twice. Are you
> >>> sure that's needed, and are we setting EBU_ADDR_SEL(0) here?  
> >>
> >> You are right, its weird only, but we need it, since different chip
> >> select has different memory region access address.  
> > 
> > Well, that doesn't make any sense, the second write to
> > EBU_ADDR_SEL(reg) overrides the first one, meaning that nand_pa is
> > actually never written to ADDR_SEL(reg).  
> 
> it will not overwrite the first one, since two different registers
> EBU_ADDR_SEL_0 EBU_ADDR_SEL  20H
> EBU_ADDR_SEL_1 EBU_ADDR_SEL  24H

See my above.

> 
> it is an internal address selection w.r.t chip select for nand physical 
> address update.
> 
> 
> >   
> >>
> >> Yes , we are setting both CS0 and CS1 memory access region, if you have
> >> any concern to optimize, please suggest me, Thanks!  
> > 
> > If you want to setup both CS, and the address written in EBU_ADDR_SEL(x)
> > is really related to the nand_pa address, then retrieve resources for
> > all CS ranges.   
> If it's not related, please explain what those
> > EBU_MEM_BASE_CS_X values encode.  
> 
> Memory Region Base Address
> FPI Bus addresses are compared to this base address in conjunction with 
> the mask control(EBU_ADDR_MASK). Driver need to program this field!

That's not explaining what the base address should be. Is 'nand_pa' the
value we should have there?
Ramuthevar, Vadivel MuruganX April 30, 2020, 7:50 a.m. UTC | #11
Hi Boris,

   Thank you very much for keep reviewing the patches and more queries...

On 29/4/2020 11:31 pm, Boris Brezillon wrote:
> On Wed, 29 Apr 2020 23:18:31 +0800
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> 
>> Hi Boris,
>>
>> On 29/4/2020 10:48 pm, Boris Brezillon wrote:
>>> On Wed, 29 Apr 2020 22:33:37 +0800
>>> "Ramuthevar, Vadivel MuruganX"
>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>>    
>>>> Hi Boris,
>>>>
>>>> On 29/4/2020 10:22 pm, Boris Brezillon wrote:
>>>>> On Wed, 29 Apr 2020 18:42:05 +0800
>>>>> "Ramuthevar, Vadivel MuruganX"
>>>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>>>>       
>>>>>> +
>>>>>> +#define EBU_ADDR_SEL(n)		(0x20 + (n) * 4)
>>>>>> +#define EBU_ADDR_MASK		(5 << 4)
>>>>>
>>>>> It's still unclear what ADDR_MASK is for. Can you add a comment
>>>>> explaining what it does?
>>>>
>>>> Thank you Boris, keep review and giving inputs, will update.
>>>
>>> Can you please explain it here before sending a new version?
>>
>> Memory Region Address Mask:
>> Specifies the number of right-most bits in the base address that should
>> be included in the address comparison. bits positions(7:4).
> 
> Okay, then the macro should be
> 
> #define EBU_ADDR_MASK(x)	((x) << 4)
> 
> And now I'd like you to explain why 5 is the right value for that field
> (I guess that has to do with the position of the CS/ALE/CLE pins).

5 : bit 26, 25, 24, 23, 22 to be included for comparison in the 
ADDR_SELx , it compares only 5 bits.

> 
>>
>>>>>       
>>>>>> +#define EBU_ADDR_SEL_REGEN	0x1
>>>>>
>>>>>       
>>>>>> +
>>>>>> +	writel(lower_32_bits(ebu_host->cs[ebu_host->cs_num].nand_pa) |
>>>>>> +	       EBU_ADDR_SEL_REGEN | EBU_ADDR_MASK,
>>>>>> +	       ebu_host->ebu + EBU_ADDR_SEL(reg));
> 
> You set EBU_ADDR_SEL(reg) once here...
> 
>>>>>> +
>>>>>> +	writel(EBU_MEM_BASE_CS_0 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN,
>>>>>> +	       ebu_host->ebu + EBU_ADDR_SEL(0));
>>>>>> +	writel(EBU_MEM_BASE_CS_1 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN,
>>>>>> +	       ebu_host->ebu + EBU_ADDR_SEL(reg));
> 
> ... and a second time here. That sounds like overwriting the
> EBU_ADDR_SEL(reg) register to me.
> 
>>>>>
>>>>> That's super weird. You seem to set EBU_ADDR_SEL(reg) twice. Are you
>>>>> sure that's needed, and are we setting EBU_ADDR_SEL(0) here?
>>>>
>>>> You are right, its weird only, but we need it, since different chip
>>>> select has different memory region access address.
>>>
>>> Well, that doesn't make any sense, the second write to
>>> EBU_ADDR_SEL(reg) overrides the first one, meaning that nand_pa is
>>> actually never written to ADDR_SEL(reg).
>>
>> it will not overwrite the first one, since two different registers
>> EBU_ADDR_SEL_0 EBU_ADDR_SEL  20H
>> EBU_ADDR_SEL_1 EBU_ADDR_SEL  24H
> 
> See my above.
> 
>>
>> it is an internal address selection w.r.t chip select for nand physical
>> address update.
>>
>>
>>>    
>>>>
>>>> Yes , we are setting both CS0 and CS1 memory access region, if you have
>>>> any concern to optimize, please suggest me, Thanks!
>>>
>>> If you want to setup both CS, and the address written in EBU_ADDR_SEL(x)
>>> is really related to the nand_pa address, then retrieve resources for
>>> all CS ranges.
>> If it's not related, please explain what those
>>> EBU_MEM_BASE_CS_X values encode.
>>
>> Memory Region Base Address
>> FPI Bus addresses are compared to this base address in conjunction with
>> the mask control(EBU_ADDR_MASK). Driver need to program this field!
> 
> That's not explaining what the base address should be. Is 'nand_pa' the
> value we should have there?

The one prorgrammed in the addr_sel register is used by the HW 
controller, it remaps to  0x174XX-> CS0 and 0x17CXX->CS1.
The hardware itself, decodes only for 1740xx/17c0xx, other random values 
cannot be programmed

Regards
Vadivel
>
Boris Brezillon April 30, 2020, 8:21 a.m. UTC | #12
On Thu, 30 Apr 2020 15:50:30 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:

> Hi Boris,
> 
>    Thank you very much for keep reviewing the patches and more queries...
> 
> On 29/4/2020 11:31 pm, Boris Brezillon wrote:
> > On Wed, 29 Apr 2020 23:18:31 +0800
> > "Ramuthevar, Vadivel MuruganX"
> > <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> >   
> >> Hi Boris,
> >>
> >> On 29/4/2020 10:48 pm, Boris Brezillon wrote:  
> >>> On Wed, 29 Apr 2020 22:33:37 +0800
> >>> "Ramuthevar, Vadivel MuruganX"
> >>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> >>>      
> >>>> Hi Boris,
> >>>>
> >>>> On 29/4/2020 10:22 pm, Boris Brezillon wrote:  
> >>>>> On Wed, 29 Apr 2020 18:42:05 +0800
> >>>>> "Ramuthevar, Vadivel MuruganX"
> >>>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> >>>>>         
> >>>>>> +
> >>>>>> +#define EBU_ADDR_SEL(n)		(0x20 + (n) * 4)
> >>>>>> +#define EBU_ADDR_MASK		(5 << 4)  
> >>>>>
> >>>>> It's still unclear what ADDR_MASK is for. Can you add a comment
> >>>>> explaining what it does?  
> >>>>
> >>>> Thank you Boris, keep review and giving inputs, will update.  
> >>>
> >>> Can you please explain it here before sending a new version?  
> >>
> >> Memory Region Address Mask:
> >> Specifies the number of right-most bits in the base address that should
> >> be included in the address comparison. bits positions(7:4).  
> > 
> > Okay, then the macro should be
> > 
> > #define EBU_ADDR_MASK(x)	((x) << 4)
> > 
> > And now I'd like you to explain why 5 is the right value for that field
> > (I guess that has to do with the position of the CS/ALE/CLE pins).  
> 
> 5 : bit 26, 25, 24, 23, 22 to be included for comparison in the 

That's 6 bits to me, not 5.

> ADDR_SELx , it compares only 5 bits.

Definitely not what I would qualify as right-most bits. So, you say the
comparison always starts at bit 22, and ends at 22+<num-addr-bits>?

> >>>> Yes , we are setting both CS0 and CS1 memory access region, if you have
> >>>> any concern to optimize, please suggest me, Thanks!  
> >>>
> >>> If you want to setup both CS, and the address written in EBU_ADDR_SEL(x)
> >>> is really related to the nand_pa address, then retrieve resources for
> >>> all CS ranges.  
> >> If it's not related, please explain what those  
> >>> EBU_MEM_BASE_CS_X values encode.  
> >>
> >> Memory Region Base Address
> >> FPI Bus addresses are compared to this base address in conjunction with
> >> the mask control(EBU_ADDR_MASK). Driver need to program this field!  
> > 
> > That's not explaining what the base address should be. Is 'nand_pa' the
> > value we should have there?  
> 
> The one prorgrammed in the addr_sel register is used by the HW 
> controller, it remaps to  0x174XX-> CS0 and 0x17CXX->CS1.
> The hardware itself, decodes only for 1740xx/17c0xx, other random values 
> cannot be programmed

The question is, is it the same value we have in nand_pa or it is
different?
Ramuthevar, Vadivel MuruganX April 30, 2020, 8:30 a.m. UTC | #13
H Boris,

On 30/4/2020 4:21 pm, Boris Brezillon wrote:
> On Thu, 30 Apr 2020 15:50:30 +0800
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> 
>> Hi Boris,
>>
>>     Thank you very much for keep reviewing the patches and more queries...
>>
>> On 29/4/2020 11:31 pm, Boris Brezillon wrote:
>>> On Wed, 29 Apr 2020 23:18:31 +0800
>>> "Ramuthevar, Vadivel MuruganX"
>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>>    
>>>> Hi Boris,
>>>>
>>>> On 29/4/2020 10:48 pm, Boris Brezillon wrote:
>>>>> On Wed, 29 Apr 2020 22:33:37 +0800
>>>>> "Ramuthevar, Vadivel MuruganX"
>>>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>>>>       
>>>>>> Hi Boris,
>>>>>>
>>>>>> On 29/4/2020 10:22 pm, Boris Brezillon wrote:
>>>>>>> On Wed, 29 Apr 2020 18:42:05 +0800
>>>>>>> "Ramuthevar, Vadivel MuruganX"
>>>>>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>>>>>>          
>>>>>>>> +
>>>>>>>> +#define EBU_ADDR_SEL(n)		(0x20 + (n) * 4)
>>>>>>>> +#define EBU_ADDR_MASK		(5 << 4)
>>>>>>>
>>>>>>> It's still unclear what ADDR_MASK is for. Can you add a comment
>>>>>>> explaining what it does?
>>>>>>
>>>>>> Thank you Boris, keep review and giving inputs, will update.
>>>>>
>>>>> Can you please explain it here before sending a new version?
>>>>
>>>> Memory Region Address Mask:
>>>> Specifies the number of right-most bits in the base address that should
>>>> be included in the address comparison. bits positions(7:4).
>>>
>>> Okay, then the macro should be
>>>
>>> #define EBU_ADDR_MASK(x)	((x) << 4)
>>>
>>> And now I'd like you to explain why 5 is the right value for that field
>>> (I guess that has to do with the position of the CS/ALE/CLE pins).
>>
>> 5 : bit 26, 25, 24, 23, 22 to be included for comparison in the
> 
> That's 6 bits to me, not 5.

No , 5 bits only the above case.
> 
>> ADDR_SELx , it compares only 5 bits.
> 
> Definitely not what I would qualify as right-most bits. So, you say the
> comparison always starts at bit 22, and ends at 22+<num-addr-bits>?

Correct

> 
>>>>>> Yes , we are setting both CS0 and CS1 memory access region, if you have
>>>>>> any concern to optimize, please suggest me, Thanks!
>>>>>
>>>>> If you want to setup both CS, and the address written in EBU_ADDR_SEL(x)
>>>>> is really related to the nand_pa address, then retrieve resources for
>>>>> all CS ranges.
>>>> If it's not related, please explain what those
>>>>> EBU_MEM_BASE_CS_X values encode.
>>>>
>>>> Memory Region Base Address
>>>> FPI Bus addresses are compared to this base address in conjunction with
>>>> the mask control(EBU_ADDR_MASK). Driver need to program this field!
>>>
>>> That's not explaining what the base address should be. Is 'nand_pa' the
>>> value we should have there?
>>
>> The one prorgrammed in the addr_sel register is used by the HW
>> controller, it remaps to  0x174XX-> CS0 and 0x17CXX->CS1.
>> The hardware itself, decodes only for 1740xx/17c0xx, other random values
>> cannot be programmed
> 
> The question is, is it the same value we have in nand_pa or it is
> different?
> 
Different address which is 0xE1400000 NAND_BASE_PHY address.

Thanks!

Regards
Vadivel
Boris Brezillon April 30, 2020, 8:36 a.m. UTC | #14
On Thu, 30 Apr 2020 16:30:15 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:

> >>>
> >>> And now I'd like you to explain why 5 is the right value for that field
> >>> (I guess that has to do with the position of the CS/ALE/CLE pins).  
> >>
> >> 5 : bit 26, 25, 24, 23, 22 to be included for comparison in the  
> > 
> > That's 6 bits to me, not 5.  
> 
> No , 5 bits only the above case.

Oops, right, sorry for the brain fart.

> > 
> > The question is, is it the same value we have in nand_pa or it is
> > different?
> >   
> Different address which is 0xE1400000 NAND_BASE_PHY address.

Then why didn't you tell me they didn't match when I suggested to pass
nand_pa? So now the question is, what does this address represent? Do
you have a reference manual I can look at to understand what this is?
Ramuthevar, Vadivel MuruganX April 30, 2020, 9:07 a.m. UTC | #15
Hi Boris,

On 30/4/2020 4:36 pm, Boris Brezillon wrote:
> On Thu, 30 Apr 2020 16:30:15 +0800
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> 
>>>>>
>>>>> And now I'd like you to explain why 5 is the right value for that field
>>>>> (I guess that has to do with the position of the CS/ALE/CLE pins).
>>>>
>>>> 5 : bit 26, 25, 24, 23, 22 to be included for comparison in the
>>>
>>> That's 6 bits to me, not 5.
>>
>> No , 5 bits only the above case.
> 
> Oops, right, sorry for the brain fart.
> 
>>>
>>> The question is, is it the same value we have in nand_pa or it is
>>> different?
>>>    
>> Different address which is 0xE1400000 NAND_BASE_PHY address.
> 
> Then why didn't you tell me they didn't match when I suggested to pass

sorry, because you keep asking nand_pa after that only I realized that.

> nand_pa? So now the question is, what does this address represent?

                EBU-MODULE
  _________     _______________________
|         |   |            |NAND CTRL  |
| FPI BUS |==>| CS0(0x174) | 0xE100    ( 0xE14/0xE1C) NAND_PHY_BASE
|_________|   |_CS1(0x17C)_|__________ |

EBU_CONRTROLLER_BASE : 0xE0F0_0000
HSNAND_BASE: 0xE100_0000
NAND_CS0: 0xE140_0000
NAND_CS1: 0xE1C0_0000

MEM_REGION_BASE_CS0: 0x17400 (internal to ebu controller )
MEM_REGION_BASE_CS1: 0x17C00

>Do you have a reference manual I can look at to understand what this is?

We dont have reference manual since it is new SoC, we keep get 
information from HW team and we have only register map

Thanks a lot !!!

Regards
Vadivel
>
Boris Brezillon April 30, 2020, 12:36 p.m. UTC | #16
On Thu, 30 Apr 2020 17:07:03 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:

> >>> The question is, is it the same value we have in nand_pa or it is
> >>> different?
> >>>      
> >> Different address which is 0xE1400000 NAND_BASE_PHY address.  
> > 
> > Then why didn't you tell me they didn't match when I suggested to pass  
> 
> sorry, because you keep asking nand_pa after that only I realized that.
> 
> > nand_pa? So now the question is, what does this address represent?  
> 
>                 EBU-MODULE
>   _________     _______________________
> |         |   |            |NAND CTRL  |
> | FPI BUS |==>| CS0(0x174) | 0xE100    ( 0xE14/0xE1C) NAND_PHY_BASE
> |_________|   |_CS1(0x17C)_|__________ |
> 
> EBU_CONRTROLLER_BASE : 0xE0F0_0000
> HSNAND_BASE: 0xE100_0000
> NAND_CS0: 0xE140_0000
> NAND_CS1: 0xE1C0_0000
> 
> MEM_REGION_BASE_CS0: 0x17400 (internal to ebu controller )
> MEM_REGION_BASE_CS1: 0x17C00
> 

Hm, I wonder if we shouldn't use a 'ranges' property to describe this
address translation. Something like

	ebu@xxx {
		ranges = <0x17400000 0xe1400000 0x1000>,
			 <0x17c00000 0xe1c00000 0x1000>;
		reg = <0x17400000>, <0x17c00000>;
		reg-names = "cs-0", "cs-1";
	}

The translated address (0xE1X00000) will be available in res->start,
and the non-translated one (0x17X00000) can be retrieved with
of_get_address(). All you'd have to do then would be calculate the
mask:

	mask = (translated_address & original_address) >> 22;
	num_comp_bits = fls(mask);
	WARN_ON(mask != GENMASK(num_comp_bits - 1, 0));

Which allows you to properly set the ADDR_SEL() register without
relying on some hardcoded values:

	writel(original_address | EBU_ADDR_SEL_REGEN |
	       EBU_ADDR_COMP_BITS(num_comp_bits),
	       ebu_host->ebu + EBU_ADDR_SEL(csid));

That's quite important if we want to merge the xway NAND driver with
this one.
Boris Brezillon April 30, 2020, 1:01 p.m. UTC | #17
On Thu, 30 Apr 2020 14:36:00 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Thu, 30 Apr 2020 17:07:03 +0800
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> 
> > >>> The question is, is it the same value we have in nand_pa or it is
> > >>> different?
> > >>>        
> > >> Different address which is 0xE1400000 NAND_BASE_PHY address.    
> > > 
> > > Then why didn't you tell me they didn't match when I suggested to pass    
> > 
> > sorry, because you keep asking nand_pa after that only I realized that.
> >   
> > > nand_pa? So now the question is, what does this address represent?    
> > 
> >                 EBU-MODULE
> >   _________     _______________________
> > |         |   |            |NAND CTRL  |
> > | FPI BUS |==>| CS0(0x174) | 0xE100    ( 0xE14/0xE1C) NAND_PHY_BASE
> > |_________|   |_CS1(0x17C)_|__________ |
> > 
> > EBU_CONRTROLLER_BASE : 0xE0F0_0000
> > HSNAND_BASE: 0xE100_0000
> > NAND_CS0: 0xE140_0000
> > NAND_CS1: 0xE1C0_0000
> > 
> > MEM_REGION_BASE_CS0: 0x17400 (internal to ebu controller )
> > MEM_REGION_BASE_CS1: 0x17C00
> >   
> 
> Hm, I wonder if we shouldn't use a 'ranges' property to describe this
> address translation. Something like
> 
> 	ebu@xxx {
> 		ranges = <0x17400000 0xe1400000 0x1000>,
> 			 <0x17c00000 0xe1c00000 0x1000>;
> 		reg = <0x17400000>, <0x17c00000>;
> 		reg-names = "cs-0", "cs-1";
> 	}
> 
> The translated address (0xE1X00000) will be available in res->start,
> and the non-translated one (0x17X00000) can be retrieved with
> of_get_address(). All you'd have to do then would be calculate the
> mask:
> 
> 	mask = (translated_address & original_address) >> 22;
> 	num_comp_bits = fls(mask);
> 	WARN_ON(mask != GENMASK(num_comp_bits - 1, 0));
> 
> Which allows you to properly set the ADDR_SEL() register without
> relying on some hardcoded values:
> 
> 	writel(original_address | EBU_ADDR_SEL_REGEN |
> 	       EBU_ADDR_COMP_BITS(num_comp_bits),
> 	       ebu_host->ebu + EBU_ADDR_SEL(csid));
> 
> That's quite important if we want to merge the xway NAND driver with
> this one.

Looks like the translation is done at the FPI bus declaration level (see
[1]). We really need to see the big picture to take a wise decision
about the bindings. Would you mind pasting your dsti/dts files
somewhere? It feels like the NAND controller is a sub-part of a more
generic 'memory' controller, in which case the NAND controller should be
declared as a child of this generic memory bus (called localbus in [1],
but maybe EBU is more accurate).

[1]https://github.com/xieyaxiongfly/Atheros_CSI_tool_OpenWRT_src/blob/master/target/linux/lantiq/files-4.14/arch/mips/boot/dts/vr9.dtsi#L162
Ramuthevar, Vadivel MuruganX May 4, 2020, 1:54 a.m. UTC | #18
Hi Boris,

   Thank you very much for the review comments and giving inputs...

On 30/4/2020 8:36 pm, Boris Brezillon wrote:
> On Thu, 30 Apr 2020 17:07:03 +0800
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> 
>>>>> The question is, is it the same value we have in nand_pa or it is
>>>>> different?
>>>>>       
>>>> Different address which is 0xE1400000 NAND_BASE_PHY address.
>>>
>>> Then why didn't you tell me they didn't match when I suggested to pass
>>
>> sorry, because you keep asking nand_pa after that only I realized that.
>>
>>> nand_pa? So now the question is, what does this address represent?
>>
>>                  EBU-MODULE
>>    _________     _______________________
>> |         |   |            |NAND CTRL  |
>> | FPI BUS |==>| CS0(0x174) | 0xE100    ( 0xE14/0xE1C) NAND_PHY_BASE
>> |_________|   |_CS1(0x17C)_|__________ |
>>
>> EBU_CONRTROLLER_BASE : 0xE0F0_0000
>> HSNAND_BASE: 0xE100_0000
>> NAND_CS0: 0xE140_0000
>> NAND_CS1: 0xE1C0_0000
>>
>> MEM_REGION_BASE_CS0: 0x17400 (internal to ebu controller )
>> MEM_REGION_BASE_CS1: 0x17C00
>>
> 
> Hm, I wonder if we shouldn't use a 'ranges' property to describe this
> address translation. Something like
> 
> 	ebu@xxx {
> 		ranges = <0x17400000 0xe1400000 0x1000>,
> 			 <0x17c00000 0xe1c00000 0x1000>;
> 		reg = <0x17400000>, <0x17c00000>;
> 		reg-names = "cs-0", "cs-1";
> 	}
> 
> The translated address (0xE1X00000) will be available in res->start,
> and the non-translated one (0x17X00000) can be retrieved with
> of_get_address(). All you'd have to do then would be calculate the
> mask:
> 
> 	mask = (translated_address & original_address) >> 22;
> 	num_comp_bits = fls(mask);
> 	WARN_ON(mask != GENMASK(num_comp_bits - 1, 0));
> 
> Which allows you to properly set the ADDR_SEL() register without
> relying on some hardcoded values:
> 
> 	writel(original_address | EBU_ADDR_SEL_REGEN |
> 	       EBU_ADDR_COMP_BITS(num_comp_bits),
> 	       ebu_host->ebu + EBU_ADDR_SEL(csid));
> 
> That's quite important if we want to merge the xway NAND driver with
> this one.

Thanks! , for the optimized and made it generic way, will update in the 
next patch-set.

Regards
Vadivel
>
Ramuthevar, Vadivel MuruganX May 4, 2020, 1:58 a.m. UTC | #19
Hi Boris,

   Thank you very much for the review comments and your time...

On 30/4/2020 9:01 pm, Boris Brezillon wrote:
> On Thu, 30 Apr 2020 14:36:00 +0200
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
>> On Thu, 30 Apr 2020 17:07:03 +0800
>> "Ramuthevar, Vadivel MuruganX"
>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>
>>>>>> The question is, is it the same value we have in nand_pa or it is
>>>>>> different?
>>>>>>         
>>>>> Different address which is 0xE1400000 NAND_BASE_PHY address.
>>>>
>>>> Then why didn't you tell me they didn't match when I suggested to pass
>>>
>>> sorry, because you keep asking nand_pa after that only I realized that.
>>>    
>>>> nand_pa? So now the question is, what does this address represent?
>>>
>>>                  EBU-MODULE
>>>    _________     _______________________
>>> |         |   |            |NAND CTRL  |
>>> | FPI BUS |==>| CS0(0x174) | 0xE100    ( 0xE14/0xE1C) NAND_PHY_BASE
>>> |_________|   |_CS1(0x17C)_|__________ |
>>>
>>> EBU_CONRTROLLER_BASE : 0xE0F0_0000
>>> HSNAND_BASE: 0xE100_0000
>>> NAND_CS0: 0xE140_0000
>>> NAND_CS1: 0xE1C0_0000
>>>
>>> MEM_REGION_BASE_CS0: 0x17400 (internal to ebu controller )
>>> MEM_REGION_BASE_CS1: 0x17C00
>>>    
>>
>> Hm, I wonder if we shouldn't use a 'ranges' property to describe this
>> address translation. Something like
>>
>> 	ebu@xxx {
>> 		ranges = <0x17400000 0xe1400000 0x1000>,
>> 			 <0x17c00000 0xe1c00000 0x1000>;
>> 		reg = <0x17400000>, <0x17c00000>;
>> 		reg-names = "cs-0", "cs-1";
>> 	}
>>
>> The translated address (0xE1X00000) will be available in res->start,
>> and the non-translated one (0x17X00000) can be retrieved with
>> of_get_address(). All you'd have to do then would be calculate the
>> mask:
>>
>> 	mask = (translated_address & original_address) >> 22;
>> 	num_comp_bits = fls(mask);
>> 	WARN_ON(mask != GENMASK(num_comp_bits - 1, 0));
>>
>> Which allows you to properly set the ADDR_SEL() register without
>> relying on some hardcoded values:
>>
>> 	writel(original_address | EBU_ADDR_SEL_REGEN |
>> 	       EBU_ADDR_COMP_BITS(num_comp_bits),
>> 	       ebu_host->ebu + EBU_ADDR_SEL(csid));
>>
>> That's quite important if we want to merge the xway NAND driver with
>> this one.
> 
> Looks like the translation is done at the FPI bus declaration level (see
> [1]). We really need to see the big picture to take a wise decision
> about the bindings. Would you mind pasting your dsti/dts files
> somewhere? It feels like the NAND controller is a sub-part of a more
> generic 'memory' controller, in which case the NAND controller should be
> declared as a child of this generic memory bus (called localbus in [1],
> but maybe EBU is more accurate).
> 
> [1]https://github.com/xieyaxiongfly/Atheros_CSI_tool_OpenWRT_src/blob/master/target/linux/lantiq/files-4.14/arch/mips/boot/dts/vr9.dtsi#L162

for the ebu-nand node in the dts file.

       ebu_nand: ebu_nand@e0f00000 {
                  compatible = "intel,lgm-ebu-nand";
                  reg = <0xe0f00000 0x100 //EBU_NAND controller
                   0xe1000000 0x300       //NAND ECC Extension access
                  0xe1400000 0x80000
                  0xe1c00000 0x10000>;
                  reg-names = "ebunand", "hsnand", "nand_cs0", "nand_cs1";
                  dmas = <&dma0 8>, <&dma0 9>;
                  dma-names = "ebu_rx", "ebu_tx";
                  clocks =  <&cgu0 LGM_GCLK_EBU>;
          };


	 &ebu_nand {
	         status = "disabled";
	        nand,cs = <1>;
	        nand-ecc-mode = "hw";
	        pinctrl-names = "default";
	        pinctrl-0 = <&ebu_nand_base &ebu_cs1>;
	};

This is not comes under fpi in our devicetree.

Regards
Vadivel
>
Ramuthevar, Vadivel MuruganX May 4, 2020, 2:02 a.m. UTC | #20
Hi Boris,

On 30/4/2020 9:01 pm, Boris Brezillon wrote:
> On Thu, 30 Apr 2020 14:36:00 +0200
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
>> On Thu, 30 Apr 2020 17:07:03 +0800
>> "Ramuthevar, Vadivel MuruganX"
>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>
>>>>>> The question is, is it the same value we have in nand_pa or it is
>>>>>> different?
>>>>>>         
>>>>> Different address which is 0xE1400000 NAND_BASE_PHY address.
>>>>
>>>> Then why didn't you tell me they didn't match when I suggested to pass
>>>
>>> sorry, because you keep asking nand_pa after that only I realized that.
>>>    
>>>> nand_pa? So now the question is, what does this address represent?
>>>
>>>                  EBU-MODULE
>>>    _________     _______________________
>>> |         |   |            |NAND CTRL  |
>>> | FPI BUS |==>| CS0(0x174) | 0xE100    ( 0xE14/0xE1C) NAND_PHY_BASE
>>> |_________|   |_CS1(0x17C)_|__________ |
>>>
>>> EBU_CONRTROLLER_BASE : 0xE0F0_0000
>>> HSNAND_BASE: 0xE100_0000
>>> NAND_CS0: 0xE140_0000
>>> NAND_CS1: 0xE1C0_0000
>>>
>>> MEM_REGION_BASE_CS0: 0x17400 (internal to ebu controller )
>>> MEM_REGION_BASE_CS1: 0x17C00
>>>    
>>
>> Hm, I wonder if we shouldn't use a 'ranges' property to describe this
>> address translation. Something like
>>
>> 	ebu@xxx {
>> 		ranges = <0x17400000 0xe1400000 0x1000>,
>> 			 <0x17c00000 0xe1c00000 0x1000>;
>> 		reg = <0x17400000>, <0x17c00000>;
>> 		reg-names = "cs-0", "cs-1";
>> 	}
>>
>> The translated address (0xE1X00000) will be available in res->start,
>> and the non-translated one (0x17X00000) can be retrieved with
>> of_get_address(). All you'd have to do then would be calculate the
>> mask:
>>
>> 	mask = (translated_address & original_address) >> 22;
>> 	num_comp_bits = fls(mask);
>> 	WARN_ON(mask != GENMASK(num_comp_bits - 1, 0));
>>
>> Which allows you to properly set the ADDR_SEL() register without
>> relying on some hardcoded values:
>>
>> 	writel(original_address | EBU_ADDR_SEL_REGEN |
>> 	       EBU_ADDR_COMP_BITS(num_comp_bits),
>> 	       ebu_host->ebu + EBU_ADDR_SEL(csid));
>>
>> That's quite important if we want to merge the xway NAND driver with
>> this one.
> 
> Looks like the translation is done at the FPI bus declaration level (see
> [1]). We really need to see the big picture to take a wise decision
> about the bindings. Would you mind pasting your dsti/dts files
> somewhere? It feels like the NAND controller is a sub-part of a more
> generic 'memory' controller, in which case the NAND controller should be
> declared as a child of this generic memory bus (called localbus in [1],
> but maybe EBU is more accurate).
> 
> [1]https://github.com/xieyaxiongfly/Atheros_CSI_tool_OpenWRT_src/blob/master/target/linux/lantiq/files-4.14/arch/mips/boot/dts/vr9.dtsi#L162


  ebu_nand: ebu_nand@e0f00000 {
                  compatible = "intel,lgm-ebu-nand";
                  reg = <0xe0f00000 0x100
                  0xe1000000 0x300
                  0xe1400000 0x80000
                  0xe1c00000 0x10000>;
                  reg-names = "ebunand", "hsnand", "nand_cs0", nand_cs1";
                  dmas = <&dma0 8>, <&dma0 9>;
                  dma-names = "ebu_rx", "ebu_tx";
                  clocks =  <&cgu0 LGM_GCLK_EBU>;
          };


	 &ebu_nand {
	         status = "disabled";
	        nand,cs = <1>;
	        nand-ecc-mode = "hw";
	        pinctrl-names = "default";
	        pinctrl-0 = <&ebu_nand_base &ebu_cs1>;
	};

>
Boris Brezillon May 4, 2020, 7:08 a.m. UTC | #21
On Mon, 4 May 2020 10:02:35 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:

> Hi Boris,
> 
> On 30/4/2020 9:01 pm, Boris Brezillon wrote:
> > On Thu, 30 Apr 2020 14:36:00 +0200
> > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >   
> >> On Thu, 30 Apr 2020 17:07:03 +0800
> >> "Ramuthevar, Vadivel MuruganX"
> >> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> >>  
> >>>>>> The question is, is it the same value we have in nand_pa or it is
> >>>>>> different?
> >>>>>>           
> >>>>> Different address which is 0xE1400000 NAND_BASE_PHY address.  
> >>>>
> >>>> Then why didn't you tell me they didn't match when I suggested to pass  
> >>>
> >>> sorry, because you keep asking nand_pa after that only I realized that.
> >>>      
> >>>> nand_pa? So now the question is, what does this address represent?  
> >>>
> >>>                  EBU-MODULE
> >>>    _________     _______________________
> >>> |         |   |            |NAND CTRL  |
> >>> | FPI BUS |==>| CS0(0x174) | 0xE100    ( 0xE14/0xE1C) NAND_PHY_BASE
> >>> |_________|   |_CS1(0x17C)_|__________ |
> >>>
> >>> EBU_CONRTROLLER_BASE : 0xE0F0_0000
> >>> HSNAND_BASE: 0xE100_0000
> >>> NAND_CS0: 0xE140_0000
> >>> NAND_CS1: 0xE1C0_0000
> >>>
> >>> MEM_REGION_BASE_CS0: 0x17400 (internal to ebu controller )
> >>> MEM_REGION_BASE_CS1: 0x17C00
> >>>      
> >>
> >> Hm, I wonder if we shouldn't use a 'ranges' property to describe this
> >> address translation. Something like
> >>
> >> 	ebu@xxx {
> >> 		ranges = <0x17400000 0xe1400000 0x1000>,
> >> 			 <0x17c00000 0xe1c00000 0x1000>;
> >> 		reg = <0x17400000>, <0x17c00000>;
> >> 		reg-names = "cs-0", "cs-1";
> >> 	}
> >>
> >> The translated address (0xE1X00000) will be available in res->start,
> >> and the non-translated one (0x17X00000) can be retrieved with
> >> of_get_address(). All you'd have to do then would be calculate the
> >> mask:
> >>
> >> 	mask = (translated_address & original_address) >> 22;
> >> 	num_comp_bits = fls(mask);
> >> 	WARN_ON(mask != GENMASK(num_comp_bits - 1, 0));
> >>
> >> Which allows you to properly set the ADDR_SEL() register without
> >> relying on some hardcoded values:
> >>
> >> 	writel(original_address | EBU_ADDR_SEL_REGEN |
> >> 	       EBU_ADDR_COMP_BITS(num_comp_bits),
> >> 	       ebu_host->ebu + EBU_ADDR_SEL(csid));
> >>
> >> That's quite important if we want to merge the xway NAND driver with
> >> this one.  
> > 
> > Looks like the translation is done at the FPI bus declaration level (see
> > [1]). We really need to see the big picture to take a wise decision
> > about the bindings. Would you mind pasting your dsti/dts files
> > somewhere? It feels like the NAND controller is a sub-part of a more
> > generic 'memory' controller, in which case the NAND controller should be
> > declared as a child of this generic memory bus (called localbus in [1],
> > but maybe EBU is more accurate).
> > 
> > [1]https://github.com/xieyaxiongfly/Atheros_CSI_tool_OpenWRT_src/blob/master/target/linux/lantiq/files-4.14/arch/mips/boot/dts/vr9.dtsi#L162  
> 
> 
>   ebu_nand: ebu_nand@e0f00000 {
>                   compatible = "intel,lgm-ebu-nand";
>                   reg = <0xe0f00000 0x100
>                   0xe1000000 0x300
>                   0xe1400000 0x80000
>                   0xe1c00000 0x10000>;
>                   reg-names = "ebunand", "hsnand", "nand_cs0", nand_cs1";
>                   dmas = <&dma0 8>, <&dma0 9>;
>                   dma-names = "ebu_rx", "ebu_tx";
>                   clocks =  <&cgu0 LGM_GCLK_EBU>;
>           };
> 
> 
> 	 &ebu_nand {
> 	         status = "disabled";
> 	        nand,cs = <1>;
> 	        nand-ecc-mode = "hw";
> 	        pinctrl-names = "default";
> 	        pinctrl-0 = <&ebu_nand_base &ebu_cs1>;
> 	};
> 
> >   

Ok. If I understand the SoC topology correctly it should actually be
something like that:

{
	...
	fpi@xxxxx {
		compatible = "intel,lgm-fpi", "simple-bus";

		/* You might have other ranges to define here */
		ranges = <0x16000000 0xe0000000 0x1000000>;

		...

		ebu@xxxx {
			compatible = "intel,lgm-ebu", "simple-bus";
			ranges;
			pinctrl-names = "default";
			pinctrl-0 = <&ebu_nand_base &ebu_cs1>;

			/*
			 * Add your PCI and NOR controller definitions
			 * here.
			 */
			...
			
			nand-controller@16f00000 {
				compatible = "intel,lgm-ebu-nand-controller";
				reg = <0x16f00000 0x100>,
				      <0x17000000 0x300>,
				      <0x17400000 0x80000>,
				      <0x17c00000 0x10000>;
				reg-names = "ebunand", "hsnand",
					     "cs0", cs1";
				#address-cells = <1>;
				#size-cells = <0>;

				/*
				 * I'm not sure if those belong here: if the
				 * DMA channels and clocks are shared by all
				 * controllers attached to the EBU they should
				 * be moved to the EBU node.
				 */
				dmas = <&dma0 8>, <&dma0 9>;
				dma-names = "rx", "tx";
				clocks =  <&cgu0 LGM_GCLK_EBU>;

				nand@1 {
					reg = <1>;
					nand-ecc-mode = "hw";
				}
			}
		}
	}
}
Ramuthevar, Vadivel MuruganX May 4, 2020, 7:15 a.m. UTC | #22
Hi Boris,

   Thank you very much for the prompt review and suggestions...

On 4/5/2020 3:08 pm, Boris Brezillon wrote:
> On Mon, 4 May 2020 10:02:35 +0800
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@linux.intel.com>  wrote:
> 
>> Hi Boris,
>>
>> On 30/4/2020 9:01 pm, Boris Brezillon wrote:
>>> On Thu, 30 Apr 2020 14:36:00 +0200
>>> Boris Brezillon<boris.brezillon@collabora.com>  wrote:
>>>    
>>>> On Thu, 30 Apr 2020 17:07:03 +0800
>>>> "Ramuthevar, Vadivel MuruganX"
>>>> <vadivel.muruganx.ramuthevar@linux.intel.com>  wrote:
>>>>   
>>>>>>>> The question is, is it the same value we have in nand_pa or it is
>>>>>>>> different?
>>>>>>>>            
>>>>>>> Different address which is 0xE1400000 NAND_BASE_PHY address.
>>>>>> Then why didn't you tell me they didn't match when I suggested to pass
>>>>> sorry, because you keep asking nand_pa after that only I realized that.
>>>>>       
>>>>>> nand_pa? So now the question is, what does this address represent?
>>>>>                   EBU-MODULE
>>>>>     _________     _______________________
>>>>> |         |   |            |NAND CTRL  |
>>>>> | FPI BUS |==>| CS0(0x174) | 0xE100    ( 0xE14/0xE1C) NAND_PHY_BASE
>>>>> |_________|   |_CS1(0x17C)_|__________ |
>>>>>
>>>>> EBU_CONRTROLLER_BASE : 0xE0F0_0000
>>>>> HSNAND_BASE: 0xE100_0000
>>>>> NAND_CS0: 0xE140_0000
>>>>> NAND_CS1: 0xE1C0_0000
>>>>>
>>>>> MEM_REGION_BASE_CS0: 0x17400 (internal to ebu controller )
>>>>> MEM_REGION_BASE_CS1: 0x17C00
>>>>>       
>>>> Hm, I wonder if we shouldn't use a 'ranges' property to describe this
>>>> address translation. Something like
>>>>
>>>> 	ebu@xxx {
>>>> 		ranges = <0x17400000 0xe1400000 0x1000>,
>>>> 			 <0x17c00000 0xe1c00000 0x1000>;
>>>> 		reg = <0x17400000>, <0x17c00000>;
>>>> 		reg-names = "cs-0", "cs-1";
>>>> 	}
>>>>
>>>> The translated address (0xE1X00000) will be available in res->start,
>>>> and the non-translated one (0x17X00000) can be retrieved with
>>>> of_get_address(). All you'd have to do then would be calculate the
>>>> mask:
>>>>
>>>> 	mask = (translated_address & original_address) >> 22;
>>>> 	num_comp_bits = fls(mask);
>>>> 	WARN_ON(mask != GENMASK(num_comp_bits - 1, 0));
>>>>
>>>> Which allows you to properly set the ADDR_SEL() register without
>>>> relying on some hardcoded values:
>>>>
>>>> 	writel(original_address | EBU_ADDR_SEL_REGEN |
>>>> 	       EBU_ADDR_COMP_BITS(num_comp_bits),
>>>> 	       ebu_host->ebu + EBU_ADDR_SEL(csid));
>>>>
>>>> That's quite important if we want to merge the xway NAND driver with
>>>> this one.
>>> Looks like the translation is done at the FPI bus declaration level (see
>>> [1]). We really need to see the big picture to take a wise decision
>>> about the bindings. Would you mind pasting your dsti/dts files
>>> somewhere? It feels like the NAND controller is a sub-part of a more
>>> generic 'memory' controller, in which case the NAND controller should be
>>> declared as a child of this generic memory bus (called localbus in [1],
>>> but maybe EBU is more accurate).
>>>
>>> [1]https://github.com/xieyaxiongfly/Atheros_CSI_tool_OpenWRT_src/blob/master/target/linux/lantiq/files-4.14/arch/mips/boot/dts/vr9.dtsi#L162   
>>
>>    ebu_nand: ebu_nand@e0f00000 {
>>                    compatible = "intel,lgm-ebu-nand";
>>                    reg = <0xe0f00000 0x100
>>                    0xe1000000 0x300
>>                    0xe1400000 0x80000
>>                    0xe1c00000 0x10000>;
>>                    reg-names = "ebunand", "hsnand", "nand_cs0", nand_cs1";
>>                    dmas = <&dma0 8>, <&dma0 9>;
>>                    dma-names = "ebu_rx", "ebu_tx";
>>                    clocks =  <&cgu0 LGM_GCLK_EBU>;
>>            };
>>
>>
>> 	 &ebu_nand {
>> 	         status = "disabled";
>> 	        nand,cs = <1>;
>> 	        nand-ecc-mode = "hw";
>> 	        pinctrl-names = "default";
>> 	        pinctrl-0 = <&ebu_nand_base &ebu_cs1>;
>> 	};
>>
>>>    
> Ok. If I understand the SoC topology correctly it should actually be
> something like that:
> 
> {
> 	...
> 	fpi@xxxxx {
> 		compatible = "intel,lgm-fpi", "simple-bus";
> 
> 		/* You might have other ranges to define here */
> 		ranges = <0x16000000 0xe0000000 0x1000000>;
> 
> 		...

Sorry, we do not have fpi tree node in our dts/dtsi file instead we have 
the below one.. , that also not included the major peripherals 
controllers node.
         /* Special part from CPU core */
         core: core {
                 compatible = "intel,core", "simple-bus";
                 #address-cells = <1>;
                 #size-cells = <1>;
                 ranges;

                 ioapic1: interrupt-controller@fec00000 {
                         #interrupt-cells = <2>;
                         #address-cells = <0>;
                         compatible = "intel,ce4100-ioapic";
                         interrupt-controller;
                         reg = <0xfec00000 0x1000>;
                         nr_entries = <256>;
                 };

                 hpet: timer@fed00000 {
                         compatible = "intel,ce4100-hpet";
                         reg = <0xfed00000 0x400>;
                 };

                 lapic0: interrupt-controller@fee00000 {
                         compatible = "intel,ce4100-lapic";
                         reg = <0xfee00000 0x1000>;
                         no_pic_mode;
                 };
         };

other than this, rest all in independent node .

Thanks!

Regards
Vadivel
Boris Brezillon May 4, 2020, 7:17 a.m. UTC | #23
On Mon, 4 May 2020 15:15:08 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:

> Hi Boris,
> 
>    Thank you very much for the prompt review and suggestions...
> 
> On 4/5/2020 3:08 pm, Boris Brezillon wrote:
> > On Mon, 4 May 2020 10:02:35 +0800
> > "Ramuthevar, Vadivel MuruganX"
> > <vadivel.muruganx.ramuthevar@linux.intel.com>  wrote:
> >   
> >> Hi Boris,
> >>
> >> On 30/4/2020 9:01 pm, Boris Brezillon wrote:  
> >>> On Thu, 30 Apr 2020 14:36:00 +0200
> >>> Boris Brezillon<boris.brezillon@collabora.com>  wrote:
> >>>      
> >>>> On Thu, 30 Apr 2020 17:07:03 +0800
> >>>> "Ramuthevar, Vadivel MuruganX"
> >>>> <vadivel.muruganx.ramuthevar@linux.intel.com>  wrote:
> >>>>     
> >>>>>>>> The question is, is it the same value we have in nand_pa or it is
> >>>>>>>> different?
> >>>>>>>>              
> >>>>>>> Different address which is 0xE1400000 NAND_BASE_PHY address.  
> >>>>>> Then why didn't you tell me they didn't match when I suggested to pass  
> >>>>> sorry, because you keep asking nand_pa after that only I realized that.
> >>>>>         
> >>>>>> nand_pa? So now the question is, what does this address represent?  
> >>>>>                   EBU-MODULE
> >>>>>     _________     _______________________
> >>>>> |         |   |            |NAND CTRL  |
> >>>>> | FPI BUS |==>| CS0(0x174) | 0xE100    ( 0xE14/0xE1C) NAND_PHY_BASE
> >>>>> |_________|   |_CS1(0x17C)_|__________ |
> >>>>>
> >>>>> EBU_CONRTROLLER_BASE : 0xE0F0_0000
> >>>>> HSNAND_BASE: 0xE100_0000
> >>>>> NAND_CS0: 0xE140_0000
> >>>>> NAND_CS1: 0xE1C0_0000
> >>>>>
> >>>>> MEM_REGION_BASE_CS0: 0x17400 (internal to ebu controller )
> >>>>> MEM_REGION_BASE_CS1: 0x17C00
> >>>>>         
> >>>> Hm, I wonder if we shouldn't use a 'ranges' property to describe this
> >>>> address translation. Something like
> >>>>
> >>>> 	ebu@xxx {
> >>>> 		ranges = <0x17400000 0xe1400000 0x1000>,
> >>>> 			 <0x17c00000 0xe1c00000 0x1000>;
> >>>> 		reg = <0x17400000>, <0x17c00000>;
> >>>> 		reg-names = "cs-0", "cs-1";
> >>>> 	}
> >>>>
> >>>> The translated address (0xE1X00000) will be available in res->start,
> >>>> and the non-translated one (0x17X00000) can be retrieved with
> >>>> of_get_address(). All you'd have to do then would be calculate the
> >>>> mask:
> >>>>
> >>>> 	mask = (translated_address & original_address) >> 22;
> >>>> 	num_comp_bits = fls(mask);
> >>>> 	WARN_ON(mask != GENMASK(num_comp_bits - 1, 0));
> >>>>
> >>>> Which allows you to properly set the ADDR_SEL() register without
> >>>> relying on some hardcoded values:
> >>>>
> >>>> 	writel(original_address | EBU_ADDR_SEL_REGEN |
> >>>> 	       EBU_ADDR_COMP_BITS(num_comp_bits),
> >>>> 	       ebu_host->ebu + EBU_ADDR_SEL(csid));
> >>>>
> >>>> That's quite important if we want to merge the xway NAND driver with
> >>>> this one.  
> >>> Looks like the translation is done at the FPI bus declaration level (see
> >>> [1]). We really need to see the big picture to take a wise decision
> >>> about the bindings. Would you mind pasting your dsti/dts files
> >>> somewhere? It feels like the NAND controller is a sub-part of a more
> >>> generic 'memory' controller, in which case the NAND controller should be
> >>> declared as a child of this generic memory bus (called localbus in [1],
> >>> but maybe EBU is more accurate).
> >>>
> >>> [1]https://github.com/xieyaxiongfly/Atheros_CSI_tool_OpenWRT_src/blob/master/target/linux/lantiq/files-4.14/arch/mips/boot/dts/vr9.dtsi#L162     
> >>
> >>    ebu_nand: ebu_nand@e0f00000 {
> >>                    compatible = "intel,lgm-ebu-nand";
> >>                    reg = <0xe0f00000 0x100
> >>                    0xe1000000 0x300
> >>                    0xe1400000 0x80000
> >>                    0xe1c00000 0x10000>;
> >>                    reg-names = "ebunand", "hsnand", "nand_cs0", nand_cs1";
> >>                    dmas = <&dma0 8>, <&dma0 9>;
> >>                    dma-names = "ebu_rx", "ebu_tx";
> >>                    clocks =  <&cgu0 LGM_GCLK_EBU>;
> >>            };
> >>
> >>
> >> 	 &ebu_nand {
> >> 	         status = "disabled";
> >> 	        nand,cs = <1>;
> >> 	        nand-ecc-mode = "hw";
> >> 	        pinctrl-names = "default";
> >> 	        pinctrl-0 = <&ebu_nand_base &ebu_cs1>;
> >> 	};
> >>  
> >>>      
> > Ok. If I understand the SoC topology correctly it should actually be
> > something like that:
> > 
> > {
> > 	...
> > 	fpi@xxxxx {
> > 		compatible = "intel,lgm-fpi", "simple-bus";
> > 
> > 		/* You might have other ranges to define here */
> > 		ranges = <0x16000000 0xe0000000 0x1000000>;
> > 
> > 		...  
> 
> Sorry, we do not have fpi tree node in our dts/dtsi file instead we have 
> the below one.. , that also not included the major peripherals 
> controllers node.
>          /* Special part from CPU core */
>          core: core {
>                  compatible = "intel,core", "simple-bus";
>                  #address-cells = <1>;
>                  #size-cells = <1>;
>                  ranges;
> 
>                  ioapic1: interrupt-controller@fec00000 {
>                          #interrupt-cells = <2>;
>                          #address-cells = <0>;
>                          compatible = "intel,ce4100-ioapic";
>                          interrupt-controller;
>                          reg = <0xfec00000 0x1000>;
>                          nr_entries = <256>;
>                  };
> 
>                  hpet: timer@fed00000 {
>                          compatible = "intel,ce4100-hpet";
>                          reg = <0xfed00000 0x400>;
>                  };
> 
>                  lapic0: interrupt-controller@fee00000 {
>                          compatible = "intel,ce4100-lapic";
>                          reg = <0xfee00000 0x1000>;
>                          no_pic_mode;
>                  };
>          };
> 
> other than this, rest all in independent node .

But you do have an FPI bus, right? If this is the case it should be
represented. Or is the "intel,core" bus actually the FPI bus that you
named differently?
Ramuthevar, Vadivel MuruganX May 4, 2020, 8:50 a.m. UTC | #24
Hi Boris,

On 4/5/2020 3:17 pm, Boris Brezillon wrote:
> On Mon, 4 May 2020 15:15:08 +0800
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> 
>> Hi Boris,
>>
>>     Thank you very much for the prompt review and suggestions...
>>
>> On 4/5/2020 3:08 pm, Boris Brezillon wrote:
>>> On Mon, 4 May 2020 10:02:35 +0800
>>> "Ramuthevar, Vadivel MuruganX"
>>> <vadivel.muruganx.ramuthevar@linux.intel.com>  wrote:
>>>    
>>>> Hi Boris,
>>>>
>>>> On 30/4/2020 9:01 pm, Boris Brezillon wrote:
>>>>> On Thu, 30 Apr 2020 14:36:00 +0200
>>>>> Boris Brezillon<boris.brezillon@collabora.com>  wrote:
>>>>>       
>>>>>> On Thu, 30 Apr 2020 17:07:03 +0800
>>>>>> "Ramuthevar, Vadivel MuruganX"
>>>>>> <vadivel.muruganx.ramuthevar@linux.intel.com>  wrote:
>>>>>>      
>>>>>>>>>> The question is, is it the same value we have in nand_pa or it is
>>>>>>>>>> different?
>>>>>>>>>>               
>>>>>>>>> Different address which is 0xE1400000 NAND_BASE_PHY address.
>>>>>>>> Then why didn't you tell me they didn't match when I suggested to pass
>>>>>>> sorry, because you keep asking nand_pa after that only I realized that.
>>>>>>>          
>>>>>>>> nand_pa? So now the question is, what does this address represent?
>>>>>>>                    EBU-MODULE
>>>>>>>      _________     _______________________
>>>>>>> |         |   |            |NAND CTRL  |
>>>>>>> | FPI BUS |==>| CS0(0x174) | 0xE100    ( 0xE14/0xE1C) NAND_PHY_BASE
>>>>>>> |_________|   |_CS1(0x17C)_|__________ |
>>>>>>>
>>>>>>> EBU_CONRTROLLER_BASE : 0xE0F0_0000
>>>>>>> HSNAND_BASE: 0xE100_0000
>>>>>>> NAND_CS0: 0xE140_0000
>>>>>>> NAND_CS1: 0xE1C0_0000
>>>>>>>
>>>>>>> MEM_REGION_BASE_CS0: 0x17400 (internal to ebu controller )
>>>>>>> MEM_REGION_BASE_CS1: 0x17C00
>>>>>>>          
>>>>>> Hm, I wonder if we shouldn't use a 'ranges' property to describe this
>>>>>> address translation. Something like
>>>>>>
>>>>>> 	ebu@xxx {
>>>>>> 		ranges = <0x17400000 0xe1400000 0x1000>,
>>>>>> 			 <0x17c00000 0xe1c00000 0x1000>;
>>>>>> 		reg = <0x17400000>, <0x17c00000>;
>>>>>> 		reg-names = "cs-0", "cs-1";
>>>>>> 	}
>>>>>>
>>>>>> The translated address (0xE1X00000) will be available in res->start,
>>>>>> and the non-translated one (0x17X00000) can be retrieved with
>>>>>> of_get_address(). All you'd have to do then would be calculate the
>>>>>> mask:
>>>>>>
>>>>>> 	mask = (translated_address & original_address) >> 22;
>>>>>> 	num_comp_bits = fls(mask);
>>>>>> 	WARN_ON(mask != GENMASK(num_comp_bits - 1, 0));
>>>>>>
>>>>>> Which allows you to properly set the ADDR_SEL() register without
>>>>>> relying on some hardcoded values:
>>>>>>
>>>>>> 	writel(original_address | EBU_ADDR_SEL_REGEN |
>>>>>> 	       EBU_ADDR_COMP_BITS(num_comp_bits),
>>>>>> 	       ebu_host->ebu + EBU_ADDR_SEL(csid));
>>>>>>
>>>>>> That's quite important if we want to merge the xway NAND driver with
>>>>>> this one.
>>>>> Looks like the translation is done at the FPI bus declaration level (see
>>>>> [1]). We really need to see the big picture to take a wise decision
>>>>> about the bindings. Would you mind pasting your dsti/dts files
>>>>> somewhere? It feels like the NAND controller is a sub-part of a more
>>>>> generic 'memory' controller, in which case the NAND controller should be
>>>>> declared as a child of this generic memory bus (called localbus in [1],
>>>>> but maybe EBU is more accurate).
>>>>>
>>>>> [1]https://github.com/xieyaxiongfly/Atheros_CSI_tool_OpenWRT_src/blob/master/target/linux/lantiq/files-4.14/arch/mips/boot/dts/vr9.dtsi#L162
>>>>
>>>>     ebu_nand: ebu_nand@e0f00000 {
>>>>                     compatible = "intel,lgm-ebu-nand";
>>>>                     reg = <0xe0f00000 0x100
>>>>                     0xe1000000 0x300
>>>>                     0xe1400000 0x80000
>>>>                     0xe1c00000 0x10000>;
>>>>                     reg-names = "ebunand", "hsnand", "nand_cs0", nand_cs1";
>>>>                     dmas = <&dma0 8>, <&dma0 9>;
>>>>                     dma-names = "ebu_rx", "ebu_tx";
>>>>                     clocks =  <&cgu0 LGM_GCLK_EBU>;
>>>>             };
>>>>
>>>>
>>>> 	 &ebu_nand {
>>>> 	         status = "disabled";
>>>> 	        nand,cs = <1>;
>>>> 	        nand-ecc-mode = "hw";
>>>> 	        pinctrl-names = "default";
>>>> 	        pinctrl-0 = <&ebu_nand_base &ebu_cs1>;
>>>> 	};
>>>>   
>>>>>       
>>> Ok. If I understand the SoC topology correctly it should actually be
>>> something like that:
>>>
>>> {
>>> 	...
>>> 	fpi@xxxxx {
>>> 		compatible = "intel,lgm-fpi", "simple-bus";
>>>
>>> 		/* You might have other ranges to define here */
>>> 		ranges = <0x16000000 0xe0000000 0x1000000>;
>>>
>>> 		...
>>
>> Sorry, we do not have fpi tree node in our dts/dtsi file instead we have
>> the below one.. , that also not included the major peripherals
>> controllers node.
>>           /* Special part from CPU core */
>>           core: core {
>>                   compatible = "intel,core", "simple-bus";
>>                   #address-cells = <1>;
>>                   #size-cells = <1>;
>>                   ranges;
>>
>>                   ioapic1: interrupt-controller@fec00000 {
>>                           #interrupt-cells = <2>;
>>                           #address-cells = <0>;
>>                           compatible = "intel,ce4100-ioapic";
>>                           interrupt-controller;
>>                           reg = <0xfec00000 0x1000>;
>>                           nr_entries = <256>;
>>                   };
>>
>>                   hpet: timer@fed00000 {
>>                           compatible = "intel,ce4100-hpet";
>>                           reg = <0xfed00000 0x400>;
>>                   };
>>
>>                   lapic0: interrupt-controller@fee00000 {
>>                           compatible = "intel,ce4100-lapic";
>>                           reg = <0xfee00000 0x1000>;
>>                           no_pic_mode;
>>                   };
>>           };
>>
>> other than this, rest all in independent node .
> 
> But you do have an FPI bus, right? If this is the case it should be
> represented.

Yes, FPI bus is slave to core which connects all the peripherals.

  Or is the "intel,core" bus actually the FPI bus that you
> named differently?

FPI slave bus connects to core bus by OCP bridge, so here it is named 
FPI bus, but SW perspective didn't have root tree which has all 
sub-nodes, as of now each peripheral has its own node.

Regards
Vadivel
>
Boris Brezillon May 4, 2020, 8:58 a.m. UTC | #25
On Mon, 4 May 2020 16:50:08 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:

> Hi Boris,
> 
> On 4/5/2020 3:17 pm, Boris Brezillon wrote:
> > On Mon, 4 May 2020 15:15:08 +0800
> > "Ramuthevar, Vadivel MuruganX"
> > <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> >   
> >> Hi Boris,
> >>
> >>     Thank you very much for the prompt review and suggestions...
> >>
> >> On 4/5/2020 3:08 pm, Boris Brezillon wrote:  
> >>> On Mon, 4 May 2020 10:02:35 +0800
> >>> "Ramuthevar, Vadivel MuruganX"
> >>> <vadivel.muruganx.ramuthevar@linux.intel.com>  wrote:
> >>>      
> >>>> Hi Boris,
> >>>>
> >>>> On 30/4/2020 9:01 pm, Boris Brezillon wrote:  
> >>>>> On Thu, 30 Apr 2020 14:36:00 +0200
> >>>>> Boris Brezillon<boris.brezillon@collabora.com>  wrote:
> >>>>>         
> >>>>>> On Thu, 30 Apr 2020 17:07:03 +0800
> >>>>>> "Ramuthevar, Vadivel MuruganX"
> >>>>>> <vadivel.muruganx.ramuthevar@linux.intel.com>  wrote:
> >>>>>>        
> >>>>>>>>>> The question is, is it the same value we have in nand_pa or it is
> >>>>>>>>>> different?
> >>>>>>>>>>                 
> >>>>>>>>> Different address which is 0xE1400000 NAND_BASE_PHY address.  
> >>>>>>>> Then why didn't you tell me they didn't match when I suggested to pass  
> >>>>>>> sorry, because you keep asking nand_pa after that only I realized that.
> >>>>>>>            
> >>>>>>>> nand_pa? So now the question is, what does this address represent?  
> >>>>>>>                    EBU-MODULE
> >>>>>>>      _________     _______________________
> >>>>>>> |         |   |            |NAND CTRL  |
> >>>>>>> | FPI BUS |==>| CS0(0x174) | 0xE100    ( 0xE14/0xE1C) NAND_PHY_BASE
> >>>>>>> |_________|   |_CS1(0x17C)_|__________ |
> >>>>>>>
> >>>>>>> EBU_CONRTROLLER_BASE : 0xE0F0_0000
> >>>>>>> HSNAND_BASE: 0xE100_0000
> >>>>>>> NAND_CS0: 0xE140_0000
> >>>>>>> NAND_CS1: 0xE1C0_0000
> >>>>>>>
> >>>>>>> MEM_REGION_BASE_CS0: 0x17400 (internal to ebu controller )
> >>>>>>> MEM_REGION_BASE_CS1: 0x17C00
> >>>>>>>            
> >>>>>> Hm, I wonder if we shouldn't use a 'ranges' property to describe this
> >>>>>> address translation. Something like
> >>>>>>
> >>>>>> 	ebu@xxx {
> >>>>>> 		ranges = <0x17400000 0xe1400000 0x1000>,
> >>>>>> 			 <0x17c00000 0xe1c00000 0x1000>;
> >>>>>> 		reg = <0x17400000>, <0x17c00000>;
> >>>>>> 		reg-names = "cs-0", "cs-1";
> >>>>>> 	}
> >>>>>>
> >>>>>> The translated address (0xE1X00000) will be available in res->start,
> >>>>>> and the non-translated one (0x17X00000) can be retrieved with
> >>>>>> of_get_address(). All you'd have to do then would be calculate the
> >>>>>> mask:
> >>>>>>
> >>>>>> 	mask = (translated_address & original_address) >> 22;
> >>>>>> 	num_comp_bits = fls(mask);
> >>>>>> 	WARN_ON(mask != GENMASK(num_comp_bits - 1, 0));
> >>>>>>
> >>>>>> Which allows you to properly set the ADDR_SEL() register without
> >>>>>> relying on some hardcoded values:
> >>>>>>
> >>>>>> 	writel(original_address | EBU_ADDR_SEL_REGEN |
> >>>>>> 	       EBU_ADDR_COMP_BITS(num_comp_bits),
> >>>>>> 	       ebu_host->ebu + EBU_ADDR_SEL(csid));
> >>>>>>
> >>>>>> That's quite important if we want to merge the xway NAND driver with
> >>>>>> this one.  
> >>>>> Looks like the translation is done at the FPI bus declaration level (see
> >>>>> [1]). We really need to see the big picture to take a wise decision
> >>>>> about the bindings. Would you mind pasting your dsti/dts files
> >>>>> somewhere? It feels like the NAND controller is a sub-part of a more
> >>>>> generic 'memory' controller, in which case the NAND controller should be
> >>>>> declared as a child of this generic memory bus (called localbus in [1],
> >>>>> but maybe EBU is more accurate).
> >>>>>
> >>>>> [1]https://github.com/xieyaxiongfly/Atheros_CSI_tool_OpenWRT_src/blob/master/target/linux/lantiq/files-4.14/arch/mips/boot/dts/vr9.dtsi#L162  
> >>>>
> >>>>     ebu_nand: ebu_nand@e0f00000 {
> >>>>                     compatible = "intel,lgm-ebu-nand";
> >>>>                     reg = <0xe0f00000 0x100
> >>>>                     0xe1000000 0x300
> >>>>                     0xe1400000 0x80000
> >>>>                     0xe1c00000 0x10000>;
> >>>>                     reg-names = "ebunand", "hsnand", "nand_cs0", nand_cs1";
> >>>>                     dmas = <&dma0 8>, <&dma0 9>;
> >>>>                     dma-names = "ebu_rx", "ebu_tx";
> >>>>                     clocks =  <&cgu0 LGM_GCLK_EBU>;
> >>>>             };
> >>>>
> >>>>
> >>>> 	 &ebu_nand {
> >>>> 	         status = "disabled";
> >>>> 	        nand,cs = <1>;
> >>>> 	        nand-ecc-mode = "hw";
> >>>> 	        pinctrl-names = "default";
> >>>> 	        pinctrl-0 = <&ebu_nand_base &ebu_cs1>;
> >>>> 	};
> >>>>     
> >>>>>         
> >>> Ok. If I understand the SoC topology correctly it should actually be
> >>> something like that:
> >>>
> >>> {
> >>> 	...
> >>> 	fpi@xxxxx {
> >>> 		compatible = "intel,lgm-fpi", "simple-bus";
> >>>
> >>> 		/* You might have other ranges to define here */
> >>> 		ranges = <0x16000000 0xe0000000 0x1000000>;
> >>>
> >>> 		...  
> >>
> >> Sorry, we do not have fpi tree node in our dts/dtsi file instead we have
> >> the below one.. , that also not included the major peripherals
> >> controllers node.
> >>           /* Special part from CPU core */
> >>           core: core {
> >>                   compatible = "intel,core", "simple-bus";
> >>                   #address-cells = <1>;
> >>                   #size-cells = <1>;
> >>                   ranges;
> >>
> >>                   ioapic1: interrupt-controller@fec00000 {
> >>                           #interrupt-cells = <2>;
> >>                           #address-cells = <0>;
> >>                           compatible = "intel,ce4100-ioapic";
> >>                           interrupt-controller;
> >>                           reg = <0xfec00000 0x1000>;
> >>                           nr_entries = <256>;
> >>                   };
> >>
> >>                   hpet: timer@fed00000 {
> >>                           compatible = "intel,ce4100-hpet";
> >>                           reg = <0xfed00000 0x400>;
> >>                   };
> >>
> >>                   lapic0: interrupt-controller@fee00000 {
> >>                           compatible = "intel,ce4100-lapic";
> >>                           reg = <0xfee00000 0x1000>;
> >>                           no_pic_mode;
> >>                   };
> >>           };
> >>
> >> other than this, rest all in independent node .  
> > 
> > But you do have an FPI bus, right? If this is the case it should be
> > represented.  
> 
> Yes, FPI bus is slave to core which connects all the peripherals.
> 
>   Or is the "intel,core" bus actually the FPI bus that you
> > named differently?  
> 
> FPI slave bus connects to core bus by OCP bridge, so here it is named 
> FPI bus, but SW perspective didn't have root tree which has all 
> sub-nodes, as of now each peripheral has its own node.

Duh, not sure that's a good idea to hide that, especially since you
have to describe the address translation that happens when crossing the
FPI bus (the ranges thing I mentioned previously).
Ramuthevar, Vadivel MuruganX May 4, 2020, 9:17 a.m. UTC | #26
Hi Boris,

On 4/5/2020 4:58 pm, Boris Brezillon wrote:
> On Mon, 4 May 2020 16:50:08 +0800
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> 
>> Hi Boris,
>>
>> On 4/5/2020 3:17 pm, Boris Brezillon wrote:
>>> On Mon, 4 May 2020 15:15:08 +0800
>>> "Ramuthevar, Vadivel MuruganX"
>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>>    
>>>> Hi Boris,
>>>>
>>>>      Thank you very much for the prompt review and suggestions...
>>>>
>>>> On 4/5/2020 3:08 pm, Boris Brezillon wrote:
>>>>> On Mon, 4 May 2020 10:02:35 +0800
>>>>> "Ramuthevar, Vadivel MuruganX"
>>>>> <vadivel.muruganx.ramuthevar@linux.intel.com>  wrote:
>>>>>       
>>>>>> Hi Boris,
>>>>>>
>>>>>> On 30/4/2020 9:01 pm, Boris Brezillon wrote:
>>>>>>> On Thu, 30 Apr 2020 14:36:00 +0200
>>>>>>> Boris Brezillon<boris.brezillon@collabora.com>  wrote:
>>>>>>>          
>>>>>>>> On Thu, 30 Apr 2020 17:07:03 +0800
>>>>>>>> "Ramuthevar, Vadivel MuruganX"
>>>>>>>> <vadivel.muruganx.ramuthevar@linux.intel.com>  wrote:
>>>>>>>>         
>>>>>>>>>>>> The question is, is it the same value we have in nand_pa or it is
>>>>>>>>>>>> different?
>>>>>>>>>>>>                  
>>>>>>>>>>> Different address which is 0xE1400000 NAND_BASE_PHY address.
>>>>>>>>>> Then why didn't you tell me they didn't match when I suggested to pass
>>>>>>>>> sorry, because you keep asking nand_pa after that only I realized that.
>>>>>>>>>             
>>>>>>>>>> nand_pa? So now the question is, what does this address represent?
>>>>>>>>>                     EBU-MODULE
>>>>>>>>>       _________     _______________________
>>>>>>>>> |         |   |            |NAND CTRL  |
>>>>>>>>> | FPI BUS |==>| CS0(0x174) | 0xE100    ( 0xE14/0xE1C) NAND_PHY_BASE
>>>>>>>>> |_________|   |_CS1(0x17C)_|__________ |
>>>>>>>>>
>>>>>>>>> EBU_CONRTROLLER_BASE : 0xE0F0_0000
>>>>>>>>> HSNAND_BASE: 0xE100_0000
>>>>>>>>> NAND_CS0: 0xE140_0000
>>>>>>>>> NAND_CS1: 0xE1C0_0000
>>>>>>>>>
>>>>>>>>> MEM_REGION_BASE_CS0: 0x17400 (internal to ebu controller )
>>>>>>>>> MEM_REGION_BASE_CS1: 0x17C00
>>>>>>>>>             
>>>>>>>> Hm, I wonder if we shouldn't use a 'ranges' property to describe this
>>>>>>>> address translation. Something like
>>>>>>>>
>>>>>>>> 	ebu@xxx {
>>>>>>>> 		ranges = <0x17400000 0xe1400000 0x1000>,
>>>>>>>> 			 <0x17c00000 0xe1c00000 0x1000>;
>>>>>>>> 		reg = <0x17400000>, <0x17c00000>;
>>>>>>>> 		reg-names = "cs-0", "cs-1";
>>>>>>>> 	}
>>>>>>>>
>>>>>>>> The translated address (0xE1X00000) will be available in res->start,
>>>>>>>> and the non-translated one (0x17X00000) can be retrieved with
>>>>>>>> of_get_address(). All you'd have to do then would be calculate the
>>>>>>>> mask:
>>>>>>>>
>>>>>>>> 	mask = (translated_address & original_address) >> 22;
>>>>>>>> 	num_comp_bits = fls(mask);
>>>>>>>> 	WARN_ON(mask != GENMASK(num_comp_bits - 1, 0));
>>>>>>>>
>>>>>>>> Which allows you to properly set the ADDR_SEL() register without
>>>>>>>> relying on some hardcoded values:
>>>>>>>>
>>>>>>>> 	writel(original_address | EBU_ADDR_SEL_REGEN |
>>>>>>>> 	       EBU_ADDR_COMP_BITS(num_comp_bits),
>>>>>>>> 	       ebu_host->ebu + EBU_ADDR_SEL(csid));
>>>>>>>>
>>>>>>>> That's quite important if we want to merge the xway NAND driver with
>>>>>>>> this one.
>>>>>>> Looks like the translation is done at the FPI bus declaration level (see
>>>>>>> [1]). We really need to see the big picture to take a wise decision
>>>>>>> about the bindings. Would you mind pasting your dsti/dts files
>>>>>>> somewhere? It feels like the NAND controller is a sub-part of a more
>>>>>>> generic 'memory' controller, in which case the NAND controller should be
>>>>>>> declared as a child of this generic memory bus (called localbus in [1],
>>>>>>> but maybe EBU is more accurate).
>>>>>>>
>>>>>>> [1]https://github.com/xieyaxiongfly/Atheros_CSI_tool_OpenWRT_src/blob/master/target/linux/lantiq/files-4.14/arch/mips/boot/dts/vr9.dtsi#L162
>>>>>>
>>>>>>      ebu_nand: ebu_nand@e0f00000 {
>>>>>>                      compatible = "intel,lgm-ebu-nand";
>>>>>>                      reg = <0xe0f00000 0x100
>>>>>>                      0xe1000000 0x300
>>>>>>                      0xe1400000 0x80000
>>>>>>                      0xe1c00000 0x10000>;
>>>>>>                      reg-names = "ebunand", "hsnand", "nand_cs0", nand_cs1";
>>>>>>                      dmas = <&dma0 8>, <&dma0 9>;
>>>>>>                      dma-names = "ebu_rx", "ebu_tx";
>>>>>>                      clocks =  <&cgu0 LGM_GCLK_EBU>;
>>>>>>              };
>>>>>>
>>>>>>
>>>>>> 	 &ebu_nand {
>>>>>> 	         status = "disabled";
>>>>>> 	        nand,cs = <1>;
>>>>>> 	        nand-ecc-mode = "hw";
>>>>>> 	        pinctrl-names = "default";
>>>>>> 	        pinctrl-0 = <&ebu_nand_base &ebu_cs1>;
>>>>>> 	};
>>>>>>      
>>>>>>>          
>>>>> Ok. If I understand the SoC topology correctly it should actually be
>>>>> something like that:
>>>>>
>>>>> {
>>>>> 	...
>>>>> 	fpi@xxxxx {
>>>>> 		compatible = "intel,lgm-fpi", "simple-bus";
>>>>>
>>>>> 		/* You might have other ranges to define here */
>>>>> 		ranges = <0x16000000 0xe0000000 0x1000000>;
>>>>>
>>>>> 		...
>>>>
>>>> Sorry, we do not have fpi tree node in our dts/dtsi file instead we have
>>>> the below one.. , that also not included the major peripherals
>>>> controllers node.
>>>>            /* Special part from CPU core */
>>>>            core: core {
>>>>                    compatible = "intel,core", "simple-bus";
>>>>                    #address-cells = <1>;
>>>>                    #size-cells = <1>;
>>>>                    ranges;
>>>>
>>>>                    ioapic1: interrupt-controller@fec00000 {
>>>>                            #interrupt-cells = <2>;
>>>>                            #address-cells = <0>;
>>>>                            compatible = "intel,ce4100-ioapic";
>>>>                            interrupt-controller;
>>>>                            reg = <0xfec00000 0x1000>;
>>>>                            nr_entries = <256>;
>>>>                    };
>>>>
>>>>                    hpet: timer@fed00000 {
>>>>                            compatible = "intel,ce4100-hpet";
>>>>                            reg = <0xfed00000 0x400>;
>>>>                    };
>>>>
>>>>                    lapic0: interrupt-controller@fee00000 {
>>>>                            compatible = "intel,ce4100-lapic";
>>>>                            reg = <0xfee00000 0x1000>;
>>>>                            no_pic_mode;
>>>>                    };
>>>>            };
>>>>
>>>> other than this, rest all in independent node .
>>>
>>> But you do have an FPI bus, right? If this is the case it should be
>>> represented.
>>
>> Yes, FPI bus is slave to core which connects all the peripherals.
>>
>>    Or is the "intel,core" bus actually the FPI bus that you
>>> named differently?
>>
>> FPI slave bus connects to core bus by OCP bridge, so here it is named
>> FPI bus, but SW perspective didn't have root tree which has all
>> sub-nodes, as of now each peripheral has its own node.
> 
> Duh, not sure that's a good idea to hide that, especially since you
> have to describe the address translation that happens when crossing the
> FPI bus (the ranges thing I mentioned previously).

Thanks Boris, Sure I will do as you have suggested.

can I proceed to send next patch-set.

Regards
Vadivel
Ramuthevar, Vadivel MuruganX May 5, 2020, 5:28 a.m. UTC | #27
Hi Boris,

On 4/5/2020 4:58 pm, Boris Brezillon wrote:
> On Mon, 4 May 2020 16:50:08 +0800
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> 
>> Hi Boris,
>>
>> On 4/5/2020 3:17 pm, Boris Brezillon wrote:
>>> On Mon, 4 May 2020 15:15:08 +0800
>>> "Ramuthevar, Vadivel MuruganX"
>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>>    
>>>> Hi Boris,
>>>>
>>>>      Thank you very much for the prompt review and suggestions...
>>>>
>>>> On 4/5/2020 3:08 pm, Boris Brezillon wrote:
>>>>> On Mon, 4 May 2020 10:02:35 +0800
>>>>> "Ramuthevar, Vadivel MuruganX"
>>>>> <vadivel.muruganx.ramuthevar@linux.intel.com>  wrote:
>>>>>       
>>>>>> Hi Boris,
>>>>>>
>>>>>> On 30/4/2020 9:01 pm, Boris Brezillon wrote:
>>>>>>> On Thu, 30 Apr 2020 14:36:00 +0200
>>>>>>> Boris Brezillon<boris.brezillon@collabora.com>  wrote:
>>>>>>>          
>>>>>>>> On Thu, 30 Apr 2020 17:07:03 +0800
>>>>>>>> "Ramuthevar, Vadivel MuruganX"
>>>>>>>> <vadivel.muruganx.ramuthevar@linux.intel.com>  wrote:
>>>>>>>>         
>>>>>>>>>>>> The question is, is it the same value we have in nand_pa or it is
>>>>>>>>>>>> different?
>>>>>>>>>>>>                  
>>>>>>>>>>> Different address which is 0xE1400000 NAND_BASE_PHY address.
>>>>>>>>>> Then why didn't you tell me they didn't match when I suggested to pass
>>>>>>>>> sorry, because you keep asking nand_pa after that only I realized that.
>>>>>>>>>             
>>>>>>>>>> nand_pa? So now the question is, what does this address represent?
>>>>>>>>>                     EBU-MODULE
>>>>>>>>>       _________     _______________________
>>>>>>>>> |         |   |            |NAND CTRL  |
>>>>>>>>> | FPI BUS |==>| CS0(0x174) | 0xE100    ( 0xE14/0xE1C) NAND_PHY_BASE
>>>>>>>>> |_________|   |_CS1(0x17C)_|__________ |
>>>>>>>>>
>>>>>>>>> EBU_CONRTROLLER_BASE : 0xE0F0_0000
>>>>>>>>> HSNAND_BASE: 0xE100_0000
>>>>>>>>> NAND_CS0: 0xE140_0000
>>>>>>>>> NAND_CS1: 0xE1C0_0000
>>>>>>>>>
>>>>>>>>> MEM_REGION_BASE_CS0: 0x17400 (internal to ebu controller )
>>>>>>>>> MEM_REGION_BASE_CS1: 0x17C00
>>>>>>>>>             
>>>>>>>> Hm, I wonder if we shouldn't use a 'ranges' property to describe this
>>>>>>>> address translation. Something like
>>>>>>>>
>>>>>>>> 	ebu@xxx {
>>>>>>>> 		ranges = <0x17400000 0xe1400000 0x1000>,
>>>>>>>> 			 <0x17c00000 0xe1c00000 0x1000>;
>>>>>>>> 		reg = <0x17400000>, <0x17c00000>;
>>>>>>>> 		reg-names = "cs-0", "cs-1";
>>>>>>>> 	}
>>>>>>>>
>>>>>>>> The translated address (0xE1X00000) will be available in res->start,
>>>>>>>> and the non-translated one (0x17X00000) can be retrieved with
>>>>>>>> of_get_address(). All you'd have to do then would be calculate the
>>>>>>>> mask:
>>>>>>>>
>>>>>>>> 	mask = (translated_address & original_address) >> 22;
>>>>>>>> 	num_comp_bits = fls(mask);
>>>>>>>> 	WARN_ON(mask != GENMASK(num_comp_bits - 1, 0));
>>>>>>>>
>>>>>>>> Which allows you to properly set the ADDR_SEL() register without
>>>>>>>> relying on some hardcoded values:
>>>>>>>>
>>>>>>>> 	writel(original_address | EBU_ADDR_SEL_REGEN |
>>>>>>>> 	       EBU_ADDR_COMP_BITS(num_comp_bits),
>>>>>>>> 	       ebu_host->ebu + EBU_ADDR_SEL(csid));
>>>>>>>>
>>>>>>>> That's quite important if we want to merge the xway NAND driver with
>>>>>>>> this one.
>>>>>>> Looks like the translation is done at the FPI bus declaration level (see
>>>>>>> [1]). We really need to see the big picture to take a wise decision
>>>>>>> about the bindings. Would you mind pasting your dsti/dts files
>>>>>>> somewhere? It feels like the NAND controller is a sub-part of a more
>>>>>>> generic 'memory' controller, in which case the NAND controller should be
>>>>>>> declared as a child of this generic memory bus (called localbus in [1],
>>>>>>> but maybe EBU is more accurate).
>>>>>>>
>>>>>>> [1]https://github.com/xieyaxiongfly/Atheros_CSI_tool_OpenWRT_src/blob/master/target/linux/lantiq/files-4.14/arch/mips/boot/dts/vr9.dtsi#L162
>>>>>>
>>>>>>      ebu_nand: ebu_nand@e0f00000 {
>>>>>>                      compatible = "intel,lgm-ebu-nand";
>>>>>>                      reg = <0xe0f00000 0x100
>>>>>>                      0xe1000000 0x300
>>>>>>                      0xe1400000 0x80000
>>>>>>                      0xe1c00000 0x10000>;
>>>>>>                      reg-names = "ebunand", "hsnand", "nand_cs0", nand_cs1";
>>>>>>                      dmas = <&dma0 8>, <&dma0 9>;
>>>>>>                      dma-names = "ebu_rx", "ebu_tx";
>>>>>>                      clocks =  <&cgu0 LGM_GCLK_EBU>;
>>>>>>              };
>>>>>>
>>>>>>
>>>>>> 	 &ebu_nand {
>>>>>> 	         status = "disabled";
>>>>>> 	        nand,cs = <1>;
>>>>>> 	        nand-ecc-mode = "hw";
>>>>>> 	        pinctrl-names = "default";
>>>>>> 	        pinctrl-0 = <&ebu_nand_base &ebu_cs1>;
>>>>>> 	};
>>>>>>      
>>>>>>>          
>>>>> Ok. If I understand the SoC topology correctly it should actually be
>>>>> something like that:
>>>>>
>>>>> {
>>>>> 	...
>>>>> 	fpi@xxxxx {
>>>>> 		compatible = "intel,lgm-fpi", "simple-bus";
>>>>>
>>>>> 		/* You might have other ranges to define here */
>>>>> 		ranges = <0x16000000 0xe0000000 0x1000000>;
>>>>>
>>>>> 		...
>>>>
>>>> Sorry, we do not have fpi tree node in our dts/dtsi file instead we have
>>>> the below one.. , that also not included the major peripherals
>>>> controllers node.
>>>>            /* Special part from CPU core */
>>>>            core: core {
>>>>                    compatible = "intel,core", "simple-bus";
>>>>                    #address-cells = <1>;
>>>>                    #size-cells = <1>;
>>>>                    ranges;
>>>>
>>>>                    ioapic1: interrupt-controller@fec00000 {
>>>>                            #interrupt-cells = <2>;
>>>>                            #address-cells = <0>;
>>>>                            compatible = "intel,ce4100-ioapic";
>>>>                            interrupt-controller;
>>>>                            reg = <0xfec00000 0x1000>;
>>>>                            nr_entries = <256>;
>>>>                    };
>>>>
>>>>                    hpet: timer@fed00000 {
>>>>                            compatible = "intel,ce4100-hpet";
>>>>                            reg = <0xfed00000 0x400>;
>>>>                    };
>>>>
>>>>                    lapic0: interrupt-controller@fee00000 {
>>>>                            compatible = "intel,ce4100-lapic";
>>>>                            reg = <0xfee00000 0x1000>;
>>>>                            no_pic_mode;
>>>>                    };
>>>>            };
>>>>
>>>> other than this, rest all in independent node .
>>>
>>> But you do have an FPI bus, right? If this is the case it should be
>>> represented.
>>
>> Yes, FPI bus is slave to core which connects all the peripherals.
>>
>>    Or is the "intel,core" bus actually the FPI bus that you
>>> named differently?
>>
>> FPI slave bus connects to core bus by OCP bridge, so here it is named
>> FPI bus, but SW perspective didn't have root tree which has all
>> sub-nodes, as of now each peripheral has its own node.
> 
> Duh, not sure that's a good idea to hide that, especially since you
> have to describe the address translation that happens when crossing the
> FPI bus (the ranges thing I mentioned previously).

Thanks! for the keep reviewing.

SW Address translation is not required, after discussion with HW team , 
came to know that 0x17400 and 0x17C00 internal to the SoC.

NOC will translate 0xE1XX... to FPI address 0x17X... internally.
There is an address translation in the NOC.
0x17X... is not visible to user.

so far added hard-coded values to CS0 and CS1 is not at required.
I will change the code accordingly and sent to you.


Regards
Vadivel

>
Boris Brezillon May 5, 2020, 7 a.m. UTC | #28
Hello Vadivel,

On Tue, 5 May 2020 13:28:58 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
 
> >>>>>>
> >>>>>>      ebu_nand: ebu_nand@e0f00000 {
> >>>>>>                      compatible = "intel,lgm-ebu-nand";
> >>>>>>                      reg = <0xe0f00000 0x100
> >>>>>>                      0xe1000000 0x300
> >>>>>>                      0xe1400000 0x80000
> >>>>>>                      0xe1c00000 0x10000>;
> >>>>>>                      reg-names = "ebunand", "hsnand", "nand_cs0", nand_cs1";
> >>>>>>                      dmas = <&dma0 8>, <&dma0 9>;
> >>>>>>                      dma-names = "ebu_rx", "ebu_tx";
> >>>>>>                      clocks =  <&cgu0 LGM_GCLK_EBU>;
> >>>>>>              };
> >>>>>>
> >>>>>>
> >>>>>> 	 &ebu_nand {
> >>>>>> 	         status = "disabled";
> >>>>>> 	        nand,cs = <1>;
> >>>>>> 	        nand-ecc-mode = "hw";
> >>>>>> 	        pinctrl-names = "default";
> >>>>>> 	        pinctrl-0 = <&ebu_nand_base &ebu_cs1>;
> >>>>>> 	};
> >>>>>>        
> >>>>>>>            
> >>>>> Ok. If I understand the SoC topology correctly it should actually be
> >>>>> something like that:
> >>>>>
> >>>>> {
> >>>>> 	...
> >>>>> 	fpi@xxxxx {
> >>>>> 		compatible = "intel,lgm-fpi", "simple-bus";
> >>>>>
> >>>>> 		/* You might have other ranges to define here */
> >>>>> 		ranges = <0x16000000 0xe0000000 0x1000000>;
> >>>>>
> >>>>> 		...  
> >>>>
> >>>> Sorry, we do not have fpi tree node in our dts/dtsi file instead we have
> >>>> the below one.. , that also not included the major peripherals
> >>>> controllers node.
> >>>>            /* Special part from CPU core */
> >>>>            core: core {
> >>>>                    compatible = "intel,core", "simple-bus";
> >>>>                    #address-cells = <1>;
> >>>>                    #size-cells = <1>;
> >>>>                    ranges;
> >>>>
> >>>>                    ioapic1: interrupt-controller@fec00000 {
> >>>>                            #interrupt-cells = <2>;
> >>>>                            #address-cells = <0>;
> >>>>                            compatible = "intel,ce4100-ioapic";
> >>>>                            interrupt-controller;
> >>>>                            reg = <0xfec00000 0x1000>;
> >>>>                            nr_entries = <256>;
> >>>>                    };
> >>>>
> >>>>                    hpet: timer@fed00000 {
> >>>>                            compatible = "intel,ce4100-hpet";
> >>>>                            reg = <0xfed00000 0x400>;
> >>>>                    };
> >>>>
> >>>>                    lapic0: interrupt-controller@fee00000 {
> >>>>                            compatible = "intel,ce4100-lapic";
> >>>>                            reg = <0xfee00000 0x1000>;
> >>>>                            no_pic_mode;
> >>>>                    };
> >>>>            };
> >>>>
> >>>> other than this, rest all in independent node .  
> >>>
> >>> But you do have an FPI bus, right? If this is the case it should be
> >>> represented.  
> >>
> >> Yes, FPI bus is slave to core which connects all the peripherals.
> >>
> >>    Or is the "intel,core" bus actually the FPI bus that you  
> >>> named differently?  
> >>
> >> FPI slave bus connects to core bus by OCP bridge, so here it is named
> >> FPI bus, but SW perspective didn't have root tree which has all
> >> sub-nodes, as of now each peripheral has its own node.  
> > 
> > Duh, not sure that's a good idea to hide that, especially since you
> > have to describe the address translation that happens when crossing the
> > FPI bus (the ranges thing I mentioned previously).  
> 
> Thanks! for the keep reviewing.
> 
> SW Address translation is not required, after discussion with HW team , 
> came to know that 0x17400 and 0x17C00 internal to the SoC.
> 
> NOC will translate 0xE1XX... to FPI address 0x17X... internally.
> There is an address translation in the NOC.
> 0x17X... is not visible to user.
> 
> so far added hard-coded values to CS0 and CS1 is not at required.
> I will change the code accordingly and sent to you.

Hm, you told me last week that writing wrong values to this register
caused the NAND controller to not work properly (you even had code that
was overwriting the dynamically calculated values by hardcoded ones, so
I suspect it indeed didn't work) and now you say this write to
EBU_ADDR_SEL() is optional?! Sorry but it's hard to believe, and I've
received so many contradictory information from you on that matter that
I can't really tell which one is correct. Not sure I want to keep
reviewing new versions of this driver in this context.

Regards,

Boris
Ramuthevar, Vadivel MuruganX May 5, 2020, 7:17 a.m. UTC | #29
Hi Boris,

On 5/5/2020 3:00 pm, Boris Brezillon wrote:
> Hello Vadivel,
> 
> On Tue, 5 May 2020 13:28:58 +0800
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>   
>>>>>>>>
>>>>>>>>       ebu_nand: ebu_nand@e0f00000 {
>>>>>>>>                       compatible = "intel,lgm-ebu-nand";
>>>>>>>>                       reg = <0xe0f00000 0x100
>>>>>>>>                       0xe1000000 0x300
>>>>>>>>                       0xe1400000 0x80000
>>>>>>>>                       0xe1c00000 0x10000>;
>>>>>>>>                       reg-names = "ebunand", "hsnand", "nand_cs0", nand_cs1";
>>>>>>>>                       dmas = <&dma0 8>, <&dma0 9>;
>>>>>>>>                       dma-names = "ebu_rx", "ebu_tx";
>>>>>>>>                       clocks =  <&cgu0 LGM_GCLK_EBU>;
>>>>>>>>               };
>>>>>>>>
>>>>>>>>
>>>>>>>> 	 &ebu_nand {
>>>>>>>> 	         status = "disabled";
>>>>>>>> 	        nand,cs = <1>;
>>>>>>>> 	        nand-ecc-mode = "hw";
>>>>>>>> 	        pinctrl-names = "default";
>>>>>>>> 	        pinctrl-0 = <&ebu_nand_base &ebu_cs1>;
>>>>>>>> 	};
>>>>>>>>         
>>>>>>>>>             
>>>>>>> Ok. If I understand the SoC topology correctly it should actually be
>>>>>>> something like that:
>>>>>>>
>>>>>>> {
>>>>>>> 	...
>>>>>>> 	fpi@xxxxx {
>>>>>>> 		compatible = "intel,lgm-fpi", "simple-bus";
>>>>>>>
>>>>>>> 		/* You might have other ranges to define here */
>>>>>>> 		ranges = <0x16000000 0xe0000000 0x1000000>;
>>>>>>>
>>>>>>> 		...
>>>>>>
>>>>>> Sorry, we do not have fpi tree node in our dts/dtsi file instead we have
>>>>>> the below one.. , that also not included the major peripherals
>>>>>> controllers node.
>>>>>>             /* Special part from CPU core */
>>>>>>             core: core {
>>>>>>                     compatible = "intel,core", "simple-bus";
>>>>>>                     #address-cells = <1>;
>>>>>>                     #size-cells = <1>;
>>>>>>                     ranges;
>>>>>>
>>>>>>                     ioapic1: interrupt-controller@fec00000 {
>>>>>>                             #interrupt-cells = <2>;
>>>>>>                             #address-cells = <0>;
>>>>>>                             compatible = "intel,ce4100-ioapic";
>>>>>>                             interrupt-controller;
>>>>>>                             reg = <0xfec00000 0x1000>;
>>>>>>                             nr_entries = <256>;
>>>>>>                     };
>>>>>>
>>>>>>                     hpet: timer@fed00000 {
>>>>>>                             compatible = "intel,ce4100-hpet";
>>>>>>                             reg = <0xfed00000 0x400>;
>>>>>>                     };
>>>>>>
>>>>>>                     lapic0: interrupt-controller@fee00000 {
>>>>>>                             compatible = "intel,ce4100-lapic";
>>>>>>                             reg = <0xfee00000 0x1000>;
>>>>>>                             no_pic_mode;
>>>>>>                     };
>>>>>>             };
>>>>>>
>>>>>> other than this, rest all in independent node .
>>>>>
>>>>> But you do have an FPI bus, right? If this is the case it should be
>>>>> represented.
>>>>
>>>> Yes, FPI bus is slave to core which connects all the peripherals.
>>>>
>>>>     Or is the "intel,core" bus actually the FPI bus that you
>>>>> named differently?
>>>>
>>>> FPI slave bus connects to core bus by OCP bridge, so here it is named
>>>> FPI bus, but SW perspective didn't have root tree which has all
>>>> sub-nodes, as of now each peripheral has its own node.
>>>
>>> Duh, not sure that's a good idea to hide that, especially since you
>>> have to describe the address translation that happens when crossing the
>>> FPI bus (the ranges thing I mentioned previously).
>>
>> Thanks! for the keep reviewing.
>>
>> SW Address translation is not required, after discussion with HW team ,
>> came to know that 0x17400 and 0x17C00 internal to the SoC.
>>
>> NOC will translate 0xE1XX... to FPI address 0x17X... internally.
>> There is an address translation in the NOC.
>> 0x17X... is not visible to user.
>>
>> so far added hard-coded values to CS0 and CS1 is not at required.
>> I will change the code accordingly and sent to you.
> 
> Hm, you told me last week that writing wrong values to this register
> caused the NAND controller to not work properly (you even had code that
> was overwriting the dynamically calculated values by hardcoded ones, so
> I suspect it indeed didn't work) and now you say this write to
> EBU_ADDR_SEL() is optional?! Sorry but it's hard to believe, and I've
> received so many contradictory information from you on that matter that
> I can't really tell which one is correct. Not sure I want to keep
> reviewing new versions of this driver in this context.

oh my bad really sorry , since last week based on the input given from 
person who has worked on legacy IP, but now I have discussed with 
low-level HW team and confirmed. also we don't have proper document 
reference manual since it's new SoC.

Please forgive me , this shouldn't be happen once again, Thanks a lot!!

Regards
Vadivel
> 
> Regards,
> 
> Boris
>
diff mbox series

Patch

diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
index a80a46bb5b8b..a026bec28f39 100644
--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -457,6 +457,14 @@  config MTD_NAND_CADENCE
 	  Enable the driver for NAND flash on platforms using a Cadence NAND
 	  controller.
 
+config MTD_NAND_INTEL_LGM
+	tristate "Support for NAND controller on Intel LGM SoC"
+	depends on OF || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  Enables support for NAND Flash chips on Intel's LGM SoC.
+          NAND flash interfaced through the External Bus Unit.
+
 comment "Misc"
 
 config MTD_SM_COMMON
diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
index 2d136b158fb7..bfc8fe4d2cb0 100644
--- a/drivers/mtd/nand/raw/Makefile
+++ b/drivers/mtd/nand/raw/Makefile
@@ -58,6 +58,7 @@  obj-$(CONFIG_MTD_NAND_TEGRA)		+= tegra_nand.o
 obj-$(CONFIG_MTD_NAND_STM32_FMC2)	+= stm32_fmc2_nand.o
 obj-$(CONFIG_MTD_NAND_MESON)		+= meson_nand.o
 obj-$(CONFIG_MTD_NAND_CADENCE)		+= cadence-nand-controller.o
+obj-$(CONFIG_MTD_NAND_INTEL_LGM)	+= intel-nand-controller.o
 
 nand-objs := nand_base.o nand_legacy.o nand_bbt.o nand_timings.o nand_ids.o
 nand-objs += nand_onfi.o
diff --git a/drivers/mtd/nand/raw/intel-nand-controller.c b/drivers/mtd/nand/raw/intel-nand-controller.c
new file mode 100644
index 000000000000..fc69c7a9280c
--- /dev/null
+++ b/drivers/mtd/nand/raw/intel-nand-controller.c
@@ -0,0 +1,750 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/* Copyright (c) 2020 Intel Corporation. */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-direction.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/rawnand.h>
+#include <linux/mtd/nand_ecc.h>
+#include <linux/mtd/nand.h>
+#include <linux/resource.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/platform_device.h>
+
+#define EBU_CLC			0x000
+#define EBU_CLC_RST		0x00000000u
+
+#define EBU_ADDR_SEL(n)		(0x20 + (n) * 4)
+#define EBU_ADDR_MASK		(5 << 4)
+#define EBU_ADDR_SEL_REGEN	0x1
+
+#define EBU_BUSCON(n)		(0x60 + (n) * 4)
+#define EBU_BUSCON_CMULT_V4	0x1
+#define EBU_BUSCON_RECOVC(n)	((n) << 2)
+#define EBU_BUSCON_HOLDC(n)	((n) << 4)
+#define EBU_BUSCON_WAITRDC(n)	((n) << 6)
+#define EBU_BUSCON_WAITWRC(n)	((n) << 8)
+#define EBU_BUSCON_BCGEN_CS	0x0
+#define EBU_BUSCON_SETUP_EN	BIT(22)
+#define EBU_BUSCON_ALEC		0xC000
+
+#define EBU_CON			0x0B0
+#define EBU_CON_NANDM_EN	BIT(0)
+#define EBU_CON_NANDM_DIS	0x0
+#define EBU_CON_CSMUX_E_EN	BIT(1)
+#define EBU_CON_ALE_P_LOW	BIT(2)
+#define EBU_CON_CLE_P_LOW	BIT(3)
+#define EBU_CON_CS_P_LOW	BIT(4)
+#define EBU_CON_SE_P_LOW	BIT(5)
+#define EBU_CON_WP_P_LOW	BIT(6)
+#define EBU_CON_PRE_P_LOW	BIT(7)
+#define EBU_CON_IN_CS_S(n)	((n) << 8)
+#define EBU_CON_OUT_CS_S(n)	((n) << 10)
+#define EBU_CON_LAT_EN_CS_P	((0x3D) << 18)
+
+#define EBU_WAIT		0x0B4
+#define EBU_WAIT_RDBY		BIT(0)
+#define EBU_WAIT_WR_C		BIT(3)
+
+#define EBU_MEM_BASE_CS_0	0x17400000
+#define EBU_MEM_BASE_CS_1	0x17C00000
+
+#define HSNAND_CTL1		0x110
+#define HSNAND_CTL1_ADDR_SHIFT	24
+
+#define HSNAND_CTL2		0x114
+#define HSNAND_CTL2_ADDR_SHIFT	8
+#define HSNAND_CTL2_CYC_N_V5	(0x2 << 16)
+
+#define HSNAND_INT_MSK_CTL	0x124
+#define HSNAND_INT_MSK_CTL_WR_C	BIT(4)
+
+#define HSNAND_INT_STA		0x128
+#define HSNAND_INT_STA_WR_C	BIT(4)
+
+#define HSNAND_CTL		0x130
+#define HSNAND_CTL_ENABLE_ECC	BIT(0)
+#define HSNAND_CTL_GO		BIT(2)
+#define HSNAND_CTL_CE_SEL_CS(n)	BIT(3 + (n))
+#define HSNAND_CTL_RW_READ	0x0
+#define HSNAND_CTL_RW_WRITE	BIT(10)
+#define HSNAND_CTL_ECC_OFF_V8TH	BIT(11)
+#define HSNAND_CTL_CKFF_EN	0x0
+#define HSNAND_CTL_MSG_EN	BIT(17)
+
+#define HSNAND_PARA0		0x13c
+#define HSNAND_PARA0_PAGE_V8192	0x3
+#define HSNAND_PARA0_PIB_V256	(0x3 << 4)
+#define HSNAND_PARA0_BYP_EN_NP	0x0
+#define HSNAND_PARA0_BYP_DEC_NP	0x0
+#define HSNAND_PARA0_TYPE_ONFI	BIT(18)
+#define HSNAND_PARA0_ADEP_EN	BIT(21)
+
+#define HSNAND_CMSG_0		0x150
+#define HSNAND_CMSG_1		0x154
+
+#define HSNAND_ALE_OFFS		BIT(2)
+#define HSNAND_CLE_OFFS		BIT(3)
+#define HSNAND_CS_OFFS		BIT(4)
+
+#define NAND_WRITE_CMD		(EBU_CON_CS_P_LOW | HSNAND_CLE_OFFS)
+#define NAND_WRITE_ADDR		(EBU_CON_CS_P_LOW | HSNAND_ALE_OFFS)
+
+#define HSNAND_ECC_OFFSET	0x008
+
+#define NAND_DATA_IFACE_CHECK_ONLY	-1
+
+#define MAX_CS	2
+
+struct ebu_nand_cs {
+	void __iomem *chipaddr;
+	dma_addr_t nand_pa;
+};
+
+struct ebu_nand_controller {
+	struct nand_controller controller;
+	struct nand_chip chip;
+	struct device *dev;
+	void __iomem *ebu;
+	void __iomem *hsnand;
+	struct dma_chan *dma_tx;
+	struct dma_chan *dma_rx;
+	struct completion dma_access_complete;
+	unsigned long clk_rate;
+	struct clk *clk;
+	u32 nd_para0;
+	u8 cs_num;
+	struct ebu_nand_cs cs[MAX_CS];
+};
+
+static inline struct ebu_nand_controller *nand_to_ebu(struct nand_chip *chip)
+{
+	return container_of(chip, struct ebu_nand_controller, chip);
+}
+
+static u8 ebu_nand_readb(struct nand_chip *chip)
+{
+	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+	void __iomem *nand_wait = ebu_host->ebu + EBU_WAIT;
+	u8 cs_num = ebu_host->cs_num;
+	u32 stat;
+	int ret;
+	u8 val;
+
+	val = readb(ebu_host->cs[cs_num].chipaddr + HSNAND_CS_OFFS);
+
+	ret = readl_poll_timeout(nand_wait, stat, stat & EBU_WAIT_WR_C,
+				 20, 1000);
+	if (ret)
+		dev_warn(ebu_host->dev,
+			 "ebu nand write timeout. nand_wait(0x%p)=0x%x\n",
+			 nand_wait, readl(nand_wait));
+
+	return val;
+}
+
+static void ebu_nand_writeb(struct nand_chip *chip, u32 offset, u8 value)
+{
+	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+	void __iomem *nand_wait = ebu_host->ebu + EBU_WAIT;
+	u8 cs_num = ebu_host->cs_num;
+	u32 stat;
+	int ret;
+
+	writeb(value, ebu_host->cs[cs_num].chipaddr + offset);
+
+	ret = readl_poll_timeout(nand_wait, stat, stat & EBU_WAIT_WR_C,
+				 20, 1000);
+	if (ret)
+		dev_warn(ebu_host->dev,
+			 "ebu nand write timeout. nand_wait(0x%p)=0x%x\n",
+			 nand_wait, readl(nand_wait));
+}
+
+static void ebu_read_buf(struct nand_chip *chip, u_char *buf, unsigned int len)
+{
+	int i;
+
+	for (i = 0; i < len; i++)
+		buf[i] = ebu_nand_readb(chip);
+}
+
+static void ebu_write_buf(struct nand_chip *chip, const u_char *buf, int len)
+{
+	int i;
+
+	for (i = 0; i < len; i++)
+		ebu_nand_writeb(chip, HSNAND_CS_OFFS, buf[i]);
+}
+
+static void ebu_nand_disable(struct nand_chip *chip)
+{
+	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+
+	writel(0, ebu_host->ebu + EBU_CON);
+}
+
+static void ebu_select_chip(struct nand_chip *chip)
+{
+	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+	void __iomem *nand_con = ebu_host->ebu + EBU_CON;
+	u32 cs = ebu_host->cs_num;
+
+	writel(EBU_CON_NANDM_EN | EBU_CON_CSMUX_E_EN | EBU_CON_CS_P_LOW |
+	       EBU_CON_SE_P_LOW | EBU_CON_WP_P_LOW | EBU_CON_PRE_P_LOW |
+	       EBU_CON_IN_CS_S(cs) | EBU_CON_OUT_CS_S(cs) |
+	       EBU_CON_LAT_EN_CS_P, nand_con);
+}
+
+static void ebu_nand_setup_timing(struct ebu_nand_controller *ctrl,
+				  const struct nand_sdr_timings *timings)
+{
+	unsigned int rate = clk_get_rate(ctrl->clk) / 1000000;
+	unsigned int period = DIV_ROUND_UP(1000000, rate);
+	u32 trecov, thold, twrwait, trdwait;
+	u32 reg = 0;
+
+	trecov = DIV_ROUND_UP(max(timings->tREA_max, timings->tREH_min),
+			      period);
+	reg |= EBU_BUSCON_RECOVC(trecov);
+
+	thold = DIV_ROUND_UP(max(timings->tDH_min, timings->tDS_min), period);
+	reg |= EBU_BUSCON_HOLDC(thold);
+
+	trdwait = DIV_ROUND_UP(max(timings->tRC_min, timings->tREH_min),
+			       period);
+	reg |= EBU_BUSCON_WAITRDC(trdwait);
+
+	twrwait = DIV_ROUND_UP(max(timings->tWC_min, timings->tWH_min), period);
+	reg |= EBU_BUSCON_WAITWRC(twrwait);
+
+	reg |= EBU_BUSCON_CMULT_V4 | EBU_BUSCON_BCGEN_CS | EBU_BUSCON_ALEC |
+		EBU_BUSCON_SETUP_EN;
+
+	writel(reg, ctrl->ebu + EBU_BUSCON(ctrl->cs_num));
+}
+
+static int ebu_nand_setup_data_interface(struct nand_chip *chip, int csline,
+					 const struct nand_data_interface *conf)
+{
+	struct ebu_nand_controller *ctrl = nand_to_ebu(chip);
+	const struct nand_sdr_timings *timings;
+
+	timings = nand_get_sdr_timings(conf);
+	if (IS_ERR(timings))
+		return PTR_ERR(timings);
+
+	if (csline == NAND_DATA_IFACE_CHECK_ONLY)
+		return 0;
+
+	ebu_nand_setup_timing(ctrl, timings);
+
+	return 0;
+}
+
+static int ebu_nand_ooblayout_ecc(struct mtd_info *mtd, int section,
+				  struct mtd_oob_region *oobregion)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+
+	if (section)
+		return -ERANGE;
+
+	oobregion->offset = HSNAND_ECC_OFFSET;
+	oobregion->length = chip->ecc.total;
+
+	return 0;
+}
+
+static int ebu_nand_ooblayout_free(struct mtd_info *mtd, int section,
+				   struct mtd_oob_region *oobregion)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+
+	if (section)
+		return -ERANGE;
+
+	oobregion->offset = chip->ecc.total + HSNAND_ECC_OFFSET;
+	oobregion->length = mtd->oobsize - oobregion->offset;
+
+	return 0;
+}
+
+static const struct mtd_ooblayout_ops ebu_nand_ooblayout_ops = {
+	.ecc = ebu_nand_ooblayout_ecc,
+	.free = ebu_nand_ooblayout_free,
+};
+
+static void ebu_dma_rx_callback(void *cookie)
+{
+	struct ebu_nand_controller *ebu_host = cookie;
+
+	dmaengine_terminate_async(ebu_host->dma_rx);
+
+	complete(&ebu_host->dma_access_complete);
+}
+
+static void ebu_dma_tx_callback(void *cookie)
+{
+	struct ebu_nand_controller *ebu_host = cookie;
+
+	dmaengine_terminate_async(ebu_host->dma_tx);
+
+	complete(&ebu_host->dma_access_complete);
+}
+
+static int ebu_dma_start(struct ebu_nand_controller *ebu_host, u32 dir,
+			 const u8 *buf, u32 len)
+{
+	struct dma_async_tx_descriptor *tx;
+	struct completion *dma_completion;
+	dma_async_tx_callback callback;
+	struct dma_chan *chan;
+	dma_cookie_t cookie;
+	unsigned long flags = DMA_CTRL_ACK | DMA_PREP_INTERRUPT;
+	dma_addr_t buf_dma;
+	int ret;
+	u32 timeout;
+
+	if (dir == DMA_DEV_TO_MEM) {
+		chan = ebu_host->dma_rx;
+		dma_completion = &ebu_host->dma_access_complete;
+		callback = ebu_dma_rx_callback;
+	} else {
+		chan = ebu_host->dma_tx;
+		dma_completion = &ebu_host->dma_access_complete;
+		callback = ebu_dma_tx_callback;
+	}
+
+	buf_dma = dma_map_single(chan->device->dev, (void *)buf, len, dir);
+	if (dma_mapping_error(chan->device->dev, buf_dma)) {
+		dev_err(ebu_host->dev, "Failed to map DMA buffer\n");
+		ret = -EIO;
+		goto err_unmap;
+	}
+
+	tx = dmaengine_prep_slave_single(chan, buf_dma, len, dir, flags);
+	if (!tx)
+		return -ENXIO;
+
+	tx->callback = callback;
+	tx->callback_param = ebu_host;
+	cookie = tx->tx_submit(tx);
+
+	ret = dma_submit_error(cookie);
+	if (ret) {
+		dev_err(ebu_host->dev, "dma_submit_error %d\n", cookie);
+		ret = -EIO;
+		goto err_unmap;
+	}
+
+	init_completion(dma_completion);
+	dma_async_issue_pending(chan);
+
+	/* Wait DMA to finish the data transfer.*/
+	timeout =
+	wait_for_completion_timeout(dma_completion, msecs_to_jiffies(1000));
+	if (!timeout) {
+		dev_err(ebu_host->dev, "I/O Error in DMA RX (status %d)\n",
+			dmaengine_tx_status(chan, cookie, NULL));
+		dmaengine_terminate_sync(chan);
+		ret = -ETIMEDOUT;
+		goto err_unmap;
+	}
+
+	return 0;
+
+err_unmap:
+	dma_unmap_single(ebu_host->dev, buf_dma, len, dir);
+
+	return ret;
+}
+
+static void ebu_nand_trigger(struct ebu_nand_controller *ebu_host,
+			     int page, u32 cmd)
+{
+	unsigned int val;
+
+	val = cmd | (page & 0xFF) << HSNAND_CTL1_ADDR_SHIFT;
+	writel(val, ebu_host->hsnand + HSNAND_CTL1);
+	val = (page & 0xFFFF00) >> 8 | HSNAND_CTL2_CYC_N_V5;
+	writel(val, ebu_host->hsnand + HSNAND_CTL2);
+
+	writel(ebu_host->nd_para0, ebu_host->hsnand + HSNAND_PARA0);
+
+	/* clear first, will update later */
+	writel(0xFFFFFFFF, ebu_host->hsnand + HSNAND_CMSG_0);
+	writel(0xFFFFFFFF, ebu_host->hsnand + HSNAND_CMSG_1);
+
+	writel(HSNAND_INT_MSK_CTL_WR_C,
+	       ebu_host->hsnand + HSNAND_INT_MSK_CTL);
+
+	val = cmd == NAND_CMD_READ0 ? HSNAND_CTL_RW_READ : HSNAND_CTL_RW_WRITE;
+
+	writel(HSNAND_CTL_MSG_EN | HSNAND_CTL_CKFF_EN |
+	       HSNAND_CTL_ECC_OFF_V8TH | HSNAND_CTL_CE_SEL_CS(ebu_host->cs_num) |
+	       HSNAND_CTL_ENABLE_ECC | HSNAND_CTL_GO | val,
+	       ebu_host->hsnand + HSNAND_CTL);
+}
+
+static int ebu_nand_read_page_hwecc(struct nand_chip *chip, u8 *buf,
+				    int oob_required, int page)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+	int ret, x;
+
+	ebu_nand_trigger(ebu_host, page, NAND_CMD_READ0);
+
+	ret = ebu_dma_start(ebu_host, DMA_DEV_TO_MEM, buf, mtd->writesize);
+	if (ret)
+		return ret;
+
+	if (oob_required)
+		chip->ecc.read_oob(chip, page);
+
+	x = readl(ebu_host->hsnand + HSNAND_CTL);
+	x &= ~HSNAND_CTL_GO;
+	writel(x, ebu_host->hsnand + HSNAND_CTL);
+
+	return 0;
+}
+
+static int ebu_nand_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
+				     int oob_required, int page)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+	void __iomem *int_sta = ebu_host->hsnand + HSNAND_INT_STA;
+	int ret, val, x;
+	__be32 reg;
+
+	ebu_nand_trigger(ebu_host, page, NAND_CMD_SEQIN);
+
+	ret = ebu_dma_start(ebu_host, DMA_MEM_TO_DEV, buf, mtd->writesize);
+	if (ret)
+		return ret;
+
+	if (oob_required) {
+		const u8 *pdata;
+
+		pdata = chip->oob_poi;
+		reg = cpu_to_be32(*pdata++);
+		writel(reg, ebu_host->hsnand + HSNAND_CMSG_0);
+
+		reg = cpu_to_be32(*pdata);
+		writel(reg, ebu_host->hsnand + HSNAND_CMSG_1);
+	}
+
+	ret = readl_poll_timeout_atomic(int_sta, val,
+					!(val & HSNAND_INT_STA_WR_C), 10, 1000);
+	if (ret)
+		return -EIO;
+
+	x = readl(ebu_host->hsnand + HSNAND_CTL);
+	x &= ~HSNAND_CTL_GO;
+	writel(x, ebu_host->hsnand + HSNAND_CTL);
+
+	return 0;
+}
+
+static const u8 ecc_strength[] = { 1, 1, 4, 8, 24, 32, 40, 60, };
+
+static int ebu_nand_attach_chip(struct nand_chip *chip)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+	u32 eccsize, eccsteps, eccbytes, ecctotal, pagesize, pg_per_blk;
+	u32 eccstrength = chip->ecc.strength;
+	u32 writesize = mtd->writesize;
+	u32 blocksize = mtd->erasesize;
+	int start, val, i;
+
+	if (chip->ecc.mode != NAND_ECC_HW)
+		return 0;
+
+	/* Check whether eccsize is 0x0 or wrong. assign eccsize = 512 if YES */
+	if (!chip->ecc.size)
+		chip->ecc.size = 512;
+	eccsize = chip->ecc.size;
+
+	switch (eccsize) {
+	case 512:
+		start = 1;
+		if (!eccstrength)
+			eccstrength = 4;
+		break;
+	case 1024:
+		start = 4;
+		if (!eccstrength)
+			eccstrength = 32;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	i = round_up(start + 1, 4);
+	for (val = start; val < i; val++) {
+		if (eccstrength == ecc_strength[val])
+			break;
+	}
+	if (val == i)
+		return -EINVAL;
+
+	if (eccstrength == 8)
+		eccbytes = 14;
+	else
+		eccbytes = DIV_ROUND_UP(eccstrength * fls(8 * eccsize), 8);
+
+	eccsteps = writesize / eccsize;
+	ecctotal = eccsteps * eccbytes;
+	if ((ecctotal + 8) > mtd->oobsize)
+		return -ERANGE;
+
+	chip->ecc.total = ecctotal;
+	pagesize = fls(writesize >> 11);
+	if (pagesize > HSNAND_PARA0_PAGE_V8192)
+		return -ERANGE;
+
+	pg_per_blk = fls((blocksize / writesize) >> 6) << 4;
+	if (pg_per_blk > HSNAND_PARA0_PIB_V256)
+		return -ERANGE;
+
+	ebu_host->nd_para0 = pagesize | pg_per_blk | HSNAND_PARA0_BYP_EN_NP |
+			     HSNAND_PARA0_BYP_DEC_NP | HSNAND_PARA0_ADEP_EN |
+			     HSNAND_PARA0_TYPE_ONFI | (val << 29);
+
+	mtd_set_ooblayout(mtd, &ebu_nand_ooblayout_ops);
+	chip->ecc.read_page = ebu_nand_read_page_hwecc;
+	chip->ecc.write_page = ebu_nand_write_page_hwecc;
+
+	return 0;
+}
+
+static int ebu_nand_exec_op(struct nand_chip *chip,
+			    const struct nand_operation *op, bool check_only)
+{
+	struct ebu_nand_controller *ctrl = nand_to_ebu(chip);
+	const struct nand_op_instr *instr = NULL;
+	unsigned int op_id;
+	int i, time_out, ret = 0;
+	u32 stat;
+
+	ebu_select_chip(chip);
+
+	for (op_id = 0; op_id < op->ninstrs; op_id++) {
+		instr = &op->instrs[op_id];
+
+		switch (instr->type) {
+		case NAND_OP_CMD_INSTR:
+			ebu_nand_writeb(chip, NAND_WRITE_CMD,
+					instr->ctx.cmd.opcode);
+			break;
+
+		case NAND_OP_ADDR_INSTR:
+			for (i = 0; i < instr->ctx.addr.naddrs; i++)
+				ebu_nand_writeb(chip, NAND_WRITE_ADDR,
+						instr->ctx.addr.addrs[i]);
+			break;
+
+		case NAND_OP_DATA_IN_INSTR:
+			ebu_read_buf(chip, instr->ctx.data.buf.in,
+				     instr->ctx.data.len);
+			break;
+
+		case NAND_OP_DATA_OUT_INSTR:
+			ebu_write_buf(chip, instr->ctx.data.buf.out,
+				      instr->ctx.data.len);
+			break;
+
+		case NAND_OP_WAITRDY_INSTR:
+			time_out = instr->ctx.waitrdy.timeout_ms * 1000;
+			ret = readl_poll_timeout(ctrl->ebu + EBU_WAIT,
+						 stat, stat & EBU_WAIT_RDBY,
+						 20, time_out);
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static const struct nand_controller_ops ebu_nand_controller_ops = {
+	.attach_chip = ebu_nand_attach_chip,
+	.exec_op = ebu_nand_exec_op,
+	.setup_data_interface = ebu_nand_setup_data_interface,
+};
+
+static void ebu_dma_cleanup(struct ebu_nand_controller *ebu_host)
+{
+	if (ebu_host->dma_rx)
+		dma_release_channel(ebu_host->dma_rx);
+
+	if (ebu_host->dma_tx)
+		dma_release_channel(ebu_host->dma_tx);
+}
+
+static int ebu_nand_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ebu_nand_controller *ebu_host;
+	struct nand_chip *nand;
+	struct mtd_info *mtd;
+	struct resource *res;
+	char *resname;
+	int ret, i;
+	u32 reg;
+
+	ebu_host = devm_kzalloc(dev, sizeof(*ebu_host), GFP_KERNEL);
+	if (!ebu_host)
+		return -ENOMEM;
+
+	ebu_host->dev = dev;
+	nand_controller_init(&ebu_host->controller);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ebunand");
+	ebu_host->ebu = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(ebu_host->ebu))
+		return PTR_ERR(ebu_host->ebu);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hsnand");
+	ebu_host->hsnand = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(ebu_host->hsnand))
+		return PTR_ERR(ebu_host->hsnand);
+
+	ret = device_property_read_u32(dev, "nand,cs", &reg);
+	if (ret) {
+		dev_err(dev, "failed to get chip select: %d\n", ret);
+		return ret;
+	}
+	ebu_host->cs_num = reg;
+
+	for (i = 0; i < MAX_CS; i++) {
+		resname = devm_kasprintf(dev, GFP_KERNEL, "nand_cs%d", i);
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+						   resname);
+		ebu_host->cs[i].chipaddr = devm_ioremap_resource(dev, res);
+		ebu_host->cs[i].nand_pa = res->start;
+			if (IS_ERR(ebu_host->cs[i].chipaddr))
+				return PTR_ERR(ebu_host->cs[i].chipaddr);
+	}
+
+	ebu_host->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(ebu_host->clk)) {
+		ret = PTR_ERR(ebu_host->clk);
+		dev_err(dev, "failed to get clock: %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(ebu_host->clk);
+	if (ret) {
+		dev_err(dev, "failed to enable clock: %d\n", ret);
+		return ret;
+	}
+	ebu_host->clk_rate = clk_get_rate(ebu_host->clk);
+
+	ebu_host->dma_tx = dma_request_chan(dev, "tx");
+	if (IS_ERR(ebu_host->dma_tx)) {
+		ret = PTR_ERR(ebu_host->dma_tx);
+		dev_err(dev, "DMA tx channel request fail!.\n");
+		goto err_cleanup_dma;
+	}
+
+	ebu_host->dma_rx = dma_request_chan(dev, "rx");
+	if (IS_ERR(ebu_host->dma_rx)) {
+		ret = PTR_ERR(ebu_host->dma_rx);
+		dev_err(dev, "DMA tx channel request fail!.\n");
+		goto err_cleanup_dma;
+	}
+
+	writel(lower_32_bits(ebu_host->cs[ebu_host->cs_num].nand_pa) |
+	       EBU_ADDR_SEL_REGEN | EBU_ADDR_MASK,
+	       ebu_host->ebu + EBU_ADDR_SEL(reg));
+
+	writel(EBU_MEM_BASE_CS_0 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN,
+	       ebu_host->ebu + EBU_ADDR_SEL(0));
+	writel(EBU_MEM_BASE_CS_1 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN,
+	       ebu_host->ebu + EBU_ADDR_SEL(reg));
+
+	nand_set_flash_node(&ebu_host->chip, dev->of_node);
+	mtd = nand_to_mtd(&ebu_host->chip);
+	mtd->dev.parent = dev;
+	ebu_host->dev = dev;
+
+	platform_set_drvdata(pdev, ebu_host);
+	nand_set_controller_data(&ebu_host->chip, ebu_host);
+
+	nand = &ebu_host->chip;
+	nand->controller = &ebu_host->controller;
+	nand->controller->ops = &ebu_nand_controller_ops;
+
+	/* Scan to find existence of the device */
+	ret = nand_scan(&ebu_host->chip, 1);
+	if (ret)
+		goto err_cleanup_dma;
+
+	ret = mtd_device_register(mtd, NULL, 0);
+	if (ret)
+		goto err_clean_nand;
+
+	return 0;
+
+err_clean_nand:
+	nand_cleanup(&ebu_host->chip);
+err_cleanup_dma:
+	ebu_dma_cleanup(ebu_host);
+	clk_disable_unprepare(ebu_host->clk);
+
+	return ret;
+}
+
+static int ebu_nand_remove(struct platform_device *pdev)
+{
+	struct ebu_nand_controller *ebu_host = platform_get_drvdata(pdev);
+
+	if (ebu_host) {
+		mtd_device_unregister(nand_to_mtd(&ebu_host->chip));
+		nand_cleanup(&ebu_host->chip);
+		ebu_nand_disable(&ebu_host->chip);
+
+		if (ebu_host->dma_rx || ebu_host->dma_tx)
+			ebu_dma_cleanup(ebu_host);
+
+		clk_disable_unprepare(ebu_host->clk);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id ebu_nand_match[] = {
+	{ .compatible = "intel,nand-controller", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ebu_nand_match);
+
+static struct platform_driver ebu_nand_driver = {
+	.probe = ebu_nand_probe,
+	.remove = ebu_nand_remove,
+	.driver = {
+		.name = "intel-nand-controller",
+		.of_match_table = ebu_nand_match,
+	},
+
+};
+module_platform_driver(ebu_nand_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Vadivel Murugan R <vadivel.muruganx.ramuthevar@intel.com>");
+MODULE_DESCRIPTION("Intel's LGM External Bus NAND Controller driver");