[net-next,0/3] net: mscc-miim: add integrated PHY reset support

The MDIO driver has support to release the integrated PHYs from reset.
This was implemented for the SparX-5 for now. Now add support for the
LAN966x, too.

Michael Walle (3):
  dt-bindings: net: mscc-miim: add lan966x compatible
  net: mdio: mscc-miim: replace magic numbers for the bus reset
  net: mdio: mscc-miim: add lan966x internal phy reset support

 .../devicetree/bindings/net/mscc-miim.txt     |  2 +-
 drivers/net/mdio/mdio-mscc-miim.c             | 59 +++++++++++++------
 2 files changed, 42 insertions(+), 19 deletions(-)

> diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
> index 64fb76c1e395..7773d5019e66 100644
> --- a/drivers/net/mdio/mdio-mscc-miim.c
> +++ b/drivers/net/mdio/mdio-mscc-miim.c
> @@ -158,18 +158,18 @@ static int mscc_miim_reset(struct mii_bus *bus)
>  {
>  	struct mscc_miim_dev *miim = bus->priv;
>  	int offset = miim->phy_reset_offset;
> +	int mask = PHY_CFG_PHY_ENA | PHY_CFG_PHY_COMMON_RESET |
> +		   PHY_CFG_PHY_RESET;

> -		ret = regmap_write(miim->phy_regs,
> -				   MSCC_PHY_REG_PHY_CFG + offset, 0x1ff);
> +		ret = regmap_write(miim->phy_regs, offset, mask);

Is mask the correct name? It is not being used in the typical way for
a mask.

  Andrew

Am 2022-03-13 01:51, schrieb Andrew Lunn:
>> diff --git a/drivers/net/mdio/mdio-mscc-miim.c 
>> b/drivers/net/mdio/mdio-mscc-miim.c
>> index 64fb76c1e395..7773d5019e66 100644
>> --- a/drivers/net/mdio/mdio-mscc-miim.c
>> +++ b/drivers/net/mdio/mdio-mscc-miim.c
>> @@ -158,18 +158,18 @@ static int mscc_miim_reset(struct mii_bus *bus)
>>  {
>>  	struct mscc_miim_dev *miim = bus->priv;
>>  	int offset = miim->phy_reset_offset;
>> +	int mask = PHY_CFG_PHY_ENA | PHY_CFG_PHY_COMMON_RESET |
>> +		   PHY_CFG_PHY_RESET;
> 
>> -		ret = regmap_write(miim->phy_regs,
>> -				   MSCC_PHY_REG_PHY_CFG + offset, 0x1ff);
>> +		ret = regmap_write(miim->phy_regs, offset, mask);
> 
> Is mask the correct name? It is not being used in the typical way for
> a mask.

It is the mask of all the reset bits, see also patch 3/3. Either all
these bits are set or none. Do you have any suggestion? I thought about
adding mask and value for the remap_update_bits() in patch 3/3 but
decided against it, just because it doesn't add any value because
mask and value are the same.

-michael

>  /* When high resolution timers aren't built-in: we can't use usleep_range() as
> @@ -157,27 +166,29 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
>  static int mscc_miim_reset(struct mii_bus *bus)
>  {
>  	struct mscc_miim_dev *miim = bus->priv;
> -	int offset = miim->phy_reset_offset;
> -	int mask = PHY_CFG_PHY_ENA | PHY_CFG_PHY_COMMON_RESET |
> -		   PHY_CFG_PHY_RESET;
> +	unsigned int offset, mask;
>  	int ret;
>  
> -	if (miim->phy_regs) {
> -		ret = regmap_write(miim->phy_regs, offset, 0);
> -		if (ret < 0) {
> -			WARN_ONCE(1, "mscc reset set error %d\n", ret);
> -			return ret;
> -		}
> +	if (!miim->phy_regs || !miim->info)
> +		return 0;

I would put the check for miim->info in the probe. Not checking the
return value from *_get_match_data() is one of the things the bots
reports and we receive patches for. You have the check, but it is
hidden away, and i doubt the bot nor the bot handlers are clever
enough to find it.

       Andrew

On Sun, Mar 13, 2022 at 02:17:55AM +0100, Michael Walle wrote:
> Am 2022-03-13 01:51, schrieb Andrew Lunn:
> > > diff --git a/drivers/net/mdio/mdio-mscc-miim.c
> > > b/drivers/net/mdio/mdio-mscc-miim.c
> > > index 64fb76c1e395..7773d5019e66 100644
> > > --- a/drivers/net/mdio/mdio-mscc-miim.c
> > > +++ b/drivers/net/mdio/mdio-mscc-miim.c
> > > @@ -158,18 +158,18 @@ static int mscc_miim_reset(struct mii_bus *bus)
> > >  {
> > >  	struct mscc_miim_dev *miim = bus->priv;
> > >  	int offset = miim->phy_reset_offset;
> > > +	int mask = PHY_CFG_PHY_ENA | PHY_CFG_PHY_COMMON_RESET |
> > > +		   PHY_CFG_PHY_RESET;
> > 
> > > -		ret = regmap_write(miim->phy_regs,
> > > -				   MSCC_PHY_REG_PHY_CFG + offset, 0x1ff);
> > > +		ret = regmap_write(miim->phy_regs, offset, mask);
> > 
> > Is mask the correct name? It is not being used in the typical way for
> > a mask.
> 
> It is the mask of all the reset bits, see also patch 3/3. Either all
> these bits are set or none.

Yes, it is you just don't use it in the typical way for a mask

	foo = bar & mask;

The name mask made me look for a read-modify-write or similar. And
that then makes me thing of race conditions.

> Do you haave any suggestion?

value everywhere? Or phy_reset_bits?

      Andrew