| Message ID | 1408550219-19825-3-git-send-email-soren.brinkmann@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/20/2014 05:56 PM, Soren Brinkmann wrote: > Add missing Ethernet phys to Zynq DTs. > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > --- > arch/arm/boot/dts/zynq-zc702.dts | 6 ++++++ > arch/arm/boot/dts/zynq-zc706.dts | 6 ++++++ > arch/arm/boot/dts/zynq-zed.dts | 6 ++++++ > 3 files changed, 18 insertions(+) > > diff --git a/arch/arm/boot/dts/zynq-zc702.dts b/arch/arm/boot/dts/zynq-zc702.dts > index 30bcfe20f0bc..fa810505ab8f 100644 > --- a/arch/arm/boot/dts/zynq-zc702.dts > +++ b/arch/arm/boot/dts/zynq-zc702.dts > @@ -36,6 +36,12 @@ > &gem0 { > status = "okay"; > phy-mode = "rgmii-id"; > + phy-handle = <ðernet_phy>; > + > + ethernet_phy: ethernet-phy@7 { > + compatible = "marvell,88e1116r", "ethernet-phy-ieee802.3-c22"; c22 is completely unused by the kernel and also c22 is default option anyway. Any advantage to have c22 specified here? Thanks, Michal
Am 21.08.2014 10:41, schrieb Michal Simek: > On 08/20/2014 05:56 PM, Soren Brinkmann wrote: >> Add missing Ethernet phys to Zynq DTs. >> >> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> >> --- >> arch/arm/boot/dts/zynq-zc702.dts | 6 ++++++ >> arch/arm/boot/dts/zynq-zc706.dts | 6 ++++++ >> arch/arm/boot/dts/zynq-zed.dts | 6 ++++++ >> 3 files changed, 18 insertions(+) >> >> diff --git a/arch/arm/boot/dts/zynq-zc702.dts b/arch/arm/boot/dts/zynq-zc702.dts >> index 30bcfe20f0bc..fa810505ab8f 100644 >> --- a/arch/arm/boot/dts/zynq-zc702.dts >> +++ b/arch/arm/boot/dts/zynq-zc702.dts >> @@ -36,6 +36,12 @@ >> &gem0 { >> status = "okay"; >> phy-mode = "rgmii-id"; >> + phy-handle = <ðernet_phy>; >> + >> + ethernet_phy: ethernet-phy@7 { >> + compatible = "marvell,88e1116r", "ethernet-phy-ieee802.3-c22"; > > c22 is completely unused by the kernel and also c22 is default option anyway. > Any advantage to have c22 specified here? http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/phy.txt Sören's marvell,* entries do not seem documented, therefore I used the documented "ethernet-phy-idAAAA.BBBB" based syntax. The documented example specifically uses -c22, too. Either it's okay to prepend unrecognized model strings, then you should update zynq-parallella.dts as well (I put the model in a comment there) or use the official strings like I used and keep the readable models as comments. Documenting all those "marvell,88e1116r", "marvell,88e1518", "marvell,88e1318" PHY bindings and possibly prepending them to the ID based strings would be another option, of course. Regards, Andreas
On Thu, 2014-08-21 at 01:32PM +0200, Andreas Färber wrote: > Am 21.08.2014 10:41, schrieb Michal Simek: > > On 08/20/2014 05:56 PM, Soren Brinkmann wrote: > >> Add missing Ethernet phys to Zynq DTs. > >> > >> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > >> --- > >> arch/arm/boot/dts/zynq-zc702.dts | 6 ++++++ > >> arch/arm/boot/dts/zynq-zc706.dts | 6 ++++++ > >> arch/arm/boot/dts/zynq-zed.dts | 6 ++++++ > >> 3 files changed, 18 insertions(+) > >> > >> diff --git a/arch/arm/boot/dts/zynq-zc702.dts b/arch/arm/boot/dts/zynq-zc702.dts > >> index 30bcfe20f0bc..fa810505ab8f 100644 > >> --- a/arch/arm/boot/dts/zynq-zc702.dts > >> +++ b/arch/arm/boot/dts/zynq-zc702.dts > >> @@ -36,6 +36,12 @@ > >> &gem0 { > >> status = "okay"; > >> phy-mode = "rgmii-id"; > >> + phy-handle = <ðernet_phy>; > >> + > >> + ethernet_phy: ethernet-phy@7 { > >> + compatible = "marvell,88e1116r", "ethernet-phy-ieee802.3-c22"; > > > > c22 is completely unused by the kernel and also c22 is default option anyway. > > Any advantage to have c22 specified here? > > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/phy.txt > > Sören's marvell,* entries do not seem documented, therefore I used the > documented "ethernet-phy-idAAAA.BBBB" based syntax. The documented > example specifically uses -c22, too. > > Either it's okay to prepend unrecognized model strings, then you should > update zynq-parallella.dts as well (I put the model in a comment there) > or use the official strings like I used and keep the readable models as > comments. Documenting all those "marvell,88e1116r", "marvell,88e1518", > "marvell,88e1318" PHY bindings and possibly prepending them to the ID > based strings would be another option, of course. These phy-bindings are everything than obvious to me. It seems the docs are spread across a couple of different files and not fully up to date either. I basically tried to get something working out of the docs, the parallela and our vendor DT files. So my thinkings: - the compatible string with the -c22 is used and documented in the phy bindings, it should be there. - the ID based strings seem to be not needed since, IIUC, the core reads the ID from the PHY and uses it, so I just left it out not trying to figure out how to obtain the correct ID - the marvell compatible strings are used in our vendor tree. They aren't used anywhere but in our vendor tree. I though keeping them is nice since it identifies the PHY fully. And in case that level of detail is needed at some point it is already there. Assuming that we wanna keep things this way, I'm happy to re-spin this patch and also add a similar compatible string to the parallela DT. Thanks, Sören
On Thu, Aug 21, 2014 at 08:49:19AM -0700, Sören Brinkmann wrote: > So my thinkings: > - the compatible string with the -c22 is used and documented in the phy > bindings, it should be there. Yes > - the ID based strings seem to be not needed since, IIUC, the core > reads the ID from the PHY and uses it, so I just left it out not > trying to figure out how to obtain the correct ID It is certainly optional, I added the property to solve some obscure problems with probing, but I've noticed people using it more broadly. I suspect it also speeds up booting a tiny bit. > - the marvell compatible strings are used in our vendor tree. They > aren't used anywhere but in our vendor tree. I though keeping them is > nice since it identifies the PHY fully. And in case that level of > detail is needed at some point it is already there. DT is complex enough already, don't include useless compatible strings in mainline - people will see and tend to blindly copy. Ideally your vendor tree would use the standard property :| Jason
On Fri, 2014-08-22 at 10:20AM -0600, Jason Gunthorpe wrote: > On Thu, Aug 21, 2014 at 08:49:19AM -0700, Sören Brinkmann wrote: > > > So my thinkings: > > - the compatible string with the -c22 is used and documented in the phy > > bindings, it should be there. > > Yes > > > - the ID based strings seem to be not needed since, IIUC, the core > > reads the ID from the PHY and uses it, so I just left it out not > > trying to figure out how to obtain the correct ID > > It is certainly optional, I added the property to solve some obscure > problems with probing, but I've noticed people using it more broadly. > I suspect it also speeds up booting a tiny bit. > > > - the marvell compatible strings are used in our vendor tree. They > > aren't used anywhere but in our vendor tree. I though keeping them is > > nice since it identifies the PHY fully. And in case that level of > > detail is needed at some point it is already there. > > DT is complex enough already, don't include useless compatible strings > in mainline - people will see and tend to blindly copy. Ideally your > vendor tree would use the standard property :| There are different opinions on this. I also heard to just add such strings, so in case this level of differentiation becomes necessary at some later point, it is already existing. Sören
On Fri, Aug 22, 2014 at 09:31:20AM -0700, Sören Brinkmann wrote: > > > - the marvell compatible strings are used in our vendor tree. They > > > aren't used anywhere but in our vendor tree. I though keeping them is > > > nice since it identifies the PHY fully. And in case that level of > > > detail is needed at some point it is already there. > > > > DT is complex enough already, don't include useless compatible strings > > in mainline - people will see and tend to blindly copy. Ideally your > > vendor tree would use the standard property :| > > There are different opinions on this. I also heard to just add such > strings, so in case this level of differentiation becomes necessary at > some later point, it is already existing. In some cases, yes, and they should be docuemented in the binding, of course. But there is already a standard compatible string format to uniquely identify a phy, so in this case another string is not justified, and just creates confusion about what is correct. Jason
2014-08-21 8:49 GMT-07:00 Sören Brinkmann <soren.brinkmann@xilinx.com>: > On Thu, 2014-08-21 at 01:32PM +0200, Andreas Färber wrote: >> Am 21.08.2014 10:41, schrieb Michal Simek: >> > On 08/20/2014 05:56 PM, Soren Brinkmann wrote: >> >> Add missing Ethernet phys to Zynq DTs. >> >> >> >> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> >> >> --- >> >> arch/arm/boot/dts/zynq-zc702.dts | 6 ++++++ >> >> arch/arm/boot/dts/zynq-zc706.dts | 6 ++++++ >> >> arch/arm/boot/dts/zynq-zed.dts | 6 ++++++ >> >> 3 files changed, 18 insertions(+) >> >> >> >> diff --git a/arch/arm/boot/dts/zynq-zc702.dts b/arch/arm/boot/dts/zynq-zc702.dts >> >> index 30bcfe20f0bc..fa810505ab8f 100644 >> >> --- a/arch/arm/boot/dts/zynq-zc702.dts >> >> +++ b/arch/arm/boot/dts/zynq-zc702.dts >> >> @@ -36,6 +36,12 @@ >> >> &gem0 { >> >> status = "okay"; >> >> phy-mode = "rgmii-id"; >> >> + phy-handle = <ðernet_phy>; >> >> + >> >> + ethernet_phy: ethernet-phy@7 { >> >> + compatible = "marvell,88e1116r", "ethernet-phy-ieee802.3-c22"; >> > >> > c22 is completely unused by the kernel and also c22 is default option anyway. >> > Any advantage to have c22 specified here? >> >> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/phy.txt >> >> Sören's marvell,* entries do not seem documented, therefore I used the >> documented "ethernet-phy-idAAAA.BBBB" based syntax. The documented >> example specifically uses -c22, too. >> >> Either it's okay to prepend unrecognized model strings, then you should >> update zynq-parallella.dts as well (I put the model in a comment there) >> or use the official strings like I used and keep the readable models as >> comments. Documenting all those "marvell,88e1116r", "marvell,88e1518", >> "marvell,88e1318" PHY bindings and possibly prepending them to the ID >> based strings would be another option, of course. > > These phy-bindings are everything than obvious to me. It seems the docs > are spread across a couple of different files and not fully up to date > either. I basically tried to get something working out of the docs, the > parallela and our vendor DT files. There is one file that describes everything that is needed in Documentation/devicetree/bindings/net/phy.txt, if it is not clear, patches are welcome. > > So my thinkings: > - the compatible string with the -c22 is used and documented in the phy > bindings, it should be there. The clause 22 compatible string is not used, but we do need it to know it is not a clause 45 PHY, which implicitly makes the clause 22 compatible string used. > - the ID based strings seem to be not needed since, IIUC, the core > reads the ID from the PHY and uses it, so I just left it out not > trying to figure out how to obtain the correct ID It is not needed, but it is one way to specify a PHY device if you do not know what compatible string to use instead. > - the marvell compatible strings are used in our vendor tree. They > aren't used anywhere but in our vendor tree. I though keeping them is > nice since it identifies the PHY fully. And in case that level of > detail is needed at some point it is already there. And this is the recommended way to do it in case we ever need to key a software decision based on the hardware. > > Assuming that we wanna keep things this way, I'm happy to re-spin this > patch and also add a similar compatible string to the parallela DT. Your initial patchset is correct with repesct to how the Device Tree binding for Ethernet PHY nodes is defined. > > Thanks, > Sören > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Aug 22, 2014 at 01:47:09PM -0700, Florian Fainelli wrote: > > - the ID based strings seem to be not needed since, IIUC, the core > > reads the ID from the PHY and uses it, so I just left it out not > > trying to figure out how to obtain the correct ID > > It is not needed, but it is one way to specify a PHY device if you do > not know what compatible string to use instead. No, it is a way to specify a PHY device if the kernel can't auto probe the Phy ID. Last I checked, the kernel doesn't support plain text compatible strings for phys - everything is driven on the phy id, either auto probed or specified in the DT. > > - the marvell compatible strings are used in our vendor tree. They > > aren't used anywhere but in our vendor tree. I though keeping them is > > nice since it identifies the PHY fully. And in case that level of > > detail is needed at some point it is already there. > > And this is the recommended way to do it in case we ever need to key a > software decision based on the hardware. All compatible strings need to be documented. .. and they need to encode more information than you get from the phy id - die revsision, package option, functional options, voltage codes. Etc. .. and they actually need to be *right* An example: The kernel reports 88E1318S for all four chips in that family, AFAIK you have to read the package marking to figure out which you have (it is the same die, with options switched on/off at packaging time). People have already posted patches trying to helpfully add a 'marvell,88E1318S' compatible string based on kernel output. Except it is wrong, it isn't actually the '8S version in the HW. Even worse, Marvell has a whole series of socket compatible phys. Just because the board the DT author looked at has a '318, doesn't mean that every board ever made will. We've actually already been switching between the 318 and 318S for production depending on which has part availability. Basically: don't try to override self-discoverable hardware in DT without a really good reason. Jason
On 08/25/2014 10:46 AM, Jason Gunthorpe wrote: > On Fri, Aug 22, 2014 at 01:47:09PM -0700, Florian Fainelli wrote: > >>> - the ID based strings seem to be not needed since, IIUC, the core >>> reads the ID from the PHY and uses it, so I just left it out not >>> trying to figure out how to obtain the correct ID >> >> It is not needed, but it is one way to specify a PHY device if you do >> not know what compatible string to use instead. > > No, it is a way to specify a PHY device if the kernel can't auto probe > the Phy ID. > > Last I checked, the kernel doesn't support plain text compatible > strings for phys - everything is driven on the phy id, either auto > probed or specified in the DT. That's right. Some PHY drivers might be relying on specific compatible strings though, but not the core PHY library that probes and maps a driver to a PHY node. > >>> - the marvell compatible strings are used in our vendor tree. They >>> aren't used anywhere but in our vendor tree. I though keeping them is >>> nice since it identifies the PHY fully. And in case that level of >>> detail is needed at some point it is already there. >> >> And this is the recommended way to do it in case we ever need to key a >> software decision based on the hardware. > > All compatible strings need to be documented. > > .. and they need to encode more information than you get from the phy > id - die revsision, package option, functional options, voltage > codes. Etc. > > .. and they actually need to be *right* Agreed. > > An example: The kernel reports 88E1318S for all four chips in that > family, AFAIK you have to read the package marking to figure out which > you have (it is the same die, with options switched on/off at > packaging time). People have already posted patches trying to > helpfully add a 'marvell,88E1318S' compatible string based on kernel > output. Except it is wrong, it isn't actually the '8S version in the > HW. > > Even worse, Marvell has a whole series of socket compatible phys. Just > because the board the DT author looked at has a '318, doesn't mean > that every board ever made will. We've actually already been switching > between the 318 and 318S for production depending on which has part > availability. > > Basically: don't try to override self-discoverable hardware in DT > without a really good reason. I think that's a very good point, at the very least let's use a compatible string that contains the full 32-bits PHY OUI. Thanks -- Florian
On 08/25/2014 10:21 PM, Florian Fainelli wrote: > On 08/25/2014 10:46 AM, Jason Gunthorpe wrote: >> On Fri, Aug 22, 2014 at 01:47:09PM -0700, Florian Fainelli wrote: >> >>>> - the ID based strings seem to be not needed since, IIUC, the core >>>> reads the ID from the PHY and uses it, so I just left it out not >>>> trying to figure out how to obtain the correct ID >>> >>> It is not needed, but it is one way to specify a PHY device if you do >>> not know what compatible string to use instead. >> >> No, it is a way to specify a PHY device if the kernel can't auto probe >> the Phy ID. >> >> Last I checked, the kernel doesn't support plain text compatible >> strings for phys - everything is driven on the phy id, either auto >> probed or specified in the DT. > > That's right. Some PHY drivers might be relying on specific compatible > strings though, but not the core PHY library that probes and maps a > driver to a PHY node. > >> >>>> - the marvell compatible strings are used in our vendor tree. They >>>> aren't used anywhere but in our vendor tree. I though keeping them is >>>> nice since it identifies the PHY fully. And in case that level of >>>> detail is needed at some point it is already there. >>> >>> And this is the recommended way to do it in case we ever need to key a >>> software decision based on the hardware. >> >> All compatible strings need to be documented. >> >> .. and they need to encode more information than you get from the phy >> id - die revsision, package option, functional options, voltage >> codes. Etc. >> >> .. and they actually need to be *right* > > Agreed. > >> >> An example: The kernel reports 88E1318S for all four chips in that >> family, AFAIK you have to read the package marking to figure out which >> you have (it is the same die, with options switched on/off at >> packaging time). People have already posted patches trying to >> helpfully add a 'marvell,88E1318S' compatible string based on kernel >> output. Except it is wrong, it isn't actually the '8S version in the >> HW. >> >> Even worse, Marvell has a whole series of socket compatible phys. Just >> because the board the DT author looked at has a '318, doesn't mean >> that every board ever made will. We've actually already been switching >> between the 318 and 318S for production depending on which has part >> availability. >> >> Basically: don't try to override self-discoverable hardware in DT >> without a really good reason. > > I think that's a very good point, at the very least let's use a > compatible string that contains the full 32-bits PHY OUI. I think resolution is: 1. Do not use marvell,88e1518 because it is not listed anywhere 2. Do not add ethernet-phy-idAAAA.BBBB because it breaks autodetection if there is different phy on the board and we shouldn't restrict us in this. In spite of autodetection takes some time. 3. "ethernet-phy-ieee802.3-c22" is optional that's why doesn't need to be added 4. Any listed compatible string has to be parsed which takes time That's why I think make sense not to use any compatible string. This should give us all flexibility which we want to have. Thanks, Michal
Am 29.08.2014 16:08, schrieb Michal Simek: > On 08/25/2014 10:21 PM, Florian Fainelli wrote: >> On 08/25/2014 10:46 AM, Jason Gunthorpe wrote: >>> On Fri, Aug 22, 2014 at 01:47:09PM -0700, Florian Fainelli wrote: >>> >>>>> - the ID based strings seem to be not needed since, IIUC, the core >>>>> reads the ID from the PHY and uses it, so I just left it out not >>>>> trying to figure out how to obtain the correct ID >>>> >>>> It is not needed, but it is one way to specify a PHY device if you do >>>> not know what compatible string to use instead. >>> >>> No, it is a way to specify a PHY device if the kernel can't auto probe >>> the Phy ID. >>> >>> Last I checked, the kernel doesn't support plain text compatible >>> strings for phys - everything is driven on the phy id, either auto >>> probed or specified in the DT. >> >> That's right. Some PHY drivers might be relying on specific compatible >> strings though, but not the core PHY library that probes and maps a >> driver to a PHY node. >> >>> >>>>> - the marvell compatible strings are used in our vendor tree. They >>>>> aren't used anywhere but in our vendor tree. I though keeping them is >>>>> nice since it identifies the PHY fully. And in case that level of >>>>> detail is needed at some point it is already there. >>>> >>>> And this is the recommended way to do it in case we ever need to key a >>>> software decision based on the hardware. >>> >>> All compatible strings need to be documented. >>> >>> .. and they need to encode more information than you get from the phy >>> id - die revsision, package option, functional options, voltage >>> codes. Etc. >>> >>> .. and they actually need to be *right* >> >> Agreed. >> >>> >>> An example: The kernel reports 88E1318S for all four chips in that >>> family, AFAIK you have to read the package marking to figure out which >>> you have (it is the same die, with options switched on/off at >>> packaging time). People have already posted patches trying to >>> helpfully add a 'marvell,88E1318S' compatible string based on kernel >>> output. Except it is wrong, it isn't actually the '8S version in the >>> HW. >>> >>> Even worse, Marvell has a whole series of socket compatible phys. Just >>> because the board the DT author looked at has a '318, doesn't mean >>> that every board ever made will. We've actually already been switching >>> between the 318 and 318S for production depending on which has part >>> availability. >>> >>> Basically: don't try to override self-discoverable hardware in DT >>> without a really good reason. >> >> I think that's a very good point, at the very least let's use a >> compatible string that contains the full 32-bits PHY OUI. > > I think resolution is: > 1. Do not use marvell,88e1518 because it is not listed anywhere > 2. Do not add ethernet-phy-idAAAA.BBBB because it breaks autodetection > if there is different phy on the board and we shouldn't restrict us in this. > In spite of autodetection takes some time. > 3. "ethernet-phy-ieee802.3-c22" is optional that's why doesn't need to be added > 4. Any listed compatible string has to be parsed which takes time > > That's why I think make sense not to use any compatible string. > This should give us all flexibility which we want to have. Sorry, but we do need some node that we can reference via phy-handle from the gem node, don't we? In that case, is not specifying any compatible string a valid option? If you don't want to specify the IDs, then I would've assumed we need to specify the -c22 in order to have *some* compatible string in order to trigger loading of the appropriate driver module. Regards, Andreas
On Fri, 2014-08-29 at 05:18PM +0200, Andreas Färber wrote: > Am 29.08.2014 16:08, schrieb Michal Simek: > > On 08/25/2014 10:21 PM, Florian Fainelli wrote: > >> On 08/25/2014 10:46 AM, Jason Gunthorpe wrote: > >>> On Fri, Aug 22, 2014 at 01:47:09PM -0700, Florian Fainelli wrote: > >>> > >>>>> - the ID based strings seem to be not needed since, IIUC, the core > >>>>> reads the ID from the PHY and uses it, so I just left it out not > >>>>> trying to figure out how to obtain the correct ID > >>>> > >>>> It is not needed, but it is one way to specify a PHY device if you do > >>>> not know what compatible string to use instead. > >>> > >>> No, it is a way to specify a PHY device if the kernel can't auto probe > >>> the Phy ID. > >>> > >>> Last I checked, the kernel doesn't support plain text compatible > >>> strings for phys - everything is driven on the phy id, either auto > >>> probed or specified in the DT. > >> > >> That's right. Some PHY drivers might be relying on specific compatible > >> strings though, but not the core PHY library that probes and maps a > >> driver to a PHY node. > >> > >>> > >>>>> - the marvell compatible strings are used in our vendor tree. They > >>>>> aren't used anywhere but in our vendor tree. I though keeping them is > >>>>> nice since it identifies the PHY fully. And in case that level of > >>>>> detail is needed at some point it is already there. > >>>> > >>>> And this is the recommended way to do it in case we ever need to key a > >>>> software decision based on the hardware. > >>> > >>> All compatible strings need to be documented. > >>> > >>> .. and they need to encode more information than you get from the phy > >>> id - die revsision, package option, functional options, voltage > >>> codes. Etc. > >>> > >>> .. and they actually need to be *right* > >> > >> Agreed. > >> > >>> > >>> An example: The kernel reports 88E1318S for all four chips in that > >>> family, AFAIK you have to read the package marking to figure out which > >>> you have (it is the same die, with options switched on/off at > >>> packaging time). People have already posted patches trying to > >>> helpfully add a 'marvell,88E1318S' compatible string based on kernel > >>> output. Except it is wrong, it isn't actually the '8S version in the > >>> HW. > >>> > >>> Even worse, Marvell has a whole series of socket compatible phys. Just > >>> because the board the DT author looked at has a '318, doesn't mean > >>> that every board ever made will. We've actually already been switching > >>> between the 318 and 318S for production depending on which has part > >>> availability. > >>> > >>> Basically: don't try to override self-discoverable hardware in DT > >>> without a really good reason. > >> > >> I think that's a very good point, at the very least let's use a > >> compatible string that contains the full 32-bits PHY OUI. > > > > I think resolution is: > > 1. Do not use marvell,88e1518 because it is not listed anywhere > > 2. Do not add ethernet-phy-idAAAA.BBBB because it breaks autodetection > > if there is different phy on the board and we shouldn't restrict us in this. > > In spite of autodetection takes some time. > > 3. "ethernet-phy-ieee802.3-c22" is optional that's why doesn't need to be added > > 4. Any listed compatible string has to be parsed which takes time > > > > That's why I think make sense not to use any compatible string. > > This should give us all flexibility which we want to have. > > Sorry, but we do need some node that we can reference via phy-handle > from the gem node, don't we? > > In that case, is not specifying any compatible string a valid option? > > If you don't want to specify the IDs, then I would've assumed we need to > specify the -c22 in order to have *some* compatible string in order to > trigger loading of the appropriate driver module. The compatible string is listed as optional property for PHYs. So, not having one is an option, I guess. But, I'd also prefer to at least keep the -c22 one, since I saw problems when I tried using -c45 (the Zed phy should support it...). Also, so far, we haven't had any phy nodes in our Zynq dts files and Ethernet worked, so the auto-detection there works pretty well apparently. But it may be problematic if more than a single PHY is on the MDIO bus, I'd assume. Sören
Am 29.08.2014 17:35, schrieb Sören Brinkmann: > On Fri, 2014-08-29 at 05:18PM +0200, Andreas Färber wrote: >> Am 29.08.2014 16:08, schrieb Michal Simek: >>> I think resolution is: >>> 1. Do not use marvell,88e1518 because it is not listed anywhere >>> 2. Do not add ethernet-phy-idAAAA.BBBB because it breaks autodetection >>> if there is different phy on the board and we shouldn't restrict us in this. >>> In spite of autodetection takes some time. >>> 3. "ethernet-phy-ieee802.3-c22" is optional that's why doesn't need to be added >>> 4. Any listed compatible string has to be parsed which takes time >>> >>> That's why I think make sense not to use any compatible string. >>> This should give us all flexibility which we want to have. >> >> Sorry, but we do need some node that we can reference via phy-handle >> from the gem node, don't we? >> >> In that case, is not specifying any compatible string a valid option? >> >> If you don't want to specify the IDs, then I would've assumed we need to >> specify the -c22 in order to have *some* compatible string in order to >> trigger loading of the appropriate driver module. > > The compatible string is listed as optional property for PHYs. So, not > having one is an option, I guess. But, I'd also prefer to at least keep > the -c22 one, since I saw problems when I tried using -c45 (the Zed phy > should support it...). > Also, so far, we haven't had any phy nodes in our Zynq dts files and > Ethernet worked, so the auto-detection there works pretty well > apparently. But it may be problematic if more than a single PHY is on > the MDIO bus, I'd assume. Hm, it didn't work for me initially, but maybe some config option was missing in multi_v7 or something else. I'll re-test next week. Andreas
On Fri, Aug 29, 2014 at 08:35:36AM -0700, Sören Brinkmann wrote: > The compatible string is listed as optional property for PHYs. So, not > having one is an option, I guess. But, I'd also prefer to at least keep > the -c22 one, since I saw problems when I tried using -c45 (the Zed phy > should support it...). -c45 and -c22 use a completely different MDIO protocol, Zed doesn't have a 10GE port, so it certainly doesn't use -c45. If there is no compatible string at all the defined default is to use -c22. > Also, so far, we haven't had any phy nodes in our Zynq dts files and > Ethernet worked, so the auto-detection there works pretty well > apparently. But it may be problematic if more than a single PHY is on > the MDIO bus, I'd assume. Phy autodetection has always worked in some cases, but for DT ethernet bindings it is expected that there is an option to specify an explicit MDIO bus, and a phy-handle phandle to point to the phy. The phy should be explicitly called out with the fixed MDIO address specified to avoid sketchy MDIO address auto probing. This provides the framework to specify interrupts and register overrides for the phy in the DT. Jason
On 08/29/2014 10:31 AM, Jason Gunthorpe wrote: > On Fri, Aug 29, 2014 at 08:35:36AM -0700, Sören Brinkmann wrote: > >> The compatible string is listed as optional property for PHYs. So, not >> having one is an option, I guess. But, I'd also prefer to at least keep >> the -c22 one, since I saw problems when I tried using -c45 (the Zed phy >> should support it...). > > -c45 and -c22 use a completely different MDIO protocol, Zed doesn't > have a 10GE port, so it certainly doesn't use -c45. Most recent 1GbE PHYs should also implement clause 45. It is a nice improvement if you are using lot of transactions, otherwise clause 45 over clause 22 is suitable and supported by the PHY library (for EEE in particular). > > If there is no compatible string at all the defined default is to use > -c22. Absolutely, the current binding makes that property optional, but I think we should make it mandatory instead. > >> Also, so far, we haven't had any phy nodes in our Zynq dts files and >> Ethernet worked, so the auto-detection there works pretty well >> apparently. But it may be problematic if more than a single PHY is on >> the MDIO bus, I'd assume. > > Phy autodetection has always worked in some cases, but for DT ethernet > bindings it is expected that there is an option to specify an explicit > MDIO bus, and a phy-handle phandle to point to the phy. The phy should > be explicitly called out with the fixed MDIO address specified to > avoid sketchy MDIO address auto probing. > > This provides the framework to specify interrupts and register > overrides for the phy in the DT. Absolutely, on the plus side, avoiding auto-detection also allows you to have a shorter boot time (usually by a factor of few hundreds of msecs depending on how the MDIO buses handle errors). -- Florian
On Fri, Aug 29, 2014 at 11:23:57AM -0700, Florian Fainelli wrote: > On 08/29/2014 10:31 AM, Jason Gunthorpe wrote: > > On Fri, Aug 29, 2014 at 08:35:36AM -0700, Sören Brinkmann wrote: > > > >> The compatible string is listed as optional property for PHYs. So, not > >> having one is an option, I guess. But, I'd also prefer to at least keep > >> the -c22 one, since I saw problems when I tried using -c45 (the Zed phy > >> should support it...). > > > > -c45 and -c22 use a completely different MDIO protocol, Zed doesn't > > have a 10GE port, so it certainly doesn't use -c45. > > Most recent 1GbE PHYs should also implement clause 45. It is a nice > improvement if you are using lot of transactions, otherwise clause 45 > over clause 22 is suitable and supported by the PHY library (for EEE in > particular). Oh, that is interesting, I haven't actually seen one of those yet.. Hum. So that is messy, even if the Zed phy supports the C45 format, the macb driver (and by my reading, the Zynq hardware) lacks the capability to generate C45 frames. It could be supported, but you'd have to use the GPIO bitbang MDIO driver to talk to the phy. So... that makes the compatible string for the phy even more confusing. 'Describe The HW' says it should have both c22 and c45 listed - however we don't have software support in Linux to negotiate c22 and c45 support between the phy bus driver and attached phy :( Jason
On 08/29/2014 04:22 PM, Jason Gunthorpe wrote: > On Fri, Aug 29, 2014 at 11:23:57AM -0700, Florian Fainelli wrote: >> On 08/29/2014 10:31 AM, Jason Gunthorpe wrote: >>> On Fri, Aug 29, 2014 at 08:35:36AM -0700, Sören Brinkmann wrote: >>> >>>> The compatible string is listed as optional property for PHYs. So, not >>>> having one is an option, I guess. But, I'd also prefer to at least keep >>>> the -c22 one, since I saw problems when I tried using -c45 (the Zed phy >>>> should support it...). >>> >>> -c45 and -c22 use a completely different MDIO protocol, Zed doesn't >>> have a 10GE port, so it certainly doesn't use -c45. >> >> Most recent 1GbE PHYs should also implement clause 45. It is a nice >> improvement if you are using lot of transactions, otherwise clause 45 >> over clause 22 is suitable and supported by the PHY library (for EEE in >> particular). > > Oh, that is interesting, I haven't actually seen one of those yet.. > > Hum. So that is messy, even if the Zed phy supports the C45 format, > the macb driver (and by my reading, the Zynq hardware) lacks the > capability to generate C45 frames. We should restrict ourselves to clause 45 over clause 22 compatibility mode, which requires no MDIO bus driver changes. > > It could be supported, but you'd have to use the GPIO bitbang MDIO > driver to talk to the phy. > > So... that makes the compatible string for the phy even more > confusing. 'Describe The HW' says it should have both c22 and c45 > listed - however we don't have software support in Linux to negotiate > c22 and c45 support between the phy bus driver and attached phy :( Right now, if you set the c45 compatible string, the MDIO bus driver and the PHY must support native c45 transactions to set phydev->is_c45, if one or the other, or neither of those work, we will fallback to c22. The part that is not figured out properly yet is how do we want to handle functions (e.g: EEE) that are only accessible using c45 (native or in compatible mode), since the PHY library uses two pairs of accessors, with the native accessors not falling back to the indirect accessor... -- Florian
On 08/30/2014 02:43 AM, Florian Fainelli wrote: > On 08/29/2014 04:22 PM, Jason Gunthorpe wrote: >> On Fri, Aug 29, 2014 at 11:23:57AM -0700, Florian Fainelli wrote: >>> On 08/29/2014 10:31 AM, Jason Gunthorpe wrote: >>>> On Fri, Aug 29, 2014 at 08:35:36AM -0700, Sören Brinkmann wrote: >>>> >>>>> The compatible string is listed as optional property for PHYs. So, not >>>>> having one is an option, I guess. But, I'd also prefer to at least keep >>>>> the -c22 one, since I saw problems when I tried using -c45 (the Zed phy >>>>> should support it...). >>>> >>>> -c45 and -c22 use a completely different MDIO protocol, Zed doesn't >>>> have a 10GE port, so it certainly doesn't use -c45. >>> >>> Most recent 1GbE PHYs should also implement clause 45. It is a nice >>> improvement if you are using lot of transactions, otherwise clause 45 >>> over clause 22 is suitable and supported by the PHY library (for EEE in >>> particular). >> >> Oh, that is interesting, I haven't actually seen one of those yet.. >> >> Hum. So that is messy, even if the Zed phy supports the C45 format, >> the macb driver (and by my reading, the Zynq hardware) lacks the >> capability to generate C45 frames. > > We should restrict ourselves to clause 45 over clause 22 compatibility > mode, which requires no MDIO bus driver changes. > >> >> It could be supported, but you'd have to use the GPIO bitbang MDIO >> driver to talk to the phy. >> >> So... that makes the compatible string for the phy even more >> confusing. 'Describe The HW' says it should have both c22 and c45 >> listed - however we don't have software support in Linux to negotiate >> c22 and c45 support between the phy bus driver and attached phy :( > > Right now, if you set the c45 compatible string, the MDIO bus driver and > the PHY must support native c45 transactions to set phydev->is_c45, if > one or the other, or neither of those work, we will fallback to c22. > > The part that is not figured out properly yet is how do we want to > handle functions (e.g: EEE) that are only accessible using c45 (native > or in compatible mode), since the PHY library uses two pairs of > accessors, with the native accessors not falling back to the indirect > accessor... ok. I have read all responses listed here and still IMHO the best resolution is not to add any compatible string. I agree with Florian that we can have shorten boot time when idAAAA.BBBB is used but also we are not checking that phy can be detect. I believe all my points in my response are still valid and we have 3 options. 1. not to add any compatible string and use autodetection 2. Add idAAAA.BBBB to shorten boot up time 3. Add c22 4. Add c22 and idAAAA.BBBB My preference is 1. because there could be problem with MII setting and autodetection is good proof that everything is working. If any user wants to have short boot up time it can specify ID in DTS and also I believe DTS will be much longer because of some IPs in PL. Thanks, Michal
diff --git a/arch/arm/boot/dts/zynq-zc702.dts b/arch/arm/boot/dts/zynq-zc702.dts index 30bcfe20f0bc..fa810505ab8f 100644 --- a/arch/arm/boot/dts/zynq-zc702.dts +++ b/arch/arm/boot/dts/zynq-zc702.dts @@ -36,6 +36,12 @@ &gem0 { status = "okay"; phy-mode = "rgmii-id"; + phy-handle = <ðernet_phy>; + + ethernet_phy: ethernet-phy@7 { + compatible = "marvell,88e1116r", "ethernet-phy-ieee802.3-c22"; + reg = <7>; + }; }; &i2c0 { diff --git a/arch/arm/boot/dts/zynq-zc706.dts b/arch/arm/boot/dts/zynq-zc706.dts index 3d9cb023e158..d446b44d45a2 100644 --- a/arch/arm/boot/dts/zynq-zc706.dts +++ b/arch/arm/boot/dts/zynq-zc706.dts @@ -33,6 +33,12 @@ &gem0 { status = "okay"; phy-mode = "rgmii-id"; + phy-handle = <ðernet_phy>; + + ethernet_phy: ethernet-phy@7 { + compatible = "marvell,88e1116r", "ethernet-phy-ieee802.3-c22"; + reg = <7>; + }; }; &i2c0 { diff --git a/arch/arm/boot/dts/zynq-zed.dts b/arch/arm/boot/dts/zynq-zed.dts index 42deb1b7dbc8..92662eb078ce 100644 --- a/arch/arm/boot/dts/zynq-zed.dts +++ b/arch/arm/boot/dts/zynq-zed.dts @@ -33,6 +33,12 @@ &gem0 { status = "okay"; phy-mode = "rgmii-id"; + phy-handle = <ðernet_phy>; + + ethernet_phy: ethernet-phy@0 { + compatible = "marvell,88e1518", "ethernet-phy-ieee802.3-c22"; + reg = <0>; + }; }; &sdhci0 {
Add missing Ethernet phys to Zynq DTs. Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> --- arch/arm/boot/dts/zynq-zc702.dts | 6 ++++++ arch/arm/boot/dts/zynq-zc706.dts | 6 ++++++ arch/arm/boot/dts/zynq-zed.dts | 6 ++++++ 3 files changed, 18 insertions(+)