All of lore.kernel.org
 help / color / mirror / Atom feed
From: Maxime Chevallier <maxime.chevallier@bootlin.com>
To: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>,
	Antoine Tenart <antoine.tenart@bootlin.com>,
	netdev@vger.kernel.org, gregory.clement@bootlin.com,
	Russell King <linux@armlinux.org.uk>,
	linux-kernel@vger.kernel.org, nadavh@marvell.com,
	thomas.petazzoni@bootlin.com, miquel.raynal@bootlin.com,
	stefanc@marvell.com, mw@semihalf.com, davem@davemloft.net,
	linux-arm-kernel@lists.infradead.org,
	Heiner Kallweit <hkallweit1@gmail.com>
Subject: Re: [PATCH net-next 4/7] net: phy: marvell10g: Add support for 2.5GBASET and 5GBASET
Date: Tue, 22 Jan 2019 11:08:51 +0100	[thread overview]
Message-ID: <20190122110851.35c316d1@bootlin.com> (raw)
In-Reply-To: <20190121201715.GA20277@lunn.ch>

Hello Andrew, Russell,

On Mon, 21 Jan 2019 21:17:15 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

>> @@ -264,8 +265,10 @@ static int mv3310_config_init(struct phy_device *phydev)
>>  	if (ret)
>>  		return ret;
>>  
>> -	linkmode_and(phydev->advertising, phydev->advertising,
>> -		     phydev->supported);
>> +	/* Make sure we advertise all the supported modes, and not just the
>> +	 * default one specified in the driver's .features.
>> +	 */
>> +	linkmode_copy(phydev->advertising, phydev->supported);  
>
>So by doing a copy of supported into advertising, you can stomping
>over any restrictions applied via of_set_phy_supported(),
>of_set_phy_eee_broken(phydev), and any pause control settings which
>might of happened.

Thanks for the explanations, this is indeed clearly not a good solution.

>What might make sense here is that a PHY driver can replace its
>.features member at run time, in its config_init() call. The core then
>needs to perform these evaluations. So i'm guessing we need to split
>this code out of probe() and move it into phy_init_hw()?

So the .features won't be read-only anymore ? We could also simply make
a helper that would add a mode to both the supported and advertising
modes list, that would be used in the 'genphy_c45_pma_read_abilities'
and config_init ?

I lack the big picture of the PHY init sequence, there seems to be a
lot of quirks and complex cases that we need to take into account, so
I'll let you decide :)

>Heiner, you know this code better than anybody. What do you think?
>
>	Andrew

Thanks for the feedback,

Maxime

-- 
Maxime Chevallier, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID (diff)
From: Maxime Chevallier <maxime.chevallier@bootlin.com>
To: Andrew Lunn <andrew@lunn.ch>
Cc: davem@davemloft.net, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Florian Fainelli <f.fainelli@gmail.com>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	linux-arm-kernel@lists.infradead.org,
	Antoine Tenart <antoine.tenart@bootlin.com>,
	thomas.petazzoni@bootlin.com, gregory.clement@bootlin.com,
	miquel.raynal@bootlin.com, nadavh@marvell.com,
	stefanc@marvell.com, mw@semihalf.com
Subject: Re: [PATCH net-next 4/7] net: phy: marvell10g: Add support for 2.5GBASET and 5GBASET
Date: Tue, 22 Jan 2019 11:08:51 +0100	[thread overview]
Message-ID: <20190122110851.35c316d1@bootlin.com> (raw)
In-Reply-To: <20190121201715.GA20277@lunn.ch>

Hello Andrew, Russell,

On Mon, 21 Jan 2019 21:17:15 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

>> @@ -264,8 +265,10 @@ static int mv3310_config_init(struct phy_device *phydev)
>>  	if (ret)
>>  		return ret;
>>  
>> -	linkmode_and(phydev->advertising, phydev->advertising,
>> -		     phydev->supported);
>> +	/* Make sure we advertise all the supported modes, and not just the
>> +	 * default one specified in the driver's .features.
>> +	 */
>> +	linkmode_copy(phydev->advertising, phydev->supported);  
>
>So by doing a copy of supported into advertising, you can stomping
>over any restrictions applied via of_set_phy_supported(),
>of_set_phy_eee_broken(phydev), and any pause control settings which
>might of happened.

Thanks for the explanations, this is indeed clearly not a good solution.

>What might make sense here is that a PHY driver can replace its
>.features member at run time, in its config_init() call. The core then
>needs to perform these evaluations. So i'm guessing we need to split
>this code out of probe() and move it into phy_init_hw()?

So the .features won't be read-only anymore ? We could also simply make
a helper that would add a mode to both the supported and advertising
modes list, that would be used in the 'genphy_c45_pma_read_abilities'
and config_init ?

I lack the big picture of the PHY init sequence, there seems to be a
lot of quirks and complex cases that we need to take into account, so
I'll let you decide :)

>Heiner, you know this code better than anybody. What do you think?
>
>	Andrew

Thanks for the feedback,

Maxime

-- 
Maxime Chevallier, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

  reply	other threads:[~2019-01-22 10:09 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-18 15:23 [PATCH net-next 0/7] net: phy: Add support for 2.5GBASET PHYs Maxime Chevallier
2019-01-18 15:23 ` Maxime Chevallier
2019-01-18 15:23 ` [PATCH net-next 1/7] net: phy: Extract genphy_c45_read_abilities from marvell10g Maxime Chevallier
2019-01-18 15:23   ` Maxime Chevallier
2019-01-20 18:51   ` Andrew Lunn
2019-01-20 18:51     ` Andrew Lunn
2019-01-21 16:20     ` Maxime Chevallier
2019-01-21 16:20       ` Maxime Chevallier
2019-01-21 16:28       ` Andrew Lunn
2019-01-21 16:28         ` Andrew Lunn
2019-01-18 15:23 ` [PATCH net-next 2/7] net: phy: Add generic support for 2.5GBaseT and 5GBaseT Maxime Chevallier
2019-01-18 15:23   ` Maxime Chevallier
2019-01-18 15:23 ` [PATCH net-next 3/7] net: phy: Read 2.5G and 5G extended abilities Maxime Chevallier
2019-01-18 15:23   ` Maxime Chevallier
2019-01-18 15:23 ` [PATCH net-next 4/7] net: phy: marvell10g: Add support for 2.5GBASET and 5GBASET Maxime Chevallier
2019-01-18 15:23   ` Maxime Chevallier
2019-01-18 15:51   ` Russell King - ARM Linux admin
2019-01-18 15:51     ` Russell King - ARM Linux admin
2019-01-21 20:17   ` Andrew Lunn
2019-01-21 20:17     ` Andrew Lunn
2019-01-22 10:08     ` Maxime Chevallier [this message]
2019-01-22 10:08       ` Maxime Chevallier
2019-01-18 15:23 ` [PATCH net-next 5/7] net: phy: marvell10g: Force reading of 2.5/5G PMA extended abilities Maxime Chevallier
2019-01-18 15:23   ` Maxime Chevallier
2019-01-20 19:08   ` Andrew Lunn
2019-01-20 19:08     ` Andrew Lunn
2019-01-21 10:35     ` Maxime Chevallier
2019-01-21 10:35       ` Maxime Chevallier
2019-01-21 10:52       ` Russell King - ARM Linux admin
2019-01-21 10:52         ` Russell King - ARM Linux admin
2019-01-21 12:29         ` Maxime Chevallier
2019-01-21 12:29           ` Maxime Chevallier
2019-01-21 13:00           ` Russell King - ARM Linux admin
2019-01-21 13:00             ` Russell King - ARM Linux admin
2019-01-28 14:26             ` Maxime Chevallier
2019-01-28 14:26               ` Maxime Chevallier
2019-02-07 23:37               ` Russell King - ARM Linux admin
2019-02-07 23:37                 ` Russell King - ARM Linux admin
2019-01-18 15:23 ` [PATCH net-next 6/7] net: mvpp2: Add 2.5GBaseT support Maxime Chevallier
2019-01-18 15:23   ` Maxime Chevallier
2019-01-18 15:23 ` [PATCH net-next 7/7] net: phy: marvell10g: add support for the 88x2110 PHY Maxime Chevallier
2019-01-18 15:23   ` Maxime Chevallier
2019-01-20 19:10   ` Andrew Lunn
2019-01-20 19:10     ` Andrew Lunn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190122110851.35c316d1@bootlin.com \
    --to=maxime.chevallier@bootlin.com \
    --cc=andrew@lunn.ch \
    --cc=antoine.tenart@bootlin.com \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=gregory.clement@bootlin.com \
    --cc=hkallweit1@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=miquel.raynal@bootlin.com \
    --cc=mw@semihalf.com \
    --cc=nadavh@marvell.com \
    --cc=netdev@vger.kernel.org \
    --cc=stefanc@marvell.com \
    --cc=thomas.petazzoni@bootlin.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.