lkml.org 
[lkml]   [2020]   [Apr]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH net-next v2 2/3] net: phy: add Broadcom BCM54140 support
Am 2020-04-19 17:49, schrieb Andrew Lunn:
> On Sun, Apr 19, 2020 at 12:12:48PM +0200, Michael Walle wrote:
>
> Hi Michael
>
>> +static int bcm54140_b0_workaround(struct phy_device *phydev)
>> +{
>> + int spare3;
>> + int ret;
>
> Could you add a comment about what this is working around?

sure

>
>> +static int bcm54140_phy_probe(struct phy_device *phydev)
>> +{
>> + struct bcm54140_phy_priv *priv;
>> + int ret;
>> +
>> + priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + phydev->priv = priv;
>> +
>> + ret = bcm54140_get_base_addr_and_port(phydev);
>> + if (ret)
>> + return ret;
>> +
>> + dev_info(&phydev->mdio.dev,
>> + "probed (port %d, base PHY address %d)\n",
>> + priv->port, priv->base_addr);
>
> phydev_dbg() ? Do we need to see this message four times?

ok. every phy will have a different port. And keep in mind,
that you might have less than four ports/PHYs here. So I'd
like to keep that as a phydev_dbg() if you agree.

>
>> +
>> + return 0;
>> +}
>> +
>> +static int bcm54140_config_init(struct phy_device *phydev)
>> +{
>> + u16 reg = 0xffff;
>> + int ret;
>> +
>> + /* Apply hardware errata */
>> + ret = bcm54140_b0_workaround(phydev);
>> + if (ret)
>> + return ret;
>> +
>> + /* Unmask events we are interested in. */
>> + reg &= ~(BCM54140_RDB_INT_DUPLEX |
>> + BCM54140_RDB_INT_SPEED |
>> + BCM54140_RDB_INT_LINK);
>> + ret = bcm_phy_write_rdb(phydev, BCM54140_RDB_IMR, reg);
>> + if (ret)
>> + return ret;
>> +
>> + /* LED1=LINKSPD[1], LED2=LINKSPD[2], LED3=ACTIVITY */
>> + ret = bcm_phy_modify_rdb(phydev, BCM54140_RDB_SPARE1,
>> + 0, BCM54140_RDB_SPARE1_LSLM);
>> + if (ret)
>> + return ret;
>
> What are the reset default for LEDs? Can the LEDs be configured via
> strapping pins? There is currently no good solution for this. Whatever
> you pick will be wrong for somebody else. At minimum, strapping pins,
> if they exist, should not be overridden.

Fair enough. There are no strapping options, just the "default
behaviour",
where LED1/2 indicates the speed, and LED3 just activity (no link
indication). And I just noticed that in this case the comment above is
wrong, because it is actually link/activity. Further, there are myriad
configuration options which I didn't want to encode altogether. So I've
just chosen the typical one (which actually matches our hardware), ie.
to have the "activity/led mode". The application note mentions some
other
concrete modes, but I don't know if its worth implementing them. Maybe
we
can have a enumeration of some distinct modes? Ie.

broadcom,led-mode = <BCM54140_NO_CHANGE>;
broadcom,led-mode = <BCM54140_ACT_LINK_MODE>;

-michael

\
 
 \ /
  Last update: 2020-04-19 18:34    [W:0.074 / U:0.560 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site