linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Michael Walle <michael@walle.cc>
To: Florian Fainelli <f.fainelli@gmail.com>,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	netdev@vger.kernel.org, Andrew Lunn <andrew@lunn.ch>,
	Heiner Kallweit <hkallweit1@gmail.com>
Subject: Re: [RFC PATCH 3/3] net: phy: at803x: add device tree binding
Date: Thu, 31 Oct 2019 00:59:49 +0100	[thread overview]
Message-ID: <B3B13FB8-42D9-42F9-8106-536F574FA35B@walle.cc> (raw)
In-Reply-To: <754a493b-a557-c369-96e1-6701ba5d5a30@gmail.com>

Am 31. Oktober 2019 00:28:02 MEZ schrieb Florian Fainelli <f.fainelli@gmail.com>:
>On 10/30/19 3:42 PM, Michael Walle wrote:
>> Add support for configuring the CLK_25M pin as well as the RGMII I/O
>> voltage by the device tree.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  drivers/net/phy/at803x.c | 156
>++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 154 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
>> index 1eb5d4fb8925..32be4c72cf4b 100644
>> --- a/drivers/net/phy/at803x.c
>> +++ b/drivers/net/phy/at803x.c
>> @@ -13,7 +13,9 @@
>>  #include <linux/netdevice.h>
>>  #include <linux/etherdevice.h>
>>  #include <linux/of_gpio.h>
>> +#include <linux/bitfield.h>
>>  #include <linux/gpio/consumer.h>
>> +#include <dt-bindings/net/atheros-at803x.h>
>>  
>>  #define AT803X_SPECIFIC_STATUS			0x11
>>  #define AT803X_SS_SPEED_MASK			(3 << 14)
>> @@ -62,6 +64,37 @@
>>  #define AT803X_DEBUG_REG_5			0x05
>>  #define AT803X_DEBUG_TX_CLK_DLY_EN		BIT(8)
>>  
>> +#define AT803X_DEBUG_REG_1F			0x1F
>> +#define AT803X_DEBUG_PLL_ON			BIT(2)
>> +#define AT803X_DEBUG_RGMII_1V8			BIT(3)
>> +
>> +/* AT803x supports either the XTAL input pad, an internal PLL or the
>> + * DSP as clock reference for the clock output pad. The XTAL
>reference
>> + * is only used for 25 MHz output, all other frequencies need the
>PLL.
>> + * The DSP as a clock reference is used in synchronous ethernet
>> + * applications.
>
>How does that tie in the mode in which the PHY is configured? In
>reverse
>MII mode, the PHY provides the TX clock which can be either 25Mhz or
>50Mhz AFAIR, in RGMII mode, the TXC provided by the MAC is internally
>resynchronized and then fed back to the MAC as a 125Mhz clock.
>
>Do you possibly need to cross check the clock output selection with the
>PHY interface?

what do you mean by mode? the "clock output pad" (maybe the term is wrong) is just an additional clock output. And I've ignored syncE mode for now. I don't think there is a real use case for now. because in almost all cases the clock out is used to generate 125MHz required by an RGMII core in the SoC. 


>
>[snip]
>> +static int at803x_parse_dt(struct phy_device *phydev)
>> +{
>> +	struct device_node *node = phydev->mdio.dev.of_node;
>> +	struct at803x_priv *priv = phydev->priv;
>> +	u32 freq, strength;
>> +	unsigned int sel;
>> +	int ret;
>> +
>> +	if (!IS_ENABLED(CONFIG_OF_MDIO))
>> +		return 0;
>> +
>> +	if (!node)
>> +		return 0;
>
>I don't think you need either of those two things, every of_* function
>would check whether the node reference is non-NULL.

The first is needed because otherwise the of_* return -ENOSYS IIRC. I guess it would make no difference here though. Although I don't know how clever the compiler is as it could optimize the whole function away if CONFIG_OF_MDIO is not enabled. 

>> +
>> +	if (of_property_read_bool(node, "atheros,keep-pll-enabled"))
>> +		priv->flags |= AT803X_KEEP_PLL_ENABLED;
>
>This should probably be a PHY tunable rather than a Device Tree
>property
>as this delves more into the policy than the pure hardware description.

To be frank. I'll first need to look into PHY tunables before answering ;) 
But keep in mind that this clock output might be used anywhere on the board. It must not have something to do with networking. The PHY has a crystal and it can generate these couple of frequencies regardless of its network operation. 

>> +
>> +	if (of_property_read_bool(node, "atheros,rgmii-io-1v8"))
>> +		priv->flags |= AT803X_RGMII_1V8;> +
>> +	ret = of_property_read_u32(node, "atheros,clk-out-frequency",
>&freq);
>> +	if (!ret) {
>> +		switch (freq) {
>> +		case 25000000:
>> +			sel = AT803X_CLK_OUT_25MHZ_XTAL;
>> +			break;
>> +		case 50000000:
>> +			sel = AT803X_CLK_OUT_50MHZ_PLL;
>> +			break;
>> +		case 62500000:
>> +			sel = AT803X_CLK_OUT_62_5MHZ_PLL;
>> +			break;
>> +		case 125000000:
>> +			sel = AT803X_CLK_OUT_125MHZ_PLL;
>> +			break;
>> +		default:
>> +			phydev_err(phydev,
>> +				   "invalid atheros,clk-out-frequency\n");
>> +			return -EINVAL;
>> +		}
>
>Maybe the PHY should be a clock provider of some sort, this might be
>especially important if the PHY supplies the Ethernet MAC's RXC (which
>would be the case in a RGMII configuration).

Could be the case, I don't know. I'm developing this on a NXP layerscape LS1028A and this SoC needs a fixed 125MHz clock for its RGMII interface (regardless if its 10/100 or 100 Mbit/s). I guess the same is true for the i.MX series. 

-michael 


  reply	other threads:[~2019-10-30 23:59 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-30 22:42 [RFC PATCH 0/3] net: phy: at803x device tree binding Michael Walle
2019-10-30 22:42 ` [RFC PATCH 1/3] net: phy: at803x: fix Kconfig description Michael Walle
2019-10-30 23:13   ` Andrew Lunn
2019-10-30 23:16   ` Florian Fainelli
2019-10-30 23:18     ` Andrew Lunn
2019-10-30 23:32       ` Florian Fainelli
2019-10-31  0:05         ` Michael Walle
2019-10-30 22:42 ` [RFC PATCH 2/3] dt-bindings: net: phy: Add support for AT803X Michael Walle
2019-10-30 23:17   ` Andrew Lunn
2019-10-31  0:14     ` Michael Walle
2019-10-31 16:45       ` Florian Fainelli
2019-10-31 17:14         ` Michael Walle
2019-10-30 23:28   ` Florian Fainelli
2019-10-30 23:36     ` Michael Walle
2019-11-01 15:03   ` Simon Horman
2019-11-02  1:19     ` Michael Walle
2019-10-30 22:42 ` [RFC PATCH 3/3] net: phy: at803x: add device tree binding Michael Walle
2019-10-30 23:21   ` Andrew Lunn
2019-10-30 23:28   ` Florian Fainelli
2019-10-30 23:59     ` Michael Walle [this message]
2019-10-31 17:22       ` Michael Walle
2019-10-31 17:35         ` Florian Fainelli
2019-11-02  1:18           ` Michael Walle

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=B3B13FB8-42D9-42F9-8106-536F574FA35B@walle.cc \
    --to=michael@walle.cc \
    --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 \
    /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 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).