Re: [RFC] NET: PHY: adds driver for Lantiq PHY11G

[Andrew Lunn]

> +static int lantiq_gphy_config_init(struct phy_device *phydev)
> +{
> + int err;
> +
> + /* Mask all interrupts */
> + err = phy_write(phydev, MII_VR9_11G_IMASK, 0);
> + if (err)
> + return err;
> +
> + /* Clear all pending interrupts */
> + phy_read(phydev, MII_VR9_11G_ISTAT);
> +
> + phy_write_mmd_indirect(phydev, 0x1e0, MDIO_MMD_VEND2, 0xc5);
> + phy_write_mmd_indirect(phydev, 0x1e1, MDIO_MMD_VEND2, 0x67);
> + phy_write_mmd_indirect(phydev, 0x1e2, MDIO_MMD_VEND2, 0x42);
> + phy_write_mmd_indirect(phydev, 0x1e3, MDIO_MMD_VEND2, 0x10);
> + phy_write_mmd_indirect(phydev, 0x1e4, MDIO_MMD_VEND2, 0x70);
> + phy_write_mmd_indirect(phydev, 0x1e5, MDIO_MMD_VEND2, 0x03);
> + phy_write_mmd_indirect(phydev, 0x1e6, MDIO_MMD_VEND2, 0x20);
> + phy_write_mmd_indirect(phydev, 0x1e7, MDIO_MMD_VEND2, 0x00);
> + phy_write_mmd_indirect(phydev, 0x1e8, MDIO_MMD_VEND2, 0x40);
> + phy_write_mmd_indirect(phydev, 0x1e9, MDIO_MMD_VEND2, 0x20);

Please could you use #define's rather than magic numbers. That helps
document what these writes are doing.

 Thanks
Andrew

Re: [RFC] NET: PHY: adds driver for Lantiq PHY11G

[Andrew Lunn]

> > I doubt the device tree maintainers will accept this. You don't normally put
> > register values in device tree.
> 
> Me too ;-)
> 

> I will look into this:
> http://lists.openwall.net/netdev/2016/03/23/61 and also try to make
> the device tree interface more generic.

Please take more notice of my emails than what was submitted.

> Should I first send a patch without the led configuration so this led stuff 
> is a separate topic?

Yes, you can do that. If you go the direction i suggests, that it a
big piece of work, and does not need to hold up the rest of the
driver.

Andrew

RE: [RFC] NET: PHY: adds driver for Lantiq PHY11G

[Mehrtens, Hauke]

> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Monday, May 23, 2016 4:42 AM
> To: Hauke Mehrtens <ha...@hauke-m.de>
> Cc: f.faine...@gmail.com; alexander.st...@systec-electronic.com;
> netdev@vger.kernel.org; j...@phrozen.org; open...@kresin.me;
> Mehrtens, Hauke <hauke.mehrt...@intel.com>
> Subject: Re: [RFC] NET: PHY: adds driver for Lantiq PHY11G
> 
> On Sun, May 22, 2016 at 09:33:51PM +0200, Hauke Mehrtens wrote:
> > Supports the Lantiq / Intel CHD 11G and 22E PHYs.
> > These PHYs are also named PEF 7061, PEF 7071, PEF 7072
> >
> > Signed-off-by: John Crispin <j...@phrozen.org>
> > Signed-off-by: Hauke Mehrtens <ha...@hauke-m.de>
> > ---
> >
> > This is based on a driver from OpenWrt / LEDE. This is send as a RFC
> > because the merge window is open now and it adds a new driver. This
> > patch was cleaned up on request of Alexander.
> >
> >
> >  .../devicetree/bindings/phy/phy-lanitq.txt | 216 +
> >  drivers/net/phy/Kconfig|   6 +
> >  drivers/net/phy/Makefile   |   1 +
> >  drivers/net/phy/lantiq.c   | 269 
> > +
> >  4 files changed, 492 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/phy/phy-lanitq.txt
> >  create mode 100644 drivers/net/phy/lantiq.c
> >
> > diff --git a/Documentation/devicetree/bindings/phy/phy-lanitq.txt
> > b/Documentation/devicetree/bindings/phy/phy-lanitq.txt
> > new file mode 100644
> > index 000..d9746e8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/phy-lanitq.txt
> > @@ -0,0 +1,216 @@
> > +Lanitq PHY binding
> > +
> > +
> > +This devicetree binding controls the lantiq ethernet phys led 
> > functionality.
> 
> Hi Hauke
> 
> You should CC: the device tree list.
> 
> > +
> > +Example:
> > +   mdio@0 {
> > +   #address-cells = <1>;
> > +   #size-cells = <0>;
> > +   compatible = "lantiq,xrx200-mdio";
> > +   phy5: ethernet-phy@5 {
> > +   reg = <0x1>;
> > +   compatible = "lantiq,phy11g", "ethernet-phy-
> ieee802.3-c22";
> > +   };
> > +   phy11: ethernet-phy@11 {
> > +   reg = <0x11>;
> > +   compatible = "lantiq,phy22f", "ethernet-phy-
> ieee802.3-c22";
> > +   lantiq,led2h = <0x00>;
> > +   lantiq,led2l = <0x03>;
> > +   };
> > +   phy12: ethernet-phy@12 {
> > +   reg = <0x12>;
> > +   compatible = "lantiq,phy22f", "ethernet-phy-
> ieee802.3-c22";
> > +   lantiq,led1h = <0x00>;
> > +   lantiq,led1l = <0x03>;
> > +   };
> > +   phy13: ethernet-phy@13 {
> > +   reg = <0x13>;
> > +   compatible = "lantiq,phy22f", "ethernet-phy-
> ieee802.3-c22";
> > +   lantiq,led2h = <0x00>;
> > +   lantiq,led2l = <0x03>;
> > +   };
> > +   phy14: ethernet-phy@14 {
> > +   reg = <0x14>;
> > +   compatible = "lantiq,phy22f", "ethernet-phy-
> ieee802.3-c22";
> > +   lantiq,led1h = <0x00>;
> > +   lantiq,led1l = <0x03>;
> > +   };
> > +   };
> > +
> > +Register Description
> > +
> > +
> > +LEDCH:
> > +
> > +Name   Hardware Reset Value
> > +LEDCH  0x00C5
> > +
> > +| 15 |||||||  8 |
> > +=
> > +|  RES |
> > +=
> > +
> > +|  7 |||||||  0 |
> > +=
> > +|   FBF   |   SBF   |RES | NACS |
> > +=
> > +
> > +Field  BitsTypeDescription
> > +FBC7:6 RW  Fast Blink Frequency
> > +   ---
> > +   0x0 (00b) F02HZ 2 Hz blinking frequency
> > +   0x1 (01b) F04HZ 4 Hz blinking f

Re: [RFC] NET: PHY: adds driver for Lantiq PHY11G

[Andrew Lunn]

On Sun, May 22, 2016 at 09:33:51PM +0200, Hauke Mehrtens wrote:
> Supports the Lantiq / Intel CHD 11G and 22E PHYs.
> These PHYs are also named PEF 7061, PEF 7071, PEF 7072
> 
> Signed-off-by: John Crispin 
> Signed-off-by: Hauke Mehrtens 
> ---
> 
> This is based on a driver from OpenWrt / LEDE. This is send as a RFC
> because the merge window is open now and it adds a new driver. This
> patch was cleaned up on request of Alexander.
> 
> 
>  .../devicetree/bindings/phy/phy-lanitq.txt | 216 +
>  drivers/net/phy/Kconfig|   6 +
>  drivers/net/phy/Makefile   |   1 +
>  drivers/net/phy/lantiq.c   | 269 
> +
>  4 files changed, 492 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/phy-lanitq.txt
>  create mode 100644 drivers/net/phy/lantiq.c
> 
> diff --git a/Documentation/devicetree/bindings/phy/phy-lanitq.txt 
> b/Documentation/devicetree/bindings/phy/phy-lanitq.txt
> new file mode 100644
> index 000..d9746e8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/phy-lanitq.txt
> @@ -0,0 +1,216 @@
> +Lanitq PHY binding
> +
> +
> +This devicetree binding controls the lantiq ethernet phys led functionality.

Hi Hauke

You should CC: the device tree list.

> +
> +Example:
> + mdio@0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "lantiq,xrx200-mdio";
> + phy5: ethernet-phy@5 {
> + reg = <0x1>;
> + compatible = "lantiq,phy11g", 
> "ethernet-phy-ieee802.3-c22";
> + };
> + phy11: ethernet-phy@11 {
> + reg = <0x11>;
> + compatible = "lantiq,phy22f", 
> "ethernet-phy-ieee802.3-c22";
> + lantiq,led2h = <0x00>;
> + lantiq,led2l = <0x03>;
> + };
> + phy12: ethernet-phy@12 {
> + reg = <0x12>;
> + compatible = "lantiq,phy22f", 
> "ethernet-phy-ieee802.3-c22";
> + lantiq,led1h = <0x00>;
> + lantiq,led1l = <0x03>;
> + };
> + phy13: ethernet-phy@13 {
> + reg = <0x13>;
> + compatible = "lantiq,phy22f", 
> "ethernet-phy-ieee802.3-c22";
> + lantiq,led2h = <0x00>;
> + lantiq,led2l = <0x03>;
> + };
> + phy14: ethernet-phy@14 {
> + reg = <0x14>;
> + compatible = "lantiq,phy22f", 
> "ethernet-phy-ieee802.3-c22";
> + lantiq,led1h = <0x00>;
> + lantiq,led1l = <0x03>;
> + };
> + };
> +
> +Register Description
> +
> +
> +LEDCH:
> +
> +Name Hardware Reset Value
> +LEDCH0x00C5
> +
> +| 15 |||||||  8 |
> +=
> +|RES |
> +=
> +
> +|  7 |||||||  0 |
> +=
> +|   FBF   |   SBF   |RES | NACS |
> +=
> +
> +FieldBitsTypeDescription
> +FBC  7:6 RW  Fast Blink Frequency
> + ---
> + 0x0 (00b) F02HZ 2 Hz blinking frequency
> + 0x1 (01b) F04HZ 4 Hz blinking frequency
> + 0x2 (10b) F08HZ 8 Hz blinking frequency
> + 0x3 (11b) F16HZ 16 Hz blinking frequency
> +
> +SBF  5:4 RW  Slow Blink Frequency
> + ---
> + 0x0 (00b) F02HZ 2 Hz blinking frequency
> + 0x1 (01b) F04HZ 4 Hz blinking frequency
> + 0x2 (10b) F08HZ 8 Hz blinking frequency
> + 0x3 (11b) F16HZ 16 Hz blinking frequency
> +
> +NACS 2:0 RW  Inverse of Scan Function
> + ---
> + 0x0 (000b) NONE No Function
> + 0x1 (001b) LINK Complex function enabled when link is up
> + 0x2 (010b) PDOWN Complex function enabled when device 
> is powered-down
> + 0x3 (011b) EEE Complex function enabled when device is 
> in EEE mode
> + 0x4 (100b) ANEG Complex function enabled when 
> auto-negotiation is running
> + 0x5 (101b) ABIST Complex function enabled when analog 
> self-test is running
> + 0x6 (110b) CDIAG Complex function enabled when cable 
> diagnostics are running
> + 0x7 (111b) TEST Complex function enabled when test mode 
> is running
> +
> +LEDCL:

I doubt the device tree maintainers will accept this. You don't
normally put 

Re: [RFC] NET: PHY: adds driver for Lantiq PHY11G

[Hauke Mehrtens]

On 05/22/2016 10:30 PM, Mathias Kresin wrote:
> Hi Hauke,
> 
> find my comments in-line.
> 
> Am 22.05.2016 um 21:33 schrieb Hauke Mehrtens:
>> Supports the Lantiq / Intel CHD 11G and 22E PHYs.
>> These PHYs are also named PEF 7061, PEF 7071, PEF 7072
>>
>> Signed-off-by: John Crispin 
>> Signed-off-by: Hauke Mehrtens 
>> ---
>>
>> This is based on a driver from OpenWrt / LEDE. This is send as a RFC
>> because the merge window is open now and it adds a new driver. This
>> patch was cleaned up on request of Alexander.
>>
>>
>>   .../devicetree/bindings/phy/phy-lanitq.txt | 216
>> +
> 
> Looks like a typo in the filename. lantiq != lanitq

Thanks for spotting this, I just copied it from OpenWrt. ;-)

> 
>>   drivers/net/phy/Kconfig|   6 +
>>   drivers/net/phy/Makefile   |   1 +
>>   drivers/net/phy/lantiq.c   | 269
>> +
>>   4 files changed, 492 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/phy/phy-lanitq.txt
>>   create mode 100644 drivers/net/phy/lantiq.c
>>
>> diff --git a/Documentation/devicetree/bindings/phy/phy-lanitq.txt
>> b/Documentation/devicetree/bindings/phy/phy-lanitq.txt
>> new file mode 100644
>> index 000..d9746e8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/phy-lanitq.txt
>> @@ -0,0 +1,216 @@
>> +Lanitq PHY binding
> 
> Same typo as mentioned above.
> 

Will change this too.

.

>> +
>> +static void lantiq_gphy_of_reg_init(struct phy_device *phydev)
>> +{
>> +struct device_node *node = phydev->mdio.dev.of_node;
>> +u32 tmp;
>> +
>> +if (!IS_ENABLED(CONFIG_OF_MDIO))
>> +return;
>> +
>> +/* store the led values if one was passed by the device tree */
>> +if (!of_property_read_u32(node, "lantiq,ledch", ))
>> +phy_write_mmd_indirect(phydev, 0x1e0, MDIO_MMD_VEND2, tmp);
>> +
>> +if (!of_property_read_u32(node, "lantiq,ledcl", ))
>> +phy_write_mmd_indirect(phydev, 0x1e1, MDIO_MMD_VEND2, tmp);
>> +
>> +if (!of_property_read_u32(node, "lantiq,led0h", ))
>> +phy_write_mmd_indirect(phydev, 0x1e2, MDIO_MMD_VEND2, tmp);
>> +
>> +if (!of_property_read_u32(node, "lantiq,led0l", ))
>> +phy_write_mmd_indirect(phydev, 0x1e3, MDIO_MMD_VEND2, tmp);
>> +
>> +if (!of_property_read_u32(node, "lantiq,led1h", ))
>> +phy_write_mmd_indirect(phydev, 0x1e4, MDIO_MMD_VEND2, tmp);
>> +
>> +if (!of_property_read_u32(node, "lantiq,led1l", ))
>> +phy_write_mmd_indirect(phydev, 0x1e5, MDIO_MMD_VEND2, tmp);
>> +
>> +if (!of_property_read_u32(node, "lantiq,led2h", ))
>> +phy_write_mmd_indirect(phydev, 0x1e6, MDIO_MMD_VEND2, tmp);
>> +
>> +if (!of_property_read_u32(node, "lantiq,led2l", ))
>> +phy_write_mmd_indirect(phydev, 0x1e7, MDIO_MMD_VEND2, tmp);
>> +
>> +/* The LED3 is only available in PEF 7072 package. */
>> +if (!of_property_read_u32(node, "lantiq,led3h", ))
>> +phy_write_mmd_indirect(phydev, 0x1e8, MDIO_MMD_VEND2, tmp);
>> +
>> +if (!of_property_read_u32(node, "lantiq,led3l", ))
>> +phy_write_mmd_indirect(phydev, 0x1e9, MDIO_MMD_VEND2, tmp);
>> +}
>> +
>> +static int lantiq_gphy_config_init(struct phy_device *phydev)
>> +{
>> +int err;
>> +
>> +/* Mask all interrupts */
>> +err = phy_write(phydev, MII_VR9_11G_IMASK, 0);
>> +if (err)
>> +return err;
>> +
>> +/* Clear all pending interrupts */
>> +phy_read(phydev, MII_VR9_11G_ISTAT);
>> +
>> +phy_write_mmd_indirect(phydev, 0x1e0, MDIO_MMD_VEND2, 0xc5);
>> +phy_write_mmd_indirect(phydev, 0x1e1, MDIO_MMD_VEND2, 0x67);
> 
> Any specific reason why the complex functions are enabled by default?

This is the same configuration the vendor SDK uses.

>> +phy_write_mmd_indirect(phydev, 0x1e2, MDIO_MMD_VEND2, 0x42);
>> +phy_write_mmd_indirect(phydev, 0x1e3, MDIO_MMD_VEND2, 0x10);
>> +phy_write_mmd_indirect(phydev, 0x1e4, MDIO_MMD_VEND2, 0x70);
>> +phy_write_mmd_indirect(phydev, 0x1e5, MDIO_MMD_VEND2, 0x03);
>> +phy_write_mmd_indirect(phydev, 0x1e6, MDIO_MMD_VEND2, 0x20);
>> +phy_write_mmd_indirect(phydev, 0x1e7, MDIO_MMD_VEND2, 0x00);
>> +phy_write_mmd_indirect(phydev, 0x1e8, MDIO_MMD_VEND2, 0x40);
>> +phy_write_mmd_indirect(phydev, 0x1e9, MDIO_MMD_VEND2, 0x20);
> 
> I would suggest to use the same blink/permanent on configuration for all
> led pins (as it is in LEDE/OpenWrt):
> 
> Constant On: 10/100/1000MBit
> Blink Fast: None
> Blink Slow: None
> Pulse: TX/RX
> 
> I'm aware of only one CPE that uses more than one led for status
> indication. All other have a single led attached to any of the pins.
> 
> This way it's only required to change the default configuration via the
> device tree bindings for the minority of the devices.

Ok, I am not aware on how all the boards are looking like. If most of
the boards only use on led it makes sense 

Re: [RFC] NET: PHY: adds driver for Lantiq PHY11G

[Mathias Kresin]

Hi Hauke,

find my comments in-line.

Am 22.05.2016 um 21:33 schrieb Hauke Mehrtens:

Supports the Lantiq / Intel CHD 11G and 22E PHYs.
These PHYs are also named PEF 7061, PEF 7071, PEF 7072

Signed-off-by: John Crispin 
Signed-off-by: Hauke Mehrtens 
---

This is based on a driver from OpenWrt / LEDE. This is send as a RFC
because the merge window is open now and it adds a new driver. This
patch was cleaned up on request of Alexander.


  .../devicetree/bindings/phy/phy-lanitq.txt | 216 +


Looks like a typo in the filename. lantiq != lanitq


  drivers/net/phy/Kconfig|   6 +
  drivers/net/phy/Makefile   |   1 +
  drivers/net/phy/lantiq.c   | 269 +
  4 files changed, 492 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/phy/phy-lanitq.txt
  create mode 100644 drivers/net/phy/lantiq.c

diff --git a/Documentation/devicetree/bindings/phy/phy-lanitq.txt 
b/Documentation/devicetree/bindings/phy/phy-lanitq.txt
new file mode 100644
index 000..d9746e8
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/phy-lanitq.txt
@@ -0,0 +1,216 @@
+Lanitq PHY binding


Same typo as mentioned above.


+
+
+This devicetree binding controls the lantiq ethernet phys led functionality.
+
+Example:
+   mdio@0 {
+   #address-cells = <1>;
+   #size-cells = <0>;
+   compatible = "lantiq,xrx200-mdio";
+   phy5: ethernet-phy@5 {
+   reg = <0x1>;
+   compatible = "lantiq,phy11g", 
"ethernet-phy-ieee802.3-c22";
+   };
+   phy11: ethernet-phy@11 {
+   reg = <0x11>;
+   compatible = "lantiq,phy22f", 
"ethernet-phy-ieee802.3-c22";
+   lantiq,led2h = <0x00>;
+   lantiq,led2l = <0x03>;
+   };
+   phy12: ethernet-phy@12 {
+   reg = <0x12>;
+   compatible = "lantiq,phy22f", 
"ethernet-phy-ieee802.3-c22";
+   lantiq,led1h = <0x00>;
+   lantiq,led1l = <0x03>;
+   };
+   phy13: ethernet-phy@13 {
+   reg = <0x13>;
+   compatible = "lantiq,phy22f", 
"ethernet-phy-ieee802.3-c22";
+   lantiq,led2h = <0x00>;
+   lantiq,led2l = <0x03>;
+   };
+   phy14: ethernet-phy@14 {
+   reg = <0x14>;
+   compatible = "lantiq,phy22f", 
"ethernet-phy-ieee802.3-c22";
+   lantiq,led1h = <0x00>;
+   lantiq,led1l = <0x03>;
+   };
+   };
+
+Register Description
+
+
+LEDCH:
+
+Name   Hardware Reset Value
+LEDCH  0x00C5
+
+| 15 |||||||  8 |
+=
+|  RES |
+=
+
+|  7 |||||||  0 |
+=
+|   FBF   |   SBF   |RES | NACS |
+=
+
+Field  BitsTypeDescription
+FBC7:6 RW  Fast Blink Frequency
+   ---
+   0x0 (00b) F02HZ 2 Hz blinking frequency
+   0x1 (01b) F04HZ 4 Hz blinking frequency
+   0x2 (10b) F08HZ 8 Hz blinking frequency
+   0x3 (11b) F16HZ 16 Hz blinking frequency
+
+SBF5:4 RW  Slow Blink Frequency
+   ---
+   0x0 (00b) F02HZ 2 Hz blinking frequency
+   0x1 (01b) F04HZ 4 Hz blinking frequency
+   0x2 (10b) F08HZ 8 Hz blinking frequency
+   0x3 (11b) F16HZ 16 Hz blinking frequency
+
+NACS   2:0 RW  Inverse of Scan Function
+   ---
+   0x0 (000b) NONE No Function
+   0x1 (001b) LINK Complex function enabled when link is up
+   0x2 (010b) PDOWN Complex function enabled when device 
is powered-down
+   0x3 (011b) EEE Complex function enabled when device is 
in EEE mode
+   0x4 (100b) ANEG Complex function enabled when 
auto-negotiation is running
+   0x5 (101b) ABIST Complex function enabled when analog 
self-test is running
+   0x6 (110b) CDIAG Complex function enabled when cable 
diagnostics are running
+   0x7 (111b) TEST Complex function enabled when test mode 
is running
+
+LEDCL:
+
+Name   Hardware Reset Value
+LEDCL  0x0067
+
+| 15 |||||||  8 |
+=
+|  RES