From: Heiner Kallweit <hkallweit1@gmail.com>
To: Jernej Skrabec <jernej.skrabec@siol.net>,
robh+dt@kernel.org, andrew@lunn.ch, f.fainelli@gmail.com
Cc: mripard@kernel.org, wens@csie.org, lee.jones@linaro.org,
linux@armlinux.org.uk, davem@davemloft.net,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [RFC PATCH 2/4] net: phy: Add support for AC200 EPHY
Date: Thu, 16 Apr 2020 22:18:52 +0200 [thread overview]
Message-ID: <0340f85c-987f-900b-53c8-d29b4672a8fa@gmail.com> (raw)
In-Reply-To: <20200416185758.1388148-3-jernej.skrabec@siol.net>
On 16.04.2020 20:57, Jernej Skrabec wrote:
> AC200 MFD IC supports Fast Ethernet PHY. Add a driver for it.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
> drivers/net/phy/Kconfig | 7 ++
> drivers/net/phy/Makefile | 1 +
> drivers/net/phy/ac200.c | 206 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 214 insertions(+)
> create mode 100644 drivers/net/phy/ac200.c
>
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 3fa33d27eeba..16af69f69eaf 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -288,6 +288,13 @@ config ADIN_PHY
> - ADIN1300 - Robust,Industrial, Low Latency 10/100/1000 Gigabit
> Ethernet PHY
>
> +config AC200_PHY
> + tristate "AC200 EPHY"
> + depends on NVMEM
> + depends on OF
> + help
> + Fast ethernet PHY as found in X-Powers AC200 multi-function device.
> +
> config AMD_PHY
> tristate "AMD PHYs"
> ---help---
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index 2f5c7093a65b..b0c5b91900fa 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_SFP) += sfp.o
> sfp-obj-$(CONFIG_SFP) += sfp-bus.o
> obj-y += $(sfp-obj-y) $(sfp-obj-m)
>
> +obj-$(CONFIG_AC200_PHY) += ac200.o
> obj-$(CONFIG_ADIN_PHY) += adin.o
> obj-$(CONFIG_AMD_PHY) += amd.o
> aquantia-objs += aquantia_main.o
> diff --git a/drivers/net/phy/ac200.c b/drivers/net/phy/ac200.c
> new file mode 100644
> index 000000000000..3d7856ff8f91
> --- /dev/null
> +++ b/drivers/net/phy/ac200.c
> @@ -0,0 +1,206 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/**
> + * Driver for AC200 Ethernet PHY
> + *
> + * Copyright (c) 2020 Jernej Skrabec <jernej.skrabec@siol.net>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mfd/ac200.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of.h>
> +#include <linux/phy.h>
> +#include <linux/platform_device.h>
> +
> +#define AC200_EPHY_ID 0x00441400
> +#define AC200_EPHY_ID_MASK 0x0ffffff0
> +
You could use PHY_ID_MATCH_MODEL() here.
> +/* macros for system ephy control 0 register */
> +#define AC200_EPHY_RESET_INVALID BIT(0)
> +#define AC200_EPHY_SYSCLK_GATING BIT(1)
> +
> +/* macros for system ephy control 1 register */
> +#define AC200_EPHY_E_EPHY_MII_IO_EN BIT(0)
> +#define AC200_EPHY_E_LNK_LED_IO_EN BIT(1)
> +#define AC200_EPHY_E_SPD_LED_IO_EN BIT(2)
> +#define AC200_EPHY_E_DPX_LED_IO_EN BIT(3)
> +
> +/* macros for ephy control register */
> +#define AC200_EPHY_SHUTDOWN BIT(0)
> +#define AC200_EPHY_LED_POL BIT(1)
> +#define AC200_EPHY_CLK_SEL BIT(2)
> +#define AC200_EPHY_ADDR(x) (((x) & 0x1F) << 4)
> +#define AC200_EPHY_XMII_SEL BIT(11)
> +#define AC200_EPHY_CALIB(x) (((x) & 0xF) << 12)
> +
> +struct ac200_ephy_dev {
> + struct phy_driver *ephy;
Why embedding a pointer and not a struct phy_driver directly?
Then you could omit the separate allocation.
ephy is not the best naming. It may be read as a phy_device.
Better use phydrv.
> + struct regmap *regmap;
> +};
> +
> +static char *ac200_phy_name = "AC200 EPHY";
> +
Why not using the name directly in the assignment?
And better naming: "AC200 Fast Ethernet"
> +static int ac200_ephy_config_init(struct phy_device *phydev)
> +{
> + const struct ac200_ephy_dev *priv = phydev->drv->driver_data;
> + unsigned int value;
> + int ret;
> +
> + phy_write(phydev, 0x1f, 0x0100); /* Switch to Page 1 */
> + phy_write(phydev, 0x12, 0x4824); /* Disable APS */
> +
> + phy_write(phydev, 0x1f, 0x0200); /* Switch to Page 2 */
> + phy_write(phydev, 0x18, 0x0000); /* PHYAFE TRX optimization */
> +
> + phy_write(phydev, 0x1f, 0x0600); /* Switch to Page 6 */
> + phy_write(phydev, 0x14, 0x708f); /* PHYAFE TX optimization */
> + phy_write(phydev, 0x13, 0xF000); /* PHYAFE RX optimization */
> + phy_write(phydev, 0x15, 0x1530);
> +
> + phy_write(phydev, 0x1f, 0x0800); /* Switch to Page 6 */
> + phy_write(phydev, 0x18, 0x00bc); /* PHYAFE TRX optimization */
> +
> + phy_write(phydev, 0x1f, 0x0100); /* switch to page 1 */
> + phy_clear_bits(phydev, 0x17, BIT(3)); /* disable intelligent IEEE */
> +
> + /* next two blocks disable 802.3az IEEE */
> + phy_write(phydev, 0x1f, 0x0200); /* switch to page 2 */
> + phy_write(phydev, 0x18, 0x0000);
> +
> + phy_write(phydev, 0x1f, 0x0000); /* switch to page 0 */
> + phy_clear_bits_mmd(phydev, 0x7, 0x3c, BIT(1));
Better use the following:
phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, 0x0000);
It makes clear that you disable advertising EEE completely.
> +
> + if (phydev->interface == PHY_INTERFACE_MODE_RMII)
> + value = AC200_EPHY_XMII_SEL;
> + else
> + value = 0;
> +
> + ret = regmap_update_bits(priv->regmap, AC200_EPHY_CTL,
> + AC200_EPHY_XMII_SEL, value);
> + if (ret)
> + return ret;
> +
I had a brief look at the spec, and it's not fully clear
to me what this register setting does. Does it affect the
MAC side and/or the PHY side?
If it affects the PHY side, then I'd expect that the chip
has to talk to the PHY via the MDIO bus. Means there should
be a PHY register for setting MII vs. RMII.
In this case the setup could be very much simplified.
Then the PHY driver wouldn't have to be embedded in the
platform driver.
> + /* FIXME: This is H6 specific */
> + phy_set_bits(phydev, 0x13, BIT(12));
> +
This seems to indicate that the same PHY is used in a slightly
different version with other Hx models. Do they use different
PHY ID's?
> + return 0;
> +}
> +
> +static int ac200_ephy_probe(struct platform_device *pdev)
> +{
> + struct ac200_dev *ac200 = dev_get_drvdata(pdev->dev.parent);
> + struct device *dev = &pdev->dev;
> + struct ac200_ephy_dev *priv;
> + struct nvmem_cell *calcell;
> + struct phy_driver *ephy;
> + u16 *caldata, calib;
> + size_t callen;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + ephy = devm_kzalloc(dev, sizeof(*ephy), GFP_KERNEL);
> + if (!ephy)
> + return -ENOMEM;
> +
> + calcell = devm_nvmem_cell_get(dev, "calibration");
> + if (IS_ERR(calcell)) {
> + dev_err(dev, "Unable to find calibration data!\n");
> + return PTR_ERR(calcell);
> + }
> +
> + caldata = nvmem_cell_read(calcell, &callen);
> + if (IS_ERR(caldata)) {
> + dev_err(dev, "Unable to read calibration data!\n");
> + return PTR_ERR(caldata);
> + }
> +
> + if (callen != 2) {
> + dev_err(dev, "Calibration data has wrong length: 2 != %zu\n",
> + callen);
> + kfree(caldata);
> + return -EINVAL;
> + }
> +
> + calib = *caldata + 3;
> + kfree(caldata);
> +
> + ephy->phy_id = AC200_EPHY_ID;
> + ephy->phy_id_mask = AC200_EPHY_ID_MASK;
> + ephy->name = ac200_phy_name;
> + ephy->driver_data = priv;
> + ephy->soft_reset = genphy_soft_reset;
> + ephy->config_init = ac200_ephy_config_init;
> + ephy->suspend = genphy_suspend;
> + ephy->resume = genphy_resume;
> +
> + priv->ephy = ephy;
> + priv->regmap = ac200->regmap;
> + platform_set_drvdata(pdev, priv);
> +
> + ret = regmap_write(ac200->regmap, AC200_SYS_EPHY_CTL0,
> + AC200_EPHY_RESET_INVALID |
> + AC200_EPHY_SYSCLK_GATING);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(ac200->regmap, AC200_SYS_EPHY_CTL1,
> + AC200_EPHY_E_EPHY_MII_IO_EN |
> + AC200_EPHY_E_LNK_LED_IO_EN |
> + AC200_EPHY_E_SPD_LED_IO_EN |
> + AC200_EPHY_E_DPX_LED_IO_EN);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(ac200->regmap, AC200_EPHY_CTL,
> + AC200_EPHY_LED_POL |
> + AC200_EPHY_CLK_SEL |
> + AC200_EPHY_ADDR(1) |
> + AC200_EPHY_CALIB(calib));
> + if (ret)
> + return ret;
> +
> + ret = phy_driver_register(priv->ephy, THIS_MODULE);
> + if (ret) {
> + dev_err(dev, "Unable to register phy\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int ac200_ephy_remove(struct platform_device *pdev)
> +{
> + struct ac200_ephy_dev *priv = platform_get_drvdata(pdev);
> +
> + phy_driver_unregister(priv->ephy);
> +
> + regmap_write(priv->regmap, AC200_EPHY_CTL, AC200_EPHY_SHUTDOWN);
> + regmap_write(priv->regmap, AC200_SYS_EPHY_CTL1, 0);
> + regmap_write(priv->regmap, AC200_SYS_EPHY_CTL0, 0);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id ac200_ephy_match[] = {
> + { .compatible = "x-powers,ac200-ephy" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ac200_ephy_match);
> +
> +static struct platform_driver ac200_ephy_driver = {
> + .probe = ac200_ephy_probe,
> + .remove = ac200_ephy_remove,
> + .driver = {
> + .name = "ac200-ephy",
> + .of_match_table = ac200_ephy_match,
> + },
> +};
> +module_platform_driver(ac200_ephy_driver);
> +
> +MODULE_AUTHOR("Jernej Skrabec <jernej.skrabec@siol.net>");
> +MODULE_DESCRIPTION("AC200 Ethernet PHY driver");
> +MODULE_LICENSE("GPL");
>
next prev parent reply other threads:[~2020-04-16 20:19 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-16 18:57 [RFC PATCH 0/4] net: mfd: AC200 Ethernet PHY Jernej Skrabec
2020-04-16 18:57 ` [RFC PATCH 1/4] mfd: Add support for AC200 Jernej Skrabec
2020-04-24 8:05 ` Lee Jones
2020-04-16 18:57 ` [RFC PATCH 2/4] net: phy: Add support for AC200 EPHY Jernej Skrabec
2020-04-16 19:18 ` Florian Fainelli
2020-04-17 15:01 ` Jernej Škrabec
2020-04-16 20:18 ` Heiner Kallweit [this message]
2020-04-17 16:03 ` Jernej Škrabec
2020-04-17 16:29 ` Heiner Kallweit
2020-04-17 17:15 ` Jernej Škrabec
2020-04-17 16:15 ` Jernej Škrabec
2020-04-17 16:16 ` Heiner Kallweit
2020-04-17 17:01 ` Andrew Lunn
2020-04-16 18:57 ` [RFC PATCH 3/4] arm64: dts: allwinner: h6: Add AC200 EPHY related nodes Jernej Skrabec
2020-04-16 18:57 ` [RFC PATCH 4/4] arm64: dts: allwinner: h6: tanix-tx6: Enable ethernet Jernej Skrabec
2020-04-16 21:54 ` [RFC PATCH 0/4] net: mfd: AC200 Ethernet PHY Andrew Lunn
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=0340f85c-987f-900b-53c8-d29b4672a8fa@gmail.com \
--to=hkallweit1@gmail.com \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=f.fainelli@gmail.com \
--cc=jernej.skrabec@siol.net \
--cc=lee.jones@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=mripard@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=wens@csie.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).