LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Trent Piepho <tpiepho@impinj.com>
To: "andrew@lunn.ch" <andrew@lunn.ch>
Cc: "hkallweit1@gmail.com" <hkallweit1@gmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"f.fainelli@gmail.com" <f.fainelli@gmail.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Subject: Re: [PATCH 5/5] net: phy: dp83867: Use unsigned variables to store unsigned properties
Date: Tue, 14 May 2019 15:37:05 +0000	[thread overview]
Message-ID: <1557848224.4229.48.camel@impinj.com> (raw)
In-Reply-To: <20190513214332.GB12345@lunn.ch>

On Mon, 2019-05-13 at 23:43 +0200, Andrew Lunn wrote:
> > > 
> > > Are there any in tree users of DP83867_CLK_O_SEL_REF_CLK? We have to
> > > be careful changing its meaning. But if nobody is actually using it...
> > 
> > Nope.  I doubt this will affect anyone.  They'd need to strap the phy
> > to get a different configuration, and the explicitly add a property,
> > which isn't in the example DTS files, to change the configuration to
> > something they didn't want, and then depend on a driver bug ignoring
> > the erroneous setting they added.
> 
> O.K, then this patch is O.K. Does the binding documentation need
> updating?

Device tree binding patch was split out of the commit and was patch 2
of the series, https://patchwork.ozlabs.org/patch/1098349/

> > > Patch 4:
> > > 
> > > This is harder. Ideally we want to fix this. At some point, somebody
> > > is going to want 'rgmii' to actually mean 'rgmii', because that is
> > > what their hardware needs.
> > > 
> > > Could you add a WARN_ON() for 'rgmii' but the PHY is actually adding a
> > > delay? And add a comment about setting the correct thing in device
> > > tree?  Hopefully we will then get patches correcting DT blobs. And if
> > > we later do need to fix 'rgmii', we will break less board.
> > 
> > Yes I can do this.  Should it warn on any use of "rgmii"?
> 
> No, i would only warn when there is a delay configured by
> strapping. If you want the PHY to be left alone, you should use
> PHY_INTERFACE_MODE_NA, which should be the default if there is no
> phy-mode property. If DT actually asked for "rgmii", it either means
> it is wrong and rgmii-id should be used to match the strapping, or
> both the strapping and the DT is wrong and somebody really does want
> "rgmii".

Ok, seems reasonable.  I've put in a phydev_warn() when the interface
is 'rgmii' and the strapping is set to have a delay.  I'm checking the
strapping config register for this, rather than the current phy
configuration of delay values.  The previous behavior of 'rgmii' mode
was "keep strapping default", rather than "keep current phy
configuration", which isn't exactly the same thing.

Here is the message:

phydev_warn(phydev,
            "PHY has delays via pin strapping, but phy-mode = 'rgmii'\n"
            "Should be 'rgmii-id' to use internal delays\n");   


      reply	other threads:[~2019-05-14 15:37 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-10 21:46 [PATCH 1/5] dt-bindings: phy: dp83867: Describe how driver behaves w.r.t rgmii delay Trent Piepho
2019-05-10 21:46 ` [PATCH 2/5] dt-bindings: phy: dp83867: Add documentation for disabling clock output Trent Piepho
2019-05-10 21:46 ` [PATCH 3/5] net: phy: dp83867: Add ability to disable output clock Trent Piepho
2019-05-10 21:46 ` [PATCH 4/5] net: phy: dp83867: Disable tx/rx delay when not configured Trent Piepho
2019-05-10 21:46 ` [PATCH 5/5] net: phy: dp83867: Use unsigned variables to store unsigned properties Trent Piepho
2019-05-11 10:41   ` Heiner Kallweit
2019-05-11 12:32     ` Heiner Kallweit
2019-05-13 19:58       ` Trent Piepho
2019-05-13 20:12         ` Heiner Kallweit
2019-05-13 20:46           ` Andrew Lunn
2019-05-13 21:26             ` Trent Piepho
2019-05-13 21:43               ` Andrew Lunn
2019-05-14 15:37                 ` Trent Piepho [this message]

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=1557848224.4229.48.camel@impinj.com \
    --to=tpiepho@impinj.com \
    --cc=andrew@lunn.ch \
    --cc=devicetree@vger.kernel.org \
    --cc=f.fainelli@gmail.com \
    --cc=hkallweit1@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --subject='Re: [PATCH 5/5] net: phy: dp83867: Use unsigned variables to store unsigned properties' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).