From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758911Ab3EWTBr (ORCPT ); Thu, 23 May 2013 15:01:47 -0400 Received: from quartz.orcorp.ca ([184.70.90.242]:42401 "EHLO quartz.orcorp.ca" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757828Ab3EWTBp (ORCPT ); Thu, 23 May 2013 15:01:45 -0400 Date: Thu, 23 May 2013 13:01:40 -0600 From: Jason Gunthorpe To: Jason Cooper Cc: Andrew Lunn , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Benjamin Herrenschmidt , linuxppc-dev@lists.ozlabs.org, David Miller , Lennert Buytenhek , Sebastian Hesselbarth Subject: Re: [PATCH 2/2] net: mv643xx_eth: proper initialization for Kirkwood SoCs Message-ID: <20130523190140.GA4010@obsidianresearch.com> References: <1369154510-4927-1-git-send-email-sebastian.hesselbarth@gmail.com> <1369253042-15082-1-git-send-email-sebastian.hesselbarth@gmail.com> <1369253042-15082-2-git-send-email-sebastian.hesselbarth@gmail.com> <20130522201607.GA18823@obsidianresearch.com> <20130523160111.GP31290@titan.lakedaemon.net> <20130523171112.GB31281@obsidianresearch.com> <20130523172339.GQ31290@titan.lakedaemon.net> <20130523175357.GB2821@obsidianresearch.com> <20130523184028.GU31290@titan.lakedaemon.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130523184028.GU31290@titan.lakedaemon.net> User-Agent: Mutt/1.5.21 (2010-09-15) X-Broken-Reverse-DNS: no host name found for IP address 10.0.0.195 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 23, 2013 at 02:40:28PM -0400, Jason Cooper wrote: > > But there is a larger problem here then just this one bit. > > > > The PSC1 register must be set properly for the board layout, and today > > we rely on the bootloader to set it. In fact, even with Sebastian's > > change the ethernet port won't work without bootloader > > intervention. The PortReset bit should also be cleared by the driver > > (and it is only present on some variants of this IP block, > > apparently). > > > > We know that some Marvell SOC's wack the ethernet registers when they > > clock gate, and the flip of Clk125Bypass is another symptom of this > > general problem. > > > > So, long term, the PSC1 must be fully set by the driver, based on DT > > information describing the board (eg RGMII/MII/1000Base-X [SFP] Phy > > type), and the layout of this register seems to vary on a SOC by SOC > > basis. > > > > Thus, I think it is appropriate to call this variant of the eth IP > > 'marvell,kirkwood-eth' which indicates that the register block follows > > the kirkwood manual and the PSC1 register specifically has the > > kirkwood layout. > > Ok, so mv643xx_eth would match both "marvell,orion-eth" and > "marvell,kirkwood-eth", then write to PSC1 iff it sees a node matching > "marvell,kirkwood-eth". I'm not too keen on that, however, the matching > of the machine doesn't look to good, either. Why are you not keen on this? It seems like normal device driver practice, that is what the data field of of_device_id is typically used for.. There are more compatible strings than just kirkwood and orion in this driver, the whole TX_BW_CONTROL_OLD_LAYOUT/TX_BW_CONTROL_NEW_LAYOUT buisness (affecting PPC/MIPS) should also someday be captured with compatible strings rather than auto-detection too.. > > The question is what other Marvell SOCs have the same PSC1 layout as > > kirkwood? > > I think marvell,psc1_reset = <>; gives us the most flexibility in > accurately describing the hardware. Agree, providing psc1_reset value is a good idea to setup the phy modes. If all 'orion' SOCs have the PSC1 value then we don't need the kirkwood differentiators, especially if things like the reset bit are in the same place. The same trick Sebastian used to capture the mac address could be used to capture the PSC1 value from the bootloader. Basically, I think any IP variants that have idential register layouts can share a compatible string, otherwise different layouts need different compatible strings, so the general format: compatible = "marvell,SOCNAME-eth", "marvell,-eth"; Seems very sane to me. At least this way if we discover more changes then the driver can match on the SOCNAME compatible string to find them. = orion for TX_BW_CONTROL_NEW_LAYOUT variants also seems reasonable.. No idea what to call TX_BW_CONTROL_OLD_LAYOUT variants, or the PPC variants, not important right now it seems. (BTW, I wonder if the driver should ideally toggle PSC1 reset at some point????) Jason From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from quartz.orcorp.ca (quartz.orcorp.ca [184.70.90.242]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 1DBF72C02A8 for ; Fri, 24 May 2013 05:01:49 +1000 (EST) Date: Thu, 23 May 2013 13:01:40 -0600 From: Jason Gunthorpe To: Jason Cooper Subject: Re: [PATCH 2/2] net: mv643xx_eth: proper initialization for Kirkwood SoCs Message-ID: <20130523190140.GA4010@obsidianresearch.com> References: <1369154510-4927-1-git-send-email-sebastian.hesselbarth@gmail.com> <1369253042-15082-1-git-send-email-sebastian.hesselbarth@gmail.com> <1369253042-15082-2-git-send-email-sebastian.hesselbarth@gmail.com> <20130522201607.GA18823@obsidianresearch.com> <20130523160111.GP31290@titan.lakedaemon.net> <20130523171112.GB31281@obsidianresearch.com> <20130523172339.GQ31290@titan.lakedaemon.net> <20130523175357.GB2821@obsidianresearch.com> <20130523184028.GU31290@titan.lakedaemon.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <20130523184028.GU31290@titan.lakedaemon.net> Cc: Andrew Lunn , linux-kernel@vger.kernel.org, Lennert Buytenhek , netdev@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, David Miller , linux-arm-kernel@lists.infradead.org, Sebastian Hesselbarth List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Thu, May 23, 2013 at 02:40:28PM -0400, Jason Cooper wrote: > > But there is a larger problem here then just this one bit. > > > > The PSC1 register must be set properly for the board layout, and today > > we rely on the bootloader to set it. In fact, even with Sebastian's > > change the ethernet port won't work without bootloader > > intervention. The PortReset bit should also be cleared by the driver > > (and it is only present on some variants of this IP block, > > apparently). > > > > We know that some Marvell SOC's wack the ethernet registers when they > > clock gate, and the flip of Clk125Bypass is another symptom of this > > general problem. > > > > So, long term, the PSC1 must be fully set by the driver, based on DT > > information describing the board (eg RGMII/MII/1000Base-X [SFP] Phy > > type), and the layout of this register seems to vary on a SOC by SOC > > basis. > > > > Thus, I think it is appropriate to call this variant of the eth IP > > 'marvell,kirkwood-eth' which indicates that the register block follows > > the kirkwood manual and the PSC1 register specifically has the > > kirkwood layout. > > Ok, so mv643xx_eth would match both "marvell,orion-eth" and > "marvell,kirkwood-eth", then write to PSC1 iff it sees a node matching > "marvell,kirkwood-eth". I'm not too keen on that, however, the matching > of the machine doesn't look to good, either. Why are you not keen on this? It seems like normal device driver practice, that is what the data field of of_device_id is typically used for.. There are more compatible strings than just kirkwood and orion in this driver, the whole TX_BW_CONTROL_OLD_LAYOUT/TX_BW_CONTROL_NEW_LAYOUT buisness (affecting PPC/MIPS) should also someday be captured with compatible strings rather than auto-detection too.. > > The question is what other Marvell SOCs have the same PSC1 layout as > > kirkwood? > > I think marvell,psc1_reset = <>; gives us the most flexibility in > accurately describing the hardware. Agree, providing psc1_reset value is a good idea to setup the phy modes. If all 'orion' SOCs have the PSC1 value then we don't need the kirkwood differentiators, especially if things like the reset bit are in the same place. The same trick Sebastian used to capture the mac address could be used to capture the PSC1 value from the bootloader. Basically, I think any IP variants that have idential register layouts can share a compatible string, otherwise different layouts need different compatible strings, so the general format: compatible = "marvell,SOCNAME-eth", "marvell,-eth"; Seems very sane to me. At least this way if we discover more changes then the driver can match on the SOCNAME compatible string to find them. = orion for TX_BW_CONTROL_NEW_LAYOUT variants also seems reasonable.. No idea what to call TX_BW_CONTROL_OLD_LAYOUT variants, or the PPC variants, not important right now it seems. (BTW, I wonder if the driver should ideally toggle PSC1 reset at some point????) Jason From mboxrd@z Thu Jan 1 00:00:00 1970 From: jgunthorpe@obsidianresearch.com (Jason Gunthorpe) Date: Thu, 23 May 2013 13:01:40 -0600 Subject: [PATCH 2/2] net: mv643xx_eth: proper initialization for Kirkwood SoCs In-Reply-To: <20130523184028.GU31290@titan.lakedaemon.net> References: <1369154510-4927-1-git-send-email-sebastian.hesselbarth@gmail.com> <1369253042-15082-1-git-send-email-sebastian.hesselbarth@gmail.com> <1369253042-15082-2-git-send-email-sebastian.hesselbarth@gmail.com> <20130522201607.GA18823@obsidianresearch.com> <20130523160111.GP31290@titan.lakedaemon.net> <20130523171112.GB31281@obsidianresearch.com> <20130523172339.GQ31290@titan.lakedaemon.net> <20130523175357.GB2821@obsidianresearch.com> <20130523184028.GU31290@titan.lakedaemon.net> Message-ID: <20130523190140.GA4010@obsidianresearch.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, May 23, 2013 at 02:40:28PM -0400, Jason Cooper wrote: > > But there is a larger problem here then just this one bit. > > > > The PSC1 register must be set properly for the board layout, and today > > we rely on the bootloader to set it. In fact, even with Sebastian's > > change the ethernet port won't work without bootloader > > intervention. The PortReset bit should also be cleared by the driver > > (and it is only present on some variants of this IP block, > > apparently). > > > > We know that some Marvell SOC's wack the ethernet registers when they > > clock gate, and the flip of Clk125Bypass is another symptom of this > > general problem. > > > > So, long term, the PSC1 must be fully set by the driver, based on DT > > information describing the board (eg RGMII/MII/1000Base-X [SFP] Phy > > type), and the layout of this register seems to vary on a SOC by SOC > > basis. > > > > Thus, I think it is appropriate to call this variant of the eth IP > > 'marvell,kirkwood-eth' which indicates that the register block follows > > the kirkwood manual and the PSC1 register specifically has the > > kirkwood layout. > > Ok, so mv643xx_eth would match both "marvell,orion-eth" and > "marvell,kirkwood-eth", then write to PSC1 iff it sees a node matching > "marvell,kirkwood-eth". I'm not too keen on that, however, the matching > of the machine doesn't look to good, either. Why are you not keen on this? It seems like normal device driver practice, that is what the data field of of_device_id is typically used for.. There are more compatible strings than just kirkwood and orion in this driver, the whole TX_BW_CONTROL_OLD_LAYOUT/TX_BW_CONTROL_NEW_LAYOUT buisness (affecting PPC/MIPS) should also someday be captured with compatible strings rather than auto-detection too.. > > The question is what other Marvell SOCs have the same PSC1 layout as > > kirkwood? > > I think marvell,psc1_reset = <>; gives us the most flexibility in > accurately describing the hardware. Agree, providing psc1_reset value is a good idea to setup the phy modes. If all 'orion' SOCs have the PSC1 value then we don't need the kirkwood differentiators, especially if things like the reset bit are in the same place. The same trick Sebastian used to capture the mac address could be used to capture the PSC1 value from the bootloader. Basically, I think any IP variants that have idential register layouts can share a compatible string, otherwise different layouts need different compatible strings, so the general format: compatible = "marvell,SOCNAME-eth", "marvell,-eth"; Seems very sane to me. At least this way if we discover more changes then the driver can match on the SOCNAME compatible string to find them. = orion for TX_BW_CONTROL_NEW_LAYOUT variants also seems reasonable.. No idea what to call TX_BW_CONTROL_OLD_LAYOUT variants, or the PPC variants, not important right now it seems. (BTW, I wonder if the driver should ideally toggle PSC1 reset at some point????) Jason