From mboxrd@z Thu Jan  1 00:00:00 1970
From: Naveen Krishna Ch <naveenkrishna.ch@gmail.com>
Date: Wed, 10 Feb 2010 11:46:55 +0530
Subject: [U-Boot] [PATCH 1/3] S5PC100: Memory SubSystem Header file,
	register description(SROMC).
In-Reply-To: <1f3430fb1002092148xe8e0566x285e37785bc109b5@mail.gmail.com>
References: <1265707341-25956-1-git-send-email-ch.naveen@samsung.com> 
	<1265708054-10917-1-git-send-email-ch.naveen@samsung.com> 
	<1f3430fb1002092148xe8e0566x285e37785bc109b5@mail.gmail.com>
Message-ID: <ef0d8a251002092216t4d7f97a5p73b1ca9677e74836@mail.gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Kang,

On 10 February 2010 11:18, Minkyu Kang <promsoft@gmail.com> wrote:

> Dear Naveen Krishna Ch,
>
> On 9 February 2010 18:34, Naveen Krishna Ch <ch.naveen@samsung.com> wrote:
> > From: Naveen Krishna CH <ch.naveen@samsung.com>
> >
> > Memory subsystem of S5PC100 handles SROM, SRAM, OneDRAM, OneNand,
> > NAND Flash, DDRs.
> > mem.h is a common place for the register description of Memory subsystem
> > of S5PC100.
> > Note: Only SROM related registers are descibed now.
> >
> > Signed-off-by: Naveen Krishna Ch <ch.naveen <at> samsung.com>
> > ---
> >  include/asm-arm/arch-s5pc1xx/mem.h |   55
> ++++++++++++++++++++++++++++++++++++
> >  1 files changed, 55 insertions(+), 0 deletions(-)
> >  create mode 100644 include/asm-arm/arch-s5pc1xx/mem.h
> >
> > diff --git a/include/asm-arm/arch-s5pc1xx/mem.h
> b/include/asm-arm/arch-s5pc1xx/mem.h
> > new file mode 100644
> > index 0000000..66272ff
> > --- /dev/null
> > +++ b/include/asm-arm/arch-s5pc1xx/mem.h
>
> I think srom.h is better than mem.h.
>
As the SMC of S5PC100 is supporting several memories SRAM, SROM, NAND, NOR,
DDR. I kept it as mem.h

>
> > @@ -0,0 +1,55 @@
> > +/*
> > + * (C) Copyright 2010 Samsung Electronics
> > + * Naveen Krishna Ch <ch.naveen@samsung.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> > + * MA 02111-1307 USA
> > + *
> > + * Note: This file contains the register description for Memory
> subsystem
> > + *      (SROM, NAND Flash, OneNand, DDR, OneDRAM) on S5PC1XX.
> > + *
> > + *      Only SROMC is defined as of now
> > + */
> > +
> > +#ifndef __ASM_ARCH_MEM_H_
> > +#define __ASM_ARCH_MEM_H_
> > +
> > +#define SROM_DATA16_WIDTH(x)    (1<<((x*4)+0))
> > +#define SROM_BYTE_ADDR_MODE(x)  (1<<((x*4)+1))  /* 0-> Half-word base
> address*/
> > +                                               /* 1-> Byte base
> address*/
> > +#define SROM_WAIT_ENABLE(x)     (1<<((x*4)+2))
> > +#define SROM_BYTE_ENABLE(x)     (1<<((x*4)+3))
>
> Do you use all of these defines?
>
For Net support i use only  SROM_DATA16_WIDTH
But for NAND support we may need SROM_BYTE_ADDR_MODE
These Macros can serve for generic pupose.

>
> > +
> > +#define SMCBC_X_Tacs    (0x0)   /* 0clk         address set-up */
> > +#define SMCBC_X_Tcos    (0x4)   /* 4clk         chip selection set-up */
> > +#define SMCBC_X_Tacc    (0xe)   /* 14clk        access cycle */
> > +#define SMCBC_X_Tcoh    (0x1)   /* 1clk         chip selection hold */
> > +#define SMCBC_X_Tah     (0x4)   /* 4clk         address holding time */
> > +#define SMCBC_X_Tacp    (0x6)   /* 6clk         page mode access cycle
> */
> > +#define SMCBC_X_PMC     (0x0)   /* normal(1data)page mode configuration
> */
>
> Please don't use lowercase at define
>
I will change it.

> and () is unnecessary.
>
Should remove it

> and.. what mean X is?

The SROM has 6 banks, I used "X" to indicate that

>
> > +
> > +#define SMC_BC_X_CON    ((SMCBC_X_Tacs<<28)|(SMCBC_X_Tcos<<24)| \
> > +                        (SMCBC_X_Tacc<<16)|(SMCBC_X_Tcoh<<12)| \
> > +                        (SMCBC_X_Tah<<8)|(SMCBC_X_Tacp<<4)|    \
> > +                        (SMCBC_X_PMC))
> > +
> > +#ifndef __ASSEMBLY__
> > +struct s5pc1xx_sromc {
> > +       unsigned int    smc_bw;
> > +       unsigned int    smc_bc[6];
> > +};
> > +#endif /* __ASSEMBLY__ */
>
> smc_bw and smc_bc are already belong to sromc structure.
> is "smc_" really need?
> Please modify it to bw and bc or srom_bw and srom_bc (according to TRM)
>
I named it according to TRM. there it mentioned smc_bc.
I wil change it if u insist on srom_bc or so.

>
> > +
> > +#endif /* __ASM_ARCH_MEM_H_ */
> > --
> > 1.6.6
> >
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
> >
>
> Let me know your opinion

> Thanks,
> Minkyu Kang.

--
> from. prom.
> www.promsoft.net
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>



-- 
Shine bright,
(: Naveen Krishna Ch :)