| Message ID | 1454528129-6144-1-git-send-email-aaro.koskinen@iki.fi |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Feb 03, 2016 at 09:35:29PM +0200, Aaro Koskinen wrote: > Commit ae461131960b ("of: of_mdio: Add a whitelist of PHY > compatibilities.") missed one compatible string used in in-tree DTBs: > in OCTEON, for selected boards, the kernel DTB pruning code will overwrite > the DTB compatible string with "marvell,88e1145", which is missing > from the whitelist. Add it. Does this overwriting means this compatibility is not visible in the current DTS files? Or did i miss it? At least for the Marvell SoCs i intend to submit a patch removing these compatible strings from the DTS files. Will you do the same for the OCTEON boards? > The patch fixes broken networking on EdgeRouter Lite. > > Fixes: ae461131960b ("of: of_mdio: Add a whitelist of PHY compatibilities.") > Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Thanks Andrew > --- > drivers/of/of_mdio.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c > index 5648317..39c4be4 100644 > --- a/drivers/of/of_mdio.c > +++ b/drivers/of/of_mdio.c > @@ -154,6 +154,7 @@ static const struct of_device_id whitelist_phys[] = { > { .compatible = "marvell,88E1111", }, > { .compatible = "marvell,88e1116", }, > { .compatible = "marvell,88e1118", }, > + { .compatible = "marvell,88e1145", }, > { .compatible = "marvell,88e1149r", }, > { .compatible = "marvell,88e1310", }, > { .compatible = "marvell,88E1510", }, > -- > 2.4.0 >
On 02/03/2016 12:08 PM, Andrew Lunn wrote: > On Wed, Feb 03, 2016 at 09:35:29PM +0200, Aaro Koskinen wrote: >> Commit ae461131960b ("of: of_mdio: Add a whitelist of PHY >> compatibilities.") missed one compatible string used in in-tree DTBs: >> in OCTEON, for selected boards, the kernel DTB pruning code will overwrite >> the DTB compatible string with "marvell,88e1145", which is missing >> from the whitelist. Add it. > > Does this overwriting means this compatibility is not visible in the > current DTS files? Or did i miss it? > > At least for the Marvell SoCs i intend to submit a patch removing > these compatible strings from the DTS files. Will you do the same for > the OCTEON boards? The compatibility strings may be present in deployed firmware, they cannot be removed. For many OCTEON boards, the device tree is a firmware-kernel ABI, it is not practical to unilaterally decide to change the bindings on the kernel side as you don't control the firmware. David Daney > >> The patch fixes broken networking on EdgeRouter Lite. >> >> Fixes: ae461131960b ("of: of_mdio: Add a whitelist of PHY compatibilities.") >> Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > Thanks > Andrew > >> --- >> drivers/of/of_mdio.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c >> index 5648317..39c4be4 100644 >> --- a/drivers/of/of_mdio.c >> +++ b/drivers/of/of_mdio.c >> @@ -154,6 +154,7 @@ static const struct of_device_id whitelist_phys[] = { >> { .compatible = "marvell,88E1111", }, >> { .compatible = "marvell,88e1116", }, >> { .compatible = "marvell,88e1118", }, >> + { .compatible = "marvell,88e1145", }, >> { .compatible = "marvell,88e1149r", }, >> { .compatible = "marvell,88e1310", }, >> { .compatible = "marvell,88E1510", }, >> -- >> 2.4.0 >>
> The compatibility strings may be present in deployed firmware, they > cannot be removed. For many OCTEON boards, the device tree is a > firmware-kernel ABI, it is not practical to unilaterally decide to > change the bindings on the kernel side as you don't control the > firmware. Hi David We are keeping backwards compatibility. The kernel has always ignored this string, and will continue to always ignore this string. But since it is being ignored, you may as well remove it in future versions of the DTB. Andrew
Hi, On Wed, Feb 03, 2016 at 09:08:57PM +0100, Andrew Lunn wrote: > On Wed, Feb 03, 2016 at 09:35:29PM +0200, Aaro Koskinen wrote: > > Commit ae461131960b ("of: of_mdio: Add a whitelist of PHY > > compatibilities.") missed one compatible string used in in-tree DTBs: > > in OCTEON, for selected boards, the kernel DTB pruning code will overwrite > > the DTB compatible string with "marvell,88e1145", which is missing > > from the whitelist. Add it. > > Does this overwriting means this compatibility is not visible in the > current DTS files? Or did i miss it? Yeah, it happens in arch/mips/cavium-octeon/octeon-platform.c: if (octeon_has_88e1145()) { fdt_nop_property(initial_boot_params, phy, "marvell,reg-init"); memset(new_name, 0, sizeof(new_name)); strcpy(new_name, "marvell,88e1145"); It took a while for me to figure out this as well... Nasty. > At least for the Marvell SoCs i intend to submit a patch removing > these compatible strings from the DTS files. Will you do the same for > the OCTEON boards? Yes, for in-tree OCTEON DTS files, I can do the update; the above strcpy needs to be deleted at the same go, and this needs go through MIPS tree. A.
Hi, On Wed, Feb 03, 2016 at 12:14:05PM -0800, David Daney wrote: > On 02/03/2016 12:08 PM, Andrew Lunn wrote: > >On Wed, Feb 03, 2016 at 09:35:29PM +0200, Aaro Koskinen wrote: > >>Commit ae461131960b ("of: of_mdio: Add a whitelist of PHY > >>compatibilities.") missed one compatible string used in in-tree DTBs: > >>in OCTEON, for selected boards, the kernel DTB pruning code will overwrite > >>the DTB compatible string with "marvell,88e1145", which is missing > >>from the whitelist. Add it. > > > >Does this overwriting means this compatibility is not visible in the > >current DTS files? Or did i miss it? > > > >At least for the Marvell SoCs i intend to submit a patch removing > >these compatible strings from the DTS files. Will you do the same for > >the OCTEON boards? > > The compatibility strings may be present in deployed firmware, they cannot > be removed. For many OCTEON boards, the device tree is a firmware-kernel > ABI, it is not practical to unilaterally decide to change the bindings on > the kernel side as you don't control the firmware. I agree from practical point of view, but OTOH kernel has never accepted those bindings as an ABI. Now users may need to put up with warnings like: [Firmware Warn]: /soc@0/mdio@1180000001800/ethernet-phy@7: Whitelisted compatible string. Please remove [Firmware Warn]: /soc@0/mdio@1180000001800/ethernet-phy@6: Whitelisted compatible string. Please remove if the strings are not updated. If user loses PHY (like now with EdgeRouter Lite), the string need to be added to the whitelist. Cannot say if this will be an issue for firmware DTB OCTEON users; the only firmware DTB board (EdgeRouter Pro) I have seems to provide correct strings: broadcom,bcm ethernet-phy-ieee802.3-c22 A.
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c index 5648317..39c4be4 100644 --- a/drivers/of/of_mdio.c +++ b/drivers/of/of_mdio.c @@ -154,6 +154,7 @@ static const struct of_device_id whitelist_phys[] = { { .compatible = "marvell,88E1111", }, { .compatible = "marvell,88e1116", }, { .compatible = "marvell,88e1118", }, + { .compatible = "marvell,88e1145", }, { .compatible = "marvell,88e1149r", }, { .compatible = "marvell,88e1310", }, { .compatible = "marvell,88E1510", },
Commit ae461131960b ("of: of_mdio: Add a whitelist of PHY compatibilities.") missed one compatible string used in in-tree DTBs: in OCTEON, for selected boards, the kernel DTB pruning code will overwrite the DTB compatible string with "marvell,88e1145", which is missing from the whitelist. Add it. The patch fixes broken networking on EdgeRouter Lite. Fixes: ae461131960b ("of: of_mdio: Add a whitelist of PHY compatibilities.") Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi> --- drivers/of/of_mdio.c | 1 + 1 file changed, 1 insertion(+)