Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752494AbdIVQ6I (ORCPT ); Fri, 22 Sep 2017 12:58:08 -0400 Received: from mail-qt0-f196.google.com ([209.85.216.196]:37265 "EHLO mail-qt0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752055AbdIVQ6G (ORCPT ); Fri, 22 Sep 2017 12:58:06 -0400 X-Google-Smtp-Source: AOwi7QC+ejmC0Hg7J54CWPghkXyLRlTopzWg9q75NYXakwPLTPOYHvYgdv9TVpjd5gIm0nZkB+RrIQ== Subject: Re: [PATCH net-next 1/4] net: dsa: move up phy enabling in core To: Andrew Lunn , Vivien Didelot Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, kernel@savoirfairelinux.com, "David S. Miller" References: <20170922161753.19563-1-vivien.didelot@savoirfairelinux.com> <20170922161753.19563-2-vivien.didelot@savoirfairelinux.com> <20170922163258.GA3470@lunn.ch> From: Florian Fainelli Message-ID: Date: Fri, 22 Sep 2017 09:58:00 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170922163258.GA3470@lunn.ch> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1289 Lines: 35 On 09/22/2017 09:32 AM, Andrew Lunn wrote: > On Fri, Sep 22, 2017 at 12:17:50PM -0400, Vivien Didelot wrote: >> bcm_sf2 is currently the only driver using the phy argument passed to >> .port_enable. It resets the state machine if the phy has been hard >> reset. This check is generic and can be moved to DSA core. >> >> dsa_port_set_state_now(p->dp, stp_state); >> >> - if (p->phy) >> - phy_start(p->phy); >> + if (phy) { >> + /* If phy_stop() has been called before, phy will be in >> + * halted state, and phy_start() will call resume. >> + * >> + * The resume path does not configure back autoneg >> + * settings, and since the internal phy may have been >> + * hard reset, we need to reset the state machine also. >> + */ >> + phy->state = PHY_READY; >> + phy_init_hw(phy); >> + phy_start(phy); >> + } > > Hi Vivien > > If this is generic, why is it needed at all here? Shouldn't this > actually by in phylib? This does not belong in the core logic within net/dsa/slave.c. The reason why this is necessary here is because we are doing a HW-based reset of the PHY, as the comment explains this is specific to how the HW works. There may be a cleaner solution to this problem, but in any case, I don't think other drivers should inherit that logic. -- Florian