| Message ID | 1487752716-14824-2-git-send-email-xuejiancheng@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/22/2017 02:38 AM, Jiancheng Xue wrote: > Add bindings for HiSilicon hi3798cv200 SoC and Poplar Board. > > Signed-off-by: Jiancheng Xue <xuejiancheng@hisilicon.com> I reviewed this before; the only change is it's separated from the original patch. Reviewed-by: Alex Elder <elder@linaro.org> > --- > Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt > index f1c1e21..1fd3dd7 100644 > --- a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt > +++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt > @@ -4,6 +4,10 @@ Hi3660 SoC > Required root node properties: > - compatible = "hisilicon,hi3660"; > > +Hi3798cv200 Poplar Board > +Required root node properties: > + - compatible = "hisilicon,hi3798cv200-poplar", "hisilicon,hi3798cv200"; > + > Hi4511 Board > Required root node properties: > - compatible = "hisilicon,hi3620-hi4511"; >
On Wed, 22 Feb 2017, Jiancheng Xue wrote: > Add bindings for HiSilicon hi3798cv200 SoC and Poplar Board. > > Signed-off-by: Jiancheng Xue <xuejiancheng@hisilicon.com> Acked-by: Peter Griffin <peter.griffin@linaro.org> > --- > Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt > index f1c1e21..1fd3dd7 100644 > --- a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt > +++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt > @@ -4,6 +4,10 @@ Hi3660 SoC > Required root node properties: > - compatible = "hisilicon,hi3660"; > > +Hi3798cv200 Poplar Board > +Required root node properties: > + - compatible = "hisilicon,hi3798cv200-poplar", "hisilicon,hi3798cv200"; > + > Hi4511 Board > Required root node properties: > - compatible = "hisilicon,hi3620-hi4511";
Am 22.02.2017 um 09:38 schrieb Jiancheng Xue: > Add bindings for HiSilicon hi3798cv200 SoC and Poplar Board. > > Signed-off-by: Jiancheng Xue <xuejiancheng@hisilicon.com> > --- > Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt > index f1c1e21..1fd3dd7 100644 > --- a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt > +++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt > @@ -4,6 +4,10 @@ Hi3660 SoC > Required root node properties: > - compatible = "hisilicon,hi3660"; > > +Hi3798cv200 Poplar Board > +Required root node properties: > + - compatible = "hisilicon,hi3798cv200-poplar", "hisilicon,hi3798cv200"; Please remember to CC previous reviewers. This still looks wrong: Why is this not "hisilicon,poplar" if you choose against "tocoding,poplar"? Is there a second Poplar board with a different SoC? Even then it would be redundant with the second compatible string. Regards, Andreas > + > Hi4511 Board > Required root node properties: > - compatible = "hisilicon,hi3620-hi4511";
Hi Andreas, On 2017/2/26 9:32, Andreas Färber wrote: > Am 22.02.2017 um 09:38 schrieb Jiancheng Xue: >> Add bindings for HiSilicon hi3798cv200 SoC and Poplar Board. >> >> Signed-off-by: Jiancheng Xue <xuejiancheng@hisilicon.com> >> --- >> Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt >> index f1c1e21..1fd3dd7 100644 >> --- a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt >> +++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt >> @@ -4,6 +4,10 @@ Hi3660 SoC >> Required root node properties: >> - compatible = "hisilicon,hi3660"; >> >> +Hi3798cv200 Poplar Board >> +Required root node properties: >> + - compatible = "hisilicon,hi3798cv200-poplar", "hisilicon,hi3798cv200"; > > Please remember to CC previous reviewers. > Sorry for that. > This still looks wrong: Why is this not "hisilicon,poplar" if you choose > against "tocoding,poplar"? I didn't think it was very important thing whether the compatbile string contained a preceding SoC name or not. I just referred to the hikey board and some other HiSilicon boards. I wanted to keep using the same rule with them. > Is there a second Poplar board with a different SoC? I can't tell about this now. > Even then it would be redundant with the second > compatible string. > The second compatilbe string can be removed here. Thanks. Regards, Jiancheng > Regards, > Andreas > >> + >> Hi4511 Board >> Required root node properties: >> - compatible = "hisilicon,hi3620-hi4511"; >
On 02/26/2017 07:24 PM, Jiancheng Xue wrote: > Hi Andreas, > > On 2017/2/26 9:32, Andreas Färber wrote: >> Am 22.02.2017 um 09:38 schrieb Jiancheng Xue: >>> Add bindings for HiSilicon hi3798cv200 SoC and Poplar Board. >>> >>> Signed-off-by: Jiancheng Xue <xuejiancheng@hisilicon.com> >>> --- >>> Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt >>> index f1c1e21..1fd3dd7 100644 >>> --- a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt >>> +++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt >>> @@ -4,6 +4,10 @@ Hi3660 SoC >>> Required root node properties: >>> - compatible = "hisilicon,hi3660"; >>> >>> +Hi3798cv200 Poplar Board >>> +Required root node properties: >>> + - compatible = "hisilicon,hi3798cv200-poplar", "hisilicon,hi3798cv200"; >> >> Please remember to CC previous reviewers. >> > Sorry for that. > >> This still looks wrong: Why is this not "hisilicon,poplar" if you choose >> against "tocoding,poplar"? > > I didn't think it was very important thing whether the compatbile string contained > a preceding SoC name or not. I just referred to the hikey board and some other > HiSilicon boards. I wanted to keep using the same rule with them. The way Jiancheng defined this was consistent with the pattern used for all other definitions of platforms found in this documentation file. Why make this one different? >> Is there a second Poplar board with a different SoC? > > I can't tell about this now. There is not. But there could be. In any case, I do accept your point that there's no need to encode the SoC identity in the board compatible string. But I don't think doing so causes harm. >> Even then it would be redundant with the second >> compatible string. I presume you're not arguing that the second compatible string should be eliminated; it seems your concern is only about including the SoC in the board's compatible string. Is that right? > The second compatilbe string can be removed here. Thanks. I don't think it should be. My position, for what it's worth, is that if a change is made to the compatible strings, it should be: compatible = "hisilicon,poplar", "hisilicon,hi3798cv200"; But I don't think it's necessary to make that change. Tocoding has no other DT presence in the kernel right now; it seems fine to tag the board with "hisilicon". -Alex > > Regards, > Jiancheng > >> Regards, >> Andreas >> >>> + >>> Hi4511 Board >>> Required root node properties: >>> - compatible = "hisilicon,hi3620-hi4511"; >> >
Hi, Am 27.02.2017 um 03:48 schrieb Alex Elder: > On 02/26/2017 07:24 PM, Jiancheng Xue wrote: >> On 2017/2/26 9:32, Andreas Färber wrote: >>> Am 22.02.2017 um 09:38 schrieb Jiancheng Xue: >>>> Add bindings for HiSilicon hi3798cv200 SoC and Poplar Board. >>>> >>>> Signed-off-by: Jiancheng Xue <xuejiancheng@hisilicon.com> >>>> --- >>>> Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt >>>> index f1c1e21..1fd3dd7 100644 >>>> --- a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt >>>> +++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt >>>> @@ -4,6 +4,10 @@ Hi3660 SoC >>>> Required root node properties: >>>> - compatible = "hisilicon,hi3660"; >>>> >>>> +Hi3798cv200 Poplar Board >>>> +Required root node properties: >>>> + - compatible = "hisilicon,hi3798cv200-poplar", "hisilicon,hi3798cv200"; >>> >>> Please remember to CC previous reviewers. >>> >> Sorry for that. >> >>> This still looks wrong: Why is this not "hisilicon,poplar" if you choose >>> against "tocoding,poplar"? >> >> I didn't think it was very important thing whether the compatbile string contained >> a preceding SoC name or not. I just referred to the hikey board and some other >> HiSilicon boards. I wanted to keep using the same rule with them. > > The way Jiancheng defined this was consistent with the pattern > used for all other definitions of platforms found in this > documentation file. Why make this one different? I am not familiar with other HiSilicon DTs but rather with several other vendors' DTs. This seems inconsistent with the rest. The only other one with SoC names in the board compatible I know of is i.MX6, where there's variations between single, dual and quad versions. >>> Is there a second Poplar board with a different SoC? >> >> I can't tell about this now. > > There is not. But there could be. In any case, I do accept > your point that there's no need to encode the SoC identity > in the board compatible string. But I don't think doing so > causes harm. > >>> Even then it would be redundant with the second >>> compatible string. > > I presume you're not arguing that the second compatible > string should be eliminated; it seems your concern is only > about including the SoC in the board's compatible string. > Is that right? > >> The second compatilbe string can be removed here. Thanks. > > I don't think it should be. Correct, the second one must stay, because that allows for matching against the SoC. of_machine_is_compatible() does not do partial comparisons afaik, so there's no value in a vendor,soc-board pattern. > My position, for what it's worth, is that if a change is > made to the compatible strings, it should be: > > compatible = "hisilicon,poplar", "hisilicon,hi3798cv200"; > > But I don't think it's necessary to make that change. Tocoding > has no other DT presence in the kernel right now; it seems fine > to tag the board with "hisilicon". Adding vendor prefixes seems a common task these days (e.g., for the UDOO Neo we had to define a udoo prefix and couldn't just reuse nxp as its SoC vendor; hwacom, kingnovel, ucrobotics are some other pending Chinese vendors I'm aware of, so tocoding isn't singled out). Whether here it should be one or the other I can't tell, but whether or not a vendor prefix is available has definitely not been the criteria. In the case of the HiKey the vendor changed from CircuitCo to LeMaker and we were able to fully reuse the bootloader and DT. On the other hand, there's no additional compatible to detect which one you are on. Independently, hisilicon.txt should be overhauled to not give contradicting instructions for SoC vs. board. The SoC should say "must contain ..." about the compatible string. Which reminds me that this patch is lacking an entry for the SoC! I also wonder why hi3620-hi4511 comes after hi3798cv200. https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Regards, Andreas
On Wed, Feb 22, 2017 at 04:38:35PM +0800, Jiancheng Xue wrote: > Add bindings for HiSilicon hi3798cv200 SoC and Poplar Board. > > Signed-off-by: Jiancheng Xue <xuejiancheng@hisilicon.com> > --- > Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt | 4 ++++ > 1 file changed, 4 insertions(+) Acked-by: Rob Herring <robh@kernel.org>
Hi Andreas, On Mon, 27 Feb 2017, Andreas Färber wrote: > Hi, > > Am 27.02.2017 um 03:48 schrieb Alex Elder: > > On 02/26/2017 07:24 PM, Jiancheng Xue wrote: > >> On 2017/2/26 9:32, Andreas Färber wrote: > >>> Am 22.02.2017 um 09:38 schrieb Jiancheng Xue: > >>>> Add bindings for HiSilicon hi3798cv200 SoC and Poplar Board. > >>>> > >>>> Signed-off-by: Jiancheng Xue <xuejiancheng@hisilicon.com> > >>>> --- > >>>> Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt | 4 ++++ > >>>> 1 file changed, 4 insertions(+) > >>>> > >>>> diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt > >>>> index f1c1e21..1fd3dd7 100644 > >>>> --- a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt > >>>> +++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt > >>>> @@ -4,6 +4,10 @@ Hi3660 SoC > >>>> Required root node properties: > >>>> - compatible = "hisilicon,hi3660"; > >>>> > >>>> +Hi3798cv200 Poplar Board > >>>> +Required root node properties: > >>>> + - compatible = "hisilicon,hi3798cv200-poplar", "hisilicon,hi3798cv200"; > >>> > >>> Please remember to CC previous reviewers. > >>> > >> Sorry for that. > >> > >>> This still looks wrong: Why is this not "hisilicon,poplar" if you choose > >>> against "tocoding,poplar"? > >> > >> I didn't think it was very important thing whether the compatbile string contained > >> a preceding SoC name or not. I just referred to the hikey board and some other > >> HiSilicon boards. I wanted to keep using the same rule with them. > > > > The way Jiancheng defined this was consistent with the pattern > > used for all other definitions of platforms found in this > > documentation file. Why make this one different? > > I am not familiar with other HiSilicon DTs but rather with several other > vendors' DTs. This seems inconsistent with the rest. The only other one > with SoC names in the board compatible I know of is i.MX6, where there's > variations between single, dual and quad versions. I've not checked all the subarchs, but STMicroelectronics chipsets I've been involved with upstreaming also use <soc>-<board> compatible strings. For STi b2120 board it could actually have STiH410 or STiH407 SoC. But others like stih418-b2199 and stih410-b2260 only have one SoC. Also stm32 arch does the same thing. Just having a quick look around arch/arm64/ as above examples were arch/arm/ and I see quite a few other examples as well e.g. compatible = "mediatek,mt6755-evb", "mediatek,mt6755"; compatible = "mediatek,mt8173-evb", "mediatek,mt8173"; compatible = "fsl,ls1043a-qds", "fsl,ls1043a"; compatible = "samsung,exynos7-espresso", "samsung,exynos7"; compatible = "zte,zx296718-evb", "zte,zx296718"; compatible = "lge,lg1313-ref", "lge,lg1313"; compatible = "rockchip,rk3368-evb-act8846", "rockchip,rk3368"; Some subarchs have a mix within them, which maybe due to the SoC also being part of the board name on some reference boards. But both ways seem to be used in the kernel. So IMHO it would be better to see consistency in arch/arm64/hisilicon* even though there is not consistency across all of arch/arm64/* and arch/arm/*. regards, Peter.
diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt index f1c1e21..1fd3dd7 100644 --- a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt @@ -4,6 +4,10 @@ Hi3660 SoC Required root node properties: - compatible = "hisilicon,hi3660"; +Hi3798cv200 Poplar Board +Required root node properties: + - compatible = "hisilicon,hi3798cv200-poplar", "hisilicon,hi3798cv200"; + Hi4511 Board Required root node properties: - compatible = "hisilicon,hi3620-hi4511";
Add bindings for HiSilicon hi3798cv200 SoC and Poplar Board. Signed-off-by: Jiancheng Xue <xuejiancheng@hisilicon.com> --- Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt | 4 ++++ 1 file changed, 4 insertions(+)