| Message ID | 1430827599-11560-6-git-send-email-bintian.wang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, > +/*Reserved 1MB memory for MCU*/ > +/memreserve/ 0x05e00000 0x00100000; What exactly is the MCU used for, and what does it use this memory for? Can this be carved out from the memory node instead? If the OS doesn't need to access this memory to communicate with the MCU, preventing the OS from mapping the memory avoids a number of potential issues. I take it that with UEFI this region is not described to the OS? [...] > + psci { > + compatible = "arm,psci-0.2"; > + method = "smc"; > + }; Are all the PSCI 0.2 mandatory features implemented? Can CPU0 be hot unplugged? [...] > + uart0: uart@f8015000 { /* console */ > + compatible = "arm,pl011", "arm,primecell"; > + reg = <0x0 0xf8015000 0x0 0x1000>; > + interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl HI6220_UART0_PCLK>; > + clock-names = "uartclk", "apb_pclk"; > + }; In a previous discussion [1] the UART on HI6220 was described as not fully PL011 compliant, with a number of differences (e.g. the FIFO length). Given that, I feel somewhat uncomfortable with the current compatible string list. What exactly are those differences? We may need a more specific compatible string (even if in addition to those existing ones), or perhaps other properties. Thanks, Mark. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/328978.html
Hello Mark, On 2015/5/6 1:13, Mark Rutland wrote: > Hi, > >> +/*Reserved 1MB memory for MCU*/ >> +/memreserve/ 0x05e00000 0x00100000; > > What exactly is the MCU used for, and what does it use this memory for? > > Can this be carved out from the memory node instead? If the OS doesn't > need to access this memory to communicate with the MCU, preventing the > OS from mapping the memory avoids a number of potential issues. > > I take it that with UEFI this region is not described to the OS? MCU is used for system low power control, the reserved memory is hard coded by hardware and used by MCU, OS will access this memory to communicate with the MCU to change the CPU frequency. > > [...] > >> + psci { >> + compatible = "arm,psci-0.2"; >> + method = "smc"; >> + }; > > Are all the PSCI 0.2 mandatory features implemented? The system off/suspend is under development, and system off will be released in next months, and system suspend may be released in the following two months. Leo does the development of PSCI, and he can give more detailed plan. So can I keep "arm,psci-0.2" here? > Can CPU0 be hot unplugged? Yes, CPU0~CPU7 all can be hot unplugged. > [...] > >> + uart0: uart@f8015000 { /* console */ >> + compatible = "arm,pl011", "arm,primecell"; >> + reg = <0x0 0xf8015000 0x0 0x1000>; >> + interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>; >> + clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl HI6220_UART0_PCLK>; >> + clock-names = "uartclk", "apb_pclk"; >> + }; > > In a previous discussion [1] the UART on HI6220 was described as not > fully PL011 compliant, with a number of differences (e.g. the FIFO > length). > > Given that, I feel somewhat uncomfortable with the current compatible > string list. What exactly are those differences? We may need a more > specific compatible string (even if in addition to those existing ones), > or perhaps other properties. The small system can be booted and the console also works well without changing any code of driver amba-pl011.c, so I think the compatible string is OK for this patch set. Hisilicon do some performance enhancements based on PL011, but the current driver "amba-pl011.c" also works on hi6220 without those enhancements driver code. Thanks, Bintian > > Thanks, > Mark. > > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/328978.html > > . >
On Wed, May 06, 2015 at 11:16:12AM +0800, Bintian Wang wrote: > On 2015/5/6 1:13, Mark Rutland wrote: > >[...] > > > >>+ psci { > >>+ compatible = "arm,psci-0.2"; > >>+ method = "smc"; > >>+ }; > > > >Are all the PSCI 0.2 mandatory features implemented? > > The system off/suspend is under development, and system off will > be released in next months, and system suspend may be released in the > following two months. Yes; now for mandatory features, except SYSTEM_OFF others have supported. for PSCI 0.2, SYSTEM_SUSPEND even is not supported, right? > So can I keep "arm,psci-0.2" here? > > >Can CPU0 be hot unplugged? > Yes, CPU0~CPU7 all can be hot unplugged. Just clarify: now use s/w method to emulate CPU and cluster's low power state; all cores can be hotplugged [1]. [1] https://github.com/96boards/arm-trusted-firmware; Thanks, Leo Yan
Hello Mark, On 2015/5/6 11:16, Bintian wrote: > Hello Mark, > > On 2015/5/6 1:13, Mark Rutland wrote: [...] >>> + uart0: uart@f8015000 { /* console */ >>> + compatible = "arm,pl011", "arm,primecell"; >>> + reg = <0x0 0xf8015000 0x0 0x1000>; >>> + interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>; >>> + clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl >>> HI6220_UART0_PCLK>; >>> + clock-names = "uartclk", "apb_pclk"; >>> + }; >> >> In a previous discussion [1] the UART on HI6220 was described as not >> fully PL011 compliant, with a number of differences (e.g. the FIFO >> length). >> >> Given that, I feel somewhat uncomfortable with the current compatible >> string list. What exactly are those differences? We may need a more >> specific compatible string (even if in addition to those existing ones), >> or perhaps other properties. > The small system can be booted and the console also works well without > changing any code of driver amba-pl011.c, so I think the compatible > string is OK for this patch set. > > Hisilicon do some performance enhancements based on PL011, but the > current driver "amba-pl011.c" also works on hi6220 without those > enhancements driver code. Checked with Hisilicon chip designer, the UART0 is used for DEBUG console and compliant with PL011 fully. Thanks, Bintian > >> >> Thanks, >> Mark. >> >> [1] >> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/328978.html >> >> >> . >> > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > . >
On Wed, May 06, 2015 at 04:51:20AM +0100, Leo Yan wrote: > On Wed, May 06, 2015 at 11:16:12AM +0800, Bintian Wang wrote: > > On 2015/5/6 1:13, Mark Rutland wrote: > > >[...] > > > > > >>+ psci { > > >>+ compatible = "arm,psci-0.2"; > > >>+ method = "smc"; > > >>+ }; > > > > > >Are all the PSCI 0.2 mandatory features implemented? > > > > The system off/suspend is under development, and system off will > > be released in next months, and system suspend may be released in the > > following two months. > > Yes; now for mandatory features, except SYSTEM_OFF others have supported. Ok. Do you mean in the next month, or in a number of months? > for PSCI 0.2, SYSTEM_SUSPEND even is not supported, right? SYSTEM_SUSPEND was introduced in PSCI 1.0, and is optional. So long as PSCI_VERSION reports 0.2 or PSCI_FEATURES reports NOT_SUPPORTED for SYSTEM_SUSPEND, it's ok to not implement. I guess PSCI_VERSION reports PSCI 0.2? > > So can I keep "arm,psci-0.2" here? > > > > >Can CPU0 be hot unplugged? > > Yes, CPU0~CPU7 all can be hot unplugged. > > Just clarify: now use s/w method to emulate CPU and cluster's low > power state; all cores can be hotplugged [1]. So long as CPUs brought online are reset appropriately, and "offline" CPUs can't bring down the system somehow, that should be fine. For now the important part is the interface. Thanks, Mark.
On Wed, May 06, 2015 at 07:50:52AM +0100, Bintian wrote: > Hello Mark, > > On 2015/5/6 11:16, Bintian wrote: > > Hello Mark, > > > > On 2015/5/6 1:13, Mark Rutland wrote: > [...] > >>> + uart0: uart@f8015000 { /* console */ > >>> + compatible = "arm,pl011", "arm,primecell"; > >>> + reg = <0x0 0xf8015000 0x0 0x1000>; > >>> + interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>; > >>> + clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl > >>> HI6220_UART0_PCLK>; > >>> + clock-names = "uartclk", "apb_pclk"; > >>> + }; > >> > >> In a previous discussion [1] the UART on HI6220 was described as not > >> fully PL011 compliant, with a number of differences (e.g. the FIFO > >> length). > >> > >> Given that, I feel somewhat uncomfortable with the current compatible > >> string list. What exactly are those differences? We may need a more > >> specific compatible string (even if in addition to those existing ones), > >> or perhaps other properties. > > The small system can be booted and the console also works well without > > changing any code of driver amba-pl011.c, so I think the compatible > > string is OK for this patch set. > > > > Hisilicon do some performance enhancements based on PL011, but the > > current driver "amba-pl011.c" also works on hi6220 without those > > enhancements driver code. > Checked with Hisilicon chip designer, the UART0 is used for DEBUG > console and compliant with PL011 fully. I think that given that we know the UART is not quite a PL011 we should add an additional compatible string just in case some difference crops up later that is problematic. So we'd have something like: compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell"; That way we can add any optimisations or workarounds later as required. Thanks, Mark.
Hello Mark, On 2015/5/6 17:30, Mark Rutland wrote: > On Wed, May 06, 2015 at 07:50:52AM +0100, Bintian wrote: >> Hello Mark, >> >> On 2015/5/6 11:16, Bintian wrote: >>> Hello Mark, >>> >>> On 2015/5/6 1:13, Mark Rutland wrote: >> [...] >>>>> + uart0: uart@f8015000 { /* console */ >>>>> + compatible = "arm,pl011", "arm,primecell"; >>>>> + reg = <0x0 0xf8015000 0x0 0x1000>; >>>>> + interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>; >>>>> + clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl >>>>> HI6220_UART0_PCLK>; >>>>> + clock-names = "uartclk", "apb_pclk"; >>>>> + }; >>>> >>>> In a previous discussion [1] the UART on HI6220 was described as not >>>> fully PL011 compliant, with a number of differences (e.g. the FIFO >>>> length). >>>> >>>> Given that, I feel somewhat uncomfortable with the current compatible >>>> string list. What exactly are those differences? We may need a more >>>> specific compatible string (even if in addition to those existing ones), >>>> or perhaps other properties. >>> The small system can be booted and the console also works well without >>> changing any code of driver amba-pl011.c, so I think the compatible >>> string is OK for this patch set. >>> >>> Hisilicon do some performance enhancements based on PL011, but the >>> current driver "amba-pl011.c" also works on hi6220 without those >>> enhancements driver code. >> Checked with Hisilicon chip designer, the UART0 is used for DEBUG >> console and compliant with PL011 fully. > > I think that given that we know the UART is not quite a PL011 we should > add an additional compatible string just in case some difference crops > up later that is problematic. > > So we'd have something like: > > compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell"; > > That way we can add any optimisations or workarounds later as required. I understand and thanks for your suggestion. Can I do not do this work in this patch set? Because I got the information UART0 is PL011 compatible. Hisilicon uart engineer can do this work in the future, maybe for UART1/UART2. Thank you Mark, BR, Bintian > > Thanks, > Mark. > > . >
On 6 May 2015 at 17:30, Mark Rutland <mark.rutland@arm.com> wrote: > On Wed, May 06, 2015 at 07:50:52AM +0100, Bintian wrote: >> Hello Mark, >> >> On 2015/5/6 11:16, Bintian wrote: >> > Hello Mark, >> > >> > On 2015/5/6 1:13, Mark Rutland wrote: >> [...] >> >>> + uart0: uart@f8015000 { /* console */ >> >>> + compatible = "arm,pl011", "arm,primecell"; >> >>> + reg = <0x0 0xf8015000 0x0 0x1000>; >> >>> + interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>; >> >>> + clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl >> >>> HI6220_UART0_PCLK>; >> >>> + clock-names = "uartclk", "apb_pclk"; >> >>> + }; >> >> >> >> In a previous discussion [1] the UART on HI6220 was described as not >> >> fully PL011 compliant, with a number of differences (e.g. the FIFO >> >> length). >> >> >> >> Given that, I feel somewhat uncomfortable with the current compatible >> >> string list. What exactly are those differences? We may need a more >> >> specific compatible string (even if in addition to those existing ones), >> >> or perhaps other properties. >> > The small system can be booted and the console also works well without >> > changing any code of driver amba-pl011.c, so I think the compatible >> > string is OK for this patch set. >> > >> > Hisilicon do some performance enhancements based on PL011, but the >> > current driver "amba-pl011.c" also works on hi6220 without those >> > enhancements driver code. >> Checked with Hisilicon chip designer, the UART0 is used for DEBUG >> console and compliant with PL011 fully. > > I think that given that we know the UART is not quite a PL011 we should > add an additional compatible string just in case some difference crops > up later that is problematic. > > So we'd have something like: > > compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell"; > > That way we can add any optimisations or workarounds later as required. > There's no code to handle the feature of hi6220 uart in mainline. It's meaningless to append "hisilicon,hi6220-uart" at here. I think it's only required when those uart code are merged into mainline. Regards Haojian
Hi, > > I think that given that we know the UART is not quite a PL011 we should > > add an additional compatible string just in case some difference crops > > up later that is problematic. > > > > So we'd have something like: > > > > compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell"; > > > > That way we can add any optimisations or workarounds later as required. > I understand and thanks for your suggestion. > > Can I do not do this work in this patch set? Because I got the > information UART0 is PL011 compatible. Hisilicon uart engineer can do > this work in the future, maybe for UART1/UART2. I am not asking you to do any driver work for this -- the current driver should ignore the "hisilicon,hi6220-uart" string and recognise "arm,pl011". I am only asking you to add the additional string to the DTS, and to update the binding document to list the new string. That way if and when we need the kernel to distinguish between a regular PL011 and the hi6220-specific variant, the DTB does not need to be updated in order to do so. Is UART 0 different from UART1 and UART2? Thanks, Mark.
On Wed, May 06, 2015 at 11:38:43AM +0100, Haojian Zhuang wrote: > On 6 May 2015 at 17:30, Mark Rutland <mark.rutland@arm.com> wrote: > > On Wed, May 06, 2015 at 07:50:52AM +0100, Bintian wrote: > >> Hello Mark, > >> > >> On 2015/5/6 11:16, Bintian wrote: > >> > Hello Mark, > >> > > >> > On 2015/5/6 1:13, Mark Rutland wrote: > >> [...] > >> >>> + uart0: uart@f8015000 { /* console */ > >> >>> + compatible = "arm,pl011", "arm,primecell"; > >> >>> + reg = <0x0 0xf8015000 0x0 0x1000>; > >> >>> + interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>; > >> >>> + clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl > >> >>> HI6220_UART0_PCLK>; > >> >>> + clock-names = "uartclk", "apb_pclk"; > >> >>> + }; > >> >> > >> >> In a previous discussion [1] the UART on HI6220 was described as not > >> >> fully PL011 compliant, with a number of differences (e.g. the FIFO > >> >> length). > >> >> > >> >> Given that, I feel somewhat uncomfortable with the current compatible > >> >> string list. What exactly are those differences? We may need a more > >> >> specific compatible string (even if in addition to those existing ones), > >> >> or perhaps other properties. > >> > The small system can be booted and the console also works well without > >> > changing any code of driver amba-pl011.c, so I think the compatible > >> > string is OK for this patch set. > >> > > >> > Hisilicon do some performance enhancements based on PL011, but the > >> > current driver "amba-pl011.c" also works on hi6220 without those > >> > enhancements driver code. > >> Checked with Hisilicon chip designer, the UART0 is used for DEBUG > >> console and compliant with PL011 fully. > > > > I think that given that we know the UART is not quite a PL011 we should > > add an additional compatible string just in case some difference crops > > up later that is problematic. > > > > So we'd have something like: > > > > compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell"; > > > > That way we can add any optimisations or workarounds later as required. > > > > There's no code to handle the feature of hi6220 uart in mainline. Sure, I wasn't suggesting that there was. > It's meaningless to append "hisilicon,hi6220-uart" at here. I think > it's only required when those uart code are merged into mainline. I disagree. Given that the UART is not quite a PL011, it is likely to behave differently under certain circumstances, and may have its own errata that need to be worked around. Even if the driver happens to currently work with it, a driver change could easily break that. By being explicit from the outset that this is a hi6220-specific PL011 variant, we can target workarounds far more effectively later, without requiring DTB updates. Having the additional string costs us nothing today. Thanks, Mark.
hi Mark, Thanks for detailed reviewing, pls see below inline comments. On Wed, May 06, 2015 at 10:20:56AM +0100, Mark Rutland wrote: > On Wed, May 06, 2015 at 04:51:20AM +0100, Leo Yan wrote: > > On Wed, May 06, 2015 at 11:16:12AM +0800, Bintian Wang wrote: > > > On 2015/5/6 1:13, Mark Rutland wrote: > > > >[...] > > > > > > > >>+ psci { > > > >>+ compatible = "arm,psci-0.2"; > > > >>+ method = "smc"; > > > >>+ }; > > > > > > > >Are all the PSCI 0.2 mandatory features implemented? > > > > > > The system off/suspend is under development, and system off will > > > be released in next months, and system suspend may be released in the > > > following two months. > > > > Yes; now for mandatory features, except SYSTEM_OFF others have supported. > > Ok. Do you mean in the next month, or in a number of months? We will finish it in May; the mainly work is to hook this API w/t PMIC to power down system. > > for PSCI 0.2, SYSTEM_SUSPEND even is not supported, right? > > SYSTEM_SUSPEND was introduced in PSCI 1.0, and is optional. > > So long as PSCI_VERSION reports 0.2 or PSCI_FEATURES reports > NOT_SUPPORTED for SYSTEM_SUSPEND, it's ok to not implement. > > I guess PSCI_VERSION reports PSCI 0.2? This is depend on ARM trusted firmware's implementation, now we are using ATFv1.1, so it will declare to support PSCI 1.0; pls see below boot log: psci: probing for conduit method from DT. psci: PSCIv1.0 detected in firmware. psci: Using standard PSCI v0.2 function IDs > > > So can I keep "arm,psci-0.2" here? > > > > > > >Can CPU0 be hot unplugged? > > > Yes, CPU0~CPU7 all can be hot unplugged. > > > > Just clarify: now use s/w method to emulate CPU and cluster's low > > power state; all cores can be hotplugged [1]. > > So long as CPUs brought online are reset appropriately, and "offline" > CPUs can't bring down the system somehow, that should be fine. For now > the important part is the interface. Thanks for confirmation. Thanks, Leo Yan
Hi Mark, 2015-05-06 18:55 GMT+08:00, Mark Rutland <mark.rutland@arm.com>: > Hi, > >> > I think that given that we know the UART is not quite a PL011 we should >> > add an additional compatible string just in case some difference crops >> > up later that is problematic. >> > >> > So we'd have something like: >> > >> > compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell"; >> > >> > That way we can add any optimisations or workarounds later as required. >> I understand and thanks for your suggestion. >> >> Can I do not do this work in this patch set? Because I got the >> information UART0 is PL011 compatible. Hisilicon uart engineer can do >> this work in the future, maybe for UART1/UART2. > > I am not asking you to do any driver work for this -- the current driver > should ignore the "hisilicon,hi6220-uart" string and recognise > "arm,pl011". > > I am only asking you to add the additional string to the DTS, and to > update the binding document to list the new string. That way if and when > we need the kernel to distinguish between a regular PL011 and the > hi6220-specific variant, the DTB does not need to be updated in order to > do so. How about add the following binding rule to my 2/5 patch: --------------------------- *Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART Required properties: - compatible: must be "hisilicon,hi6220-uart", "arm,primecell", "arm,pl011" - reg: exactly one register range with length 0x1000 - interrupts: exactly one interrupt specifier See also bindings/serial/pl011.txt Example: uart0: uart@f8015000 { compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell"; reg = <0x0 0xf8015000 0x0 0x1000>; interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>; clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl HI6220_UART0_PCLK>; clock-names = "uartclk", "apb_pclk"; }; --------------------------- > Is UART 0 different from UART1 and UART2? Yes, but my patch just includes UART0, we do some changements for UART1/2 to improve performance. Thanks, Bintian > > Thanks, > Mark. > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
Hi, > How about add the following binding rule to my 2/5 patch: > --------------------------- > *Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART > > Required properties: > - compatible: must be "hisilicon,hi6220-uart", "arm,primecell", "arm,pl011" "arm,primecell" should come last. > - reg: exactly one register range with length 0x1000 > - interrupts: exactly one interrupt specifier > > See also bindings/serial/pl011.txt > > Example: > > uart0: uart@f8015000 { > compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell"; > reg = <0x0 0xf8015000 0x0 0x1000>; > interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl HI6220_UART0_PCLK>; > clock-names = "uartclk", "apb_pclk"; > }; How about for the moment we just modify the existing pl011 binding document to allow for the hi6220-specific string in addition to its existing strings? We can always split it out later if the hi6220 UART binding needs to be significantly divergent. > > Is UART 0 different from UART1 and UART2? > Yes, but my patch just includes UART0, we do some changements for UART1/2 > to improve performance. Sure, but we need to make sure we can choose a sane compatible string for them, that won't clash with whatever we choose for UART0. Are they the same IP as UART0, or fundamentally different? Are they also PL011 derived? Thanks, Mark.
On Wed, May 06, 2015 at 05:03:29PM +0100, Brent Wang wrote: > Hi Mark, > > 2015-05-06 23:44 GMT+08:00, Mark Rutland <mark.rutland@arm.com>: > > Hi, > > > >> How about add the following binding rule to my 2/5 patch: > >> --------------------------- > >> *Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART > >> > >> Required properties: > >> - compatible: must be "hisilicon,hi6220-uart", "arm,primecell", > >> "arm,pl011" > > > > "arm,primecell" should come last. > > > >> - reg: exactly one register range with length 0x1000 > >> - interrupts: exactly one interrupt specifier > >> > >> See also bindings/serial/pl011.txt > >> > >> Example: > >> > >> uart0: uart@f8015000 { > >> compatible = "hisilicon,hi6220-uart", "arm,pl011", > >> "arm,primecell"; > >> reg = <0x0 0xf8015000 0x0 0x1000>; > >> interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>; > >> clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl > >> HI6220_UART0_PCLK>; > >> clock-names = "uartclk", "apb_pclk"; > >> }; > > > > How about for the moment we just modify the existing pl011 binding > > document to allow for the hi6220-specific string in addition to its > > existing strings? We can always split it out later if the hi6220 UART > > binding needs to be significantly divergent. > How about adding the following rule to pl011.txt? > ---------- > diff --git a/Documentation/devicetree/bindings/serial/pl011.txt > b/Documentation/devicetree/bindings/serial/pl011.txt > index ba3ecb8..1221ccb 100644 > --- a/Documentation/devicetree/bindings/serial/pl011.txt > +++ b/Documentation/devicetree/bindings/serial/pl011.txt > @@ -35,6 +35,10 @@ Optional properties: > - poll-timeout-ms: > Poll timeout when auto-poll is set, default > 3000ms. > +- compatible: "hisilicon,hi6220-uart": > + Hisilicon does some enhancements (e.g. larger FIFO length) > + based on PL011, so when using these UART hosts, this compatible > + string should be added. Up at the top of the document there's already a description of the compatible property. Just change it into a list something like: - compatible: should contain one of the following sequences: * "arm,pl011", "arm,primecell" * "hisilicon,hi6220-pl011", "arm,pl011", "arm,primecell" > See also bindings/arm/primecell.txt > ------------ > > > >> > Is UART 0 different from UART1 and UART2? > >> Yes, but my patch just includes UART0, we do some changements for > >> UART1/2 > >> to improve performance. > > > > Sure, but we need to make sure we can choose a sane compatible string > > for them, that won't clash with whatever we choose for UART0. > OK, do I also need to add compatible ""hisilicon,hi6220-uart"," to UART0 node? This depends on a number of factors. Questions below. > > Are they the same IP as UART0, or fundamentally different? > The same IP. > > > > Are they also PL011 derived? > Yes, just do some enhancements to improve performance. What exactly is different between UART0 and UART{1,2}, given they are the same IP? Can UART{1,2} be used as if they are standard PL011 instances? Is the difference internal, or do they just have different clocks/regulators/etc? Do they have feature control pins tied off differently? Are they simply programmed differently at reset by firmware? Thanks, Mark.
Hi Mark, 2015-05-07 0:23 GMT+08:00, Mark Rutland <mark.rutland@arm.com>: > On Wed, May 06, 2015 at 05:03:29PM +0100, Brent Wang wrote: >> Hi Mark, >> >> 2015-05-06 23:44 GMT+08:00, Mark Rutland <mark.rutland@arm.com>: >> > Hi, >> > >> >> How about add the following binding rule to my 2/5 patch: >> >> --------------------------- >> >> *Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART >> >> >> >> Required properties: >> >> - compatible: must be "hisilicon,hi6220-uart", "arm,primecell", >> >> "arm,pl011" >> > >> > "arm,primecell" should come last. >> > >> >> - reg: exactly one register range with length 0x1000 >> >> - interrupts: exactly one interrupt specifier >> >> >> >> See also bindings/serial/pl011.txt >> >> >> >> Example: >> >> >> >> uart0: uart@f8015000 { >> >> compatible = "hisilicon,hi6220-uart", "arm,pl011", >> >> "arm,primecell"; >> >> reg = <0x0 0xf8015000 0x0 0x1000>; >> >> interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>; >> >> clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl >> >> HI6220_UART0_PCLK>; >> >> clock-names = "uartclk", "apb_pclk"; >> >> }; >> > >> > How about for the moment we just modify the existing pl011 binding >> > document to allow for the hi6220-specific string in addition to its >> > existing strings? We can always split it out later if the hi6220 UART >> > binding needs to be significantly divergent. >> How about adding the following rule to pl011.txt? >> ---------- >> diff --git a/Documentation/devicetree/bindings/serial/pl011.txt >> b/Documentation/devicetree/bindings/serial/pl011.txt >> index ba3ecb8..1221ccb 100644 >> --- a/Documentation/devicetree/bindings/serial/pl011.txt >> +++ b/Documentation/devicetree/bindings/serial/pl011.txt >> @@ -35,6 +35,10 @@ Optional properties: >> - poll-timeout-ms: >> Poll timeout when auto-poll is set, default >> 3000ms. >> +- compatible: "hisilicon,hi6220-uart": >> + Hisilicon does some enhancements (e.g. larger FIFO length) >> + based on PL011, so when using these UART hosts, this >> compatible >> + string should be added. > > Up at the top of the document there's already a description of the > compatible property. Just change it into a list something like: > > - compatible: should contain one of the following sequences: > * "arm,pl011", "arm,primecell" > * "hisilicon,hi6220-pl011", "arm,pl011", "arm,primecell" OK, I will add in next version. > >> See also bindings/arm/primecell.txt >> ------------ >> > >> >> > Is UART 0 different from UART1 and UART2? >> >> Yes, but my patch just includes UART0, we do some changements for >> >> UART1/2 >> >> to improve performance. >> > >> > Sure, but we need to make sure we can choose a sane compatible string >> > for them, that won't clash with whatever we choose for UART0. >> OK, do I also need to add compatible ""hisilicon,hi6220-uart"," to UART0 >> node? > > This depends on a number of factors. Questions below. > >> > Are they the same IP as UART0, or fundamentally different? >> The same IP. >> > >> > Are they also PL011 derived? >> Yes, just do some enhancements to improve performance. > > What exactly is different between UART0 and UART{1,2}, given they are > the same IP? I know they are the same IP, but UART{1,2} have larger FIFO length. > > Can UART{1,2} be used as if they are standard PL011 instances? Yes > Is the difference internal, or do they just have different > clocks/regulators/etc? The different FIFO length means internal? Sure, they have different clocks/regualtors. > > Do they have feature control pins tied off differently? UART{1,2} have reset function, we must disable the reset before using them, is it the feature control pins you mentioned ? > > Are they simply programmed differently at reset by firmware? I think so. For hi6220 uart{1,2} , I think add one "struct vendor_data vendor_hisilicon" to "amba-pl011.c" may be a good solution in the future. I am not the uart driver developer and the Hisilicon engineer who develop it may have a deep discussion with you when upstreaming UART{1,2}. I suggest not add "hisilicon,hi6220-uart" to UART0; Must go to bed, it's very late here :( Thanks, Bintian > > Thanks, > Mark. >
Hi Mark, On 2015/5/7 1:15, Brent Wang wrote: > Hi Mark, > > 2015-05-07 0:23 GMT+08:00, Mark Rutland <mark.rutland@arm.com>: >> On Wed, May 06, 2015 at 05:03:29PM +0100, Brent Wang wrote: >>> Hi Mark, >>> >>> 2015-05-06 23:44 GMT+08:00, Mark Rutland <mark.rutland@arm.com>: >>>> Hi, >>>> >>>>> How about add the following binding rule to my 2/5 patch: >>>>> --------------------------- >>>>> *Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART >>>>> >>>>> Required properties: >>>>> - compatible: must be "hisilicon,hi6220-uart", "arm,primecell", >>>>> "arm,pl011" >>>> >>>> "arm,primecell" should come last. >>>> >>>>> - reg: exactly one register range with length 0x1000 >>>>> - interrupts: exactly one interrupt specifier >>>>> >>>>> See also bindings/serial/pl011.txt >>>>> >>>>> Example: >>>>> >>>>> uart0: uart@f8015000 { >>>>> compatible = "hisilicon,hi6220-uart", "arm,pl011", >>>>> "arm,primecell"; >>>>> reg = <0x0 0xf8015000 0x0 0x1000>; >>>>> interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>; >>>>> clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl >>>>> HI6220_UART0_PCLK>; >>>>> clock-names = "uartclk", "apb_pclk"; >>>>> }; >>>> >>>> How about for the moment we just modify the existing pl011 binding >>>> document to allow for the hi6220-specific string in addition to its >>>> existing strings? We can always split it out later if the hi6220 UART >>>> binding needs to be significantly divergent. >>> How about adding the following rule to pl011.txt? >>> ---------- >>> diff --git a/Documentation/devicetree/bindings/serial/pl011.txt >>> b/Documentation/devicetree/bindings/serial/pl011.txt >>> index ba3ecb8..1221ccb 100644 >>> --- a/Documentation/devicetree/bindings/serial/pl011.txt >>> +++ b/Documentation/devicetree/bindings/serial/pl011.txt >>> @@ -35,6 +35,10 @@ Optional properties: >>> - poll-timeout-ms: >>> Poll timeout when auto-poll is set, default >>> 3000ms. >>> +- compatible: "hisilicon,hi6220-uart": >>> + Hisilicon does some enhancements (e.g. larger FIFO length) >>> + based on PL011, so when using these UART hosts, this >>> compatible >>> + string should be added. >> >> Up at the top of the document there's already a description of the >> compatible property. Just change it into a list something like: >> >> - compatible: should contain one of the following sequences: >> * "arm,pl011", "arm,primecell" >> * "hisilicon,hi6220-pl011", "arm,pl011", "arm,primecell" > OK, I will add in next version. > >> >>> See also bindings/arm/primecell.txt >>> ------------ >>>> >>>>>> Is UART 0 different from UART1 and UART2? >>>>> Yes, but my patch just includes UART0, we do some changements for >>>>> UART1/2 >>>>> to improve performance. >>>> >>>> Sure, but we need to make sure we can choose a sane compatible string >>>> for them, that won't clash with whatever we choose for UART0. >>> OK, do I also need to add compatible ""hisilicon,hi6220-uart"," to UART0 >>> node? >> >> This depends on a number of factors. Questions below. >> >>>> Are they the same IP as UART0, or fundamentally different? >>> The same IP. >>>> >>>> Are they also PL011 derived? >>> Yes, just do some enhancements to improve performance. >> >> What exactly is different between UART0 and UART{1,2}, given they are >> the same IP? > I know they are the same IP, but UART{1,2} have larger FIFO length. > >> >> Can UART{1,2} be used as if they are standard PL011 instances? > Yes > >> Is the difference internal, or do they just have different >> clocks/regulators/etc? > The different FIFO length means internal? Sure, they have different > clocks/regualtors. >> >> Do they have feature control pins tied off differently? > UART{1,2} have reset function, we must disable the reset before using > them, is it the feature > control pins you mentioned ? >> >> Are they simply programmed differently at reset by firmware? > I think so. > > For hi6220 uart{1,2} , I think add one "struct vendor_data > vendor_hisilicon" to "amba-pl011.c" may be a good solution in the > future. > > I am not the uart driver developer and the Hisilicon engineer who > develop it may have a deep > discussion with you when upstreaming UART{1,2}. > > I suggest not add "hisilicon,hi6220-uart" to UART0; How about not adding "hisilicon,hi6220-uart" to UART0, and just add this compatible string for future use? you know, the UART0 is PL011 compatible and I checked this with chip designer. If there is no problem, I will send another version of this patch set and add this compatible string to pl011.txt as you suggested. Thanks, Bintian > > Must go to bed, it's very late here :( > > Thanks, > > Bintian >> >> Thanks, >> Mark. >> > >
Hi Mark, > Hi, > > > > I think that given that we know the UART is not quite a PL011 we should > > > add an additional compatible string just in case some difference crops > > > up later that is problematic. > > > > > > So we'd have something like: > > > > > > compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell"; > > > > > > That way we can add any optimisations or workarounds later as required. > > I understand and thanks for your suggestion. > > > > Can I do not do this work in this patch set? Because I got the > > information UART0 is PL011 compatible. Hisilicon uart engineer can do > > this work in the future, maybe for UART1/UART2. > > I am not asking you to do any driver work for this -- the current driver > should ignore the "hisilicon,hi6220-uart" string and recognise > "arm,pl011". > > I am only asking you to add the additional string to the DTS, and to > update the binding document to list the new string. That way if and when > we need the kernel to distinguish between a regular PL011 and the > hi6220-specific variant, the DTB does not need to be updated in order to > do so. How about add the following binding rule to the 2/5 patch: --------------------------- *Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART Required properties: - compatible: must be "hisilicon,hi6220-uart", "arm,primecell", "arm,pl011" - reg: exactly one register range with length 0x1000 - interrupts: exactly one interrupt specifier See also bindings/serial/pl011.txt Example: uart0: uart@f8015000 { compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell"; reg = <0x0 0xf8015000 0x0 0x1000>; interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>; clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl HI6220_UART0_PCLK>; clock-names = "uartclk", "apb_pclk"; }; --------------------------- > Is UART 0 different from UART1 and UART2? Yes, but my patch just includes UART0, we do some changements for UART1/2 to improve performance. Thanks, Bintian > > Thanks, > Mark. > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Please ignore this email. On 2015/5/13 15:12, Bintian Wang wrote: > Hi Mark, > >> Hi, >> >>>> I think that given that we know the UART is not quite a PL011 we should >>>> add an additional compatible string just in case some difference crops >>>> up later that is problematic. >>>> >>>> So we'd have something like: >>>> >>>> compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell"; >>>> >>>> That way we can add any optimisations or workarounds later as required. >>> I understand and thanks for your suggestion. >>> >>> Can I do not do this work in this patch set? Because I got the >>> information UART0 is PL011 compatible. Hisilicon uart engineer can do >>> this work in the future, maybe for UART1/UART2. >> >> I am not asking you to do any driver work for this -- the current driver >> should ignore the "hisilicon,hi6220-uart" string and recognise >> "arm,pl011". >> >> I am only asking you to add the additional string to the DTS, and to >> update the binding document to list the new string. That way if and when >> we need the kernel to distinguish between a regular PL011 and the >> hi6220-specific variant, the DTB does not need to be updated in order to >> do so. > How about add the following binding rule to the 2/5 patch: > --------------------------- > *Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART > > Required properties: > - compatible: must be "hisilicon,hi6220-uart", "arm,primecell", "arm,pl011" > - reg: exactly one register range with length 0x1000 > - interrupts: exactly one interrupt specifier > > See also bindings/serial/pl011.txt > > Example: > > uart0: uart@f8015000 { > compatible = "hisilicon,hi6220-uart", "arm,pl011", "arm,primecell"; > reg = <0x0 0xf8015000 0x0 0x1000>; > interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl HI6220_UART0_PCLK>; > clock-names = "uartclk", "apb_pclk"; > }; > --------------------------- > >> Is UART 0 different from UART1 and UART2? > Yes, but my patch just includes UART0, we do some changements for UART1/2 > to improve performance. > > Thanks, > > Bintian > >> >> Thanks, >> Mark. >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > > > . >
diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile index ad26a75..38913be 100644 --- a/arch/arm64/boot/dts/Makefile +++ b/arch/arm64/boot/dts/Makefile @@ -4,6 +4,7 @@ dts-dirs += arm dts-dirs += cavium dts-dirs += exynos dts-dirs += freescale +dts-dirs += hisilicon dts-dirs += mediatek dts-dirs += qcom dts-dirs += sprd diff --git a/arch/arm64/boot/dts/hisilicon/Makefile b/arch/arm64/boot/dts/hisilicon/Makefile new file mode 100644 index 0000000..fa81a6e --- /dev/null +++ b/arch/arm64/boot/dts/hisilicon/Makefile @@ -0,0 +1,5 @@ +dtb-$(CONFIG_ARCH_HISI) += hi6220-hikey.dtb + +always := $(dtb-y) +subdir-y := $(dts-dirs) +clean-files := *.dtb diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts new file mode 100644 index 0000000..e36a539 --- /dev/null +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts @@ -0,0 +1,31 @@ +/* + * dts file for Hisilicon HiKey Development Board + * + * Copyright (C) 2015, Hisilicon Ltd. + * + */ + +/dts-v1/; + +/*Reserved 1MB memory for MCU*/ +/memreserve/ 0x05e00000 0x00100000; + +#include "hi6220.dtsi" + +/ { + model = "HiKey Development Board"; + compatible = "hisilicon,hi6220-hikey", "hisilicon,hi6220"; + + aliases { + serial0 = &uart0; + }; + + chosen { + stdout-path = "serial0:115200n8"; + }; + + memory@0 { + device_type = "memory"; + reg = <0x0 0x0 0x0 0x40000000>; + }; +}; diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi new file mode 100644 index 0000000..e774865 --- /dev/null +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi @@ -0,0 +1,172 @@ +/* + * dts file for Hisilicon Hi6220 SoC + * + * Copyright (C) 2015, Hisilicon Ltd. + */ + +#include <dt-bindings/clock/hi6220-clock.h> +#include <dt-bindings/interrupt-controller/arm-gic.h> + +/ { + compatible = "hisilicon,hi6220"; + interrupt-parent = <&gic>; + #address-cells = <2>; + #size-cells = <2>; + + psci { + compatible = "arm,psci-0.2"; + method = "smc"; + }; + + cpus { + #address-cells = <2>; + #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@0 { + compatible = "arm,cortex-a53", "arm,armv8"; + device_type = "cpu"; + reg = <0x0 0x0>; + enable-method = "psci"; + }; + + cpu1: cpu@1 { + compatible = "arm,cortex-a53", "arm,armv8"; + device_type = "cpu"; + reg = <0x0 0x1>; + enable-method = "psci"; + }; + + cpu2: cpu@2 { + compatible = "arm,cortex-a53", "arm,armv8"; + device_type = "cpu"; + reg = <0x0 0x2>; + enable-method = "psci"; + }; + + cpu3: cpu@3 { + compatible = "arm,cortex-a53", "arm,armv8"; + device_type = "cpu"; + reg = <0x0 0x3>; + enable-method = "psci"; + }; + + cpu4: cpu@100 { + compatible = "arm,cortex-a53", "arm,armv8"; + device_type = "cpu"; + reg = <0x0 0x100>; + enable-method = "psci"; + }; + + cpu5: cpu@101 { + compatible = "arm,cortex-a53", "arm,armv8"; + device_type = "cpu"; + reg = <0x0 0x101>; + enable-method = "psci"; + }; + + cpu6: cpu@102 { + compatible = "arm,cortex-a53", "arm,armv8"; + device_type = "cpu"; + reg = <0x0 0x102>; + enable-method = "psci"; + }; + + cpu7: cpu@103 { + compatible = "arm,cortex-a53", "arm,armv8"; + device_type = "cpu"; + reg = <0x0 0x103>; + enable-method = "psci"; + }; + }; + + gic: interrupt-controller@f6801000 { + compatible = "arm,gic-400"; + reg = <0x0 0xf6801000 0 0x1000>, /* GICD */ + <0x0 0xf6802000 0 0x2000>, /* GICC */ + <0x0 0xf6804000 0 0x2000>, /* GICH */ + <0x0 0xf6806000 0 0x2000>; /* GICV */ + #address-cells = <0>; + #interrupt-cells = <3>; + interrupt-controller; + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>; + }; + + timer { + compatible = "arm,armv8-timer"; + interrupt-parent = <&gic>; + interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>, + <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>, + <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>, + <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>; + }; + + soc { + compatible = "simple-bus"; + #address-cells = <2>; + #size-cells = <2>; + ranges; + + ao_ctrl: ao_ctrl { + compatible = "hisilicon,hi6220-aoctrl", "syscon"; + reg = <0x0 0xf7800000 0x0 0x2000>; + #clock-cells = <1>; + }; + + sys_ctrl: sys_ctrl { + compatible = "hisilicon,hi6220-sysctrl", "syscon"; + reg = <0x0 0xf7030000 0x0 0x2000>; + #clock-cells = <1>; + }; + + media_ctrl: media_ctrl { + compatible = "hisilicon,hi6220-mediactrl", "syscon"; + reg = <0x0 0xf4410000 0x0 0x1000>; + #clock-cells = <1>; + }; + + pm_ctrl: pm_ctrl { + compatible = "hisilicon,hi6220-pmctrl", "syscon"; + reg = <0x0 0xf7032000 0x0 0x1000>; + #clock-cells = <1>; + }; + + uart0: uart@f8015000 { /* console */ + compatible = "arm,pl011", "arm,primecell"; + reg = <0x0 0xf8015000 0x0 0x1000>; + interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl HI6220_UART0_PCLK>; + clock-names = "uartclk", "apb_pclk"; + }; + }; +};