* Re: [PATCH v2 01/18] MIPS: intel: Add initial support for Intel MIPS SoCs
From: Hua Ma @ 2018-08-06 9:12 UTC (permalink / raw)
To: Paul Burton, Songjun Wu
Cc: yixin.zhu, chuanhua.lei, qi-ming.wu, linux-mips, linux-clk,
linux-serial, devicetree, James Hogan, linux-kernel, Ralf Baechle
In-Reply-To: <20180803174924.iqzmbtz5hrf5dlzu@pburton-laptop>
On 8/4/2018 1:49 AM, Paul Burton wrote:
> Hi Songjun / Hua,
>
> On Fri, Aug 03, 2018 at 11:02:20AM +0800, Songjun Wu wrote:
>> From: Hua Ma <hua.ma@linux.intel.com>
>>
>> Add initial support for Intel MIPS interAptiv SoCs made by Intel.
>> This series will add support for the grx500 family.
>>
>> The series allows booting a minimal system using a initramfs.
> Thanks for submitting this - I have some comments below.
Thanks for the review.
>> diff --git a/arch/mips/Kbuild.platforms b/arch/mips/Kbuild.platforms
>> index ac7ad54f984f..bcd647060f3e 100644
>> --- a/arch/mips/Kbuild.platforms
>> +++ b/arch/mips/Kbuild.platforms
>> @@ -12,6 +12,7 @@ platforms += cobalt
>> platforms += dec
>> platforms += emma
>> platforms += generic
>> +platforms += intel-mips
>> platforms += jazz
>> platforms += jz4740
>> platforms += lantiq
> Oh EVA, why must you ruin nice things... Ideally I'd be suggesting that
> we use the generic platform but it doesn't yet have a nice way to deal
> with non-standard EVA setups.
yes, we only support EVA.
> It would be good if we could make sure that's the only reason for your
> custom platform though, so that once generic does support EVA we can
> migrate your system over. Most notably, it would be good to make use of
> the UHI-specified boot protocol if possible (ie. $r4==-2, $r5==&dtb).
>
> It looks like your prom_init_cmdline() supports multiple boot protocols
> - could you clarify which is actually used?
this patch only support build-in dts, we will do a clean up.
>> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
>> index 08c10c518f83..2d34f17f3e24 100644
>> --- a/arch/mips/Kconfig
>> +++ b/arch/mips/Kconfig
>> @@ -409,6 +409,34 @@ config LANTIQ
>> select ARCH_HAS_RESET_CONTROLLER
>> select RESET_CONTROLLER
>>
>> +config INTEL_MIPS
>> + bool "Intel MIPS interAptiv SoC based platforms"
>> + select BOOT_RAW
>> + select CEVT_R4K
>> + select COMMON_CLK
>> + select CPU_MIPS32_3_5_EVA
>> + select CPU_MIPS32_3_5_FEATURES
>> + select CPU_MIPSR2_IRQ_EI
>> + select CPU_MIPSR2_IRQ_VI
>> + select CSRC_R4K
>> + select DMA_NONCOHERENT
>> + select GENERIC_ISA_DMA
>> + select IRQ_MIPS_CPU
>> + select MFD_CORE
>> + select MFD_SYSCON
>> + select MIPS_CPU_SCACHE
>> + select MIPS_GIC
>> + select SYS_HAS_CPU_MIPS32_R1
> For a system based on interAptiv you should never need to build a
> MIPS32r1 kernel, so you should remove the above select.
will remove.
>> diff --git a/arch/mips/include/asm/mach-intel-mips/cpu-feature-overrides.h b/arch/mips/include/asm/mach-intel-mips/cpu-feature-overrides.h
>> new file mode 100644
>> index 000000000000..ac5f3b943d2a
>> --- /dev/null
>> +++ b/arch/mips/include/asm/mach-intel-mips/cpu-feature-overrides.h
>> @@ -0,0 +1,61 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * This file was derived from: include/asm-mips/cpu-features.h
>> + * Copyright (C) 2003, 2004 Ralf Baechle
>> + * Copyright (C) 2004 Maciej W. Rozycki
>> + * Copyright (C) 2018 Intel Corporation.
>> + */
>> +
>> +#ifndef __ASM_MACH_INTEL_MIPS_CPU_FEATURE_OVERRIDES_H
>> +#define __ASM_MACH_INTEL_MIPS_CPU_FEATURE_OVERRIDES_H
>> +
>> +#define cpu_has_tlb 1
>> +#define cpu_has_4kex 1
>> +#define cpu_has_3k_cache 0
>> +#define cpu_has_4k_cache 1
>> +#define cpu_has_tx39_cache 0
>> +#define cpu_has_sb1_cache 0
>> +#define cpu_has_fpu 0
>> +#define cpu_has_32fpr 0
>> +#define cpu_has_counter 1
>> +#define cpu_has_watch 1
>> +#define cpu_has_divec 1
>> +
>> +#define cpu_has_prefetch 1
>> +#define cpu_has_ejtag 1
>> +#define cpu_has_llsc 1
>> +
>> +#define cpu_has_mips16 0
>> +#define cpu_has_mdmx 0
>> +#define cpu_has_mips3d 0
>> +#define cpu_has_smartmips 0
>> +#define cpu_has_mmips 0
>> +#define cpu_has_vz 0
>> +
>> +#define cpu_has_mips32r1 1
>> +#define cpu_has_mips32r2 1
>> +#define cpu_has_mips64r1 0
>> +#define cpu_has_mips64r2 0
>> +
>> +#define cpu_has_dsp 1
>> +#define cpu_has_dsp2 0
>> +#define cpu_has_mipsmt 1
>> +
>> +#define cpu_has_vint 1
>> +#define cpu_has_veic 0
>> +
>> +#define cpu_has_64bits 0
>> +#define cpu_has_64bit_zero_reg 0
>> +#define cpu_has_64bit_gp_regs 0
>> +#define cpu_has_64bit_addresses 0
>> +
>> +#define cpu_has_cm2 1
>> +#define cpu_has_cm2_l2sync 1
>> +#define cpu_has_eva 1
>> +#define cpu_has_tlbinv 1
>> +
>> +#define cpu_dcache_line_size() 32
>> +#define cpu_icache_line_size() 32
>> +#define cpu_scache_line_size() 32
> If you rebase atop linux-next or mips-next then you should find that
> many of these defines are now redundant, especially after removing the
> SYS_HAS_CPU_MIPS32_R1 select which means your kernel build will always
> target MIPS32r2.
>
> Due to architectural restrictions on where various options can be
> present, you should be able to remove:
>
> - cpu_has_4kex
> - cpu_has_3k_cache
> - cpu_has_4k_cache
> - cpu_has_32fpr
> - cpu_has_divec
> - cpu_has_prefetch
> - cpu_has_llsc
>
> cpu_has_mmips defaults to a compile-time zero unless you select
> CONFIG_SYS_SUPPORTS_MICROMIPS, so please remove that one.
>
> cpu_has_64bit_gp_regs & cpu_has_64bit_addresses both default to zero
> already for 32bit kernel builds, so please remove those.
>
> cpu_has_cm2 & cpu_has_cm2_l2sync don't exist anywhere in-tree, so please
> remove those.
>
> Additionally cpu_has_sb1_cache is not used anywhere, or defined by
> asm/cpu-features.h & seems to just be left over in some platform
> override files including presumably one you based yours on. Please
> remove it too.
Thanks, will remove.
> Also you select CPU_MIPSR2_IRQ_EI but define cpu_has_veic as 0, could
> you check that mismatch?
The hardware does support, but the software does not support.
>> diff --git a/arch/mips/include/asm/mach-intel-mips/irq.h b/arch/mips/include/asm/mach-intel-mips/irq.h
>> new file mode 100644
>> index 000000000000..12a949084856
>> --- /dev/null
>> +++ b/arch/mips/include/asm/mach-intel-mips/irq.h
>> @@ -0,0 +1,17 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2014 Lei Chuanhua <Chuanhua.lei@lantiq.com>
>> + * Copyright (C) 2018 Intel Corporation.
>> + */
>> +
>> +#ifndef __INTEL_MIPS_IRQ_H
>> +#define __INTEL_MIPS_IRQ_H
>> +
>> +#define MIPS_CPU_IRQ_BASE 0
>> +#define MIPS_GIC_IRQ_BASE (MIPS_CPU_IRQ_BASE + 8)
> These 2 defines are the defaults anyway, so please remove them.
Thanks, will remove.
>> +#define NR_IRQS 256
> And if you don't actually need this then you could remove irq.h entirely
> - do you actually use more than 128 IRQs?
Yes, the hardware support 256 IRQs.
>> diff --git a/arch/mips/include/asm/mach-intel-mips/spaces.h b/arch/mips/include/asm/mach-intel-mips/spaces.h
>> new file mode 100644
>> index 000000000000..80e7b09f712c
>> --- /dev/null
>> +++ b/arch/mips/include/asm/mach-intel-mips/spaces.h
>> % >% >%
>> +#define IO_SIZE _AC(0x10000000, UL)
>> +#define IO_SHIFT _AC(0x10000000, UL)
> These IO_ defines don't appear to be used anywhere?
Thanks, will remove.
>> +/* IO space one */
>> +#define __pa_symbol(x) __pa(x)
> Can you explain why you need this, rather than the default definition of
> __pa_symbol()? The comment doesn't seem to describe much of anything.
Thanks, will remove.
>> diff --git a/arch/mips/include/asm/mach-intel-mips/war.h b/arch/mips/include/asm/mach-intel-mips/war.h
>> new file mode 100644
>> index 000000000000..1c95553151e1
>> --- /dev/null
>> +++ b/arch/mips/include/asm/mach-intel-mips/war.h
>> @@ -0,0 +1,18 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __ASM_MIPS_MACH_INTEL_MIPS_WAR_H
>> +#define __ASM_MIPS_MACH_INTEL_MIPS_WAR_H
>> +
>> +#define R4600_V1_INDEX_ICACHEOP_WAR 0
>> +#define R4600_V1_HIT_CACHEOP_WAR 0
>> +#define R4600_V2_HIT_CACHEOP_WAR 0
>> +#define R5432_CP0_INTERRUPT_WAR 0
>> +#define BCM1250_M3_WAR 0
>> +#define SIBYTE_1956_WAR 0
>> +#define MIPS4K_ICACHE_REFILL_WAR 0
>> +#define MIPS_CACHE_SYNC_WAR 0
>> +#define TX49XX_ICACHE_INDEX_INV_WAR 0
>> +#define ICACHE_REFILLS_WORKAROUND_WAR 0
>> +#define R10000_LLSC_WAR 0
>> +#define MIPS34K_MISSED_ITLB_WAR 0
>> +
>> +#endif /* __ASM_MIPS_MACH_INTEL_MIPS_WAR_H */
> Since you need none of these workarounds, you shouldn't need war.h at
> all.
Thanks, will remove this file.
>> diff --git a/arch/mips/intel-mips/Kconfig b/arch/mips/intel-mips/Kconfig
>> new file mode 100644
>> index 000000000000..35d2ae2b5408
>> --- /dev/null
>> +++ b/arch/mips/intel-mips/Kconfig
>> @@ -0,0 +1,22 @@
>> +if INTEL_MIPS
>> +
>> +choice
>> + prompt "Built-in device tree"
>> + help
>> + Legacy bootloaders do not pass a DTB pointer to the kernel, so
>> + if a "wrapper" is not being used, the kernel will need to include
>> + a device tree that matches the target board.
>> +
>> + The builtin DTB will only be used if the firmware does not supply
>> + a valid DTB.
>> +
>> +config DTB_INTEL_MIPS_NONE
>> + bool "None"
>> +
>> +config DTB_INTEL_MIPS_GRX500
>> + bool "Intel MIPS GRX500 Board"
>> + select BUILTIN_DTB
>> +
>> +endchoice
>> +
>> +endif
> So do you actually have both styles of bootloader?
this patch only support the build-in, will do a clean up.
>> diff --git a/arch/mips/intel-mips/prom.c b/arch/mips/intel-mips/prom.c
>> new file mode 100644
>> index 000000000000..a1b1393c13bc
>> --- /dev/null
>> +++ b/arch/mips/intel-mips/prom.c
>> % >% >%
>> +static void __init prom_init_cmdline(void)
>> +{
>> + int i;
>> + int argc;
>> + char **argv;
>> +
>> + /*
>> + * If u-boot pass parameters, it is ok, however, if without u-boot
>> + * JTAG or other tool has to reset all register value before it goes
>> + * emulation most likely belongs to this category
>> + */
>> + if (fw_arg0 == 0 || fw_arg1 == 0)
>> + return;
> I don't understand what you're trying to say here, or why this check
> exists. If you boot with fw_arg0 == fw_arg1 == 0 then you'd just hit the
> loop below right, and execute zero iterations of it? That seems like it
> would be fine without this special case.
this patch do not support this , will remove.
>> + /*
>> + * a0: fw_arg0 - the number of string in init cmdline
>> + * a1: fw_arg1 - the address of string in init cmdline
>> + *
>> + * In accordance with the MIPS UHI specification,
>> + * the bootloader can pass the following arguments to the kernel:
>> + * - $a0: -2.
>> + * - $a1: KSEG0 address of the flattened device-tree blob.
>> + */
>> + if (fw_arg0 == -2)
>> + return;
>> +
>> + argc = fw_arg0;
>> + argv = (char **)KSEG1ADDR(fw_arg1);
>> +
>> + arcs_cmdline[0] = '\0';
>> +
>> + for (i = 0; i < argc; i++) {
>> + char *p = (char *)KSEG1ADDR(argv[i]);
>> +
>> + if (argv[i] && *p) {
>> + strlcat(arcs_cmdline, p, sizeof(arcs_cmdline));
>> + strlcat(arcs_cmdline, " ", sizeof(arcs_cmdline));
>> + }
>> + }
> Why do you need to use kseg1? Why can't the arguments be accessed
> cached?
>
> Are the arguments passed as virtual or physical addresses? If virtual &
> we can access them cached then you could replace all of this with a call
> to fw_init_cmdline().
this patch only support build-in dts, will remove .
>> +static int __init plat_enable_iocoherency(void)
>> +{
>> + if (!mips_cps_numiocu(0))
>> + return 0;
>> +
>> + /* Nothing special needs to be done to enable coherency */
>> + pr_info("Coherence Manager IOCU detected\n");
>> + /* Second IOCU for MPE or other master access register */
>> + write_gcr_reg0_base(0xa0000000);
>> + write_gcr_reg0_mask(0xf8000000 | CM_GCR_REGn_MASK_CMTGT_IOCU1);
>> + return 1;
>> +}
>> +
>> +static void __init plat_setup_iocoherency(void)
>> +{
>> + if (plat_enable_iocoherency() &&
>> + coherentio == IO_COHERENCE_DISABLED) {
>> + pr_info("Hardware DMA cache coherency disabled\n");
>> + return;
>> + }
>> + panic("This kind of IO coherency is not supported!");
>> +}
> Since you select CONFIG_DMA_NONCOHERENT in Kconfig, coherentio will
> always equal IO_COHERENCE_DISABLED. That suggests to me that the above 2
> functions are probably useless, or at least needlessly convoluted.
Thanks, will remove.
>> +static int __init plat_publish_devices(void)
>> +{
>> + if (!of_have_populated_dt())
>> + return 0;
>> + return of_platform_populate(NULL, of_default_bus_match_table, NULL,
>> + NULL);
>> +}
>> +arch_initcall(plat_publish_devices);
> The core DT code calls of_platform_populate() already (see
> of_platform_default_populate_init()), so you can remove this function.
>
> Thanks,
> Paul
Thanks, will remove.
^ permalink raw reply
* Re: [PATCH v2 14/18] serial: intel: Add CCF support
From: Wu, Songjun @ 2018-08-06 8:58 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Arnd Bergmann, Greg KH, Hauke Mehrtens, hua.ma, yixin.zhu,
chuanhua.lei, qi-ming.wu, Linux MIPS Mailing List, linux-clk,
open list:SERIAL DRIVERS,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux Kernel Mailing List, Jiri Slaby
In-Reply-To: <CAMuHMdWY9NuYOXq8sD9wmH48=vhMcSBomRY9ZbC+tGE3PGiTGg@mail.gmail.com>
On 8/6/2018 3:20 PM, Geert Uytterhoeven wrote:
> Hi Songjun,
>
> On Mon, Aug 6, 2018 at 9:15 AM Wu, Songjun <songjun.wu@linux.intel.com> wrote:
>> On 8/5/2018 5:03 AM, Arnd Bergmann wrote:
>>> On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman
>>> <gregkh@linuxfoundation.org> wrote:
>>>> On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote:
>>>>> On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote:
>>>>>> On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote:
>>>>> This patch makes it possible to use it with the legacy lantiq code and
>>>>> also with the common clock framework. I see multiple options to fix this
>>>>> problem.
>>>>>
>>>>> 1. The current approach to have it as a compile variant for a) legacy
>>>>> lantiq arch code without common clock framework and b) support for SoCs
>>>>> using the common clock framework.
>>>>> 2. Convert the lantiq arch code to the common clock framework. This
>>>>> would be a good approach, but it need some efforts.
>>>>> 3. Remove the arch/mips/lantiq code. There are still users of this code.
>>>>> 4. Use the old APIs also for the new xRX500 SoC, I do not like this
>>>>> approach.
>>>>> 5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally
>>>>> available and provide some better wrapper code.
>>>> I don't really care what you do at this point in time, but you all
>>>> should know better than the crazy #ifdef is not allowed to try to
>>>> prevent/allow the inclusion of a .h file. Checkpatch might have even
>>>> warned you about it, right?
>>>>
>>>> So do it correctly, odds are #5 is correct, as that makes it work like
>>>> any other device in the kernel. You are not unique here.
>>> The best approach here would clearly be 2. We don't want platform
>>> specific header files for doing things that should be completely generic.
>>>
>>> Converting lantiq to the common-clk framework obviously requires
>>> some work, but then again the whole arch/mips/lantiq/clk.c file
>>> is fairly short and maybe not that hard to convert.
>>>
>>> >From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you
>>> already use the clkdev lookup mechanism for some devices without
>>> using COMMON_CLK, so I would assume that you can also use those
>>> for the remaining clks, which would be much simpler. It registers
>>> one anonymous clk there as
>>>
>>> clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1);
>>>
>>> so why not add replace that with two named clocks and just use
>>> the same names in the DT for the newer chip?
>>>
>>> Arnd
>> We discussed internally and have another solution for this issue.
>> Add one lantiq.h in the serial folder, and use "#ifdef preprocessor" in
>> lantiq.h,
>> also providing no-op stub functions in the #else case, then call those
>> functions
>> unconditionally from lantiq.c to avoid #ifdef in C file.
>>
>> To support CCF in legacy product is another topic, is not included in
>> this patch.
>>
>> The implementation is as following：
>> #ifdef CONFIG_LANTIQ
>> #include <lantiq_soc.h>
>> #else
>> #define LTQ_EARLY_ASC 0
>> #define CPHYSADDR(_val) 0
>>
>> static inline struct clk *clk_get_fpi(void)
>> {
>> return NULL;
>> }
>> #endif
> Why not use clkdev_add(), as Arnd suggested?
> That would be a 3-line patch without introducing a new header file and an ugly
> #ifdef, which complicates compile coverage testing?
>
> Gr{oetje,eeting}s,
>
> Geert
The reason we add a new head file is also for two macros(LTQ_EARLY_ASC
and CPHYSADDR)
used by legacy product. We need to provide the no-op stub for these two
macro for new product.
^ permalink raw reply
* Re: [PATCH v2 14/18] serial: intel: Add CCF support
From: Geert Uytterhoeven @ 2018-08-06 7:20 UTC (permalink / raw)
To: songjun.wu
Cc: Arnd Bergmann, Greg KH, Hauke Mehrtens, hua.ma, yixin.zhu,
chuanhua.lei, qi-ming.wu, Linux MIPS Mailing List, linux-clk,
open list:SERIAL DRIVERS,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux Kernel Mailing List, Jiri Slaby
In-Reply-To: <acd28f40-4342-7f67-8468-7d4578f614a1@linux.intel.com>
Hi Songjun,
On Mon, Aug 6, 2018 at 9:15 AM Wu, Songjun <songjun.wu@linux.intel.com> wrote:
> On 8/5/2018 5:03 AM, Arnd Bergmann wrote:
> > On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> >> On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote:
> >>> On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote:
> >>>> On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote:
> >>> This patch makes it possible to use it with the legacy lantiq code and
> >>> also with the common clock framework. I see multiple options to fix this
> >>> problem.
> >>>
> >>> 1. The current approach to have it as a compile variant for a) legacy
> >>> lantiq arch code without common clock framework and b) support for SoCs
> >>> using the common clock framework.
> >>> 2. Convert the lantiq arch code to the common clock framework. This
> >>> would be a good approach, but it need some efforts.
> >>> 3. Remove the arch/mips/lantiq code. There are still users of this code.
> >>> 4. Use the old APIs also for the new xRX500 SoC, I do not like this
> >>> approach.
> >>> 5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally
> >>> available and provide some better wrapper code.
> >> I don't really care what you do at this point in time, but you all
> >> should know better than the crazy #ifdef is not allowed to try to
> >> prevent/allow the inclusion of a .h file. Checkpatch might have even
> >> warned you about it, right?
> >>
> >> So do it correctly, odds are #5 is correct, as that makes it work like
> >> any other device in the kernel. You are not unique here.
> > The best approach here would clearly be 2. We don't want platform
> > specific header files for doing things that should be completely generic.
> >
> > Converting lantiq to the common-clk framework obviously requires
> > some work, but then again the whole arch/mips/lantiq/clk.c file
> > is fairly short and maybe not that hard to convert.
> >
> > >From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you
> > already use the clkdev lookup mechanism for some devices without
> > using COMMON_CLK, so I would assume that you can also use those
> > for the remaining clks, which would be much simpler. It registers
> > one anonymous clk there as
> >
> > clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1);
> >
> > so why not add replace that with two named clocks and just use
> > the same names in the DT for the newer chip?
> >
> > Arnd
> We discussed internally and have another solution for this issue.
> Add one lantiq.h in the serial folder, and use "#ifdef preprocessor" in
> lantiq.h,
> also providing no-op stub functions in the #else case, then call those
> functions
> unconditionally from lantiq.c to avoid #ifdef in C file.
>
> To support CCF in legacy product is another topic, is not included in
> this patch.
>
> The implementation is as following：
> #ifdef CONFIG_LANTIQ
> #include <lantiq_soc.h>
> #else
> #define LTQ_EARLY_ASC 0
> #define CPHYSADDR(_val) 0
>
> static inline struct clk *clk_get_fpi(void)
> {
> return NULL;
> }
> #endif
Why not use clkdev_add(), as Arnd suggested?
That would be a 3-line patch without introducing a new header file and an ugly
#ifdef, which complicates compile coverage testing?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v2 15/18] serial: intel: Support more platform
From: Wu, Songjun @ 2018-08-06 7:20 UTC (permalink / raw)
To: Christoph Hellwig
Cc: hua.ma, yixin.zhu, chuanhua.lei, qi-ming.wu, linux-mips,
linux-clk, linux-serial, devicetree, Greg Kroah-Hartman,
linux-kernel, Jiri Slaby
In-Reply-To: <20180805083722.GA30000@infradead.org>
On 8/5/2018 4:37 PM, Christoph Hellwig wrote:
> The subject line also seems odd, your are changing deps on the lantiq
> driver, not some (nonexistent) intel serial driver.
>
Your suggestion is reasonable, it will be changed to "serial: lantiq".
^ permalink raw reply
* Re: [PATCH v2 14/18] serial: intel: Add CCF support
From: Wu, Songjun @ 2018-08-06 7:05 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman
Cc: Hauke Mehrtens, hua.ma, yixin.zhu, chuanhua.lei, qi-ming.wu,
open list:RALINK MIPS ARCHITECTURE, linux-clk, linux-serial, DTML,
Linux Kernel Mailing List, Jiri Slaby
In-Reply-To: <CAK8P3a3qs34LuhPeaef2wPHYEWbYO5N-4n7763BcaDyppiJ6DA@mail.gmail.com>
On 8/5/2018 5:03 AM, Arnd Bergmann wrote:
> On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote:
>>> On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote:
>>>> On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote:
>>> This patch makes it possible to use it with the legacy lantiq code and
>>> also with the common clock framework. I see multiple options to fix this
>>> problem.
>>>
>>> 1. The current approach to have it as a compile variant for a) legacy
>>> lantiq arch code without common clock framework and b) support for SoCs
>>> using the common clock framework.
>>> 2. Convert the lantiq arch code to the common clock framework. This
>>> would be a good approach, but it need some efforts.
>>> 3. Remove the arch/mips/lantiq code. There are still users of this code.
>>> 4. Use the old APIs also for the new xRX500 SoC, I do not like this
>>> approach.
>>> 5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally
>>> available and provide some better wrapper code.
>> I don't really care what you do at this point in time, but you all
>> should know better than the crazy #ifdef is not allowed to try to
>> prevent/allow the inclusion of a .h file. Checkpatch might have even
>> warned you about it, right?
>>
>> So do it correctly, odds are #5 is correct, as that makes it work like
>> any other device in the kernel. You are not unique here.
> The best approach here would clearly be 2. We don't want platform
> specific header files for doing things that should be completely generic.
>
> Converting lantiq to the common-clk framework obviously requires
> some work, but then again the whole arch/mips/lantiq/clk.c file
> is fairly short and maybe not that hard to convert.
>
> >From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you
> already use the clkdev lookup mechanism for some devices without
> using COMMON_CLK, so I would assume that you can also use those
> for the remaining clks, which would be much simpler. It registers
> one anonymous clk there as
>
> clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1);
>
> so why not add replace that with two named clocks and just use
> the same names in the DT for the newer chip?
>
> Arnd
We discussed internally and have another solution for this issue.
Add one lantiq.h in the serial folder, and use "#ifdef preprocessor" in
lantiq.h,
also providing no-op stub functions in the #else case, then call those
functions
unconditionally from lantiq.c to avoid #ifdef in C file.
To support CCF in legacy product is another topic, is not included in
this patch.
The implementation is as following：
#ifdef CONFIG_LANTIQ
#include <lantiq_soc.h>
#else
#define LTQ_EARLY_ASC 0
#define CPHYSADDR(_val) 0
static inline struct clk *clk_get_fpi(void)
{
return NULL;
}
#endif
^ permalink raw reply
* [RFC] serial: sc16is7xx: Use DT sub-nodes for UART ports
From: Andreas Färber @ 2018-08-05 23:26 UTC (permalink / raw)
To: linux-serial, Rob Herring
Cc: linux-mips, jringle, allsey87, Jakub Kicinski, Xue Liu,
Andreas Färber, Greg Kroah-Hartman, Jiri Slaby, linux-kernel
In-Reply-To: <CAL_JsqKNnfgESG6ON95D7nD8VNrcVy7-x6cGGnae_GbbGKAuPQ@mail.gmail.com>
This is to allow using serdev.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
drivers/tty/serial/sc16is7xx.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 243c96025053..ad7267274f65 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1213,9 +1213,31 @@ static int sc16is7xx_probe(struct device *dev,
SC16IS7XX_IOCONTROL_SRESET_BIT);
for (i = 0; i < devtype->nr_uart; ++i) {
+#ifdef CONFIG_OF
+ struct device_node *np;
+ struct platform_device *pdev;
+ char name[6] = "uartx";
+#endif
+
s->p[i].line = i;
/* Initialize port data */
+#ifdef CONFIG_OF
+ name[4] = '0' + i;
+ np = of_get_child_by_name(dev->of_node, name);
+ if (IS_ERR(np)) {
+ ret = PTR_ERR(np);
+ goto out_ports;
+ }
+ pdev = of_platform_device_create(np, NULL, dev);
+ if (IS_ERR(pdev)) {
+ ret = PTR_ERR(pdev);
+ goto out_ports;
+ }
+ platform_set_drvdata(pdev, dev_get_drvdata(dev));
+ s->p[i].port.dev = &pdev->dev;
+#else
s->p[i].port.dev = dev;
+#endif
s->p[i].port.irq = irq;
s->p[i].port.type = PORT_SC16IS7XX;
s->p[i].port.fifosize = SC16IS7XX_FIFO_SIZE;
@@ -1271,6 +1293,9 @@ static int sc16is7xx_probe(struct device *dev,
for (i--; i >= 0; i--) {
uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
clear_bit(s->p[i].port.line, &sc16is7xx_lines);
+#ifdef CONFIG_OF
+ of_platform_device_destroy(s->p[i].port.dev, NULL);
+#endif
}
#ifdef CONFIG_GPIOLIB
--
2.16.4
^ permalink raw reply related
* Re: [PATCH v2 15/18] serial: intel: Support more platform
From: Christoph Hellwig @ 2018-08-05 8:37 UTC (permalink / raw)
To: Songjun Wu
Cc: hua.ma, yixin.zhu, chuanhua.lei, qi-ming.wu, linux-mips,
linux-clk, linux-serial, devicetree, Greg Kroah-Hartman,
linux-kernel, Jiri Slaby
In-Reply-To: <20180803030237.3366-16-songjun.wu@linux.intel.com>
The subject line also seems odd, your are changing deps on the lantiq
driver, not some (nonexistent) intel serial driver.
^ permalink raw reply
* Re: [PATCH v2 14/18] serial: intel: Add CCF support
From: Arnd Bergmann @ 2018-08-04 21:03 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Hauke Mehrtens, Wu, Songjun, hua.ma, yixin.zhu, chuanhua.lei,
qi-ming.wu, open list:RALINK MIPS ARCHITECTURE, linux-clk,
linux-serial, DTML, Linux Kernel Mailing List, Jiri Slaby
In-Reply-To: <20180804124309.GB4920@kroah.com>
On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote:
>> On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote:
>> > On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote:
>> This patch makes it possible to use it with the legacy lantiq code and
>> also with the common clock framework. I see multiple options to fix this
>> problem.
>>
>> 1. The current approach to have it as a compile variant for a) legacy
>> lantiq arch code without common clock framework and b) support for SoCs
>> using the common clock framework.
>> 2. Convert the lantiq arch code to the common clock framework. This
>> would be a good approach, but it need some efforts.
>> 3. Remove the arch/mips/lantiq code. There are still users of this code.
>> 4. Use the old APIs also for the new xRX500 SoC, I do not like this
>> approach.
>> 5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally
>> available and provide some better wrapper code.
>
> I don't really care what you do at this point in time, but you all
> should know better than the crazy #ifdef is not allowed to try to
> prevent/allow the inclusion of a .h file. Checkpatch might have even
> warned you about it, right?
>
> So do it correctly, odds are #5 is correct, as that makes it work like
> any other device in the kernel. You are not unique here.
The best approach here would clearly be 2. We don't want platform
specific header files for doing things that should be completely generic.
Converting lantiq to the common-clk framework obviously requires
some work, but then again the whole arch/mips/lantiq/clk.c file
is fairly short and maybe not that hard to convert.
>From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you
already use the clkdev lookup mechanism for some devices without
using COMMON_CLK, so I would assume that you can also use those
for the remaining clks, which would be much simpler. It registers
one anonymous clk there as
clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1);
so why not add replace that with two named clocks and just use
the same names in the DT for the newer chip?
Arnd
^ permalink raw reply
* Re: [PATCH v2 14/18] serial: intel: Add CCF support
From: Greg Kroah-Hartman @ 2018-08-04 12:43 UTC (permalink / raw)
To: Hauke Mehrtens
Cc: Wu, Songjun, hua.ma, yixin.zhu, chuanhua.lei, qi-ming.wu,
linux-mips, linux-clk, linux-serial, devicetree, linux-kernel,
Jiri Slaby
In-Reply-To: <3360edd2-f3d8-b860-13fa-ce680edbfd0a@hauke-m.de>
On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote:
> On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote:
> > On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote:
> >>
> >>
> >> On 8/3/2018 1:56 PM, Greg Kroah-Hartman wrote:
> >>> On Fri, Aug 03, 2018 at 11:02:33AM +0800, Songjun Wu wrote:
> >>>> Previous implementation uses platform-dependent API to get the clock.
> >>>> Those functions are not available for other SoC which uses the same IP.
> >>>> The CCF (Common Clock Framework) have an abstraction based APIs for
> >>>> clock. In future, the platform specific code will be removed when the
> >>>> legacy soc use CCF as well.
> >>>> Change to use CCF APIs to get clock and rate. So that different SoCs
> >>>> can use the same driver.
> >>>>
> >>>> Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
> >>>> ---
> >>>>
> >>>> Changes in v2: None
> >>>>
> >>>> drivers/tty/serial/lantiq.c | 11 +++++++++++
> >>>> 1 file changed, 11 insertions(+)
> >>>>
> >>>> diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
> >>>> index 36479d66fb7c..35518ab3a80d 100644
> >>>> --- a/drivers/tty/serial/lantiq.c
> >>>> +++ b/drivers/tty/serial/lantiq.c
> >>>> @@ -26,7 +26,9 @@
> >>>> #include <linux/clk.h>
> >>>> #include <linux/gpio.h>
> >>>> +#ifdef CONFIG_LANTIQ
> >>>> #include <lantiq_soc.h>
> >>>> +#endif
> >>> That is never how you do this in Linux, you know better.
> >>>
> >>> Please go and get this patchset reviewed and signed-off-by from other
> >>> internal Intel kernel developers before resending it next time. It is
> >>> their job to find and fix your basic errors like this, not ours.
> >> Thank you for your comment.
> >> Actually, we have discussed this issue internally.
> >> We put the reason why we use "#ifdef CONFIG_LANTIQ" preprocessor in commit
> >> message in "[PATCH v2 08/18] serial: intel: Get serial id from dts".
> >> Please refer the commit message below.
> >>
> >> "#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC
> >> macro is defined in lantiq_soc.h.
> >> lantiq_soc.h is in arch path for legacy product support.
> >>
> >> arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
> >>
> >> If "#ifdef preprocessor" is changed to
> >> "if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled,
> >> code using LTQ_EARLY_ASC is compiled.
> >> Compilation will fail for no LTQ_EARLY_ASC defined.
> >
> > Sorry, but no. Why is this one tiny driver/chip somehow more "special"
> > than all of the tens of thousands of other devices we support to warrent
> > it getting some sort of special exception to do things differently?
> > What happens to the next device that wants to do it this way?
> >
> > Our coding style and rules are there for a reason, do not violate them
> > thinking your device is the only one that matters.
> >
> > Do it properly, again, you all know better than this.
> >
> > greg k-h
> >
> Hi Greg,
>
> The problem is that the Lantiq SoC code in arch/mips/lantiq does not use
> the common clock framework, but it uses the clk framework directly. It
> defines CONFIG_HAVE_CLK and CONFIG_CLKDEV_LOOKUP, but not
> CONFIG_COMMON_CLK. The xRX500 SoC which is being added here is about 2
> generations more recent than the VR9/xRX200 SoC which is the latest
> which is supported by the code in arch/mips/lantiq. With this new SoC we
> switched to the common clock framework. This driver is used by the older
> SoC and also by the new ones because this IP core is pretty similar in
> all the SoCs.
> This patch makes it possible to use it with the legacy lantiq code and
> also with the common clock framework. I see multiple options to fix this
> problem.
>
> 1. The current approach to have it as a compile variant for a) legacy
> lantiq arch code without common clock framework and b) support for SoCs
> using the common clock framework.
> 2. Convert the lantiq arch code to the common clock framework. This
> would be a good approach, but it need some efforts.
> 3. Remove the arch/mips/lantiq code. There are still users of this code.
> 4. Use the old APIs also for the new xRX500 SoC, I do not like this
> approach.
> 5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally
> available and provide some better wrapper code.
I don't really care what you do at this point in time, but you all
should know better than the crazy #ifdef is not allowed to try to
prevent/allow the inclusion of a .h file. Checkpatch might have even
warned you about it, right?
So do it correctly, odds are #5 is correct, as that makes it work like
any other device in the kernel. You are not unique here.
greg k-h
^ permalink raw reply
* Re: [PATCH v2 04/18] MIPS: dts: Add initial support for Intel MIPS SoCs
From: Hauke Mehrtens @ 2018-08-04 11:11 UTC (permalink / raw)
To: Songjun Wu, hua.ma, yixin.zhu, chuanhua.lei, qi-ming.wu
Cc: linux-mips, linux-clk, linux-serial, devicetree, James Hogan,
linux-kernel, Paul Burton, Rob Herring, Mark Rutland,
Ralf Baechle
In-Reply-To: <20180803030237.3366-5-songjun.wu@linux.intel.com>
[-- Attachment #1.1: Type: text/plain, Size: 5166 bytes --]
On 08/03/2018 05:02 AM, Songjun Wu wrote:
> From: Hua Ma <hua.ma@linux.intel.com>
>
> Add dts files to support Intel MIPS SoCs:
> - xrx500.dtsi is the chip dts
> - easy350_anywan.dts is the board dts
>
> Signed-off-by: Hua Ma <hua.ma@linux.intel.com>
> Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
> ---
>
> Changes in v2:
> - New patch split from previous patch
> - The memory address is changed to @20000000
> - Update to obj-$(CONFIG_BUILTIN_DTB) as per commit fca3aa166422
>
> arch/mips/boot/dts/Makefile | 1 +
> arch/mips/boot/dts/intel-mips/Makefile | 4 ++
> arch/mips/boot/dts/intel-mips/easy350_anywan.dts | 26 ++++++++++
> arch/mips/boot/dts/intel-mips/xrx500.dtsi | 66 ++++++++++++++++++++++++
> 4 files changed, 97 insertions(+)
> create mode 100644 arch/mips/boot/dts/intel-mips/Makefile
> create mode 100644 arch/mips/boot/dts/intel-mips/easy350_anywan.dts
> create mode 100644 arch/mips/boot/dts/intel-mips/xrx500.dtsi
>
> diff --git a/arch/mips/boot/dts/Makefile b/arch/mips/boot/dts/Makefile
> index 1e79cab8e269..05f52f279047 100644
> --- a/arch/mips/boot/dts/Makefile
> +++ b/arch/mips/boot/dts/Makefile
> @@ -3,6 +3,7 @@ subdir-y += brcm
> subdir-y += cavium-octeon
> subdir-y += img
> subdir-y += ingenic
> +subdir-y += intel-mips
> subdir-y += lantiq
> subdir-y += mscc
> subdir-y += mti
> diff --git a/arch/mips/boot/dts/intel-mips/Makefile b/arch/mips/boot/dts/intel-mips/Makefile
> new file mode 100644
> index 000000000000..adfaabbbb07c
> --- /dev/null
> +++ b/arch/mips/boot/dts/intel-mips/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +dtb-$(CONFIG_DTB_INTEL_MIPS_GRX500) += easy350_anywan.dtb
> +
> +obj-$(CONFIG_BUILTIN_DTB) += $(addsuffix .o, $(dtb-y))
> diff --git a/arch/mips/boot/dts/intel-mips/easy350_anywan.dts b/arch/mips/boot/dts/intel-mips/easy350_anywan.dts
> new file mode 100644
> index 000000000000..e5e95f90c5e7
> --- /dev/null
> +++ b/arch/mips/boot/dts/intel-mips/easy350_anywan.dts
> @@ -0,0 +1,26 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/dts-v1/;
> +
> +#include <dt-bindings/interrupt-controller/mips-gic.h>
> +#include <dt-bindings/clock/intel,grx500-clk.h>
> +
> +#include "xrx500.dtsi"
> +
> +/ {
> + model = "EASY350 ANYWAN (GRX350) Main model";
Main model can be removed, it does not identify the board.
> + compatible = "intel,easy350-anywan";
I think this should be
compatible = "intel,easy350-anywan", "intel,xrx500";
Are there different revisions of the EASY350 Anywan board or only of the
EASY550 board?There are at least some differences in the power supply on
the EASY550 V1 and EASY550 V2 board. I would suggest to be here very
specific to make it easier when adding more boards.
> +
> + aliases {
> + serial0 = &asc0;
> + };
> +
> + chosen {
> + bootargs = "earlycon=lantiq,0x16600000 clk_ignore_unused";
What happens when you remove clk_ignore_unused?
If it crashes we should probably define some of the clock to be always
active.
> + stdout-path = "serial0";
> + };
> +
> + memory@20000000 {
> + device_type = "memory";
> + reg = <0x20000000 0x0e000000>;
> + };
> +};
> diff --git a/arch/mips/boot/dts/intel-mips/xrx500.dtsi b/arch/mips/boot/dts/intel-mips/xrx500.dtsi
> new file mode 100644
> index 000000000000..54c5f8f8b604
> --- /dev/null
> +++ b/arch/mips/boot/dts/intel-mips/xrx500.dtsi
> @@ -0,0 +1,66 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "intel,xrx500";
> +
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + cpu0: cpu@0 {
> + device_type = "cpu";
> + compatible = "mti,interaptiv";
> + clocks = <&cgu CLK_CPU>;
> + reg = <0>;
> + };
> +
> + cpu1: cpu@1 {
> + device_type = "cpu";
> + compatible = "mti,interaptiv";
> + reg = <1>;
> + };
> + };
> +
> + cpu_intc: interrupt-controller {
> + compatible = "mti,cpu-interrupt-controller";
> +
> + interrupt-controller;
> + #interrupt-cells = <1>;
> + };
> +
> + gic: gic@12320000 {
> + compatible = "mti,gic";
> + reg = <0x12320000 0x20000>;
> +
> + interrupt-controller;
> + #interrupt-cells = <3>;
> + /*
> + * Declare the interrupt-parent even though the mti,gic
> + * binding doesn't require it, such that the kernel can
> + * figure out that cpu_intc is the root interrupt
> + * controller & should be probed first.
> + */
> + interrupt-parent = <&cpu_intc>;
> + mti,reserved-ipi-vectors = <56 8>;
> + };
> +
> + cgu: cgu@16200000 {
> + compatible = "intel,grx500-cgu", "syscon";
> + reg = <0x16200000 0x200>;
> + #clock-cells = <1>;
> + };
> +
> + asc0: serial@16600000 {
> + compatible = "lantiq,asc";
> + reg = <0x16600000 0x100000>;
> +
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_SHARED 103 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SHARED 105 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SHARED 106 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cgu CLK_SSX4>, <&cgu GCLK_UART>;
> + clock-names = "freq", "asc";
> + };
> +};
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v2 14/18] serial: intel: Add CCF support
From: Hauke Mehrtens @ 2018-08-04 10:54 UTC (permalink / raw)
To: Greg Kroah-Hartman, Wu, Songjun
Cc: hua.ma, yixin.zhu, chuanhua.lei, qi-ming.wu, linux-mips,
linux-clk, linux-serial, devicetree, linux-kernel, Jiri Slaby
In-Reply-To: <20180803103023.GA6557@kroah.com>
[-- Attachment #1.1: Type: text/plain, Size: 4124 bytes --]
On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote:
> On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote:
>>
>>
>> On 8/3/2018 1:56 PM, Greg Kroah-Hartman wrote:
>>> On Fri, Aug 03, 2018 at 11:02:33AM +0800, Songjun Wu wrote:
>>>> Previous implementation uses platform-dependent API to get the clock.
>>>> Those functions are not available for other SoC which uses the same IP.
>>>> The CCF (Common Clock Framework) have an abstraction based APIs for
>>>> clock. In future, the platform specific code will be removed when the
>>>> legacy soc use CCF as well.
>>>> Change to use CCF APIs to get clock and rate. So that different SoCs
>>>> can use the same driver.
>>>>
>>>> Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
>>>> ---
>>>>
>>>> Changes in v2: None
>>>>
>>>> drivers/tty/serial/lantiq.c | 11 +++++++++++
>>>> 1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
>>>> index 36479d66fb7c..35518ab3a80d 100644
>>>> --- a/drivers/tty/serial/lantiq.c
>>>> +++ b/drivers/tty/serial/lantiq.c
>>>> @@ -26,7 +26,9 @@
>>>> #include <linux/clk.h>
>>>> #include <linux/gpio.h>
>>>> +#ifdef CONFIG_LANTIQ
>>>> #include <lantiq_soc.h>
>>>> +#endif
>>> That is never how you do this in Linux, you know better.
>>>
>>> Please go and get this patchset reviewed and signed-off-by from other
>>> internal Intel kernel developers before resending it next time. It is
>>> their job to find and fix your basic errors like this, not ours.
>> Thank you for your comment.
>> Actually, we have discussed this issue internally.
>> We put the reason why we use "#ifdef CONFIG_LANTIQ" preprocessor in commit
>> message in "[PATCH v2 08/18] serial: intel: Get serial id from dts".
>> Please refer the commit message below.
>>
>> "#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC
>> macro is defined in lantiq_soc.h.
>> lantiq_soc.h is in arch path for legacy product support.
>>
>> arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
>>
>> If "#ifdef preprocessor" is changed to
>> "if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled,
>> code using LTQ_EARLY_ASC is compiled.
>> Compilation will fail for no LTQ_EARLY_ASC defined.
>
> Sorry, but no. Why is this one tiny driver/chip somehow more "special"
> than all of the tens of thousands of other devices we support to warrent
> it getting some sort of special exception to do things differently?
> What happens to the next device that wants to do it this way?
>
> Our coding style and rules are there for a reason, do not violate them
> thinking your device is the only one that matters.
>
> Do it properly, again, you all know better than this.
>
> greg k-h
>
Hi Greg,
The problem is that the Lantiq SoC code in arch/mips/lantiq does not use
the common clock framework, but it uses the clk framework directly. It
defines CONFIG_HAVE_CLK and CONFIG_CLKDEV_LOOKUP, but not
CONFIG_COMMON_CLK. The xRX500 SoC which is being added here is about 2
generations more recent than the VR9/xRX200 SoC which is the latest
which is supported by the code in arch/mips/lantiq. With this new SoC we
switched to the common clock framework. This driver is used by the older
SoC and also by the new ones because this IP core is pretty similar in
all the SoCs.
This patch makes it possible to use it with the legacy lantiq code and
also with the common clock framework. I see multiple options to fix this
problem.
1. The current approach to have it as a compile variant for a) legacy
lantiq arch code without common clock framework and b) support for SoCs
using the common clock framework.
2. Convert the lantiq arch code to the common clock framework. This
would be a good approach, but it need some efforts.
3. Remove the arch/mips/lantiq code. There are still users of this code.
4. Use the old APIs also for the new xRX500 SoC, I do not like this
approach.
5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally
available and provide some better wrapper code.
Hauke
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v3 3/8] mailbox: Add transmit done by blocking option
From: Mikko Perttunen @ 2018-08-04 10:45 UTC (permalink / raw)
To: Jassi Brar, Mikko Perttunen
Cc: Greg KH, Thierry Reding, Jon Hunter, Devicetree List,
linux-serial, linux-tegra, ", linux-arm-kernel",
linux-mediatek, srv_heupstream, Linux Kernel Mailing List
In-Reply-To: <CABb+yY0_dKC_cYFwwb-aeRgrx=sjv=8VLP=C6DJEecFf=v+mkQ@mail.gmail.com>
On 08/03/2018 03:54 PM, Jassi Brar wrote:
> On Mon, Jul 2, 2018 at 5:10 PM, Mikko Perttunen <mperttunen@nvidia.com> wrote:
>> Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the
>> send_data function of the mailbox driver is expected to block until
>> the message has been sent. The new option is used with the Tegra
>> Combined UART driver to minimize unnecessary overhead when transmitting
>> data.
>>
> 1) TXDONE_BY_BLOCK flag :-
> Have you tried setting the flag mbox_chan->mbox_client->tx_block ?
No - I suppose I should have done that. I'm a bit concerned about
overhead as send_data may be called thousands of times per second, so I
tried to make it as close as possible to the downstream driver that just
pokes the mailbox register directly.
>
> 2) Implementing TEGRA_HSP_MBOX_TYPE_SM :-
> In mailbox framework, a controller is a collection of identical
> channels. That is, instances of the same class.
> So ideally, in probe you should populate a controller for each
> type of channel, i.e, DB, SM, SS and AS.
Hmm, yes, I guess this would be possible if I change the mailbox core to
allow registering multiple controllers per device.
Thanks!
Mikko
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH v2 01/18] MIPS: intel: Add initial support for Intel MIPS SoCs
From: Paul Burton @ 2018-08-03 17:49 UTC (permalink / raw)
To: Songjun Wu
Cc: hua.ma, yixin.zhu, chuanhua.lei, qi-ming.wu, linux-mips,
linux-clk, linux-serial, devicetree, James Hogan, linux-kernel,
Ralf Baechle
In-Reply-To: <20180803030237.3366-2-songjun.wu@linux.intel.com>
Hi Songjun / Hua,
On Fri, Aug 03, 2018 at 11:02:20AM +0800, Songjun Wu wrote:
> From: Hua Ma <hua.ma@linux.intel.com>
>
> Add initial support for Intel MIPS interAptiv SoCs made by Intel.
> This series will add support for the grx500 family.
>
> The series allows booting a minimal system using a initramfs.
Thanks for submitting this - I have some comments below.
> diff --git a/arch/mips/Kbuild.platforms b/arch/mips/Kbuild.platforms
> index ac7ad54f984f..bcd647060f3e 100644
> --- a/arch/mips/Kbuild.platforms
> +++ b/arch/mips/Kbuild.platforms
> @@ -12,6 +12,7 @@ platforms += cobalt
> platforms += dec
> platforms += emma
> platforms += generic
> +platforms += intel-mips
> platforms += jazz
> platforms += jz4740
> platforms += lantiq
Oh EVA, why must you ruin nice things... Ideally I'd be suggesting that
we use the generic platform but it doesn't yet have a nice way to deal
with non-standard EVA setups.
It would be good if we could make sure that's the only reason for your
custom platform though, so that once generic does support EVA we can
migrate your system over. Most notably, it would be good to make use of
the UHI-specified boot protocol if possible (ie. $r4==-2, $r5==&dtb).
It looks like your prom_init_cmdline() supports multiple boot protocols
- could you clarify which is actually used?
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index 08c10c518f83..2d34f17f3e24 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -409,6 +409,34 @@ config LANTIQ
> select ARCH_HAS_RESET_CONTROLLER
> select RESET_CONTROLLER
>
> +config INTEL_MIPS
> + bool "Intel MIPS interAptiv SoC based platforms"
> + select BOOT_RAW
> + select CEVT_R4K
> + select COMMON_CLK
> + select CPU_MIPS32_3_5_EVA
> + select CPU_MIPS32_3_5_FEATURES
> + select CPU_MIPSR2_IRQ_EI
> + select CPU_MIPSR2_IRQ_VI
> + select CSRC_R4K
> + select DMA_NONCOHERENT
> + select GENERIC_ISA_DMA
> + select IRQ_MIPS_CPU
> + select MFD_CORE
> + select MFD_SYSCON
> + select MIPS_CPU_SCACHE
> + select MIPS_GIC
> + select SYS_HAS_CPU_MIPS32_R1
For a system based on interAptiv you should never need to build a
MIPS32r1 kernel, so you should remove the above select.
> diff --git a/arch/mips/include/asm/mach-intel-mips/cpu-feature-overrides.h b/arch/mips/include/asm/mach-intel-mips/cpu-feature-overrides.h
> new file mode 100644
> index 000000000000..ac5f3b943d2a
> --- /dev/null
> +++ b/arch/mips/include/asm/mach-intel-mips/cpu-feature-overrides.h
> @@ -0,0 +1,61 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * This file was derived from: include/asm-mips/cpu-features.h
> + * Copyright (C) 2003, 2004 Ralf Baechle
> + * Copyright (C) 2004 Maciej W. Rozycki
> + * Copyright (C) 2018 Intel Corporation.
> + */
> +
> +#ifndef __ASM_MACH_INTEL_MIPS_CPU_FEATURE_OVERRIDES_H
> +#define __ASM_MACH_INTEL_MIPS_CPU_FEATURE_OVERRIDES_H
> +
> +#define cpu_has_tlb 1
> +#define cpu_has_4kex 1
> +#define cpu_has_3k_cache 0
> +#define cpu_has_4k_cache 1
> +#define cpu_has_tx39_cache 0
> +#define cpu_has_sb1_cache 0
> +#define cpu_has_fpu 0
> +#define cpu_has_32fpr 0
> +#define cpu_has_counter 1
> +#define cpu_has_watch 1
> +#define cpu_has_divec 1
> +
> +#define cpu_has_prefetch 1
> +#define cpu_has_ejtag 1
> +#define cpu_has_llsc 1
> +
> +#define cpu_has_mips16 0
> +#define cpu_has_mdmx 0
> +#define cpu_has_mips3d 0
> +#define cpu_has_smartmips 0
> +#define cpu_has_mmips 0
> +#define cpu_has_vz 0
> +
> +#define cpu_has_mips32r1 1
> +#define cpu_has_mips32r2 1
> +#define cpu_has_mips64r1 0
> +#define cpu_has_mips64r2 0
> +
> +#define cpu_has_dsp 1
> +#define cpu_has_dsp2 0
> +#define cpu_has_mipsmt 1
> +
> +#define cpu_has_vint 1
> +#define cpu_has_veic 0
> +
> +#define cpu_has_64bits 0
> +#define cpu_has_64bit_zero_reg 0
> +#define cpu_has_64bit_gp_regs 0
> +#define cpu_has_64bit_addresses 0
> +
> +#define cpu_has_cm2 1
> +#define cpu_has_cm2_l2sync 1
> +#define cpu_has_eva 1
> +#define cpu_has_tlbinv 1
> +
> +#define cpu_dcache_line_size() 32
> +#define cpu_icache_line_size() 32
> +#define cpu_scache_line_size() 32
If you rebase atop linux-next or mips-next then you should find that
many of these defines are now redundant, especially after removing the
SYS_HAS_CPU_MIPS32_R1 select which means your kernel build will always
target MIPS32r2.
Due to architectural restrictions on where various options can be
present, you should be able to remove:
- cpu_has_4kex
- cpu_has_3k_cache
- cpu_has_4k_cache
- cpu_has_32fpr
- cpu_has_divec
- cpu_has_prefetch
- cpu_has_llsc
cpu_has_mmips defaults to a compile-time zero unless you select
CONFIG_SYS_SUPPORTS_MICROMIPS, so please remove that one.
cpu_has_64bit_gp_regs & cpu_has_64bit_addresses both default to zero
already for 32bit kernel builds, so please remove those.
cpu_has_cm2 & cpu_has_cm2_l2sync don't exist anywhere in-tree, so please
remove those.
Additionally cpu_has_sb1_cache is not used anywhere, or defined by
asm/cpu-features.h & seems to just be left over in some platform
override files including presumably one you based yours on. Please
remove it too.
Also you select CPU_MIPSR2_IRQ_EI but define cpu_has_veic as 0, could
you check that mismatch?
> diff --git a/arch/mips/include/asm/mach-intel-mips/irq.h b/arch/mips/include/asm/mach-intel-mips/irq.h
> new file mode 100644
> index 000000000000..12a949084856
> --- /dev/null
> +++ b/arch/mips/include/asm/mach-intel-mips/irq.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2014 Lei Chuanhua <Chuanhua.lei@lantiq.com>
> + * Copyright (C) 2018 Intel Corporation.
> + */
> +
> +#ifndef __INTEL_MIPS_IRQ_H
> +#define __INTEL_MIPS_IRQ_H
> +
> +#define MIPS_CPU_IRQ_BASE 0
> +#define MIPS_GIC_IRQ_BASE (MIPS_CPU_IRQ_BASE + 8)
These 2 defines are the defaults anyway, so please remove them.
> +#define NR_IRQS 256
And if you don't actually need this then you could remove irq.h entirely
- do you actually use more than 128 IRQs?
> diff --git a/arch/mips/include/asm/mach-intel-mips/spaces.h b/arch/mips/include/asm/mach-intel-mips/spaces.h
> new file mode 100644
> index 000000000000..80e7b09f712c
> --- /dev/null
> +++ b/arch/mips/include/asm/mach-intel-mips/spaces.h
>% >% >%
> +#define IO_SIZE _AC(0x10000000, UL)
> +#define IO_SHIFT _AC(0x10000000, UL)
These IO_ defines don't appear to be used anywhere?
> +/* IO space one */
> +#define __pa_symbol(x) __pa(x)
Can you explain why you need this, rather than the default definition of
__pa_symbol()? The comment doesn't seem to describe much of anything.
> diff --git a/arch/mips/include/asm/mach-intel-mips/war.h b/arch/mips/include/asm/mach-intel-mips/war.h
> new file mode 100644
> index 000000000000..1c95553151e1
> --- /dev/null
> +++ b/arch/mips/include/asm/mach-intel-mips/war.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ASM_MIPS_MACH_INTEL_MIPS_WAR_H
> +#define __ASM_MIPS_MACH_INTEL_MIPS_WAR_H
> +
> +#define R4600_V1_INDEX_ICACHEOP_WAR 0
> +#define R4600_V1_HIT_CACHEOP_WAR 0
> +#define R4600_V2_HIT_CACHEOP_WAR 0
> +#define R5432_CP0_INTERRUPT_WAR 0
> +#define BCM1250_M3_WAR 0
> +#define SIBYTE_1956_WAR 0
> +#define MIPS4K_ICACHE_REFILL_WAR 0
> +#define MIPS_CACHE_SYNC_WAR 0
> +#define TX49XX_ICACHE_INDEX_INV_WAR 0
> +#define ICACHE_REFILLS_WORKAROUND_WAR 0
> +#define R10000_LLSC_WAR 0
> +#define MIPS34K_MISSED_ITLB_WAR 0
> +
> +#endif /* __ASM_MIPS_MACH_INTEL_MIPS_WAR_H */
Since you need none of these workarounds, you shouldn't need war.h at
all.
> diff --git a/arch/mips/intel-mips/Kconfig b/arch/mips/intel-mips/Kconfig
> new file mode 100644
> index 000000000000..35d2ae2b5408
> --- /dev/null
> +++ b/arch/mips/intel-mips/Kconfig
> @@ -0,0 +1,22 @@
> +if INTEL_MIPS
> +
> +choice
> + prompt "Built-in device tree"
> + help
> + Legacy bootloaders do not pass a DTB pointer to the kernel, so
> + if a "wrapper" is not being used, the kernel will need to include
> + a device tree that matches the target board.
> +
> + The builtin DTB will only be used if the firmware does not supply
> + a valid DTB.
> +
> +config DTB_INTEL_MIPS_NONE
> + bool "None"
> +
> +config DTB_INTEL_MIPS_GRX500
> + bool "Intel MIPS GRX500 Board"
> + select BUILTIN_DTB
> +
> +endchoice
> +
> +endif
So do you actually have both styles of bootloader?
> diff --git a/arch/mips/intel-mips/prom.c b/arch/mips/intel-mips/prom.c
> new file mode 100644
> index 000000000000..a1b1393c13bc
> --- /dev/null
> +++ b/arch/mips/intel-mips/prom.c
>% >% >%
> +static void __init prom_init_cmdline(void)
> +{
> + int i;
> + int argc;
> + char **argv;
> +
> + /*
> + * If u-boot pass parameters, it is ok, however, if without u-boot
> + * JTAG or other tool has to reset all register value before it goes
> + * emulation most likely belongs to this category
> + */
> + if (fw_arg0 == 0 || fw_arg1 == 0)
> + return;
I don't understand what you're trying to say here, or why this check
exists. If you boot with fw_arg0 == fw_arg1 == 0 then you'd just hit the
loop below right, and execute zero iterations of it? That seems like it
would be fine without this special case.
> + /*
> + * a0: fw_arg0 - the number of string in init cmdline
> + * a1: fw_arg1 - the address of string in init cmdline
> + *
> + * In accordance with the MIPS UHI specification,
> + * the bootloader can pass the following arguments to the kernel:
> + * - $a0: -2.
> + * - $a1: KSEG0 address of the flattened device-tree blob.
> + */
> + if (fw_arg0 == -2)
> + return;
> +
> + argc = fw_arg0;
> + argv = (char **)KSEG1ADDR(fw_arg1);
> +
> + arcs_cmdline[0] = '\0';
> +
> + for (i = 0; i < argc; i++) {
> + char *p = (char *)KSEG1ADDR(argv[i]);
> +
> + if (argv[i] && *p) {
> + strlcat(arcs_cmdline, p, sizeof(arcs_cmdline));
> + strlcat(arcs_cmdline, " ", sizeof(arcs_cmdline));
> + }
> + }
Why do you need to use kseg1? Why can't the arguments be accessed
cached?
Are the arguments passed as virtual or physical addresses? If virtual &
we can access them cached then you could replace all of this with a call
to fw_init_cmdline().
> +static int __init plat_enable_iocoherency(void)
> +{
> + if (!mips_cps_numiocu(0))
> + return 0;
> +
> + /* Nothing special needs to be done to enable coherency */
> + pr_info("Coherence Manager IOCU detected\n");
> + /* Second IOCU for MPE or other master access register */
> + write_gcr_reg0_base(0xa0000000);
> + write_gcr_reg0_mask(0xf8000000 | CM_GCR_REGn_MASK_CMTGT_IOCU1);
> + return 1;
> +}
> +
> +static void __init plat_setup_iocoherency(void)
> +{
> + if (plat_enable_iocoherency() &&
> + coherentio == IO_COHERENCE_DISABLED) {
> + pr_info("Hardware DMA cache coherency disabled\n");
> + return;
> + }
> + panic("This kind of IO coherency is not supported!");
> +}
Since you select CONFIG_DMA_NONCOHERENT in Kconfig, coherentio will
always equal IO_COHERENCE_DISABLED. That suggests to me that the above 2
functions are probably useless, or at least needlessly convoluted.
> +static int __init plat_publish_devices(void)
> +{
> + if (!of_have_populated_dt())
> + return 0;
> + return of_platform_populate(NULL, of_default_bus_match_table, NULL,
> + NULL);
> +}
> +arch_initcall(plat_publish_devices);
The core DT code calls of_platform_populate() already (see
of_platform_default_populate_init()), so you can remove this function.
Thanks,
Paul
^ permalink raw reply
* Re: [PATCH v3 3/8] mailbox: Add transmit done by blocking option
From: Jassi Brar @ 2018-08-03 12:54 UTC (permalink / raw)
To: Mikko Perttunen
Cc: Greg KH, Thierry Reding, Jon Hunter, Devicetree List,
linux-serial, linux-tegra,
, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, srv_heupstream,
Linux Kernel Mailing List
In-Reply-To: <20180702114033.15654-4-mperttunen@nvidia.com>
On Mon, Jul 2, 2018 at 5:10 PM, Mikko Perttunen <mperttunen@nvidia.com> wrote:
> Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the
> send_data function of the mailbox driver is expected to block until
> the message has been sent. The new option is used with the Tegra
> Combined UART driver to minimize unnecessary overhead when transmitting
> data.
>
1) TXDONE_BY_BLOCK flag :-
Have you tried setting the flag mbox_chan->mbox_client->tx_block ?
2) Implementing TEGRA_HSP_MBOX_TYPE_SM :-
In mailbox framework, a controller is a collection of identical
channels. That is, instances of the same class.
So ideally, in probe you should populate a controller for each
type of channel, i.e, DB, SM, SS and AS.
^ permalink raw reply
* Re: [PATCH v4 03/10] arm64: dts: Add Mediatek SoC MT8183 and evaluation board dts and Makefile
From: Matthias Brugger @ 2018-08-03 12:52 UTC (permalink / raw)
To: Erin Lo, Rob Herring, Mark Rutland, Thomas Gleixner, Jason Cooper,
Marc Zyngier, Greg Kroah-Hartman, Stephen Boyd
Cc: devicetree, srv_heupstream, linux-kernel, linux-serial,
linux-mediatek, linux-arm-kernel, yingjoe.chen, mars.cheng,
eddie.huang, linux-clk, Ben Ho
In-Reply-To: <1533015487-60189-4-git-send-email-erin.lo@mediatek.com>
On 31/07/18 07:38, Erin Lo wrote:
> From: Ben Ho <Ben.Ho@mediatek.com>
>
> Add basic chip support for Mediatek 8183
>
> Signed-off-by: Ben Ho <Ben.Ho@mediatek.com>
> Signed-off-by: Erin Lo <erin.lo@mediatek.com>
> ---
> arch/arm64/boot/dts/mediatek/Makefile | 1 +
> arch/arm64/boot/dts/mediatek/mt8183-evb.dts | 23 +++++
> arch/arm64/boot/dts/mediatek/mt8183.dtsi | 146 ++++++++++++++++++++++++++++
> 3 files changed, 170 insertions(+)
> create mode 100644 arch/arm64/boot/dts/mediatek/mt8183-evb.dts
> create mode 100644 arch/arm64/boot/dts/mediatek/mt8183.dtsi
>
> diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
> index 7506b0d..a91d462 100644
> --- a/arch/arm64/boot/dts/mediatek/Makefile
> +++ b/arch/arm64/boot/dts/mediatek/Makefile
> @@ -6,3 +6,4 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt6795-evb.dtb
> dtb-$(CONFIG_ARCH_MEDIATEK) += mt6797-evb.dtb
> dtb-$(CONFIG_ARCH_MEDIATEK) += mt7622-rfb1.dtb
> dtb-$(CONFIG_ARCH_MEDIATEK) += mt8173-evb.dtb
> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-evb.dtb
> diff --git a/arch/arm64/boot/dts/mediatek/mt8183-evb.dts b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts
> new file mode 100644
> index 0000000..2a3dd5a
> --- /dev/null
> +++ b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Ben Ho <ben.ho@mediatek.com>
> + * Erin Lo <erin.lo@mediatek.com>
> + */
> +
> +/dts-v1/;
> +#include "mt8183.dtsi"
> +
> +/ {
> + model = "MediaTek MT8183 evaluation board";
> + compatible = "mediatek,mt8183-evb", "mediatek,mt8183";
> +
> + memory@40000000 {
> + device_type = "memory";
> + reg = <0 0x40000000 0 0x80000000>;
> + };
> +
> + chosen {
> + stdout-path = "serial0:921600n8";
This should go into 10/10.
Regards,
Matthias
> + };
> +};
> diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> new file mode 100644
> index 0000000..1553265
> --- /dev/null
> +++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> @@ -0,0 +1,146 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Ben Ho <ben.ho@mediatek.com>
> + * Erin Lo <erin.lo@mediatek.com>
> + */
> +
> +#include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +/ {
> + compatible = "mediatek,mt8183";
> + interrupt-parent = <&sysirq>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + cpu-map {
> + cluster0 {
> + core0 {
> + cpu = <&cpu0>;
> + };
> + core1 {
> + cpu = <&cpu1>;
> + };
> + core2 {
> + cpu = <&cpu2>;
> + };
> + core3 {
> + cpu = <&cpu3>;
> + };
> + };
> +
> + cluster1 {
> + core0 {
> + cpu = <&cpu4>;
> + };
> + core1 {
> + cpu = <&cpu5>;
> + };
> + core2 {
> + cpu = <&cpu6>;
> + };
> + core3 {
> + cpu = <&cpu7>;
> + };
> + };
> + };
> +
> + cpu0: cpu@000 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + reg = <0x000>;
> + enable-method = "psci";
> + };
> +
> + cpu1: cpu@001 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + reg = <0x001>;
> + enable-method = "psci";
> + };
> +
> + cpu2: cpu@002 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + reg = <0x002>;
> + enable-method = "psci";
> + };
> +
> + cpu3: cpu@003 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + reg = <0x003>;
> + enable-method = "psci";
> + };
> +
> + cpu4: cpu@100 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a73";
> + reg = <0x100>;
> + enable-method = "psci";
> + };
> +
> + cpu5: cpu@101 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a73";
> + reg = <0x101>;
> + enable-method = "psci";
> + };
> +
> + cpu6: cpu@102 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a73";
> + reg = <0x102>;
> + enable-method = "psci";
> + };
> +
> + cpu7: cpu@103 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a73";
> + reg = <0x103>;
> + enable-method = "psci";
> + };
> + };
> +
> + psci {
> + compatible = "arm,psci-1.0";
> + method = "smc";
> + };
> +
> + timer {
> + compatible = "arm,armv8-timer";
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>;
> + };
> +
> + gic: interrupt-controller@0c000000 {
> + compatible = "arm,gic-v3";
> + #interrupt-cells = <3>;
> + interrupt-parent = <&gic>;
> + interrupt-controller;
> + reg = <0 0x0c000000 0 0x40000>, /* GICD */
> + <0 0x0c100000 0 0x200000>, /* GICR */
> + <0 0x0c400000 0 0x2000>, /* GICC */
> + <0 0x0c410000 0 0x1000>, /* GICH */
> + <0 0x0c420000 0 0x2000>; /* GICV */
> +
> + interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> + };
> +
> + sysirq: intpol-controller@0c530a80 {
> + compatible = "mediatek,mt8183-sysirq",
> + "mediatek,mt6577-sysirq";
> + interrupt-controller;
> + #interrupt-cells = <3>;
> + interrupt-parent = <&gic>;
> + reg = <0 0x0c530a80 0 0x50>;
> + };
> +};
>
^ permalink raw reply
* Re: [PATCH v2 14/18] serial: intel: Add CCF support
From: Greg Kroah-Hartman @ 2018-08-03 10:30 UTC (permalink / raw)
To: Wu, Songjun
Cc: hua.ma, yixin.zhu, chuanhua.lei, qi-ming.wu, linux-mips,
linux-clk, linux-serial, devicetree, linux-kernel, Jiri Slaby
In-Reply-To: <763bba56-3701-7fe9-9b31-4710594b40d5@linux.intel.com>
On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote:
>
>
> On 8/3/2018 1:56 PM, Greg Kroah-Hartman wrote:
> > On Fri, Aug 03, 2018 at 11:02:33AM +0800, Songjun Wu wrote:
> > > Previous implementation uses platform-dependent API to get the clock.
> > > Those functions are not available for other SoC which uses the same IP.
> > > The CCF (Common Clock Framework) have an abstraction based APIs for
> > > clock. In future, the platform specific code will be removed when the
> > > legacy soc use CCF as well.
> > > Change to use CCF APIs to get clock and rate. So that different SoCs
> > > can use the same driver.
> > >
> > > Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
> > > ---
> > >
> > > Changes in v2: None
> > >
> > > drivers/tty/serial/lantiq.c | 11 +++++++++++
> > > 1 file changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
> > > index 36479d66fb7c..35518ab3a80d 100644
> > > --- a/drivers/tty/serial/lantiq.c
> > > +++ b/drivers/tty/serial/lantiq.c
> > > @@ -26,7 +26,9 @@
> > > #include <linux/clk.h>
> > > #include <linux/gpio.h>
> > > +#ifdef CONFIG_LANTIQ
> > > #include <lantiq_soc.h>
> > > +#endif
> > That is never how you do this in Linux, you know better.
> >
> > Please go and get this patchset reviewed and signed-off-by from other
> > internal Intel kernel developers before resending it next time. It is
> > their job to find and fix your basic errors like this, not ours.
> Thank you for your comment.
> Actually, we have discussed this issue internally.
> We put the reason why we use "#ifdef CONFIG_LANTIQ" preprocessor in commit
> message in "[PATCH v2 08/18] serial: intel: Get serial id from dts".
> Please refer the commit message below.
>
> "#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC
> macro is defined in lantiq_soc.h.
> lantiq_soc.h is in arch path for legacy product support.
>
> arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
>
> If "#ifdef preprocessor" is changed to
> "if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled,
> code using LTQ_EARLY_ASC is compiled.
> Compilation will fail for no LTQ_EARLY_ASC defined.
Sorry, but no. Why is this one tiny driver/chip somehow more "special"
than all of the tens of thousands of other devices we support to warrent
it getting some sort of special exception to do things differently?
What happens to the next device that wants to do it this way?
Our coding style and rules are there for a reason, do not violate them
thinking your device is the only one that matters.
Do it properly, again, you all know better than this.
greg k-h
^ permalink raw reply
* Re: [PATCH v2 2/2] tty/serial: atmel: add ISO7816 support
From: Ludovic Desroches @ 2018-08-03 8:50 UTC (permalink / raw)
To: Richard Genoud
Cc: linux-serial, linux-arch, linux-arm-kernel, gregkh, jslaby, arnd,
nicolas.ferre, alexandre.belloni, linux-kernel
In-Reply-To: <1353b166-c1d0-6be6-a873-a54d210869fd@sorico.fr>
Hi Richard,
On Fri, Jul 27, 2018 at 04:39:17PM +0200, Richard Genoud wrote:
> Hi Ludovic,
>
> On 19/07/2018 10:47, Ludovic Desroches wrote:
> > From: Nicolas Ferre <nicolas.ferre@microchip.com>
> >
> > When mode is set in atmel_config_iso7816() we backup last RS232 mode
> > for coming back to this mode if requested.
> > Also allow setup of T=0 and T=1 parameter and basic support in set_termios
> > function as well.
> > Report NACK and ITER errors in irq handler.
> >
> > Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> > Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
> > ---
> > drivers/tty/serial/atmel_serial.c | 170 +++++++++++++++++++++++++++++++++++---
> > drivers/tty/serial/atmel_serial.h | 3 +-
> > 2 files changed, 162 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> > index 8e4428725848..cec958f1e7d4 100644
> > --- a/drivers/tty/serial/atmel_serial.c
> > +++ b/drivers/tty/serial/atmel_serial.c
> > @@ -34,6 +34,7 @@
> > #include <linux/suspend.h>
> > #include <linux/mm.h>
> >
> > +#include <asm/div64.h>
> > #include <asm/io.h>
> > #include <asm/ioctls.h>
> >
> > @@ -147,6 +148,8 @@ struct atmel_uart_port {
> > struct circ_buf rx_ring;
> >
> > struct mctrl_gpios *gpios;
> > + u32 backup_mode; /* MR saved during iso7816 operations */
> > + u32 backup_brgr; /* BRGR saved during iso7816 operations */
> > unsigned int tx_done_mask;
> > u32 fifo_size;
> > u32 rts_high;
> > @@ -362,6 +365,132 @@ static int atmel_config_rs485(struct uart_port *port,
> > return 0;
> > }
> >
> > +static unsigned int atmel_calc_cd(struct uart_port *port,
> > + struct serial_iso7816 *iso7816conf)
> > +{
> > + struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
> > + unsigned int cd;
> > + u64 mck_rate;
> > +
> > + mck_rate = (u64)clk_get_rate(atmel_port->clk);
> > + do_div(mck_rate, iso7816conf->clk);
> > + cd = mck_rate;
> > + return cd;
> > +}
> > +
> > +static unsigned int atmel_calc_fidi(struct uart_port *port,
> > + struct serial_iso7816 *iso7816conf)
> > +{
> > + u64 fidi = 0;
> > +
> > + if (iso7816conf->sc_fi && iso7816conf->sc_di) {
> > + fidi = (u64)iso7816conf->sc_fi;
> > + do_div(fidi, iso7816conf->sc_di);
> > + }
> > + return (u32)fidi;
> > +}
> > +
> > +/* Enable or disable the iso7816 support */
> > +/* Called with interrupts disabled */
> > +static int atmel_config_iso7816(struct uart_port *port,
> > + struct serial_iso7816 *iso7816conf)
> > +{
> > + struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
> > + unsigned int mode, t;
> > + unsigned int cd, fidi;
> > + int ret = 0;
> > +
> > + /* Disable RX and TX */
> > + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RXDIS | ATMEL_US_TXDIS);
> > + /* Disable interrupts */
> > + atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
> > +
> > + mode = atmel_uart_readl(port, ATMEL_US_MR);
> > +
> > + if (iso7816conf->flags & SER_ISO7816_ENABLED) {
> > + mode &= ~ATMEL_US_USMODE;
> > +
> > + if ((iso7816conf->flags & SER_ISO7816_T_PARAM)
> > + == SER_ISO7816_T(0)) {
> > + mode |= ATMEL_US_USMODE_ISO7816_T0;
> > + t = 0;
> > + } else if ((iso7816conf->flags & SER_ISO7816_T_PARAM)
> > + == SER_ISO7816_T(1)) {
> > + mode |= ATMEL_US_USMODE_ISO7816_T1;
> > + t = 1;
> > + } else {
> > + dev_warn(port->dev, "ISO7816 Type not supported. Resetting\n");
> > + memset(iso7816conf, 0, sizeof(struct serial_iso7816));
> > + goto err_out;
> > + }
> > +
> > + dev_dbg(port->dev, "Setting USART to ISO7816 mode T%d\n", t);
> > +
> > + mode &= ~(ATMEL_US_USCLKS | ATMEL_US_CHRL
> > + | ATMEL_US_NBSTOP | ATMEL_US_PAR);
> This could be merged in the mode &= line above.
>
Ok
> > +
> > + /* NACK configuration */
> > + if ((iso7816conf->flags & SER_ISO7816_T_PARAM)
> > + == SER_ISO7816_T(0))
> > + mode |= ATMEL_US_DSNACK;
> > + else
> > + mode |= ATMEL_US_INACK;
> This could be also part of the if () above.
>
Ok
> > + /* select mck clock, and output */
> > + mode |= ATMEL_US_USCLKS_MCK | ATMEL_US_CLKO;
> > + /* set parity for normal/inverse mode + max iterations */
> > + mode |= ATMEL_US_PAR_EVEN | ATMEL_US_NBSTOP_1 | (3 << 24);
> Is this really needed ?
> In the documentation, I found:
> "The configuration is 8 data bits, even parity and 1 or 2 stop bits,
> regardless of the values programmed in the CHRL, MODE9, PAR and CHMODE
> fields."
It's not very clear. It is followed by "Parity Bit (PAR) can be used to
transmit in Normal or Inverse mode." and you have to choose 1 or 2 stop
bits.
> And, for MAX_ITERATIONS, could you add a macro instead of (x << 24) ?
> (ATMEL_US_MAX_ITER mask is already defined).
Yes
> And why 3 ? Should the user-space be allowed to control the max
> automatic iteration ? or is it more like a "same-value-for-every-one"
> thing ?
>
It's an arbitrary choice. I have not seen any reference to number of
repetition in the ISO7816 spec but I may miss it. So I would say it's
not the same value for everyone. Is it useful to let the user setting
it? I don't know, maybe we can wait for a request to export this
setting.
> > +
> > + cd = atmel_calc_cd(port, iso7816conf);
> > + fidi = atmel_calc_fidi(port, iso7816conf);
> > + if (fidi < 0) {
> I guess you meant (fidi == 0)
> Because fidi is unsigned and atmel_calc_fidi() returns also an unsigned.
Right.
>
> > + dev_warn(port->dev, "ISO7816 fidi = 0, Generator generates no signal\n");
> > + } else if (fidi == 1 || fidi == 2) {
> > + dev_err(port->dev, "ISO7816 fidi = %u, value not supported\n", fidi);
> > + ret = -EINVAL;
> > + goto err_out;
> > + }
> And you may also want to check upper values ( <2048 or <65536, depending
> on the SoC)
>
I'll do, I assumed that user space won't request weird values but it's
probably better to not trust him.
> > +
> > + if (!(port->iso7816.flags & SER_ISO7816_ENABLED)) {
> > + /* port not yet in iso7816 mode: store configuration */
> > + atmel_port->backup_mode = atmel_uart_readl(port, ATMEL_US_MR);
> > + atmel_port->backup_brgr = atmel_uart_readl(port, ATMEL_US_BRGR);
> > + }
> > +
> > + /* Actually set ISO7816 mode */
> > + atmel_uart_writel(port, ATMEL_US_TTGR, iso7816conf->tg);
> iso7816conf->tg comes from user-space unchecked. AFAIK, max value is 255
Ok I'll check it.
>
> > + atmel_uart_writel(port, ATMEL_US_BRGR, cd);
> > + atmel_uart_writel(port, ATMEL_US_FIDIR, fidi);
> > +
> > + atmel_port->tx_done_mask = ATMEL_US_TXEMPTY | ATMEL_US_NACK | ATMEL_US_ITERATION;
> > + } else {
> > + dev_dbg(port->dev, "Setting UART to RS232\n");
> > + /* back to last RS232 settings */
> > + mode = atmel_port->backup_mode;
> > + memset(iso7816conf, 0, sizeof(struct serial_iso7816));
> > + atmel_uart_writel(port, ATMEL_US_TTGR, 0);
> > + atmel_uart_writel(port, ATMEL_US_BRGR, atmel_port->backup_brgr);
> > + atmel_uart_writel(port, ATMEL_US_FIDIR, 0x174);
> > +
> > + if (atmel_use_pdc_tx(port))
> > + atmel_port->tx_done_mask = ATMEL_US_ENDTX |
> > + ATMEL_US_TXBUFE;
> > + else
> > + atmel_port->tx_done_mask = ATMEL_US_TXRDY;
> > + }
> > +
> > + port->iso7816 = *iso7816conf;
> > +
> > + atmel_uart_writel(port, ATMEL_US_MR, mode);
> > +
> > +err_out:
> > + /* Enable interrupts */
> > + atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask);
> > + /* Enable RX and TX */
> > + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RXEN | ATMEL_US_TXEN);
> Is it all right to enable RX/TX unconditionally here ?
>
As I didn't see any issue at the moment, I kept it like this. But I have
noticed this warning in the datasheet and I keep it in mind:
"Enabling both the receiver and the transmitter at the same time in
ISO7816 mode maylead to unpredictable results"
> > +
> > + return ret;
> > +}
> > +
> > /*
> > * Return TIOCSER_TEMT when transmitter FIFO and Shift register is empty.
> > */
> > @@ -481,8 +610,9 @@ static void atmel_stop_tx(struct uart_port *port)
> > /* Disable interrupts */
> > atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
> >
> > - if ((port->rs485.flags & SER_RS485_ENABLED) &&
> > - !(port->rs485.flags & SER_RS485_RX_DURING_TX))
> > + if (((port->rs485.flags & SER_RS485_ENABLED) &&
> > + !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
> > + port->iso7816.flags & SER_ISO7816_ENABLED)
> > atmel_start_rx(port);
> > }
> >
> > @@ -500,8 +630,9 @@ static void atmel_start_tx(struct uart_port *port)
> > return;
> >
> > if (atmel_use_pdc_tx(port) || atmel_use_dma_tx(port))
> > - if ((port->rs485.flags & SER_RS485_ENABLED) &&
> > - !(port->rs485.flags & SER_RS485_RX_DURING_TX))
> > + if (((port->rs485.flags & SER_RS485_ENABLED) &&
> > + !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
> > + port->iso7816.flags & SER_ISO7816_ENABLED)
> > atmel_stop_rx(port);
> >
> > if (atmel_use_pdc_tx(port))
> > @@ -799,8 +930,9 @@ static void atmel_complete_tx_dma(void *arg)
> > */
> > if (!uart_circ_empty(xmit))
> > atmel_tasklet_schedule(atmel_port, &atmel_port->tasklet_tx);
> > - else if ((port->rs485.flags & SER_RS485_ENABLED) &&
> > - !(port->rs485.flags & SER_RS485_RX_DURING_TX)) {
> > + else if (((port->rs485.flags & SER_RS485_ENABLED) &&
> > + !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
> > + port->iso7816.flags & SER_ISO7816_ENABLED) {
> > /* DMA done, stop TX, start RX for RS485 */
> > atmel_start_rx(port);
> > }
> > @@ -1281,6 +1413,9 @@ atmel_handle_status(struct uart_port *port, unsigned int pending,
> > wake_up_interruptible(&port->state->port.delta_msr_wait);
> > }
> > }
> > +
> > + if (pending & (ATMEL_US_NACK | ATMEL_US_ITERATION))
> > + dev_dbg(port->dev, "ISO7816 ERROR (0x%08x)\n", pending);
> > }
> >
> > /*
> > @@ -1373,8 +1508,9 @@ static void atmel_tx_pdc(struct uart_port *port)
> > atmel_uart_writel(port, ATMEL_US_IER,
> > atmel_port->tx_done_mask);
> > } else {
> > - if ((port->rs485.flags & SER_RS485_ENABLED) &&
> > - !(port->rs485.flags & SER_RS485_RX_DURING_TX)) {
> > + if (((port->rs485.flags & SER_RS485_ENABLED) &&
> > + !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
> > + port->iso7816.flags & SER_ISO7816_ENABLED) {
> > /* DMA done, stop TX, start RX for RS485 */
> > atmel_start_rx(port);
> > }
> > @@ -2099,6 +2235,17 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
> > atmel_uart_writel(port, ATMEL_US_TTGR,
> > port->rs485.delay_rts_after_send);
> > mode |= ATMEL_US_USMODE_RS485;
> > + } else if (port->iso7816.flags & SER_ISO7816_ENABLED) {
> > + atmel_uart_writel(port, ATMEL_US_TTGR, port->iso7816.tg);
> > + /* select mck clock, and output */
> > + mode |= ATMEL_US_USCLKS_MCK | ATMEL_US_CLKO;
> > + /* set max iterations */
> > + mode |= (3 << 24);
> Same remark for macro / hardcoded value.
>
OK
> > + if ((port->iso7816.flags & SER_ISO7816_T_PARAM)
> > + == SER_ISO7816_T(0))
> > + mode |= ATMEL_US_USMODE_ISO7816_T0;
> > + else
> > + mode |= ATMEL_US_USMODE_ISO7816_T1;
> > } else if (termios->c_cflag & CRTSCTS) {
> > /* RS232 with hardware handshake (RTS/CTS) */
> > if (atmel_use_fifo(port) &&
> > @@ -2175,7 +2322,8 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
> > }
> > quot = cd | fp << ATMEL_US_FP_OFFSET;
> >
> > - atmel_uart_writel(port, ATMEL_US_BRGR, quot);
> > + if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
> > + atmel_uart_writel(port, ATMEL_US_BRGR, quot);
> > atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
> > atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
> > atmel_port->tx_stopped = false;
> > @@ -2355,6 +2503,7 @@ static int atmel_init_port(struct atmel_uart_port *atmel_port,
> > port->mapbase = pdev->resource[0].start;
> > port->irq = pdev->resource[1].start;
> > port->rs485_config = atmel_config_rs485;
> > + port->iso7816_config = atmel_config_iso7816;
> > port->membase = NULL;
> >
> > memset(&atmel_port->rx_ring, 0, sizeof(atmel_port->rx_ring));
> > @@ -2379,7 +2528,8 @@ static int atmel_init_port(struct atmel_uart_port *atmel_port,
> > }
> >
> > /* Use TXEMPTY for interrupt when rs485 else TXRDY or ENDTX|TXBUFE */
> > - if (port->rs485.flags & SER_RS485_ENABLED)
> > + if (port->rs485.flags & SER_RS485_ENABLED ||
> > + port->iso7816.flags & SER_ISO7816_ENABLED)
> please update the comment above.
>
OK
> > atmel_port->tx_done_mask = ATMEL_US_TXEMPTY;
> > else if (atmel_use_pdc_tx(port)) {
> > port->fifosize = PDC_BUFFER_SIZE;
> > diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
> > index ba3a2437cde4..fff51f5fe8bc 100644
> > --- a/drivers/tty/serial/atmel_serial.h
> > +++ b/drivers/tty/serial/atmel_serial.h
> > @@ -124,7 +124,8 @@
> > #define ATMEL_US_TTGR 0x28 /* Transmitter Timeguard Register */
> > #define ATMEL_US_TG GENMASK(7, 0) /* Timeguard Value */
> >
> > -#define ATMEL_US_FIDI 0x40 /* FI DI Ratio Register */
> > +#define ATMEL_US_FIDIR 0x40 /* FI DI Ratio Register */
> > +#define ATMEL_US_FIDI GENMASK(15, 0) /* FIDI ratio */
> > #define ATMEL_US_NER 0x44 /* Number of Errors Register */
> > #define ATMEL_US_IF 0x4c /* IrDA Filter Register */
> >
> >
>
> Thanks !
>
> Richard.
Thanks for your review, I'll send a new version.
Regards
Ludovic
^ permalink raw reply
* Re: [PATCH v2 14/18] serial: intel: Add CCF support
From: Wu, Songjun @ 2018-08-03 7:33 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: hua.ma, yixin.zhu, chuanhua.lei, qi-ming.wu, linux-mips,
linux-clk, linux-serial, devicetree, linux-kernel, Jiri Slaby
In-Reply-To: <20180803055640.GA32226@kroah.com>
On 8/3/2018 1:56 PM, Greg Kroah-Hartman wrote:
> On Fri, Aug 03, 2018 at 11:02:33AM +0800, Songjun Wu wrote:
>> Previous implementation uses platform-dependent API to get the clock.
>> Those functions are not available for other SoC which uses the same IP.
>> The CCF (Common Clock Framework) have an abstraction based APIs for
>> clock. In future, the platform specific code will be removed when the
>> legacy soc use CCF as well.
>> Change to use CCF APIs to get clock and rate. So that different SoCs
>> can use the same driver.
>>
>> Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
>> ---
>>
>> Changes in v2: None
>>
>> drivers/tty/serial/lantiq.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
>> index 36479d66fb7c..35518ab3a80d 100644
>> --- a/drivers/tty/serial/lantiq.c
>> +++ b/drivers/tty/serial/lantiq.c
>> @@ -26,7 +26,9 @@
>> #include <linux/clk.h>
>> #include <linux/gpio.h>
>>
>> +#ifdef CONFIG_LANTIQ
>> #include <lantiq_soc.h>
>> +#endif
> That is never how you do this in Linux, you know better.
>
> Please go and get this patchset reviewed and signed-off-by from other
> internal Intel kernel developers before resending it next time. It is
> their job to find and fix your basic errors like this, not ours.
Thank you for your comment.
Actually, we have discussed this issue internally.
We put the reason why we use "#ifdef CONFIG_LANTIQ" preprocessor in commit
message in "[PATCH v2 08/18] serial: intel: Get serial id from dts".
Please refer the commit message below.
"#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC
macro is defined in lantiq_soc.h.
lantiq_soc.h is in arch path for legacy product support.
arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
If "#ifdef preprocessor" is changed to
"if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled,
code using LTQ_EARLY_ASC is compiled.
Compilation will fail for no LTQ_EARLY_ASC defined.
^ permalink raw reply
* Re: [PATCH v2 15/18] serial: intel: Support more platform
From: Wu, Songjun @ 2018-08-03 7:21 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: hua.ma, yixin.zhu, chuanhua.lei, qi-ming.wu, linux-mips,
linux-clk, linux-serial, devicetree, linux-kernel, Jiri Slaby
In-Reply-To: <20180803055709.GB32226@kroah.com>
On 8/3/2018 1:57 PM, Greg Kroah-Hartman wrote:
> On Fri, Aug 03, 2018 at 11:02:34AM +0800, Songjun Wu wrote:
>> Support more platform.
>>
>> Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
>> ---
> Your changelog text makes no sense, sorry.
Thanks for your comment.
I will describe it more clearly.
^ permalink raw reply
* Re: [PATCH v2 15/18] serial: intel: Support more platform
From: Greg Kroah-Hartman @ 2018-08-03 5:57 UTC (permalink / raw)
To: Songjun Wu
Cc: hua.ma, yixin.zhu, chuanhua.lei, qi-ming.wu, linux-mips,
linux-clk, linux-serial, devicetree, linux-kernel, Jiri Slaby
In-Reply-To: <20180803030237.3366-16-songjun.wu@linux.intel.com>
On Fri, Aug 03, 2018 at 11:02:34AM +0800, Songjun Wu wrote:
> Support more platform.
>
> Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
> ---
Your changelog text makes no sense, sorry.
greg k-h
^ permalink raw reply
* Re: [PATCH v2 14/18] serial: intel: Add CCF support
From: Greg Kroah-Hartman @ 2018-08-03 5:56 UTC (permalink / raw)
To: Songjun Wu
Cc: hua.ma, yixin.zhu, chuanhua.lei, qi-ming.wu, linux-mips,
linux-clk, linux-serial, devicetree, linux-kernel, Jiri Slaby
In-Reply-To: <20180803030237.3366-15-songjun.wu@linux.intel.com>
On Fri, Aug 03, 2018 at 11:02:33AM +0800, Songjun Wu wrote:
> Previous implementation uses platform-dependent API to get the clock.
> Those functions are not available for other SoC which uses the same IP.
> The CCF (Common Clock Framework) have an abstraction based APIs for
> clock. In future, the platform specific code will be removed when the
> legacy soc use CCF as well.
> Change to use CCF APIs to get clock and rate. So that different SoCs
> can use the same driver.
>
> Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
> ---
>
> Changes in v2: None
>
> drivers/tty/serial/lantiq.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
> index 36479d66fb7c..35518ab3a80d 100644
> --- a/drivers/tty/serial/lantiq.c
> +++ b/drivers/tty/serial/lantiq.c
> @@ -26,7 +26,9 @@
> #include <linux/clk.h>
> #include <linux/gpio.h>
>
> +#ifdef CONFIG_LANTIQ
> #include <lantiq_soc.h>
> +#endif
That is never how you do this in Linux, you know better.
Please go and get this patchset reviewed and signed-off-by from other
internal Intel kernel developers before resending it next time. It is
their job to find and fix your basic errors like this, not ours.
greg k-h
^ permalink raw reply
* Re: [PATCH v2 08/18] serial: intel: Get serial id from dts
From: Greg Kroah-Hartman @ 2018-08-03 5:43 UTC (permalink / raw)
To: Songjun Wu
Cc: hua.ma, yixin.zhu, chuanhua.lei, qi-ming.wu, linux-mips,
linux-clk, linux-serial, devicetree, linux-kernel, Jiri Slaby
In-Reply-To: <20180803030237.3366-9-songjun.wu@linux.intel.com>
On Fri, Aug 03, 2018 at 11:02:27AM +0800, Songjun Wu wrote:
> Get serial id from dts.
>
> "#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC
> macro is defined in lantiq_soc.h.
> lantiq_soc.h is in arch path for legacy product support.
>
> arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
>
> If "#ifdef preprocessor" is changed to
> "if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled,
> code using LTQ_EARLY_ASC is compiled.
> Compilation will fail for no LTQ_EARLY_ASC defined.
>
> Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
> ---
>
> Changes in v2: None
>
> drivers/tty/serial/lantiq.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
> index 044128277248..836ca51460f2 100644
> --- a/drivers/tty/serial/lantiq.c
> +++ b/drivers/tty/serial/lantiq.c
> @@ -6,6 +6,7 @@
> * Copyright (C) 2007 Felix Fietkau <nbd@openwrt.org>
> * Copyright (C) 2007 John Crispin <john@phrozen.org>
> * Copyright (C) 2010 Thomas Langer, <thomas.langer@lantiq.com>
> + * Copyright (C) 2018 Intel Corporation.
Your changes here do not warrent the addition of a copyright line, don't
you agree? If not, please get a signed-off-by from your corporate
lawyer who does this this is warrented when you resend this patch.
thanks,
greg k-h
^ permalink raw reply
* [PATCH v2 18/18] dt-bindings: serial: lantiq: Add optional properties for CCF
From: Songjun Wu @ 2018-08-03 3:02 UTC (permalink / raw)
To: hua.ma, yixin.zhu, chuanhua.lei, qi-ming.wu
Cc: linux-mips, linux-clk, linux-serial, devicetree, Songjun Wu,
linux-kernel, Rob Herring, Greg Kroah-Hartman, Mark Rutland
In-Reply-To: <20180803030237.3366-1-songjun.wu@linux.intel.com>
Clocks and clock-names are updated in device tree binding.
Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
---
Changes in v2: None
Documentation/devicetree/bindings/serial/lantiq_asc.txt | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/Documentation/devicetree/bindings/serial/lantiq_asc.txt b/Documentation/devicetree/bindings/serial/lantiq_asc.txt
index 3acbd309ab9d..40e81a5818f6 100644
--- a/Documentation/devicetree/bindings/serial/lantiq_asc.txt
+++ b/Documentation/devicetree/bindings/serial/lantiq_asc.txt
@@ -6,8 +6,23 @@ Required properties:
- interrupts: the 3 (tx rx err) interrupt numbers. The interrupt specifier
depends on the interrupt-parent interrupt controller.
+Optional properties:
+- clocks: Should contain frequency clock and gate clock
+- clock-names: Should be "freq" and "asc"
+
Example:
+asc0: serial@16600000 {
+ compatible = "lantiq,asc";
+ reg = <0x16600000 0x100000>;
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SHARED 103 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SHARED 105 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SHARED 106 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cgu CLK_SSX4>, <&cgu GCLK_UART>;
+ clock-names = "freq", "asc";
+};
+
asc1: serial@e100c00 {
compatible = "lantiq,asc";
reg = <0xE100C00 0x400>;
--
2.11.0
^ permalink raw reply related
* [PATCH v2 17/18] serial: intel: Change init_lqasc to static declaration
From: Songjun Wu @ 2018-08-03 3:02 UTC (permalink / raw)
To: hua.ma, yixin.zhu, chuanhua.lei, qi-ming.wu
Cc: linux-mips, linux-clk, linux-serial, devicetree, Songjun Wu,
Greg Kroah-Hartman, linux-kernel, Jiri Slaby
In-Reply-To: <20180803030237.3366-1-songjun.wu@linux.intel.com>
init_lqasc() is only used internally, change to static declaration.
Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
---
Changes in v2: None
drivers/tty/serial/lantiq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
index 804aad60ed80..2bb8e37e6886 100644
--- a/drivers/tty/serial/lantiq.c
+++ b/drivers/tty/serial/lantiq.c
@@ -788,7 +788,7 @@ static struct platform_driver lqasc_driver = {
},
};
-int __init
+static int __init
init_lqasc(void)
{
int ret;
--
2.11.0
^ permalink raw reply related
* [PATCH v2 16/18] serial: intel: Reorder the head files
From: Songjun Wu @ 2018-08-03 3:02 UTC (permalink / raw)
To: hua.ma, yixin.zhu, chuanhua.lei, qi-ming.wu
Cc: linux-mips, linux-clk, linux-serial, devicetree, Songjun Wu,
Greg Kroah-Hartman, linux-kernel, Jiri Slaby
In-Reply-To: <20180803030237.3366-1-songjun.wu@linux.intel.com>
Reorder the head files according to the coding style.
Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
---
Changes in v2:
- New patch to reorder the head files according to the coding style.
drivers/tty/serial/lantiq.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
index 35518ab3a80d..804aad60ed80 100644
--- a/drivers/tty/serial/lantiq.c
+++ b/drivers/tty/serial/lantiq.c
@@ -9,22 +9,22 @@
* Copyright (C) 2018 Intel Corporation.
*/
-#include <linux/slab.h>
-#include <linux/ioport.h>
-#include <linux/init.h>
+#include <linux/clk.h>
#include <linux/console.h>
-#include <linux/sysrq.h>
#include <linux/device.h>
-#include <linux/tty.h>
-#include <linux/tty_flip.h>
-#include <linux/serial_core.h>
-#include <linux/serial.h>
-#include <linux/of_platform.h>
+#include <linux/gpio.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
#include <linux/of_address.h>
#include <linux/of_irq.h>
-#include <linux/io.h>
-#include <linux/clk.h>
-#include <linux/gpio.h>
+#include <linux/of_platform.h>
+#include <linux/serial.h>
+#include <linux/serial_core.h>
+#include <linux/slab.h>
+#include <linux/sysrq.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
#ifdef CONFIG_LANTIQ
#include <lantiq_soc.h>
--
2.11.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox