From: Yoshinori Sato <ysato@users.sourceforge.jp>
To: Mark Rutland <mark.rutland@arm.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Subject: Re: [PATCH v11 19/19] h8300: devicetree source
Date: Sat, 09 May 2015 20:19:28 +0900 [thread overview]
Message-ID: <87r3qqov5r.wl-ysato@users.sourceforge.jp> (raw)
In-Reply-To: <20150508165000.GB11680@leverpostej>
At Fri, 8 May 2015 17:50:00 +0100,
Mark Rutland wrote:
>
> Hi,
>
> > +++ b/Documentation/devicetree/bindings/h8300/cpu.txt
> > @@ -0,0 +1,17 @@
> > +* H8/300 CPU bindings
> > +
> > +Required properties:
> > +
> > +- compatible: Compatible property value should be "renesas,h8300".
> > +- reg: Contains CPU index.
>
> What does the "CPU index" correspond to physically on the CPU?
>
> Can h8300 support SMP?
No.
It's unnecessary.
> > +- clock-frequency: Contains the clock frequency for CPU, in Hz.
>
> Is this strictly necessary?
Yes.
There are no ways to calculate.
> > +- renesas,bus-width: Contain the memory bus width.
>
> What's this needed for?
Hmm...
This value can get bus controller setting.
It's considered.
>
> [...]
>
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,h8300h-intc.txt
> > @@ -0,0 +1,20 @@
> > +* H8/300H Interrupt controller
> > +
> > +Required properties:
> > +
> > +- compatible: has to be "renesas,h8300h-intc", "renesas,h8300-intc" as fallback.
> > +- #interrupt-cells: has to be <1>: an interrupt index and flags, as defined in
> > + interrupts.txt in this directory
>
> Surely you need two cells to encode index and flags?
That's right.
flag field needed.
>
> > +
> > +Optional properties:
> > +
> > +- any properties, listed in interrupts.txt, and any standard resource allocation
> > + properties
> > +
> > +Example:
> > +
> > + h8intc: intc@0 {
>
> Nit: call this "interrupt-controller" rather than "ntc".
OK.
Updated.
> Without a reg you shouldn't have a unit-address (the '@0' part).
>
> How does the CPU communicate with this controller? I assume it's not
> MMIO.
Interrupt controller is MMIO.
It set base address.
> These comments also apply to "renesas,h8s-intc".
OK.
> > diff --git a/arch/h8300/boot/dts/edosk2674.dts b/arch/h8300/boot/dts/edosk2674.dts
> > new file mode 100644
> > index 0000000..60e73b9
> > --- /dev/null
> > +++ b/arch/h8300/boot/dts/edosk2674.dts
> > @@ -0,0 +1,109 @@
> > +#include <dt-bindings/clock/renesas,8bit-timer.h>
> > +
> > +/dts-v1/;
> > +/ {
> > + compatible = "renesas,edosk2674";
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + interrupt-parent = <&h8intc>;
> > +
> > + chosen {
> > + bootargs = "console=ttySC2,38400";
> > + };
>
> It would be great if you could use stdout-path from the start rather
> than passing console= in bootargs. That makes things far less fragile
> w.r.t. physical vs logical naming, and keeps console and earlycon in
> sync.
>
> It also means that a user can replace bootargs and still expect the
> console to work by default (unless overridden explicitly).
OK.
> > + aliases {
> > + serial0 = &sci0;
> > + serial1 = &sci1;
> > + serial2 = &sci2;
> > + };
> > +
> > + clocks {
> > + ranges;
> > + #address-cells = <1>;
> > + #size-cells = <1>;
>
> Please get rid of the clocks container node and place the clocks
> directly under the root node. There's nothing magic about a /clocks
> node, and it's not been listed as a bus of any sort.
OK.
> > + pllclk: pllclk {
> > + compatible = "renesas,h8s2678-pll-clock";
> > + clocks = <&xclk>;
> > + #clock-cells = <0>;
> > + reg = <0xfee03b 2>, <0xfee045 2>;
> > + };
> > + cclk: cclk {
> > + compatible = "renesas,h8300-div-clock";
> > + clocks = <&pllclk>;
> > + #clock-cells = <0>;
> > + reg = <0xfee03b 2>;
> > + renesas,width = <3>;
> > + };
>
> Are there existing bindings for these? I didn't see any as part of the
> portion of the series I was Cc'd for.
No.
It new clock bindings.
Please refer Message-Id: <1431097479-21101-18-git-send-email-ysato@users.sourceforge.jp>
> > + memory@0 {
>
> Nit: the unit-address should math the address in the reg entry (here it
> should be 400000 rather than 0).
OK.
> [...]
>
> > + chosen {
> > + bootargs = "earlyprintk=h8300-sim console=ttySC0";
> > + };
>
> If you implement earlycon you'd only need a single stdout-path entry
> here, which would make this much nicer.
OK.
> Thanks,
> Mark.
Thanks comment.
It was helpful.
--
Yoshinori Sato
<ysato@users.sourceforge.jp>
next prev parent reply other threads:[~2015-05-09 11:19 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-08 15:04 [PATCH v11 00/19] Re-introduce h8300 architecture Yoshinori Sato
2015-05-08 15:04 ` [PATCH v11 01/19] MAINTAINERS: Add H8/300 entry Yoshinori Sato
2015-05-08 15:04 ` [PATCH v11 02/19] mksysmap: Add h8300 local symbol pattern Yoshinori Sato
2015-05-08 15:04 ` [PATCH v11 03/19] Add ELF machine Yoshinori Sato
2015-05-08 15:04 ` [PATCH v11 04/19] sh-sci: Add h8300 SCI Yoshinori Sato
2015-05-08 15:04 ` [PATCH v11 05/19] asm-generic: Add common asm-offsets.h Yoshinori Sato
2015-05-08 15:04 ` [PATCH v11 06/19] h8300: Assembly headers Yoshinori Sato
2015-05-10 10:46 ` Richard Weinberger
2015-05-11 3:30 ` Yoshinori Sato
2015-05-08 15:04 ` [PATCH v11 07/19] h8300: UAPI headers Yoshinori Sato
2015-05-08 15:04 ` [PATCH v11 08/19] h8300: Exception and Interrupt handling Yoshinori Sato
2015-05-08 15:04 ` [PATCH v11 09/19] h8300: kernel booting Yoshinori Sato
2015-05-08 15:04 ` [PATCH v11 10/19] h8300: Process and signal Yoshinori Sato
2015-05-08 15:04 ` [PATCH v11 11/19] h8300: CPU depend helpers Yoshinori Sato
2015-05-08 15:04 ` [PATCH v11 12/19] h8300: miscellaneous functions Yoshinori Sato
2015-05-08 15:04 ` [PATCH v11 13/19] h8300: Memory management Yoshinori Sato
2015-05-08 15:04 ` [PATCH v11 14/19] h8300: library functions Yoshinori Sato
2015-05-08 15:04 ` [PATCH v11 15/19] h8300: Build scripts Yoshinori Sato
2015-05-08 15:04 ` [PATCH v11 16/19] h8300: configs Yoshinori Sato
2015-05-08 15:04 ` [PATCH v11 17/19] h8300: clock driver Yoshinori Sato
2015-05-08 15:04 ` [PATCH v11 18/19] h8300: clocksource Yoshinori Sato
2015-05-08 15:04 ` [PATCH v11 19/19] h8300: devicetree source Yoshinori Sato
2015-05-08 16:50 ` Mark Rutland
2015-05-09 11:19 ` Yoshinori Sato [this message]
2015-05-08 18:54 ` [PATCH v11 00/19] Re-introduce h8300 architecture Guenter Roeck
2015-05-09 11:20 ` Yoshinori Sato
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87r3qqov5r.wl-ysato@users.sourceforge.jp \
--to=ysato@users.sourceforge.jp \
--cc=devicetree@vger.kernel.org \
--cc=linux-arch@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).