| 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
| ||
|
Message-ID: <20161007202225.GC8266@lunn.ch>
Date: Fri, 7 Oct 2016 22:22:25 +0200
From: Andrew Lunn <andrew@...n.ch>
To: "Allan W. Nielsen" <allan.nielsen@...rosemi.com>
Cc: netdev@...r.kernel.org, f.fainelli@...il.com,
raju.lakkaraju@...rosemi.com
Subject: Re: [PATCH net-next v9 1/1] net: phy: Cleanup the Edge-Rate feature
in Microsemi PHYs.
On Fri, Oct 07, 2016 at 10:28:24AM +0200, Allan W. Nielsen wrote:
> Edge-Rate cleanup include the following:
> - Updated device tree bindings documentation for edge-rate
> - The edge-rate is now specified as a "slowdown", meaning that it is now
> being specified as positive values instead of negative (both
> documentation and implementation wise).
> - Only explicitly documented values for "vsc8531,vddmac" and
> "vsc8531,edge-slowdown" are accepted by the device driver.
> - Deleting include/dt-bindings/net/mscc-phy-vsc8531.h as it was not needed.
>
> Signed-off-by: Allan W. Nielsen <allan.nielsen@...rosemi.com>
> Signed-off-by: Raju Lakkaraju <raju.lakkaraju@...rosemi.com>
> ---
> .../devicetree/bindings/net/mscc-phy-vsc8531.txt | 49 ++++++++------
> drivers/net/phy/mscc.c | 79 +++++++++++++---------
> include/dt-bindings/net/mscc-phy-vsc8531.h | 21 ------
> 3 files changed, 75 insertions(+), 74 deletions(-)
> delete mode 100644 include/dt-bindings/net/mscc-phy-vsc8531.h
>
> diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> index 99c7eb0..f552033 100644
> --- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> +++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> @@ -6,20 +6,27 @@ Required properties:
> Documentation/devicetree/bindings/net/phy.txt
>
> Optional properties:
> -- vsc8531,vddmac : The vddmac in mV.
> +- vsc8531,vddmac : The vddmac in mV. Allowed values is listed
> + in the first row of Table 1 (below).
> + This property is only used in combination
> + with the 'edge-slowdown' property.
> + Default value is 3300.
> - vsc8531,edge-slowdown : % the edge should be slowed down relative to
> - the fastest possible edge time. Native sign
> - need not enter.
> + the fastest possible edge time.
> Edge rate sets the drive strength of the MAC
> - interface output signals. Changing the drive
> - strength will affect the edge rate of the output
> - signal. The goal of this setting is to help
> - reduce electrical emission (EMI) by being able
> - to reprogram drive strength and in effect slow
> - down the edge rate if desired. Table 1 shows the
> - impact to the edge rate per VDDMAC supply for each
> - drive strength setting.
> - Ref: Table:1 - Edge rate change below.
> + interface output signals. Changing the
> + drive strength will affect the edge rate of
> + the output signal. The goal of this setting
> + is to help reduce electrical emission (EMI)
> + by being able to reprogram drive strength
> + and in effect slow down the edge rate if
> + desired.
> + To adjust the edge-slowdown, the 'vddmac'
> + must be specified. Table 1 lists the
> + supported edge-slowdown values for a given
> + 'vddmac'.
> + Default value is 0%.
> + Ref: Table:1 - Edge rate change (below).
>
> Note: see dt-bindings/net/mscc-phy-vsc8531.h for applicable values
Hi Allen
Overall, this is much better. I just have a few nitpicks.
dt-bindings/net/mscc-phy-vsc8531.h is removed by this patch. It would
be good to also remove the reference.
>
> @@ -29,23 +36,23 @@ Table: 1 - Edge rate change
> | |
> | 3300 mV 2500 mV 1800 mV 1500 mV |
> |---------------------------------------------------------------|
> -| Default Deafult Default Default |
> +| 0% 0% 0% 0% |
> | (Fastest) (recommended) (recommended) |
> |---------------------------------------------------------------|
> -| -2% -3% -5% -6% |
> +| 2% 3% 5% 6% |
> |---------------------------------------------------------------|
> -| -4% -6% -9% -14% |
> +| 4% 6% 9% 14% |
> |---------------------------------------------------------------|
> -| -7% -10% -16% -21% |
> +| 7% 10% 16% 21% |
> |(recommended) (recommended) |
> |---------------------------------------------------------------|
> -| -10% -14% -23% -29% |
> +| 10% 14% 23% 29% |
> |---------------------------------------------------------------|
> -| -17% -23% -35% -42% |
> +| 17% 23% 35% 42% |
> |---------------------------------------------------------------|
> -| -29% -37% -52% -58% |
> +| 29% 37% 52% 58% |
> |---------------------------------------------------------------|
> -| -53% -63% -76% -77% |
> +| 53% 63% 76% 77% |
> | (slowest) |
> |---------------------------------------------------------------|
>
> @@ -54,5 +61,5 @@ Example:
> vsc8531_0: ethernet-phy@0 {
> compatible = "ethernet-phy-id0007.0570";
> vsc8531,vddmac = <3300>;
> - vsc8531,edge-slowdown = <21>;
> + vsc8531,edge-slowdown = <7>;
> };
> diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> index a17573e..2bd00b6 100644
> --- a/drivers/net/phy/mscc.c
> +++ b/drivers/net/phy/mscc.c
> @@ -12,7 +12,6 @@
> #include <linux/mii.h>
> #include <linux/phy.h>
> #include <linux/of.h>
> -#include <dt-bindings/net/mscc-phy-vsc8531.h>
>
> enum rgmii_rx_clock_delay {
> RGMII_RX_CLK_DELAY_0_2_NS = 0,
> @@ -56,21 +55,27 @@ enum rgmii_rx_clock_delay {
> #define PHY_ID_VSC8531 0x00070570
> #define PHY_ID_VSC8541 0x00070770
>
> -struct edge_rate_table {
> +#define MSCC_VDDMAC_1500 1500
> +#define MSCC_VDDMAC_1800 1800
> +#define MSCC_VDDMAC_2500 2500
> +#define MSCC_VDDMAC_3300 3300
> +
> +struct vsc8531_edge_rate_table {
> u16 vddmac;
> - int slowdown[MSCC_SLOWDOWN_MAX];
> + u8 slowdown[8];
> };
>
> -struct edge_rate_table edge_table[MSCC_VDDMAC_MAX] = {
> - {3300, { 0, -2, -4, -7, -10, -17, -29, -53} },
> - {2500, { 0, -3, -6, -10, -14, -23, -37, -63} },
> - {1800, { 0, -5, -9, -16, -23, -35, -52, -76} },
> - {1500, { 0, -6, -14, -21, -29, -42, -58, -77} },
> +static const struct vsc8531_edge_rate_table edge_table[] = {
> + {MSCC_VDDMAC_3300, { 0, 2, 4, 7, 10, 17, 29, 53} },
> + {MSCC_VDDMAC_2500, { 0, 3, 6, 10, 14, 23, 37, 63} },
> + {MSCC_VDDMAC_1800, { 0, 5, 9, 16, 23, 35, 52, 76} },
> + {MSCC_VDDMAC_1500, { 0, 6, 14, 21, 29, 42, 58, 77} },
> };
>
> struct vsc8531_private {
> u8 edge_slowdown;
> u16 vddmac;
> + int rate_magic;
> };
You don't need edge_slowdown and vddmac in the private structure,
since they are never used after determining what rate_magic is.
>
> static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
> @@ -81,29 +86,21 @@ static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
> return rc;
> }
>
> -static u8 edge_rate_magic_get(u16 vddmac,
> - int slowdown)
> +static int vsc85xx_edge_rate_magic_get(u16 vddmac, u8 slowdown)
> {
> - int rc = (MSCC_SLOWDOWN_MAX - 1);
> u8 vdd;
> u8 sd;
> -
> - for (vdd = 0; vdd < MSCC_VDDMAC_MAX; vdd++) {
> - if (edge_table[vdd].vddmac == vddmac) {
> - for (sd = 0; sd < MSCC_SLOWDOWN_MAX; sd++) {
> - if (edge_table[vdd].slowdown[sd] <= slowdown) {
> - rc = (MSCC_SLOWDOWN_MAX - sd - 1);
> - break;
> - }
> - }
> - }
> - }
> -
> - return rc;
> + u8 sd_array_size = ARRAY_SIZE(edge_table[0].slowdown);
> +
> + for (vdd = 0; vdd < ARRAY_SIZE(edge_table); vdd++)
> + if (edge_table[vdd].vddmac == vddmac)
> + for (sd = 0; sd < sd_array_size; sd++)
> + if (edge_table[vdd].slowdown[sd] == slowdown)
> + return (sd_array_size - sd - 1);
> + return -EINVAL;
> }
>
> -static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev,
> - u8 edge_rate)
> +static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev, u8 edge_rate)
> {
> int rc;
> u16 reg_val;
> @@ -199,17 +196,38 @@ static int vsc8531_of_init(struct phy_device *phydev)
> &vsc8531->vddmac);
> if (rc == -EINVAL)
> vsc8531->vddmac = MSCC_VDDMAC_3300;
> +
> rc = of_property_read_u8(of_node, "vsc8531,edge-slowdown",
> &vsc8531->edge_slowdown);
> if (rc == -EINVAL)
> vsc8531->edge_slowdown = 0;
>
> - rc = 0;
> - return rc;
> + vsc8531->rate_magic = vsc85xx_edge_rate_magic_get(
> + vsc8531->vddmac, vsc8531->edge_slowdown);
> + if (vsc8531->rate_magic < 0) {
> + phydev_err(phydev,
> + "Invalid vsc8531,vddmac or vsc8531,edge-slowdown");
> + return vsc8531->rate_magic;
> + }
> +
> + return 0;
> }
> #else
> static int vsc8531_of_init(struct phy_device *phydev)
> {
> + struct vsc8531_private *vsc8531 = phydev->priv;
> +
> + vsc8531->vddmac = MSCC_VDDMAC_3300;
> + vsc8531->edge_slowdown = 0;
> +
> + vsc8531->rate_magic = vsc85xx_edge_rate_magic_get(
> + vsc8531->vddmac, vsc8531->edge_slowdown);
> + if (vsc8531->rate_magic < 0) {
> + phydev_err(phydev,
> + "Invalid vsc8531,vddmac or vsc8531,edge-slowdown");
> + return vsc8531->rate_magic;
> + }
You could skip all this and just have:
vsc8531->rate_magic = 0;
vsc8531->vddmac and vsc8531->edge_slowdown are not used anywhere else.
Andrew
Powered by blists - more mailing lists