Re: [PATCH 01/27] ARM: EXYNOS: Add Exynos3250 SoC ID

[Tomasz Figa]

Hi Chanwoo,

On 14.04.2014 07:13, Chanwoo Choi wrote:
> On 04/11/2014 05:39 PM, Tomasz Figa wrote:
> > On 11.04.2014 08:32, Chanwoo Choi wrote:
> > > On 04/11/2014 10:46 AM, Olof Johansson wrote:
> > > > On Thu, Apr 10, 2014 at 06:37:12PM +0900, Chanwoo Choi wrote:
> > > > > diff --git a/arch/arm/plat-samsung/include/plat/cpu.h b/arch/arm/plat-samsung/include/plat/cpu.h
> > > > > index 5992b8d..3d808f6b 100644
> > > > > --- a/arch/arm/plat-samsung/include/plat/cpu.h
> > > > > +++ b/arch/arm/plat-samsung/include/plat/cpu.h
> > > > > @@ -43,6 +43,9 @@ extern unsigned long samsung_cpu_id;
> > > > >    #define S5PV210_CPU_ID        0x43110000
> > > > >    #define S5PV210_CPU_MASK    0xFFFFF000
> > > > >
> > > > > +#define EXYNOS3250_SOC_ID       0xE3472000
> > > > > +#define EXYNOS3_SOC_MASK        0xFFFFF000
> > > > > +
> > > > >    #define EXYNOS4210_CPU_ID    0x43210000
> > > > >    #define EXYNOS4212_CPU_ID    0x43220000
> > > > >    #define EXYNOS4412_CPU_ID    0xE4412200
> > > > > @@ -68,6 +71,7 @@ IS_SAMSUNG_CPU(s5p6440, S5P6440_CPU_ID, S5P64XX_CPU_MASK)
> > > > >    IS_SAMSUNG_CPU(s5p6450, S5P6450_CPU_ID, S5P64XX_CPU_MASK)
> > > > >    IS_SAMSUNG_CPU(s5pc100, S5PC100_CPU_ID, S5PC100_CPU_MASK)
> > > > >    IS_SAMSUNG_CPU(s5pv210, S5PV210_CPU_ID, S5PV210_CPU_MASK)
> > > > > +IS_SAMSUNG_CPU(exynos3250, EXYNOS3250_SOC_ID, EXYNOS3_SOC_MASK)
> > > > >    IS_SAMSUNG_CPU(exynos4210, EXYNOS4210_CPU_ID, EXYNOS4_CPU_MASK)
> > > > >    IS_SAMSUNG_CPU(exynos4212, EXYNOS4212_CPU_ID, EXYNOS4_CPU_MASK)
> > > > >    IS_SAMSUNG_CPU(exynos4412, EXYNOS4412_CPU_ID, EXYNOS4_CPU_MASK)
> > > > > @@ -126,6 +130,12 @@ IS_SAMSUNG_CPU(exynos5440, EXYNOS5440_SOC_ID, EXYNOS5_SOC_MASK)
> > > > >    # define soc_is_s5pv210()    0
> > > > >    #endif
> > > > >
> > > > > +#if defined(CONFIG_SOC_EXYNOS3250)
> > > > > +# define soc_is_exynos3250()    is_samsung_exynos3250()
> > > > > +#else
> > > > > +# define soc_is_exynos3250()    0
> > > > > +#endif
> > > >
> > > > In general, I think we have too much code littered with soc_is_<foo>() going
> > > > on, so please try to avoid adding more for this SoC. Especially in cases where
> > > > you just want to bail out of certain features where we might already have
> > > > function pointers to control if a function is called or not, such as the
> > > > firmware interfaces.
> > >
> > > Do you prefer dt helper function such as following function instead of new soc_is_xx() ?
> > > - of_machine_is_compatible("samsung,exynos3250")
> > >
> > > If you are OK, I'll use of_machine_is_compatible() instead of soc_is_xx().
> >
> > First of all, there is still a lot of code in mach-exynos/ using the soc_is_xx() macros, so having some SoCs use them and other SoCs use of_machine_is_compatible() wouldn't make the code cleaner.
> >
> > For now, I wouldn't mind adding soc_is_exynos3250(), but in general such code surrounded with if (soc_is_xx()) blocks should be reworked to use something better, for example function pointers, as Olof suggested.
>
> I thought 'function pointers' method instead of soc_is_xxx() macro as following two case:
> I need more detailed explanation/example of "for example function pointers, as Olof suggested." sentence.
>
> [case 1]
> Each Exynos SoC has other function pointers according to compatible name of DT.
>
> For example, arch/arm/mach-exynos/firmware.c
>
> static const struct firmware_ops exynos_firmware_ops = {
> 	.do_idle		= exynos_do_idle,
> 	.set_cpu_boot_addr	= exynos_set_cpu_boot_addr,
> 	.cpu_boot		= exynos_cpu_boot,
> };
> static const struct firmware_ops exynos3250_firmware_ops = {
> 	.do_idle		= exynos_do_idle,
> 	.set_cpu_boot_addr	= exynos4212_set_cpu_boot_addr,
> 	.cpu_boot		= exynos3250_cpu_boot,
> };
>
> static const struct firmware_ops exynos4212_firmware_ops = {
> 	.do_idle		= exynos_do_idle,
> 	.set_cpu_boot_addr	= exynos4212_set_cpu_boot_addr,
> 	.cpu_boot		= exynos4212_cpu_boot,
> };
>
> struct secure_firmware {
> 	char *name;
> 	const struct firmware_ops *ops;
> } exynos_secure_firmware[] __initconst = {
> 	{ "samsung,secure-firmware",		&exynos_firmware_ops },
> 	{ "samsung,exynos3250-secure-firmware", &exynos3250_firmware_ops },
> 	{ "samsung,exynos4212-secure-firmware", &exynos4212_firmware_ops },
> };

This is probably the right solution. Another would be to detect which 
firmware ops to use by matching root node with particular SoC compatible 
strings.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/