The do_mdio_entry was responsible for generating a phy alias configure
that according to the phy driver's mdio_device_id, before apply this
patch, which alias configure is like "alias mdio:000000010100000100001
1011101????", it doesn't match the phy_id of mdio_uevent, because of
the phy_id was a hexadecimal digit and the mido uevent is consisit of
phy_id with the char 'p', the uevent string is different from alias.
Add this patch that mdio alias configure will can match mdio uevent.
Signed-off-by: Yinbo Zhu <[email protected]>
---
Change in v4:
Add following explain information.
Hi Russell King ,
I had given you feedback lots of times, but it may be mail list server issue, you don't accept my mail,
and I don't get your mail, then I add that what i want explain in v1 patch for your v1 patch comment,
you can check. then for v3 patch that is for rework commit inforation accorting , just , I notice you
have a comment in v2, but i dont' accept your mail. and I give you explain as follows:
> No. I think we've been over the reasons already. It _will_ break
> existing module loading.
> If I look at the PHY IDs on my Clearfog board:
> /sys/bus/mdio_bus/devices/f1072004.mdio-mii:00/phy_id:0x01410dd1
> /sys/bus/mdio_bus/devices/mv88e6xxx-0:00/phy_id:0x01410eb1
> /sys/bus/mdio_bus/devices/mv88e6xxx-0:01/phy_id:0x01410eb1
> /sys/bus/mdio_bus/devices/mv88e6xxx-0:02/phy_id:0x01410eb1
> /sys/bus/mdio_bus/devices/mv88e6xxx-0:03/phy_id:0x01410eb1
> /sys/bus/mdio_bus/devices/mv88e6xxx-0:04/phy_id:0x01410eb1
> /sys/bus/mdio_bus/devices/mv88e6xxx-0:0f/phy_id:0x01410ea1
> and then look at the PHY IDs that the kernel uses in the drivers, and
> thus will be used in the module's alias tables.
> #define MARVELL_PHY_ID_88E1510 0x01410dd0
> #define MARVELL_PHY_ID_88E1540 0x01410eb0
> #define MARVELL_PHY_ID_88E1545 0x01410ea0
> These numbers are different. This is not just one board. The last nibble
> of the PHY ID is generally the PHY revision, but that is not universal.
> See Atheros PHYs, where we match the entire ID except bit 4.
> You can not "simplify" the "ugly" matching like this. It's the way it is
> for good reason. Using the whole ID will _not_ cause a match, and your
> change will cause a regression.
On my platform, I can find following, stmmac-xx that is is mac name, it represent this platform has two mac
controller, it isn't phy, but it's sub dir has phy id it is phy silicon id, and devices name is set by mdio bus,
then, you said that "where we match the entire ID except bit 4." I think marvell it is special, and you can have
look other phy,e.g. motorcomm phy, that phy mask is 0x00000fff or 0x0000ffff or ther, for different phy silicon,
that phy maskit is not same, and that phy mask it isn't device property, you dont't get it from register, and mdio
bus for phy register a device then mdiobus_scan will get phy id that phy id it is include all value, and don't has
mask operation. then phy uevent must use phy_id that from phy register and that uevent doesn't include phy mask, if
uevent add phy mask, then you need define a phy mask. if you have mature consideration, you will find that definition
phy mask it isn't appropriate, if you define it in mac driver, because mac can select different phy, if you define it
in dts, then phy driver is "of" type, mdio_uevent will doesn't be called. if you ask phy_id & phy_mask as a phy uevent,
I think it is no make sense, e.g. 88e1512 and 88e1510 that has different "phy revision" so that phy silicon has difference,
and uevent should be unique. If you have no other opinion on the above view, Back to this patch, that phy id of uevent
need match phy alias configure, so alis configure must use phy id all value.
In addition, Even if you hadn't consided what I said above, you need to know one thing, uevent match alias that must be full
matching. not Partial matching. I said it a long time ago. why you think Binary digit and "?" can match dev uevent,
according my code analysis and test analysis that any platform and any mdio phy device is all fail that be matched if that
phy driver module and phy dev was use uevent-alias mechanism
[root@localhost ~]# cat /sys/bus/mdio/devices/stmmac-19\:00/phy_id
0x01410dd1
[root@localhost ~]#
[root@localhost ~]# cat /sys/bus/mdio/devices/stmmac-18\:00/phy_id
0x01410dd1
[root@localhost ~]#
Thanks,
BRs,
Yinbo Zhu.
include/linux/mod_devicetable.h | 2 ++
scripts/mod/file2alias.c | 17 +----------------
2 files changed, 3 insertions(+), 16 deletions(-)
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index ae2e75d..7bd23bf 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -595,6 +595,8 @@ struct platform_device_id {
kernel_ulong_t driver_data;
};
+#define MDIO_ANY_ID (~0)
+
#define MDIO_NAME_SIZE 32
#define MDIO_MODULE_PREFIX "mdio:"
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 49aba86..63f3149 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1027,24 +1027,9 @@ static int do_platform_entry(const char *filename,
static int do_mdio_entry(const char *filename,
void *symval, char *alias)
{
- int i;
DEF_FIELD(symval, mdio_device_id, phy_id);
- DEF_FIELD(symval, mdio_device_id, phy_id_mask);
-
alias += sprintf(alias, MDIO_MODULE_PREFIX);
-
- for (i = 0; i < 32; i++) {
- if (!((phy_id_mask >> (31-i)) & 1))
- *(alias++) = '?';
- else if ((phy_id >> (31-i)) & 1)
- *(alias++) = '1';
- else
- *(alias++) = '0';
- }
-
- /* Terminate the string */
- *alias = 0;
-
+ ADD(alias, "p", phy_id != MDIO_ANY_ID, phy_id);
return 1;
}
--
1.8.3.1
The of_device_uevent_modalias is service for 'of' type platform driver
, which ask the first args must be 'of' that use MODULE_DEVICE_TABLE
when driver was exported, but ethernet phy is a kind of 'mdio' type
device and it is inappropriate if driver use 'of' type for exporting,
in fact, most mainstream ethernet phy driver hasn't used 'of' type,
even though phy driver was exported use 'of' type and it's irrelevant
with mdio_uevent, at this time, platform_uevent was responsible for
reporting uevent to match modules.alias configure, so, whatever that
of_device_uevent_modalias was unnecessary, this patch was to remove it
and add phy_id as modio uevent then ethernet phy module auto load
function will work well.
Signed-off-by: Yinbo Zhu <[email protected]>
---
drivers/net/phy/mdio_bus.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 6865d93..999f0d4 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -962,12 +962,12 @@ static int mdio_bus_match(struct device *dev, struct device_driver *drv)
static int mdio_uevent(struct device *dev, struct kobj_uevent_env *env)
{
- int rc;
+ struct phy_device *pdev;
- /* Some devices have extra OF data and an OF-style MODALIAS */
- rc = of_device_uevent_modalias(dev, env);
- if (rc != -ENODEV)
- return rc;
+ pdev = to_phy_device(dev);
+
+ if (add_uevent_var(env, "MODALIAS=mdio:p%08X", pdev->phy_id))
+ return -ENOMEM;
return 0;
}
@@ -991,7 +991,7 @@ static int mdio_uevent(struct device *dev, struct kobj_uevent_env *env)
};
struct bus_type mdio_bus_type = {
- .name = "mdio_bus",
+ .name = "mdio",
.dev_groups = mdio_bus_dev_groups,
.match = mdio_bus_match,
.uevent = mdio_uevent,
--
1.8.3.1
On Sat, Dec 04, 2021 at 05:13:27PM +0800, Yinbo Zhu wrote:
> The do_mdio_entry was responsible for generating a phy alias configure
> that according to the phy driver's mdio_device_id, before apply this
> patch, which alias configure is like "alias mdio:000000010100000100001
> 1011101????", it doesn't match the phy_id of mdio_uevent, because of
> the phy_id was a hexadecimal digit and the mido uevent is consisit of
> phy_id with the char 'p', the uevent string is different from alias.
> Add this patch that mdio alias configure will can match mdio uevent.
>
> Signed-off-by: Yinbo Zhu <[email protected]>
NAK.
> ---
> Change in v4:
> Add following explain information.
>
> Hi Russell King ,
>
>
> I had given you feedback lots of times, but it may be mail list server issue, you don't accept my mail,
>
> and I don't get your mail, then I add that what i want explain in v1 patch for your v1 patch comment,
>
> you can check. then for v3 patch that is for rework commit inforation accorting , just , I notice you
>
> have a comment in v2, but i dont' accept your mail. and I give you explain as follows:
>
>
> > No. I think we've been over the reasons already. It _will_ break
> > existing module loading.
>
> > If I look at the PHY IDs on my Clearfog board:
>
> > /sys/bus/mdio_bus/devices/f1072004.mdio-mii:00/phy_id:0x01410dd1
> > /sys/bus/mdio_bus/devices/mv88e6xxx-0:00/phy_id:0x01410eb1
> > /sys/bus/mdio_bus/devices/mv88e6xxx-0:01/phy_id:0x01410eb1
> > /sys/bus/mdio_bus/devices/mv88e6xxx-0:02/phy_id:0x01410eb1
> > /sys/bus/mdio_bus/devices/mv88e6xxx-0:03/phy_id:0x01410eb1
> > /sys/bus/mdio_bus/devices/mv88e6xxx-0:04/phy_id:0x01410eb1
> > /sys/bus/mdio_bus/devices/mv88e6xxx-0:0f/phy_id:0x01410ea1
>
> > and then look at the PHY IDs that the kernel uses in the drivers, and
> > thus will be used in the module's alias tables.
>
> > #define MARVELL_PHY_ID_88E1510 0x01410dd0
> > #define MARVELL_PHY_ID_88E1540 0x01410eb0
> > #define MARVELL_PHY_ID_88E1545 0x01410ea0
>
> > These numbers are different. This is not just one board. The last nibble
> > of the PHY ID is generally the PHY revision, but that is not universal.
> > See Atheros PHYs, where we match the entire ID except bit 4.
>
> > You can not "simplify" the "ugly" matching like this. It's the way it is
> > for good reason. Using the whole ID will _not_ cause a match, and your
> > change will cause a regression.
>
> On my platform, I can find following, stmmac-xx that is is mac name, it represent this platform has two mac
> controller, it isn't phy, but it's sub dir has phy id it is phy silicon id, and devices name is set by mdio bus,
> then, you said that "where we match the entire ID except bit 4." I think marvell it is special, and you can have
> look other phy,e.g. motorcomm phy, that phy mask is 0x00000fff or 0x0000ffff or ther, for different phy silicon,
> that phy maskit is not same, and that phy mask it isn't device property, you dont't get it from register, and mdio
> bus for phy register a device then mdiobus_scan will get phy id that phy id it is include all value, and don't has
> mask operation. then phy uevent must use phy_id that from phy register and that uevent doesn't include phy mask, if
> uevent add phy mask, then you need define a phy mask. if you have mature consideration, you will find that definition
> phy mask it isn't appropriate, if you define it in mac driver, because mac can select different phy, if you define it
> in dts, then phy driver is "of" type, mdio_uevent will doesn't be called. if you ask phy_id & phy_mask as a phy uevent,
> I think it is no make sense, e.g. 88e1512 and 88e1510 that has different "phy revision" so that phy silicon has difference,
> and uevent should be unique. If you have no other opinion on the above view, Back to this patch, that phy id of uevent
> need match phy alias configure, so alis configure must use phy id all value.
>
> In addition, Even if you hadn't consided what I said above, you need to know one thing, uevent match alias that must be full
> matching. not Partial matching. I said it a long time ago. why you think Binary digit and "?" can match dev uevent,
> according my code analysis and test analysis that any platform and any mdio phy device is all fail that be matched if that
> phy driver module and phy dev was use uevent-alias mechanism
>
> [root@localhost ~]# cat /sys/bus/mdio/devices/stmmac-19\:00/phy_id
> 0x01410dd1
> [root@localhost ~]#
> [root@localhost ~]# cat /sys/bus/mdio/devices/stmmac-18\:00/phy_id
> 0x01410dd1
> [root@localhost ~]#
>
> Thanks,
> BRs,
> Yinbo Zhu.
>
>
> include/linux/mod_devicetable.h | 2 ++
> scripts/mod/file2alias.c | 17 +----------------
> 2 files changed, 3 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index ae2e75d..7bd23bf 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -595,6 +595,8 @@ struct platform_device_id {
> kernel_ulong_t driver_data;
> };
>
> +#define MDIO_ANY_ID (~0)
> +
> #define MDIO_NAME_SIZE 32
> #define MDIO_MODULE_PREFIX "mdio:"
>
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 49aba86..63f3149 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1027,24 +1027,9 @@ static int do_platform_entry(const char *filename,
> static int do_mdio_entry(const char *filename,
> void *symval, char *alias)
> {
> - int i;
> DEF_FIELD(symval, mdio_device_id, phy_id);
> - DEF_FIELD(symval, mdio_device_id, phy_id_mask);
> -
> alias += sprintf(alias, MDIO_MODULE_PREFIX);
> -
> - for (i = 0; i < 32; i++) {
> - if (!((phy_id_mask >> (31-i)) & 1))
> - *(alias++) = '?';
> - else if ((phy_id >> (31-i)) & 1)
> - *(alias++) = '1';
> - else
> - *(alias++) = '0';
> - }
> -
> - /* Terminate the string */
> - *alias = 0;
> -
> + ADD(alias, "p", phy_id != MDIO_ANY_ID, phy_id);
> return 1;
> }
>
> --
> 1.8.3.1
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Sat, Dec 04, 2021 at 05:13:28PM +0800, Yinbo Zhu wrote:
> The of_device_uevent_modalias is service for 'of' type platform driver
> , which ask the first args must be 'of' that use MODULE_DEVICE_TABLE
> when driver was exported, but ethernet phy is a kind of 'mdio' type
> device and it is inappropriate if driver use 'of' type for exporting,
> in fact, most mainstream ethernet phy driver hasn't used 'of' type,
> even though phy driver was exported use 'of' type and it's irrelevant
> with mdio_uevent, at this time, platform_uevent was responsible for
> reporting uevent to match modules.alias configure, so, whatever that
> of_device_uevent_modalias was unnecessary, this patch was to remove it
> and add phy_id as modio uevent then ethernet phy module auto load
> function will work well.
>
> Signed-off-by: Yinbo Zhu <[email protected]>
NAK.
> ---
>
> drivers/net/phy/mdio_bus.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 6865d93..999f0d4 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -962,12 +962,12 @@ static int mdio_bus_match(struct device *dev, struct device_driver *drv)
>
> static int mdio_uevent(struct device *dev, struct kobj_uevent_env *env)
> {
> - int rc;
> + struct phy_device *pdev;
>
> - /* Some devices have extra OF data and an OF-style MODALIAS */
> - rc = of_device_uevent_modalias(dev, env);
> - if (rc != -ENODEV)
> - return rc;
> + pdev = to_phy_device(dev);
> +
> + if (add_uevent_var(env, "MODALIAS=mdio:p%08X", pdev->phy_id))
> + return -ENOMEM;
>
> return 0;
> }
> @@ -991,7 +991,7 @@ static int mdio_uevent(struct device *dev, struct kobj_uevent_env *env)
> };
>
> struct bus_type mdio_bus_type = {
> - .name = "mdio_bus",
> + .name = "mdio",
> .dev_groups = mdio_bus_dev_groups,
> .match = mdio_bus_match,
> .uevent = mdio_uevent,
> --
> 1.8.3.1
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Sat, Dec 04, 2021 at 05:13:27PM +0800, Yinbo Zhu wrote:
> The do_mdio_entry was responsible for generating a phy alias configure
> that according to the phy driver's mdio_device_id, before apply this
> patch, which alias configure is like "alias mdio:000000010100000100001
> 1011101????", it doesn't match the phy_id of mdio_uevent, because of
> the phy_id was a hexadecimal digit and the mido uevent is consisit of
> phy_id with the char 'p', the uevent string is different from alias.
> Add this patch that mdio alias configure will can match mdio uevent.
>
> Signed-off-by: Yinbo Zhu <[email protected]>
> ---
> Change in v4:
> Add following explain information.
Adding an explanation will not stop the regression happening. You will
continue to get a NACK while your change causes a regression. Please
do not post again until you have addressed the regression.
Andrew