Date:   Sun, 4 Dec 2022 21:29:59 +0100
From:   Piergiorgio Beruto <piergiorgio.beruto@...il.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, Oleksij Rempel <o.rempel@...gutronix.de>
Subject: Re: [PATCH net-next 4/4] driver/ncn26000: add PLCA support

On Sun, Dec 04, 2022 at 05:06:50PM +0000, Russell King (Oracle) wrote:
> On Sun, Dec 04, 2022 at 03:32:06AM +0100, Piergiorgio Beruto wrote:
> > +	/* HW bug workaround: the default value of the PLCA TO_TIMER should be
> > +	 * 32, where the current version of NCN26000 reports 24. This will be
> > +	 * fixed in future PHY versions. For the time being, we force the right
> > +	 * default here.
> > +	 */
> > +	ret = phy_write_mmd(phydev,
> > +			    MDIO_MMD_OATC14,
> > +			    MDIO_OATC14_PLCA_TOTMR,
> > +			    TO_TMR_DEFAULT);
> 
> Better formatting please.
> 
> 	return phy_write_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_TOTMR,
> 			     TO_TMR_DEFAULT);
> 
> is sufficient. No need for "ret" (and there are folk who will create a
> cleanup patch to do this, so might as well get it right on submission.)
Ok, I will change the formatting.
On the use of ret, I did that just because I was planning to add more
features to be pre-configured in the future. But for the time being I
can do it as you suggest.
 
> > +/**
> > + * genphy_c45_plca_get_cfg - get PLCA configuration from standard registers
> > + * @phydev: target phy_device struct
> > + * @plca_cfg: output structure to store the PLCA configuration
> > + *
> > + * Description: if the PHY complies to the Open Alliance TC14 10BASE-T1S PLCA
> > + *   Management Registers specifications, this function can be used to retrieve
> > + *   the current PLCA configuration from the standard registers in MMD 31.
> > + */
> > +int genphy_c45_plca_get_cfg(struct phy_device *phydev,
> > +			    struct phy_plca_cfg *plca_cfg)
> > +{
> > +	int ret;
> > +
> > +	ret = phy_read_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_IDVER);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	plca_cfg->version = (u32)ret;
>
> ->version has type s32, so is signed. Clearly, from the above code, it
> can't be negative (since negative integer values are an error.) So why
> is ->version declared in patch 1 as signed? The cast here to u32 also
> seems strange.
> 
> Also, since the register you're reading can be no more than 16 bits
> wide, using s32 seems like a waste.
Please, see the discussion I had with Andrew on this topic.

> > +
> > +	ret = phy_read_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_CTRL0);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	plca_cfg->enabled = !!(((u16)ret) & MDIO_OATC14_PLCA_EN);
> 
> ->enabled has type s16, but it clearly boolean in nature. It could be
> a u8 instead. No need for that u16 cast either.
I'll remove all the casts. I apologize, but in some environments the
coding rules may be different, requiring unnecessary casts for
safety/verification reasons. So I still need to adapt my style to match
the kernel's. Please, have patience with me...

> > +
> > +	// first of all, disable PLCA if required
> > +	if (plca_cfg->enabled == 0) {
> > +		ret = phy_clear_bits_mmd(phydev,
> > +					 MDIO_MMD_OATC14,
> > +					 MDIO_OATC14_PLCA_CTRL0,
> > +					 MDIO_OATC14_PLCA_EN);
> > +
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> 
> Does this need to be disabled when making changes? Just wondering
> why you handle this disable explicitly early.
It is just a way to avoig configuration glitches. If we need to disable
PLCA, it is better to do it before changing any other parameter, so that
you don't disturb the other nodes on the multi-drop network.

> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(genphy_c45_plca_get_status);
> > +
> >  struct phy_driver genphy_c45_driver = {
> >  	.phy_id         = 0xffffffff,
> >  	.phy_id_mask    = 0xffffffff,
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index 2dfb85c6e596..4548c8e8f6a9 100644
> > --- a/include/linux/phy.h
> > +++ b/include/linux/phy.h
> > @@ -811,7 +811,7 @@ struct phy_plca_cfg {
> >   * struct phy_plca_status - Status of the PLCA (Physical Layer Collision
> >   * Avoidance) Reconciliation Sublayer.
> >   *
> > - * @status: The PLCA status as reported by the PST bit in the PLCA STATUS
> > + * @pst: The PLCA status as reported by the PST bit in the PLCA STATUS
> >   *	register(31.CA03), indicating BEACON activity.
> >   *
> >   * A structure containing status information of the PLCA RS configuration.
> > @@ -819,7 +819,7 @@ struct phy_plca_cfg {
> >   * what is actually used.
> >   */
> >  struct phy_plca_status {
> > -	bool status;
> > +	bool pst;
> >  };
> 
> Shouldn't this be in patch 1?
I thought to first promote the changes to the "higher layers" of
ethtool/netlink, then create the appropriate interface towards phylib.
Are you suggesting to merge patches 1 & 2?

> > +
> >  #endif /* _UAPI__LINUX_MDIO_H__ */
> > diff --git a/net/ethtool/plca.c b/net/ethtool/plca.c
> > index 371d8098225e..ab50d8b48bd6 100644
> > --- a/net/ethtool/plca.c
> > +++ b/net/ethtool/plca.c
> > @@ -269,7 +269,7 @@ static int plca_get_status_fill_reply(struct sk_buff *skb,
> >  				      const struct ethnl_reply_data *reply_base)
> >  {
> >  	const struct plca_reply_data *data = PLCA_REPDATA(reply_base);
> > -	const u8 status = data->plca_st.status;
> > +	const u8 status = data->plca_st.pst;
> 
> Shouldn't this be in a different patch?
Ah, logically, yes. I thought since it is an aesthetic change to leave
it there. But if you like I can try to merge it with patch 2.

Thanks,
Piergiorgio

Powered by blists - more mailing lists