| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 12 Jan 2022 12:07:53 +0100
From: Martin Schiller <ms@....tdt.de>
To: Tim Harvey <tharvey@...eworks.com>
Cc: Hauke Mehrtens <hauke@...ke-m.de>,
martin.blumenstingl@...glemail.com,
Florian Fainelli <f.fainelli@...il.com>,
Andrew Lunn <andrew@...n.ch>, hkallweit1@...il.com,
Russell King - ARM Linux <linux@...linux.org.uk>,
David Miller <davem@...emloft.net>, kuba@...nel.org,
netdev <netdev@...r.kernel.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal
delay configuration
On 2022-01-11 20:12, Tim Harvey wrote:
> On Mon, Jan 10, 2022 at 11:44 PM Martin Schiller <ms@....tdt.de> wrote:
>>
>> On 2022-01-11 00:12, Tim Harvey wrote:
>> > On Mon, Jul 19, 2021 at 2:07 AM Martin Schiller <ms@....tdt.de> wrote:
>> >>
>> >> This adds the possibility to configure the RGMII RX/TX clock skew via
>> >> devicetree.
>> >>
>> >> Simply set phy mode to "rgmii-id", "rgmii-rxid" or "rgmii-txid" and
>> >> add
>> >> the "rx-internal-delay-ps" or "tx-internal-delay-ps" property to the
>> >> devicetree.
>> >>
>> >> Furthermore, a warning is now issued if the phy mode is configured to
>> >> "rgmii" and an internal delay is set in the phy (e.g. by
>> >> pin-strapping),
>> >> as in the dp83867 driver.
>> >>
>> >> Signed-off-by: Martin Schiller <ms@....tdt.de>
>> >> ---
>> >>
>> >> Changes to v5:
>> >> o remove #if IS_ENABLED(CONFIG_OF_MDIO) check
>> >> o rename new function to xway_gphy_rgmii_init()
>> >>
>> >> Changes to v4:
>> >> o Fix Alignment to match open parenthesis
>> >>
>> >> Changes to v3:
>> >> o Fix typo in commit message
>> >> o use FIELD_PREP() and FIELD_GET() macros
>> >> o further code cleanups
>> >> o always mask rxskew AND txskew value in the register value
>> >>
>> >> Changes to v2:
>> >> o Fix missing whitespace in warning.
>> >>
>> >> Changes to v1:
>> >> o code cleanup and use phy_modify().
>> >> o use default of 2.0ns if delay property is absent instead of
>> >> returning
>> >> an error.
>> >>
>> >> ---
>> >> drivers/net/phy/intel-xway.c | 78
>> >> ++++++++++++++++++++++++++++++++++++
>> >> 1 file changed, 78 insertions(+)
>> >>
>> >> diff --git a/drivers/net/phy/intel-xway.c
>> >> b/drivers/net/phy/intel-xway.c
>> >> index d453ec016168..fd7da2eeb963 100644
>> >> --- a/drivers/net/phy/intel-xway.c
>> >> +++ b/drivers/net/phy/intel-xway.c
>> >> @@ -8,11 +8,16 @@
>> >> #include <linux/module.h>
>> >> #include <linux/phy.h>
>> >> #include <linux/of.h>
>> >> +#include <linux/bitfield.h>
>> >>
>> >> +#define XWAY_MDIO_MIICTRL 0x17 /* mii control */
>> >> #define XWAY_MDIO_IMASK 0x19 /* interrupt
>> >> mask */
>> >> #define XWAY_MDIO_ISTAT 0x1A /* interrupt
>> >> status */
>> >> #define XWAY_MDIO_LED 0x1B /* led control */
>> >>
>> >> +#define XWAY_MDIO_MIICTRL_RXSKEW_MASK GENMASK(14, 12)
>> >> +#define XWAY_MDIO_MIICTRL_TXSKEW_MASK GENMASK(10, 8)
>> >> +
>> >> /* bit 15:12 are reserved */
>> >> #define XWAY_MDIO_LED_LED3_EN BIT(11) /* Enable the
>> >> integrated function of LED3 */
>> >> #define XWAY_MDIO_LED_LED2_EN BIT(10) /* Enable the
>> >> integrated function of LED2 */
>> >> @@ -157,6 +162,75 @@
>> >> #define PHY_ID_PHY11G_VR9_1_2 0xD565A409
>> >> #define PHY_ID_PHY22F_VR9_1_2 0xD565A419
>> >>
>> >> +static const int xway_internal_delay[] = {0, 500, 1000, 1500, 2000,
>> >> 2500,
>> >> + 3000, 3500};
>> >> +
>> >> +static int xway_gphy_rgmii_init(struct phy_device *phydev)
>> >> +{
>> >> + struct device *dev = &phydev->mdio.dev;
>> >> + unsigned int delay_size = ARRAY_SIZE(xway_internal_delay);
>> >> + s32 int_delay;
>> >> + int val = 0;
>> >> +
>> >> + if (!phy_interface_is_rgmii(phydev))
>> >> + return 0;
>> >> +
>> >> + /* Existing behavior was to use default pin strapping delay in
>> >> rgmii
>> >> + * mode, but rgmii should have meant no delay. Warn existing
>> >> users,
>> >> + * but do not change anything at the moment.
>> >> + */
>> >> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII) {
>> >> + u16 txskew, rxskew;
>> >> +
>> >> + val = phy_read(phydev, XWAY_MDIO_MIICTRL);
>> >> + if (val < 0)
>> >> + return val;
>> >> +
>> >> + txskew = FIELD_GET(XWAY_MDIO_MIICTRL_TXSKEW_MASK,
>> >> val);
>> >> + rxskew = FIELD_GET(XWAY_MDIO_MIICTRL_RXSKEW_MASK,
>> >> val);
>> >> +
>> >> + if (txskew > 0 || rxskew > 0)
>> >> + phydev_warn(phydev,
>> >> + "PHY has delays (e.g. via pin
>> >> strapping), but phy-mode = 'rgmii'\n"
>> >> + "Should be 'rgmii-id' to use
>> >> internal delays txskew:%d ps rxskew:%d ps\n",
>> >> + xway_internal_delay[txskew],
>> >> + xway_internal_delay[rxskew]);
>> >> + return 0;
>> >> + }
>> >> +
>> >> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
>> >> + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
>> >> + int_delay = phy_get_internal_delay(phydev, dev,
>> >> +
>> >> xway_internal_delay,
>> >> + delay_size, true);
>> >> +
>> >> + if (int_delay < 0) {
>> >> + phydev_warn(phydev, "rx-internal-delay-ps is
>> >> missing, use default of 2.0 ns\n");
>> >> + int_delay = 4; /* 2000 ps */
>> >> + }
>> >> +
>> >> + val |= FIELD_PREP(XWAY_MDIO_MIICTRL_RXSKEW_MASK,
>> >> int_delay);
>> >> + }
>> >> +
>> >> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
>> >> + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
>> >> + int_delay = phy_get_internal_delay(phydev, dev,
>> >> +
>> >> xway_internal_delay,
>> >> + delay_size, false);
>> >> +
>> >> + if (int_delay < 0) {
>> >> + phydev_warn(phydev, "tx-internal-delay-ps is
>> >> missing, use default of 2.0 ns\n");
>> >> + int_delay = 4; /* 2000 ps */
>> >> + }
>> >> +
>> >> + val |= FIELD_PREP(XWAY_MDIO_MIICTRL_TXSKEW_MASK,
>> >> int_delay);
>> >> + }
>> >> +
>> >> + return phy_modify(phydev, XWAY_MDIO_MIICTRL,
>> >> + XWAY_MDIO_MIICTRL_RXSKEW_MASK |
>> >> + XWAY_MDIO_MIICTRL_TXSKEW_MASK, val);
>> >> +}
>> >> +
>> >> static int xway_gphy_config_init(struct phy_device *phydev)
>> >> {
>> >> int err;
>> >> @@ -204,6 +278,10 @@ static int xway_gphy_config_init(struct
>> >> phy_device *phydev)
>> >> phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2H, ledxh);
>> >> phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2L, ledxl);
>> >>
>> >> + err = xway_gphy_rgmii_init(phydev);
>> >> + if (err)
>> >> + return err;
>> >> +
>> >> return 0;
>> >> }
>> >>
>> >> --
>> >> 2.20.1
>> >>
>> >
>> > Martin,
>> >
>> > I've got some boards with the GPY111 phy on them and I'm finding that
>> > modifying XWAY_MDIO_MIICTRL to change the skew has no effect unless I
>> > do a soft reset (BCMR_RESET) first. I don't see anything in the
>> > datasheet which specifies this to be the case so I'm interested it
>> > what you have found. Are you sure adjusting the skews like this
>> > without a soft (or hard pin based) reset actually works?
>> >
>> > Best regards,
>> >
>> > Tim
>>
>> Hello Tim,
>>
>> yes, you are right. It is not applied immediately. The link needs to
>> be
>> toggled to get this settings active. But my experience shows that this
>> would be done in the further boot process anyway e.g. by restarting
>> the
>> autonegotiation etc.
>>
>
> Martin,
>
> I added a debug statement in xway_gphy_rgmii_init and here you can see
> it gets called 'before' the link comes up from the NIC on a board that
> has a cable plugged in at power-on. I can tell from testing that the
> rx_delay/tx_delay set in xway_gphy_rgmii_init does not actually take
> effect unless I then bring the link down and up again manually as you
> indicate.
>
> # dmesg | egrep "xway|nicvf"
> [ 6.855971] xway_gphy_rgmii_init mdio_thunder MDI_MIICTRL:0xb100
> rx_delay=1500 tx_delay=500
> [ 6.999651] nicvf, ver 1.0
> [ 7.002478] nicvf 0000:05:00.1: Adding to iommu group 7
> [ 7.007785] nicvf 0000:05:00.1: enabling device (0004 -> 0006)
> [ 7.053189] nicvf 0000:05:00.2: Adding to iommu group 8
> [ 7.058511] nicvf 0000:05:00.2: enabling device (0004 -> 0006)
> [ 11.044616] nicvf 0000:05:00.2 eth1: Link is Up 1000 Mbps Full
> duplex
>
> If I add a 'genphy_soft_reset(phydev);' at the top of
> xway_gphy_rgmii_init before the write to XWAY_MDIO_MIICTRL the values
> do take effect so perhaps that's the proper fix.
OK, I see that we have to change something here.
But I would like to avoid a complete reset (BMCR_RESET) if possible.
How about this:
Before configuring the skews set BMCR_PDOWN using genphy_suspend(phydev)
and when we are done let's call genphy_resume(phydev).
>
> I'm not fond of even using this phy driver either as it blatantly
> forces LED configuration which may not agree with what boot firmware
> does. I've noticed phy drivers starting to configure LED behavior more
> and more but it seems like there should be dt bindings for that or
> maybe an option to preserve the configuration that is set from boot
> firmware.
There is already an approach which is used in openwrt [1] and which
Hauke tried to get into the kernel [2].
Interesting would be also a solution like this approach here [3].
But this is rather off-topic now.
[1]
https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/lantiq/patches-5.10/0023-NET-PHY-add-led-support-for-intel-xway.patch;h=fb8d97511066bd7e20f8cd298401e81f74e02ae9;hb=d337731f85c880acc96e8a6b99b62aeb57b8253f
[2] https://www.spinics.net/lists/devicetree/msg129574.html
[3] https://www.spinics.net/lists/linux-leds/msg17241.html
Powered by blists - more mailing lists