Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Murali Karicheri]

On 10/20/2015 04:24 AM, Arnd Bergmann wrote:

On Monday 19 October 2015 14:50:57 Murali Karicheri wrote:

Arnd, by your statement 'I don't see any code that does this' do you
expect a piece of code that embed the license in the binary image? If
so, that seems weired to me.

Many of the drivers including this patch has the following statement in
the license that is additional company specific license such as BSD that
is applicable.

 Cut and pasted from drivers/crypto/fcrypt.c ===
   * 2. Redistributions in binary form must reproduce the above copyright
   *notice, this list of conditions and the following disclaimer in the
   *documentation and/or other materials provided with the distribution.
=
I read this as if the source is compiled and distributed as a binary
either a kernel module ko file or as part of the kernel binary, this
term must apply. Usually this is part of documentation that goes with
the product AFAIK.


Sorry, my fault. This is indeed the standard BSD license, and
I misread this as having to produce the copyright statement
from the binary itself. Please ignore whatever I said on the
subject.

Arnd


Thanks Arnd for the clarification.

--
Murali Karicheri
Linux Kernel, Keystone
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Arnd Bergmann]

On Monday 19 October 2015 14:50:57 Murali Karicheri wrote:
> Arnd, by your statement 'I don't see any code that does this' do you 
> expect a piece of code that embed the license in the binary image? If 
> so, that seems weired to me.
> 
> Many of the drivers including this patch has the following statement in 
> the license that is additional company specific license such as BSD that 
> is applicable.
> 
>  Cut and pasted from drivers/crypto/fcrypt.c ===
>   * 2. Redistributions in binary form must reproduce the above copyright
>   *notice, this list of conditions and the following disclaimer in the
>   *documentation and/or other materials provided with the distribution.
> =
> I read this as if the source is compiled and distributed as a binary 
> either a kernel module ko file or as part of the kernel binary, this 
> term must apply. Usually this is part of documentation that goes with 
> the product AFAIK.

Sorry, my fault. This is indeed the standard BSD license, and
I misread this as having to produce the copyright statement
from the binary itself. Please ignore whatever I said on the
subject.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Arnd Bergmann]

On Monday 19 October 2015 14:50:57 Murali Karicheri wrote:
> Arnd, by your statement 'I don't see any code that does this' do you 
> expect a piece of code that embed the license in the binary image? If 
> so, that seems weired to me.
> 
> Many of the drivers including this patch has the following statement in 
> the license that is additional company specific license such as BSD that 
> is applicable.
> 
>  Cut and pasted from drivers/crypto/fcrypt.c ===
>   * 2. Redistributions in binary form must reproduce the above copyright
>   *notice, this list of conditions and the following disclaimer in the
>   *documentation and/or other materials provided with the distribution.
> =
> I read this as if the source is compiled and distributed as a binary 
> either a kernel module ko file or as part of the kernel binary, this 
> term must apply. Usually this is part of documentation that goes with 
> the product AFAIK.

Sorry, my fault. This is indeed the standard BSD license, and
I misread this as having to produce the copyright statement
from the binary itself. Please ignore whatever I said on the
subject.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Murali Karicheri]

On 10/20/2015 04:24 AM, Arnd Bergmann wrote:

On Monday 19 October 2015 14:50:57 Murali Karicheri wrote:

Arnd, by your statement 'I don't see any code that does this' do you
expect a piece of code that embed the license in the binary image? If
so, that seems weired to me.

Many of the drivers including this patch has the following statement in
the license that is additional company specific license such as BSD that
is applicable.

 Cut and pasted from drivers/crypto/fcrypt.c ===
   * 2. Redistributions in binary form must reproduce the above copyright
   *notice, this list of conditions and the following disclaimer in the
   *documentation and/or other materials provided with the distribution.
=
I read this as if the source is compiled and distributed as a binary
either a kernel module ko file or as part of the kernel binary, this
term must apply. Usually this is part of documentation that goes with
the product AFAIK.


Sorry, my fault. This is indeed the standard BSD license, and
I misread this as having to produce the copyright statement
from the binary itself. Please ignore whatever I said on the
subject.

Arnd


Thanks Arnd for the clarification.

--
Murali Karicheri
Linux Kernel, Keystone
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Murali Karicheri]

On 10/19/2015 10:47 AM, Kwok, WingMan wrote:

Hi,


-Original Message-
From: Arnd Bergmann [mailto:a...@arndb.de]
Sent: Thursday, October 15, 2015 3:35 PM
To: Karicheri, Muralidharan
Cc: linux-arm-ker...@lists.infradead.org; Kwok, WingMan; robh...@kernel.org;
pawel.m...@arm.com; mark.rutl...@arm.com; ijc+devicet...@hellion.org.uk;
ga...@codeaurora.org; KISHON VIJAY ABRAHAM; Quadros, Roger;
bhelg...@google.com; ssant...@kernel.org; li...@arm.linux.org.uk;
devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
p...@vger.kernel.org
Subject: Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and
pcie

On Thursday 15 October 2015 12:01:04 Murali Karicheri wrote:



+ * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the
+ * distribution.


The current code does not do this when compiled, which might be a
problem for distributors. Can you clarify the license?


Arnd,

Can you elaborate on this? I did a grep on the string "Redistributions
in binary form must reproduce the above copyright" and I could find
several instance of this. So I am not sure what you mean by "The current
code does not do this when compiled".


You write that the binary form of the code must produce the copyright
notice. I don't see any code that does this. If I was looking in the
wrong place, let me know.

Arnd




Thanks Wingman for the response.

Arnd, by your statement 'I don't see any code that does this' do you 
expect a piece of code that embed the license in the binary image? If 
so, that seems weired to me.


Many of the drivers including this patch has the following statement in 
the license that is additional company specific license such as BSD that 
is applicable.


 Cut and pasted from drivers/crypto/fcrypt.c ===
 * 2. Redistributions in binary form must reproduce the above copyright
 *notice, this list of conditions and the following disclaimer in the
 *documentation and/or other materials provided with the distribution.
=
I read this as if the source is compiled and distributed as a binary 
either a kernel module ko file or as part of the kernel binary, this 
term must apply. Usually this is part of documentation that goes with 
the product AFAIK.


Murali


For example, we did a grep of the following

mypc:linux(personal/linux/serdes) $ grep -rnI "Redistributions in binary form must 
reproduce the above copyright" ./net/
./net/sunrpc/auth_gss/auth_gss.c:18: *  2. Redistributions in binary form must 
reproduce the above copyright
./net/sunrpc/auth_gss/gss_mech_switch.c:15: *  2. Redistributions in binary 
form must reproduce the above copyright
./net/sunrpc/auth_gss/gss_krb5_mech.c:16: *  2. Redistributions in binary form 
must reproduce the above copyright
./net/bluetooth/ecc.c:10: *  * Redistributions in binary form must reproduce 
the above copyright
./net/bluetooth/ecc.h:10: *  * Redistributions in binary form must reproduce 
the above copyright
./net/can/gw.c:12: * 2. Redistributions in binary form must reproduce the above 
copyright
./net/can/af_can.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/can/proc.c:12: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/can/bcm.c:12: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/can/raw.c:12: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/can/af_can.h:10: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/discover.c:13: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/node.h:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/netlink_compat.c:10: * 2. Redistributions in binary form must 
reproduce the above copyright
./net/tipc/name_distr.h:13: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/bearer.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/name_table.h:13: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/name_distr.c:13: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/addr.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/subscr.h:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/link.h:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/net.h:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/netlink.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/sysctl.c:12: * 2. Redistributions in binary form must reprod

RE: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Kwok, WingMan]

Hi,

> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Thursday, October 15, 2015 3:35 PM
> To: Karicheri, Muralidharan
> Cc: linux-arm-ker...@lists.infradead.org; Kwok, WingMan; robh...@kernel.org;
> pawel.m...@arm.com; mark.rutl...@arm.com; ijc+devicet...@hellion.org.uk;
> ga...@codeaurora.org; KISHON VIJAY ABRAHAM; Quadros, Roger;
> bhelg...@google.com; ssant...@kernel.org; li...@arm.linux.org.uk;
> devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> p...@vger.kernel.org
> Subject: Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and
> pcie
> 
> On Thursday 15 October 2015 12:01:04 Murali Karicheri wrote:
> >
> > >> + * Redistributions in binary form must reproduce the above copyright
> > >> + * notice, this list of conditions and the following disclaimer in the
> > >> + * documentation and/or other materials provided with the
> > >> + * distribution.
> > >
> > > The current code does not do this when compiled, which might be a
> > > problem for distributors. Can you clarify the license?
> > >
> > Arnd,
> >
> > Can you elaborate on this? I did a grep on the string "Redistributions
> > in binary form must reproduce the above copyright" and I could find
> > several instance of this. So I am not sure what you mean by "The current
> > code does not do this when compiled".
> 
> You write that the binary form of the code must produce the copyright
> notice. I don't see any code that does this. If I was looking in the
> wrong place, let me know.
> 
>   Arnd

For example, we did a grep of the following 

mypc:linux(personal/linux/serdes) $ grep -rnI "Redistributions in binary form 
must reproduce the above copyright" ./net/
./net/sunrpc/auth_gss/auth_gss.c:18: *  2. Redistributions in binary form must 
reproduce the above copyright
./net/sunrpc/auth_gss/gss_mech_switch.c:15: *  2. Redistributions in binary 
form must reproduce the above copyright
./net/sunrpc/auth_gss/gss_krb5_mech.c:16: *  2. Redistributions in binary form 
must reproduce the above copyright
./net/bluetooth/ecc.c:10: *  * Redistributions in binary form must reproduce 
the above copyright
./net/bluetooth/ecc.h:10: *  * Redistributions in binary form must reproduce 
the above copyright
./net/can/gw.c:12: * 2. Redistributions in binary form must reproduce the above 
copyright
./net/can/af_can.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/can/proc.c:12: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/can/bcm.c:12: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/can/raw.c:12: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/can/af_can.h:10: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/discover.c:13: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/node.h:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/netlink_compat.c:10: * 2. Redistributions in binary form must 
reproduce the above copyright
./net/tipc/name_distr.h:13: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/bearer.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/name_table.h:13: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/name_distr.c:13: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/addr.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/subscr.h:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/link.h:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/net.h:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/netlink.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/sysctl.c:12: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/udp_media.c:11: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/socket.h:11: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/subscr.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/msg.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/name_table.c:13: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/msg.h:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/core.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/socket.c:13: * 2. Redistr

RE: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Kwok, WingMan]

Hi,

> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Thursday, October 15, 2015 3:35 PM
> To: Karicheri, Muralidharan
> Cc: linux-arm-ker...@lists.infradead.org; Kwok, WingMan; robh...@kernel.org;
> pawel.m...@arm.com; mark.rutl...@arm.com; ijc+devicet...@hellion.org.uk;
> ga...@codeaurora.org; KISHON VIJAY ABRAHAM; Quadros, Roger;
> bhelg...@google.com; ssant...@kernel.org; li...@arm.linux.org.uk;
> devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> p...@vger.kernel.org
> Subject: Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and
> pcie
> 
> On Thursday 15 October 2015 12:01:04 Murali Karicheri wrote:
> >
> > >> + * Redistributions in binary form must reproduce the above copyright
> > >> + * notice, this list of conditions and the following disclaimer in the
> > >> + * documentation and/or other materials provided with the
> > >> + * distribution.
> > >
> > > The current code does not do this when compiled, which might be a
> > > problem for distributors. Can you clarify the license?
> > >
> > Arnd,
> >
> > Can you elaborate on this? I did a grep on the string "Redistributions
> > in binary form must reproduce the above copyright" and I could find
> > several instance of this. So I am not sure what you mean by "The current
> > code does not do this when compiled".
> 
> You write that the binary form of the code must produce the copyright
> notice. I don't see any code that does this. If I was looking in the
> wrong place, let me know.
> 
>   Arnd

For example, we did a grep of the following 

mypc:linux(personal/linux/serdes) $ grep -rnI "Redistributions in binary form 
must reproduce the above copyright" ./net/
./net/sunrpc/auth_gss/auth_gss.c:18: *  2. Redistributions in binary form must 
reproduce the above copyright
./net/sunrpc/auth_gss/gss_mech_switch.c:15: *  2. Redistributions in binary 
form must reproduce the above copyright
./net/sunrpc/auth_gss/gss_krb5_mech.c:16: *  2. Redistributions in binary form 
must reproduce the above copyright
./net/bluetooth/ecc.c:10: *  * Redistributions in binary form must reproduce 
the above copyright
./net/bluetooth/ecc.h:10: *  * Redistributions in binary form must reproduce 
the above copyright
./net/can/gw.c:12: * 2. Redistributions in binary form must reproduce the above 
copyright
./net/can/af_can.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/can/proc.c:12: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/can/bcm.c:12: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/can/raw.c:12: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/can/af_can.h:10: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/discover.c:13: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/node.h:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/netlink_compat.c:10: * 2. Redistributions in binary form must 
reproduce the above copyright
./net/tipc/name_distr.h:13: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/bearer.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/name_table.h:13: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/name_distr.c:13: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/addr.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/subscr.h:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/link.h:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/net.h:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/netlink.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/sysctl.c:12: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/udp_media.c:11: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/socket.h:11: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/subscr.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/msg.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/name_table.c:13: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/msg.h:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/core.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/socket.c:13: * 2. Redistr

Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Murali Karicheri]

On 10/19/2015 10:47 AM, Kwok, WingMan wrote:

Hi,


-Original Message-
From: Arnd Bergmann [mailto:a...@arndb.de]
Sent: Thursday, October 15, 2015 3:35 PM
To: Karicheri, Muralidharan
Cc: linux-arm-ker...@lists.infradead.org; Kwok, WingMan; robh...@kernel.org;
pawel.m...@arm.com; mark.rutl...@arm.com; ijc+devicet...@hellion.org.uk;
ga...@codeaurora.org; KISHON VIJAY ABRAHAM; Quadros, Roger;
bhelg...@google.com; ssant...@kernel.org; li...@arm.linux.org.uk;
devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
p...@vger.kernel.org
Subject: Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and
pcie

On Thursday 15 October 2015 12:01:04 Murali Karicheri wrote:



+ * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the
+ * distribution.


The current code does not do this when compiled, which might be a
problem for distributors. Can you clarify the license?


Arnd,

Can you elaborate on this? I did a grep on the string "Redistributions
in binary form must reproduce the above copyright" and I could find
several instance of this. So I am not sure what you mean by "The current
code does not do this when compiled".


You write that the binary form of the code must produce the copyright
notice. I don't see any code that does this. If I was looking in the
wrong place, let me know.

Arnd




Thanks Wingman for the response.

Arnd, by your statement 'I don't see any code that does this' do you 
expect a piece of code that embed the license in the binary image? If 
so, that seems weired to me.


Many of the drivers including this patch has the following statement in 
the license that is additional company specific license such as BSD that 
is applicable.


 Cut and pasted from drivers/crypto/fcrypt.c ===
 * 2. Redistributions in binary form must reproduce the above copyright
 *notice, this list of conditions and the following disclaimer in the
 *documentation and/or other materials provided with the distribution.
=
I read this as if the source is compiled and distributed as a binary 
either a kernel module ko file or as part of the kernel binary, this 
term must apply. Usually this is part of documentation that goes with 
the product AFAIK.


Murali


For example, we did a grep of the following

mypc:linux(personal/linux/serdes) $ grep -rnI "Redistributions in binary form must 
reproduce the above copyright" ./net/
./net/sunrpc/auth_gss/auth_gss.c:18: *  2. Redistributions in binary form must 
reproduce the above copyright
./net/sunrpc/auth_gss/gss_mech_switch.c:15: *  2. Redistributions in binary 
form must reproduce the above copyright
./net/sunrpc/auth_gss/gss_krb5_mech.c:16: *  2. Redistributions in binary form 
must reproduce the above copyright
./net/bluetooth/ecc.c:10: *  * Redistributions in binary form must reproduce 
the above copyright
./net/bluetooth/ecc.h:10: *  * Redistributions in binary form must reproduce 
the above copyright
./net/can/gw.c:12: * 2. Redistributions in binary form must reproduce the above 
copyright
./net/can/af_can.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/can/proc.c:12: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/can/bcm.c:12: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/can/raw.c:12: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/can/af_can.h:10: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/discover.c:13: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/node.h:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/netlink_compat.c:10: * 2. Redistributions in binary form must 
reproduce the above copyright
./net/tipc/name_distr.h:13: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/bearer.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/name_table.h:13: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/name_distr.c:13: * 2. Redistributions in binary form must reproduce 
the above copyright
./net/tipc/addr.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/subscr.h:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/link.h:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/net.h:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/netlink.c:13: * 2. Redistributions in binary form must reproduce the 
above copyright
./net/tipc/sysctl.c:12: * 2. Redistributions in binary form must reprod

RE: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Kwok, WingMan]

Hello,

> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Thursday, October 15, 2015 4:53 PM
> To: Kwok, WingMan
> Cc: linux-arm-ker...@lists.infradead.org; robh...@kernel.org;
> pawel.m...@arm.com; mark.rutl...@arm.com; ijc+devicet...@hellion.org.uk;
> ga...@codeaurora.org; KISHON VIJAY ABRAHAM; Quadros, Roger; Karicheri,
> Muralidharan; bhelg...@google.com; ssant...@kernel.org;
> li...@arm.linux.org.uk; devicet...@vger.kernel.org; linux-
> ker...@vger.kernel.org; linux-...@vger.kernel.org
> Subject: Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and
> pcie
> 
> On Thursday 15 October 2015 20:08:32 Kwok, WingMan wrote:
> >
> > > > +#define reg_rmw(addr, value, mask) \
> > > > +   __raw_writel(((__raw_readl(addr) & (~(mask))) | \
> > > > +   (value & (mask))), (addr))
> > >
> > > not endian safe, and potentially racy.
> > >
> >
> > will change to
> >
> > #define reg_rmw(addr, value, mask) \
> > writel(((readl(addr) & (~(mask))) | \
> > (value & (mask))), (addr))
> 
> Ok, sounds good. Note that if any of this is performance critical,
> better use readl_relaxed(), but as long as this is just for setup
> code and not for data transfers, staying with readl() as you
> suggest is better.
> 

since this is only for initialization, I'll probably stay with readl().

> > > > +static inline void _kserdes_reset_cdr(void __iomem *sregs, int lane)
> > > > +{
> > > > +   /* toggle signal detect */
> > > > +   _kserdes_force_signal_detect_low(sregs, lane);
> > > > +   mdelay(1);
> > > > +   _kserdes_force_signal_detect_high(sregs, lane);
> > > > +}
> > >
> > > Can you change the code so you can use msleep(1) here?
> > >
> >
> > will replace delays with usleep_range()
> 
> Ok.
> 
> > > > +
> > > > +   do {
> > > > +   mdelay(10);
> > > > +   memset(lane_down, 0, sizeof(lane_down));
> > > > +
> > > > +   link_up = _kserdes_check_link_status(dev, sregs,
> > > > +pcsr_regmap, lanes,
> > > > +lanes_enable,
> > > > +current_state,
> lane_down);
> > > > +
> > > > +   /* if we did not get link up then wait 100ms
> > > > +* before calling it again
> > > > +*/
> > > > +   if (link_up)
> > > > +   break;
> > > > +
> > > > +   for (i = 0; i < lanes; i++) {
> > > > +   if ((lanes_enable & (1 << i)) && lane_down[i])
> > > > +   dev_dbg(dev,
> > > > +   "XGE: detected lane down on lane
> %d\n",
> > > > +   i);
> > > > +   }
> > > > +
> > > > +   if (++retries > 100)
> > > > +   return -ETIMEDOUT;
> > > > +
> > > > +   } while (!link_up);
> > >
> > > an more importantly here. Blocking the CPU for over one second is not
> good.
> > >
> > > Any use of mdelay() should have a comment explaining why you cannot use
> > > msleep() in that instance.
> > >
> >
> > will replace delays with usleep_range()
> 
> Here you have to be careful with the total runtime. Using usleep_range()
> is a good idea, and you can have a particularly wide range, but then you
> should changen the timeout condition from number of retries to total
> elapsed time like
> 
>   unsigned long timeout = jiffies + HZ; /* 1 second maximum */
>   do {
>   ...
> 
>   if (link_up)
>   break;
> 
>   if (time_after(jiffies, timeout)
>   return -ETIMEOUT;
> 
>   usleep_range(1000, 5);
>   } while (1);
> 

will do.  

>   Arnd

Thanks so much for your comments.
WingMan
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Kwok, WingMan]

Hello,

> -Original Message-
> From: Rob Herring [mailto:robh...@kernel.org]
> Sent: Thursday, October 15, 2015 12:15 PM
> To: Kwok, WingMan
> Cc: Pawel Moll; Mark Rutland; Ian Campbell; Kumar Gala; KISHON VIJAY ABRAHAM;
> Quadros, Roger; Karicheri, Muralidharan; Bjorn Helgaas; Santosh Shilimkar;
> Russell King - ARM Linux; devicet...@vger.kernel.org; linux-
> ker...@vger.kernel.org; linux-...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org
> Subject: Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and
> pcie
> 
> On Thu, Oct 15, 2015 at 9:25 AM, WingMan Kwok  wrote:
> > On TI's Keystone platforms, several peripherals such as the
> > gbe ethernet switch, 10gbe ethernet switch and PCIe controller
> > require the use of a SerDes for converting SoC parallel data into
> > serialized data that can be output over a high-speed electrical
> > interface, and also converting high-speed serial input data
> > into parallel data that can be processed by the SoC.  The
> > SerDeses used by those peripherals, though they may be different,
> > are largely similar in functionality and setup.
> >
> > This patch provides a SerDes phy driver implementation that can be
> > used by the above mentioned peripheral drivers to configure their
> > respective SerDeses.
> >
> > v1:
> > - see cover letter for review comments addressed.
> >
> > Signed-off-by: WingMan Kwok 
> > ---
> >  Documentation/devicetree/bindings/phy/ti-phy.txt |  278 +++
> >  drivers/phy/Kconfig  |8 +
> >  drivers/phy/Makefile |1 +
> >  drivers/phy/phy-keystone-serdes.c| 2373
> ++
> >  4 files changed, 2660 insertions(+)
> >  create mode 100644 drivers/phy/phy-keystone-serdes.c
> >
> > diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt
> b/Documentation/devicetree/bindings/phy/ti-phy.txt
> > index 9cf9446..4dca271 100644
> > --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
> > +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
> > @@ -115,4 +115,282 @@ sata_phy: phy@4A096000 {
> > clock-names = "sysclk", "refclk";
> > syscon-pllreset = <_conf 0x3fc>;
> > #phy-cells = <0>;
> > +
> > +TI Keystone SerDes PHY
> > +==
> > +
> > +Required properties:
> > + - compatible: should be one of
> > +   * "ti,keystone-serdes-gbe"
> > +   * "ti,keystone-serdes-xgbe"
> > +   * "ti,keystone-serdes-pcie"
> 
> These are different blocks or different modes of the same block? It's
> fine if the former. If the latter, then you should have a single
> compatible and then have a mode property. Perhaps phy-connection-type
> from ePAPR ethernet binding can be extended.
> 

these are different hw blocks configured specifically
for the corresponding peripheral.

> 
> > + - reg:
> > +   * base address and length of the SerDes register set
> > + - reg-names:
> > +   * "serdes"
> > +   - name of the reg SerDes register set
> 
> reg-names is kind of pointless with only 1.
> 

will remove.

> > + - #phy-cells:
> > +   * From the generic phy bindings, must be 0;
> > + - num-lanes:
> > +   * Number of lanes in SerDes.
> > +
> > +Optional properties:
> > + - syscon-peripheral:
> > +   * Handle to the subsystem register region of the peripheral
> > + inside which the SerDes exists.
> > + - syscon-link:
> > +   * Handle to the Link register region of the peripheral inside
> > + which the SerDes exists.  Example: it is the PCSR register
> > + region in the case of 10gbe.
> > + - rx-force-enable:
> > +   * Include this property if receiver attenuation and boost are
> > + to be configured with specific values defined in rx-force.
> > + - link-rate-kbps:
> > +   * SerDes link rate to be configured, in kbps.
> > +
> > +
> > +For gbe and 10gbe SerDes, it is optional to represent each lane as
> > +a sub-node, which can be enabled or disabled individually using
> > +the "status" property.
> > +
> > +Required properties (lane sub-node):
> > + - reg:
> > +   * lane number
> > +
> > +Optional properties (lane sub-node):
> > + - control-rate:
> > +   * Lane control rate
> > +   0: full rate
> > +   1: half rate
> > +   2: quarter rate

Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Arnd Bergmann]

On Thursday 15 October 2015 20:08:32 Kwok, WingMan wrote:
> 
> > > +#define reg_rmw(addr, value, mask) \
> > > +   __raw_writel(((__raw_readl(addr) & (~(mask))) | \
> > > +   (value & (mask))), (addr))
> > 
> > not endian safe, and potentially racy.
> > 
> 
> will change to 
> 
> #define reg_rmw(addr, value, mask) \
> writel(((readl(addr) & (~(mask))) | \
> (value & (mask))), (addr))

Ok, sounds good. Note that if any of this is performance critical,
better use readl_relaxed(), but as long as this is just for setup
code and not for data transfers, staying with readl() as you
suggest is better.

> > > +static inline void _kserdes_reset_cdr(void __iomem *sregs, int lane)
> > > +{
> > > +   /* toggle signal detect */
> > > +   _kserdes_force_signal_detect_low(sregs, lane);
> > > +   mdelay(1);
> > > +   _kserdes_force_signal_detect_high(sregs, lane);
> > > +}
> > 
> > Can you change the code so you can use msleep(1) here?
> > 
> 
> will replace delays with usleep_range()

Ok.

> > > +
> > > +   do {
> > > +   mdelay(10);
> > > +   memset(lane_down, 0, sizeof(lane_down));
> > > +
> > > +   link_up = _kserdes_check_link_status(dev, sregs,
> > > +pcsr_regmap, lanes,
> > > +lanes_enable,
> > > +current_state, 
> > > lane_down);
> > > +
> > > +   /* if we did not get link up then wait 100ms
> > > +* before calling it again
> > > +*/
> > > +   if (link_up)
> > > +   break;
> > > +
> > > +   for (i = 0; i < lanes; i++) {
> > > +   if ((lanes_enable & (1 << i)) && lane_down[i])
> > > +   dev_dbg(dev,
> > > +   "XGE: detected lane down on lane 
> > > %d\n",
> > > +   i);
> > > +   }
> > > +
> > > +   if (++retries > 100)
> > > +   return -ETIMEDOUT;
> > > +
> > > +   } while (!link_up);
> > 
> > an more importantly here. Blocking the CPU for over one second is not good.
> > 
> > Any use of mdelay() should have a comment explaining why you cannot use
> > msleep() in that instance.
> > 
> 
> will replace delays with usleep_range()

Here you have to be careful with the total runtime. Using usleep_range()
is a good idea, and you can have a particularly wide range, but then you
should changen the timeout condition from number of retries to total
elapsed time like

unsigned long timeout = jiffies + HZ; /* 1 second maximum */
do {
...

if (link_up)
break;

if (time_after(jiffies, timeout)
return -ETIMEOUT;

usleep_range(1000, 5);
} while (1);

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Kwok, WingMan]

Hi,

> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Thursday, October 15, 2015 10:51 AM
> To: linux-arm-ker...@lists.infradead.org
> Cc: Kwok, WingMan; robh...@kernel.org; pawel.m...@arm.com;
> mark.rutl...@arm.com; ijc+devicet...@hellion.org.uk; ga...@codeaurora.org;
> KISHON VIJAY ABRAHAM; Quadros, Roger; Karicheri, Muralidharan;
> bhelg...@google.com; ssant...@kernel.org; li...@arm.linux.org.uk;
> devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> p...@vger.kernel.org
> Subject: Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and
> pcie
> 
> On Thursday 15 October 2015 10:25:44 WingMan Kwok wrote:
> > On TI's Keystone platforms, several peripherals such as the
> > gbe ethernet switch, 10gbe ethernet switch and PCIe controller
> > require the use of a SerDes for converting SoC parallel data into
> > serialized data that can be output over a high-speed electrical
> > interface, and also converting high-speed serial input data
> > into parallel data that can be processed by the SoC.  The
> > SerDeses used by those peripherals, though they may be different,
> > are largely similar in functionality and setup.
> >
> > This patch provides a SerDes phy driver implementation that can be
> > used by the above mentioned peripheral drivers to configure their
> > respective SerDeses.
> >
> > v1:
> > - see cover letter for review comments addressed.
> >
> > Signed-off-by: WingMan Kwok 
> > ---
> >  Documentation/devicetree/bindings/phy/ti-phy.txt |  278 +++
> >  drivers/phy/Kconfig  |8 +
> >  drivers/phy/Makefile |1 +
> >  drivers/phy/phy-keystone-serdes.c| 2373
> ++
> >  4 files changed, 2660 insertions(+)
> >  create mode 100644 drivers/phy/phy-keystone-serdes.c
> 
> This is quite a bit of code. Are you very sure that this PHY is
> not used on any other SoC family, and that it is not licensed
> from a third party? I would hate to see multiple copies of
> this getting merged into the kernel over time, so thename should
> be chosen carefully to let the next person know when they have
> related hardware.
> 
> > +
> > +gbe_serdes0: gbe_serdes@232a000 {
> 
> 
> make that phy@232a000, the name should be one of the usual identifiers,
> not specific to the instance.
> 

will change to something like gbe_serdes0: phy@232a000 {};

> > +config PHY_TI_KEYSTONE_SERDES
> > +   tristate "TI Keystone SerDes PHY support"
> > +   depends on OF && ARCH_KEYSTONE
> > +   select GENERIC_PHY
> > +   help
> > + This option enables support for TI Keystone SerDes PHY found
> > + in peripherals GBE, 10GBE and PCIe.
> > +
> 
> (ARCH_KEYSTONE || COMPILE_TEST) ?
> 

will add COMPILE_TEST

> > + * Redistributions in binary form must reproduce the above copyright
> > + * notice, this list of conditions and the following disclaimer in the
> > + * documentation and/or other materials provided with the
> > + * distribution.
> 
> The current code does not do this when compiled, which might be a
> problem for distributors. Can you clarify the license?
> 

will investigate

> > +#define reg_rmw(addr, value, mask) \
> > +   __raw_writel(((__raw_readl(addr) & (~(mask))) | \
> > +   (value & (mask))), (addr))
> 
> not endian safe, and potentially racy.
> 

will change to 

#define reg_rmw(addr, value, mask) \
writel(((readl(addr) & (~(mask))) | \
(value & (mask))), (addr))

> > +static inline void _kserdes_reset_cdr(void __iomem *sregs, int lane)
> > +{
> > +   /* toggle signal detect */
> > +   _kserdes_force_signal_detect_low(sregs, lane);
> > +   mdelay(1);
> > +   _kserdes_force_signal_detect_high(sregs, lane);
> > +}
> 
> Can you change the code so you can use msleep(1) here?
> 

will replace delays with usleep_range()

> > +
> > +   do {
> > +   mdelay(10);
> > +   memset(lane_down, 0, sizeof(lane_down));
> > +
> > +   link_up = _kserdes_check_link_status(dev, sregs,
> > +pcsr_regmap, lanes,
> > +lanes_enable,
> > +current_state, lane_down);
> > +
> > +   /* if we did not get link up then wait 100ms
> > +* before calling it again
> > +*/
> > +   if (link_up)
> > +   break;
> &g

Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Arnd Bergmann]

On Thursday 15 October 2015 12:01:04 Murali Karicheri wrote:
> 
> >> + * Redistributions in binary form must reproduce the above copyright
> >> + * notice, this list of conditions and the following disclaimer in the
> >> + * documentation and/or other materials provided with the
> >> + * distribution.
> >
> > The current code does not do this when compiled, which might be a
> > problem for distributors. Can you clarify the license?
> >
> Arnd,
> 
> Can you elaborate on this? I did a grep on the string "Redistributions 
> in binary form must reproduce the above copyright" and I could find 
> several instance of this. So I am not sure what you mean by "The current 
> code does not do this when compiled".

You write that the binary form of the code must produce the copyright
notice. I don't see any code that does this. If I was looking in the
wrong place, let me know.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Rob Herring]

On Thu, Oct 15, 2015 at 9:25 AM, WingMan Kwok  wrote:
> On TI's Keystone platforms, several peripherals such as the
> gbe ethernet switch, 10gbe ethernet switch and PCIe controller
> require the use of a SerDes for converting SoC parallel data into
> serialized data that can be output over a high-speed electrical
> interface, and also converting high-speed serial input data
> into parallel data that can be processed by the SoC.  The
> SerDeses used by those peripherals, though they may be different,
> are largely similar in functionality and setup.
>
> This patch provides a SerDes phy driver implementation that can be
> used by the above mentioned peripheral drivers to configure their
> respective SerDeses.
>
> v1:
> - see cover letter for review comments addressed.
>
> Signed-off-by: WingMan Kwok 
> ---
>  Documentation/devicetree/bindings/phy/ti-phy.txt |  278 +++
>  drivers/phy/Kconfig  |8 +
>  drivers/phy/Makefile |1 +
>  drivers/phy/phy-keystone-serdes.c| 2373 
> ++
>  4 files changed, 2660 insertions(+)
>  create mode 100644 drivers/phy/phy-keystone-serdes.c
>
> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt 
> b/Documentation/devicetree/bindings/phy/ti-phy.txt
> index 9cf9446..4dca271 100644
> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
> @@ -115,4 +115,282 @@ sata_phy: phy@4A096000 {
> clock-names = "sysclk", "refclk";
> syscon-pllreset = <_conf 0x3fc>;
> #phy-cells = <0>;
> +
> +TI Keystone SerDes PHY
> +==
> +
> +Required properties:
> + - compatible: should be one of
> +   * "ti,keystone-serdes-gbe"
> +   * "ti,keystone-serdes-xgbe"
> +   * "ti,keystone-serdes-pcie"

These are different blocks or different modes of the same block? It's
fine if the former. If the latter, then you should have a single
compatible and then have a mode property. Perhaps phy-connection-type
from ePAPR ethernet binding can be extended.


> + - reg:
> +   * base address and length of the SerDes register set
> + - reg-names:
> +   * "serdes"
> +   - name of the reg SerDes register set

reg-names is kind of pointless with only 1.

> + - #phy-cells:
> +   * From the generic phy bindings, must be 0;
> + - num-lanes:
> +   * Number of lanes in SerDes.
> +
> +Optional properties:
> + - syscon-peripheral:
> +   * Handle to the subsystem register region of the peripheral
> + inside which the SerDes exists.
> + - syscon-link:
> +   * Handle to the Link register region of the peripheral inside
> + which the SerDes exists.  Example: it is the PCSR register
> + region in the case of 10gbe.
> + - rx-force-enable:
> +   * Include this property if receiver attenuation and boost are
> + to be configured with specific values defined in rx-force.
> + - link-rate-kbps:
> +   * SerDes link rate to be configured, in kbps.
> +
> +
> +For gbe and 10gbe SerDes, it is optional to represent each lane as
> +a sub-node, which can be enabled or disabled individually using
> +the "status" property.
> +
> +Required properties (lane sub-node):
> + - reg:
> +   * lane number
> +
> +Optional properties (lane sub-node):
> + - control-rate:
> +   * Lane control rate
> +   0: full rate
> +   1: half rate
> +   2: quarter rate
> + - rx-start:
> +   * Initial lane rx equalizer attenuation and boost configurations.
> +   * Must be array of 2 integers.
> + - rx-force:
> +   * Forced lane rx equalizer attenuation and boost configurations.
> +   * Must be array of 2 integers.
> + - tx-coeff:
> +   * Lane c1, c2, cm, attenuation and regulator output voltage
> + configurations.
> +   * Must be array of 5 integers.
> + - loopback:
> +   * Include this property to enable loopback at the SerDes
> + lane level.

This seems overly complicated. Do you really expect these to be
different per lane?

> +
> +Example for Keystone K2E GBE:
> +-
> +
> +gbe_serdes0: gbe_serdes@232a000 {
> +   #phy-cells  = <0>;
> +   compatible  = "ti,keystone-serdes-gbe";
> +   reg = <0x0232a000 0x2000>;
> +   reg-names   = "serdes";
> +   link-rate-kbps  = <125>;
> +   num-lanes   = <4>;
> +   lanes {
> +   #address-cells = <1>;
> +   #size-cells = <0>;
> +   lane@0 {
> +   /*loopback;*/
> +   reg = <0>;
> +   control-rate= <2>; /* quart */
> +   rx-start= <7 5>;
> +   rx-force= <1 1>;
> +   tx-coeff= <0 0 0 12 4>;
> +  /* c1 c2 cm att 

Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Murali Karicheri]

On 10/15/2015 10:51 AM, Arnd Bergmann wrote:

On Thursday 15 October 2015 10:25:44 WingMan Kwok wrote:

On TI's Keystone platforms, several peripherals such as the
gbe ethernet switch, 10gbe ethernet switch and PCIe controller
require the use of a SerDes for converting SoC parallel data into
serialized data that can be output over a high-speed electrical
interface, and also converting high-speed serial input data
into parallel data that can be processed by the SoC.  The
SerDeses used by those peripherals, though they may be different,
are largely similar in functionality and setup.

This patch provides a SerDes phy driver implementation that can be
used by the above mentioned peripheral drivers to configure their
respective SerDeses.

v1:


cut-


+ * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the
+ * distribution.


The current code does not do this when compiled, which might be a
problem for distributors. Can you clarify the license?


Arnd,

Can you elaborate on this? I did a grep on the string "Redistributions 
in binary form must reproduce the above copyright" and I could find 
several instance of this. So I am not sure what you mean by "The current 
code does not do this when compiled".


Thanks
--
Murali Karicheri
Linux Kernel, Keystone
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Arnd Bergmann]

On Thursday 15 October 2015 10:25:44 WingMan Kwok wrote:
> On TI's Keystone platforms, several peripherals such as the
> gbe ethernet switch, 10gbe ethernet switch and PCIe controller
> require the use of a SerDes for converting SoC parallel data into
> serialized data that can be output over a high-speed electrical
> interface, and also converting high-speed serial input data
> into parallel data that can be processed by the SoC.  The
> SerDeses used by those peripherals, though they may be different,
> are largely similar in functionality and setup.
> 
> This patch provides a SerDes phy driver implementation that can be
> used by the above mentioned peripheral drivers to configure their
> respective SerDeses.
> 
> v1:
>   - see cover letter for review comments addressed.
> 
> Signed-off-by: WingMan Kwok 
> ---
>  Documentation/devicetree/bindings/phy/ti-phy.txt |  278 +++
>  drivers/phy/Kconfig  |8 +
>  drivers/phy/Makefile |1 +
>  drivers/phy/phy-keystone-serdes.c| 2373 
> ++
>  4 files changed, 2660 insertions(+)
>  create mode 100644 drivers/phy/phy-keystone-serdes.c

This is quite a bit of code. Are you very sure that this PHY is
not used on any other SoC family, and that it is not licensed
from a third party? I would hate to see multiple copies of
this getting merged into the kernel over time, so thename should
be chosen carefully to let the next person know when they have
related hardware.

> +
> +gbe_serdes0: gbe_serdes@232a000 {


make that phy@232a000, the name should be one of the usual identifiers,
not specific to the instance.

> +config PHY_TI_KEYSTONE_SERDES
> + tristate "TI Keystone SerDes PHY support"
> + depends on OF && ARCH_KEYSTONE
> + select GENERIC_PHY
> + help
> +   This option enables support for TI Keystone SerDes PHY found
> +   in peripherals GBE, 10GBE and PCIe.
> +

(ARCH_KEYSTONE || COMPILE_TEST) ?

> + * Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the
> + * distribution.

The current code does not do this when compiled, which might be a
problem for distributors. Can you clarify the license?

> +#define reg_rmw(addr, value, mask) \
> + __raw_writel(((__raw_readl(addr) & (~(mask))) | \
> + (value & (mask))), (addr))

not endian safe, and potentially racy.

> +static inline void _kserdes_reset_cdr(void __iomem *sregs, int lane)
> +{
> + /* toggle signal detect */
> + _kserdes_force_signal_detect_low(sregs, lane);
> + mdelay(1);
> + _kserdes_force_signal_detect_high(sregs, lane);
> +}

Can you change the code so you can use msleep(1) here?

> +
> + do {
> + mdelay(10);
> + memset(lane_down, 0, sizeof(lane_down));
> +
> + link_up = _kserdes_check_link_status(dev, sregs,
> +  pcsr_regmap, lanes,
> +  lanes_enable,
> +  current_state, lane_down);
> +
> + /* if we did not get link up then wait 100ms
> +  * before calling it again
> +  */
> + if (link_up)
> + break;
> +
> + for (i = 0; i < lanes; i++) {
> + if ((lanes_enable & (1 << i)) && lane_down[i])
> + dev_dbg(dev,
> + "XGE: detected lane down on lane %d\n",
> + i);
> + }
> +
> + if (++retries > 100)
> + return -ETIMEDOUT;
> +
> + } while (!link_up);

an more importantly here. Blocking the CPU for over one second is not good.

Any use of mdelay() should have a comment explaining why you cannot use
msleep() in that instance.

> +
> +static int __init keystone_serdes_phy_init(void)
> +{
> + return platform_driver_register(_driver);
> +}
> +module_init(keystone_serdes_phy_init);
> +
> +static void __exit keystone_serdes_phy_exit(void)
> +{
> + platform_driver_unregister(_driver);
> +}
> +module_exit(keystone_serdes_phy_exit);

module_platform_driver()

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Rob Herring]

On Thu, Oct 15, 2015 at 9:25 AM, WingMan Kwok  wrote:
> On TI's Keystone platforms, several peripherals such as the
> gbe ethernet switch, 10gbe ethernet switch and PCIe controller
> require the use of a SerDes for converting SoC parallel data into
> serialized data that can be output over a high-speed electrical
> interface, and also converting high-speed serial input data
> into parallel data that can be processed by the SoC.  The
> SerDeses used by those peripherals, though they may be different,
> are largely similar in functionality and setup.
>
> This patch provides a SerDes phy driver implementation that can be
> used by the above mentioned peripheral drivers to configure their
> respective SerDeses.
>
> v1:
> - see cover letter for review comments addressed.
>
> Signed-off-by: WingMan Kwok 
> ---
>  Documentation/devicetree/bindings/phy/ti-phy.txt |  278 +++
>  drivers/phy/Kconfig  |8 +
>  drivers/phy/Makefile |1 +
>  drivers/phy/phy-keystone-serdes.c| 2373 
> ++
>  4 files changed, 2660 insertions(+)
>  create mode 100644 drivers/phy/phy-keystone-serdes.c
>
> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt 
> b/Documentation/devicetree/bindings/phy/ti-phy.txt
> index 9cf9446..4dca271 100644
> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
> @@ -115,4 +115,282 @@ sata_phy: phy@4A096000 {
> clock-names = "sysclk", "refclk";
> syscon-pllreset = <_conf 0x3fc>;
> #phy-cells = <0>;
> +
> +TI Keystone SerDes PHY
> +==
> +
> +Required properties:
> + - compatible: should be one of
> +   * "ti,keystone-serdes-gbe"
> +   * "ti,keystone-serdes-xgbe"
> +   * "ti,keystone-serdes-pcie"

These are different blocks or different modes of the same block? It's
fine if the former. If the latter, then you should have a single
compatible and then have a mode property. Perhaps phy-connection-type
from ePAPR ethernet binding can be extended.


> + - reg:
> +   * base address and length of the SerDes register set
> + - reg-names:
> +   * "serdes"
> +   - name of the reg SerDes register set

reg-names is kind of pointless with only 1.

> + - #phy-cells:
> +   * From the generic phy bindings, must be 0;
> + - num-lanes:
> +   * Number of lanes in SerDes.
> +
> +Optional properties:
> + - syscon-peripheral:
> +   * Handle to the subsystem register region of the peripheral
> + inside which the SerDes exists.
> + - syscon-link:
> +   * Handle to the Link register region of the peripheral inside
> + which the SerDes exists.  Example: it is the PCSR register
> + region in the case of 10gbe.
> + - rx-force-enable:
> +   * Include this property if receiver attenuation and boost are
> + to be configured with specific values defined in rx-force.
> + - link-rate-kbps:
> +   * SerDes link rate to be configured, in kbps.
> +
> +
> +For gbe and 10gbe SerDes, it is optional to represent each lane as
> +a sub-node, which can be enabled or disabled individually using
> +the "status" property.
> +
> +Required properties (lane sub-node):
> + - reg:
> +   * lane number
> +
> +Optional properties (lane sub-node):
> + - control-rate:
> +   * Lane control rate
> +   0: full rate
> +   1: half rate
> +   2: quarter rate
> + - rx-start:
> +   * Initial lane rx equalizer attenuation and boost configurations.
> +   * Must be array of 2 integers.
> + - rx-force:
> +   * Forced lane rx equalizer attenuation and boost configurations.
> +   * Must be array of 2 integers.
> + - tx-coeff:
> +   * Lane c1, c2, cm, attenuation and regulator output voltage
> + configurations.
> +   * Must be array of 5 integers.
> + - loopback:
> +   * Include this property to enable loopback at the SerDes
> + lane level.

This seems overly complicated. Do you really expect these to be
different per lane?

> +
> +Example for Keystone K2E GBE:
> +-
> +
> +gbe_serdes0: gbe_serdes@232a000 {
> +   #phy-cells  = <0>;
> +   compatible  = "ti,keystone-serdes-gbe";
> +   reg = <0x0232a000 0x2000>;
> +   reg-names   = "serdes";
> +   link-rate-kbps  = <125>;
> +   num-lanes   = <4>;
> +   lanes {
> +   #address-cells = <1>;
> +   #size-cells = <0>;
> +   lane@0 {
> +   /*loopback;*/
> +   reg = <0>;
> +   control-rate= <2>; /* quart */
> +   rx-start= <7 5>;
> +   rx-force= <1 1>;
> +   tx-coeff= <0 0 0 12 4>;
> +  

Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Murali Karicheri]

On 10/15/2015 10:51 AM, Arnd Bergmann wrote:

On Thursday 15 October 2015 10:25:44 WingMan Kwok wrote:

On TI's Keystone platforms, several peripherals such as the
gbe ethernet switch, 10gbe ethernet switch and PCIe controller
require the use of a SerDes for converting SoC parallel data into
serialized data that can be output over a high-speed electrical
interface, and also converting high-speed serial input data
into parallel data that can be processed by the SoC.  The
SerDeses used by those peripherals, though they may be different,
are largely similar in functionality and setup.

This patch provides a SerDes phy driver implementation that can be
used by the above mentioned peripheral drivers to configure their
respective SerDeses.

v1:


cut-


+ * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the
+ * distribution.


The current code does not do this when compiled, which might be a
problem for distributors. Can you clarify the license?


Arnd,

Can you elaborate on this? I did a grep on the string "Redistributions 
in binary form must reproduce the above copyright" and I could find 
several instance of this. So I am not sure what you mean by "The current 
code does not do this when compiled".


Thanks
--
Murali Karicheri
Linux Kernel, Keystone
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Arnd Bergmann]

On Thursday 15 October 2015 10:25:44 WingMan Kwok wrote:
> On TI's Keystone platforms, several peripherals such as the
> gbe ethernet switch, 10gbe ethernet switch and PCIe controller
> require the use of a SerDes for converting SoC parallel data into
> serialized data that can be output over a high-speed electrical
> interface, and also converting high-speed serial input data
> into parallel data that can be processed by the SoC.  The
> SerDeses used by those peripherals, though they may be different,
> are largely similar in functionality and setup.
> 
> This patch provides a SerDes phy driver implementation that can be
> used by the above mentioned peripheral drivers to configure their
> respective SerDeses.
> 
> v1:
>   - see cover letter for review comments addressed.
> 
> Signed-off-by: WingMan Kwok 
> ---
>  Documentation/devicetree/bindings/phy/ti-phy.txt |  278 +++
>  drivers/phy/Kconfig  |8 +
>  drivers/phy/Makefile |1 +
>  drivers/phy/phy-keystone-serdes.c| 2373 
> ++
>  4 files changed, 2660 insertions(+)
>  create mode 100644 drivers/phy/phy-keystone-serdes.c

This is quite a bit of code. Are you very sure that this PHY is
not used on any other SoC family, and that it is not licensed
from a third party? I would hate to see multiple copies of
this getting merged into the kernel over time, so thename should
be chosen carefully to let the next person know when they have
related hardware.

> +
> +gbe_serdes0: gbe_serdes@232a000 {


make that phy@232a000, the name should be one of the usual identifiers,
not specific to the instance.

> +config PHY_TI_KEYSTONE_SERDES
> + tristate "TI Keystone SerDes PHY support"
> + depends on OF && ARCH_KEYSTONE
> + select GENERIC_PHY
> + help
> +   This option enables support for TI Keystone SerDes PHY found
> +   in peripherals GBE, 10GBE and PCIe.
> +

(ARCH_KEYSTONE || COMPILE_TEST) ?

> + * Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the
> + * distribution.

The current code does not do this when compiled, which might be a
problem for distributors. Can you clarify the license?

> +#define reg_rmw(addr, value, mask) \
> + __raw_writel(((__raw_readl(addr) & (~(mask))) | \
> + (value & (mask))), (addr))

not endian safe, and potentially racy.

> +static inline void _kserdes_reset_cdr(void __iomem *sregs, int lane)
> +{
> + /* toggle signal detect */
> + _kserdes_force_signal_detect_low(sregs, lane);
> + mdelay(1);
> + _kserdes_force_signal_detect_high(sregs, lane);
> +}

Can you change the code so you can use msleep(1) here?

> +
> + do {
> + mdelay(10);
> + memset(lane_down, 0, sizeof(lane_down));
> +
> + link_up = _kserdes_check_link_status(dev, sregs,
> +  pcsr_regmap, lanes,
> +  lanes_enable,
> +  current_state, lane_down);
> +
> + /* if we did not get link up then wait 100ms
> +  * before calling it again
> +  */
> + if (link_up)
> + break;
> +
> + for (i = 0; i < lanes; i++) {
> + if ((lanes_enable & (1 << i)) && lane_down[i])
> + dev_dbg(dev,
> + "XGE: detected lane down on lane %d\n",
> + i);
> + }
> +
> + if (++retries > 100)
> + return -ETIMEDOUT;
> +
> + } while (!link_up);

an more importantly here. Blocking the CPU for over one second is not good.

Any use of mdelay() should have a comment explaining why you cannot use
msleep() in that instance.

> +
> +static int __init keystone_serdes_phy_init(void)
> +{
> + return platform_driver_register(_driver);
> +}
> +module_init(keystone_serdes_phy_init);
> +
> +static void __exit keystone_serdes_phy_exit(void)
> +{
> + platform_driver_unregister(_driver);
> +}
> +module_exit(keystone_serdes_phy_exit);

module_platform_driver()

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Kwok, WingMan]

Hello,

> -Original Message-
> From: Rob Herring [mailto:robh...@kernel.org]
> Sent: Thursday, October 15, 2015 12:15 PM
> To: Kwok, WingMan
> Cc: Pawel Moll; Mark Rutland; Ian Campbell; Kumar Gala; KISHON VIJAY ABRAHAM;
> Quadros, Roger; Karicheri, Muralidharan; Bjorn Helgaas; Santosh Shilimkar;
> Russell King - ARM Linux; devicet...@vger.kernel.org; linux-
> ker...@vger.kernel.org; linux-...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org
> Subject: Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and
> pcie
> 
> On Thu, Oct 15, 2015 at 9:25 AM, WingMan Kwok <w-kw...@ti.com> wrote:
> > On TI's Keystone platforms, several peripherals such as the
> > gbe ethernet switch, 10gbe ethernet switch and PCIe controller
> > require the use of a SerDes for converting SoC parallel data into
> > serialized data that can be output over a high-speed electrical
> > interface, and also converting high-speed serial input data
> > into parallel data that can be processed by the SoC.  The
> > SerDeses used by those peripherals, though they may be different,
> > are largely similar in functionality and setup.
> >
> > This patch provides a SerDes phy driver implementation that can be
> > used by the above mentioned peripheral drivers to configure their
> > respective SerDeses.
> >
> > v1:
> > - see cover letter for review comments addressed.
> >
> > Signed-off-by: WingMan Kwok <w-kw...@ti.com>
> > ---
> >  Documentation/devicetree/bindings/phy/ti-phy.txt |  278 +++
> >  drivers/phy/Kconfig  |8 +
> >  drivers/phy/Makefile |1 +
> >  drivers/phy/phy-keystone-serdes.c| 2373
> ++
> >  4 files changed, 2660 insertions(+)
> >  create mode 100644 drivers/phy/phy-keystone-serdes.c
> >
> > diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt
> b/Documentation/devicetree/bindings/phy/ti-phy.txt
> > index 9cf9446..4dca271 100644
> > --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
> > +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
> > @@ -115,4 +115,282 @@ sata_phy: phy@4A096000 {
> > clock-names = "sysclk", "refclk";
> > syscon-pllreset = <_conf 0x3fc>;
> > #phy-cells = <0>;
> > +
> > +TI Keystone SerDes PHY
> > +==
> > +
> > +Required properties:
> > + - compatible: should be one of
> > +   * "ti,keystone-serdes-gbe"
> > +   * "ti,keystone-serdes-xgbe"
> > +   * "ti,keystone-serdes-pcie"
> 
> These are different blocks or different modes of the same block? It's
> fine if the former. If the latter, then you should have a single
> compatible and then have a mode property. Perhaps phy-connection-type
> from ePAPR ethernet binding can be extended.
> 

these are different hw blocks configured specifically
for the corresponding peripheral.

> 
> > + - reg:
> > +   * base address and length of the SerDes register set
> > + - reg-names:
> > +   * "serdes"
> > +   - name of the reg SerDes register set
> 
> reg-names is kind of pointless with only 1.
> 

will remove.

> > + - #phy-cells:
> > +   * From the generic phy bindings, must be 0;
> > + - num-lanes:
> > +   * Number of lanes in SerDes.
> > +
> > +Optional properties:
> > + - syscon-peripheral:
> > +   * Handle to the subsystem register region of the peripheral
> > + inside which the SerDes exists.
> > + - syscon-link:
> > +   * Handle to the Link register region of the peripheral inside
> > + which the SerDes exists.  Example: it is the PCSR register
> > + region in the case of 10gbe.
> > + - rx-force-enable:
> > +   * Include this property if receiver attenuation and boost are
> > + to be configured with specific values defined in rx-force.
> > + - link-rate-kbps:
> > +   * SerDes link rate to be configured, in kbps.
> > +
> > +
> > +For gbe and 10gbe SerDes, it is optional to represent each lane as
> > +a sub-node, which can be enabled or disabled individually using
> > +the "status" property.
> > +
> > +Required properties (lane sub-node):
> > + - reg:
> > +   * lane number
> > +
> > +Optional properties (lane sub-node):
> > + - control-rate:
> > +   * Lane control rate
> > +   0: full rate
> > +   1: half r

RE: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Kwok, WingMan]

Hello,

> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Thursday, October 15, 2015 4:53 PM
> To: Kwok, WingMan
> Cc: linux-arm-ker...@lists.infradead.org; robh...@kernel.org;
> pawel.m...@arm.com; mark.rutl...@arm.com; ijc+devicet...@hellion.org.uk;
> ga...@codeaurora.org; KISHON VIJAY ABRAHAM; Quadros, Roger; Karicheri,
> Muralidharan; bhelg...@google.com; ssant...@kernel.org;
> li...@arm.linux.org.uk; devicet...@vger.kernel.org; linux-
> ker...@vger.kernel.org; linux-...@vger.kernel.org
> Subject: Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and
> pcie
> 
> On Thursday 15 October 2015 20:08:32 Kwok, WingMan wrote:
> >
> > > > +#define reg_rmw(addr, value, mask) \
> > > > +   __raw_writel(((__raw_readl(addr) & (~(mask))) | \
> > > > +   (value & (mask))), (addr))
> > >
> > > not endian safe, and potentially racy.
> > >
> >
> > will change to
> >
> > #define reg_rmw(addr, value, mask) \
> > writel(((readl(addr) & (~(mask))) | \
> > (value & (mask))), (addr))
> 
> Ok, sounds good. Note that if any of this is performance critical,
> better use readl_relaxed(), but as long as this is just for setup
> code and not for data transfers, staying with readl() as you
> suggest is better.
> 

since this is only for initialization, I'll probably stay with readl().

> > > > +static inline void _kserdes_reset_cdr(void __iomem *sregs, int lane)
> > > > +{
> > > > +   /* toggle signal detect */
> > > > +   _kserdes_force_signal_detect_low(sregs, lane);
> > > > +   mdelay(1);
> > > > +   _kserdes_force_signal_detect_high(sregs, lane);
> > > > +}
> > >
> > > Can you change the code so you can use msleep(1) here?
> > >
> >
> > will replace delays with usleep_range()
> 
> Ok.
> 
> > > > +
> > > > +   do {
> > > > +   mdelay(10);
> > > > +   memset(lane_down, 0, sizeof(lane_down));
> > > > +
> > > > +   link_up = _kserdes_check_link_status(dev, sregs,
> > > > +pcsr_regmap, lanes,
> > > > +lanes_enable,
> > > > +current_state,
> lane_down);
> > > > +
> > > > +   /* if we did not get link up then wait 100ms
> > > > +* before calling it again
> > > > +*/
> > > > +   if (link_up)
> > > > +   break;
> > > > +
> > > > +   for (i = 0; i < lanes; i++) {
> > > > +   if ((lanes_enable & (1 << i)) && lane_down[i])
> > > > +   dev_dbg(dev,
> > > > +   "XGE: detected lane down on lane
> %d\n",
> > > > +   i);
> > > > +   }
> > > > +
> > > > +   if (++retries > 100)
> > > > +   return -ETIMEDOUT;
> > > > +
> > > > +   } while (!link_up);
> > >
> > > an more importantly here. Blocking the CPU for over one second is not
> good.
> > >
> > > Any use of mdelay() should have a comment explaining why you cannot use
> > > msleep() in that instance.
> > >
> >
> > will replace delays with usleep_range()
> 
> Here you have to be careful with the total runtime. Using usleep_range()
> is a good idea, and you can have a particularly wide range, but then you
> should changen the timeout condition from number of retries to total
> elapsed time like
> 
>   unsigned long timeout = jiffies + HZ; /* 1 second maximum */
>   do {
>   ...
> 
>   if (link_up)
>   break;
> 
>   if (time_after(jiffies, timeout)
>   return -ETIMEOUT;
> 
>   usleep_range(1000, 5);
>   } while (1);
> 

will do.  

>   Arnd

Thanks so much for your comments.
WingMan
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Kwok, WingMan]

Hi,

> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Thursday, October 15, 2015 10:51 AM
> To: linux-arm-ker...@lists.infradead.org
> Cc: Kwok, WingMan; robh...@kernel.org; pawel.m...@arm.com;
> mark.rutl...@arm.com; ijc+devicet...@hellion.org.uk; ga...@codeaurora.org;
> KISHON VIJAY ABRAHAM; Quadros, Roger; Karicheri, Muralidharan;
> bhelg...@google.com; ssant...@kernel.org; li...@arm.linux.org.uk;
> devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> p...@vger.kernel.org
> Subject: Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and
> pcie
> 
> On Thursday 15 October 2015 10:25:44 WingMan Kwok wrote:
> > On TI's Keystone platforms, several peripherals such as the
> > gbe ethernet switch, 10gbe ethernet switch and PCIe controller
> > require the use of a SerDes for converting SoC parallel data into
> > serialized data that can be output over a high-speed electrical
> > interface, and also converting high-speed serial input data
> > into parallel data that can be processed by the SoC.  The
> > SerDeses used by those peripherals, though they may be different,
> > are largely similar in functionality and setup.
> >
> > This patch provides a SerDes phy driver implementation that can be
> > used by the above mentioned peripheral drivers to configure their
> > respective SerDeses.
> >
> > v1:
> > - see cover letter for review comments addressed.
> >
> > Signed-off-by: WingMan Kwok <w-kw...@ti.com>
> > ---
> >  Documentation/devicetree/bindings/phy/ti-phy.txt |  278 +++
> >  drivers/phy/Kconfig  |8 +
> >  drivers/phy/Makefile |1 +
> >  drivers/phy/phy-keystone-serdes.c| 2373
> ++
> >  4 files changed, 2660 insertions(+)
> >  create mode 100644 drivers/phy/phy-keystone-serdes.c
> 
> This is quite a bit of code. Are you very sure that this PHY is
> not used on any other SoC family, and that it is not licensed
> from a third party? I would hate to see multiple copies of
> this getting merged into the kernel over time, so thename should
> be chosen carefully to let the next person know when they have
> related hardware.
> 
> > +
> > +gbe_serdes0: gbe_serdes@232a000 {
> 
> 
> make that phy@232a000, the name should be one of the usual identifiers,
> not specific to the instance.
> 

will change to something like gbe_serdes0: phy@232a000 {};

> > +config PHY_TI_KEYSTONE_SERDES
> > +   tristate "TI Keystone SerDes PHY support"
> > +   depends on OF && ARCH_KEYSTONE
> > +   select GENERIC_PHY
> > +   help
> > + This option enables support for TI Keystone SerDes PHY found
> > + in peripherals GBE, 10GBE and PCIe.
> > +
> 
> (ARCH_KEYSTONE || COMPILE_TEST) ?
> 

will add COMPILE_TEST

> > + * Redistributions in binary form must reproduce the above copyright
> > + * notice, this list of conditions and the following disclaimer in the
> > + * documentation and/or other materials provided with the
> > + * distribution.
> 
> The current code does not do this when compiled, which might be a
> problem for distributors. Can you clarify the license?
> 

will investigate

> > +#define reg_rmw(addr, value, mask) \
> > +   __raw_writel(((__raw_readl(addr) & (~(mask))) | \
> > +   (value & (mask))), (addr))
> 
> not endian safe, and potentially racy.
> 

will change to 

#define reg_rmw(addr, value, mask) \
writel(((readl(addr) & (~(mask))) | \
(value & (mask))), (addr))

> > +static inline void _kserdes_reset_cdr(void __iomem *sregs, int lane)
> > +{
> > +   /* toggle signal detect */
> > +   _kserdes_force_signal_detect_low(sregs, lane);
> > +   mdelay(1);
> > +   _kserdes_force_signal_detect_high(sregs, lane);
> > +}
> 
> Can you change the code so you can use msleep(1) here?
> 

will replace delays with usleep_range()

> > +
> > +   do {
> > +   mdelay(10);
> > +   memset(lane_down, 0, sizeof(lane_down));
> > +
> > +   link_up = _kserdes_check_link_status(dev, sregs,
> > +pcsr_regmap, lanes,
> > +lanes_enable,
> > +current_state, lane_down);
> > +
> > +   /* if we did not get link up then wait 100ms
> > +* before calling it again
> > +*/
> > +   if (link_up)
> > +

Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Arnd Bergmann]

On Thursday 15 October 2015 20:08:32 Kwok, WingMan wrote:
> 
> > > +#define reg_rmw(addr, value, mask) \
> > > +   __raw_writel(((__raw_readl(addr) & (~(mask))) | \
> > > +   (value & (mask))), (addr))
> > 
> > not endian safe, and potentially racy.
> > 
> 
> will change to 
> 
> #define reg_rmw(addr, value, mask) \
> writel(((readl(addr) & (~(mask))) | \
> (value & (mask))), (addr))

Ok, sounds good. Note that if any of this is performance critical,
better use readl_relaxed(), but as long as this is just for setup
code and not for data transfers, staying with readl() as you
suggest is better.

> > > +static inline void _kserdes_reset_cdr(void __iomem *sregs, int lane)
> > > +{
> > > +   /* toggle signal detect */
> > > +   _kserdes_force_signal_detect_low(sregs, lane);
> > > +   mdelay(1);
> > > +   _kserdes_force_signal_detect_high(sregs, lane);
> > > +}
> > 
> > Can you change the code so you can use msleep(1) here?
> > 
> 
> will replace delays with usleep_range()

Ok.

> > > +
> > > +   do {
> > > +   mdelay(10);
> > > +   memset(lane_down, 0, sizeof(lane_down));
> > > +
> > > +   link_up = _kserdes_check_link_status(dev, sregs,
> > > +pcsr_regmap, lanes,
> > > +lanes_enable,
> > > +current_state, 
> > > lane_down);
> > > +
> > > +   /* if we did not get link up then wait 100ms
> > > +* before calling it again
> > > +*/
> > > +   if (link_up)
> > > +   break;
> > > +
> > > +   for (i = 0; i < lanes; i++) {
> > > +   if ((lanes_enable & (1 << i)) && lane_down[i])
> > > +   dev_dbg(dev,
> > > +   "XGE: detected lane down on lane 
> > > %d\n",
> > > +   i);
> > > +   }
> > > +
> > > +   if (++retries > 100)
> > > +   return -ETIMEDOUT;
> > > +
> > > +   } while (!link_up);
> > 
> > an more importantly here. Blocking the CPU for over one second is not good.
> > 
> > Any use of mdelay() should have a comment explaining why you cannot use
> > msleep() in that instance.
> > 
> 
> will replace delays with usleep_range()

Here you have to be careful with the total runtime. Using usleep_range()
is a good idea, and you can have a particularly wide range, but then you
should changen the timeout condition from number of retries to total
elapsed time like

unsigned long timeout = jiffies + HZ; /* 1 second maximum */
do {
...

if (link_up)
break;

if (time_after(jiffies, timeout)
return -ETIMEOUT;

usleep_range(1000, 5);
} while (1);

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and pcie

[Arnd Bergmann]

On Thursday 15 October 2015 12:01:04 Murali Karicheri wrote:
> 
> >> + * Redistributions in binary form must reproduce the above copyright
> >> + * notice, this list of conditions and the following disclaimer in the
> >> + * documentation and/or other materials provided with the
> >> + * distribution.
> >
> > The current code does not do this when compiled, which might be a
> > problem for distributors. Can you clarify the license?
> >
> Arnd,
> 
> Can you elaborate on this? I did a grep on the string "Redistributions 
> in binary form must reproduce the above copyright" and I could find 
> several instance of this. So I am not sure what you mean by "The current 
> code does not do this when compiled".

You write that the binary form of the code must produce the copyright
notice. I don't see any code that does this. If I was looking in the
wrong place, let me know.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/