| Message ID | 1432950661-23060-5-git-send-email-bintian.wang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, May 30, 2015 at 3:50 AM, Bintian Wang <bintian.wang@huawei.com> wrote: > Hisilicon does some performance enhancements based on PL011(e.g. larger > FIFO length), so add one compatible string "hisilicon,hi6220-uart" for That compatible string in the commit message is not even the same as in the patch. > future optimisations or workarounds works. > > Signed-off-by: Bintian Wang <bintian.wang@huawei.com> > Suggested-by: Mark Rutland <mark.rutland@arm.com> Maybe I missed out on the earlier conversation, but do you mean that the PrimeCell ID has not been properly set up to something unique in this HiSilicon version of the PL011 block? Even if so: do not override the compatible string like this, that is not the PrimeCell style. Define an 8 bit vendor ID (like tha ASCII for 'H' 0x48 or whatever) and encode it for these variants, if the hardware is just using the ARM default PrimeCell ID, override it in the device tree like Broadcom are doing in arch/arm/boot/dts/bcm2835.dtsi: arm,primecell-periphid = <0x00241011>; Maybe yours would be: arm,primecell-periphid = <0x00048011>; For a first HiSilicon variant, then do some <include/linux/amba/bus.h>: enum amba_vendor { AMBA_VENDOR_ARM = 0x41, + AMBA_VENDOR_HISILICON = 0x48, Then patch drivers/tty/serial/amba_pl011.c to add vendor_hisilicon and a match table for 0x00048011 just like everyone else. Yours, Linus Walleij
On 02/06/15 09:59, Linus Walleij wrote: > On Sat, May 30, 2015 at 3:50 AM, Bintian Wang <bintian.wang@huawei.com> wrote: > >> Hisilicon does some performance enhancements based on PL011(e.g. larger >> FIFO length), so add one compatible string "hisilicon,hi6220-uart" for > > That compatible string in the commit message is not even > the same as in the patch. > >> future optimisations or workarounds works. >> >> Signed-off-by: Bintian Wang <bintian.wang@huawei.com> >> Suggested-by: Mark Rutland <mark.rutland@arm.com> > > Maybe I missed out on the earlier conversation, but do you > mean that the PrimeCell ID has not been properly set up > to something unique in this HiSilicon version of the PL011 > block? > > Even if so: do not override the compatible string like this, > that is not the PrimeCell style. > > Define an 8 bit vendor ID (like tha ASCII for 'H' 0x48 > or whatever) and encode it for these variants, if the > hardware is just using the ARM default PrimeCell > ID, override it in the device tree like Broadcom > are doing in arch/arm/boot/dts/bcm2835.dtsi: > > arm,primecell-periphid = <0x00241011>; > > Maybe yours would be: > > arm,primecell-periphid = <0x00048011>; > > For a first HiSilicon variant, then do some > <include/linux/amba/bus.h>: > > enum amba_vendor { > AMBA_VENDOR_ARM = 0x41, > + AMBA_VENDOR_HISILICON = 0x48, > > Then patch drivers/tty/serial/amba_pl011.c to add vendor_hisilicon > and a match table for 0x00048011 just like everyone else. That feels weird. This amba_vendor enum is not under control of the DT author, nor the kernel. This is a set of codes that are managed by a third party (probably ARM). What if some company with a name starting with 'H' (Hilarious Inc?) comes up with some actual HW and ends up conflicting with the above? Thanks, M.
On Tue, Jun 02, 2015 at 10:13:26AM +0100, Marc Zyngier wrote: > That feels weird. This amba_vendor enum is not under control of the DT > author, nor the kernel. This is a set of codes that are managed by a > third party (probably ARM). What if some company with a name starting > with 'H' (Hilarious Inc?) comes up with some actual HW and ends up > conflicting with the above? Presumably, HiSilicon have their own vendor code already, which presumably would be required for them to have a license to modify ARM IP.
On 2015/6/2 16:59, Linus Walleij wrote: > On Sat, May 30, 2015 at 3:50 AM, Bintian Wang <bintian.wang@huawei.com> wrote: > >> Hisilicon does some performance enhancements based on PL011(e.g. larger >> FIFO length), so add one compatible string "hisilicon,hi6220-uart" for > > That compatible string in the commit message is not even > the same as in the patch. The UART0 is PL011 compatible, the UART1/2 have some performance enhancements features, so based on Mark's suggestion and I add this compatible string just for future use. > >> future optimisations or workarounds works. >> >> Signed-off-by: Bintian Wang <bintian.wang@huawei.com> >> Suggested-by: Mark Rutland <mark.rutland@arm.com> > > Maybe I missed out on the earlier conversation, but do you > mean that the PrimeCell ID has not been properly set up > to something unique in this HiSilicon version of the PL011 > block? > > Even if so: do not override the compatible string like this, > that is not the PrimeCell style. > > Define an 8 bit vendor ID (like tha ASCII for 'H' 0x48 > or whatever) and encode it for these variants, if the > hardware is just using the ARM default PrimeCell > ID, override it in the device tree like Broadcom > are doing in arch/arm/boot/dts/bcm2835.dtsi: > > arm,primecell-periphid = <0x00241011>; > > Maybe yours would be: > > arm,primecell-periphid = <0x00048011>; > > For a first HiSilicon variant, then do some > <include/linux/amba/bus.h>: > > enum amba_vendor { > AMBA_VENDOR_ARM = 0x41, > + AMBA_VENDOR_HISILICON = 0x48, > > Then patch drivers/tty/serial/amba_pl011.c to add vendor_hisilicon > and a match table for 0x00048011 just like everyone else. Thanks and BR, Bintian > Yours, > Linus Walleij > > . >
On Tue, Jun 02, 2015 at 06:55:20PM +0800, Bintian wrote: > On 2015/6/2 16:59, Linus Walleij wrote: > >On Sat, May 30, 2015 at 3:50 AM, Bintian Wang <bintian.wang@huawei.com> wrote: > > > >>Hisilicon does some performance enhancements based on PL011(e.g. larger > >>FIFO length), so add one compatible string "hisilicon,hi6220-uart" for > > > >That compatible string in the commit message is not even > >the same as in the patch. > The UART0 is PL011 compatible, the UART1/2 have some performance > enhancements features, so based on Mark's suggestion and I add this > compatible string just for future use. Please don't submit it with this series. This patch should not be part of this series, it should be part of the series which modifies the PL011 driver, so it can be reviewed along with those changes. Until then, I'm going to NAK this patch. The thing that worries me though is that the subject line says this is a "Hisilicon *designed* UART". If Hisilicon _designed_ this UART, presumably they have changed the *vendor* field of the UART ID _not_ to indicate that ARM Ltd designed it? If they've merely modified the parameters, and given the ARM Ltd PL011 a larger fifo, then there isn't really much of a problem - we've been here before, except the vendor has had a real vendor ID for the field (in the case of ST), plus we've had different FIFO lengths for ARM hardware too (32 bytes instead of 16 for revision 3 and above.) Lastly, if you're not having to modify the PL011 driver in any way, you don't need to have a compatible. In any case, you _shouldn't_ for AMBA devices. AMBA does not match drivers based on OF compatible strings, so using OF compatible strings with the AMBA bus is just wrong. The AMBA compatible strings are there so that the generic OF code knows how to create the devices.
Hello Russell, On 2015/6/2 19:24, Russell King - ARM Linux wrote: > On Tue, Jun 02, 2015 at 06:55:20PM +0800, Bintian wrote: >> On 2015/6/2 16:59, Linus Walleij wrote: >>> On Sat, May 30, 2015 at 3:50 AM, Bintian Wang <bintian.wang@huawei.com> wrote: >>> >>>> Hisilicon does some performance enhancements based on PL011(e.g. larger >>>> FIFO length), so add one compatible string "hisilicon,hi6220-uart" for >>> >>> That compatible string in the commit message is not even >>> the same as in the patch. >> The UART0 is PL011 compatible, the UART1/2 have some performance >> enhancements features, so based on Mark's suggestion and I add this >> compatible string just for future use. > > Please don't submit it with this series. > > This patch should not be part of this series, it should be part of the > series which modifies the PL011 driver, so it can be reviewed along with > those changes. I agree with you and it's OK to me to remove this patch now. Could you help to ack the reset patches or I should send the version 10 without this patch? > > Until then, I'm going to NAK this patch. > > The thing that worries me though is that the subject line says this > is a "Hisilicon *designed* UART". If Hisilicon _designed_ this UART, > presumably they have changed the *vendor* field of the UART ID _not_ > to indicate that ARM Ltd designed it? > > If they've merely modified the parameters, and given the ARM Ltd PL011 > a larger fifo, then there isn't really much of a problem - we've been > here before, except the vendor has had a real vendor ID for the field > (in the case of ST), plus we've had different FIFO lengths for ARM > hardware too (32 bytes instead of 16 for revision 3 and above.) I think there is problem with my subject description, it's ARM designed indeed and Hisilicon just did some performance enhancements but not for UART0 in hi6220. > Lastly, if you're not having to modify the PL011 driver in any way, > you don't need to have a compatible. In any case, you _shouldn't_ for > AMBA devices. AMBA does not match drivers based on OF compatible > strings, so using OF compatible strings with the AMBA bus is just wrong. > The AMBA compatible strings are there so that the generic OF code knows > how to create the devices. Right. Thank you Russell. BR, Bintian
On Tue, Jun 2, 2015 at 11:13 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 02/06/15 09:59, Linus Walleij wrote: >> enum amba_vendor { >> AMBA_VENDOR_ARM = 0x41, >> + AMBA_VENDOR_HISILICON = 0x48, >> >> Then patch drivers/tty/serial/amba_pl011.c to add vendor_hisilicon >> and a match table for 0x00048011 just like everyone else. > > That feels weird. This amba_vendor enum is not under control of the DT > author, nor the kernel. This is a set of codes that are managed by a > third party (probably ARM). What if some company with a name starting > with 'H' (Hilarious Inc?) comes up with some actual HW and ends up > conflicting with the above? Sorry, as indicated by commit db4fa45ed3182d8206af241811dfc99369ffa849 "spi: pl022: Add support for chip select extension" bits [6:0] should be the JEP106 manufacturer identity code. Not that I know how you get such a code. Hardware which is modified and does not set the JEP106 ID code is buggy I guess. Anyways, the pattern we have to handle this is to use that ID code. Yours, Linus Walleij
diff --git a/Documentation/devicetree/bindings/serial/pl011.txt b/Documentation/devicetree/bindings/serial/pl011.txt index ba3ecb8..cb9fd9d 100644 --- a/Documentation/devicetree/bindings/serial/pl011.txt +++ b/Documentation/devicetree/bindings/serial/pl011.txt @@ -1,7 +1,9 @@ * ARM AMBA Primecell PL011 serial UART Required properties: -- compatible: must be "arm,primecell", "arm,pl011" +- compatible: should contain one of the following sequences: + * "arm,pl011", "arm,primecell" + * "hisilicon,hi6220-pl011", "arm,pl011", "arm,primecell" - reg: exactly one register range with length 0x1000 - interrupts: exactly one interrupt specifier
Hisilicon does some performance enhancements based on PL011(e.g. larger FIFO length), so add one compatible string "hisilicon,hi6220-uart" for future optimisations or workarounds works. Signed-off-by: Bintian Wang <bintian.wang@huawei.com> Suggested-by: Mark Rutland <mark.rutland@arm.com> --- Documentation/devicetree/bindings/serial/pl011.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)