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: <49297ed2-84dc-3e9b-8f40-af915476a091@st.com> Date: Mon, 8 May 2017 16:28:21 +0200 From: Giuseppe CAVALLARO <peppe.cavallaro@...com> To: Corentin Labbe <clabbe.montjoie@...il.com> CC: Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>, Alexandre Torgue <alexandre.torgue@...com>, <netdev@...r.kernel.org>, <stable@...r.kernel.org> Subject: Re: [PATCH] net: ethernet: stmmac: properly set PS bit in MII configurations during reset Hello On 5/3/2017 4:30 PM, Corentin Labbe wrote: > On Wed, May 03, 2017 at 10:13:56AM +0200, Giuseppe CAVALLARO wrote: >> Hello Thomas >> >> this was initially set by using the hw->link.port; both the core_init >> and adjust callback >> should invoke the hook and tuning the PS bit according to the speed and >> mode. >> So maybe the ->set_ps is superfluous and you could reuse the existent hook >> >> let me know >> >> Regards >> peppe >> > I just see that GMAC_CONTROL and MAC_CTRL_REG are the same, so why not create a custom adjust_link for each dwmac type ? > This will permit to call it instead of set_ps() and remove lots of if (has_gmac) and co in stmmac_adjust_link() > Basicly replace all between "ctrl = readl()... and writel(ctrl)" by a sot of priv->hw->mac->adjust_link() > > It will also help a lot for my dwmac-sun8i inclusion (since I add some if has_sun8i:)) Corentin, I think this is a good idea and maybe necessary now that the driver is supporting a lot of chips. In the past it was sufficient to have a adjust link function and a stmmac_hw_fix_mac_speed to invoke dedicated hook shared between MAC10/100 and GMAC inside STM platforms. Thomas, I wonder if you could take a look at the priv->plat->fix_mac_speed. This can be used for setting internal registers too. Regards Peppe > > Regards > >> On 4/27/2017 11:45 AM, Thomas Petazzoni wrote: >>> On the SPEAr600 SoC, which has the dwmac1000 variant of the IP block, >>> the DMA reset never succeeds when a MII PHY is used (no problem with a >>> GMII PHY). The dwmac_dma_reset() function sets the >>> DMA_BUS_MODE_SFT_RESET bit in the DMA_BUS_MODE register, and then >>> polls until this bit clears. When a MII PHY is used, with the current >>> driver, this bit never clears and the driver therefore doesn't work. >>> >>> The reason is that the PS bit of the GMAC_CONTROL register should be >>> correctly configured for the DMA reset to work. When the PS bit is 0, >>> it tells the MAC we have a GMII PHY, when the PS bit is 1, it tells >>> the MAC we have a MII PHY. >>> >>> Doing a DMA reset clears all registers, so the PS bit is cleared as >>> well. This makes the DMA reset work fine with a GMII PHY. However, >>> with MII PHY, the PS bit should be set. >>> >>> We have identified this issue thanks to two SPEAr600 platform: >>> >>> - One equipped with a GMII PHY, with which the existing driver was >>> working fine. >>> >>> - One equipped with a MII PHY, where the current driver fails because >>> the DMA reset times out. >>> >>> This patch fixes the problem for the MII PHY configuration, and has >>> been tested with a GMII PHY configuration as well. >>> >>> In terms of implement, since the ->reset() hook is implemented in the >>> DMA related code, we do not want to touch directly from this function >>> the MAC registers. Therefore, a ->set_ps() hook has been added to >>> stmmac_ops, which gets called between the moment the reset is asserted >>> and the polling loop waiting for the reset bit to clear. >>> >>> In order for this ->set_ps() hook to decide what to do, we pass it the >>> "struct mac_device_info" so it can access the MAC registers, and the >>> PHY interface type so it knows if we're using a MII PHY or not. >>> >>> The ->set_ps() hook is only implemented for the dwmac1000 case. >>> >>> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@...e-electrons.com> >>> Cc: <stable@...r.kernel.org> >>> --- >>> Do not hesitate to suggest ideas for alternative implementations, I'm >>> not sure if the current proposal is the one that fits best with the >>> current design of the driver. >>> --- >>> drivers/net/ethernet/stmicro/stmmac/common.h | 12 +++++++++--- >>> drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c | 16 ++++++++++++++++ >>> drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.h | 3 ++- >>> drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c | 7 ++++++- >>> drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h | 3 ++- >>> drivers/net/ethernet/stmicro/stmmac/dwmac_lib.c | 6 +++++- >>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++- >>> 7 files changed, 42 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h >>> index 04d9245..d576f95 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/common.h >>> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h >>> @@ -407,10 +407,13 @@ struct stmmac_desc_ops { >>> extern const struct stmmac_desc_ops enh_desc_ops; >>> extern const struct stmmac_desc_ops ndesc_ops; >>> >>> +struct mac_device_info; >>> + >>> /* Specific DMA helpers */ >>> struct stmmac_dma_ops { >>> /* DMA core initialization */ >>> - int (*reset)(void __iomem *ioaddr); >>> + int (*reset)(void __iomem *ioaddr, struct mac_device_info *hw, >>> + phy_interface_t interface); >>> void (*init)(void __iomem *ioaddr, struct stmmac_dma_cfg *dma_cfg, >>> u32 dma_tx, u32 dma_rx, int atds); >>> /* Configure the AXI Bus Mode Register */ >>> @@ -445,12 +448,15 @@ struct stmmac_dma_ops { >>> void (*enable_tso)(void __iomem *ioaddr, bool en, u32 chan); >>> }; >>> >>> -struct mac_device_info; >>> - >>> /* Helpers to program the MAC core */ >>> struct stmmac_ops { >>> /* MAC core initialization */ >>> void (*core_init)(struct mac_device_info *hw, int mtu); >>> + /* Set port select. Called between asserting DMA reset and >>> + * waiting for the reset bit to clear. >>> + */ >>> + void (*set_ps)(struct mac_device_info *hw, >>> + phy_interface_t interface); >>> /* Enable and verify that the IPC module is supported */ >>> int (*rx_ipc)(struct mac_device_info *hw); >>> /* Enable RX Queues */ >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c >>> index 19b9b308..dfcbb5b 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c >>> @@ -75,6 +75,21 @@ static void dwmac1000_core_init(struct mac_device_info *hw, int mtu) >>> #endif >>> } >>> >>> +static void dwmac1000_set_ps(struct mac_device_info *hw, >>> + phy_interface_t interface) >>> +{ >>> + void __iomem *ioaddr = hw->pcsr; >>> + u32 value = readl(ioaddr + GMAC_CONTROL); >>> + >>> + /* When a MII PHY is used, we must set the PS bit for the DMA >>> + * reset to succeed. >>> + */ >>> + if (interface == PHY_INTERFACE_MODE_MII) >>> + value |= GMAC_CONTROL_PS; >>> + >>> + writel(value, ioaddr + GMAC_CONTROL); >>> +} >>> + >>> static int dwmac1000_rx_ipc_enable(struct mac_device_info *hw) >>> { >>> void __iomem *ioaddr = hw->pcsr; >>> @@ -488,6 +503,7 @@ static void dwmac1000_debug(void __iomem *ioaddr, struct stmmac_extra_stats *x) >>> >>> static const struct stmmac_ops dwmac1000_ops = { >>> .core_init = dwmac1000_core_init, >>> + .set_ps = dwmac1000_set_ps, >>> .rx_ipc = dwmac1000_rx_ipc_enable, >>> .dump_regs = dwmac1000_dump_regs, >>> .host_irq_status = dwmac1000_irq_status, >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.h b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.h >>> index 1b06df7..e9c6c49 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.h >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.h >>> @@ -183,7 +183,8 @@ >>> #define DMA_CHAN0_DBG_STAT_RPS GENMASK(11, 8) >>> #define DMA_CHAN0_DBG_STAT_RPS_SHIFT 8 >>> >>> -int dwmac4_dma_reset(void __iomem *ioaddr); >>> +int dwmac4_dma_reset(void __iomem *ioaddr, struct mac_device_info *hw, >>> + phy_interface_t interface); >>> void dwmac4_enable_dma_transmission(void __iomem *ioaddr, u32 tail_ptr); >>> void dwmac4_enable_dma_irq(void __iomem *ioaddr); >>> void dwmac410_enable_dma_irq(void __iomem *ioaddr); >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c >>> index c7326d5..485eecb 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c >>> @@ -14,7 +14,8 @@ >>> #include "dwmac4_dma.h" >>> #include "dwmac4.h" >>> >>> -int dwmac4_dma_reset(void __iomem *ioaddr) >>> +int dwmac4_dma_reset(void __iomem *ioaddr, struct mac_device_info *hw, >>> + phy_interface_t interface) >>> { >>> u32 value = readl(ioaddr + DMA_BUS_MODE); >>> int limit; >>> @@ -22,6 +23,10 @@ int dwmac4_dma_reset(void __iomem *ioaddr) >>> /* DMA SW reset */ >>> value |= DMA_BUS_MODE_SFT_RESET; >>> writel(value, ioaddr + DMA_BUS_MODE); >>> + >>> + if (hw->mac->set_ps) >>> + hw->mac->set_ps(hw, interface); >>> + >>> limit = 10; >>> while (limit--) { >>> if (!(readl(ioaddr + DMA_BUS_MODE) & DMA_BUS_MODE_SFT_RESET)) >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h b/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h >>> index 56e485f..25ae028 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h >>> @@ -144,6 +144,7 @@ void dwmac_dma_stop_tx(void __iomem *ioaddr); >>> void dwmac_dma_start_rx(void __iomem *ioaddr); >>> void dwmac_dma_stop_rx(void __iomem *ioaddr); >>> int dwmac_dma_interrupt(void __iomem *ioaddr, struct stmmac_extra_stats *x); >>> -int dwmac_dma_reset(void __iomem *ioaddr); >>> +int dwmac_dma_reset(void __iomem *ioaddr, struct mac_device_info *hw, >>> + phy_interface_t interface); >>> >>> #endif /* __DWMAC_DMA_H__ */ >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac_lib.c b/drivers/net/ethernet/stmicro/stmmac/dwmac_lib.c >>> index e60bfca..1a17df5 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac_lib.c >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac_lib.c >>> @@ -23,7 +23,8 @@ >>> >>> #define GMAC_HI_REG_AE 0x80000000 >>> >>> -int dwmac_dma_reset(void __iomem *ioaddr) >>> +int dwmac_dma_reset(void __iomem *ioaddr, struct mac_device_info *hw, >>> + phy_interface_t interface) >>> { >>> u32 value = readl(ioaddr + DMA_BUS_MODE); >>> int err; >>> @@ -32,6 +33,9 @@ int dwmac_dma_reset(void __iomem *ioaddr) >>> value |= DMA_BUS_MODE_SFT_RESET; >>> writel(value, ioaddr + DMA_BUS_MODE); >>> >>> + if (hw->mac->set_ps) >>> + hw->mac->set_ps(hw, interface); >>> + >>> err = readl_poll_timeout(ioaddr + DMA_BUS_MODE, value, >>> !(value & DMA_BUS_MODE_SFT_RESET), >>> 100000, 10000); >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>> index 4498a38..66bc218 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>> @@ -1585,7 +1585,8 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv) >>> if (priv->extend_desc && (priv->mode == STMMAC_RING_MODE)) >>> atds = 1; >>> >>> - ret = priv->hw->dma->reset(priv->ioaddr); >>> + ret = priv->hw->dma->reset(priv->ioaddr, priv->hw, >>> + priv->plat->interface); >>> if (ret) { >>> dev_err(priv->device, "Failed to reset the dma\n"); >>> return ret; >>
Powered by blists - more mailing lists