| 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: <20201210125706.saub7c2rarifhbx4@mchp-dev-shegelun>
Date: Thu, 10 Dec 2020 13:57:06 +0100
From: Steen Hegelund <steen.hegelund@...rochip.com>
To: Andrew Lunn <andrew@...n.ch>
CC: Kishon Vijay Abraham I <kishon@...com>,
Vinod Koul <vkoul@...nel.org>,
<linux-arm-kernel@...ts.infradead.org>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Lars Povlsen <lars.povlsen@...rochip.com>,
Bjarni Jonasson <bjarni.jonasson@...rochip.com>,
Microchip UNG Driver List <UNGLinuxDriver@...rochip.com>,
<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v9 3/4] phy: Add Sparx5 ethernet serdes PHY driver
On 10.12.2020 03:11, Andrew Lunn wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index 01b53f86004c..f6a094c81e86 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -66,9 +66,11 @@ source "drivers/phy/broadcom/Kconfig"
>> source "drivers/phy/cadence/Kconfig"
>> source "drivers/phy/freescale/Kconfig"
>> source "drivers/phy/hisilicon/Kconfig"
>> +source "drivers/phy/intel/Kconfig"
>
>That looks odd.
>
>> source "drivers/phy/lantiq/Kconfig"
>> source "drivers/phy/marvell/Kconfig"
>> source "drivers/phy/mediatek/Kconfig"
>> +source "drivers/phy/microchip/Kconfig"
>> source "drivers/phy/motorola/Kconfig"
>> source "drivers/phy/mscc/Kconfig"
>> source "drivers/phy/qualcomm/Kconfig"
>> @@ -80,7 +82,6 @@ source "drivers/phy/socionext/Kconfig"
>> source "drivers/phy/st/Kconfig"
>> source "drivers/phy/tegra/Kconfig"
>> source "drivers/phy/ti/Kconfig"
>> -source "drivers/phy/intel/Kconfig"
>> source "drivers/phy/xilinx/Kconfig"
>
>Ah. Please make that a separate patch.
Yes - it was really a separate change as a result of my sorting...
>
>> + value = sdx5_rd(priv, SD25G_LANE_CMU_C0(sd_index));
>> + value = SD25G_LANE_CMU_C0_PLL_LOL_UDL_GET(value);
>> +
>> + if (value) {
>> + dev_err(macro->priv->dev, "CMU_C0 pll_lol_udl: 0x%x\n", value);
>> + ret = -EINVAL;
>> + }
>> +
>> + value = sdx5_rd(priv, SD_LANE_25G_SD_LANE_STAT(sd_index));
>> + value = SD_LANE_25G_SD_LANE_STAT_PMA_RST_DONE_GET(value);
>> +
>> + if (value != 0x1) {
>> + dev_err(macro->priv->dev, "sd_lane_stat pma_rst_done: 0x%x\n", value);
>> + ret = -EINVAL;
>> + }
>
>These error messages are not very helpful. Could you be a bit more
>descriptive. Or do you think there is sufficient black magic in the
>hardware that nobody outside of Microchip will be able to debug it?
I will dig up some better descriptions...
>
>> +static int sparx5_serdes_get_serdesmode(phy_interface_t portmode,
>> + struct phy_configure_opts_eth_serdes *conf)
>> +{
>> + switch (portmode) {
>> + case PHY_INTERFACE_MODE_1000BASEX:
>> + if (conf->speed == SPEED_2500)
>> + return SPX5_SD_MODE_2G5;
>> + if (conf->speed == SPEED_100)
>> + return SPX5_SD_MODE_100FX;
>> + return SPX5_SD_MODE_1000BASEX;
>
>Please could you explain this. Why different speeds for 1000BaseX?
I will remove this. It was taken from our bare-metal API (MESA) and
only relevant in that context because it did not have an explicit 2500G
mode.
>
>> + case PHY_INTERFACE_MODE_SGMII:
>> + return SPX5_SD_MODE_1000BASEX;
>
>Here there could be some oddities, depending on how 10Mbps and 100Mbps
>is implemented. But 1000BASEX only supports 1Gbps.
>
The same Serdes mode is used for SGMII and 1000BaseX. Speeds 10M/100M
is handled by repeating the byte sequence 100/10 times to get to 1G
serdes speed.
>> +static int sparx5_serdes_validate(struct phy *phy, enum phy_mode mode,
>> + int submode,
>> + union phy_configure_opts *opts)
>> +{
>> + struct sparx5_serdes_macro *macro = phy_get_drvdata(phy);
>> + struct sparx5_serdes_private *priv = macro->priv;
>> + u32 value, analog_sd;
>> +
>> + if (mode != PHY_MODE_ETHERNET)
>> + return -EINVAL;
>> +
>> + switch (submode) {
>> + case PHY_INTERFACE_MODE_1000BASEX:
>> + case PHY_INTERFACE_MODE_SGMII:
>> + case PHY_INTERFACE_MODE_QSGMII:
>> + case PHY_INTERFACE_MODE_10GBASER:
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + if (macro->serdestype == SPX5_SDT_6G) {
>> + value = sdx5_rd(priv, SD6G_LANE_LANE_DF(macro->stpidx));
>> + analog_sd = SD6G_LANE_LANE_DF_PMA2PCS_RXEI_FILTERED_GET(value);
>> + } else if (macro->serdestype == SPX5_SDT_10G) {
>> + value = sdx5_rd(priv, SD10G_LANE_LANE_DF(macro->stpidx));
>> + analog_sd = SD10G_LANE_LANE_DF_PMA2PCS_RXEI_FILTERED_GET(value);
>> + } else {
>> + value = sdx5_rd(priv, SD25G_LANE_LANE_DE(macro->stpidx));
>> + analog_sd = SD25G_LANE_LANE_DE_LN_PMA_RXEI_GET(value);
>> + }
>> + /* Link up is when analog_sd == 0 */
>> + return analog_sd;
>
>The documentation says:
>
> /**
> * @validate:
> *
> * Optional.
> *
> * Used to check that the current set of parameters can be
> * handled by the phy. Implementations are free to tune the
> * parameters passed as arguments if needed by some
> * implementation detail or constraints. It must not change
> * any actual configuration of the PHY, so calling it as many
> * times as deemed fit by the consumer must have no side
> * effect.
> *
> * Returns: 0 if the configuration can be applied, an negative
> * error code otherwise
> */
>
>So why are returning link up information?
Yes that was a bit of a hijacking of the function. I will remove that.
I also removed the dependency on this behaviour in the client driver in the
meantime.
I think a status function on the generic phy would be useful, but I will
take that as separate issue.
>
> Andrew
Thanks for the comments.
BR
Steen
---------------------------------------
Steen Hegelund
steen.hegelund@...rochip.com
Powered by blists - more mailing lists