[{"id":1762049,"web_url":"http://patchwork.ozlabs.org/comment/1762049/","msgid":"<816adc32-9df6-3433-b30c-79490a6328d1@gmail.com>","list_archive_url":null,"date":"2017-09-02T00:35:37","subject":"Re: [PATCH] net: ethernet: ibm-emac: Add 5482 PHY init for\n\tOpenBlocks 600","submitter":{"id":2800,"url":"http://patchwork.ozlabs.org/api/people/2800/","name":"Florian Fainelli","email":"f.fainelli@gmail.com"},"content":"On 08/31/2017 09:44 PM, Benjamin Herrenschmidt wrote:\n> The vendor patches initialize those registers to get the\n> PHY working properly.\n> \n> Sadly I don't have that PHY spec and whatever Broadcom PHY\n> code we already have don't seem to document these two shadow\n> registers (unless I miscalculated the address) so I'm keeping\n> this as \"vendor magic for that board\". The vendor has long\n> abandoned that product, but I find it handy to test ppc405\n> kernels and so would like to keep it alive upstream :-)\n> \n> Signed-off-by: Benjamin Herrenschmidt \n> ---\n> \n> Note: Ideally, the whole driver should switch over to the\n> generic PHY layer. However this is a much bigger undertaking\n> which requires access to a bunch of HW to test, and for which\n> I have neither the time nor the HW available these days.\n\nYes it sure does and the function names are so close, it is almost\nirresistible not to do it.\n\n> \n> (Some of the HW could prove hard to find ...)\n> ---\n> drivers/net/ethernet/ibm/emac/phy.c | 30 ++++++++++++++++++++++++++++++\n> 1 file changed, 30 insertions(+)\n> \n> diff --git a/drivers/net/ethernet/ibm/emac/phy.c b/drivers/net/ethernet/ibm/emac/phy.c\n> index 35865d05fccd..daa10de542fb 100644\n> --- a/drivers/net/ethernet/ibm/emac/phy.c\n> +++ b/drivers/net/ethernet/ibm/emac/phy.c\n> @@ -24,6 +24,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> #include \"emac.h\"\n> #include \"phy.h\"\n> @@ -363,6 +364,34 @@ static struct mii_phy_def bcm5248_phy_def = {\n> \t.ops\t\t= &generic_phy_ops\n> };\n> \n> +static int bcm5482_init(struct mii_phy *phy)\n> +{\n> +\tif (!of_machine_is_compatible(\"plathome,obs600\"))\n> +\t\treturn 0;\n\nYou can probably include brcmphy.h and pull the definition for at least\n0x1c: MII_BCM54XX_SHD\n\n> +\n> +\t/* Magic inits from vendor original patches */\n> +\tphy_write(phy, 0x1c, 0xa410);\n\nWhat you are doing here is write to shadow register 9 (9 << 10) which is\nthe LED control register, and making the activity LED be driven on\nactivity/link as opposed to just activity. So this can probably be\nwritten as:\n\n\tphy_write(phy, MII_BCM54XX_SHD, MII_BCM54XX_SHD_WRITE |\n\t\t MII_BCM54XX_SHD_VAL(9) | MII_BCM54XX_SHD_DATA(BIT(4));\n\n> +\tphy_write(phy, 0x1c, 0x8804);\n\nAnd here you are writing to the spare control 1 register and setting bit\n2 (which appears reserved but this is not clear) which would be enabling\nthe activity LED for 10BaseT or no link which can be written as:\n\n\tphy_write(phy, MII_BCM54XX_SHD, MII_BCM54XX_SHD_WRITE |\n\t\t MII_BCM54XX_SHD_VAL(2) | MII_BCM4XX_SHD_DATA(BIT(2));\n\nSo basically you are touching registers that only affect LED\nconfiguration and should not be doing anything else...\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +static const struct mii_phy_ops bcm5482_phy_ops = {\n> +\t.init\t\t= bcm5482_init,\n> +\t.setup_aneg\t= genmii_setup_aneg,\n> +\t.setup_forced\t= genmii_setup_forced,\n> +\t.poll_link\t= genmii_poll_link,\n> +\t.read_link\t= genmii_read_link\n> +};\n> +\n> +static struct mii_phy_def bcm5482_phy_def = {\n> +\n> +\t.phy_id\t\t= 0x0143bcb0,\n> +\t.phy_id_mask\t= 0x0ffffff0,\n> +\t.name\t\t= \"BCM5482 Gigabit Ethernet\",\n> +\t.ops\t\t= &bcm5482_phy_ops\n> +};\n> +\n> static int m88e1111_init(struct mii_phy *phy)\n> {\n> \tpr_debug(\"%s: Marvell 88E1111 Ethernet\\n\", __func__);\n> @@ -499,6 +528,7 @@ static struct mii_phy_def *mii_phy_table[] = {\n> \t&et1011c_phy_def,\n> \t&cis8201_phy_def,\n> \t&bcm5248_phy_def,\n> +\t&bcm5482_phy_def,\n> \t&m88e1111_phy_def,\n> \t&m88e1112_phy_def,\n> \t&ar8035_phy_def,\n>","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"TFtQzpkG\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xkcbT1cbGz9sPt\n\tfor ;\n\tSat, 2 Sep 2017 10:35:49 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752651AbdIBAfp (ORCPT );\n\tFri, 1 Sep 2017 20:35:45 -0400","from mail-qk0-f193.google.com ([209.85.220.193]:36734 \"EHLO\n\tmail-qk0-f193.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752642AbdIBAfo (ORCPT\n\t); Fri, 1 Sep 2017 20:35:44 -0400","by mail-qk0-f193.google.com with SMTP id l65so1237511qkc.3\n\tfor ; Fri, 01 Sep 2017 17:35:43 -0700 (PDT)","from [10.112.156.244] ([192.19.255.250])\n\tby smtp.googlemail.com with ESMTPSA id\n\ti51sm1103817qta.75.2017.09.01.17.35.40\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tFri, 01 Sep 2017 17:35:41 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=subject:to:references:from:message-id:date:user-agent:mime-version\n\t:in-reply-to:content-language:content-transfer-encoding;\n\tbh=QOtZnoWfkZII5332uDVmX2BnkpplPIYRGQkVz/u0K0U=;\n\tb=TFtQzpkGu40/+A4POCpYsmfemq1H/b+5dfWry1yU7MrNNfAAtNCuu+RavkOkO9HM2F\n\ti/C8sAomapvZz9X75oYWB84K4uPxcbh4xep+0iqV9RzMTOc+QvchsnqflJHxINvjVgHz\n\tyXaKzhajPHu1P//WFEUCLEhRMWB90R5A7xPjJGZf97RaNz6+CCqjp7qeb8xdkzd0bMFa\n\t8ywDDCaWJAY/ph9nUf0WEDLMnKK4ryn2XXCeqBRWo98GldsFMEmGlYwWadxuqTefka/V\n\t5snp3LbdQ7H4bXbG9zMOuEA5VFQXjW1PtDlbZwlMWKE4B45T97zKhmFKpnIb3pPktDFW\n\tNtdQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=QOtZnoWfkZII5332uDVmX2BnkpplPIYRGQkVz/u0K0U=;\n\tb=D+i7IZ/j8ToGQSoynn0jLOzpZ87J/NiMGj0gCqUC5sYqP1JkAEh/FYmlougSJkUQyi\n\t2gJ7p9i1qVkLmPlVVcnfi7MltVsfWVeNPS081Hu7joXHIeNzozKfIIzTja0VjxdLjkgb\n\t0B0oT5Zy2yvWJ+YFPzxQ4YDZb0i0p/Yw0UH5TX+fSk+SqDuCYs44MJBRK1oF9ji5+i9D\n\tb2X68J9kZjcj1vBp23IwmHOQkNtHi4TihLNJjx4lpKJrBhXOJGjLUhkuLIx0gHogOvNq\n\t2eldBvU2OMrKWK2vdwd5W7DFmIOsJVFswNIWbWqRxytYr/oGV/zWmN/6owiRuVsJShHG\n\trfLg==","X-Gm-Message-State":"AHPjjUgPUL5PK9NX9pKdi8aJ4XnSh+BQnMLOJSWt3lVwK2mGpNKfWtyL\n\tke3rxPIM/zMrjZHXuTo=","X-Google-Smtp-Source":"ADKCNb6eF7AsH2b4OhydwxI+qmnCzKUz6bGM0jLLMJYUDlvtgfciaAROLf7B/eyai/iRZyD8rMcFUQ==","X-Received":"by 10.55.97.201 with SMTP id v192mr554127qkb.153.1504312543123; \n\tFri, 01 Sep 2017 17:35:43 -0700 (PDT)","Subject":"Re: [PATCH] net: ethernet: ibm-emac: Add 5482 PHY init for\n\tOpenBlocks 600","To":"Benjamin Herrenschmidt , netdev@vger.kernel.org","References":"<1504241089.4974.67.camel@kernel.crashing.org>","From":"Florian Fainelli ","Message-ID":"<816adc32-9df6-3433-b30c-79490a6328d1@gmail.com>","Date":"Fri, 1 Sep 2017 17:35:37 -0700","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<1504241089.4974.67.camel@kernel.crashing.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1762051,"web_url":"http://patchwork.ozlabs.org/comment/1762051/","msgid":"<1504315323.4974.98.camel@kernel.crashing.org>","list_archive_url":null,"date":"2017-09-02T01:22:03","subject":"Re: [PATCH] net: ethernet: ibm-emac: Add 5482 PHY init for\n\tOpenBlocks 600","submitter":{"id":38,"url":"http://patchwork.ozlabs.org/api/people/38/","name":"Benjamin Herrenschmidt","email":"benh@kernel.crashing.org"},"content":"On Fri, 2017-09-01 at 17:35 -0700, Florian Fainelli wrote:\n> On 08/31/2017 09:44 PM, Benjamin Herrenschmidt wrote:\n> > The vendor patches initialize those registers to get the\n> > PHY working properly.\n> > \n> > Sadly I don't have that PHY spec and whatever Broadcom PHY\n> > code we already have don't seem to document these two shadow\n> > registers (unless I miscalculated the address) so I'm keeping\n> > this as \"vendor magic for that board\". The vendor has long\n> > abandoned that product, but I find it handy to test ppc405\n> > kernels and so would like to keep it alive upstream :-)\n> > \n> > Signed-off-by: Benjamin Herrenschmidt \n> > ---\n> > \n> > Note: Ideally, the whole driver should switch over to the\n> > generic PHY layer. However this is a much bigger undertaking\n> > which requires access to a bunch of HW to test, and for which\n> > I have neither the time nor the HW available these days.\n> \n> Yes it sure does and the function names are so close, it is almost\n> irresistible not to do it.\n\nI think there's some common ancestry :-)\n\nThat said, I'm weary of doing it without proper testing, especially\nthose old cell blades which I'm not sure I still have a functional\none, and whatever is using gpcs...\n\nCheers,\nBen.\n\n> \n> > \n> > (Some of the HW could prove hard to find ...)\n> > ---\n> > drivers/net/ethernet/ibm/emac/phy.c | 30 ++++++++++++++++++++++++++++++\n> > 1 file changed, 30 insertions(+)\n> > \n> > diff --git a/drivers/net/ethernet/ibm/emac/phy.c b/drivers/net/ethernet/ibm/emac/phy.c\n> > index 35865d05fccd..daa10de542fb 100644\n> > --- a/drivers/net/ethernet/ibm/emac/phy.c\n> > +++ b/drivers/net/ethernet/ibm/emac/phy.c\n> > @@ -24,6 +24,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > \n> > #include \"emac.h\"\n> > #include \"phy.h\"\n> > @@ -363,6 +364,34 @@ static struct mii_phy_def bcm5248_phy_def = {\n> > \t.ops\t\t= &generic_phy_ops\n> > };\n> > \n> > +static int bcm5482_init(struct mii_phy *phy)\n> > +{\n> > +\tif (!of_machine_is_compatible(\"plathome,obs600\"))\n> > +\t\treturn 0;\n> \n> You can probably include brcmphy.h and pull the definition for at least\n> 0x1c: MII_BCM54XX_SHD\n\nYup.\n\n> > +\n> > +\t/* Magic inits from vendor original patches */\n> > +\tphy_write(phy, 0x1c, 0xa410);\n> \n> What you are doing here is write to shadow register 9 (9 << 10) which is\n> the LED control register, and making the activity LED be driven on\n> activity/link as opposed to just activity. So this can probably be\n> written as:\n\nOk so I really don't *need* that in fact.\n\n\n> \tphy_write(phy, MII_BCM54XX_SHD, MII_BCM54XX_SHD_WRITE |\n> \t\t MII_BCM54XX_SHD_VAL(9) | MII_BCM54XX_SHD_DATA(BIT(4));\n> \n> > +\tphy_write(phy, 0x1c, 0x8804);\n> \n> And here you are writing to the spare control 1 register and setting bit\n> 2 (which appears reserved but this is not clear) which would be enabling\n> the activity LED for 10BaseT or no link which can be written as:\n> \n> \tphy_write(phy, MII_BCM54XX_SHD, MII_BCM54XX_SHD_WRITE |\n> \t\t MII_BCM54XX_SHD_VAL(2) | MII_BCM4XX_SHD_DATA(BIT(2));\n> \n> So basically you are touching registers that only affect LED\n> configuration and should not be doing anything else...\n\nI wonder if I need to bother at all then. I was worried it was related\nto actual function of the device, but if it's just LEDs, I think I may\nas well just drop it.\n\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +static const struct mii_phy_ops bcm5482_phy_ops = {\n> > +\t.init\t\t= bcm5482_init,\n> > +\t.setup_aneg\t= genmii_setup_aneg,\n> > +\t.setup_forced\t= genmii_setup_forced,\n> > +\t.poll_link\t= genmii_poll_link,\n> > +\t.read_link\t= genmii_read_link\n> > +};\n> > +\n> > +static struct mii_phy_def bcm5482_phy_def = {\n> > +\n> > +\t.phy_id\t\t= 0x0143bcb0,\n> > +\t.phy_id_mask\t= 0x0ffffff0,\n> > +\t.name\t\t= \"BCM5482 Gigabit Ethernet\",\n> > +\t.ops\t\t= &bcm5482_phy_ops\n> > +};\n> > +\n> > static int m88e1111_init(struct mii_phy *phy)\n> > {\n> > \tpr_debug(\"%s: Marvell 88E1111 Ethernet\\n\", __func__);\n> > @@ -499,6 +528,7 @@ static struct mii_phy_def *mii_phy_table[] = {\n> > \t&et1011c_phy_def,\n> > \t&cis8201_phy_def,\n> > \t&bcm5248_phy_def,\n> > +\t&bcm5482_phy_def,\n> > \t&m88e1111_phy_def,\n> > \t&m88e1112_phy_def,\n> > \t&ar8035_phy_def,\n> > \n> \n>","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xkdd336gwz9sMN\n\tfor ;\n\tSat, 2 Sep 2017 11:22:15 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751400AbdIBBWJ (ORCPT );\n\tFri, 1 Sep 2017 21:22:09 -0400","from gate.crashing.org ([63.228.1.57]:58255 \"EHLO\n\tgate.crashing.org\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1750941AbdIBBWI (ORCPT );\n\tFri, 1 Sep 2017 21:22:08 -0400","from localhost (localhost.localdomain [127.0.0.1])\n\tby gate.crashing.org (8.14.1/8.13.8) with ESMTP id v821M4hY010343;\n\tFri, 1 Sep 2017 20:22:05 -0500"],"Message-ID":"<1504315323.4974.98.camel@kernel.crashing.org>","Subject":"Re: [PATCH] net: ethernet: ibm-emac: Add 5482 PHY init for\n\tOpenBlocks 600","From":"Benjamin Herrenschmidt ","To":"Florian Fainelli , netdev@vger.kernel.org","Date":"Sat, 02 Sep 2017 11:22:03 +1000","In-Reply-To":"<816adc32-9df6-3433-b30c-79490a6328d1@gmail.com>","References":"<1504241089.4974.67.camel@kernel.crashing.org>\n\t<816adc32-9df6-3433-b30c-79490a6328d1@gmail.com>","Content-Type":"text/plain; charset=\"UTF-8\"","X-Mailer":"Evolution 3.24.5 (3.24.5-1.fc26) ","Mime-Version":"1.0","Content-Transfer-Encoding":"7bit","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}}]