| Message ID | 1449295856-31068-3-git-send-email-yankejian@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Saturday 05 December 2015 14:10:56 yankejian wrote: > diff --git a/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt b/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt > index 80411b2..ecacfa4 100644 > --- a/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt > +++ b/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt > @@ -4,8 +4,6 @@ Required properties: > - compatible: should be "hisilicon,hns-dsaf-v1" or "hisilicon,hns-dsaf-v2". > "hisilicon,hns-dsaf-v1" is for hip05. > "hisilicon,hns-dsaf-v2" is for Hi1610 and Hi1612. > -- dsa-name: dsa fabric name who provide this interface. > - should be "dsafX", X is the dsaf id. > - mode: dsa fabric mode string. only support one of dsaf modes like these: > "2port-64vf", > "6port-16rss", > @@ -26,9 +24,8 @@ Required properties: > > Example: > > -dsa: dsa@c7000000 { > +dsaf0: dsa@c7000000 { > compatible = "hisilicon,hns-dsaf-v1"; > - dsa_name = "dsaf0"; > mode = "6port-16rss"; > interrupt-parent = <&mbigen_dsa>; > reg = <0x0 0xC0000000 0x0 0x420000 > diff --git a/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt b/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt > index 41d19be..e6a9d1c 100644 > --- a/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt > +++ b/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt > @@ -4,8 +4,9 @@ Required properties: > - compatible: "hisilicon,hns-nic-v1" or "hisilicon,hns-nic-v2". > "hisilicon,hns-nic-v1" is for hip05. > "hisilicon,hns-nic-v2" is for Hi1610 and Hi1612. > -- ae-name: accelerator name who provides this interface, > - is simply a name referring to the name of name in the accelerator node. > +- ae-handle: accelerator engine handle for hns, > + specifies a reference to the associating hardware driver node. > + see Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt > - port-id: is the index of port provided by DSAF (the accelerator). DSAF can > connect to 8 PHYs. Port 0 to 1 are both used for adminstration purpose. They > are called debug ports. > @@ -41,7 +42,7 @@ Example: > > This looks like an incompatible change, as you add and remove required properties. Is there a way to support both the old and the new style? Arnd
On 2015/12/6 6:19, Arnd Bergmann wrote: > On Saturday 05 December 2015 14:10:56 yankejian wrote: >> diff --git a/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt b/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt >> index 80411b2..ecacfa4 100644 >> --- a/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt >> +++ b/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt >> @@ -4,8 +4,6 @@ Required properties: >> - compatible: should be "hisilicon,hns-dsaf-v1" or "hisilicon,hns-dsaf-v2". >> "hisilicon,hns-dsaf-v1" is for hip05. >> "hisilicon,hns-dsaf-v2" is for Hi1610 and Hi1612. >> -- dsa-name: dsa fabric name who provide this interface. >> - should be "dsafX", X is the dsaf id. >> - mode: dsa fabric mode string. only support one of dsaf modes like these: >> "2port-64vf", >> "6port-16rss", >> @@ -26,9 +24,8 @@ Required properties: >> >> Example: >> >> -dsa: dsa@c7000000 { >> +dsaf0: dsa@c7000000 { >> compatible = "hisilicon,hns-dsaf-v1"; >> - dsa_name = "dsaf0"; >> mode = "6port-16rss"; >> interrupt-parent = <&mbigen_dsa>; >> reg = <0x0 0xC0000000 0x0 0x420000 >> diff --git a/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt b/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt >> index 41d19be..e6a9d1c 100644 >> --- a/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt >> +++ b/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt >> @@ -4,8 +4,9 @@ Required properties: >> - compatible: "hisilicon,hns-nic-v1" or "hisilicon,hns-nic-v2". >> "hisilicon,hns-nic-v1" is for hip05. >> "hisilicon,hns-nic-v2" is for Hi1610 and Hi1612. >> -- ae-name: accelerator name who provides this interface, >> - is simply a name referring to the name of name in the accelerator node. >> +- ae-handle: accelerator engine handle for hns, >> + specifies a reference to the associating hardware driver node. >> + see Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt >> - port-id: is the index of port provided by DSAF (the accelerator). DSAF can >> connect to 8 PHYs. Port 0 to 1 are both used for adminstration purpose. They >> are called debug ports. >> @@ -41,7 +42,7 @@ Example: >> >> > This looks like an incompatible change, as you add and remove > required properties. Is there a way to support both the old and > the new style? > > Arnd > > . Hi Arnd, Thanks for your suggestions. it must be set the same strings in dsaf node and every enet node before. it seems inappropriate. as Rob Herring <robh@kernel.org> 's suggestions, that would solve associating enet with a particular dsaf. so we discus the solution with Yisen Zhuang <yisen.zhuang@huawei.com>. we decide to use the new way instead of the old one. Best regards, yankejian >
On Monday 07 December 2015 15:14:13 Yankejian wrote: > On 2015/12/6 6:19, Arnd Bergmann wrote: > > On Saturday 05 December 2015 14:10:56 yankejian wrote: > >> diff --git a/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt b/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt > >> index 80411b2..ecacfa4 100644 > >> --- a/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt > >> +++ b/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt > >> @@ -4,8 +4,6 @@ Required properties: > >> - compatible: should be "hisilicon,hns-dsaf-v1" or "hisilicon,hns-dsaf-v2". > >> "hisilicon,hns-dsaf-v1" is for hip05. > >> "hisilicon,hns-dsaf-v2" is for Hi1610 and Hi1612. > >> -- dsa-name: dsa fabric name who provide this interface. > >> - should be "dsafX", X is the dsaf id. > >> - mode: dsa fabric mode string. only support one of dsaf modes like these: > >> "2port-64vf", > >> "6port-16rss", > >> @@ -26,9 +24,8 @@ Required properties: > >> > >> Example: > >> > >> -dsa: dsa@c7000000 { > >> +dsaf0: dsa@c7000000 { > >> compatible = "hisilicon,hns-dsaf-v1"; > >> - dsa_name = "dsaf0"; > >> mode = "6port-16rss"; > >> interrupt-parent = <&mbigen_dsa>; > >> reg = <0x0 0xC0000000 0x0 0x420000 > >> diff --git a/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt b/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt > >> index 41d19be..e6a9d1c 100644 > >> --- a/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt > >> +++ b/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt > >> @@ -4,8 +4,9 @@ Required properties: > >> - compatible: "hisilicon,hns-nic-v1" or "hisilicon,hns-nic-v2". > >> "hisilicon,hns-nic-v1" is for hip05. > >> "hisilicon,hns-nic-v2" is for Hi1610 and Hi1612. > >> -- ae-name: accelerator name who provides this interface, > >> - is simply a name referring to the name of name in the accelerator node. > >> +- ae-handle: accelerator engine handle for hns, > >> + specifies a reference to the associating hardware driver node. > >> + see Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt > >> - port-id: is the index of port provided by DSAF (the accelerator). DSAF can > >> connect to 8 PHYs. Port 0 to 1 are both used for adminstration purpose. They > >> are called debug ports. > >> @@ -41,7 +42,7 @@ Example: > >> > >> > > This looks like an incompatible change, as you add and remove > > required properties. Is there a way to support both the old and > > the new style? > > > > Arnd > > > > . > > Hi Arnd, > Thanks for your suggestions. it must be set the same strings in dsaf node and every enet node before. > it seems inappropriate. as Rob Herring <robh@kernel.org> 's suggestions, that would solve associating > enet with a particular dsaf. so we discus the solution with Yisen Zhuang <yisen.zhuang@huawei.com>. > we decide to use the new way instead of the old one. I agree the new form looks better than the original way, but I'm worried about the migration path. You don't explain in the patch description how you want to ensure that nothing breaks for existing systems. We generally try to avoid doing incompatible changes altogether and prefer to keep backwards compatibility, unless we can prove that no other systems exist that would get impacted by the change. Are you sure that nobody ships a DTB file for this hardware with their firmware that would now require an incompatible update which in turn breaks old kernels? Are you sure that there is no hardware using the same dsa hardware with out-of-tree dts files that need to make the same change but might not be aware of the change? Arnd
On 2015/12/7 17:40, Arnd Bergmann wrote: > On Monday 07 December 2015 15:14:13 Yankejian wrote: >> On 2015/12/6 6:19, Arnd Bergmann wrote: >>> On Saturday 05 December 2015 14:10:56 yankejian wrote: >>>> diff --git a/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt b/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt >>>> index 80411b2..ecacfa4 100644 >>>> --- a/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt >>>> +++ b/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt >>>> @@ -4,8 +4,6 @@ Required properties: >>>> - compatible: should be "hisilicon,hns-dsaf-v1" or "hisilicon,hns-dsaf-v2". >>>> "hisilicon,hns-dsaf-v1" is for hip05. >>>> "hisilicon,hns-dsaf-v2" is for Hi1610 and Hi1612. >>>> -- dsa-name: dsa fabric name who provide this interface. >>>> - should be "dsafX", X is the dsaf id. >>>> - mode: dsa fabric mode string. only support one of dsaf modes like these: >>>> "2port-64vf", >>>> "6port-16rss", >>>> @@ -26,9 +24,8 @@ Required properties: >>>> >>>> Example: >>>> >>>> -dsa: dsa@c7000000 { >>>> +dsaf0: dsa@c7000000 { >>>> compatible = "hisilicon,hns-dsaf-v1"; >>>> - dsa_name = "dsaf0"; >>>> mode = "6port-16rss"; >>>> interrupt-parent = <&mbigen_dsa>; >>>> reg = <0x0 0xC0000000 0x0 0x420000 >>>> diff --git a/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt b/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt >>>> index 41d19be..e6a9d1c 100644 >>>> --- a/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt >>>> +++ b/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt >>>> @@ -4,8 +4,9 @@ Required properties: >>>> - compatible: "hisilicon,hns-nic-v1" or "hisilicon,hns-nic-v2". >>>> "hisilicon,hns-nic-v1" is for hip05. >>>> "hisilicon,hns-nic-v2" is for Hi1610 and Hi1612. >>>> -- ae-name: accelerator name who provides this interface, >>>> - is simply a name referring to the name of name in the accelerator node. >>>> +- ae-handle: accelerator engine handle for hns, >>>> + specifies a reference to the associating hardware driver node. >>>> + see Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt >>>> - port-id: is the index of port provided by DSAF (the accelerator). DSAF can >>>> connect to 8 PHYs. Port 0 to 1 are both used for adminstration purpose. They >>>> are called debug ports. >>>> @@ -41,7 +42,7 @@ Example: >>>> >>>> >>> This looks like an incompatible change, as you add and remove >>> required properties. Is there a way to support both the old and >>> the new style? >>> >>> Arnd >>> >>> . >> Hi Arnd, >> Thanks for your suggestions. it must be set the same strings in dsaf node and every enet node before. >> it seems inappropriate. as Rob Herring <robh@kernel.org> 's suggestions, that would solve associating >> enet with a particular dsaf. so we discus the solution with Yisen Zhuang <yisen.zhuang@huawei.com>. >> we decide to use the new way instead of the old one. > I agree the new form looks better than the original way, but I'm worried > about the migration path. You don't explain in the patch description > how you want to ensure that nothing breaks for existing systems. > > We generally try to avoid doing incompatible changes altogether and > prefer to keep backwards compatibility, unless we can prove that no > other systems exist that would get impacted by the change. > > Are you sure that nobody ships a DTB file for this hardware with their > firmware that would now require an incompatible update which in turn > breaks old kernels? > > Are you sure that there is no hardware using the same dsa hardware > with out-of-tree dts files that need to make the same change but might > not be aware of the change? > > Arnd > > . hi Arnd. thanks a lot for pointing it out. It is great regret that this change breaks compatibility with old dtbs. this is a new driver which is run on developing boards, and all the clients' boards are developing boards. So we provide them a method to update the firmware on the board, once we update the dtsi and kernel, we require our clients to update the existed firmware and kernel. Therefore, these changes is actually under control. Shall we treat this by this way? thanks very much.
On Wednesday 09 December 2015 17:25:13 Yankejian wrote: > thanks a lot for pointing it out. > > It is great regret that this change breaks compatibility with old dtbs. > this is a new driver which is run on developing boards, and all the > clients' boards are developing boards. So we provide them a method to > update the firmware on the board, once we update the dtsi and kernel, > we require our clients to update the existed firmware and kernel. > Therefore, these changes is actually under control. Shall we treat this > by this way? Ok, if you can show that the incompatible change is safe to do, that's fine. Just put the explanation above into the patch description to document that you have considered the effects of the change, and to ensure that it gets done atomically. Also, you have to merge the two patches into one to allow bisection, or do a series of three patches that need to be applied in order: 1. add ae-handle property 2. change driver to use that property 3. remove ae-name property Arnd
On 2015/12/9 18:00, Arnd Bergmann wrote: > On Wednesday 09 December 2015 17:25:13 Yankejian wrote: >> thanks a lot for pointing it out. >> >> It is great regret that this change breaks compatibility with old dtbs. >> this is a new driver which is run on developing boards, and all the >> clients' boards are developing boards. So we provide them a method to >> update the firmware on the board, once we update the dtsi and kernel, >> we require our clients to update the existed firmware and kernel. >> Therefore, these changes is actually under control. Shall we treat this >> by this way? > Ok, if you can show that the incompatible change is safe to do, that's > fine. Just put the explanation above into the patch description to > document that you have considered the effects of the change, and > to ensure that it gets done atomically. > > Also, you have to merge the two patches into one to allow bisection, > or do a series of three patches that need to be applied in order: > > 1. add ae-handle property > 2. change driver to use that property > 3. remove ae-name property > > Arnd > > . > Ok, thanks for your good advices. i will change it in V2 PATCH. Bset Regards yankejian
diff --git a/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt b/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt index 80411b2..ecacfa4 100644 --- a/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt +++ b/Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt @@ -4,8 +4,6 @@ Required properties: - compatible: should be "hisilicon,hns-dsaf-v1" or "hisilicon,hns-dsaf-v2". "hisilicon,hns-dsaf-v1" is for hip05. "hisilicon,hns-dsaf-v2" is for Hi1610 and Hi1612. -- dsa-name: dsa fabric name who provide this interface. - should be "dsafX", X is the dsaf id. - mode: dsa fabric mode string. only support one of dsaf modes like these: "2port-64vf", "6port-16rss", @@ -26,9 +24,8 @@ Required properties: Example: -dsa: dsa@c7000000 { +dsaf0: dsa@c7000000 { compatible = "hisilicon,hns-dsaf-v1"; - dsa_name = "dsaf0"; mode = "6port-16rss"; interrupt-parent = <&mbigen_dsa>; reg = <0x0 0xC0000000 0x0 0x420000 diff --git a/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt b/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt index 41d19be..e6a9d1c 100644 --- a/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt +++ b/Documentation/devicetree/bindings/net/hisilicon-hns-nic.txt @@ -4,8 +4,9 @@ Required properties: - compatible: "hisilicon,hns-nic-v1" or "hisilicon,hns-nic-v2". "hisilicon,hns-nic-v1" is for hip05. "hisilicon,hns-nic-v2" is for Hi1610 and Hi1612. -- ae-name: accelerator name who provides this interface, - is simply a name referring to the name of name in the accelerator node. +- ae-handle: accelerator engine handle for hns, + specifies a reference to the associating hardware driver node. + see Documentation/devicetree/bindings/net/hisilicon-hns-dsaf.txt - port-id: is the index of port provided by DSAF (the accelerator). DSAF can connect to 8 PHYs. Port 0 to 1 are both used for adminstration purpose. They are called debug ports. @@ -41,7 +42,7 @@ Example: ethernet@0{ compatible = "hisilicon,hns-nic-v1"; - ae-name = "dsaf0"; + ae-handle = <&dsaf0>; port-id = <0>; local-mac-address = [a2 14 e4 4b 56 76]; }; diff --git a/arch/arm64/boot/dts/hisilicon/hip05_hns.dtsi b/arch/arm64/boot/dts/hisilicon/hip05_hns.dtsi index 606dd5a..89c883e 100644 --- a/arch/arm64/boot/dts/hisilicon/hip05_hns.dtsi +++ b/arch/arm64/boot/dts/hisilicon/hip05_hns.dtsi @@ -23,9 +23,8 @@ soc0: soc@000000000 { }; }; - dsa: dsa@c7000000 { + dsaf0: dsa@c7000000 { compatible = "hisilicon,hns-dsaf-v1"; - dsa_name = "dsaf0"; mode = "6port-16rss"; interrupt-parent = <&mbigen_dsa>; @@ -127,7 +126,7 @@ soc0: soc@000000000 { eth0: ethernet@0{ compatible = "hisilicon,hns-nic-v1"; - ae-name = "dsaf0"; + ae-handle = <&dsaf0>; port-id = <0>; local-mac-address = [00 00 00 01 00 58]; status = "disabled"; @@ -135,14 +134,14 @@ soc0: soc@000000000 { }; eth1: ethernet@1{ compatible = "hisilicon,hns-nic-v1"; - ae-name = "dsaf0"; + ae-handle = <&dsaf0>; port-id = <1>; status = "disabled"; dma-coherent; }; eth2: ethernet@2{ compatible = "hisilicon,hns-nic-v1"; - ae-name = "dsaf0"; + ae-handle = <&dsaf0>; port-id = <2>; local-mac-address = [00 00 00 01 00 5a]; status = "disabled"; @@ -150,7 +149,7 @@ soc0: soc@000000000 { }; eth3: ethernet@3{ compatible = "hisilicon,hns-nic-v1"; - ae-name = "dsaf0"; + ae-handle = <&dsaf0>; port-id = <3>; local-mac-address = [00 00 00 01 00 5b]; status = "disabled"; @@ -158,7 +157,7 @@ soc0: soc@000000000 { }; eth4: ethernet@4{ compatible = "hisilicon,hns-nic-v1"; - ae-name = "dsaf0"; + ae-handle = <&dsaf0>; port-id = <4>; local-mac-address = [00 00 00 01 00 5c]; status = "disabled"; @@ -166,7 +165,7 @@ soc0: soc@000000000 { }; eth5: ethernet@5{ compatible = "hisilicon,hns-nic-v1"; - ae-name = "dsaf0"; + ae-handle = <&dsaf0>; port-id = <5>; local-mac-address = [00 00 00 01 00 5d]; status = "disabled"; @@ -174,7 +173,7 @@ soc0: soc@000000000 { }; eth6: ethernet@6{ compatible = "hisilicon,hns-nic-v1"; - ae-name = "dsaf0"; + ae-handle = <&dsaf0>; port-id = <6>; local-mac-address = [00 00 00 01 00 5e]; status = "disabled"; @@ -182,7 +181,7 @@ soc0: soc@000000000 { }; eth7: ethernet@7{ compatible = "hisilicon,hns-nic-v1"; - ae-name = "dsaf0"; + ae-handle = <&dsaf0>; port-id = <7>; local-mac-address = [00 00 00 01 00 5f]; status = "disabled";
when enet specisies a reference to dsaf, the correlative config and documents needs to update. this patch updates the correlative dtsi file and bindings documents . Signed-off-by: yankejian <yankejian@huawei.com> --- .../devicetree/bindings/net/hisilicon-hns-dsaf.txt | 5 +---- .../devicetree/bindings/net/hisilicon-hns-nic.txt | 7 ++++--- arch/arm64/boot/dts/hisilicon/hip05_hns.dtsi | 19 +++++++++---------- 3 files changed, 14 insertions(+), 17 deletions(-)