Re: [PATCH] DSA support for Micrel KSZ8895
> The patches are under review internally and will need to be updated > and approved by Woojung before formal submission. Problem is > although KSZ8795 and KSZ8895 drivers are new code and will be > submitted as RFC, they depend on the change of KSZ9477 driver > currently in the kernel, which require more rigorous review. Please be aware that they will also go though review when you post them. This can be anything from great, nice job, to throw them away and start again. Since you are submitting RFCs we understand it is early code, issues still to be solved, and we can make suggestions how to solve those issues. Post early, post often... Andrew
RE: [PATCH] DSA support for Micrel KSZ8895
> -Original Message- > From: Maxim Uvarov [mailto:muva...@gmail.com] > Sent: Wednesday, September 06, 2017 2:15 AM > To: Tristram Ha - C24268 > Cc: Pavel Machek; Woojung Huh - C21699; Nathan Conrad; Vivien Didelot; > Florian Fainelli; netdev; linux-ker...@vger.kernel.org; Andrew Lunn > Subject: Re: [PATCH] DSA support for Micrel KSZ8895 > > 2017-08-31 0:32 GMT+03:00 <tristram...@microchip.com>: > >> On Mon 2017-08-28 16:09:27, Andrew Lunn wrote: > >> > > I may be confused here, but AFAICT: > >> > > > >> > > 1) Yes, it has standard layout when accessed over MDIO. > >> > > >> > > >> > Section 4.8 of the datasheet says: > >> > > >> > All the registers defined in this section can be also accessed > >> > via the SPI interface. > >> > > >> > Meaning all PHY registers can be access via the SPI interface. So > >> > you should be able to make a standard Linux MDIO bus driver which > >> > performs SPI reads. > >> > >> As far as I can tell (and their driver confirms) -- yes, all those > >> registers can be accessed over the SPI, they are just shuffled > >> around... hence MDIO emulation code. I copied it from their code (see > >> the copyrights) so no, I don't believe there's nicer solution. > >> > >> Best regards, > >> > >> > >> Pavel > > > > Can you hold on your developing work on KSZ8895 driver? I am afraid your > effort may be in vain. We at Microchip are planning to release DSA drivers > for all KSZ switches, starting at KSZ8795, then KSZ8895, and KSZ8863. > > > > The driver files all follow the structures of the current KSZ9477 DSA > > driver, > and the file tag_ksz.c will be updated to handle the tail tag of different > chips, > which requires including the ksz_priv.h header. That is required > nevertheless to support using the offload_fwd_mark indication. > > > > The KSZ8795 driver will be submitted after Labor Day (9/4) if testing > > reveals > no problem. The KSZ8895 driver will be submitted right after that. You > should have no problem using the driver right away. > > > > Hello Tristram, is there any update for that driver? > > Maxim. > The patches are under review internally and will need to be updated and approved by Woojung before formal submission. Problem is although KSZ8795 and KSZ8895 drivers are new code and will be submitted as RFC, they depend on the change of KSZ9477 driver currently in the kernel, which require more rigorous review.
Re: [PATCH] DSA support for Micrel KSZ8895
2017-08-31 0:32 GMT+03:00: >> On Mon 2017-08-28 16:09:27, Andrew Lunn wrote: >> > > I may be confused here, but AFAICT: >> > > >> > > 1) Yes, it has standard layout when accessed over MDIO. >> > >> > >> > Section 4.8 of the datasheet says: >> > >> > All the registers defined in this section can be also accessed >> > via the SPI interface. >> > >> > Meaning all PHY registers can be access via the SPI interface. So you >> > should be able to make a standard Linux MDIO bus driver which performs >> > SPI reads. >> >> As far as I can tell (and their driver confirms) -- yes, all those registers >> can be >> accessed over the SPI, they are just shuffled around... hence MDIO >> emulation code. I copied it from their code (see the copyrights) so no, I >> don't >> believe there's nicer solution. >> >> Best regards, >> >> Pavel > > Can you hold on your developing work on KSZ8895 driver? I am afraid your > effort may be in vain. We at Microchip are planning to release DSA drivers > for all KSZ switches, starting at KSZ8795, then KSZ8895, and KSZ8863. > > The driver files all follow the structures of the current KSZ9477 DSA driver, > and the file tag_ksz.c will be updated to handle the tail tag of different > chips, which requires including the ksz_priv.h header. That is required > nevertheless to support using the offload_fwd_mark indication. > > The KSZ8795 driver will be submitted after Labor Day (9/4) if testing reveals > no problem. The KSZ8895 driver will be submitted right after that. You > should have no problem using the driver right away. > Hello Tristram, is there any update for that driver? Maxim. > Tristram Ha > Principal Software Engineer > Microchip Technology Inc. > -- Best regards, Maxim Uvarov
Re: [PATCH] DSA support for Micrel KSZ8895
Hi! > Section 4.8 of the datasheet says: > > All the registers defined in this section can be also accessed > via the SPI interface. > > Meaning all PHY registers can be access via the SPI interface. So you > should be able to make a standard Linux MDIO bus driver which performs > SPI reads. > >>> > >>> As far as I can tell (and their driver confirms) -- yes, all those > >>> registers can be > >>> accessed over the SPI, they are just shuffled around... hence MDIO > >>> emulation code. I copied it from their code (see the copyrights) so no, I > >>> don't > >>> believe there's nicer solution. > >>> > >>> Best regards, > >> > >> Can you hold on your developing work on KSZ8895 driver? I am afraid your > >> effort may be in vain. We at Microchip are planning to release DSA > >> drivers for all KSZ switches, starting at KSZ8795, then KSZ8895, and > >> KSZ8863. > >> > > > > Well, thanks for heads up... but its too late to stop now. I already > > have working code, without the advanced features. > > No driver has landed yet nor has any driver been posted in a proper form > or shape, so at this point neither of you are able to make any claims as > to which one should be chosen. I certainly do not want to make any claims. Tristram's driver is likely to support all (most?) features of the chip, which is not my goal. > > I don't know how far away you are with the development. You may want > > to start from my driver (but its probably too late now). > > I would tend to favor Tristram's submission when we see it because he > claims support for more devices and it is likely to be backed and > maintained by Microchip in the future. Well, I guess we decide when we see the code, that's how it works, right? > I am sure there will be opportunity for you to contribute a lot to this > driver. Of course, this all depends on the code quality and timing, but > having two people work on the same things in parallel is just a complete > waste of each other's time so we might as well wait for Tristram to post > the said driver and define a plan of action from there? Well, it would be good to see the code, so we can judge the quality. Normally, code is posted before testing, so this kind of problems does not arise. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH] DSA support for Micrel KSZ8895
On 09/01/2017 05:15 AM, Pavel Machek wrote: > Hi! > > On Wed 2017-08-30 21:32:07, tristram...@microchip.com wrote: >>> On Mon 2017-08-28 16:09:27, Andrew Lunn wrote: > I may be confused here, but AFAICT: > > 1) Yes, it has standard layout when accessed over MDIO. Section 4.8 of the datasheet says: All the registers defined in this section can be also accessed via the SPI interface. Meaning all PHY registers can be access via the SPI interface. So you should be able to make a standard Linux MDIO bus driver which performs SPI reads. >>> >>> As far as I can tell (and their driver confirms) -- yes, all those >>> registers can be >>> accessed over the SPI, they are just shuffled around... hence MDIO >>> emulation code. I copied it from their code (see the copyrights) so no, I >>> don't >>> believe there's nicer solution. >>> >>> Best regards, >> >> Can you hold on your developing work on KSZ8895 driver? I am afraid your >> effort may be in vain. We at Microchip are planning to release DSA drivers >> for all KSZ switches, starting at KSZ8795, then KSZ8895, and KSZ8863. >> > > Well, thanks for heads up... but its too late to stop now. I already > have working code, without the advanced features. No driver has landed yet nor has any driver been posted in a proper form or shape, so at this point neither of you are able to make any claims as to which one should be chosen. > > I don't know how far away you are with the development. You may want > to start from my driver (but its probably too late now). I would tend to favor Tristram's submission when we see it because he claims support for more devices and it is likely to be backed and maintained by Microchip in the future. I am sure there will be opportunity for you to contribute a lot to this driver. Of course, this all depends on the code quality and timing, but having two people work on the same things in parallel is just a complete waste of each other's time so we might as well wait for Tristram to post the said driver and define a plan of action from there? -- Florian
Re: [PATCH] DSA support for Micrel KSZ8895
Hi! On Wed 2017-08-30 21:32:07, tristram...@microchip.com wrote: > > On Mon 2017-08-28 16:09:27, Andrew Lunn wrote: > > > > I may be confused here, but AFAICT: > > > > > > > > 1) Yes, it has standard layout when accessed over MDIO. > > > > > > > > > Section 4.8 of the datasheet says: > > > > > > All the registers defined in this section can be also accessed > > > via the SPI interface. > > > > > > Meaning all PHY registers can be access via the SPI interface. So you > > > should be able to make a standard Linux MDIO bus driver which performs > > > SPI reads. > > > > As far as I can tell (and their driver confirms) -- yes, all those > > registers can be > > accessed over the SPI, they are just shuffled around... hence MDIO > > emulation code. I copied it from their code (see the copyrights) so no, I > > don't > > believe there's nicer solution. > > > > Best regards, > > Can you hold on your developing work on KSZ8895 driver? I am afraid your > effort may be in vain. We at Microchip are planning to release DSA drivers > for all KSZ switches, starting at KSZ8795, then KSZ8895, and KSZ8863. > Well, thanks for heads up... but its too late to stop now. I already have working code, without the advanced features. I don't know how far away you are with the development. You may want to start from my driver (but its probably too late now). > The driver files all follow the structures of the current KSZ9477 DSA driver, > and the file tag_ksz.c will be updated to handle the tail tag of different > chips, which requires including the ksz_priv.h header. That is required > nevertheless to support using the offload_fwd_mark indication. > > The KSZ8795 driver will be submitted after Labor Day (9/4) if > testing reveals no problem. The KSZ8895 driver will be submitted > right after that. You should have no problem using the driver right > away. Well, normally world can help with the testing, too. It would be nice to see the code, as [RFC]. There's great chance that you'll have to modify the code to adress review feedback, which is why seeing code soon is useful. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH] DSA support for Micrel KSZ8895
> The KSZ8795 driver will be submitted after Labor Day (9/4) if > testing reveals no problem. The KSZ8895 driver will be submitted > right after that. You should have no problem using the driver right > away. Hi Tristram Release early, release often. It stops people wasting time Also, we are likely to give you feedback, asking you to make changes. Testing is important, but you are probably going to have to do it a number of times. So it is not everything passes now, don't worry, you will have time to fix things up as you go through review cycles. Andrew
RE: [PATCH] DSA support for Micrel KSZ8895
> On Mon 2017-08-28 16:09:27, Andrew Lunn wrote: > > > I may be confused here, but AFAICT: > > > > > > 1) Yes, it has standard layout when accessed over MDIO. > > > > > > Section 4.8 of the datasheet says: > > > > All the registers defined in this section can be also accessed > > via the SPI interface. > > > > Meaning all PHY registers can be access via the SPI interface. So you > > should be able to make a standard Linux MDIO bus driver which performs > > SPI reads. > > As far as I can tell (and their driver confirms) -- yes, all those registers > can be > accessed over the SPI, they are just shuffled around... hence MDIO > emulation code. I copied it from their code (see the copyrights) so no, I > don't > believe there's nicer solution. > > Best regards, > > Pavel Can you hold on your developing work on KSZ8895 driver? I am afraid your effort may be in vain. We at Microchip are planning to release DSA drivers for all KSZ switches, starting at KSZ8795, then KSZ8895, and KSZ8863. The driver files all follow the structures of the current KSZ9477 DSA driver, and the file tag_ksz.c will be updated to handle the tail tag of different chips, which requires including the ksz_priv.h header. That is required nevertheless to support using the offload_fwd_mark indication. The KSZ8795 driver will be submitted after Labor Day (9/4) if testing reveals no problem. The KSZ8895 driver will be submitted right after that. You should have no problem using the driver right away. Tristram Ha Principal Software Engineer Microchip Technology Inc.
Re: [PATCH] DSA support for Micrel KSZ8895
2017-08-30 0:23 GMT+03:00 Florian Fainelli: > On 08/29/2017 02:15 PM, Pavel Machek wrote: >> On Tue 2017-08-29 14:26:04, Andrew Lunn wrote: But the MDIO emaulation code is from their driver, after lots of deletions. >>> >>> Is this driver supposed to run on lots of different OSs? That would >>> explain why they ignored the Linux MDIO and PHY layers. >> >> It did not look particulary portable. > > Part of the problem is that they need to duplicate the standard MII > definitions, whereas we could re-use those from include/linux/mii.h > and/or mdio.h. > >> >>> If possible, please make use of the Linux infrastructure. >> >> I did not find any infrastructure I could use instead >> ksz_mdio_emulation. > > fixed PHY/swphy.c is as close as it could get, but it is a highly > simplified version of this. > >> >> Now, drivers/net/phy/spi_ks8995.c can access the PHY registers, and I >> can not see any translation there, so there may be something I'm >> missing. > > I don't see anything in that driver that seems to access PHY registers > what makes you think it does? > > There's got to be a way to perform indirect accesses through SPI, > Woojung, do you know? > As I understand they just attach phy on spi bus with generic driver: phy_addr = 0; phy_mode = PHY_INTERFACE_MODE_MII; snprintf(bus_id, MII_BUS_ID_SIZE, "sw.%d", sw_device_present); snprintf(phy_id, MII_BUS_ID_SIZE, PHY_ID_FMT, bus_id, phy_addr); phydev = phy_attach(netdev, phy_id, 0, phy_mode); if (!IS_ERR(phydev)) { phydev->adjust_link = sw_adjust_link; return phydev; } Where is bus is: bus = mdiobus_alloc(); bus->read = ksz_mii_read; (spi read function) bus->write = ksz_mii_write; Then just generic reads: .config_aneg= genphy_config_aneg, .read_status= genphy_read_status, Maxim. >> >> Pointers would be welcome at this point. >> >> Thanks, >> Pavel >> > > > -- > Florian -- Best regards, Maxim Uvarov
Re: [PATCH] DSA support for Micrel KSZ8895
On 08/29/2017 02:15 PM, Pavel Machek wrote: > On Tue 2017-08-29 14:26:04, Andrew Lunn wrote: >>> But the MDIO emaulation code is from their driver, after lots of >>> deletions. >> >> Is this driver supposed to run on lots of different OSs? That would >> explain why they ignored the Linux MDIO and PHY layers. > > It did not look particulary portable. Part of the problem is that they need to duplicate the standard MII definitions, whereas we could re-use those from include/linux/mii.h and/or mdio.h. > >> If possible, please make use of the Linux infrastructure. > > I did not find any infrastructure I could use instead > ksz_mdio_emulation. fixed PHY/swphy.c is as close as it could get, but it is a highly simplified version of this. > > Now, drivers/net/phy/spi_ks8995.c can access the PHY registers, and I > can not see any translation there, so there may be something I'm > missing. I don't see anything in that driver that seems to access PHY registers what makes you think it does? There's got to be a way to perform indirect accesses through SPI, Woojung, do you know? > > Pointers would be welcome at this point. > > Thanks, > Pavel > -- Florian
Re: [PATCH] DSA support for Micrel KSZ8895
On Tue 2017-08-29 14:26:04, Andrew Lunn wrote: > > But the MDIO emaulation code is from their driver, after lots of > > deletions. > > Is this driver supposed to run on lots of different OSs? That would > explain why they ignored the Linux MDIO and PHY layers. It did not look particulary portable. > If possible, please make use of the Linux infrastructure. I did not find any infrastructure I could use instead ksz_mdio_emulation. Now, drivers/net/phy/spi_ks8995.c can access the PHY registers, and I can not see any translation there, so there may be something I'm missing. Pointers would be welcome at this point. Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH] DSA support for Micrel KSZ8895
Hi Pavel, [auto build test ERROR on net-next/master] [also build test ERROR on v4.13-rc7 next-20170829] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Pavel-Machek/DSA-support-for-Micrel-KSZ8895/20170829-220156 config: xtensa-allmodconfig (attached as .config) compiler: xtensa-linux-gcc (GCC) 4.9.0 reproduce: wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=xtensa All error/warnings (new ones prefixed by >>): In file included from drivers/net/dsa/microchip/ksz_common.c:31:0: >> drivers/net/dsa/microchip/ksz_9477_reg.h:19:2: error: #error This is not >> switch we have #error This is not switch we have ^ -- drivers/net/dsa/microchip/ksz_8895.c: In function 'ksz_reset_switch': drivers/net/dsa/microchip/ksz_8895.c:109:21: warning: unused variable 'dev' [-Wunused-variable] struct ksz_device *dev = ds->priv; ^ drivers/net/dsa/microchip/ksz_8895.c: In function 'ksz_br_join': drivers/net/dsa/microchip/ksz_8895.c:262:21: warning: unused variable 'dev' [-Wunused-variable] struct ksz_device *dev = ds->priv; ^ drivers/net/dsa/microchip/ksz_8895.c: In function 'ksz_br_leave': drivers/net/dsa/microchip/ksz_8895.c:270:21: warning: unused variable 'dev' [-Wunused-variable] struct ksz_device *dev = ds->priv; ^ drivers/net/dsa/microchip/ksz_8895.c: At top level: >> drivers/net/dsa/microchip/ksz_8895.c:469:12: warning: 'struct >> switchdev_obj_port_fdb' declared inside parameter list struct switchdev_trans *trans) ^ >> drivers/net/dsa/microchip/ksz_8895.c:469:12: warning: its scope is only this >> definition or declaration, which is probably not what you want drivers/net/dsa/microchip/ksz_8895.c:497:16: warning: 'struct switchdev_obj_port_fdb' declared inside parameter list struct switchdev_trans *trans) ^ drivers/net/dsa/microchip/ksz_8895.c:503:21: warning: 'struct switchdev_obj_port_fdb' declared inside parameter list const struct switchdev_obj_port_fdb *fdb) ^ drivers/net/dsa/microchip/ksz_8895.c:510:9: warning: 'struct switchdev_obj_port_fdb' declared inside parameter list switchdev_obj_dump_cb_t *cb) ^ >> drivers/net/dsa/microchip/ksz_8895.c:576:2: error: unknown field >> 'port_vlan_dump' specified in initializer .port_vlan_dump = ksz_port_vlan_dump, ^ >> drivers/net/dsa/microchip/ksz_8895.c:576:2: warning: initialization from >> incompatible pointer type drivers/net/dsa/microchip/ksz_8895.c:576:2: warning: (near initialization for 'ksz_switch_ops.port_fdb_add') >> drivers/net/dsa/microchip/ksz_8895.c:577:2: error: unknown field >> 'port_fdb_prepare' specified in initializer .port_fdb_prepare = ksz_port_fdb_prepare, ^ drivers/net/dsa/microchip/ksz_8895.c:577:2: warning: initialization from incompatible pointer type drivers/net/dsa/microchip/ksz_8895.c:577:2: warning: (near initialization for 'ksz_switch_ops.port_fdb_del') drivers/net/dsa/microchip/ksz_8895.c:578:2: warning: initialization from incompatible pointer type .port_fdb_dump = ksz_port_fdb_dump, ^ drivers/net/dsa/microchip/ksz_8895.c:578:2: warning: (near initialization for 'ksz_switch_ops.port_fdb_dump') drivers/net/dsa/microchip/ksz_8895.c:579:2: warning: initialization from incompatible pointer type .port_fdb_add = ksz_port_fdb_add, ^ drivers/net/dsa/microchip/ksz_8895.c:579:2: warning: (near initialization for 'ksz_switch_ops.port_fdb_add') drivers/net/dsa/microchip/ksz_8895.c:580:2: warning: initialization from incompatible pointer type .port_fdb_del = ksz_port_fdb_del, ^ drivers/net/dsa/microchip/ksz_8895.c:580:2: warning: (near initialization for 'ksz_switch_ops.port_fdb_del') >> drivers/net/dsa/microchip/ksz_8895.c:584:2: error: unknown field >> 'port_mdb_dump' specified in initializer .port_mdb_dump = ksz_port_mdb_dump, ^ drivers/net/dsa/microchip/ksz_8895.c:584:2: warning: initialization from incompatible pointer type drivers/net/dsa/microchip/ksz_8895.c:584:2: warning: (near initialization for 'ksz_switch_ops.get_rxnfc') vim +19 drivers/net/dsa/microchip/ksz_9477_reg.h > 19 #error This is not switch we have 20 #ifndef __KSZ9477_REGS_H 21 #define __KSZ9477_REGS_H 22 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: [PATCH] DSA support for Micrel KSZ8895
> But the MDIO emaulation code is from their driver, after lots of > deletions. Is this driver supposed to run on lots of different OSs? That would explain why they ignored the Linux MDIO and PHY layers. If possible, please make use of the Linux infrastructure. Andrew
Re: [PATCH] DSA support for Micrel KSZ8895
On Mon 2017-08-28 16:09:27, Andrew Lunn wrote: > > I may be confused here, but AFAICT: > > > > 1) Yes, it has standard layout when accessed over MDIO. > > > Section 4.8 of the datasheet says: > > All the registers defined in this section can be also accessed > via the SPI interface. > > Meaning all PHY registers can be access via the SPI interface. So you > should be able to make a standard Linux MDIO bus driver which performs > SPI reads. As far as I can tell (and their driver confirms) -- yes, all those registers can be accessed over the SPI, they are just shuffled around... hence MDIO emulation code. I copied it from their code (see the copyrights) so no, I don't believe there's nicer solution. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH] DSA support for Micrel KSZ8895
Hi! > Micrel has some drivers on their web site to support some chips. For > that chips they do virtual mdio over spi. > And driver is available on download page: > http://www.microchip.com/wwwproducts/en/KSZ8895 > > Documentation->Software library. > > Both driver and DSA driver. Driver has to work with some minor fixups > related to your kernel version. But I think they are don't care about > up-streaming that code. > So you can take their code as a reference. "Minor fixups". Take a look at the driver.. I wanted to do a "minor fixups". It turned out it was easier to start from scratch. But the MDIO emaulation code is from their driver, after lots of deletions. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH] DSA support for Micrel KSZ8895
Micrel has some drivers on their web site to support some chips. For that chips they do virtual mdio over spi. And driver is available on download page: http://www.microchip.com/wwwproducts/en/KSZ8895 Documentation->Software library. Both driver and DSA driver. Driver has to work with some minor fixups related to your kernel version. But I think they are don't care about up-streaming that code. So you can take their code as a reference. 2017-08-28 17:09 GMT+03:00 Andrew Lunn: >> I may be confused here, but AFAICT: >> >> 1) Yes, it has standard layout when accessed over MDIO. > > > Section 4.8 of the datasheet says: > > All the registers defined in this section can be also accessed > via the SPI interface. > > Meaning all PHY registers can be access via the SPI interface. So you > should be able to make a standard Linux MDIO bus driver which performs > SPI reads. > > Andrew Micrel has some drivers on their web site to support some chips. For that chips they do virtual mdio over spi. And driver is available on download page: http://www.microchip.com/wwwproducts/en/KSZ8895 Documentation->Software library. Both driver and DSA driver. Driver has to work with some minor fixups related to your kernel version. But I think they are don't care about up-streaming that code. So you can take their code as a reference. -- Best regards, Maxim Uvarov
Re: [PATCH] DSA support for Micrel KSZ8895
> I may be confused here, but AFAICT: > > 1) Yes, it has standard layout when accessed over MDIO. Section 4.8 of the datasheet says: All the registers defined in this section can be also accessed via the SPI interface. Meaning all PHY registers can be access via the SPI interface. So you should be able to make a standard Linux MDIO bus driver which performs SPI reads. Andrew
Re: [PATCH] DSA support for Micrel KSZ8895
Hi! Thanks for review. > > + case PHY_REG_STATUS: > > + ksz_pread8(sw, p, P_LINK_STATUS, ); > > + ksz_pread8(sw, p, P_SPEED_STATUS, ); > > + data = PHY_100BTX_FD_CAPABLE | > > + PHY_100BTX_CAPABLE | > > + PHY_10BT_FD_CAPABLE | > > + PHY_10BT_CAPABLE | > > + PHY_AUTO_NEG_CAPABLE; > > + if (link & PORT_AUTO_NEG_COMPLETE) > > + data |= PHY_AUTO_NEG_ACKNOWLEDGE; > > + if (link & PORT_STAT_LINK_GOOD) > > + data |= PHY_LINK_STATUS; > > + break; > > + case PHY_REG_ID_1: > > + data = KSZ8895_ID_HI; > > + break; > > + case PHY_REG_ID_2: > > + data = KSZ8895_ID_LO; > > + break; > > According to the datasheet, the PHY has the normal ID registers, > which have the value 0x0022, 0x1450. So it should be possible to have > a standard PHY driver in drivers/net/phy. > > In fact, the IDs suggest it is a micrel phy, and 1430, 1435 are > already supported. So it could be you only need minor modifications to > the micrel.c. I may be confused here, but AFAICT: 1) Yes, it has standard layout when accessed over MDIO. But then there's no access to the bridging functionality, and MDIO access may not be available. [I was told not to use it for this design, so I did not]. 2) drivers/net/phy/spi_ks8995.c can be trivially modified to work with this chip.. but then you don't get the bridge functionality. (And I'm not sure how it works / who translates layouts in this case.) I'd like to get rid of this code, or use some existing code instead, but I don't think it is possible while keeping the SPI accesss. Let me know if I'm wrong. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH] DSA support for Micrel KSZ8895
Hi! > >No, tag_ksz part probably is not acceptable. Do you see solution > >better than just copying it into tag_ksz1 file? > > You could have all Micrel tag implementations live under net/dsa/tag_ksz.c > and have e.g: DSA_TAG_PROTO_KSZ for the current (newer) switches and > DSA_TAG_PROTO_KSZ_LEGACY (or any other name) for the older switches and you > would provide two sets of function pointers depending on which protocol is > requested by the switch. > > Considering the minor difference needed in tagging here, it might be > acceptable to actually keep the current functions and just have the xmit() > call check what get_tag_protocol returns and use word 1 or 0 based on that. > Even though that's a fast path it shouldn't hurt performance too much. If it > does, we can always copy the tagging protocol into dsa_slave_priv so you have > a fast access to it. > Actually I believe I can do optimizer tricks to keep this zero-cost with clean code, if needed. > > > >Any more comments, etc? > > The MII emulation bits are interesting, was it not sufficient if you > implemented phy_read and phy_write operations that perform the necessary > internal PHY accesses or maybe you don't get access to standard MII > registers? b53 does such a thing and we merely just need to do a simple shift > to access the MII register number, thus avoiding the translation. > We don't get standard MII registers over SPI bus. > >Help would be welcome. > > I concur with Andrew, try to get a patch series, even an RFC one together so > we can review things individually. > > How functional is your driver so far? I'd say the basic stuff to get working: > counters (debugging), link management (auto-negotiation, forced, etc.) and > basic bridging: all ports separate by default and working port to port > switching when brought together in a bridge. VLAN, FDB, MDB, other ethtool > goodies can be added later on. > Which counters are essential? Link management and basic bridging should work, not sure if I'll have time to do more than that. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH] DSA support for Micrel KSZ8895
Hi! > > No, tag_ksz part probably is not acceptable. Do you see solution > > better than just copying it into tag_ksz1 file? > > How about something like this, which needs further work to actually > compile, but should give you the idea. If that's acceptable, yes, I can do something similar. I don't think CONFIG_NET_DSA_TAG_KSZ_8K / CONFIG_NET_DSA_TAG_KSZ_9K is suitable naming (these will probably differ according to number of ports), what about keeping CONFIG_NET_DSA_TAG_KSZ and adding CONFIG_NET_DSA_TAG_KSZ_1B (for one byte)? Thanks, Pavel >Andrew > > index 99e38af85fc5..843e77b7c270 100644 > --- a/net/dsa/dsa.c > +++ b/net/dsa/dsa.c > @@ -49,8 +49,11 @@ const struct dsa_device_ops *dsa_device_ops[DSA_TAG_LAST] > = { > #ifdef CONFIG_NET_DSA_TAG_EDSA > [DSA_TAG_PROTO_EDSA] = _netdev_ops, > #endif > -#ifdef CONFIG_NET_DSA_TAG_KSZ > - [DSA_TAG_PROTO_KSZ] = _netdev_ops, > +#ifdef CONFIG_NET_DSA_TAG_KSZ_8K > + [DSA_TAG_PROTO_KSZ8K] = _netdev_ops, > +#endif > +#ifdef CONFIG_NET_DSA_TAG_KSZ_9K > + [DSA_TAG_PROTO_KSZ9K] = _netdev_ops, > #endif > #ifdef CONFIG_NET_DSA_TAG_LAN9303 > [DSA_TAG_PROTO_LAN9303] = _netdev_ops, > diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c > index de66ca8e6201..398b833889f1 100644 > --- a/net/dsa/tag_ksz.c > +++ b/net/dsa/tag_ksz.c > @@ -35,6 +35,9 @@ > static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev) > { > struct dsa_slave_priv *p = netdev_priv(dev); > + struct dsa_port *dp = p->dp; > + struct dsa_switch *ds = dp->ds; > + struct dsa_switch_tree *dst = ds->dst; > struct sk_buff *nskb; > int padlen; > u8 *tag; > @@ -69,8 +72,14 @@ static struct sk_buff *ksz_xmit(struct sk_buff *skb, > struct net_device *dev) > } > > tag = skb_put(nskb, KSZ_INGRESS_TAG_LEN); > - tag[0] = 0; > - tag[1] = 1 << p->dp->index; /* destination port */ > + if (dst->tag_ops == ksz8k_netdev_ops) { > + tag[0] = 1 << p->dp->index; /* destination port */0; > + tag[1] = 0; > + } > + > + if (dst->tag_ops == ksz9k_netdev_ops) { > + tag[0] = 0; > + tag[1] = 1 << p->dp->index; /* destination port */ > > return nskb; > } > @@ -98,7 +107,12 @@ static struct sk_buff *ksz_rcv(struct sk_buff *skb, > struct net_device *dev, > return skb; > } > > -const struct dsa_device_ops ksz_netdev_ops = { > +const struct dsa_device_ops ksz8k_netdev_ops = { > + .xmit = ksz_xmit, > + .rcv= ksz_rcv, > +}; > + > +const struct dsa_device_ops ksz9k_netdev_ops = { > .xmit = ksz_xmit, > .rcv= ksz_rcv, > }; -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
RE: [PATCH] DSA support for Micrel KSZ8895
Pavel, Thanks for update and sorry about email format (due to web-access version) I'll do review when getting back to office later this week. - Woojung From: Pavel Machek [pa...@denx.de] Sent: Sunday, August 27, 2017 8:36 AM To: Woojung Huh - C21699; nathan.leigh.con...@gmail.com Cc: vivien.dide...@savoirfairelinux.com; f.faine...@gmail.com; netdev@vger.kernel.org; linux-ker...@vger.kernel.org; tristram...@micrel.com; and...@lunn.ch; pa...@denx.de Subject: [PATCH] DSA support for Micrel KSZ8895 Hi! So I fought with the driver a bit more, and now I have something that kind-of-works. "great great hack" belows worries me. Yeah, disabled code needs to be removed before merge. No, tag_ksz part probably is not acceptable. Do you see solution better than just copying it into tag_ksz1 file? Any more comments, etc? Help would be welcome.
Re: [PATCH] DSA support for Micrel KSZ8895
On August 27, 2017 5:36:58 AM PDT, Pavel Machekwrote: >Hi! > >So I fought with the driver a bit more, and now I have something that >kind-of-works. > >"great great hack" belows worries me. > >Yeah, disabled code needs to be removed before merge. > >No, tag_ksz part probably is not acceptable. Do you see solution >better than just copying it into tag_ksz1 file? You could have all Micrel tag implementations live under net/dsa/tag_ksz.c and have e.g: DSA_TAG_PROTO_KSZ for the current (newer) switches and DSA_TAG_PROTO_KSZ_LEGACY (or any other name) for the older switches and you would provide two sets of function pointers depending on which protocol is requested by the switch. Considering the minor difference needed in tagging here, it might be acceptable to actually keep the current functions and just have the xmit() call check what get_tag_protocol returns and use word 1 or 0 based on that. Even though that's a fast path it shouldn't hurt performance too much. If it does, we can always copy the tagging protocol into dsa_slave_priv so you have a fast access to it. > >Any more comments, etc? The MII emulation bits are interesting, was it not sufficient if you implemented phy_read and phy_write operations that perform the necessary internal PHY accesses or maybe you don't get access to standard MII registers? b53 does such a thing and we merely just need to do a simple shift to access the MII register number, thus avoiding the translation. > >Help would be welcome. I concur with Andrew, try to get a patch series, even an RFC one together so we can review things individually. How functional is your driver so far? I'd say the basic stuff to get working: counters (debugging), link management (auto-negotiation, forced, etc.) and basic bridging: all ports separate by default and working port to port switching when brought together in a bridge. VLAN, FDB, MDB, other ethtool goodies can be added later on. -- Florian
Re: [PATCH] DSA support for Micrel KSZ8895
> No, tag_ksz part probably is not acceptable. Do you see solution > better than just copying it into tag_ksz1 file? How about something like this, which needs further work to actually compile, but should give you the idea. Andrew index 99e38af85fc5..843e77b7c270 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -49,8 +49,11 @@ const struct dsa_device_ops *dsa_device_ops[DSA_TAG_LAST] = { #ifdef CONFIG_NET_DSA_TAG_EDSA [DSA_TAG_PROTO_EDSA] = _netdev_ops, #endif -#ifdef CONFIG_NET_DSA_TAG_KSZ - [DSA_TAG_PROTO_KSZ] = _netdev_ops, +#ifdef CONFIG_NET_DSA_TAG_KSZ_8K + [DSA_TAG_PROTO_KSZ8K] = _netdev_ops, +#endif +#ifdef CONFIG_NET_DSA_TAG_KSZ_9K + [DSA_TAG_PROTO_KSZ9K] = _netdev_ops, #endif #ifdef CONFIG_NET_DSA_TAG_LAN9303 [DSA_TAG_PROTO_LAN9303] = _netdev_ops, diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c index de66ca8e6201..398b833889f1 100644 --- a/net/dsa/tag_ksz.c +++ b/net/dsa/tag_ksz.c @@ -35,6 +35,9 @@ static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev) { struct dsa_slave_priv *p = netdev_priv(dev); + struct dsa_port *dp = p->dp; + struct dsa_switch *ds = dp->ds; + struct dsa_switch_tree *dst = ds->dst; struct sk_buff *nskb; int padlen; u8 *tag; @@ -69,8 +72,14 @@ static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev) } tag = skb_put(nskb, KSZ_INGRESS_TAG_LEN); - tag[0] = 0; - tag[1] = 1 << p->dp->index; /* destination port */ + if (dst->tag_ops == ksz8k_netdev_ops) { + tag[0] = 1 << p->dp->index; /* destination port */0; + tag[1] = 0; + } + + if (dst->tag_ops == ksz9k_netdev_ops) { + tag[0] = 0; + tag[1] = 1 << p->dp->index; /* destination port */ return nskb; } @@ -98,7 +107,12 @@ static struct sk_buff *ksz_rcv(struct sk_buff *skb, struct net_device *dev, return skb; } -const struct dsa_device_ops ksz_netdev_ops = { +const struct dsa_device_ops ksz8k_netdev_ops = { + .xmit = ksz_xmit, + .rcv= ksz_rcv, +}; + +const struct dsa_device_ops ksz9k_netdev_ops = { .xmit = ksz_xmit, .rcv= ksz_rcv, };
Re: [PATCH] DSA support for Micrel KSZ8895
> +/** > + * sw_r_phy - read data from PHY register > + * @sw: The switch instance. > + * @phy: PHY address to read. > + * @reg: PHY register to read. > + * @val: Buffer to store the read data. > + * > + * This routine reads data from the PHY register. > + */ > +static void sw_r_phy(struct ksz_device *sw, u16 phy, u16 reg, u16 *val) > +{ > + u8 ctrl; > + u8 restart; > + u8 link; > + u8 speed; > + u8 force; > + u8 p = phy; > + u16 data = 0; > + > + switch (reg) { > + case PHY_REG_CTRL: > + ksz_pread8(sw, p, P_LOCAL_CTRL, ); > + ksz_pread8(sw, p, P_NEG_RESTART_CTRL, ); > + ksz_pread8(sw, p, P_SPEED_STATUS, ); > + ksz_pread8(sw, p, P_FORCE_CTRL, ); > + if (restart & PORT_PHY_LOOPBACK) > + data |= PHY_LOOPBACK; > + if (force & PORT_FORCE_100_MBIT) > + data |= PHY_SPEED_100MBIT; > + if (!(force & PORT_AUTO_NEG_DISABLE)) > + data |= PHY_AUTO_NEG_ENABLE; > + if (restart & PORT_POWER_DOWN) > + data |= PHY_POWER_DOWN; > + if (restart & PORT_AUTO_NEG_RESTART) > + data |= PHY_AUTO_NEG_RESTART; > + if (force & PORT_FORCE_FULL_DUPLEX) > + data |= PHY_FULL_DUPLEX; > + if (speed & PORT_HP_MDIX) > + data |= PHY_HP_MDIX; > + if (restart & PORT_FORCE_MDIX) > + data |= PHY_FORCE_MDIX; > + if (restart & PORT_AUTO_MDIX_DISABLE) > + data |= PHY_AUTO_MDIX_DISABLE; > + if (restart & PORT_TX_DISABLE) > + data |= PHY_TRANSMIT_DISABLE; > + if (restart & PORT_LED_OFF) > + data |= PHY_LED_DISABLE; > + break; > + case PHY_REG_STATUS: > + ksz_pread8(sw, p, P_LINK_STATUS, ); > + ksz_pread8(sw, p, P_SPEED_STATUS, ); > + data = PHY_100BTX_FD_CAPABLE | > + PHY_100BTX_CAPABLE | > + PHY_10BT_FD_CAPABLE | > + PHY_10BT_CAPABLE | > + PHY_AUTO_NEG_CAPABLE; > + if (link & PORT_AUTO_NEG_COMPLETE) > + data |= PHY_AUTO_NEG_ACKNOWLEDGE; > + if (link & PORT_STAT_LINK_GOOD) > + data |= PHY_LINK_STATUS; > + break; > + case PHY_REG_ID_1: > + data = KSZ8895_ID_HI; > + break; > + case PHY_REG_ID_2: > + data = KSZ8895_ID_LO; > + break; According to the datasheet, the PHY has the normal ID registers, which have the value 0x0022, 0x1450. So it should be possible to have a standard PHY driver in drivers/net/phy. In fact, the IDs suggest it is a micrel phy, and 1430, 1435 are already supported. So it could be you only need minor modifications to the micrel.c. Andrew
Re: [PATCH] DSA support for Micrel KSZ8895
On Sun, Aug 27, 2017 at 02:36:58PM +0200, Pavel Machek wrote: > Hi! > > So I fought with the driver a bit more, and now I have something that > kind-of-works. Thanks for keeping on working on this. > "great great hack" belows worries me. > > Yeah, disabled code needs to be removed before merge. > > No, tag_ksz part probably is not acceptable. Do you see solution > better than just copying it into tag_ksz1 file? > > Any more comments, etc? It would help with review if you split this up into multiple patches. The change to the tagger should be one patch. The mdio emulation would make a reasonable standalone patch etc. I will do a more detailed review later. Andrew
[PATCH] DSA support for Micrel KSZ8895
Hi! So I fought with the driver a bit more, and now I have something that kind-of-works. "great great hack" belows worries me. Yeah, disabled code needs to be removed before merge. No, tag_ksz part probably is not acceptable. Do you see solution better than just copying it into tag_ksz1 file? Any more comments, etc? Help would be welcome. Best regards, Pavel Signed-off-by: Pavel Machekdiff --git a/drivers/net/dsa/microchip/Kconfig b/drivers/net/dsa/microchip/Kconfig index a8b8f59099ce..7b7d7ddb3488 100644 --- a/drivers/net/dsa/microchip/Kconfig +++ b/drivers/net/dsa/microchip/Kconfig @@ -1,12 +1,25 @@ menuconfig MICROCHIP_KSZ - tristate "Microchip KSZ series switch support" + tristate "Microchip KSZ 9477 series switch support" + depends on NET_DSA + select NET_DSA_TAG_KSZ + help + This driver adds support for Microchip KSZ switch chips. + +menuconfig MICROCHIP_KSZ_8895 + tristate "Microchip KSZ 8895 series switch support" depends on NET_DSA select NET_DSA_TAG_KSZ help This driver adds support for Microchip KSZ switch chips. config MICROCHIP_KSZ_SPI_DRIVER - tristate "KSZ series SPI connected switch driver" + tristate "KSZ 9477 series SPI connected switch driver" depends on MICROCHIP_KSZ && SPI help Select to enable support for registering switches configured through SPI. + +config MICROCHIP_KSZ_8895_SPI_DRIVER + tristate "KSZ 8895 series SPI connected switch driver" + depends on MICROCHIP_KSZ_8895 && SPI + help + Select to enable support for registering switches configured through SPI. diff --git a/drivers/net/dsa/microchip/Makefile b/drivers/net/dsa/microchip/Makefile index ed335e29fae8..b6a17f79d2d9 100644 --- a/drivers/net/dsa/microchip/Makefile +++ b/drivers/net/dsa/microchip/Makefile @@ -1,2 +1,4 @@ obj-$(CONFIG_MICROCHIP_KSZ)+= ksz_common.o +obj-$(CONFIG_MICROCHIP_KSZ_8895)+= ksz_8895.o obj-$(CONFIG_MICROCHIP_KSZ_SPI_DRIVER) += ksz_spi.o +obj-$(CONFIG_MICROCHIP_KSZ_8895_SPI_DRIVER)+= ksz_8895_spi.o diff --git a/drivers/net/dsa/microchip/ksz_8895.c b/drivers/net/dsa/microchip/ksz_8895.c new file mode 100644 index ..d546e08b1281 --- /dev/null +++ b/drivers/net/dsa/microchip/ksz_8895.c @@ -0,0 +1,721 @@ +/* + * Microchip switch driver main logic + * + * Copyright (C) 2017 + * Copyright (C) 2017 Pavel Machek + * + * Permission to use, copy, modify, and/or distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "ksz_8895_reg.h" +#include "ksz_priv.h" + +static const struct { + int index; + char string[ETH_GSTRING_LEN]; +} mib_names[TOTAL_SWITCH_COUNTER_NUM] = { + { 0x00, "???" }, +}; + +static void ksz_cfg(struct ksz_device *dev, u32 addr, u8 bits, bool set) +{ + u8 data; + + ksz_read8(dev, addr, ); + if (set) + data |= bits; + else + data &= ~bits; + ksz_write8(dev, addr, data); +} + +#if 0 +static void ksz_cfg32(struct ksz_device *dev, u32 addr, u32 bits, bool set) +{ + u32 data; + + ksz_read32(dev, addr, ); + if (set) + data |= bits; + else + data &= ~bits; + ksz_write32(dev, addr, data); +} +#endif + +static void ksz_port_cfg(struct ksz_device *dev, int port, int offset, u8 bits, +bool set) +{ + u32 addr; + u8 data; + + addr = PORT_CTRL_ADDR(port, offset); + ksz_read8(dev, addr, ); + + if (set) + data |= bits; + else + data &= ~bits; + + ksz_write8(dev, addr, data); +} + +#if 0 +static void ksz_port_cfg32(struct ksz_device *dev, int port, int offset, + u32 bits, bool set) +{ + u32 addr; + u32 data; + + addr = PORT_CTRL_ADDR(port, offset); + ksz_read32(dev, addr, ); + + if (set) + data |= bits; + else + data &= ~bits; + + ksz_write32(dev, addr, data); +} +#endif + +#define NOTIMPL() do { NOTIMPLV(); return -EJUKEBOX; }