Message ID | 1498154732-19854-3-git-send-email-timur@codeaurora.org |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
From: Timur Tabi <timur@codeaurora.org> Date: Thu, 22 Jun 2017 13:05:31 -0500 > It doesn't make sense to reset the EMAC in the middle of initializing > it during the probe. > > Tested-by: Richard Ruigrok <rruigrok@codeaurora.org> > Signed-off-by: Timur Tabi <timur@codeaurora.org> Why not? What if the boot loader or something else left the chip in a weird state? I'm not applying this. If it's correct, the explanation in this commit message need to be imporved. The change must be better justified.
On 06/23/2017 01:00 PM, David Miller wrote: > What if the boot loader or something else left the chip in > a weird state? We depend on the boot loader leaving the NIC in a very specific state already, otherwise the driver can't initialize the hardware. The firmware has to pre-initialize the EMAC for us. Not only that, but the driver was resetting the MAC *after* programming the clocks (on non-ACPI systems) and initializing both PHYs. > I'm not applying this. > > If it's correct, the explanation in this commit message need > to be imporved. The change must be better justified. Since this is for ACPI systems, I could do this: if (!has_acpi_companion(&pdev->dev)) emac_mac_reset(adpt); But at the very least, the call should be moved to earlier in the function.
From: Timur Tabi <timur@codeaurora.org> Date: Fri, 23 Jun 2017 13:37:58 -0500 > On 06/23/2017 01:00 PM, David Miller wrote: >> What if the boot loader or something else left the chip in >> a weird state? > > We depend on the boot loader leaving the NIC in a very specific state > already, otherwise the driver can't initialize the hardware. The > firmware has to pre-initialize the EMAC for us. > > Not only that, but the driver was resetting the MAC *after* > programming the clocks (on non-ACPI systems) and initializing both > PHYs. > >> I'm not applying this. >> If it's correct, the explanation in this commit message need >> to be imporved. The change must be better justified. > > Since this is for ACPI systems, I could do this: > > if (!has_acpi_companion(&pdev->dev)) > emac_mac_reset(adpt); > > But at the very least, the call should be moved to earlier in the > function. Please just explain the ACPI situation in the commit log message and resubmit the series. Thanks.
diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c index 77c5c92..746d94e 100644 --- a/drivers/net/ethernet/qualcomm/emac/emac.c +++ b/drivers/net/ethernet/qualcomm/emac/emac.c @@ -683,8 +683,6 @@ static int emac_probe(struct platform_device *pdev) goto err_undo_mdiobus; } - emac_mac_reset(adpt); - /* set hw features */ netdev->features = NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_RXCSUM | NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_HW_VLAN_CTAG_RX |