Kyungmin Park wrote:
> On Tue, Jan 19, 2010 at 11:27 AM, Kukjin Kim <kgene.kim@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > ---
(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@xxxxxxxxxxx>, Senior Engineer,
System LSI Division, SAMSUNG ELECTRONICS CO., LTD.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
