Re: [PATCH] DSA support for Micrel KSZ8895

[Andrew Lunn]

> 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

[Tristram.Ha]

> -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

[Maxim Uvarov]

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

[Pavel Machek]

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

[Florian Fainelli]

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

[Pavel Machek]

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

[Andrew Lunn]

> 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

[Tristram.Ha]

> 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

[Maxim Uvarov]

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

[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?

> 
> Pointers would be welcome at this point.
> 
> Thanks,
>   Pavel
> 


-- 
Florian

Re: [PATCH] DSA support for Micrel KSZ8895

[Pavel Machek]

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

[kbuild test robot]

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

[Andrew Lunn]

> 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

[Pavel Machek]

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

[Pavel Machek]

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

[Maxim Uvarov]

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

[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

Re: [PATCH] DSA support for Micrel KSZ8895

[Pavel Machek]

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

[Pavel Machek]

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

[Pavel Machek]

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

[Woojung.Huh]

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

[Florian Fainelli]

On August 27, 2017 5:36:58 AM PDT, Pavel Machek  wrote:
>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

[Andrew Lunn]

> 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

[Andrew Lunn]

> +/**
> + * 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

[Andrew Lunn]

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