| Message ID | 20240802100448.10745-1-pmenzel@molgen.mpg.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tg3: Add param `short_preamble` to enable MDIO traffic to external PHYs | expand |
On Fri, Aug 2, 2024 at 3:05 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote: > diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c > index 0ec5f01551f9..9b4ab201fd9a 100644 > --- a/drivers/net/ethernet/broadcom/tg3.c > +++ b/drivers/net/ethernet/broadcom/tg3.c > @@ -233,6 +233,10 @@ static int tg3_debug = -1; /* -1 == use TG3_DEF_MSG_ENABLE as value */ > module_param(tg3_debug, int, 0); > MODULE_PARM_DESC(tg3_debug, "Tigon3 bitmapped debugging message enable value"); > > +static int short_preamble = 0; > +module_param(short_preamble, int, 0); > +MODULE_PARM_DESC(short_preamble, "Enable short preamble."); > + Module parameters are generally not accepted. If this is something other devices can potentially use, it's better to use a more common interface.
[Cc: +SONiC Linux kernel maintainers, cf patch submission [1]] Dear Michael, Thank you for the quick reply. Am 02.08.24 um 21:51 schrieb Michael Chan: > On Fri, Aug 2, 2024 at 3:05 AM Paul Menzel wrote: > >> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c >> index 0ec5f01551f9..9b4ab201fd9a 100644 >> --- a/drivers/net/ethernet/broadcom/tg3.c >> +++ b/drivers/net/ethernet/broadcom/tg3.c >> @@ -233,6 +233,10 @@ static int tg3_debug = -1; /* -1 == use TG3_DEF_MSG_ENABLE as value */ >> module_param(tg3_debug, int, 0); >> MODULE_PARM_DESC(tg3_debug, "Tigon3 bitmapped debugging message enable value"); >> >> +static int short_preamble = 0; >> +module_param(short_preamble, int, 0); >> +MODULE_PARM_DESC(short_preamble, "Enable short preamble."); >> + > > Module parameters are generally not accepted. If this is something > other devices can potentially use, it's better to use a more common > interface. I saw the patch in the SONiC repository and took a shot at upstreaming it. `tg.h` defines the macro: #define MAC_MI_MODE_SHORT_PREAMBLE 0x00000002 Any idea how this should be used? Can it be enabled unconditionally? I do not even have the datasheet. Kind regards, Paul [1]: https://lore.kernel.org/netdev/20240802100448.10745-1-pmenzel@molgen.mpg.de/
On Fri, Aug 2, 2024 at 1:55 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote: > > >> +static int short_preamble = 0; > >> +module_param(short_preamble, int, 0); > >> +MODULE_PARM_DESC(short_preamble, "Enable short preamble."); > >> + > > > > Module parameters are generally not accepted. If this is something > > other devices can potentially use, it's better to use a more common > > interface. > > I saw the patch in the SONiC repository and took a shot at upstreaming > it. `tg.h` defines the macro: > > #define MAC_MI_MODE_SHORT_PREAMBLE 0x00000002 > > Any idea how this should be used? Can it be enabled unconditionally? I > do not even have the datasheet. > Here's the programmer's reference: https://docs.broadcom.com/doc/571X-5720-PG1XX But there is not much additional information about the SHORT_PREAMBLE bit. I will need to ask internally about this bit. Thanks.
On Fri, Aug 02, 2024 at 12:51:15PM -0700, Michael Chan wrote: > On Fri, Aug 2, 2024 at 3:05 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote: > > > diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c > > index 0ec5f01551f9..9b4ab201fd9a 100644 > > --- a/drivers/net/ethernet/broadcom/tg3.c > > +++ b/drivers/net/ethernet/broadcom/tg3.c > > @@ -233,6 +233,10 @@ static int tg3_debug = -1; /* -1 == use TG3_DEF_MSG_ENABLE as value */ > > module_param(tg3_debug, int, 0); > > MODULE_PARM_DESC(tg3_debug, "Tigon3 bitmapped debugging message enable value"); > > > > +static int short_preamble = 0; > > +module_param(short_preamble, int, 0); > > +MODULE_PARM_DESC(short_preamble, "Enable short preamble."); > > + > > Module parameters are generally not accepted. If this is something > other devices can potentially use, it's better to use a more common > interface. +1 Most systems supporting suppressed preamble do this using the DT property 'suppress-preamble'. See: Documentation/devicetree/bindings/net/mdio.yaml You could add an ACPI parameter for x86. You could also consider implementing clock-frequency if the MDIO bus master and all the devices on the bus support faster than 2.5MHz. Andrew
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c index 0ec5f01551f9..9b4ab201fd9a 100644 --- a/drivers/net/ethernet/broadcom/tg3.c +++ b/drivers/net/ethernet/broadcom/tg3.c @@ -233,6 +233,10 @@ static int tg3_debug = -1; /* -1 == use TG3_DEF_MSG_ENABLE as value */ module_param(tg3_debug, int, 0); MODULE_PARM_DESC(tg3_debug, "Tigon3 bitmapped debugging message enable value"); +static int short_preamble = 0; +module_param(short_preamble, int, 0); +MODULE_PARM_DESC(short_preamble, "Enable short preamble."); + #define TG3_DRV_DATA_FLAG_10_100_ONLY 0x0001 #define TG3_DRV_DATA_FLAG_5705_10_100 0x0002 @@ -1493,6 +1497,11 @@ static void tg3_mdio_config_5785(struct tg3 *tp) static void tg3_mdio_start(struct tg3 *tp) { tp->mi_mode &= ~MAC_MI_MODE_AUTO_POLL; + + if(short_preamble) { + tp->mi_mode |= MAC_MI_MODE_SHORT_PREAMBLE; + } + tw32_f(MAC_MI_MODE, tp->mi_mode); udelay(80);