Linux Serial subsystem development
 help / color / mirror / Atom feed
* Re: [PATCH v2 22/35] nds32: Device tree support
From: Greentime Hu @ 2017-11-28  6:54 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
	DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
	linux-serial-u79uwXL29TY76Z2rM5mHXA, Vincent Chen
In-Reply-To: <CAK8P3a3nczwuuna8BGRQU11hhOFZMqGQqn9i_7D=Tzrc1PizFQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

2017-11-27 22:30 GMT+08:00 Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>:
> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> @@ -0,0 +1,55 @@
>> +/dts-v1/;
>> +/ {
>> +       compatible = "nds32 ae3xx";
>
> The compatible string doesn't seem to match the binding, it should always have
> vendor prefix.

Sorry I forgot to check this.
I will provide a document in bindings like
"Documentation/devicetree/bindings/nds32/andestech-boards".


>> +       #address-cells = <1>;
>> +       #size-cells = <1>;
>> +       interrupt-parent = <&intc>;
>> +
>> +       chosen {
>> +               bootargs = "earlycon console=ttyS0,38400n8 debug loglevel=7";
>> +               stdout-path = &serial0;
>> +       };
>
> I would drop the bootargs here, this is something that should be set by the
> bootloader and is up to the user.

Thanks
I will drop it in the next version patch.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 10/35] nds32: Atomic operations
From: Vincent Chen @ 2017-11-28  4:24 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Greentime Hu, Greentime, Linux Kernel Mailing List, Arnd Bergmann,
	linux-arch, Thomas Gleixner, Jason Cooper, Marc Zyngier,
	Rob Herring, Networking, DTML, Al Viro, David Howells,
	Will Deacon, Daniel Lezcano, linux-serial, Vincent Chen
In-Reply-To: <20171127135724.yy2jjjuby3xu74mm@lakrids.cambridge.arm.com>

2017-11-27 21:57 GMT+08:00 Mark Rutland <mark.rutland@arm.com>:
> Hi,
>
> On Mon, Nov 27, 2017 at 08:27:57PM +0800, Greentime Hu wrote:
>> +static inline void arch_spin_unlock(arch_spinlock_t * lock)
>> +{
>> +     asm volatile(
>> +             "xor    $r15,  $r15, $r15\n"
>> +             "swi    $r15, [%0]\n"
>> +             :
>> +             :"r"(&lock->lock)
>> +             :"memory");
>> +}
>
> This looks suspicious. There's no clobber for $r15, so isn't this
> corrupting a GPR behind the back of the compiler?
>

Thanks.
$r15 is reserved for assembler.
For safety, compiler always put $r15 in clobber register list by default.


> Shouldn't there be a tmp variable for the register allocation rather
> than hard-coding $r15?
>
>> +static inline void arch_write_unlock(arch_rwlock_t * rw)
>> +{
>> +     asm volatile(
>> +             "xor    $r15, $r15, $r15\n"
>> +             "swi    $r15, [%0]\n"
>> +             :
>> +             :"r"(&rw->lock)
>> +             :"memory","$r15");
>> +}
>
> This time you have a clobber, but it's still not clear to me why you
> don't use a tmp variable and leave the register allocation to the
> compiler.
>

Thanks.
When I recheck the code, I found spinlock.h is not needed due that we
do not support SMP.
So, We decide to remove spinlock.h in the next version patch.

Vincent
> Thanks,
> Mark.

^ permalink raw reply

* Re: [PATCH v2 29/35] dt-bindings: nds32 CPU Bindings
From: Greentime Hu @ 2017-11-28  3:18 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Greentime, Linux Kernel Mailing List, Arnd Bergmann, linux-arch,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring, netdev,
	Vincent Chen, DTML, Al Viro, David Howells, Will Deacon,
	Daniel Lezcano, linux-serial-u79uwXL29TY76Z2rM5mHXA, Vincent Chen,
	Rick Chen, Zong Li
In-Reply-To: <20171127134232.q343uymer47zt74m-agMKViyK24J5pKCnmE3YQBJ8xKzm50AiAL8bYrjMMd8@public.gmane.org>

2017-11-27 21:42 GMT+08:00 Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>:
> Him
>
> On Mon, Nov 27, 2017 at 08:28:16PM +0800, Greentime Hu wrote:
>> From: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>>
>> This patch adds nds32 CPU binding documents.
>>
>> Signed-off-by: Vincent Chen <vincentc-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>> Signed-off-by: Rick Chen <rick-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>> Signed-off-by: Zong Li <zong-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>> Signed-off-by: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/nds32/cpus.txt |   32 ++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/nds32/cpus.txt
>>
>> diff --git a/Documentation/devicetree/bindings/nds32/cpus.txt b/Documentation/devicetree/bindings/nds32/cpus.txt
>> new file mode 100644
>> index 0000000..c302c89
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/nds32/cpus.txt
>> @@ -0,0 +1,32 @@
>> +* Andestech Processor Binding
>> +
>> +This binding specifies what properties must be available in the device tree
>> +representation of a Andestech Processor Core, which is the root node in the
>> +tree.
>> +
>> +Required properties:
>> +
>> +     - compatible:
>> +             Usage: required
>> +             Value type: <string>
>> +             Definition: should be one of:
>> +                     "andestech,n13"
>> +                     "andestech,n15"
>> +                     "andestech,d15"
>> +                     "andestech,n10"
>> +                     "andestech,d10"
>> +                     "andestech,nds32v3"
>
> Are these specific parts, or architecture versions?
>
> I guess "andestech,nds32v3" is an architecture version, and the rest are
> specific parts?

Yes, nds32v3 is an architecture version and the rest are nds32v3
compatible processor's name.

>> +     - device_type
>> +             Usage: required
>> +             Value type: <string>
>> +             Definition: must be "cpu"
>> +     - clock-frequency: Contains the clock frequency for CPU, in Hz.
>> +
>> +* Examples
>> +
>> +/ {
>> +     cpu {
>> +             device_type = "cpu";
>> +             compatible = "andestech,n13", "andestech,nds32v3";
>> +     };
>
> This is missing the required clock-frequency property.
>
> There should be a /cpus node, with each CPU listed under that, to align
> with the devicetree spec. Even if you never support SMP, there's no
> reason to be different from other architectures.
>
> You should have something like:
>
> / {
>         cpus {
>                 cpu {
>                         device_type = "cpu";
>                         compatible = "andestech,n13";
>                         clock-frequency = <...>;
>                 };
>         };
> };
>
> Thanks,
> Mark.

Thanks.
I will modify it in the next version patch.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 30/35] net: faraday add nds32 support.
From: Greentime Hu @ 2017-11-28  2:55 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
	DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
	linux-serial-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CAK8P3a2GJERt78uWgdDy+Azr-ZMcOcB+D6Akq99tSfwKmt2LiA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

2017-11-27 22:15 GMT+08:00 Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>:
> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> From: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>>
>> Signed-off-by: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>
> This is missing a patch description.
>

Thanks.
Sorry. I miss it. I will add the commit messages in the next version patch.

>> diff --git a/drivers/net/ethernet/faraday/Kconfig b/drivers/net/ethernet/faraday/Kconfig
>> index 040c7f1..0ae6e9a 100644
>> --- a/drivers/net/ethernet/faraday/Kconfig
>> +++ b/drivers/net/ethernet/faraday/Kconfig
>> @@ -5,7 +5,7 @@
>>  config NET_VENDOR_FARADAY
>>         bool "Faraday devices"
>>         default y
>> -       depends on ARM
>> +       depends on ARM || NDS32
>>         ---help---
>>           If you have a network (Ethernet) card belonging to this class, say Y.
>>
>
> We probably want to make all three here
>
> depends on ARM || NDS32 || COMPILE_TEST
>

Thanks.
I will modify it in the next version patch.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 34/35] clocksource/drivers/Kconfig: Support andestech atcpit100 timer
From: Greentime Hu @ 2017-11-28  2:53 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
	DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
	linux-serial-u79uwXL29TY76Z2rM5mHXA, Rick Chen
In-Reply-To: <CAK8P3a2ovDuwWCq2HABZaCVGO04TX0VdgnQbK65RvHhsMEzsiw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

2017-11-27 22:11 GMT+08:00 Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>:
> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> From: Rick Chen <rickchen36-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> Add CLKSRC_ATCPIT100 for Andestech atcpit100 timer selection.
>> It often be used in Andestech AE3XX platform.
>>
>> Signed-off-by: Rick Chen <rickchen36-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Signed-off-by: Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> No need to split out the Makefile patch from the actual driver, they
> clearly belong together.
> The binding change should be a separate patch, as you did.
>
> It's probably best to separate the driver patches from the
> architecture submission
> in the future, they can simply get merged by the respective subsystem
> maintainers,
> with a smaller Cc list.
>

Hi, Arnd:

Thanks.
We will merge these 2 patches together in the next version patch.
We will sent to clocksource subsystem with a seperate patchset.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 18/35] nds32: Debugging support
From: Vincent Chen @ 2017-11-28  2:21 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Greentime Hu, Greentime, Linux Kernel Mailing List, linux-arch,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring,
	Networking, DTML, Al Viro, David Howells, Will Deacon,
	Daniel Lezcano, linux-serial-u79uwXL29TY76Z2rM5mHXA, Vincent Chen
In-Reply-To: <CAK8P3a2czU7=jECXFOvtRNhrq3zyX7gV7sa3OFPQ-8A4U8iH0A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

2017-11-27 22:34 GMT+08:00 Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>:
> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> +struct user_pt_regs {
>> +       long uregs[26];
>> +       long fp;
>> +       long gp;
>> +       long lp;
>> +       long sp;
>> +       long ipc;
>> +#if defined(CONFIG_HWZOL)
>> +       long lb;
>> +       long le;
>> +       long lc;
>> +#else
>> +       long dummy[3];
>> +#endif
>> +       long syscallno;
>> +};
>
> You cannot reference CONFIG_ symbols in a uapi header, since the
> configuration headers
> are not available.
>
Thanks
I will modify it in the next version patch.

>        Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 16/35] nds32: Signal handling support
From: Vincent Chen @ 2017-11-28  2:21 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Greentime Hu, Greentime, Linux Kernel Mailing List, linux-arch,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring,
	Networking, DTML, Al Viro, David Howells, Will Deacon,
	Daniel Lezcano, linux-serial-u79uwXL29TY76Z2rM5mHXA, Vincent Chen
In-Reply-To: <CAK8P3a1sqhLwz3WqM0Qx4w0SBWqFWMuXVgX4p9StpacfWdSnUA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

2017-11-27 22:37 GMT+08:00 Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>:
> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> +#ifndef _ASMNDS32_SIGNAL_H
>> +#define _ASMNDS32_SIGNAL_H
>> +
>> +#define SA_RESTORER    0x04000000
>> +
>> +#include <asm-generic/signal.h>
>> +#endif
>
> As documented in asm-generic/signal.h, new architectures should not define
> SA_RESTORER.
>

Thanks.
I will remove it in the next version patch.

>         Arnd

Vincent
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 14/35] nds32: System calls handling
From: Vincent Chen @ 2017-11-28  2:18 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Greentime Hu, Greentime, Linux Kernel Mailing List, linux-arch,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring,
	Networking, DTML, Al Viro, David Howells, Will Deacon,
	Daniel Lezcano, linux-serial, Vincent Chen
In-Reply-To: <CAK8P3a3V7=4LjT38xT2VBQW4sTgpKergar2KoO8brbMZjR--1w@mail.gmail.com>

2017-11-27 22:46 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
>
>> diff --git a/arch/nds32/include/asm/syscalls.h b/arch/nds32/include/asm/syscalls.h
>> new file mode 100644
>> index 0000000..741ccdc
>> --- /dev/null
>> +++ b/arch/nds32/include/asm/syscalls.h
>> @@ -0,0 +1,27 @@
>> +/*
>> + * Copyright (C) 2005-2017 Andes Technology Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef __ASM_NDS32_SYSCALLS_H
>> +#define __ASM_NDS32_SYSCALLS_H
>> +
>> +int sys_cacheflush(unsigned long addr, unsigned long len, unsigned int op);
>> +long sys_fadvise64_64_wrapper(int fd, int advice, loff_t offset, loff_t len);
>> +asmlinkage int sys_syscall(void);
>> +asmlinkage int sys_rt_sigreturn_wrapper(void);
>
> You mentioned that sys_syscall() should be removed in this version.
>

Sorry, I will remove it in next version patch.

> By convention, all syscall declarations should start with 'asmlinkage long',
> even though this has no effect in your architecture.
>

Thanks.
I will modify it  in next version patch.

>> diff --git a/arch/nds32/include/uapi/asm/unistd.h b/arch/nds32/include/uapi/asm/unistd.h
>> new file mode 100644
>> index 0000000..2bad1e7
>> --- /dev/null
>> +++ b/arch/nds32/include/uapi/asm/unistd.h
>
>> +
>> +#define __ARCH_WANT_RENAMEAT
>> +#define __ARCH_WANT_SYSCALL_OFF_T
>
> These two should not be here.
>

Thanks.
But, I don't know I should move these two macro to which file.
In asm-generic/unistd.h, these two are used to decide whether relative
syscall number is defined or not.
Therefore,  I put these two macros here in order that these two
definitions are available in user space.


>> +#define __ARCH_WANT_SYNC_FILE_RANGE2
>
> This one is fine though
>
>> +/* Use the standard ABI for syscalls */
>> +#include <asm-generic/unistd.h>
>> +
>> +__SYSCALL(__NR_fadvise64_64, sys_fadvise64_64_wrapper)
>> +__SYSCALL(__NR_rt_sigreturn, sys_rt_sigreturn_wrapper)
>
> I think you are overriding the array assignment here, this may cause a
> compiler warning when building with "make C=1 V=1" since you have
> the same index assigned to two pointers. A better way to do this
> is to override the name:
>
> #define sys_rt_sigreturn sys_rt_sigreturn_wrapper
> #define sys_fadvise64_64 sys_fadvise64_64_wrapper
> #include <asm-generic/unistd.h>
>
Thanks,
I will follow your suggestion to modify it in the next version patch.

>
>       Arnd


Vincent

^ permalink raw reply

* Re: [PATCH v2 22/35] nds32: Device tree support
From: Rob Herring @ 2017-11-27 19:14 UTC (permalink / raw)
  To: Greentime Hu
  Cc: greentime, linux-kernel@vger.kernel.org, Arnd Bergmann,
	linux-arch@vger.kernel.org, Thomas Gleixner, Jason Cooper,
	Marc Zyngier, netdev, deanbo422, devicetree@vger.kernel.org,
	Al Viro, David Howells, Will Deacon, Daniel Lezcano,
	linux-serial@vger.kernel.org, Vincent Chen
In-Reply-To: <CAL_Jsq+c4vt4-royBuTxAj+AY2wFHMugyyy41S5YP-QXyF2gbQ@mail.gmail.com>

On Mon, Nov 27, 2017 at 1:07 PM, Rob Herring <robh+dt@kernel.org> wrote:
> On Mon, Nov 27, 2017 at 6:28 AM, Greentime Hu <green.hu@gmail.com> wrote:
>> From: Greentime Hu <greentime@andestech.com>
>>
>> This patch adds support for device tree.
>>
>> Signed-off-by: Vincent Chen <vincentc@andestech.com>
>> Signed-off-by: Greentime Hu <greentime@andestech.com>
>> ---
>>  arch/nds32/boot/dts/Makefile   |    8 ++++++
>>  arch/nds32/boot/dts/ae3xx.dts  |   55 ++++++++++++++++++++++++++++++++++++
>>  arch/nds32/boot/dts/ag101p.dts |   60 ++++++++++++++++++++++++++++++++++++++++
>>  arch/nds32/kernel/devtree.c    |   45 ++++++++++++++++++++++++++++++
>>  4 files changed, 168 insertions(+)
>>  create mode 100644 arch/nds32/boot/dts/Makefile
>>  create mode 100644 arch/nds32/boot/dts/ae3xx.dts
>>  create mode 100644 arch/nds32/boot/dts/ag101p.dts
>>  create mode 100644 arch/nds32/kernel/devtree.c
>>
>> diff --git a/arch/nds32/boot/dts/Makefile b/arch/nds32/boot/dts/Makefile
>> new file mode 100644
>> index 0000000..d31faa8
>> --- /dev/null
>> +++ b/arch/nds32/boot/dts/Makefile
>> @@ -0,0 +1,8 @@
>> +ifneq '$(CONFIG_NDS32_BUILTIN_DTB)' '""'
>
> Built-in dtb's are really for legacy bootloader cases where the
> bootloader doesn't understand dtbs. Do you have that here?
>
> Plus, I don't see any code here to handle the built-in dtb.

I see it is in patch 2. That means patch 2 won't actually compile. You
should move the devtree.c parts at least and maybe the Makefile
infrastructure part too to patch 2. I don't think you need to split
things up so much, but I'll defer to Arnd on that.

Rob

^ permalink raw reply

* Re: [PATCH v2 22/35] nds32: Device tree support
From: Rob Herring @ 2017-11-27 19:07 UTC (permalink / raw)
  To: Greentime Hu
  Cc: greentime, linux-kernel@vger.kernel.org, Arnd Bergmann,
	linux-arch@vger.kernel.org, Thomas Gleixner, Jason Cooper,
	Marc Zyngier, netdev, deanbo422, devicetree@vger.kernel.org,
	Al Viro, David Howells, Will Deacon, Daniel Lezcano,
	linux-serial@vger.kernel.org, Vincent Chen
In-Reply-To: <1055af0e8b98571b4461d6f899d043fe7f17408b.1511785528.git.green.hu@gmail.com>

On Mon, Nov 27, 2017 at 6:28 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch adds support for device tree.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
> ---
>  arch/nds32/boot/dts/Makefile   |    8 ++++++
>  arch/nds32/boot/dts/ae3xx.dts  |   55 ++++++++++++++++++++++++++++++++++++
>  arch/nds32/boot/dts/ag101p.dts |   60 ++++++++++++++++++++++++++++++++++++++++
>  arch/nds32/kernel/devtree.c    |   45 ++++++++++++++++++++++++++++++
>  4 files changed, 168 insertions(+)
>  create mode 100644 arch/nds32/boot/dts/Makefile
>  create mode 100644 arch/nds32/boot/dts/ae3xx.dts
>  create mode 100644 arch/nds32/boot/dts/ag101p.dts
>  create mode 100644 arch/nds32/kernel/devtree.c
>
> diff --git a/arch/nds32/boot/dts/Makefile b/arch/nds32/boot/dts/Makefile
> new file mode 100644
> index 0000000..d31faa8
> --- /dev/null
> +++ b/arch/nds32/boot/dts/Makefile
> @@ -0,0 +1,8 @@
> +ifneq '$(CONFIG_NDS32_BUILTIN_DTB)' '""'

Built-in dtb's are really for legacy bootloader cases where the
bootloader doesn't understand dtbs. Do you have that here?

Plus, I don't see any code here to handle the built-in dtb.

> +BUILTIN_DTB := $(patsubst "%",%,$(CONFIG_NDS32_BUILTIN_DTB)).dtb.o
> +else
> +BUILTIN_DTB :=
> +endif
> +obj-$(CONFIG_OF) += $(BUILTIN_DTB)
> +
> +clean-files := *.dtb *.dtb.S
> diff --git a/arch/nds32/boot/dts/ae3xx.dts b/arch/nds32/boot/dts/ae3xx.dts
> new file mode 100644
> index 0000000..4181060
> --- /dev/null
> +++ b/arch/nds32/boot/dts/ae3xx.dts
> @@ -0,0 +1,55 @@
> +/dts-v1/;
> +/ {
> +       compatible = "nds32 ae3xx";

This compatible needs to be documented and is not valid. Needs to be
in the form "vendor,board-name" without spaces.

> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +       interrupt-parent = <&intc>;
> +
> +       chosen {
> +               bootargs = "earlycon console=ttyS0,38400n8 debug loglevel=7";
> +               stdout-path = &serial0;
> +       };
> +
> +       memory@0 {
> +               device_type = "memory";
> +               reg = <0x00000000 0x40000000>;
> +       };
> +
> +       cpu {
> +               device_type = "cpu";
> +               compatible = "andestech,n13", "andestech,nds32v3";
> +               clock-frequency = <60000000>;
> +       };
> +
> +       intc: interrupt-controller {
> +               compatible = "andestech,ativic32";
> +               #interrupt-cells = <1>;
> +               interrupt-controller;
> +       };
> +
> +       serial0: serial@f0300000 {
> +               compatible = "andestech,uart16550", "ns16550a";
> +               reg = <0xf0300000 0x1000>;
> +               interrupts = <8>;
> +               clock-frequency = <14745600>;
> +               reg-shift = <2>;
> +               reg-offset = <32>;
> +               no-loopback-test = <1>;
> +       };
> +
> +       timer0: timer@f0400000 {
> +               compatible = "andestech,atcpit100";
> +               reg = <0xf0400000 0x1000>;
> +               interrupts = <2>;
> +               clock-frequency = <30000000>;
> +               cycle-count-offset = <0x38>;
> +               cycle-count-down;
> +       };
> +
> +       mac0: mac@e0100000 {

ethernet@...

> +               compatible = "andestech,atmac100";
> +               reg = <0xe0100000 0x1000>;
> +               interrupts = <18>;
> +       };
> +
> +};
> diff --git a/arch/nds32/boot/dts/ag101p.dts b/arch/nds32/boot/dts/ag101p.dts
> new file mode 100644
> index 0000000..f1cb540
> --- /dev/null
> +++ b/arch/nds32/boot/dts/ag101p.dts
> @@ -0,0 +1,60 @@
> +/dts-v1/;
> +/ {
> +       compatible = "nds32 ag101p";

Same here.

> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +       interrupt-parent = <&intc>;
> +
> +       chosen {
> +               bootargs = "earlycon console=ttyS0,38400n8 debug loglevel=7";
> +               stdout-path = &serial0;
> +       };
> +
> +       memory@0 {
> +               device_type = "memory";
> +               reg = <0x00000000 0x40000000>;
> +       };
> +
> +       cpu@0 {
> +               device_type = "cpu";
> +               compatible = "andestech,n13";
> +               clock-frequency = <60000000>;
> +               next-level-cache = <&L2>;
> +       };
> +
> +       intc: interrupt-controller {
> +               compatible = "andestech,ativic32";
> +               #interrupt-cells = <2>;
> +               interrupt-controller;
> +       };
> +
> +       serial0: serial@99600000 {
> +               compatible = "andestech,uart16550", "ns16550a";
> +               reg = <0x99600000 0x1000>;
> +               interrupts = <7 4>;
> +               clock-frequency = <14745600>;
> +               reg-shift = <2>;
> +               no-loopback-test = <1>;
> +       };
> +
> +       timer0: timer@98400000 {
> +               compatible = "andestech,atftmr010";
> +               reg = <0x98400000 0x1000>;
> +               interrupts = <19 4>;
> +               clock-frequency = <15000000>;
> +               cycle-count-offset = <0x20>;
> +       };
> +
> +       mac0: mac@90900000 {
> +               compatible = "andestech,atmac100";
> +               reg = <0x90900000 0x1000>;
> +               interrupts = <25 4>;
> +       };
> +
> +       L2: l2-cache {
> +               compatible = "andestech,atl2c";
> +               reg = <0x90f00000 0x1000>;
> +               cache-unified;
> +               cache-level = <2>;
> +       };
> +};
> diff --git a/arch/nds32/kernel/devtree.c b/arch/nds32/kernel/devtree.c
> new file mode 100644
> index 0000000..2af0f1c
> --- /dev/null
> +++ b/arch/nds32/kernel/devtree.c
> @@ -0,0 +1,45 @@
> +/*
> + * Copyright (C) 2005-2017 Andes Technology Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/memblock.h>
> +#include <linux/of_fdt.h>
> +#include <linux/bootmem.h>
> +
> +void __init early_init_dt_add_memory_arch(u64 base, u64 size)
> +{
> +       size &= PAGE_MASK;
> +       memblock_add_node(base, size, 0);
> +}
> +
> +void *__init early_init_dt_alloc_memory_arch(u64 size, u64 align)
> +{
> +       return alloc_bootmem_align(size, align);
> +}

You should be able to use the default functions for these 2.

> +
> +void __init early_init_devtree(void *params)
> +{
> +       if (!params || !early_init_dt_scan(params)) {
> +               pr_crit("\n"
> +                       "Error: invalid device tree blob at (virtual address 0x%p)\n"
> +                       "The dtb must be 8-byte aligned and must not exceed 8 KB in size\n"

Why the size limit? That's pretty small for a DT.

> +                       "\nPlease check your bootloader.", params);
> +
> +               while (true)
> +                       cpu_relax();

Might as well use BUG_ON here if you're not going to continue. It's
generally better to WARN and continue on otherwise the messages aren't
visible until the console is up. However, if you have DT errors this
early, there's not much you can really do here.

> +       }
> +
> +       dump_stack_set_arch_desc("%s (DT)", of_flat_dt_get_machine_name());
> +}
> --
> 1.7.9.5
>

^ permalink raw reply

* Re: [PATCH v2 11/35] nds32: Device specific operations
From: Arnd Bergmann @ 2017-11-27 14:51 UTC (permalink / raw)
  To: Greentime Hu
  Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
	DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
	linux-serial, Vincent Chen
In-Reply-To: <ab110d0eaa525b07b1790d67bbb020046fb78792.1511785528.git.green.hu@gmail.com>

On Mon, Nov 27, 2017 at 1:27 PM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch introduces ioremap implementations.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
> ---
>  arch/nds32/include/asm/io.h |   25 +++++++++++++++
>  arch/nds32/mm/ioremap.c     |   75 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 100 insertions(+)
>  create mode 100644 arch/nds32/include/asm/io.h
>  create mode 100644 arch/nds32/mm/ioremap.c
>
> diff --git a/arch/nds32/include/asm/io.h b/arch/nds32/include/asm/io.h
> new file mode 100644
> index 0000000..b83dea1
> --- /dev/null
> +++ b/arch/nds32/include/asm/io.h
> @@ -0,0 +1,25 @@

> +#ifndef __ASM_NDS32_IO_H
> +#define __ASM_NDS32_IO_H
> +
> +#ifdef __KERNEL__
> +void iounmap(void __iomem * addr);
> +#include <asm-generic/io.h>
> +
> +#endif /* __KERNEL__ */
> +#endif /* __ASM_NDS32_IO_H */

Here, you should define a lot of the I/O accessor functions,
dereferencing a pointer
is generally not enough to guarantee an atomic MMIO operation. You need to
force the access to use the correct size to prevent the compiler from issuing
byte-sized operations when it thinks the pointer might be unaligned, and
there should be barriers that ensure a memory access is synchronized with
a DMA that might be triggered by a writel, or claimed to be completed after
a readl. Please see the risc-v header for this, it has many good explanations.

        Arnd

^ permalink raw reply

* Re: [PATCH v2 14/35] nds32: System calls handling
From: Arnd Bergmann @ 2017-11-27 14:46 UTC (permalink / raw)
  To: Greentime Hu
  Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
	DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
	linux-serial, Vincent Chen
In-Reply-To: <ee45d9c7b0388c87a93a26484e859001477eb3a2.1511785528.git.green.hu@gmail.com>

On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:

> diff --git a/arch/nds32/include/asm/syscalls.h b/arch/nds32/include/asm/syscalls.h
> new file mode 100644
> index 0000000..741ccdc
> --- /dev/null
> +++ b/arch/nds32/include/asm/syscalls.h
> @@ -0,0 +1,27 @@
> +/*
> + * Copyright (C) 2005-2017 Andes Technology Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __ASM_NDS32_SYSCALLS_H
> +#define __ASM_NDS32_SYSCALLS_H
> +
> +int sys_cacheflush(unsigned long addr, unsigned long len, unsigned int op);
> +long sys_fadvise64_64_wrapper(int fd, int advice, loff_t offset, loff_t len);
> +asmlinkage int sys_syscall(void);
> +asmlinkage int sys_rt_sigreturn_wrapper(void);

You mentioned that sys_syscall() should be removed in this version.

By convention, all syscall declarations should start with 'asmlinkage long',
even though this has no effect in your architecture.

> diff --git a/arch/nds32/include/uapi/asm/unistd.h b/arch/nds32/include/uapi/asm/unistd.h
> new file mode 100644
> index 0000000..2bad1e7
> --- /dev/null
> +++ b/arch/nds32/include/uapi/asm/unistd.h

> +
> +#define __ARCH_WANT_RENAMEAT
> +#define __ARCH_WANT_SYSCALL_OFF_T

These two should not be here.

> +#define __ARCH_WANT_SYNC_FILE_RANGE2

This one is fine though

> +/* Use the standard ABI for syscalls */
> +#include <asm-generic/unistd.h>
> +
> +__SYSCALL(__NR_fadvise64_64, sys_fadvise64_64_wrapper)
> +__SYSCALL(__NR_rt_sigreturn, sys_rt_sigreturn_wrapper)

I think you are overriding the array assignment here, this may cause a
compiler warning when building with "make C=1 V=1" since you have
the same index assigned to two pointers. A better way to do this
is to override the name:

#define sys_rt_sigreturn sys_rt_sigreturn_wrapper
#define sys_fadvise64_64 sys_fadvise64_64_wrapper
#include <asm-generic/unistd.h>


      Arnd

^ permalink raw reply

* Re: [PATCH v2 16/35] nds32: Signal handling support
From: Arnd Bergmann @ 2017-11-27 14:37 UTC (permalink / raw)
  To: Greentime Hu
  Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
	DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
	linux-serial-u79uwXL29TY76Z2rM5mHXA, Vincent Chen
In-Reply-To: <21cfd623872d4377ba5064cb7302bff49ebf917e.1511785528.git.green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> +#ifndef _ASMNDS32_SIGNAL_H
> +#define _ASMNDS32_SIGNAL_H
> +
> +#define SA_RESTORER    0x04000000
> +
> +#include <asm-generic/signal.h>
> +#endif

As documented in asm-generic/signal.h, new architectures should not define
SA_RESTORER.

        Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 18/35] nds32: Debugging support
From: Arnd Bergmann @ 2017-11-27 14:34 UTC (permalink / raw)
  To: Greentime Hu
  Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
	DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
	linux-serial, Vincent Chen
In-Reply-To: <01fc485408504e6a43b768664f44b9fd42a89503.1511785528.git.green.hu@gmail.com>

On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
> +struct user_pt_regs {
> +       long uregs[26];
> +       long fp;
> +       long gp;
> +       long lp;
> +       long sp;
> +       long ipc;
> +#if defined(CONFIG_HWZOL)
> +       long lb;
> +       long le;
> +       long lc;
> +#else
> +       long dummy[3];
> +#endif
> +       long syscallno;
> +};

You cannot reference CONFIG_ symbols in a uapi header, since the
configuration headers
are not available.

       Arnd

^ permalink raw reply

* Re: [PATCH v2 19/35] nds32: L2 cache support
From: Arnd Bergmann @ 2017-11-27 14:33 UTC (permalink / raw)
  To: Greentime Hu
  Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
	DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
	linux-serial, Vincent Chen
In-Reply-To: <380808fe9139e2c3800ae76dfa6aa5cbbbc4894f.1511785528.git.green.hu@gmail.com>

On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
> +
> +#define L2C_R_REG(offset)               __raw_readl(atl2c_base + offset)
> +#define L2C_W_REG(offset, value)        __raw_writel(value, atl2c_base + offset)

__raw_readl() is generally not endian-safe, and might  not have the barriers you
require here. Could you use readl/writel here, and only fall back to
readl_relaxed()/writel_relaxed() when you absolutely must avoid the barriers?

> diff --git a/arch/nds32/kernel/atl2c.c b/arch/nds32/kernel/atl2c.c
> new file mode 100644
> index 0000000..dd87fc9
> --- /dev/null
> +++ b/arch/nds32/kernel/atl2c.c
> +#include <linux/compiler.h>
> +#include <linux/of_address.h>
> +#include <linux/of_fdt.h>
> +#include <linux/of_platform.h>
> +#include <asm/l2_cache.h>

If this is the only file that includes asm/l2_cache.h, then I'd simply
move the entire
contents in here, rather than having a separate file in the global namespace.

          Arnd

^ permalink raw reply

* Re: [PATCH v2 22/35] nds32: Device tree support
From: Arnd Bergmann @ 2017-11-27 14:30 UTC (permalink / raw)
  To: Greentime Hu
  Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
	DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
	linux-serial, Vincent Chen
In-Reply-To: <1055af0e8b98571b4461d6f899d043fe7f17408b.1511785528.git.green.hu@gmail.com>

On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
> @@ -0,0 +1,55 @@
> +/dts-v1/;
> +/ {
> +       compatible = "nds32 ae3xx";

The compatible string doesn't seem to match the binding, it should always have
vendor prefix.

> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +       interrupt-parent = <&intc>;
> +
> +       chosen {
> +               bootargs = "earlycon console=ttyS0,38400n8 debug loglevel=7";
> +               stdout-path = &serial0;
> +       };

I would drop the bootargs here, this is something that should be set by the
bootloader and is up to the user.

      Arnd

^ permalink raw reply

* Re: [PATCH v2 24/35] nds32: defconfig
From: Arnd Bergmann @ 2017-11-27 14:27 UTC (permalink / raw)
  To: Greentime Hu
  Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
	DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
	linux-serial, Vincent Chen
In-Reply-To: <ba7df99550bdd5c06d01ef8f427d116280b8f52d.1511785528.git.green.hu@gmail.com>

On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:

> +CONFIG_EXPERT=y

You normally shouldn't need CONFIG_EXPERT in a defconfig file, by definition
this is needed for unusual configurations only.

> +CONFIG_NDS32_BUILTIN_DTB="ae3xx"

Having a built-in DTB also makes no sense for a defconfig, when the idea is that
it can run on as many machines as possible.

> +CONFIG_BLK_DEV_LOOP=y
> +CONFIG_BLK_DEV_RAM=y
> +CONFIG_BLK_DEV_RAM_SIZE=8192

Having ramdisk built-in is rather unusual, most users don't use ramdisks
any more since we don't need it for booting with the initramfs.

> +CONFIG_BRIDGE=y
> +CONFIG_TUN=y

These also look unusual. Would it make sense to have these as loadable
modules, or do you require having everything built-in for your workflow?

> +CONFIG_SOUND=y
> +CONFIG_SND=y
> +# CONFIG_SND_SUPPORT_OLD_API is not set
> +# CONFIG_SND_VERBOSE_PROCFS is not set

It seems the sound subsystem is enabled, but no drivers are selected,
so it won't actually do anything.

> +# CONFIG_USB_SUPPORT is not set
> +CONFIG_MMC=y

Same for MMC. If you are still trying to get the respective device drivers
merged, that doesn't need to stop you from enabling the configurations
here.

       Arnd

^ permalink raw reply

* Re: [PATCH v2 25/35] nds32: Build infrastructure
From: Arnd Bergmann @ 2017-11-27 14:21 UTC (permalink / raw)
  To: Greentime Hu
  Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
	DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
	linux-serial, Vincent Chen
In-Reply-To: <5e1be9ebc591c6de79b75f726a5a38b2564eaa92.1511785528.git.green.hu@gmail.com>

On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:

> diff --git a/arch/nds32/Kconfig.cpu b/arch/nds32/Kconfig.cpu
> new file mode 100644
> index 0000000..6b4013f
> --- /dev/null
> +++ b/arch/nds32/Kconfig.cpu
> @@ -0,0 +1,131 @@
> +comment "Processor Features"
> +
> +config CPU_BIG_ENDIAN
> +       bool "Big endian"
> +       def_bool n
> +
> +config CPU_LITTLE_ENDIAN
> +       bool "Little endian"
> +       def_bool y

These must be mutually exclusive, you surely get some build error if you try to
set both here.

You can either make it a 'choice' statement to pick between the two, or you
can make CPU_LITTLE_ENDIAN a silent option like

config CPU_BIG_ENDIAN
       bool "Big endian"

config CPU_LITTLE_ENDIAN
       def_bool !CPU_BIG_ENDIAN

> +config CPU_CACHE_NONALIASING
> +       bool "Non-aliasing cache"
> +       help
> +         If this CPU is using VIPT data cache and its cache way size is larger
> +         than page size, say N. If it is using PIPT data cache, say Y.
> +
> +         If unsure, say Y.

Can you determine this from the CPU type?

> +choice
> +       prompt "Memory split"
> +       depends on MMU
> +       default VMSPLIT_3G
> +       help
> +         Select the desired split between kernel and user memory.
> +
> +         If you are not absolutely sure what you are doing, leave this
> +         option alone!
> +
> +       config VMSPLIT_3G
> +               bool "3G/1G user/kernel split"
> +       config VMSPLIT_3G_OPT
> +               bool "3G/1G user/kernel split (for full 1G low memory)"
> +       config VMSPLIT_2G
> +               bool "2G/2G user/kernel split"
> +       config VMSPLIT_1G
> +               bool "1G/3G user/kernel split"
> +endchoice

I think you mentioned that the 1GB configuration is quite common, how
about making VMSPLIT_3G_OPT the default here?

       Arnd

^ permalink raw reply

* Re: [PATCH v2 30/35] net: faraday add nds32 support.
From: Arnd Bergmann @ 2017-11-27 14:15 UTC (permalink / raw)
  To: Greentime Hu
  Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
	DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
	linux-serial
In-Reply-To: <1aa759dbbb1032dd20e5fadbb0ce5aa4b8b22690.1511785528.git.green.hu@gmail.com>

On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> Signed-off-by: Greentime Hu <greentime@andestech.com>

This is missing a patch description.

> diff --git a/drivers/net/ethernet/faraday/Kconfig b/drivers/net/ethernet/faraday/Kconfig
> index 040c7f1..0ae6e9a 100644
> --- a/drivers/net/ethernet/faraday/Kconfig
> +++ b/drivers/net/ethernet/faraday/Kconfig
> @@ -5,7 +5,7 @@
>  config NET_VENDOR_FARADAY
>         bool "Faraday devices"
>         default y
> -       depends on ARM
> +       depends on ARM || NDS32
>         ---help---
>           If you have a network (Ethernet) card belonging to this class, say Y.
>

We probably want to make all three here

depends on ARM || NDS32 || COMPILE_TEST

     Arnd

^ permalink raw reply

* Re: [PATCH v2 32/35] asm-generic/io.h: move ioremap_nocache/ioremap_uc/ioremap_wc/ioremap_wt out of ifndef CONFIG_MMU
From: Arnd Bergmann @ 2017-11-27 14:14 UTC (permalink / raw)
  To: Greentime Hu
  Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
	DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
	linux-serial, Vincent Chen
In-Reply-To: <44e31de43dab9abecdc08f9a87244205f054cfa6.1511785528.git.green.hu@gmail.com>

On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> It allows some architectures to use this generic macro instead of
> defining theirs.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
> ---
>  include/asm-generic/io.h |   18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

This looks good, but you probably want to do this earlier in the series, so you
can build on top of this change.

Please keep this patch as part of your series,

Acked-by: Arnd Bergmann <arnd@arndb.de>

^ permalink raw reply

* Re: [PATCH v2 34/35] clocksource/drivers/Kconfig: Support andestech atcpit100 timer
From: Arnd Bergmann @ 2017-11-27 14:11 UTC (permalink / raw)
  To: Greentime Hu
  Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
	DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
	linux-serial, Rick Chen
In-Reply-To: <1a22db002413ff60851737736a86b40c38877220.1511785528.git.green.hu@gmail.com>

On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Rick Chen <rickchen36@gmail.com>
>
> Add CLKSRC_ATCPIT100 for Andestech atcpit100 timer selection.
> It often be used in Andestech AE3XX platform.
>
> Signed-off-by: Rick Chen <rickchen36@gmail.com>
> Signed-off-by: Greentime Hu <green.hu@gmail.com>

No need to split out the Makefile patch from the actual driver, they
clearly belong together.
The binding change should be a separate patch, as you did.

It's probably best to separate the driver patches from the
architecture submission
in the future, they can simply get merged by the respective subsystem
maintainers,
with a smaller Cc list.

      Arnd

^ permalink raw reply

* Re: [PATCH v2 10/35] nds32: Atomic operations
From: Mark Rutland @ 2017-11-27 13:57 UTC (permalink / raw)
  To: Greentime Hu
  Cc: greentime, linux-kernel, arnd, linux-arch, tglx, jason,
	marc.zyngier, robh+dt, netdev, deanbo422, devicetree, viro,
	dhowells, will.deacon, daniel.lezcano, linux-serial, Vincent Chen
In-Reply-To: <56a24fae3cec68f80c41eebd98fcceb9b137ac64.1511785528.git.green.hu@gmail.com>

Hi,

On Mon, Nov 27, 2017 at 08:27:57PM +0800, Greentime Hu wrote:
> +static inline void arch_spin_unlock(arch_spinlock_t * lock)
> +{
> +	asm volatile(
> +		"xor	$r15,  $r15, $r15\n"
> +		"swi	$r15, [%0]\n"
> +		:
> +		:"r"(&lock->lock)
> +		:"memory");
> +}

This looks suspicious. There's no clobber for $r15, so isn't this
corrupting a GPR behind the back of the compiler?

Shouldn't there be a tmp variable for the register allocation rather
than hard-coding $r15?

> +static inline void arch_write_unlock(arch_rwlock_t * rw)
> +{
> +	asm volatile(
> +		"xor	$r15, $r15, $r15\n"
> +		"swi	$r15, [%0]\n"
> +		:
> +		:"r"(&rw->lock)
> +		:"memory","$r15");
> +}

This time you have a clobber, but it's still not clear to me why you
don't use a tmp variable and leave the register allocation to the
compiler.

Thanks,
Mark.

^ permalink raw reply

* Re: [PATCH v2 06/35] nds32: MMU fault handling and page table management
From: Mark Rutland @ 2017-11-27 13:51 UTC (permalink / raw)
  To: Greentime Hu
  Cc: greentime-MUIXKm3Oiri1Z/+hSey0Gg,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, arnd-r2nGTMty4D4,
	linux-arch-u79uwXL29TY76Z2rM5mHXA, tglx-hfZtesqFncYOwBW4kG4KsQ,
	jason-NLaQJdtUoK4Be96aLqz0jA, marc.zyngier-5wv7dgnIgG8,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, netdev-u79uwXL29TY76Z2rM5mHXA,
	deanbo422-Re5JQEeQqe8AvxtiuMwx3w,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn,
	dhowells-H+wXaHxf7aLQT0dZR+AlfA, will.deacon-5wv7dgnIgG8,
	daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A,
	linux-serial-u79uwXL29TY76Z2rM5mHXA, Vincent Chen
In-Reply-To: <ba92adae5d20d99c7c18e75146642a2ccbd5d047.1511785528.git.green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Hi,

On Mon, Nov 27, 2017 at 08:27:53PM +0800, Greentime Hu wrote:
> +void do_page_fault(unsigned long entry, unsigned long addr,
> +		   unsigned int error_code, struct pt_regs *regs)
> +{

> +	/*
> +	 * As per x86, we may deadlock here. However, since the kernel only
> +	 * validly references user space from well defined areas of the code,
> +	 * we can bug out early if this is from code which shouldn't.
> +	 */
> +	if (unlikely(!down_read_trylock(&mm->mmap_sem))) {
> +		if (!user_mode(regs) &&
> +		    !search_exception_tables(instruction_pointer(regs)))
> +			goto no_context;
> +retry:
> +		down_read(&mm->mmap_sem);
> +	} else {
> +		/*
> +		 * The above down_read_trylock() might have succeeded in which
> +		 * case, we'll have missed the might_sleep() from down_read().
> +		 */
> +		might_sleep();
> +		if (IS_ENABLED(CONFIG_DEBUG_VM)) {
> +			if (!user_mode(regs) &&
> +			    !search_exception_tables(instruction_pointer(regs)))
> +				goto no_context;
> +		}
> +	}

> +	fault = handle_mm_fault(vma, addr, flags);
> +
> +	/*
> +	 * If we need to retry but a fatal signal is pending, handle the
> +	 * signal first. We do not need to release the mmap_sem because it
> +	 * would already be released in __lock_page_or_retry in mm/filemap.c.
> +	 */
> +	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
> +		return;

I believe you can get stuck in a livelock here (with an unkillable
task), if a uaccess primitive tries to access a region protected by a
userfaultfd. Please see:

  https://lkml.kernel.org/r/1499782590-31366-1-git-send-email-mark.rutland-5wv7dgnIgG8@public.gmane.org

... for details and a test case.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 29/35] dt-bindings: nds32 CPU Bindings
From: Mark Rutland @ 2017-11-27 13:42 UTC (permalink / raw)
  To: Greentime Hu
  Cc: greentime, linux-kernel, arnd, linux-arch, tglx, jason,
	marc.zyngier, robh+dt, netdev, deanbo422, devicetree, viro,
	dhowells, will.deacon, daniel.lezcano, linux-serial, Vincent Chen,
	Rick Chen, Zong Li
In-Reply-To: <20a04f4a5b08dcec221e0c511e42ff3df711ae66.1511785528.git.green.hu@gmail.com>

Him

On Mon, Nov 27, 2017 at 08:28:16PM +0800, Greentime Hu wrote:
> From: Greentime Hu <greentime@andestech.com>
> 
> This patch adds nds32 CPU binding documents.
> 
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Rick Chen <rick@andestech.com>
> Signed-off-by: Zong Li <zong@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
> ---
>  Documentation/devicetree/bindings/nds32/cpus.txt |   32 ++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nds32/cpus.txt
> 
> diff --git a/Documentation/devicetree/bindings/nds32/cpus.txt b/Documentation/devicetree/bindings/nds32/cpus.txt
> new file mode 100644
> index 0000000..c302c89
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nds32/cpus.txt
> @@ -0,0 +1,32 @@
> +* Andestech Processor Binding
> +
> +This binding specifies what properties must be available in the device tree
> +representation of a Andestech Processor Core, which is the root node in the
> +tree.
> +
> +Required properties:
> +
> +	- compatible:
> +		Usage: required
> +		Value type: <string>
> +		Definition: should be one of:
> +			"andestech,n13"
> +			"andestech,n15"
> +			"andestech,d15"
> +			"andestech,n10"
> +			"andestech,d10"
> +			"andestech,nds32v3"

Are these specific parts, or architecture versions?

I guess "andestech,nds32v3" is an architecture version, and the rest are
specific parts?

> +	- device_type
> +		Usage: required
> +		Value type: <string>
> +		Definition: must be "cpu"
> +	- clock-frequency: Contains the clock frequency for CPU, in Hz.
> +
> +* Examples
> +
> +/ {
> +	cpu {
> +		device_type = "cpu";
> +		compatible = "andestech,n13", "andestech,nds32v3";
> +	};

This is missing the required clock-frequency property.

There should be a /cpus node, with each CPU listed under that, to align
with the devicetree spec. Even if you never support SMP, there's no
reason to be different from other architectures.

You should have something like:

/ {
	cpus {
		cpu {
			device_type = "cpu";
			compatible = "andestech,n13";
			clock-frequency = <...>;
		};
	};
};

Thanks,
Mark.

^ permalink raw reply

* [PATCH v2 35/35] dt-bindings: timer: Add andestech atcpit100 timer binding doc
From: Greentime Hu @ 2017-11-27 12:28 UTC (permalink / raw)
  To: greentime, linux-kernel, arnd, linux-arch, tglx, jason,
	marc.zyngier, robh+dt, netdev, deanbo422, devicetree, viro,
	dhowells, will.deacon, daniel.lezcano, linux-serial
  Cc: Rick Chen, green.hu
In-Reply-To: <cover.1511785528.git.green.hu@gmail.com>

From: Rick Chen <rickchen36@gmail.com>

Add a document to describe Andestech atcpit100 timer and
binding information.

Signed-off-by: Rick Chen <rickchen36@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Greentime Hu <green.hu@gmail.com>
---
 .../bindings/timer/andestech,atcpit100-timer.txt   |   33 ++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/andestech,atcpit100-timer.txt

diff --git a/Documentation/devicetree/bindings/timer/andestech,atcpit100-timer.txt b/Documentation/devicetree/bindings/timer/andestech,atcpit100-timer.txt
new file mode 100644
index 0000000..b97ff32
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/andestech,atcpit100-timer.txt
@@ -0,0 +1,33 @@
+Andestech ATCPIT100 timer
+------------------------------------------------------------------
+ATCPIT100 is a generic IP block from Andes Technology, embedded in
+Andestech AE3XX platforms and other designs.
+
+This timer is a set of compact multi-function timers, which can be
+used as pulse width modulators (PWM) as well as simple timers.
+
+It supports up to 4 PIT channels. Each PIT channel is a
+multi-function timer and provide the following usage scenarios:
+One 32-bit timer
+Two 16-bit timers
+Four 8-bit timers
+One 16-bit PWM
+One 16-bit timer and one 8-bit PWM
+Two 8-bit timer and one 8-bit PWM
+
+Required properties:
+- compatible	: Should be "andestech,atcpit100"
+- reg		: Address and length of the register set
+- interrupts	: Reference to the timer interrupt
+- clocks : a clock to provide the tick rate for "andestech,atcpit100"
+- clock-names : should be "PCLK" for the external tick timer.
+
+Examples:
+
+timer0: timer@f0400000 {
+	compatible = "andestech,atcpit100";
+	reg = <0xf0400000 0x1000>;
+	interrupts = <2 4>;
+	clocks = <&clk_pll>;
+	clock-names = "PCLK";
+};
-- 
1.7.9.5

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox