Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Sean Anderson <sean.anderson@seco.com>
Subject: Re: [RFC net-next PATCH 10/16] net: macb: Move PCS settings to PCS
 callbacks
To:     "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc:     netdev@vger.kernel.org, "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, linux-kernel@vger.kernel.org,
        Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Claudiu Beznea <claudiu.beznea@microchip.com>,
        Nicolas Ferre <nicolas.ferre@microchip.com>
References: <20211004191527.1610759-1-sean.anderson@seco.com>
 <20211004191527.1610759-11-sean.anderson@seco.com>
 <YVwjjghGcXaEYgY+@shell.armlinux.org.uk>
Message-ID: <7c92218c-baec-a991-9d6b-af42dfabbad3@seco.com>
Date:   Tue, 5 Oct 2021 12:03:50 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
In-Reply-To: <YVwjjghGcXaEYgY+@shell.armlinux.org.uk>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NXZBdjdGbTdJdjdlcExEd1c2UWNwZFQwclhWZWxVcXdQdEp1U2lIRXE0ODNR?=
 =?utf-8?B?K1BhZ2lES1QzTHpqaW00d2p3bTdUcEw2bnU2d0E2MGE2M2VyMWsweTNSYktL?=
 =?utf-8?B?V1dTWE4xMFJOK29adjluSm9mQjg3S1RKVHYvcEFnVFV5NWJqRCtseHVydkxF?=
 =?utf-8?B?QVpVRXBIL2RJdWVHK1I1VHhycVZINk1PaXpxYlIxZTdDY0VOeHVOQ2R2MUdr?=
 =?utf-8?B?b0cydjRibFZFVTJtK0F2bitQSDF4WGJ0LytvcXVQbFdtWWxxSmpiZ0lMQThm?=
 =?utf-8?B?WHdIbTFYQW1MTkszUG0ySjdzbElLSVZmeDNpTWlaRDhPV2lkNVVuRElrWUlE?=
 =?utf-8?B?a2dzK0dwZ1BaYiswL1E5MFdobU9UL1k2V2toWTZFMDNCVytJVVpkY1YyOUFj?=
 =?utf-8?B?NzFpbmVUYng1Z2wvQktiTDBJYnFWVURYdjB4aGZKRlhEajRrZ0dUcFJ3M0hm?=
 =?utf-8?B?NFBLRjhRSE5SckNscXJ5SWhEalcwZDVVTnpURFh5RzNNVkJSakJ1c3lkV3dO?=
 =?utf-8?B?T01jQTI0TUhLLzZwTC8xeEh3L08zZkN0aWJxdkVRTVRwS0ZDTGlGa1M4a05G?=
 =?utf-8?B?b3RqY3lnQkZUd2RxVUtySW1zOFlOVDBETStnWExFdG9JN2Z5QmpYUVJyb1cx?=
 =?utf-8?B?TWhwbnpGNWM3ajZ5di9Wa1Fhc05MMm5FZm9MMU1sdUtURWRaUWJRb1phZ0V5?=
 =?utf-8?B?VzhLcSt2S1R5ejZUN1VPek90VUlJUnFzQ0RLTlYyditkOWlRWmhNbVpldGxK?=
 =?utf-8?B?QnkrdUczdDFHWVNINU9ha2V0cW16bGR1VTlVNVVQcVk3ZnhNeWs1STFHTFc3?=
 =?utf-8?B?eEZHZ2hsajNzMUplZ3dxL3VMR1o3aUc3eTJSYWFkdXpJd3lWb2hrZXY4Qms3?=
 =?utf-8?B?N1BIdUFNdU5SeFpua2ZtSDgzT3c4dFBaZEk5czA0T3pFdGFFK3d4MTc1Mjc5?=
 =?utf-8?B?Witic210ODQrUGJjVDVMRkl2eFBGQ2RQVjN5eDRkTjc1cU8xaVNubkF6ODh6?=
 =?utf-8?B?elF2NTgzQ2RRNyttKzIwYlBTZVVoTTdrM1VhRXhuWjdvbnZtZU1lN0lCMCth?=
 =?utf-8?B?RGprdHJieUdMNDVESUZxOFdCOXE4YjBNUXhXRk9jZHEvMEMrR1ZIaG1Db1I4?=
 =?utf-8?B?SWdLYldvS0diM0IzQ3J2L0N3ZVE5LzJkWGlKajJlRXBRRytyenZkV0FyZ1hW?=
 =?utf-8?B?N1FpeTgyb250WlQyWHRDWVFmZEpaUzlzbFFyZXFnanBZQWl1SitCMTNKbkFS?=
 =?utf-8?B?UkI1ZUtXemMxZ1ZsYkpQMVRJWnhrTDVtWGZYK0UvM2RpNmRiNWhZWHFzQWZQ?=
 =?utf-8?B?ZVErNUp3VUtEOGxmZDhuY3N0V1FMcHRmNlBsdSt1WWRNZDZKN0wzSWxjZHFR?=
 =?utf-8?B?Qi9UMHk1QXhhWkxZU2xTTm5pQ2ZPdjVOZ0podFJQL3RQWXVmTEtzMDM0aUtl?=
 =?utf-8?B?dEZTWkJ4S1V4U1g1MUsrSUxtL0t3Qi9zS1g1ZVFVV2pqUVVqYVFhZmh3QVZJ?=
 =?utf-8?B?M0tDMTI3b2NPNUhBOXZIQzIyR2x2aXJZSFZpc3VSeVMxVitPdmJweC9jNytv?=
 =?utf-8?B?cHdEbDRvYWNVRTRzc2FzdDJBcVdJcVpGRU05eE1QM0RFQ1ZzSzBuNUdCN1pm?=
 =?utf-8?B?ZnJLM2JqeHZHYnBjZUZvUG5FTVd6TFBWYXAwc3doNmYwZEVYTU9KekVYcUV3?=
 =?utf-8?B?NDRkeHIwN05UMkZKcEFiaDFnYmhiNzNjSWNtTFAzV2N0RjlQaWxNMWF0UFlX?=
 =?utf-8?Q?Oq0ok/HNmIdX6KoFNTp/aunNDBkKbt1SYuoo9mN?=
X-OriginatorOrg: seco.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6b35d93c-af73-4c5b-a495-08d98819bb4f
X-MS-Exchange-CrossTenant-AuthSource: DB7PR03MB4523.eurprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Oct 2021 16:03:55.1323
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: bebe97c3-6438-442e-ade3-ff17aa50e733
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 7C9mw9u4Pr/3Q+avf8Sb75NcV2IBISiM8xP55Gq/Hd6Hpr/6K2Aak+xBG3GEZKxJ71QvI10dxmIePO3b0ONukQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB9PR03MB7514
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Russell,

On 10/5/21 6:06 AM, Russell King (Oracle) wrote:
> On Mon, Oct 04, 2021 at 03:15:21PM -0400, Sean Anderson wrote:
>> +static void macb_pcs_get_state(struct phylink_pcs *pcs,
>> +			       struct phylink_link_state *state)
>> +{
>> +	struct macb *bp = pcs_to_macb(pcs);
>> +
>> +	if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
>> +		state->interface = PHY_INTERFACE_MODE_SGMII;
>> +	else
>> +		state->interface = PHY_INTERFACE_MODE_1000BASEX;
>
> There is no requirement to set state->interface here. Phylink doesn't
> cater for interface changes when reading the state. As documented,
> phylink will set state->interface already before calling this function
> to indicate what interface mode it is currently expecting from the
> hardware.

Ok, so instead I should be doing something like

if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
	interface = PHY_INTERFACE_MODE_SGMII;
else
	interface = PHY_INTERFACE_MODE_1000BASEX;

if (interface != state->interface) {
	state->link = 0;
	return;
}

?

>> +static int macb_pcs_config_an(struct macb *bp, unsigned int mode,
>> +			      phy_interface_t interface,
>> +			      const unsigned long *advertising)
>> +{
>> +	bool changed = false;
>> +	u16 old, new;
>> +
>> +	old = gem_readl(bp, PCSANADV);
>> +	new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising,
>> +						       old);
>> +	if (old != new) {
>> +		changed = true;
>> +		gem_writel(bp, PCSANADV, new);
>> +	}
>> +
>> +	old = new = gem_readl(bp, PCSCNTRL);
>> +	if (mode == MLO_AN_INBAND)
>
> Please use phylink_autoneg_inband(mode) here.

Ok.

>
>> +		new |= BMCR_ANENABLE;
>> +	else
>> +		new &= ~BMCR_ANENABLE;
>> +	if (old != new) {
>> +		changed = true;
>> +		gem_writel(bp, PCSCNTRL, new);
>> +	}
>
> There has been the suggestion that we should allow in-band AN to be
> disabled in 1000base-X if we're in in-band mode according to the
> ethtool state.

This logic is taken from phylink_mii_c22_pcs_config. Maybe I should add
another _encode variant? I hadn't done this here because the logic was
only one if statement.

> I have a patch that adds that.

Have you posted it?

>> +	return changed;
>> +}
>> +
>> +static int macb_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
>> +			   phy_interface_t interface,
>> +			   const unsigned long *advertising,
>> +			   bool permit_pause_to_mac)
>> +{
>> +	bool changed = false;
>> +	struct macb *bp = pcs_to_macb(pcs);
>> +	u16 old, new;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&bp->lock, flags);
>> +	old = new = gem_readl(bp, NCFGR);
>> +	if (interface == PHY_INTERFACE_MODE_SGMII) {
>> +		new |= GEM_BIT(SGMIIEN);
>> +	} else if (interface == PHY_INTERFACE_MODE_1000BASEX) {
>> +		new &= ~GEM_BIT(SGMIIEN);
>> +	} else {
>> +		spin_lock_irqsave(&bp->lock, flags);
>> +		return -EOPNOTSUPP;
>
> You can't actually abort at this point - phylink will print the error
> and carry on regardless. The checking is all done via the validate()
> callback and if that indicates the interface mode is acceptable, then
> it should be accepted.

Ok, so where can the PCS NAK an interface? This is the only callback
which has a return code, so I assumed this was the correct place to say
"no, we don't support this." This is what lynx_pcs_config does as well.

>>  static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = {
>>  	.pcs_get_state = macb_usx_pcs_get_state,
>>  	.pcs_config = macb_usx_pcs_config,
>> -	.pcs_link_up = macb_usx_pcs_link_up,
>>  };
>>
>>  static const struct phylink_pcs_ops macb_phylink_pcs_ops = {
>>  	.pcs_get_state = macb_pcs_get_state,
>> -	.pcs_an_restart = macb_pcs_an_restart,
>
> You don't want to remove this.

Hm, I didn't realized I had removed this, so I add it back in the next
patch. I will merge that one into this one.


> When operating in 1000BASE-X mode, it
> will be called if a restart is required (e.g. macb_pcs_config()
> returning positive, or an ethtool request.) You need to keep the empty
> function.

Ok, perhaps I can add some sanity checks for this in pcs_register.

--Sean

> That may also help the diff algorithm to produce a cleaner
> patch too.