Linux-Devicetree Archive on lore.kernel.org
 help / color / Atom feed
From: Andrew Lunn <andrew@lunn.ch>
To: Dan Murphy <dmurphy@ti.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>,
	hkallweit1@gmail.com, davem@davemloft.net, robh@kernel.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org
Subject: Re: [PATCH net-next v2 4/4] net: dp83869: Add RGMII internal delay configuration
Date: Sun, 24 May 2020 00:07:41 +0200
Message-ID: <20200523220741.GO610998@lunn.ch> (raw)
In-Reply-To: <a59412a5-7cc6-dc70-b851-c7d65c1635b7@ti.com>

> > Any why is your PHY special, in that is does care about out of range
> > delays, when others using new the new core helper don't?
> 
> We are not rounding to nearest here.  Basically the helper works to find the
> best match
> 
> If the delay passed in is less than or equal to the smallest delay then
> return the smallest delay index
> 
> If the delay passed in is greater then the largest delay then return the max
> delay index

+               /* Find an approximate index by looking up the table */
+               if (delay > delay_values[i - 1] &&
+                   delay < delay_values[i]) {
+                       if (delay - delay_values[i - 1] < delay_values[i] - delay)
+                               return i - 1;
+                       else
+                               return i;

This appears to round to the nearest value when it is not an exact
match.

The documentation is a hint to the DT developer what value to put in
DT. By saying it rounders, the developer does not need to go digging
through the source code to find an exact value, otherwise -EINVAL will
be returned. They can just use the value the HW engineer suggested,
and the PHY will pick whatever is nearest.

> Not sure what you mean about this PHY being special.  This helper is
> not PHY specific.

As you said, if out of range, the helper returns the top/bottom
value. Your PHY is special, the top/bottom value is not good enough,
you throw an error.

The point of helpers is to give uniform behaviour. We have one line
helpers, simply because they give uniform behaviour, rather than have
each driver do it subtlety different. But it also means drivers should
try to not add additional constraints over what the helper already
has, unless it is actually required by the hardware.

> After I think about this more I am thinking a helper may be over kill here
> and the delay to setting should be done within the PHY driver itself

The helper is useful, it will result in uniform handling of rounding
between DT values and what the PHY can actually do. But please also
move your range check and error message inside the helper.

     Andrew

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-22 12:25 [PATCH net-next v2 0/4] RGMII Internal delay common property Dan Murphy
2020-05-22 12:25 ` [PATCH net-next v2 1/4] dt-bindings: net: Add tx and rx internal delays Dan Murphy
2020-05-23 15:13   ` Andrew Lunn
2020-05-22 12:25 ` [PATCH net-next v2 2/4] net: phy: Add a helper to return the index for of the internal delay Dan Murphy
2020-05-22 16:11   ` Florian Fainelli
2020-05-22 18:27     ` Dan Murphy
2020-05-22 12:25 ` [PATCH net-next v2 3/4] dt-bindings: net: Add RGMII internal delay for DP83869 Dan Murphy
2020-05-22 12:25 ` [PATCH net-next v2 4/4] net: dp83869: Add RGMII internal delay configuration Dan Murphy
2020-05-22 16:13   ` Florian Fainelli
2020-05-22 18:50     ` Dan Murphy
2020-05-23 15:09       ` Andrew Lunn
2020-05-23 21:40         ` Dan Murphy
2020-05-23 22:07           ` Andrew Lunn [this message]
2020-05-26 17:48             ` Dan Murphy
  -- strict thread matches above, loose matches on Subject: below --
2020-05-20 12:18 [PATCH net-next v2 0/4] DP83869 Enhancements Dan Murphy
2020-05-20 12:18 ` [PATCH net-next v2 4/4] net: dp83869: Add RGMII internal delay configuration Dan Murphy

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=20200523220741.GO610998@lunn.ch \
    --to=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=dmurphy@ti.com \
    --cc=f.fainelli@gmail.com \
    --cc=hkallweit1@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=robh@kernel.org \
    /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

Linux-Devicetree Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-devicetree/0 linux-devicetree/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-devicetree linux-devicetree/ https://lore.kernel.org/linux-devicetree \
		devicetree@vger.kernel.org
	public-inbox-index linux-devicetree

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-devicetree


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git