RE: [PATCH 01/10] ARM: S5PV210: Add Samsung S5PV210 CPU support

[Kukjin Kim <kgene.kim@samsung.com>]

Kyungmin Park wrote:
> On Tue, Jan 19, 2010 at 11:27 AM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > This patch adds support for Samsung S5PV210 CPU. This patch also adds
> > an entry for S5PV210 cpu in plat-s5p cpu table.
> >
> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> > ---

(snip)

> > +/* GPIO bank sizes */
> > +#define S5PV210_GPIO_A0_NR     (8)
> > +#define S5PV210_GPIO_A1_NR     (4)
> > +#define S5PV210_GPIO_B_NR      (8)
> > +#define S5PV210_GPIO_C0_NR     (5)
> > +#define S5PV210_GPIO_C1_NR     (5)
> > +#define S5PV210_GPIO_D0_NR     (4)
> > +#define S5PV210_GPIO_D1_NR     (6)
> > +#define S5PV210_GPIO_E0_NR     (8)
> > +#define S5PV210_GPIO_E1_NR     (5)
> > +#define S5PV210_GPIO_F0_NR     (8)
> > +#define S5PV210_GPIO_F1_NR     (8)
> > +#define S5PV210_GPIO_F2_NR     (8)
> > +#define S5PV210_GPIO_F3_NR     (6)
> > +#define S5PV210_GPIO_G0_NR     (7)
> > +#define S5PV210_GPIO_G1_NR     (7)
> > +#define S5PV210_GPIO_G2_NR     (7)
> > +#define S5PV210_GPIO_G3_NR     (7)
> > +#define S5PV210_GPIO_H0_NR     (8)
> > +#define S5PV210_GPIO_H1_NR     (8)
> > +#define S5PV210_GPIO_H2_NR     (8)
> > +#define S5PV210_GPIO_H3_NR     (8)
> > +#define S5PV210_GPIO_I_NR      (7)
> > +#define S5PV210_GPIO_J0_NR     (8)
> > +#define S5PV210_GPIO_J1_NR     (6)
> > +#define S5PV210_GPIO_J2_NR     (8)
> > +#define S5PV210_GPIO_J3_NR     (8)
> > +#define S5PV210_GPIO_J4_NR     (5)
> 
> Is there any reason to use the actual NR number? How about to define
> it's all 8. why not?
> E.g., this chip support only 7 at GPIO I. then when we set the 7th
> bit. what's the matter?
> This make a below macros are simple.

Hi,

There are banks that have less than 8 gpio's.
So it is better to keep to define the bank sizes correctly.

> > +
> > +/* GPIO bank numbers */
> > +
> > +/* CONFIG_S3C_GPIO_SPACE allows the user to select extra
> > + * space for debugging purposes so that any accidental
> > + * change from one gpio bank to another can be caught.
> > +*/
> > +
> > +#define S5PV210_GPIO_NEXT(__gpio) \
> > +       ((__gpio##_START) + (__gpio##_NR) + CONFIG_S3C_GPIO_SPACE + 1)
> > +
> > +enum s5p_gpio_number {
> > +       S5PV210_GPIO_A0_START   = 0,
> > +       S5PV210_GPIO_A1_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_A0),
> > +       S5PV210_GPIO_B_START    =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_A1),
> > +       S5PV210_GPIO_C0_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_B),
> > +       S5PV210_GPIO_C1_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_C0),
> > +       S5PV210_GPIO_D0_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_C1),
> > +       S5PV210_GPIO_D1_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_D0),
> > +       S5PV210_GPIO_E0_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_D1),
> > +       S5PV210_GPIO_E1_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_E0),
> > +       S5PV210_GPIO_F0_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_E1),
> > +       S5PV210_GPIO_F1_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_F0),
> > +       S5PV210_GPIO_F2_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_F1),
> > +       S5PV210_GPIO_F3_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_F2),
> > +       S5PV210_GPIO_G0_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_F3),
> > +       S5PV210_GPIO_G1_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_G0),
> > +       S5PV210_GPIO_G2_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_G1),
> > +       S5PV210_GPIO_G3_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_G2),
> > +       S5PV210_GPIO_H0_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_G3),
> > +       S5PV210_GPIO_H1_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_H0),
> > +       S5PV210_GPIO_H2_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_H1),
> > +       S5PV210_GPIO_H3_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_H2),
> > +       S5PV210_GPIO_I_START    =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_H3),
> > +       S5PV210_GPIO_J0_START   = S5PV210_GPIO_NEXT(S5PV210_GPIO_I),
> > +       S5PV210_GPIO_J1_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_J0),
> > +       S5PV210_GPIO_J2_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_J1),
> > +       S5PV210_GPIO_J3_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_J2),
> > +       S5PV210_GPIO_J4_START   =
> S5PV210_GPIO_NEXT(S5PV210_GPIO_J3),
> > +};


(snip)


> > +/* UART */
> > +#define S5PV210_PA_UART                (0xE2900000)
> > +#define S5P_PA_UART            S5PV210_PA_UART
> > +#define S5P_VA_UART            S3C_VA_UART
> > +
> > +#define S5P_PA_UART0           (S5P_PA_UART + 0x0)
> > +#define S5P_PA_UART1           (S5P_PA_UART + 0x400)
> > +#define S5P_PA_UART2           (S5P_PA_UART + 0x800)
> > +#define S5P_PA_UART3           (S5P_PA_UART + 0xC00)
> > +#define S5P_UART_OFFSET                (0x400)
> > +
> > +#define S5P_VA_UARTx(x)                (S5P_VA_UART + (S5P_PA_UART &
> 0xfffff) \
> > +                               + ((x) * S5P_UART_OFFSET))
> > +
> > +#define S5P_VA_UART0           S5P_VA_UARTx(0)
> > +#define S5P_VA_UART1           S5P_VA_UARTx(1)
> > +#define S5P_VA_UART2           S5P_VA_UARTx(2)
> > +#define S5P_VA_UART3           S5P_VA_UARTx(3)
> > +#define S5P_SZ_UART            SZ_256
> 
> Please remove unfamiliar x S5P_VA_UARTx -> S5P_VA_UART.

The S5P_VA_UARTx(x) macro is used calculate the virtual base address for all
instances of UART module. So this macro is fine.

(snip)

> > +#define PHYS_OFFSET            UL(0x20000000)
> > +#define CONSISTENT_DMA_SIZE    (SZ_8M + SZ_4M + SZ_2M)
> > +#define NODE_MEM_SIZE_BITS     28
> 
> Can you confirm the PHYS_OFFSET is right? At previous time LSI guided
> that you should use the 0x3000'0000 for MFC issues. And please keep
> the consistent If you commit this one. we have to change the our
> environment from bootloader and kernel. Now we use the local kernel
> and mainline.
> 
> Also how can you use if we got the 0x2000'0000 with 128MiB and
> 0x4000'0000 with 384MiB or 512MiB.
> There's too much memory holes. Please don't say use the split the user
> and kernel 2G/2G configuration
> If we modify the base address, we can use above memory configuration
> without any changes.

According to hardware spec, PHYS_OFFSET is correct.

Actually in the previous kernel version, we used 0x3000_0000 as PHYS_OFFSET
because of some issue. But there is no problem about that, in the present
kernel.

I can understand, your concern if it changes.
So I will think more about that.

Thank you for your input.

(snip)

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
System LSI Division, SAMSUNG ELECTRONICS CO., LTD.