Re: [PATCH v2] watchdog: aspeed: fix integer overflow in set_timeout handler

[Tao Ren]

On Thu, Apr 15, 2021 at 10:07:32PM -0700, Guenter Roeck wrote:
> On 4/15/21 7:13 PM, rentao.b...@gmail.com wrote:
> > From: Tao Ren 
> > 
> > Fix the time comparison (timeout vs. max_hw_heartbeat_ms) in set_timeout
> > handler to avoid potential integer overflow when the supplied timeout is
> > greater than aspeed's maximum allowed timeout (4294 seconds).
> > 
> 
> I think this is the wrong focus: What this fixes is the wrong hardware
> timeout calculation. Again, I think that the wrong calculation leads to
> the overflow should not be the focus of this patch, though it can of
> course be mentioned.
> 
> I'll leave it up to Wim to decide if he wants to apply the patch with the
> current explanation.
> 
> Thanks,
> Guenter

Sorry I didn't get your point correctly, and I guess it was because of
my lack of knowledge in timeout/max_hw_heartbeat_ms/worker (hopefully
my understanding is correct now :))

Let me drop this patch and send a new one with different subject and
description soon.


Cheers,

Tao

Re: [PATCH] watchdog: aspeed: fix integer overflow in set_timeout handler

[Tao Ren]

On Thu, Apr 15, 2021 at 05:50:32PM -0700, Guenter Roeck wrote:
> On 4/15/21 5:12 PM, rentao.b...@gmail.com wrote:
> > From: Tao Ren 
> > 
> > Fix the time comparison (timeout vs. max_hw_heartbeat_ms) in set_timeout
> > handler to avoid potential integer overflow when the supplied timeout is
> > greater than aspeed's maximum allowed timeout (4294 seconds).
> > 
> > Fixes: efa859f7d786 ("watchdog: Add Aspeed watchdog driver")
> > Reported-by: Amithash Prasad 
> > Signed-off-by: Tao Ren 
> > ---
> >  drivers/watchdog/aspeed_wdt.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> > index 7e00960651fa..9f77272dc906 100644
> > --- a/drivers/watchdog/aspeed_wdt.c
> > +++ b/drivers/watchdog/aspeed_wdt.c
> > @@ -145,9 +145,8 @@ static int aspeed_wdt_set_timeout(struct 
> > watchdog_device *wdd,
> > struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
> > u32 actual;
> >  
> > -   wdd->timeout = timeout;
> > -
> > -   actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
> > +   actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);
> > +   wdd->timeout = actual;
> >  
> > writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
> > writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> > 
> 
> If the provided timeout is larger than the supported hardware timeout,
> the watchdog core will ping the hardware on behalf of userspace.
> The above code would defeat that mechanism for no good reason.
> 
> NACK.
> 
> Guenter

Thanks Guenter for Joel for the quick review!

The integer overflow happens at (actual * WDT_RATE_1MHZ). For example,
if a user tries to set timeout to 4295 seconds, then the hardware would
be programmed to timeout after about 32 milliseconds. I would say this
behavior is not expected?


Cheers,

Tao

Re: [PATCH] usb: gadget: aspeed: set port_dev dma mask

[Tao Ren]

On Mon, Mar 29, 2021 at 08:17:35AM +0200, Christoph Hellwig wrote:
> On Sat, Mar 27, 2021 at 03:17:59PM -0700, Tao Ren wrote:
> > On Fri, Mar 26, 2021 at 01:05:26PM +0100, Christoph Hellwig wrote:
> > > On Fri, Mar 26, 2021 at 12:03:03PM +, Robin Murphy wrote:
> > > > This might happen to work out, but is far from correct. Just wait until 
> > > > you 
> > > > try it on a platform where the USB controller is behind an IOMMU...
> > > >
> > > > It looks like something is more fundamentally wrong here - the device 
> > > > passed to DMA API calls must be the actual hardware device performing 
> > > > the 
> > > > DMA, which in USB-land I believe means the controller's sysdev.
> > > 
> > > The shiny new usb_intf_get_dma_device API provides the device to use.
> > 
> > Thanks Robin and Christoph for the feedback.
> > 
> > If I understand correctly, usb_intf_get_dma_device API is mainly for usb
> > host drivers? I just found usb_gadget_map_request_by_dev API: does it
> > make sense to replace usb_gadget_map_request with
> > usb_gadget_map_request_by_dev so we can pass the actual DMA-capable
> > hardware device (aspeed-vhub platform device) to the API?
> 
> Oh, right you're dealing with a gadget side driver.  Not sure about
> the API there, I'll let the relevant maintainers chime in.

Given this is not the right path, I will drop the patch and work out a
new fix soon (by calling usb_gadget_map_request_by_dev, and with
modified subject).


Cheers,

Tao

Re: [PATCH] usb: gadget: aspeed: set port_dev dma mask

[Tao Ren]

On Fri, Mar 26, 2021 at 01:05:26PM +0100, Christoph Hellwig wrote:
> On Fri, Mar 26, 2021 at 12:03:03PM +, Robin Murphy wrote:
> > This might happen to work out, but is far from correct. Just wait until you 
> > try it on a platform where the USB controller is behind an IOMMU...
> >
> > It looks like something is more fundamentally wrong here - the device 
> > passed to DMA API calls must be the actual hardware device performing the 
> > DMA, which in USB-land I believe means the controller's sysdev.
> 
> The shiny new usb_intf_get_dma_device API provides the device to use.

Thanks Robin and Christoph for the feedback.

If I understand correctly, usb_intf_get_dma_device API is mainly for usb
host drivers? I just found usb_gadget_map_request_by_dev API: does it
make sense to replace usb_gadget_map_request with
usb_gadget_map_request_by_dev so we can pass the actual DMA-capable
hardware device (aspeed-vhub platform device) to the API?


Cheers,

Tao

Re: [PATCH v4 1/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring driver

[Tao Ren]

On Mon, Nov 23, 2020 at 05:18:32AM -0800, Guenter Roeck wrote:
> On Sun, Nov 22, 2020 at 11:45:31PM -0800, rentao.b...@gmail.com wrote:
> > From: Tao Ren 
> > 
> > Add hardware monitoring driver for the Maxim MAX127 chip.
> > 
> > MAX127 min/max range handling code is inspired by the max197 driver.
> > 
> > Signed-off-by: Tao Ren 
> > ---
> >  Changes in v4:
> >- delete unnecessary "#include" lines.
> >- simplify i2c_transfer() error handling.
> >- add mutex to protect ctrl_byte in write_min|max() functions.
> >  Changes in v3:
> >- no code change. xdp maintainers were removed from to/cc list.
> >  Changes in v2:
> >- replace devm_hwmon_device_register_with_groups() with
> >  devm_hwmon_device_register_with_info() API.
> >- divide min/max read and write methods to separate functions.
> >- fix raw-to-vin conversion logic.
> >- refine ctrl_byte handling so mutex is not needed to protect the
> >  byte.
> >- improve i2c_transfer() error handling.
> >- a few other improvements (comments, variable naming, and etc.).
> > 
> >  drivers/hwmon/Kconfig  |   9 ++
> >  drivers/hwmon/Makefile |   1 +
> >  drivers/hwmon/max127.c | 346 +
> >  3 files changed, 356 insertions(+)
> >  create mode 100644 drivers/hwmon/max127.c
> > 
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 9d600e0c5584..716df51edc87 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -950,6 +950,15 @@ config SENSORS_MAX
> >   This driver can also be built as a module. If so, the module
> >   will be called max.
> >  
> > +config SENSORS_MAX127
> > +   tristate "Maxim MAX127 12-bit 8-channel Data Acquisition System"
> > +   depends on I2C
> > +   help
> > + Say y here to support Maxim's MAX127 DAS chips.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called max127.
> > +
> >  config SENSORS_MAX16065
> > tristate "Maxim MAX16065 System Manager and compatibles"
> > depends on I2C
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 1083bbfac779..01ca5d3fbad4 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -127,6 +127,7 @@ obj-$(CONFIG_SENSORS_LTC4260)   += ltc4260.o
> >  obj-$(CONFIG_SENSORS_LTC4261)  += ltc4261.o
> >  obj-$(CONFIG_SENSORS_LTQ_CPUTEMP) += ltq-cputemp.o
> >  obj-$(CONFIG_SENSORS_MAX)  += max.o
> > +obj-$(CONFIG_SENSORS_MAX127)   += max127.o
> >  obj-$(CONFIG_SENSORS_MAX16065) += max16065.o
> >  obj-$(CONFIG_SENSORS_MAX1619)  += max1619.o
> >  obj-$(CONFIG_SENSORS_MAX1668)  += max1668.o
> > diff --git a/drivers/hwmon/max127.c b/drivers/hwmon/max127.c
> > new file mode 100644
> > index ..1c54146b6086
> > --- /dev/null
> > +++ b/drivers/hwmon/max127.c
> > @@ -0,0 +1,346 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Hardware monitoring driver for MAX127.
> > + *
> > + * Copyright (c) 2020 Facebook Inc.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +/*
> > + * MAX127 Control Byte. Refer to MAX127 datasheet, Table 1 "Control-Byte
> > + * Format" for details.
> > + */
> > +#define MAX127_CTRL_START  BIT(7)
> > +#define MAX127_CTRL_SEL_SHIFT  4
> > +#define MAX127_CTRL_RNGBIT(3)
> > +#define MAX127_CTRL_BIPBIT(2)
> > +#define MAX127_CTRL_PD1BIT(1)
> > +#define MAX127_CTRL_PD0BIT(0)
> > +
> > +#define MAX127_NUM_CHANNELS8
> > +#define MAX127_SET_CHANNEL(ch) (((ch) & 7) << MAX127_CTRL_SEL_SHIFT)
> > +
> > +/*
> > + * MAX127 channel input ranges. Refer to MAX127 datasheet, Table 3 "Range
> > + * and Polarity Selection" for details.
> > + */
> > +#define MAX127_FULL_RANGE  1   /* 10V */
> > +#define MAX127_HALF_RANGE  5000/* 5V */
> > +
> > +/*
> > + * MAX127 returns 2 bytes at read:
> > + *   - the first byte contains data[11:4].
> > + *   - the second byte contains data[3:0] (MSB) and 4 dummy 0s (LSB).
> > + * Refer to MAX127 datasheet, "Read a Conversion (Read Cycle)" section
> > + * for details.
> > + */
> > +#define MAX127_DATA_LEN2
> > +#define MAX12

Re: [PATCH v3 1/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring driver

[Tao Ren]

On Mon, Nov 23, 2020 at 05:16:41AM -0800, Guenter Roeck wrote:
> On Sun, Nov 22, 2020 at 11:54:49PM -0800, Tao Ren wrote:
> > On Sun, Nov 22, 2020 at 10:33:42AM -0800, Guenter Roeck wrote:
> > > On Thu, Nov 19, 2020 at 09:53:23AM -0800, rentao.b...@gmail.com wrote:
> > > > From: Tao Ren 
> > > > 
> > > > Add hardware monitoring driver for the Maxim MAX127 chip.
> > > > 
> > > > MAX127 min/max range handling code is inspired by the max197 driver.
> > > > 
> > > > Signed-off-by: Tao Ren 
> > > 
> > > Nice cleanup. Couple of minor comments.
> > > 
> > > Thanks,
> > > Guenter
> > > 
> > > > ---
> > > >  Changes in v3:
> > > >- no code change. xdp maintainers were removed from to/cc list.
> > > >  Changes in v2:
> > > >- replace devm_hwmon_device_register_with_groups() with
> > > >  devm_hwmon_device_register_with_info() API.
> > > >- divide min/max read and write methods to separate functions.
> > > >- fix raw-to-vin conversion logic.
> > > >- refine ctrl_byte handling so mutex is not needed to protect the
> > > >  byte.
> > > >- improve i2c_transfer() error handling.
> > > >- a few other improvements (comments, variable naming, and etc.).
> > > > 
> > > >  drivers/hwmon/Kconfig  |   9 ++
> > > >  drivers/hwmon/Makefile |   1 +
> > > >  drivers/hwmon/max127.c | 346 +
> > > >  3 files changed, 356 insertions(+)
> > > >  create mode 100644 drivers/hwmon/max127.c
> > > > 
> > > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > > > index 9d600e0c5584..716df51edc87 100644
> > > > --- a/drivers/hwmon/Kconfig
> > > > +++ b/drivers/hwmon/Kconfig
> > > > @@ -950,6 +950,15 @@ config SENSORS_MAX
> > > >   This driver can also be built as a module. If so, the module
> > > >   will be called max.
> > > >  
> > > > +config SENSORS_MAX127
> > > > +   tristate "Maxim MAX127 12-bit 8-channel Data Acquisition System"
> > > > +   depends on I2C
> > > > +   help
> > > > + Say y here to support Maxim's MAX127 DAS chips.
> > > > +
> > > > + This driver can also be built as a module. If so, the module
> > > > + will be called max127.
> > > > +
> > > >  config SENSORS_MAX16065
> > > > tristate "Maxim MAX16065 System Manager and compatibles"
> > > > depends on I2C
> > > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > > > index 1083bbfac779..01ca5d3fbad4 100644
> > > > --- a/drivers/hwmon/Makefile
> > > > +++ b/drivers/hwmon/Makefile
> > > > @@ -127,6 +127,7 @@ obj-$(CONFIG_SENSORS_LTC4260)   += ltc4260.o
> > > >  obj-$(CONFIG_SENSORS_LTC4261)  += ltc4261.o
> > > >  obj-$(CONFIG_SENSORS_LTQ_CPUTEMP) += ltq-cputemp.o
> > > >  obj-$(CONFIG_SENSORS_MAX)  += max.o
> > > > +obj-$(CONFIG_SENSORS_MAX127)   += max127.o
> > > >  obj-$(CONFIG_SENSORS_MAX16065) += max16065.o
> > > >  obj-$(CONFIG_SENSORS_MAX1619)  += max1619.o
> > > >  obj-$(CONFIG_SENSORS_MAX1668)  += max1668.o
> > > > diff --git a/drivers/hwmon/max127.c b/drivers/hwmon/max127.c
> > > > new file mode 100644
> > > > index ..3df4c225a6a2
> > > > --- /dev/null
> > > > +++ b/drivers/hwmon/max127.c
> > > > @@ -0,0 +1,346 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + * Hardware monitoring driver for MAX127.
> > > > + *
> > > > + * Copyright (c) 2020 Facebook Inc.
> > > > + */
> > > > +
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > 
> > > Not needed.
> > > 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > 
> > > Not needed.
> > 
> > Thanks for pointing it out. Both includes are deleted in v4.
> > 
> > > 
> > > > +
> > > > +/*
> > > > + * MAX127 Control Byte. Refer to MAX127 datasheet, Table 1 
> > > > "Control-Byte
> > > > + * Format" for details.
> &

Re: [PATCH v3 1/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring driver

[Tao Ren]

On Sun, Nov 22, 2020 at 10:33:42AM -0800, Guenter Roeck wrote:
> On Thu, Nov 19, 2020 at 09:53:23AM -0800, rentao.b...@gmail.com wrote:
> > From: Tao Ren 
> > 
> > Add hardware monitoring driver for the Maxim MAX127 chip.
> > 
> > MAX127 min/max range handling code is inspired by the max197 driver.
> > 
> > Signed-off-by: Tao Ren 
> 
> Nice cleanup. Couple of minor comments.
> 
> Thanks,
> Guenter
> 
> > ---
> >  Changes in v3:
> >- no code change. xdp maintainers were removed from to/cc list.
> >  Changes in v2:
> >- replace devm_hwmon_device_register_with_groups() with
> >  devm_hwmon_device_register_with_info() API.
> >- divide min/max read and write methods to separate functions.
> >- fix raw-to-vin conversion logic.
> >- refine ctrl_byte handling so mutex is not needed to protect the
> >  byte.
> >- improve i2c_transfer() error handling.
> >- a few other improvements (comments, variable naming, and etc.).
> > 
> >  drivers/hwmon/Kconfig  |   9 ++
> >  drivers/hwmon/Makefile |   1 +
> >  drivers/hwmon/max127.c | 346 +
> >  3 files changed, 356 insertions(+)
> >  create mode 100644 drivers/hwmon/max127.c
> > 
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 9d600e0c5584..716df51edc87 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -950,6 +950,15 @@ config SENSORS_MAX
> >   This driver can also be built as a module. If so, the module
> >   will be called max.
> >  
> > +config SENSORS_MAX127
> > +   tristate "Maxim MAX127 12-bit 8-channel Data Acquisition System"
> > +   depends on I2C
> > +   help
> > + Say y here to support Maxim's MAX127 DAS chips.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called max127.
> > +
> >  config SENSORS_MAX16065
> > tristate "Maxim MAX16065 System Manager and compatibles"
> > depends on I2C
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 1083bbfac779..01ca5d3fbad4 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -127,6 +127,7 @@ obj-$(CONFIG_SENSORS_LTC4260)   += ltc4260.o
> >  obj-$(CONFIG_SENSORS_LTC4261)  += ltc4261.o
> >  obj-$(CONFIG_SENSORS_LTQ_CPUTEMP) += ltq-cputemp.o
> >  obj-$(CONFIG_SENSORS_MAX)  += max.o
> > +obj-$(CONFIG_SENSORS_MAX127)   += max127.o
> >  obj-$(CONFIG_SENSORS_MAX16065) += max16065.o
> >  obj-$(CONFIG_SENSORS_MAX1619)  += max1619.o
> >  obj-$(CONFIG_SENSORS_MAX1668)  += max1668.o
> > diff --git a/drivers/hwmon/max127.c b/drivers/hwmon/max127.c
> > new file mode 100644
> > index ..3df4c225a6a2
> > --- /dev/null
> > +++ b/drivers/hwmon/max127.c
> > @@ -0,0 +1,346 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Hardware monitoring driver for MAX127.
> > + *
> > + * Copyright (c) 2020 Facebook Inc.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> 
> Not needed.
> 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> 
> Not needed.

Thanks for pointing it out. Both includes are deleted in v4.

> 
> > +
> > +/*
> > + * MAX127 Control Byte. Refer to MAX127 datasheet, Table 1 "Control-Byte
> > + * Format" for details.
> > + */
> > +#define MAX127_CTRL_START  BIT(7)
> > +#define MAX127_CTRL_SEL_SHIFT  4
> > +#define MAX127_CTRL_RNGBIT(3)
> > +#define MAX127_CTRL_BIPBIT(2)
> > +#define MAX127_CTRL_PD1BIT(1)
> > +#define MAX127_CTRL_PD0BIT(0)
> > +
> > +#define MAX127_NUM_CHANNELS8
> > +#define MAX127_SET_CHANNEL(ch) (((ch) & 7) << MAX127_CTRL_SEL_SHIFT)
> > +
> > +/*
> > + * MAX127 channel input ranges. Refer to MAX127 datasheet, Table 3 "Range
> > + * and Polarity Selection" for details.
> > + */
> > +#define MAX127_FULL_RANGE  1   /* 10V */
> > +#define MAX127_HALF_RANGE  5000/* 5V */
> > +
> > +/*
> > + * MAX127 returns 2 bytes at read:
> > + *   - the first byte contains data[11:4].
> > + *   - the second byte contains data[3:0] (MSB) and 4 dummy 0s (LSB).
> > + * Refer to MAX127 datasheet, "Read a Conversion (Read Cycle)" section
> > + * for details.
> > + */
> > +#define MAX127_DATA_LEN

Re: [PATCH v2 0/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring

[Tao Ren]

On Wed, Nov 18, 2020 at 05:26:53PM -0800, Guenter Roeck wrote:
> On Wed, Nov 18, 2020 at 05:01:19PM -0800, Guenter Roeck wrote:
> > On Wed, Nov 18, 2020 at 03:42:53PM -0800, Tao Ren wrote:
> > > On Thu, Nov 19, 2020 at 12:27:19AM +0100, Andrew Lunn wrote:
> > > > On Wed, Nov 18, 2020 at 03:09:27PM -0800, rentao.b...@gmail.com wrote:
> > > > > From: Tao Ren 
> > > > > 
> > > > > The patch series adds hardware monitoring driver for the Maxim MAX127
> > > > > chip.
> > > > 
> > > > Hi Tao
> > > > 
> > > > Why are using sending a hwmon driver to the networking mailing list?
> > > > 
> > > > Andrew
> > > 
> > > Hi Andrew,
> > > 
> > > I added netdev because the mailing list is included in "get_maintainer.pl
> > > Documentation/hwmon/index.rst" output. Is it the right command to find
> > > reviewers? Could you please suggest? Thank you.
> > 
> > I have no idea why running get_maintainer.pl on
> > Documentation/hwmon/index.rst returns such a large list of mailing
> > lists and people. For some reason it includes everyone in the XDP
> > maintainer list. If anyone has an idea how that happens, please
> > let me know - we'll want to get this fixed to avoid the same problem
> > in the future.
> > 
> 
> I found it. The XDP maintainer entry has:
> 
> K:xdp
> 
> This matches Documentation/hwmon/index.rst.
> 
> $ grep xdp Documentation/hwmon/index.rst
>xdpe12284
> 
> It seems to me that a context match such as "xdp" in MAINTAINERS isn't
> really appropriate. "xdp" matches a total of 348 files in the kernel.
> The large majority of those is not XDP related. The maintainers
> of XDP (and all the listed mailing lists) should not be surprised
> to get a large number of odd review requests if they want to review
> every single patch on files which include the term "xdp".
> 
> Guenter

Thanks Guenter and Andrew. Given xdp maintainers were included by
mistake, I will remove them from the future discussions of this hwmon
patch series.


Cheers,

Tao

Re: [PATCH v2 0/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring

[Tao Ren]

On Thu, Nov 19, 2020 at 12:27:19AM +0100, Andrew Lunn wrote:
> On Wed, Nov 18, 2020 at 03:09:27PM -0800, rentao.b...@gmail.com wrote:
> > From: Tao Ren 
> > 
> > The patch series adds hardware monitoring driver for the Maxim MAX127
> > chip.
> 
> Hi Tao
> 
> Why are using sending a hwmon driver to the networking mailing list?
> 
> Andrew

Hi Andrew,

I added netdev because the mailing list is included in "get_maintainer.pl
Documentation/hwmon/index.rst" output. Is it the right command to find
reviewers? Could you please suggest? Thank you.


Cheers,

Tao

Re: [PATCH 1/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring driver

[Tao Ren]

Hi Guenter,

Thanks for pointing out these problems. I'm working on the comments and
will send out v2 soon.


Cheers,

Tao

On Mon, Nov 16, 2020 at 09:13:52PM -0800, Guenter Roeck wrote:
> On Mon, Nov 16, 2020 at 05:09:43PM -0800, rentao.b...@gmail.com wrote:
> > From: Tao Ren 
> > 
> > Add hardware monitoring driver for the Maxim MAX127 chip.
> > 
> > MAX127 min/max range handling code is inspired by the max197 driver.
> > 
> > Signed-off-by: Tao Ren 
> > ---
> >  drivers/hwmon/Kconfig  |   9 ++
> >  drivers/hwmon/Makefile |   1 +
> >  drivers/hwmon/max127.c | 286 +
> >  3 files changed, 296 insertions(+)
> >  create mode 100644 drivers/hwmon/max127.c
> > 
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 9d600e0c5584..716df51edc87 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -950,6 +950,15 @@ config SENSORS_MAX
> >   This driver can also be built as a module. If so, the module
> >   will be called max.
> >  
> > +config SENSORS_MAX127
> > +   tristate "Maxim MAX127 12-bit 8-channel Data Acquisition System"
> > +   depends on I2C
> > +   help
> > + Say y here to support Maxim's MAX127 DAS chips.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called max127.
> > +
> >  config SENSORS_MAX16065
> > tristate "Maxim MAX16065 System Manager and compatibles"
> > depends on I2C
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 1083bbfac779..01ca5d3fbad4 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -127,6 +127,7 @@ obj-$(CONFIG_SENSORS_LTC4260)   += ltc4260.o
> >  obj-$(CONFIG_SENSORS_LTC4261)  += ltc4261.o
> >  obj-$(CONFIG_SENSORS_LTQ_CPUTEMP) += ltq-cputemp.o
> >  obj-$(CONFIG_SENSORS_MAX)  += max.o
> > +obj-$(CONFIG_SENSORS_MAX127)   += max127.o
> >  obj-$(CONFIG_SENSORS_MAX16065) += max16065.o
> >  obj-$(CONFIG_SENSORS_MAX1619)  += max1619.o
> >  obj-$(CONFIG_SENSORS_MAX1668)  += max1668.o
> > diff --git a/drivers/hwmon/max127.c b/drivers/hwmon/max127.c
> > new file mode 100644
> > index ..df74a95bcf28
> > --- /dev/null
> > +++ b/drivers/hwmon/max127.c
> > @@ -0,0 +1,286 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Hardware monitoring driver for MAX127.
> > + *
> > + * Copyright (c) 2020 Facebook Inc.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +/* MAX127 Control Byte. */
> > +#define MAX127_CTRL_START  BIT(7)
> > +#define MAX127_CTRL_SEL_OFFSET 4
> 
> That would better be named _SHIFT.
> 
> > +#define MAX127_CTRL_RNGBIT(3)
> > +#define MAX127_CTRL_BIPBIT(2)
> > +#define MAX127_CTRL_PD1BIT(1)
> > +#define MAX127_CTRL_PD0BIT(0)
> > +
> > +#define MAX127_NUM_CHANNELS8
> > +#define MAX127_SET_CHANNEL(ch) (((ch) & 7) << (MAX127_CTRL_SEL_OFFSET))
> 
> () around MAX127_CTRL_SEL_OFFSET is unnecessary.
> 
> > +
> > +#define MAX127_INPUT_LIMIT 10  /* 10V */
> > +
> > +/*
> > + * MAX127 returns 2 bytes at read:
> > + *   - the first byte contains data[11:4].
> > + *   - the second byte contains data[3:0] (MSB) and 4 dummy 0s (LSB).
> > + */
> > +#define MAX127_DATA1_SHIFT 4
> > +
> > +struct max127_data {
> > +   struct mutex lock;
> > +   struct i2c_client *client;
> > +   int input_limit;
> > +   u8 ctrl_byte[MAX127_NUM_CHANNELS];
> > +};
> > +
> > +static int max127_select_channel(struct max127_data *data, int channel)
> > +{
> > +   int status;
> > +   struct i2c_client *client = data->client;
> > +   struct i2c_msg msg = {
> > +   .addr = client->addr,
> > +   .flags = 0,
> > +   .len = 1,
> > +   .buf = >ctrl_byte[channel],
> > +   };
> > +
> > +   status = i2c_transfer(client->adapter, , 1);
> > +   if (status != 1)
> > +   return status;
> > +
> 
> Other drivers assume that this function can return 0. Please
> take that into account as well.
> 
> > +   return 0;
> > +}
> > +
> > +static int max127_read_channel(struct max127_data *data, int channel, u16 
> > *vin)
> > +{
> >

Re: [PATCH] ARM: dts: aspeed: minipack: Fixup I2C tree

[Tao Ren]

On Mon, Nov 16, 2020 at 03:33:45AM +, Joel Stanley wrote:
> On Tue, 10 Nov 2020 at 07:24,  wrote:
> >
> > From: Tao Ren 
> >
> > Create all the i2c switches in device tree and use aliases to assign
> > child channels with consistent bus numbers.
> >
> > Besides, "i2c-mux-idle-disconnect" is set for all the i2c switches to
> > avoid potential conflicts when multiple devices (beind the switches) use
> > the same device address.
> >
> > Signed-off-by: Tao Ren 
> 
> Reviewed-by: Joel Stanley 
> 
> I will apply for 5.11.

Great. Thank you Joel!


Cheers,

Tao

Re: [PATCH 0/4] ARM: dts: aspeed: Add Facebook Galaxy100 BMC

[Tao Ren]

On Thu, Nov 12, 2020 at 05:49:30AM +, Joel Stanley wrote:
> On Thu, 12 Nov 2020 at 03:18, Patrick Williams  wrote:
> >
> > On Wed, Nov 11, 2020 at 11:34:10PM +, Joel Stanley wrote:
> > > On Wed, 11 Nov 2020 at 23:23,  wrote:
> > > >
> > > > From: Tao Ren 
> > > >
> > > > The patch series adds the initial version of device tree for Facebook
> > > > Galaxy100 (AST2400) BMC.
> > > >
> > > > Patch #1 adds common dtsi to minimize duplicated device entries across
> > > > Facebook Network AST2400 BMC device trees.
> > > >
> > > > Patch #2 simplfies Wedge40 device tree by using the common dtsi.
> > > >
> > > > Patch #3 simplfies Wedge100 device tree by using the common dtsi.
> > > >
> > > > Patch #4 adds the initial version of device tree for Facebook Galaxy100
> > > > BMC.
> > >
> > > Nice. They look good to me.
> > >
> > > Reviewed-by: Joel Stanley 
> > >
> > > Is there another person familiar with the design you would like to
> > > review before I merge?
> >
> > Also,
> >
> > Reviewed-by: Patrick Williams 
> 
> Thanks. I have merged them into the aspeed tree for 5.11.
> 
> Cheers,
> 
> Joel

Thank you Joel.

BTW, I sent out another 2 patches for AST2500 dts earlier this week; not
sure if they were successfully delivered; if not, I will resend them
earlier next week.


Cheers,

Tao

Re: [PATCH 0/4] ARM: dts: aspeed: Add Facebook Galaxy100 BMC

[Tao Ren]

On Wed, Nov 11, 2020 at 09:18:28PM -0600, Patrick Williams wrote:
> On Wed, Nov 11, 2020 at 11:34:10PM +, Joel Stanley wrote:
> > On Wed, 11 Nov 2020 at 23:23,  wrote:
> > >
> > > From: Tao Ren 
> > >
> > > The patch series adds the initial version of device tree for Facebook
> > > Galaxy100 (AST2400) BMC.
> > >
> > > Patch #1 adds common dtsi to minimize duplicated device entries across
> > > Facebook Network AST2400 BMC device trees.
> > >
> > > Patch #2 simplfies Wedge40 device tree by using the common dtsi.
> > >
> > > Patch #3 simplfies Wedge100 device tree by using the common dtsi.
> > >
> > > Patch #4 adds the initial version of device tree for Facebook Galaxy100
> > > BMC.
> > 
> > Nice. They look good to me.
> > 
> > Reviewed-by: Joel Stanley 
> > 
> > Is there another person familiar with the design you would like to
> > review before I merge?
> 
> Also,
> 
> Reviewed-by: Patrick Williams 
> 
> -- 
> Patrick Williams

Thank you Patrick!

Cheers,

Tao

Re: [PATCH 0/4] ARM: dts: aspeed: Add Facebook Galaxy100 BMC

[Tao Ren]

On Wed, Nov 11, 2020 at 11:34:10PM +, Joel Stanley wrote:
> On Wed, 11 Nov 2020 at 23:23,  wrote:
> >
> > From: Tao Ren 
> >
> > The patch series adds the initial version of device tree for Facebook
> > Galaxy100 (AST2400) BMC.
> >
> > Patch #1 adds common dtsi to minimize duplicated device entries across
> > Facebook Network AST2400 BMC device trees.
> >
> > Patch #2 simplfies Wedge40 device tree by using the common dtsi.
> >
> > Patch #3 simplfies Wedge100 device tree by using the common dtsi.
> >
> > Patch #4 adds the initial version of device tree for Facebook Galaxy100
> > BMC.
> 
> Nice. They look good to me.
> 
> Reviewed-by: Joel Stanley 

Thanks a lot for the quick review, Joel.

> Is there another person familiar with the design you would like to
> review before I merge?

Patrick Williams sometimes helps reviewing my patches although he doesn't
work on Network BMCs. Let me see if he has bandwidth this time :)


Cheers,

Tao

Re: [PATCH 3/3] ARM: dts: add ehci uhci enable in evb dts

[Tao Ren]

On Wed, Sep 30, 2020 at 12:08:23PM +0800, Ryan Chen wrote:
> Add EHCI UHCI enable build in aspeed-ast2600-evb.dts
> 
> Signed-off-by: Ryan Chen 

Reviewed-by: Tao Ren 

Re: [PATCH 2/3] usb: host: add uhci compatible support for ast2600-uhci

[Tao Ren]

On Wed, Sep 30, 2020 at 12:08:22PM +0800, Ryan Chen wrote:
> Add support for AST2600 SOC UHCI driver.
> 
> Signed-off-by: Ryan Chen 

Reviewed-by: Tao Ren 

Re: [PATCH 1/3] configs: aspeed: enable UHCI driver in defconfig

[Tao Ren]

On Wed, Sep 30, 2020 at 12:08:21PM +0800, Ryan Chen wrote:
> Enable UHCI driver in aspeed_g5_defconfig.
> 
> Signed-off-by: Ryan Chen 

Reviewed-by: Tao Ren 

Re: [PATCH] gpio: aspeed: fix ast2600 bank properties

[Tao Ren]

On Thu, Sep 17, 2020 at 08:42:27AM +0930, Andrew Jeffery wrote:
> 
> 
> On Thu, 17 Sep 2020, at 06:12, rentao.b...@gmail.com wrote:
> > From: Tao Ren 
> > 
> > GPIO_U is mapped to the least significant byte of input/output mask, and
> > the byte in "output" mask should be 0 because GPIO_U is input only. All
> > the other bits need to be 1 because GPIO_V/W/X support both input and
> > output modes.
> > 
> > Similarly, GPIO_Y/Z are mapped to the 2 least significant bytes, and the
> > according bits need to be 1 because GPIO_Y/Z support both input and
> > output modes.
> > 
> > Fixes: ab4a85534c3e ("gpio: aspeed: Add in ast2600 details to Aspeed 
> > driver")
> > Signed-off-by: Tao Ren 
> 
> Thanks Tao,
> 
> Reviewed-by: Andrew Jeffery 

Thanks Andrew for the quick review.

Cheers,

Tao

Re: [PATCH v2 3/5] i2c: aspeed: Mask IRQ status to relevant bits

[Tao Ren]

On Tue, Sep 08, 2020 at 03:00:59PM -0500, Eddie James wrote:
> Mask the IRQ status to only the bits that the driver checks. This
> prevents excessive driver warnings when operating in slave mode
> when additional bits are set that the driver doesn't handle.
> 
> Signed-off-by: Eddie James 

Reviewed-by: Tao Ren 

> ---
>  drivers/i2c/busses/i2c-aspeed.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 31268074c422..2a388911038a 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -69,6 +69,7 @@
>   * These share bit definitions, so use the same values for the enable &
>   * status bits.
>   */
> +#define ASPEED_I2CD_INTR_ALL 0xf000
>  #define ASPEED_I2CD_INTR_SDA_DL_TIMEOUT  BIT(14)
>  #define ASPEED_I2CD_INTR_BUS_RECOVER_DONEBIT(13)
>  #define ASPEED_I2CD_INTR_SLAVE_MATCH BIT(7)
> @@ -604,6 +605,7 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void 
> *dev_id)
>   writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
>  bus->base + ASPEED_I2C_INTR_STS_REG);
>   readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> + irq_received &= ASPEED_I2CD_INTR_ALL;
>   irq_remaining = irq_received;
>  
>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> -- 
> 2.26.2
> 

Re: [PATCH] usb: gadget: aspeed: fixup vhub port irq handling

[Tao Ren]

On Mon, Aug 31, 2020 at 12:54:57PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Tao Ren  writes:
> > On Mon, Aug 17, 2020 at 04:49:32PM +0300, Felipe Balbi wrote:
> >> 
> >> Hi,
> >> 
> >> rentao.b...@gmail.com writes:
> >> > From: Tao Ren 
> >> >
> >> > This is a follow-on patch for commit a23be4ed8f48 ("usb: gadget: aspeed:
> >> > improve vhub port irq handling"): for_each_set_bit() is replaced with
> >> > simple for() loop because for() loop runs faster on ASPEED BMC.
> >> >
> >> > Signed-off-by: Tao Ren 
> >> > ---
> >> >  drivers/usb/gadget/udc/aspeed-vhub/core.c | 10 +++---
> >> >  drivers/usb/gadget/udc/aspeed-vhub/vhub.h |  3 +++
> >> >  2 files changed, 6 insertions(+), 7 deletions(-)
> >> >
> >> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c 
> >> > b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> >> > index cdf96911e4b1..be7bb64e3594 100644
> >> > --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> >> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> >> > @@ -135,13 +135,9 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
> >> >  
> >> >  /* Handle device interrupts */
> >> >  if (istat & vhub->port_irq_mask) {
> >> > -unsigned long bitmap = istat;
> >> > -int offset = VHUB_IRQ_DEV1_BIT;
> >> > -int size = VHUB_IRQ_DEV1_BIT + vhub->max_ports;
> >> > -
> >> > -for_each_set_bit_from(offset, , size) {
> >> > -i = offset - VHUB_IRQ_DEV1_BIT;
> >> > -ast_vhub_dev_irq(>ports[i].dev);
> >> > +for (i = 0; i < vhub->max_ports; i++) {
> >> > +if (istat & VHUB_DEV_IRQ(i))
> >> > +ast_vhub_dev_irq(>ports[i].dev);
> >> 
> >> how have you measured your statement above? for_each_set_bit() does
> >> exactly what you did. Unless your architecture has an instruction which
> >> helps finds the next set bit (like cls on ARM), which, then, makes it
> >> much faster.
> >
> > I did some testing and result shows for() loop runs faster than
> > for_each_set_bit() loop. Please refer to details below (discussion with
> > Benjamin in the original patch) and kindly let me know your suggestions.
> 
> no strong feelings, just surprised that you're already worried about
> 20~40 cycles of cpu time ;-)
> 
> Patch applied for next merge window

Thanks Felipe. Ben had some concerns about interrupt handling cost on
AST2400 BMC (ARM9), hence we did the comparison and noticed the small
difference :)


Cheers,

Tao

Re: [PATCH 4/5] ARM: dts: aspeed: minipack: Update 64MB FMC flash layout

[Tao Ren]

On Tue, Aug 25, 2020 at 09:18:08AM -0500, Patrick Williams wrote:
> On Mon, Aug 24, 2020 at 02:19:47PM -0700, rentao.b...@gmail.com wrote:
> > From: Tao Ren 
> > 
> > Set 64Mb FMC flash layout in Minipack device tree explicitly because the
> > flash layout was removed from "ast2500-facebook-netbmc-common.dtsi".
> > 
> > Please note "data0" partition' size is updated to 4MB to be consistent
> > with other Facebook OpenBMC platforms.
> > 
> > Signed-off-by: Tao Ren 
> > ---
> >  .../boot/dts/aspeed-bmc-facebook-minipack.dts | 47 ++-
> >  1 file changed, 45 insertions(+), 2 deletions(-)
> > 
> 
> Reviewed-by: Patrick Williams 

Thanks for the review, Patrick.


Cheers,

Tao

Re: [PATCH 5/5] ARM: dts: aspeed: Add Facebook Wedge400 BMC

[Tao Ren]

On Tue, Aug 25, 2020 at 04:07:56AM +, Joel Stanley wrote:
> On Mon, 24 Aug 2020 at 21:20,  wrote:
> >
> > From: Tao Ren 
> >
> > Add initial version of device tree for Facebook Wedge400 (AST2500) BMC.
> >
> > Signed-off-by: Tao Ren 
> 
> Reviewed-by: Joel Stanley 

Thank you for the review, Joel.


Cheers,

Tao

Re: [PATCH 3/5] i2c: aspeed: Mask IRQ status to relevant bits

[Tao Ren]

On Tue, Aug 25, 2020 at 02:47:51PM -0500, Eddie James wrote:
> 
> On 8/25/20 1:38 AM, Joel Stanley wrote:
> > On Thu, 20 Aug 2020 at 16:12, Eddie James  wrote:
> > > Mask the IRQ status to only the bits that the driver checks. This
> > > prevents excessive driver warnings when operating in slave mode
> > > when additional bits are set that the driver doesn't handle.
> > > 
> > > Signed-off-by: Eddie James 
> > > ---
> > >   drivers/i2c/busses/i2c-aspeed.c | 1 +
> > >   1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-aspeed.c 
> > > b/drivers/i2c/busses/i2c-aspeed.c
> > > index 31268074c422..abf40f2af8b4 100644
> > > --- a/drivers/i2c/busses/i2c-aspeed.c
> > > +++ b/drivers/i2c/busses/i2c-aspeed.c
> > > @@ -604,6 +604,7 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void 
> > > *dev_id)
> > >  writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> > > bus->base + ASPEED_I2C_INTR_STS_REG);
> > >  readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> > > +   irq_received &= 0xf000;
> > >  irq_remaining = irq_received;
> > This would defeat the check for irq_remaining. I don't think we want to do 
> > this.
> > 
> > Can you explain why these bits are being set in slave mode?
> 
> 
> No, I don't have any documentation for the bits that are masked off here, so
> I don't know why they would get set.
> 
> The check for irq_remaining is still useful for detecting that the driver
> state machine might be out of sync with what the master is doing.

I have a similar patch in my local tree, and the reason being: AST2600
I2C Controller may set I2CD10[25:24] to report Current Slave Parking
Status (defined in new register I2CS24) even though the new register
mode is off. The 2 bits can be ignored in legacy mode, and Ryan from
ASPEED could confirm it.


Cheers,

Tao

Re: [PATCH] usb: gadget: aspeed: fixup vhub port irq handling

[Tao Ren]

On Mon, Aug 17, 2020 at 04:49:32PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> rentao.b...@gmail.com writes:
> > From: Tao Ren 
> >
> > This is a follow-on patch for commit a23be4ed8f48 ("usb: gadget: aspeed:
> > improve vhub port irq handling"): for_each_set_bit() is replaced with
> > simple for() loop because for() loop runs faster on ASPEED BMC.
> >
> > Signed-off-by: Tao Ren 
> > ---
> >  drivers/usb/gadget/udc/aspeed-vhub/core.c | 10 +++---
> >  drivers/usb/gadget/udc/aspeed-vhub/vhub.h |  3 +++
> >  2 files changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c 
> > b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > index cdf96911e4b1..be7bb64e3594 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > @@ -135,13 +135,9 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
> >  
> > /* Handle device interrupts */
> > if (istat & vhub->port_irq_mask) {
> > -   unsigned long bitmap = istat;
> > -   int offset = VHUB_IRQ_DEV1_BIT;
> > -   int size = VHUB_IRQ_DEV1_BIT + vhub->max_ports;
> > -
> > -   for_each_set_bit_from(offset, , size) {
> > -   i = offset - VHUB_IRQ_DEV1_BIT;
> > -   ast_vhub_dev_irq(>ports[i].dev);
> > +   for (i = 0; i < vhub->max_ports; i++) {
> > +   if (istat & VHUB_DEV_IRQ(i))
> > +   ast_vhub_dev_irq(>ports[i].dev);
> 
> how have you measured your statement above? for_each_set_bit() does
> exactly what you did. Unless your architecture has an instruction which
> helps finds the next set bit (like cls on ARM), which, then, makes it
> much faster.

I did some testing and result shows for() loop runs faster than
for_each_set_bit() loop. Please refer to details below (discussion with
Benjamin in the original patch) and kindly let me know your suggestions.

> On Mon, 2020-04-06 at 23:02 -0700, Tao Ren wrote:
> > I ran some testing on my ast2400 and ast2500 BMC and looks like the
> > for() loop runs faster than for_each_set_bit_from() loop in my
> > environment. I'm not sure if something needs to be revised in my test
> > code, but please kindly share your suggestions:
> >
> > I use get_cycles() to calculate execution time of 2 different loops, and
> > ast_vhub_dev_irq() is replaced with barrier() to avoid "noise"; below
> > are the results:
> >
> >   - when downstream port number is 5 and only 1 irq bit is set, it takes
> > ~30 cycles to finish for_each_set_bit() loop, and 20-25 cycles to
> > finish the for() loop.
> >
> >   - if downstream port number is 5 and all 5 bits are set, then
> > for_each_set_bit() loop takes ~50 cycles and for() loop takes ~25
> > cycles.
> >
> >   - when I increase downsteam port number to 16 and set 1 irq bit, the
> > for_each_set_bit() loop takes ~30 cycles and for() loop takes 25
> > cycles. It's a little surprise to me because I thought for() loop
> > would cost 60+ cycles (3 times of the value when port number is 5).
> >
> >   - if downstream port number is 16 and all irq status bits are set,
> > then for_each_set_bit() loop takes 60-70 cycles and for() loop takes
> > 30+ cycles.


Cheers,

Tao

Re: [PATCH v2 0/3] ARM: dts: aspeed: fixup wedge40 device tree

[Tao Ren]

On Fri, Jul 24, 2020 at 05:32:30AM +, Joel Stanley wrote:
> On Thu, 23 Jul 2020 at 23:05,  wrote:
> >
> > From: Tao Ren 
> >
> > The patch series update several devices' settings in Facebook Wedge40
> > device tree.
> >
> > Patch #1 disables a few i2c controllers as they are not being used at
> > present.
> >
> > Patch #2 enables adc device for voltage monitoring.
> >
> > Patch #3 enables pwm_tacho device for fan control and monitoring.
> >
> > Tao Ren (3):
> >   ARM: dts: aspeed: wedge40: disable a few i2c controllers
> >   ARM: dts: aspeed: wedge40: enable adc device
> >   ARM: dts: aspeed: wedge40: enable pwm_tacho device
> 
> I have merged this series into the aspeed dt-for-5.9 branch.
> 
> Cheers,
> 
> Joel

Thanks a lot Joel. Have a great weekend.


Cheers,

Tao

Re: [PATCH] usb: gadget: fix langid kernel-doc warning in usbstring.c

[Tao Ren]

On Sun, Jun 28, 2020 at 08:08:03PM -0700, Randy Dunlap wrote:
> From: Randy Dunlap 
> 
> Fix spelling of the 'langid' function argument in the kernel-doc
> notation to quieten a kernel-doc warning.
> 
> ../drivers/usb/gadget/usbstring.c:77: warning: Function parameter or member 
> 'langid' not described in 'usb_validate_langid'
> ../drivers/usb/gadget/usbstring.c:77: warning: Excess function parameter 
> 'lang' description in 'usb_validate_langid'
> 
> Fixes: 17309a6a4356 ("usb: gadget: add "usb_validate_langid" function")
> Signed-off-by: Randy Dunlap 
> Cc: Tao Ren 
> Cc: Felipe Balbi 

Thanks for the fix, Randy!

Reviewed-by: Tao Ren 

> ---
>  drivers/usb/gadget/usbstring.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- linux-next-20200626.orig/drivers/usb/gadget/usbstring.c
> +++ linux-next-20200626/drivers/usb/gadget/usbstring.c
> @@ -68,7 +68,7 @@ EXPORT_SYMBOL_GPL(usb_gadget_get_string)
>  
>  /**
>   * usb_validate_langid - validate usb language identifiers
> - * @lang: usb language identifier
> + * @langid: usb language identifier
>   *
>   * Returns true for valid language identifier, otherwise false.
>   */
> 

Re: [PATCH] usb: gadget: aspeed: fixup vhub port irq handling

[Tao Ren]

Hi Ben,

I sent out the follow-on patch per Greg's suggestion, and the purpose is
to include the latest version (v4) of the original commit. As v4 was
already Acked-by you, can I add the tag for this patch? Or are you
willing to Ack it again?


Cheers,

Tao

On Wed, May 27, 2020 at 06:11:54PM -0700, rentao.b...@gmail.com wrote:
> From: Tao Ren 
> 
> This is a follow-on patch for commit a23be4ed8f48 ("usb: gadget: aspeed:
> improve vhub port irq handling"): for_each_set_bit() is replaced with
> simple for() loop because for() loop runs faster on ASPEED BMC.
> 
> Signed-off-by: Tao Ren 
> ---
>  drivers/usb/gadget/udc/aspeed-vhub/core.c | 10 +++---
>  drivers/usb/gadget/udc/aspeed-vhub/vhub.h |  3 +++
>  2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c 
> b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> index cdf96911e4b1..be7bb64e3594 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> @@ -135,13 +135,9 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
>  
>   /* Handle device interrupts */
>   if (istat & vhub->port_irq_mask) {
> - unsigned long bitmap = istat;
> - int offset = VHUB_IRQ_DEV1_BIT;
> - int size = VHUB_IRQ_DEV1_BIT + vhub->max_ports;
> -
> - for_each_set_bit_from(offset, , size) {
> - i = offset - VHUB_IRQ_DEV1_BIT;
> - ast_vhub_dev_irq(>ports[i].dev);
> + for (i = 0; i < vhub->max_ports; i++) {
> + if (istat & VHUB_DEV_IRQ(i))
> + ast_vhub_dev_irq(>ports[i].dev);
>   }
>   }
>  
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h 
> b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> index 2e5a1ef14a75..87a5dea12d3c 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> @@ -67,6 +67,9 @@
>  #define VHUB_IRQ_HUB_EP0_SETUP   (1 << 0)
>  #define VHUB_IRQ_ACK_ALL 0x1ff
>  
> +/* Downstream device IRQ mask. */
> +#define VHUB_DEV_IRQ(n)  (VHUB_IRQ_DEVICE1 << 
> (n))
> +
>  /* SW reset reg */
>  #define VHUB_SW_RESET_EP_POOL(1 << 9)
>  #define VHUB_SW_RESET_DMA_CONTROLLER (1 << 8)
> -- 
> 2.17.1
> 

[PATCH 1/4] ARM: dts: aspeed: add dtsi for Facebook AST2500 Network BMCs

[Tao Ren]

Introduce "facebook-netbmc-ast2500-common.dtsi" which is included by all
Facebook AST2500 Network BMC platforms. The major purpose is to minimize
duplicated device entries cross Facebook Network BMC dts files.

Signed-off-by: Tao Ren 
---
 .../dts/facebook-netbmc-ast2500-common.dtsi   | 96 +++
 1 file changed, 96 insertions(+)
 create mode 100644 arch/arm/boot/dts/facebook-netbmc-ast2500-common.dtsi

diff --git a/arch/arm/boot/dts/facebook-netbmc-ast2500-common.dtsi 
b/arch/arm/boot/dts/facebook-netbmc-ast2500-common.dtsi
new file mode 100644
index ..7a395ba56512
--- /dev/null
+++ b/arch/arm/boot/dts/facebook-netbmc-ast2500-common.dtsi
@@ -0,0 +1,96 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Copyright (c) 2019 Facebook Inc.
+
+#include "aspeed-g5.dtsi"
+
+/ {
+   memory@8000 {
+   reg = <0x8000 0x4000>;
+   };
+};
+
+/*
+ * Update reset type to "system" (full chip) to fix warm reboot hang issue
+ * when reset type is set to default ("soc", gated by reset mask registers).
+ */
+ {
+   status = "okay";
+   aspeed,reset-type = "system";
+};
+
+ {
+   status = "disabled";
+};
+
+ {
+   status = "okay";
+   pinctrl-names = "default";
+   pinctrl-0 = <_txd1_default
+_rxd1_default>;
+};
+
+ {
+   status = "okay";
+   pinctrl-names = "default";
+   pinctrl-0 = <_txd3_default
+_rxd3_default>;
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+
+   fmc_flash0: flash@0 {
+   status = "okay";
+   m25p,fast-read;
+   label = "spi0.0";
+
+#include "facebook-bmc-flash-layout.dtsi"
+   };
+
+   fmc_flash1: flash@1 {
+   status = "okay";
+   m25p,fast-read;
+   label = "spi0.1";
+
+   partitions {
+   compatible = "fixed-partitions";
+   #address-cells = <1>;
+   #size-cells = <1>;
+
+   flash1@0 {
+   reg = <0x0 0x200>;
+   label = "flash1";
+   };
+   };
+   };
+};
+
+ {
+   status = "okay";
+   no-hw-checksum;
+   pinctrl-names = "default";
+   pinctrl-0 = <_rgmii2_default _mdio2_default>;
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+
+   pinctrl-names = "default";
+   pinctrl-0 = <_sd2_default>;
+};
-- 
2.17.1

[PATCH 4/4] ARM: dts: aspeed: yamp: include dtsi for common network BMC devices

[Tao Ren]

The patch simplifies Yamp device tree by including dtsi to define
devices which are common to Facebook AST2500 Network BMC platforms.

Below is the summary of changes comparing with previous dts version:
  - enabling the second firmware flash.
  - enabling the emmc device in slot #1.

Signed-off-by: Tao Ren 
---
 .../arm/boot/dts/aspeed-bmc-facebook-yamp.dts | 62 ++-
 1 file changed, 5 insertions(+), 57 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-yamp.dts 
b/arch/arm/boot/dts/aspeed-bmc-facebook-yamp.dts
index 4e09a9cf32b7..b184fa1abb60 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-yamp.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-yamp.dts
@@ -2,7 +2,7 @@
 // Copyright (c) 2018 Facebook Inc.
 /dts-v1/;
 
-#include "aspeed-g5.dtsi"
+#include "facebook-netbmc-ast2500-common.dtsi"
 
 / {
model = "Facebook YAMP 100 BMC";
@@ -23,47 +23,6 @@
stdout-path = 
bootargs = "console=ttyS0,9600n8 root=/dev/ram rw";
};
-
-   memory@8000 {
-   reg = <0x8000 0x2000>;
-   };
-};
-
- {
-   aspeed,external-nodes = < >;
-};
-
-/*
- * Update reset type to "system" (full chip) to fix warm reboot hang issue
- * when reset type is set to default ("soc", gated by reset mask registers).
- */
- {
-   status = "okay";
-   aspeed,reset-type = "system";
-};
-
-/*
- * wdt2 is not used by Yamp.
- */
- {
-   status = "disabled";
-};
-
- {
-   status = "okay";
-   flash@0 {
-   status = "okay";
-   m25p,fast-read;
-   label = "bmc";
-#include "facebook-bmc-flash-layout.dtsi"
-   };
-};
-
- {
-   status = "okay";
-   pinctrl-names = "default";
-   pinctrl-0 = <_txd1_default
-_rxd1_default>;
 };
 
  {
@@ -73,17 +32,6 @@
 _rxd2_default>;
 };
 
- {
-   status = "okay";
-   pinctrl-names = "default";
-   pinctrl-0 = <_txd3_default
-_rxd3_default>;
-};
-
- {
-   status = "okay";
-};
-
  {
status = "okay";
use-ncsi;
@@ -92,6 +40,10 @@
pinctrl-0 = <_rmii1_default>;
 };
 
+ {
+   status = "disabled";
+};
+
  {
status = "okay";
 };
@@ -154,7 +106,3 @@
  {
status = "okay";
 };
-
- {
-   status = "okay";
-};
-- 
2.17.1

[PATCH 2/4] ARM: dts: aspeed: cmm: include dtsi for common network BMC devices

[Tao Ren]

The patch simplifies CMM device tree by including dtsi to define devices
which are common to Facebook AST2500 Network BMC platforms.

Below is the summary of changes comparing with previous dts version:
  - enabling the second firmware flash.
  - enabling the emmc device in slot #0.

Signed-off-by: Tao Ren 
---
 arch/arm/boot/dts/aspeed-bmc-facebook-cmm.dts | 66 +--
 1 file changed, 16 insertions(+), 50 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-cmm.dts 
b/arch/arm/boot/dts/aspeed-bmc-facebook-cmm.dts
index d519d307aa2a..0a031132594a 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-cmm.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-cmm.dts
@@ -2,7 +2,7 @@
 // Copyright (c) 2018 Facebook Inc.
 /dts-v1/;
 
-#include "aspeed-g5.dtsi"
+#include "facebook-netbmc-ast2500-common.dtsi"
 
 / {
model = "Facebook Backpack CMM BMC";
@@ -53,10 +53,6 @@
bootargs = "console=ttyS1,9600n8 root=/dev/ram rw earlyprintk";
};
 
-   memory@8000 {
-   reg = <0x8000 0x2000>;
-   };
-
ast-adc-hwmon {
compatible = "iio-hwmon";
io-channels = < 0>, < 1>, < 2>, < 3>,
@@ -64,39 +60,7 @@
};
 };
 
- {
-   aspeed,external-nodes = < >;
-};
-
-/*
- * Update reset type to "system" (full chip) to fix warm reboot hang issue
- * when reset type is set to default ("soc", gated by reset mask registers).
- */
- {
-   status = "okay";
-   aspeed,reset-type = "system";
-};
-
-/*
- * wdt2 is not used by Backpack CMM.
- */
- {
-   status = "disabled";
-};
-
- {
-   status = "okay";
-   flash@0 {
-   status = "okay";
-   m25p,fast-read;
-   label = "bmc";
-#include "facebook-bmc-flash-layout.dtsi"
-   };
-};
-
  {
-   status = "okay";
-   pinctrl-names = "default";
pinctrl-0 = <_txd1_default
 _rxd1_default
 _ncts1_default
@@ -107,8 +71,6 @@
 };
 
  {
-   status = "okay";
-   pinctrl-names = "default";
pinctrl-0 = <_txd3_default
 _rxd3_default
 _ncts3_default
@@ -123,17 +85,6 @@
 _rxd4_default>;
 };
 
- {
-   status = "okay";
-};
-
- {
-   status = "okay";
-   no-hw-checksum;
-   pinctrl-names = "default";
-   pinctrl-0 = <_rgmii2_default _mdio2_default>;
-};
-
 /*
  * I2C bus reserved for communication with COM-E.
  */
@@ -380,3 +331,18 @@
  {
status = "okay";
 };
+
+ {
+   status = "disabled";
+};
+
+ {
+   status = "okay";
+
+   pinctrl-names = "default";
+   pinctrl-0 = <_sd1_default>;
+};
+
+ {
+   status = "disabled";
+};
-- 
2.17.1

[PATCH 3/4] ARM: dts: aspeed: minipack: include dtsi for common network BMC devices

[Tao Ren]

The patch simplifies Minipack device tree by including dtsi to define
devices which are common to Facebook AST2500 Network BMC platforms.

Below is the summary of changes comparing with previous dts version:
  - enabling the second firmware flash.
  - updating firmware flashes' size from 32MB to 64MB.
  - enabling the emmc device in slot #1.

Signed-off-by: Tao Ren 
---
 .../boot/dts/aspeed-bmc-facebook-minipack.dts | 59 ++-
 1 file changed, 19 insertions(+), 40 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-minipack.dts 
b/arch/arm/boot/dts/aspeed-bmc-facebook-minipack.dts
index c05478296446..ed1a77c76ce9 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-minipack.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-minipack.dts
@@ -2,7 +2,7 @@
 // Copyright (c) 2018 Facebook Inc.
 /dts-v1/;
 
-#include "aspeed-g5.dtsi"
+#include "facebook-netbmc-ast2500-common.dtsi"
 
 / {
model = "Facebook Minipack 100 BMC";
@@ -76,35 +76,36 @@
stdout-path = 
bootargs = "debug console=ttyS1,9600n8 root=/dev/ram rw";
};
-
-   memory@8000 {
-   reg = <0x8000 0x2000>;
-   };
 };
 
- {
+ {
status = "okay";
aspeed,reset-type = "system";
 };
 
- {
-   status = "okay";
-   aspeed,reset-type = "system";
+/*
+ * Both firmware flashes are 64MB on Minipack BMC.
+ */
+_flash0 {
+   partitions {
+   data0@1c0 {
+   reg = <0x1c0 0x240>;
+   };
+   flash0@0 {
+   reg = <0x0 0x400>;
+   };
+   };
 };
 
- {
-   status = "okay";
-   flash@0 {
-   status = "okay";
-   m25p,fast-read;
-   label = "bmc";
-#include "facebook-bmc-flash-layout.dtsi"
+_flash1 {
+   partitions {
+   flash1@0 {
+   reg = <0x0 0x400>;
+   };
};
 };
 
  {
-   status = "okay";
-   pinctrl-names = "default";
pinctrl-0 = <_txd1_default
 _rxd1_default
 _ncts1_default
@@ -120,13 +121,6 @@
 _rxd2_default>;
 };
 
- {
-   status = "okay";
-   pinctrl-names = "default";
-   pinctrl-0 = <_txd3_default
-_rxd3_default>;
-};
-
  {
status = "okay";
pinctrl-names = "default";
@@ -134,17 +128,6 @@
 _rxd4_default>;
 };
 
- {
-   status = "okay";
-};
-
- {
-   status = "okay";
-   no-hw-checksum;
-   pinctrl-names = "default";
-   pinctrl-0 = <_rgmii2_default _mdio2_default>;
-};
-
  {
status = "okay";
bus-frequency = <40>;
@@ -423,7 +406,3 @@
  {
status = "okay";
 };
-
- {
-   status = "okay";
-};
-- 
2.17.1

[PATCH 0/4] ARM: dts: aspeed: add dtsi for Facebook AST2500 Network BMCs

[Tao Ren]

The patch series adds "facebook-netbmc-ast2500-common.dtsi" which defines
devices that are common cross all Facebook AST2500 Network BMC platforms.
The major purpose is to minimize duplicated device entries among Facebook
Network BMC dts files.

Patch #1 (of 4) adds "facebook-netbmc-ast2500-common.dtsi" file, and the
remaining 3 patches update CMM, Minipack and Yamp device tree to consume
the new dtsi file.

Tao Ren (4):
  ARM: dts: aspeed: add dtsi for Facebook AST2500 Network BMCs
  ARM: dts: aspeed: cmm: include dtsi for common network BMC devices
  ARM: dts: aspeed: minipack: include dtsi for common network BMC
devices
  ARM: dts: aspeed: yamp: include dtsi for common network BMC devices

 arch/arm/boot/dts/aspeed-bmc-facebook-cmm.dts | 66 -
 .../boot/dts/aspeed-bmc-facebook-minipack.dts | 59 
 .../arm/boot/dts/aspeed-bmc-facebook-yamp.dts | 62 +---
 .../dts/facebook-netbmc-ast2500-common.dtsi   | 96 +++
 4 files changed, 136 insertions(+), 147 deletions(-)
 create mode 100644 arch/arm/boot/dts/facebook-netbmc-ast2500-common.dtsi

-- 
2.17.1

Re: [PATCH net-next v9 0/3] net: phy: support 1000Base-X auto-negotiation for BCM54616S

[Tao Ren]

Hi David,

I'm not sure if the patch series v9 was delivered to your mailbox successfully,
but somehow I still cannot find the patches in patchwork.

Any suggestions? Maybe my git is mis-configured? 


Thanks,

Tao

On 10/15/19 12:12 PM, Tao Ren wrote:
> This patch series aims at supporting auto negotiation when BCM54616S is
> running in 1000Base-X mode: without the patch series, BCM54616S PHY driver
> would report incorrect link speed in 1000Base-X mode.
> 
> Patch #1 (of 3) modifies assignment to OR when dealing with dev_flags in
> phy_attach_direct function, so that dev_flags updated in BCM54616S PHY's
> probe callback won't be lost.
> 
> Patch #2 (of 3) adds several genphy_c37_* functions to support clause 37
> 1000Base-X auto-negotiation, and these functions are called in BCM54616S
> PHY driver.
> 
> Patch #3 (of 3) detects BCM54616S PHY's operation mode and calls according
> genphy_c37_* functions to configure auto-negotiation and parse link
> attributes (speed, duplex, and etc.) in 1000Base-X mode.
> 
> Heiner Kallweit (1):
>   net: phy: add support for clause 37 auto-negotiation
> 
> Tao Ren (2):
>   net: phy: modify assignment to OR for dev_flags in phy_attach_direct
>   net: phy: broadcom: add 1000Base-X support for BCM54616S
> 
>  drivers/net/phy/broadcom.c   |  57 +-
>  drivers/net/phy/phy_device.c | 141 ++-
>  include/linux/brcmphy.h  |  10 ++-
>  include/linux/phy.h  |   4 +
>  4 files changed, 205 insertions(+), 7 deletions(-)
> 

Re: [PATCH net-next v8 2/3] net: phy: add support for clause 37 auto-negotiation

[Tao Ren]

On 10/15/19 10:22 AM, Florian Fainelli wrote:
> On 10/15/19 10:20 AM, David Miller wrote:
>> From: Tao Ren 
>> Date: Tue, 15 Oct 2019 17:16:26 +
>>
>>> Can you please apply the patch series to net-next tree when you have
>>> bandwidth? All the 3 patches are reviewed.
>>
>> If it is not active in patchwork you need to repost.
>>
> 
> Tao, can you pick up this series and provide a proper cover letter for
> it (git format-patch --cover-letter) that way it can be picked up by David?

Sure. Thank you for the suggestion. Will do it.


Thanks,

Tao

Re: [PATCH net-next v8 2/3] net: phy: add support for clause 37 auto-negotiation

[Tao Ren]

On 10/15/19 10:20 AM, David Miller wrote:
> From: Tao Ren 
> Date: Tue, 15 Oct 2019 17:16:26 +
> 
>> Can you please apply the patch series to net-next tree when you have
>> bandwidth? All the 3 patches are reviewed.
> 
> If it is not active in patchwork you need to repost.

Got it. Let me check and repost soon.

Thanks,

Tao

Re: [PATCH net-next v8 2/3] net: phy: add support for clause 37 auto-negotiation

[Tao Ren]

Hi David,

Can you please apply the patch series to net-next tree when you have bandwidth? 
All the 3 patches are reviewed.


Thanks,

Tao

On 9/14/19 7:17 AM, Andrew Lunn wrote:
> On Mon, Sep 09, 2019 at 01:49:06PM -0700, Tao Ren wrote:
>> From: Heiner Kallweit 
>>
>> This patch adds support for clause 37 1000Base-X auto-negotiation.
>>
>> Signed-off-by: Heiner Kallweit 
>> Signed-off-by: Tao Ren 
>> Tested-by: René van Dorst 
> 
> Reviewed-by: Andrew Lunn 
> 
> Andrew
> 

Re: [PATCH net-next v8 2/3] net: phy: add support for clause 37 auto-negotiation

[Tao Ren]

On 9/14/19 7:17 AM, Andrew Lunn wrote:
> On Mon, Sep 09, 2019 at 01:49:06PM -0700, Tao Ren wrote:
>> From: Heiner Kallweit 
>>
>> This patch adds support for clause 37 1000Base-X auto-negotiation.
>>
>> Signed-off-by: Heiner Kallweit 
>> Signed-off-by: Tao Ren 
>> Tested-by: René van Dorst 
> 
> Reviewed-by: Andrew Lunn 
> 
> Andrew

Thanks a lot, Andrew.


Cheers,

Tao

Re: [PATCH net-next v8 2/3] net: phy: add support for clause 37 auto-negotiation

[Tao Ren]

Hi Andrew / Florian,

On 9/9/19 1:49 PM, Tao Ren wrote:
> From: Heiner Kallweit 
> 
> This patch adds support for clause 37 1000Base-X auto-negotiation.
> 
> Signed-off-by: Heiner Kallweit 
> Signed-off-by: Tao Ren 
> Tested-by: René van Dorst 

Just want to check if I missed your comments (I noticed some wired problems on 
my email client)?

If yes (sorry I missed something), could you please re-send your comments? And 
I will address them as soon as possible.


Thanks,

Tao

> ---
>  Changes in v8:
>   - Rebased the patch on top of net-next HEAD.
>  Changes in v7:
>   - Update "if (AUTONEG_ENABLE != phydev->autoneg)" to
> "if (phydev->autoneg != AUTONEG_ENABLE)" so checkpatch.pl is happy.
>  Changes in v6:
>   - add "Signed-off-by: Tao Ren "
>  Changes in v1-v5:
>   - nothing changed. It's given v5 just to align with the version of
> patch series.
> 
>  drivers/net/phy/phy_device.c | 139 +++
>  include/linux/phy.h  |   4 +
>  2 files changed, 143 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 8933f07d39e9..dd05f750bb3f 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1607,6 +1607,40 @@ static int genphy_config_advert(struct phy_device 
> *phydev)
>   return changed;
>  }
>  
> +/**
> + * genphy_c37_config_advert - sanitize and advertise auto-negotiation 
> parameters
> + * @phydev: target phy_device struct
> + *
> + * Description: Writes MII_ADVERTISE with the appropriate values,
> + *   after sanitizing the values to make sure we only advertise
> + *   what is supported.  Returns < 0 on error, 0 if the PHY's advertisement
> + *   hasn't changed, and > 0 if it has changed. This function is intended
> + *   for Clause 37 1000Base-X mode.
> + */
> +static int genphy_c37_config_advert(struct phy_device *phydev)
> +{
> + u16 adv = 0;
> +
> + /* Only allow advertising what this PHY supports */
> + linkmode_and(phydev->advertising, phydev->advertising,
> +  phydev->supported);
> +
> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> +   phydev->advertising))
> + adv |= ADVERTISE_1000XFULL;
> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +   phydev->advertising))
> + adv |= ADVERTISE_1000XPAUSE;
> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +   phydev->advertising))
> + adv |= ADVERTISE_1000XPSE_ASYM;
> +
> + return phy_modify_changed(phydev, MII_ADVERTISE,
> +   ADVERTISE_1000XFULL | ADVERTISE_1000XPAUSE |
> +   ADVERTISE_1000XHALF | ADVERTISE_1000XPSE_ASYM,
> +   adv);
> +}
> +
>  /**
>   * genphy_config_eee_advert - disable unwanted eee mode advertisement
>   * @phydev: target phy_device struct
> @@ -1715,6 +1749,54 @@ int __genphy_config_aneg(struct phy_device *phydev, 
> bool changed)
>  }
>  EXPORT_SYMBOL(__genphy_config_aneg);
>  
> +/**
> + * genphy_c37_config_aneg - restart auto-negotiation or write BMCR
> + * @phydev: target phy_device struct
> + *
> + * Description: If auto-negotiation is enabled, we configure the
> + *   advertising, and then restart auto-negotiation.  If it is not
> + *   enabled, then we write the BMCR. This function is intended
> + *   for use with Clause 37 1000Base-X mode.
> + */
> +int genphy_c37_config_aneg(struct phy_device *phydev)
> +{
> + int err, changed;
> +
> + if (phydev->autoneg != AUTONEG_ENABLE)
> + return genphy_setup_forced(phydev);
> +
> + err = phy_modify(phydev, MII_BMCR, BMCR_SPEED1000 | BMCR_SPEED100,
> +  BMCR_SPEED1000);
> + if (err)
> + return err;
> +
> + changed = genphy_c37_config_advert(phydev);
> + if (changed < 0) /* error */
> + return changed;
> +
> + if (!changed) {
> + /* Advertisement hasn't changed, but maybe aneg was never on to
> +  * begin with?  Or maybe phy was isolated?
> +  */
> + int ctl = phy_read(phydev, MII_BMCR);
> +
> + if (ctl < 0)
> + return ctl;
> +
> + if (!(ctl & BMCR_ANENABLE) || (ctl & BMCR_ISOLATE))
> + changed = 1; /* do restart aneg */
> + }
> +
> + /* Only restart aneg if we are advertising something different
> +  * than we were before.
> +  */
> + if (changed > 0)
&g

[PATCH net-next v8 2/3] net: phy: add support for clause 37 auto-negotiation

[Tao Ren]

From: Heiner Kallweit 

This patch adds support for clause 37 1000Base-X auto-negotiation.

Signed-off-by: Heiner Kallweit 
Signed-off-by: Tao Ren 
Tested-by: René van Dorst 
---
 Changes in v8:
  - Rebased the patch on top of net-next HEAD.
 Changes in v7:
  - Update "if (AUTONEG_ENABLE != phydev->autoneg)" to
"if (phydev->autoneg != AUTONEG_ENABLE)" so checkpatch.pl is happy.
 Changes in v6:
  - add "Signed-off-by: Tao Ren "
 Changes in v1-v5:
  - nothing changed. It's given v5 just to align with the version of
patch series.

 drivers/net/phy/phy_device.c | 139 +++
 include/linux/phy.h  |   4 +
 2 files changed, 143 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8933f07d39e9..dd05f750bb3f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1607,6 +1607,40 @@ static int genphy_config_advert(struct phy_device 
*phydev)
return changed;
 }
 
+/**
+ * genphy_c37_config_advert - sanitize and advertise auto-negotiation 
parameters
+ * @phydev: target phy_device struct
+ *
+ * Description: Writes MII_ADVERTISE with the appropriate values,
+ *   after sanitizing the values to make sure we only advertise
+ *   what is supported.  Returns < 0 on error, 0 if the PHY's advertisement
+ *   hasn't changed, and > 0 if it has changed. This function is intended
+ *   for Clause 37 1000Base-X mode.
+ */
+static int genphy_c37_config_advert(struct phy_device *phydev)
+{
+   u16 adv = 0;
+
+   /* Only allow advertising what this PHY supports */
+   linkmode_and(phydev->advertising, phydev->advertising,
+phydev->supported);
+
+   if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+ phydev->advertising))
+   adv |= ADVERTISE_1000XFULL;
+   if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+ phydev->advertising))
+   adv |= ADVERTISE_1000XPAUSE;
+   if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+ phydev->advertising))
+   adv |= ADVERTISE_1000XPSE_ASYM;
+
+   return phy_modify_changed(phydev, MII_ADVERTISE,
+ ADVERTISE_1000XFULL | ADVERTISE_1000XPAUSE |
+ ADVERTISE_1000XHALF | ADVERTISE_1000XPSE_ASYM,
+ adv);
+}
+
 /**
  * genphy_config_eee_advert - disable unwanted eee mode advertisement
  * @phydev: target phy_device struct
@@ -1715,6 +1749,54 @@ int __genphy_config_aneg(struct phy_device *phydev, bool 
changed)
 }
 EXPORT_SYMBOL(__genphy_config_aneg);
 
+/**
+ * genphy_c37_config_aneg - restart auto-negotiation or write BMCR
+ * @phydev: target phy_device struct
+ *
+ * Description: If auto-negotiation is enabled, we configure the
+ *   advertising, and then restart auto-negotiation.  If it is not
+ *   enabled, then we write the BMCR. This function is intended
+ *   for use with Clause 37 1000Base-X mode.
+ */
+int genphy_c37_config_aneg(struct phy_device *phydev)
+{
+   int err, changed;
+
+   if (phydev->autoneg != AUTONEG_ENABLE)
+   return genphy_setup_forced(phydev);
+
+   err = phy_modify(phydev, MII_BMCR, BMCR_SPEED1000 | BMCR_SPEED100,
+BMCR_SPEED1000);
+   if (err)
+   return err;
+
+   changed = genphy_c37_config_advert(phydev);
+   if (changed < 0) /* error */
+   return changed;
+
+   if (!changed) {
+   /* Advertisement hasn't changed, but maybe aneg was never on to
+* begin with?  Or maybe phy was isolated?
+*/
+   int ctl = phy_read(phydev, MII_BMCR);
+
+   if (ctl < 0)
+   return ctl;
+
+   if (!(ctl & BMCR_ANENABLE) || (ctl & BMCR_ISOLATE))
+   changed = 1; /* do restart aneg */
+   }
+
+   /* Only restart aneg if we are advertising something different
+* than we were before.
+*/
+   if (changed > 0)
+   return genphy_restart_aneg(phydev);
+
+   return 0;
+}
+EXPORT_SYMBOL(genphy_c37_config_aneg);
+
 /**
  * genphy_aneg_done - return auto-negotiation status
  * @phydev: target phy_device struct
@@ -1862,6 +1944,63 @@ int genphy_read_status(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(genphy_read_status);
 
+/**
+ * genphy_c37_read_status - check the link status and update current link state
+ * @phydev: target phy_device struct
+ *
+ * Description: Check the link, then figure out the current state
+ *   by comparing what we advertise with what the link partner
+ *   advertises. This function is for Clause 37 1000Base-X mode.
+ */
+int genphy_c37_read_status(struct phy_device *phydev)
+{
+   int lpa, err, old_link = phydev->link;
+
+   /* Update th

[PATCH net-next v8 1/3] net: phy: modify assignment to OR for dev_flags in phy_attach_direct

[Tao Ren]

Modify the assignment to OR when dealing with phydev->dev_flags in
phy_attach_direct function, and this is to make sure dev_flags set in
driver's probe callback won't be lost.

Suggested-by: Andrew Lunn 
CC: Heiner Kallweit 
CC: Vladimir Oltean 
Signed-off-by: Tao Ren 
Reviewed-by: Andrew Lunn 
Reviewed-by: Florian Fainelli 
---
 Changes:
  - nothing is changed in v1-v7: it's given v8 to align with the version
of patch series.

 drivers/net/phy/phy_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d347ddcac45b..8933f07d39e9 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1270,7 +1270,7 @@ int phy_attach_direct(struct net_device *dev, struct 
phy_device *phydev,
phydev_err(phydev, "error creating 'phy_standalone' 
sysfs entry\n");
}
 
-   phydev->dev_flags = flags;
+   phydev->dev_flags |= flags;
 
phydev->interface = interface;
 
-- 
2.17.1

[PATCH net-next v8 3/3] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

The BCM54616S PHY cannot work properly in RGMII->1000Base-X mode, mainly
because genphy functions are designed for copper links, and 1000Base-X
(clause 37) auto negotiation needs to be handled differently.

This patch enables 1000Base-X support for BCM54616S by customizing 3
driver callbacks, and it's verified to be working on Facebook CMM BMC
platform (RGMII->1000Base-KX):

  - probe: probe callback detects PHY's operation mode based on
INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
Control register.

  - config_aneg: calls genphy_c37_config_aneg when the PHY is running in
1000Base-X mode; otherwise, genphy_config_aneg will be called.

  - read_status: calls genphy_c37_read_status when the PHY is running in
1000Base-X mode; otherwise, genphy_read_status will be called.

Note: BCM54616S PHY can also be configured in RGMII->100Base-FX mode, and
100Base-FX support is not available as of now.

Signed-off-by: Tao Ren 
Acked-by: Vladimir Oltean 
Reviewed-by: Florian Fainelli 
---
 No changes in v8.
 Changes in v7:
  - Add comment "BCM54616S 100Base-FX is not supported".
 Changes in v6:
  - nothing changed.
 Changes in v5:
  - include Heiner's patch "net: phy: add support for clause 37
auto-negotiation" into the series.
  - use genphy_c37_config_aneg and genphy_c37_read_status in BCM54616S
PHY driver's callback when the PHY is running in 1000Base-X mode.
 Changes in v4:
  - add bcm54616s_config_aneg_1000bx() to deal with auto negotiation in
1000Base-X mode.
 Changes in v3:
  - rename bcm5482_read_status to bcm54xx_read_status so the callback can
be shared by BCM5482 and BCM54616S.
 Changes in v2:
  - Auto-detect PHY operation mode instead of passing DT node.
  - move PHY mode auto-detect logic from config_init to probe callback.
  - only set speed (not including duplex) in read_status callback.
  - update patch description with more background to avoid confusion.
  - patch #1 in the series ("net: phy: broadcom: set features explicitly
for BCM54616") is dropped.

 drivers/net/phy/broadcom.c | 57 +++---
 include/linux/brcmphy.h| 10 +--
 2 files changed, 61 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 937d0059e8ac..5fd9293513d8 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device *phydev)
/*
 * Select 1000BASE-X register set (primary SerDes)
 */
-   reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
-   bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
-reg | BCM5482_SHD_MODE_1000BX);
+   reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
+   bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
+reg | BCM54XX_SHD_MODE_1000BX);
 
/*
 * LED1=ACTIVITYLED, LED3=LINKSPD[2]
@@ -451,12 +451,47 @@ static int bcm5481_config_aneg(struct phy_device *phydev)
return ret;
 }
 
+static int bcm54616s_probe(struct phy_device *phydev)
+{
+   int val, intf_sel;
+
+   val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
+   if (val < 0)
+   return val;
+
+   /* The PHY is strapped in RGMII-fiber mode when INTERF_SEL[1:0]
+* is 01b, and the link between PHY and its link partner can be
+* either 1000Base-X or 100Base-FX.
+* RGMII-1000Base-X is properly supported, but RGMII-100Base-FX
+* support is still missing as of now.
+*/
+   intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1;
+   if (intf_sel == 1) {
+   val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL);
+   if (val < 0)
+   return val;
+
+   /* Bit 0 of the SerDes 100-FX Control register, when set
+* to 1, sets the MII/RGMII -> 100BASE-FX configuration.
+* When this bit is set to 0, it sets the GMII/RGMII ->
+* 1000BASE-X configuration.
+*/
+   if (!(val & BCM54616S_100FX_MODE))
+   phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX;
+   }
+
+   return 0;
+}
+
 static int bcm54616s_config_aneg(struct phy_device *phydev)
 {
int ret;
 
/* Aneg firsly. */
-   ret = genphy_config_aneg(phydev);
+   if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX)
+   ret = genphy_c37_config_aneg(phydev);
+   else
+   ret = genphy_config_aneg(phydev);
 
/* Then we can set up the delay. */
bcm54xx_config_clock_delay(phydev);
@@ -464,6 +499,18 @@ static int bcm54616s_config_aneg(struct phy_device *phydev)
return ret;
 }
 
+static int bcm54616s_read_status(str

Re: [PATCH net-next v7 2/3] net: phy: add support for clause 37 auto-negotiation

[Tao Ren]

On 8/19/19 4:15 PM, René van Dorst wrote:
> Hi Tao,
> 
> Quoting Tao Ren :
> 
>> On 8/11/19 4:40 PM, Tao Ren wrote:
>>> From: Heiner Kallweit 
>>>
>>> This patch adds support for clause 37 1000Base-X auto-negotiation.
>>>
>>> Signed-off-by: Heiner Kallweit 
>>> Signed-off-by: Tao Ren 
>>
>> A kind reminder: could someone help to review the patch when you have 
>> bandwidth?
>>
> 
> FWIW: I have a similar setup with my device. MAC -> PHY -> SFP cage.
> PHY is a Qualcomm at8031 and is used as a RGMII-to-SerDes converter.
> SerDes only support 100Base-FX and 1000Base-X in this converter mode.
> PHY also supports a RJ45 port but that is not wired on my device.
> 
> I converted [0] at803x driver to make use of the PHYLINK API for SFP cage and
> also of these new c37 functions.
> 
> In autoneg on and off, it detects the link and can ping a host on the network.
> Tested with 1gbit BiDi optical(1000Base-X) and RJ45 module(SGMII).
> Both work and both devices detects unplug and plug-in of the cable.
> 
> output of ethtool:
> 
> Autoneg on
> Settings for lan5:
>     Supported ports: [ TP MII ]
>     Supported link modes:   100baseT/Half 100baseT/Full
>     1000baseT/Full
>     1000baseX/Full
>     Supported pause frame use: Symmetric Receive-only
>     Supports auto-negotiation: Yes
>     Supported FEC modes: Not reported
>     Advertised link modes:  100baseT/Half 100baseT/Full
>     1000baseT/Full
>     1000baseX/Full
>     Advertised pause frame use: Symmetric Receive-only
>     Advertised auto-negotiation: Yes
>     Advertised FEC modes: Not reported
>     Link partner advertised link modes:  1000baseX/Full
>     Link partner advertised pause frame use: Symmetric Receive-only
>     Link partner advertised auto-negotiation: Yes
>     Link partner advertised FEC modes: Not reported
>     Speed: 1000Mb/s
>     Duplex: Full
>     Port: MII
>     PHYAD: 7
>     Transceiver: internal
>     Auto-negotiation: on
>     Supports Wake-on: g
>     Wake-on: d
>     Link detected: yes
> 
> Autoneg off
> Settings for lan5:
>     Supported ports: [ TP MII ]
>     Supported link modes:   100baseT/Half 100baseT/Full
>     1000baseT/Full
>     1000baseX/Full
>     Supported pause frame use: Symmetric Receive-only
>     Supports auto-negotiation: Yes
>     Supported FEC modes: Not reported
>     Advertised link modes:  1000baseT/Full
>     Advertised pause frame use: Symmetric Receive-only
>     Advertised auto-negotiation: No
>     Advertised FEC modes: Not reported
>     Speed: 1000Mb/s
>     Duplex: Full
>     Port: MII
>     PHYAD: 7
>     Transceiver: internal
>     Auto-negotiation: off
>     Supports Wake-on: g
>     Wake-on: d
>     Link detected: yes
> 
> Tested-by: René van Dorst 
> 
> Greats,
> 
> René

Great. Thank you for the testing, René.


Cheers,

Tao

Re: [PATCH net-next v7 2/3] net: phy: add support for clause 37 auto-negotiation

[Tao Ren]

On 8/11/19 4:40 PM, Tao Ren wrote:
> From: Heiner Kallweit 
> 
> This patch adds support for clause 37 1000Base-X auto-negotiation.
> 
> Signed-off-by: Heiner Kallweit 
> Signed-off-by: Tao Ren 

A kind reminder: could someone help to review the patch when you have bandwidth?


Cheers,

Tao

> ---
>  Changes in v7:
>   - Update "if (AUTONEG_ENABLE != phydev->autoneg)" to
> "if (phydev->autoneg != AUTONEG_ENABLE)" so checkpatch.pl is happy.
>  Changes in v6:
>   - add "Signed-off-by: Tao Ren "
>  Changes in v1-v5:
>   - nothing changed. It's given v5 just to align with the version of
> patch series.
> 
>  drivers/net/phy/phy_device.c | 139 +++
>  include/linux/phy.h  |   5 ++
>  2 files changed, 144 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 252a712d1b2b..301a794b2963 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1617,6 +1617,40 @@ static int genphy_config_advert(struct phy_device 
> *phydev)
>   return changed;
>  }
>  
> +/**
> + * genphy_c37_config_advert - sanitize and advertise auto-negotiation 
> parameters
> + * @phydev: target phy_device struct
> + *
> + * Description: Writes MII_ADVERTISE with the appropriate values,
> + *   after sanitizing the values to make sure we only advertise
> + *   what is supported.  Returns < 0 on error, 0 if the PHY's advertisement
> + *   hasn't changed, and > 0 if it has changed. This function is intended
> + *   for Clause 37 1000Base-X mode.
> + */
> +static int genphy_c37_config_advert(struct phy_device *phydev)
> +{
> + u16 adv = 0;
> +
> + /* Only allow advertising what this PHY supports */
> + linkmode_and(phydev->advertising, phydev->advertising,
> +  phydev->supported);
> +
> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> +   phydev->advertising))
> + adv |= ADVERTISE_1000XFULL;
> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +   phydev->advertising))
> + adv |= ADVERTISE_1000XPAUSE;
> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +   phydev->advertising))
> + adv |= ADVERTISE_1000XPSE_ASYM;
> +
> + return phy_modify_changed(phydev, MII_ADVERTISE,
> +   ADVERTISE_1000XFULL | ADVERTISE_1000XPAUSE |
> +   ADVERTISE_1000XHALF | ADVERTISE_1000XPSE_ASYM,
> +   adv);
> +}
> +
>  /**
>   * genphy_config_eee_advert - disable unwanted eee mode advertisement
>   * @phydev: target phy_device struct
> @@ -1726,6 +1760,54 @@ int genphy_config_aneg(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL(genphy_config_aneg);
>  
> +/**
> + * genphy_c37_config_aneg - restart auto-negotiation or write BMCR
> + * @phydev: target phy_device struct
> + *
> + * Description: If auto-negotiation is enabled, we configure the
> + *   advertising, and then restart auto-negotiation.  If it is not
> + *   enabled, then we write the BMCR. This function is intended
> + *   for use with Clause 37 1000Base-X mode.
> + */
> +int genphy_c37_config_aneg(struct phy_device *phydev)
> +{
> + int err, changed;
> +
> + if (phydev->autoneg != AUTONEG_ENABLE)
> + return genphy_setup_forced(phydev);
> +
> + err = phy_modify(phydev, MII_BMCR, BMCR_SPEED1000 | BMCR_SPEED100,
> +  BMCR_SPEED1000);
> + if (err)
> + return err;
> +
> + changed = genphy_c37_config_advert(phydev);
> + if (changed < 0) /* error */
> + return changed;
> +
> + if (!changed) {
> + /* Advertisement hasn't changed, but maybe aneg was never on to
> +  * begin with?  Or maybe phy was isolated?
> +  */
> + int ctl = phy_read(phydev, MII_BMCR);
> +
> + if (ctl < 0)
> + return ctl;
> +
> + if (!(ctl & BMCR_ANENABLE) || (ctl & BMCR_ISOLATE))
> + changed = 1; /* do restart aneg */
> + }
> +
> + /* Only restart aneg if we are advertising something different
> +  * than we were before.
> +  */
> + if (changed > 0)
> + return genphy_restart_aneg(phydev);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(genphy_c37_config_aneg);
> +
>  /**
>   * genphy_aneg_done - return auto-negotiation status
>   * @phydev: target phy_device struct
> @@ -1864,6 +1946,63 @@ int

Re: [PATCH net-next v7 3/3] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 8/15/19 4:36 PM, Florian Fainelli wrote:
> On 8/11/19 4:40 PM, Tao Ren wrote:
>> The BCM54616S PHY cannot work properly in RGMII->1000Base-X mode, mainly
>> because genphy functions are designed for copper links, and 1000Base-X
>> (clause 37) auto negotiation needs to be handled differently.
>>
>> This patch enables 1000Base-X support for BCM54616S by customizing 3
>> driver callbacks, and it's verified to be working on Facebook CMM BMC
>> platform (RGMII->1000Base-KX):
>>
>>   - probe: probe callback detects PHY's operation mode based on
>> INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>> Control register.
>>
>>   - config_aneg: calls genphy_c37_config_aneg when the PHY is running in
>> 1000Base-X mode; otherwise, genphy_config_aneg will be called.
>>
>>   - read_status: calls genphy_c37_read_status when the PHY is running in
>> 1000Base-X mode; otherwise, genphy_read_status will be called.
>>
>> Note: BCM54616S PHY can also be configured in RGMII->100Base-FX mode, and
>> 100Base-FX support is not available as of now.
>>
>> Signed-off-by: Tao Ren 
> 
>> -reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
>> -bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
>> - reg | BCM5482_SHD_MODE_1000BX);
>> +reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
>> +bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
>> + reg | BCM54XX_SHD_MODE_1000BX);
> 
> This could have been a separate patch, but this looks reasonable to me
> and this is correct with the datasheet, thanks Tao.
> 
> Reviewed-by: Florian Fainelli 

Thank you FLorian.

It is great experience working with you all to figure out root cause and work 
out the patches, and I really appreciate it.


Cheers,

Tao

Re: [PATCH net-next v7 3/3] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 8/15/19 4:15 PM, Vladimir Oltean wrote:
> Hi Tao,
> 
> On Fri, 16 Aug 2019 at 02:13, Tao Ren  wrote:
>>
>> Hi Andrew / Florian / Heiner / Vladimir,
>>
>> Any further suggestions on the patch series?
>>
>>
>> Thanks,
>>
>> Tao
>>
>> On 8/11/19 4:40 PM, Tao Ren wrote:
>>> The BCM54616S PHY cannot work properly in RGMII->1000Base-X mode, mainly
>>> because genphy functions are designed for copper links, and 1000Base-X
>>> (clause 37) auto negotiation needs to be handled differently.
>>>
>>> This patch enables 1000Base-X support for BCM54616S by customizing 3
>>> driver callbacks, and it's verified to be working on Facebook CMM BMC
>>> platform (RGMII->1000Base-KX):
>>>
>>>   - probe: probe callback detects PHY's operation mode based on
>>> INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>>> Control register.
>>>
>>>   - config_aneg: calls genphy_c37_config_aneg when the PHY is running in
>>> 1000Base-X mode; otherwise, genphy_config_aneg will be called.
>>>
>>>   - read_status: calls genphy_c37_read_status when the PHY is running in
>>> 1000Base-X mode; otherwise, genphy_read_status will be called.
>>>
>>> Note: BCM54616S PHY can also be configured in RGMII->100Base-FX mode, and
>>> 100Base-FX support is not available as of now.
>>>
>>> Signed-off-by: Tao Ren 
>>> ---
> 
> The patchset looks good to me. However I am not a maintainer.
> If it helps,
> 
> Acked-by: Vladimir Oltean 

Thank you Vladimir!


Cheers,

Tao

Re: [PATCH net-next v7 3/3] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

Hi Andrew / Florian / Heiner / Vladimir,

Any further suggestions on the patch series?


Thanks,

Tao

On 8/11/19 4:40 PM, Tao Ren wrote:
> The BCM54616S PHY cannot work properly in RGMII->1000Base-X mode, mainly
> because genphy functions are designed for copper links, and 1000Base-X
> (clause 37) auto negotiation needs to be handled differently.
> 
> This patch enables 1000Base-X support for BCM54616S by customizing 3
> driver callbacks, and it's verified to be working on Facebook CMM BMC
> platform (RGMII->1000Base-KX):
> 
>   - probe: probe callback detects PHY's operation mode based on
> INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
> Control register.
> 
>   - config_aneg: calls genphy_c37_config_aneg when the PHY is running in
> 1000Base-X mode; otherwise, genphy_config_aneg will be called.
> 
>   - read_status: calls genphy_c37_read_status when the PHY is running in
> 1000Base-X mode; otherwise, genphy_read_status will be called.
> 
> Note: BCM54616S PHY can also be configured in RGMII->100Base-FX mode, and
> 100Base-FX support is not available as of now.
> 
> Signed-off-by: Tao Ren 
> ---
>  Changes in v7:
>   - Add comment "BCM54616S 100Base-FX is not supported".
>  Changes in v6:
>   - nothing changed.
>  Changes in v5:
>   - include Heiner's patch "net: phy: add support for clause 37
> auto-negotiation" into the series.
>   - use genphy_c37_config_aneg and genphy_c37_read_status in BCM54616S
> PHY driver's callback when the PHY is running in 1000Base-X mode.
>  Changes in v4:
>   - add bcm54616s_config_aneg_1000bx() to deal with auto negotiation in
> 1000Base-X mode.
>  Changes in v3:
>   - rename bcm5482_read_status to bcm54xx_read_status so the callback can
> be shared by BCM5482 and BCM54616S.
>  Changes in v2:
>   - Auto-detect PHY operation mode instead of passing DT node.
>   - move PHY mode auto-detect logic from config_init to probe callback.
>   - only set speed (not including duplex) in read_status callback.
>   - update patch description with more background to avoid confusion.
>   - patch #1 in the series ("net: phy: broadcom: set features explicitly
> for BCM54616") is dropped.
> 
>  drivers/net/phy/broadcom.c | 57 +++---
>  include/linux/brcmphy.h| 10 +--
>  2 files changed, 61 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
> index 937d0059e8ac..5fd9293513d8 100644
> --- a/drivers/net/phy/broadcom.c
> +++ b/drivers/net/phy/broadcom.c
> @@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device *phydev)
>   /*
>* Select 1000BASE-X register set (primary SerDes)
>*/
> - reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
> - bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
> -  reg | BCM5482_SHD_MODE_1000BX);
> + reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
> + bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
> +  reg | BCM54XX_SHD_MODE_1000BX);
>  
>   /*
>* LED1=ACTIVITYLED, LED3=LINKSPD[2]
> @@ -451,12 +451,47 @@ static int bcm5481_config_aneg(struct phy_device 
> *phydev)
>   return ret;
>  }
>  
> +static int bcm54616s_probe(struct phy_device *phydev)
> +{
> + int val, intf_sel;
> +
> + val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
> + if (val < 0)
> + return val;
> +
> + /* The PHY is strapped in RGMII-fiber mode when INTERF_SEL[1:0]
> +  * is 01b, and the link between PHY and its link partner can be
> +  * either 1000Base-X or 100Base-FX.
> +  * RGMII-1000Base-X is properly supported, but RGMII-100Base-FX
> +  * support is still missing as of now.
> +  */
> + intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1;
> + if (intf_sel == 1) {
> + val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL);
> + if (val < 0)
> + return val;
> +
> + /* Bit 0 of the SerDes 100-FX Control register, when set
> +  * to 1, sets the MII/RGMII -> 100BASE-FX configuration.
> +  * When this bit is set to 0, it sets the GMII/RGMII ->
> +  * 1000BASE-X configuration.
> +  */
> + if (!(val & BCM54616S_100FX_MODE))
> + phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX;
> + }
> +
> + return 0;
> +}
> +
>  static int bcm54616s_config_aneg(struct phy_device *phydev)
>  {
&

[PATCH net-next v7 2/3] net: phy: add support for clause 37 auto-negotiation

[Tao Ren]

From: Heiner Kallweit 

This patch adds support for clause 37 1000Base-X auto-negotiation.

Signed-off-by: Heiner Kallweit 
Signed-off-by: Tao Ren 
---
 Changes in v7:
  - Update "if (AUTONEG_ENABLE != phydev->autoneg)" to
"if (phydev->autoneg != AUTONEG_ENABLE)" so checkpatch.pl is happy.
 Changes in v6:
  - add "Signed-off-by: Tao Ren "
 Changes in v1-v5:
  - nothing changed. It's given v5 just to align with the version of
patch series.

 drivers/net/phy/phy_device.c | 139 +++
 include/linux/phy.h  |   5 ++
 2 files changed, 144 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 252a712d1b2b..301a794b2963 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1617,6 +1617,40 @@ static int genphy_config_advert(struct phy_device 
*phydev)
return changed;
 }
 
+/**
+ * genphy_c37_config_advert - sanitize and advertise auto-negotiation 
parameters
+ * @phydev: target phy_device struct
+ *
+ * Description: Writes MII_ADVERTISE with the appropriate values,
+ *   after sanitizing the values to make sure we only advertise
+ *   what is supported.  Returns < 0 on error, 0 if the PHY's advertisement
+ *   hasn't changed, and > 0 if it has changed. This function is intended
+ *   for Clause 37 1000Base-X mode.
+ */
+static int genphy_c37_config_advert(struct phy_device *phydev)
+{
+   u16 adv = 0;
+
+   /* Only allow advertising what this PHY supports */
+   linkmode_and(phydev->advertising, phydev->advertising,
+phydev->supported);
+
+   if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+ phydev->advertising))
+   adv |= ADVERTISE_1000XFULL;
+   if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+ phydev->advertising))
+   adv |= ADVERTISE_1000XPAUSE;
+   if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+ phydev->advertising))
+   adv |= ADVERTISE_1000XPSE_ASYM;
+
+   return phy_modify_changed(phydev, MII_ADVERTISE,
+ ADVERTISE_1000XFULL | ADVERTISE_1000XPAUSE |
+ ADVERTISE_1000XHALF | ADVERTISE_1000XPSE_ASYM,
+ adv);
+}
+
 /**
  * genphy_config_eee_advert - disable unwanted eee mode advertisement
  * @phydev: target phy_device struct
@@ -1726,6 +1760,54 @@ int genphy_config_aneg(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(genphy_config_aneg);
 
+/**
+ * genphy_c37_config_aneg - restart auto-negotiation or write BMCR
+ * @phydev: target phy_device struct
+ *
+ * Description: If auto-negotiation is enabled, we configure the
+ *   advertising, and then restart auto-negotiation.  If it is not
+ *   enabled, then we write the BMCR. This function is intended
+ *   for use with Clause 37 1000Base-X mode.
+ */
+int genphy_c37_config_aneg(struct phy_device *phydev)
+{
+   int err, changed;
+
+   if (phydev->autoneg != AUTONEG_ENABLE)
+   return genphy_setup_forced(phydev);
+
+   err = phy_modify(phydev, MII_BMCR, BMCR_SPEED1000 | BMCR_SPEED100,
+BMCR_SPEED1000);
+   if (err)
+   return err;
+
+   changed = genphy_c37_config_advert(phydev);
+   if (changed < 0) /* error */
+   return changed;
+
+   if (!changed) {
+   /* Advertisement hasn't changed, but maybe aneg was never on to
+* begin with?  Or maybe phy was isolated?
+*/
+   int ctl = phy_read(phydev, MII_BMCR);
+
+   if (ctl < 0)
+   return ctl;
+
+   if (!(ctl & BMCR_ANENABLE) || (ctl & BMCR_ISOLATE))
+   changed = 1; /* do restart aneg */
+   }
+
+   /* Only restart aneg if we are advertising something different
+* than we were before.
+*/
+   if (changed > 0)
+   return genphy_restart_aneg(phydev);
+
+   return 0;
+}
+EXPORT_SYMBOL(genphy_c37_config_aneg);
+
 /**
  * genphy_aneg_done - return auto-negotiation status
  * @phydev: target phy_device struct
@@ -1864,6 +1946,63 @@ int genphy_read_status(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(genphy_read_status);
 
+/**
+ * genphy_c37_read_status - check the link status and update current link state
+ * @phydev: target phy_device struct
+ *
+ * Description: Check the link, then figure out the current state
+ *   by comparing what we advertise with what the link partner
+ *   advertises. This function is for Clause 37 1000Base-X mode.
+ */
+int genphy_c37_read_status(struct phy_device *phydev)
+{
+   int lpa, err, old_link = phydev->link;
+
+   /* Update the link, but return if there was an error */
+   err = genphy_update_link(phydev);
+   if (err)
+   

[PATCH net-next v7 1/3] net: phy: modify assignment to OR for dev_flags in phy_attach_direct

[Tao Ren]

Modify the assignment to OR when dealing with phydev->dev_flags in
phy_attach_direct function, and this is to make sure dev_flags set in
driver's probe callback won't be lost.

Suggested-by: Andrew Lunn 
CC: Heiner Kallweit 
CC: Vladimir Oltean 
Signed-off-by: Tao Ren 
Reviewed-by: Andrew Lunn 
---
 Changes:
  - nothing is changed in v1-v6: it's given v7 to align with the version
of patch series.

 drivers/net/phy/phy_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 7ddd91df99e3..252a712d1b2b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1270,7 +1270,7 @@ int phy_attach_direct(struct net_device *dev, struct 
phy_device *phydev,
phydev_err(phydev, "error creating 'phy_standalone' 
sysfs entry\n");
}
 
-   phydev->dev_flags = flags;
+   phydev->dev_flags |= flags;
 
phydev->interface = interface;
 
-- 
2.17.1

[PATCH net-next v7 3/3] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

The BCM54616S PHY cannot work properly in RGMII->1000Base-X mode, mainly
because genphy functions are designed for copper links, and 1000Base-X
(clause 37) auto negotiation needs to be handled differently.

This patch enables 1000Base-X support for BCM54616S by customizing 3
driver callbacks, and it's verified to be working on Facebook CMM BMC
platform (RGMII->1000Base-KX):

  - probe: probe callback detects PHY's operation mode based on
INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
Control register.

  - config_aneg: calls genphy_c37_config_aneg when the PHY is running in
1000Base-X mode; otherwise, genphy_config_aneg will be called.

  - read_status: calls genphy_c37_read_status when the PHY is running in
1000Base-X mode; otherwise, genphy_read_status will be called.

Note: BCM54616S PHY can also be configured in RGMII->100Base-FX mode, and
100Base-FX support is not available as of now.

Signed-off-by: Tao Ren 
---
 Changes in v7:
  - Add comment "BCM54616S 100Base-FX is not supported".
 Changes in v6:
  - nothing changed.
 Changes in v5:
  - include Heiner's patch "net: phy: add support for clause 37
auto-negotiation" into the series.
  - use genphy_c37_config_aneg and genphy_c37_read_status in BCM54616S
PHY driver's callback when the PHY is running in 1000Base-X mode.
 Changes in v4:
  - add bcm54616s_config_aneg_1000bx() to deal with auto negotiation in
1000Base-X mode.
 Changes in v3:
  - rename bcm5482_read_status to bcm54xx_read_status so the callback can
be shared by BCM5482 and BCM54616S.
 Changes in v2:
  - Auto-detect PHY operation mode instead of passing DT node.
  - move PHY mode auto-detect logic from config_init to probe callback.
  - only set speed (not including duplex) in read_status callback.
  - update patch description with more background to avoid confusion.
  - patch #1 in the series ("net: phy: broadcom: set features explicitly
for BCM54616") is dropped.

 drivers/net/phy/broadcom.c | 57 +++---
 include/linux/brcmphy.h| 10 +--
 2 files changed, 61 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 937d0059e8ac..5fd9293513d8 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device *phydev)
/*
 * Select 1000BASE-X register set (primary SerDes)
 */
-   reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
-   bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
-reg | BCM5482_SHD_MODE_1000BX);
+   reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
+   bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
+reg | BCM54XX_SHD_MODE_1000BX);
 
/*
 * LED1=ACTIVITYLED, LED3=LINKSPD[2]
@@ -451,12 +451,47 @@ static int bcm5481_config_aneg(struct phy_device *phydev)
return ret;
 }
 
+static int bcm54616s_probe(struct phy_device *phydev)
+{
+   int val, intf_sel;
+
+   val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
+   if (val < 0)
+   return val;
+
+   /* The PHY is strapped in RGMII-fiber mode when INTERF_SEL[1:0]
+* is 01b, and the link between PHY and its link partner can be
+* either 1000Base-X or 100Base-FX.
+* RGMII-1000Base-X is properly supported, but RGMII-100Base-FX
+* support is still missing as of now.
+*/
+   intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1;
+   if (intf_sel == 1) {
+   val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL);
+   if (val < 0)
+   return val;
+
+   /* Bit 0 of the SerDes 100-FX Control register, when set
+* to 1, sets the MII/RGMII -> 100BASE-FX configuration.
+* When this bit is set to 0, it sets the GMII/RGMII ->
+* 1000BASE-X configuration.
+*/
+   if (!(val & BCM54616S_100FX_MODE))
+   phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX;
+   }
+
+   return 0;
+}
+
 static int bcm54616s_config_aneg(struct phy_device *phydev)
 {
int ret;
 
/* Aneg firsly. */
-   ret = genphy_config_aneg(phydev);
+   if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX)
+   ret = genphy_c37_config_aneg(phydev);
+   else
+   ret = genphy_config_aneg(phydev);
 
/* Then we can set up the delay. */
bcm54xx_config_clock_delay(phydev);
@@ -464,6 +499,18 @@ static int bcm54616s_config_aneg(struct phy_device *phydev)
return ret;
 }
 
+static int bcm54616s_read_status(struct phy_device *phydev)
+{
+   int err;
+
+   if (phydev->dev_flags &a

Re: [Potential Spoof] Re: [PATCH net-next v6 3/3] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 8/9/19 2:59 PM, Heiner Kallweit wrote:
> On 09.08.2019 23:13, Tao Ren wrote:
>> On 8/9/19 1:54 PM, Tao Ren wrote:
>>> Hi Heiner,
>>>
>>> On 8/9/19 1:21 PM, Heiner Kallweit wrote:
>>>> On 09.08.2019 07:44, Tao Ren wrote:
>>>>> The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
>>>>> example, on Facebook CMM BMC platform), mainly because genphy functions
>>>>> are designed for copper links, and 1000Base-X (clause 37) auto negotiation
>>>>> needs to be handled differently.
>>>>>
>>>>> This patch enables 1000Base-X support for BCM54616S by customizing 3
>>>>> driver callbacks:
>>>>>
>>>>>   - probe: probe callback detects PHY's operation mode based on
>>>>> INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>>>>> Control register.
>>>>>
>>>>>   - config_aneg: calls genphy_c37_config_aneg when the PHY is running in
>>>>> 1000Base-X mode; otherwise, genphy_config_aneg will be called.
>>>>>
>>>>>   - read_status: calls genphy_c37_read_status when the PHY is running in
>>>>> 1000Base-X mode; otherwise, genphy_read_status will be called.
>>>>>
>>>>> Signed-off-by: Tao Ren 
>>>>> ---
>>>>>  Changes in v6:
>>>>>   - nothing changed.
>>>>>  Changes in v5:
>>>>>   - include Heiner's patch "net: phy: add support for clause 37
>>>>> auto-negotiation" into the series.
>>>>>   - use genphy_c37_config_aneg and genphy_c37_read_status in BCM54616S
>>>>> PHY driver's callback when the PHY is running in 1000Base-X mode.
>>>>>  Changes in v4:
>>>>>   - add bcm54616s_config_aneg_1000bx() to deal with auto negotiation in
>>>>> 1000Base-X mode.
>>>>>  Changes in v3:
>>>>>   - rename bcm5482_read_status to bcm54xx_read_status so the callback can
>>>>> be shared by BCM5482 and BCM54616S.
>>>>>  Changes in v2:
>>>>>   - Auto-detect PHY operation mode instead of passing DT node.
>>>>>   - move PHY mode auto-detect logic from config_init to probe callback.
>>>>>   - only set speed (not including duplex) in read_status callback.
>>>>>   - update patch description with more background to avoid confusion.
>>>>>   - patch #1 in the series ("net: phy: broadcom: set features explicitly
>>>>> for BCM54616") is dropped: the fix should go to get_features callback
>>>>> which may potentially depend on this patch.
>>>>>
>>>>>  drivers/net/phy/broadcom.c | 54 +++---
>>>>>  include/linux/brcmphy.h| 10 +--
>>>>>  2 files changed, 58 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
>>>>> index 937d0059e8ac..fbd76a31c142 100644
>>>>> --- a/drivers/net/phy/broadcom.c
>>>>> +++ b/drivers/net/phy/broadcom.c
>>>>> @@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device 
>>>>> *phydev)
>>>>>   /*
>>>>>* Select 1000BASE-X register set (primary SerDes)
>>>>>*/
>>>>> - reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
>>>>> - bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
>>>>> -  reg | BCM5482_SHD_MODE_1000BX);
>>>>> + reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
>>>>> + bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
>>>>> +  reg | BCM54XX_SHD_MODE_1000BX);
>>>>>  
>>>>>   /*
>>>>>* LED1=ACTIVITYLED, LED3=LINKSPD[2]
>>>>> @@ -451,12 +451,44 @@ static int bcm5481_config_aneg(struct phy_device 
>>>>> *phydev)
>>>>>   return ret;
>>>>>  }
>>>>>  
>>>>> +static int bcm54616s_probe(struct phy_device *phydev)
>>>>> +{
>>>>> + int val, intf_sel;
>>>>> +
>>>>> + val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
>>>>> + if (val < 0)
>>>>> + return val;
>>>>> +
>>>>> + /* The PHY is strapped i

Re: [Potential Spoof] Re: [PATCH net-next v6 3/3] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 8/9/19 1:54 PM, Tao Ren wrote:
> Hi Heiner,
> 
> On 8/9/19 1:21 PM, Heiner Kallweit wrote:
>> On 09.08.2019 07:44, Tao Ren wrote:
>>> The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
>>> example, on Facebook CMM BMC platform), mainly because genphy functions
>>> are designed for copper links, and 1000Base-X (clause 37) auto negotiation
>>> needs to be handled differently.
>>>
>>> This patch enables 1000Base-X support for BCM54616S by customizing 3
>>> driver callbacks:
>>>
>>>   - probe: probe callback detects PHY's operation mode based on
>>> INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>>> Control register.
>>>
>>>   - config_aneg: calls genphy_c37_config_aneg when the PHY is running in
>>> 1000Base-X mode; otherwise, genphy_config_aneg will be called.
>>>
>>>   - read_status: calls genphy_c37_read_status when the PHY is running in
>>> 1000Base-X mode; otherwise, genphy_read_status will be called.
>>>
>>> Signed-off-by: Tao Ren 
>>> ---
>>>  Changes in v6:
>>>   - nothing changed.
>>>  Changes in v5:
>>>   - include Heiner's patch "net: phy: add support for clause 37
>>> auto-negotiation" into the series.
>>>   - use genphy_c37_config_aneg and genphy_c37_read_status in BCM54616S
>>> PHY driver's callback when the PHY is running in 1000Base-X mode.
>>>  Changes in v4:
>>>   - add bcm54616s_config_aneg_1000bx() to deal with auto negotiation in
>>> 1000Base-X mode.
>>>  Changes in v3:
>>>   - rename bcm5482_read_status to bcm54xx_read_status so the callback can
>>> be shared by BCM5482 and BCM54616S.
>>>  Changes in v2:
>>>   - Auto-detect PHY operation mode instead of passing DT node.
>>>   - move PHY mode auto-detect logic from config_init to probe callback.
>>>   - only set speed (not including duplex) in read_status callback.
>>>   - update patch description with more background to avoid confusion.
>>>   - patch #1 in the series ("net: phy: broadcom: set features explicitly
>>> for BCM54616") is dropped: the fix should go to get_features callback
>>> which may potentially depend on this patch.
>>>
>>>  drivers/net/phy/broadcom.c | 54 +++---
>>>  include/linux/brcmphy.h| 10 +--
>>>  2 files changed, 58 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
>>> index 937d0059e8ac..fbd76a31c142 100644
>>> --- a/drivers/net/phy/broadcom.c
>>> +++ b/drivers/net/phy/broadcom.c
>>> @@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device 
>>> *phydev)
>>> /*
>>>  * Select 1000BASE-X register set (primary SerDes)
>>>  */
>>> -   reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
>>> -   bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
>>> -reg | BCM5482_SHD_MODE_1000BX);
>>> +   reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
>>> +   bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
>>> +reg | BCM54XX_SHD_MODE_1000BX);
>>>  
>>> /*
>>>  * LED1=ACTIVITYLED, LED3=LINKSPD[2]
>>> @@ -451,12 +451,44 @@ static int bcm5481_config_aneg(struct phy_device 
>>> *phydev)
>>> return ret;
>>>  }
>>>  
>>> +static int bcm54616s_probe(struct phy_device *phydev)
>>> +{
>>> +   int val, intf_sel;
>>> +
>>> +   val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
>>> +   if (val < 0)
>>> +   return val;
>>> +
>>> +   /* The PHY is strapped in RGMII to fiber mode when INTERF_SEL[1:0]
>>> +* is 01b.
>>> +*/
>>> +   intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1;
>>> +   if (intf_sel == 1) {
>>> +   val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL);
>>> +   if (val < 0)
>>> +   return val;
>>> +
>>> +   /* Bit 0 of the SerDes 100-FX Control register, when set
>>> +* to 1, sets the MII/RGMII -> 100BASE-FX configuration.
>>> +* When this bit is set to 0, it sets the GMII/RGMII ->
>>> +* 1000BASE-X configuration.
>>>

Re: [PATCH net-next v6 3/3] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

Hi Heiner,

On 8/9/19 1:21 PM, Heiner Kallweit wrote:
> On 09.08.2019 07:44, Tao Ren wrote:
>> The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
>> example, on Facebook CMM BMC platform), mainly because genphy functions
>> are designed for copper links, and 1000Base-X (clause 37) auto negotiation
>> needs to be handled differently.
>>
>> This patch enables 1000Base-X support for BCM54616S by customizing 3
>> driver callbacks:
>>
>>   - probe: probe callback detects PHY's operation mode based on
>> INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>> Control register.
>>
>>   - config_aneg: calls genphy_c37_config_aneg when the PHY is running in
>> 1000Base-X mode; otherwise, genphy_config_aneg will be called.
>>
>>   - read_status: calls genphy_c37_read_status when the PHY is running in
>> 1000Base-X mode; otherwise, genphy_read_status will be called.
>>
>> Signed-off-by: Tao Ren 
>> ---
>>  Changes in v6:
>>   - nothing changed.
>>  Changes in v5:
>>   - include Heiner's patch "net: phy: add support for clause 37
>> auto-negotiation" into the series.
>>   - use genphy_c37_config_aneg and genphy_c37_read_status in BCM54616S
>> PHY driver's callback when the PHY is running in 1000Base-X mode.
>>  Changes in v4:
>>   - add bcm54616s_config_aneg_1000bx() to deal with auto negotiation in
>> 1000Base-X mode.
>>  Changes in v3:
>>   - rename bcm5482_read_status to bcm54xx_read_status so the callback can
>> be shared by BCM5482 and BCM54616S.
>>  Changes in v2:
>>   - Auto-detect PHY operation mode instead of passing DT node.
>>   - move PHY mode auto-detect logic from config_init to probe callback.
>>   - only set speed (not including duplex) in read_status callback.
>>   - update patch description with more background to avoid confusion.
>>   - patch #1 in the series ("net: phy: broadcom: set features explicitly
>> for BCM54616") is dropped: the fix should go to get_features callback
>> which may potentially depend on this patch.
>>
>>  drivers/net/phy/broadcom.c | 54 +++---
>>  include/linux/brcmphy.h| 10 +--
>>  2 files changed, 58 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
>> index 937d0059e8ac..fbd76a31c142 100644
>> --- a/drivers/net/phy/broadcom.c
>> +++ b/drivers/net/phy/broadcom.c
>> @@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device *phydev)
>>  /*
>>   * Select 1000BASE-X register set (primary SerDes)
>>   */
>> -reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
>> -bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
>> - reg | BCM5482_SHD_MODE_1000BX);
>> +reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
>> +bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
>> + reg | BCM54XX_SHD_MODE_1000BX);
>>  
>>  /*
>>   * LED1=ACTIVITYLED, LED3=LINKSPD[2]
>> @@ -451,12 +451,44 @@ static int bcm5481_config_aneg(struct phy_device 
>> *phydev)
>>  return ret;
>>  }
>>  
>> +static int bcm54616s_probe(struct phy_device *phydev)
>> +{
>> +int val, intf_sel;
>> +
>> +val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
>> +if (val < 0)
>> +return val;
>> +
>> +/* The PHY is strapped in RGMII to fiber mode when INTERF_SEL[1:0]
>> + * is 01b.
>> + */
>> +intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1;
>> +if (intf_sel == 1) {
>> +val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL);
>> +if (val < 0)
>> +return val;
>> +
>> +/* Bit 0 of the SerDes 100-FX Control register, when set
>> + * to 1, sets the MII/RGMII -> 100BASE-FX configuration.
>> + * When this bit is set to 0, it sets the GMII/RGMII ->
>> + * 1000BASE-X configuration.
>> + */
>> +if (!(val & BCM54616S_100FX_MODE))
>> +phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX;
>> +}
>> +
>> +return 0;
>> +}
>> +
>>  static int bcm54616s_config_aneg(struct phy_device *phydev)
>>  {
>>  int ret;
>>  
>>  /* Aneg firsly. */
>> -ret 

[PATCH net-next v6 1/3] net: phy: modify assignment to OR for dev_flags in phy_attach_direct

[Tao Ren]

Modify the assignment to OR when dealing with phydev->dev_flags in
phy_attach_direct function, and this is to make sure dev_flags set in
driver's probe callback won't be lost.

Suggested-by: Andrew Lunn 
CC: Heiner Kallweit 
CC: Vladimir Oltean 
Signed-off-by: Tao Ren 
Reviewed-by: Andrew Lunn 
---
 Changes:
  - nothing is changed in v1-v5: it's given v6 to align with the version
of patch series.

 drivers/net/phy/phy_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 7ddd91df99e3..252a712d1b2b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1270,7 +1270,7 @@ int phy_attach_direct(struct net_device *dev, struct 
phy_device *phydev,
phydev_err(phydev, "error creating 'phy_standalone' 
sysfs entry\n");
}
 
-   phydev->dev_flags = flags;
+   phydev->dev_flags |= flags;
 
phydev->interface = interface;
 
-- 
2.17.1

[PATCH net-next v6 2/3] net: phy: add support for clause 37 auto-negotiation

[Tao Ren]

From: Heiner Kallweit 

This patch adds support for clause 37 1000Base-X auto-negotiation.

Signed-off-by: Heiner Kallweit 
Signed-off-by: Tao Ren 
---
 Changes in v6:
  - add "Signed-off-by: Tao Ren "
 Changes in v1-v5:
  - nothing changed. It's given v6 just to align with the version of
patch series.

 drivers/net/phy/phy_device.c | 139 +++
 include/linux/phy.h  |   5 ++
 2 files changed, 144 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 252a712d1b2b..7c5315302937 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1617,6 +1617,40 @@ static int genphy_config_advert(struct phy_device 
*phydev)
return changed;
 }
 
+/**
+ * genphy_c37_config_advert - sanitize and advertise auto-negotiation 
parameters
+ * @phydev: target phy_device struct
+ *
+ * Description: Writes MII_ADVERTISE with the appropriate values,
+ *   after sanitizing the values to make sure we only advertise
+ *   what is supported.  Returns < 0 on error, 0 if the PHY's advertisement
+ *   hasn't changed, and > 0 if it has changed. This function is intended
+ *   for Clause 37 1000Base-X mode.
+ */
+static int genphy_c37_config_advert(struct phy_device *phydev)
+{
+   u16 adv = 0;
+
+   /* Only allow advertising what this PHY supports */
+   linkmode_and(phydev->advertising, phydev->advertising,
+phydev->supported);
+
+   if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+ phydev->advertising))
+   adv |= ADVERTISE_1000XFULL;
+   if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+ phydev->advertising))
+   adv |= ADVERTISE_1000XPAUSE;
+   if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+ phydev->advertising))
+   adv |= ADVERTISE_1000XPSE_ASYM;
+
+   return phy_modify_changed(phydev, MII_ADVERTISE,
+ ADVERTISE_1000XFULL | ADVERTISE_1000XPAUSE |
+ ADVERTISE_1000XHALF | ADVERTISE_1000XPSE_ASYM,
+ adv);
+}
+
 /**
  * genphy_config_eee_advert - disable unwanted eee mode advertisement
  * @phydev: target phy_device struct
@@ -1726,6 +1760,54 @@ int genphy_config_aneg(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(genphy_config_aneg);
 
+/**
+ * genphy_c37_config_aneg - restart auto-negotiation or write BMCR
+ * @phydev: target phy_device struct
+ *
+ * Description: If auto-negotiation is enabled, we configure the
+ *   advertising, and then restart auto-negotiation.  If it is not
+ *   enabled, then we write the BMCR. This function is intended
+ *   for use with Clause 37 1000Base-X mode.
+ */
+int genphy_c37_config_aneg(struct phy_device *phydev)
+{
+   int err, changed;
+
+   if (AUTONEG_ENABLE != phydev->autoneg)
+   return genphy_setup_forced(phydev);
+
+   err = phy_modify(phydev, MII_BMCR, BMCR_SPEED1000 | BMCR_SPEED100,
+BMCR_SPEED1000);
+   if (err)
+   return err;
+
+   changed = genphy_c37_config_advert(phydev);
+   if (changed < 0) /* error */
+   return changed;
+
+   if (!changed) {
+   /* Advertisement hasn't changed, but maybe aneg was never on to
+* begin with?  Or maybe phy was isolated?
+*/
+   int ctl = phy_read(phydev, MII_BMCR);
+
+   if (ctl < 0)
+   return ctl;
+
+   if (!(ctl & BMCR_ANENABLE) || (ctl & BMCR_ISOLATE))
+   changed = 1; /* do restart aneg */
+   }
+
+   /* Only restart aneg if we are advertising something different
+* than we were before.
+*/
+   if (changed > 0)
+   return genphy_restart_aneg(phydev);
+
+   return 0;
+}
+EXPORT_SYMBOL(genphy_c37_config_aneg);
+
 /**
  * genphy_aneg_done - return auto-negotiation status
  * @phydev: target phy_device struct
@@ -1864,6 +1946,63 @@ int genphy_read_status(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(genphy_read_status);
 
+/**
+ * genphy_c37_read_status - check the link status and update current link state
+ * @phydev: target phy_device struct
+ *
+ * Description: Check the link, then figure out the current state
+ *   by comparing what we advertise with what the link partner
+ *   advertises. This function is for Clause 37 1000Base-X mode.
+ */
+int genphy_c37_read_status(struct phy_device *phydev)
+{
+   int lpa, err, old_link = phydev->link;
+
+   /* Update the link, but return if there was an error */
+   err = genphy_update_link(phydev);
+   if (err)
+   return err;
+
+   /* why bother the PHY if nothing can have changed */
+   if (phydev->autoneg == AUTONEG_ENABLE && old_link && phyde

[PATCH net-next v6 3/3] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
example, on Facebook CMM BMC platform), mainly because genphy functions
are designed for copper links, and 1000Base-X (clause 37) auto negotiation
needs to be handled differently.

This patch enables 1000Base-X support for BCM54616S by customizing 3
driver callbacks:

  - probe: probe callback detects PHY's operation mode based on
INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
Control register.

  - config_aneg: calls genphy_c37_config_aneg when the PHY is running in
1000Base-X mode; otherwise, genphy_config_aneg will be called.

  - read_status: calls genphy_c37_read_status when the PHY is running in
1000Base-X mode; otherwise, genphy_read_status will be called.

Signed-off-by: Tao Ren 
---
 Changes in v6:
  - nothing changed.
 Changes in v5:
  - include Heiner's patch "net: phy: add support for clause 37
auto-negotiation" into the series.
  - use genphy_c37_config_aneg and genphy_c37_read_status in BCM54616S
PHY driver's callback when the PHY is running in 1000Base-X mode.
 Changes in v4:
  - add bcm54616s_config_aneg_1000bx() to deal with auto negotiation in
1000Base-X mode.
 Changes in v3:
  - rename bcm5482_read_status to bcm54xx_read_status so the callback can
be shared by BCM5482 and BCM54616S.
 Changes in v2:
  - Auto-detect PHY operation mode instead of passing DT node.
  - move PHY mode auto-detect logic from config_init to probe callback.
  - only set speed (not including duplex) in read_status callback.
  - update patch description with more background to avoid confusion.
  - patch #1 in the series ("net: phy: broadcom: set features explicitly
for BCM54616") is dropped: the fix should go to get_features callback
which may potentially depend on this patch.

 drivers/net/phy/broadcom.c | 54 +++---
 include/linux/brcmphy.h| 10 +--
 2 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 937d0059e8ac..fbd76a31c142 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device *phydev)
/*
 * Select 1000BASE-X register set (primary SerDes)
 */
-   reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
-   bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
-reg | BCM5482_SHD_MODE_1000BX);
+   reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
+   bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
+reg | BCM54XX_SHD_MODE_1000BX);
 
/*
 * LED1=ACTIVITYLED, LED3=LINKSPD[2]
@@ -451,12 +451,44 @@ static int bcm5481_config_aneg(struct phy_device *phydev)
return ret;
 }
 
+static int bcm54616s_probe(struct phy_device *phydev)
+{
+   int val, intf_sel;
+
+   val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
+   if (val < 0)
+   return val;
+
+   /* The PHY is strapped in RGMII to fiber mode when INTERF_SEL[1:0]
+* is 01b.
+*/
+   intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1;
+   if (intf_sel == 1) {
+   val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL);
+   if (val < 0)
+   return val;
+
+   /* Bit 0 of the SerDes 100-FX Control register, when set
+* to 1, sets the MII/RGMII -> 100BASE-FX configuration.
+* When this bit is set to 0, it sets the GMII/RGMII ->
+* 1000BASE-X configuration.
+*/
+   if (!(val & BCM54616S_100FX_MODE))
+   phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX;
+   }
+
+   return 0;
+}
+
 static int bcm54616s_config_aneg(struct phy_device *phydev)
 {
int ret;
 
/* Aneg firsly. */
-   ret = genphy_config_aneg(phydev);
+   if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX)
+   ret = genphy_c37_config_aneg(phydev);
+   else
+   ret = genphy_config_aneg(phydev);
 
/* Then we can set up the delay. */
bcm54xx_config_clock_delay(phydev);
@@ -464,6 +496,18 @@ static int bcm54616s_config_aneg(struct phy_device *phydev)
return ret;
 }
 
+static int bcm54616s_read_status(struct phy_device *phydev)
+{
+   int err;
+
+   if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX)
+   err = genphy_c37_read_status(phydev);
+   else
+   err = genphy_read_status(phydev);
+
+   return err;
+}
+
 static int brcm_phy_setbits(struct phy_device *phydev, int reg, int set)
 {
int val;
@@ -655,6 +699,8 @@ static struct phy_driver broadcom_drivers[] = {
.config_aneg= bcm54616s_config_aneg

Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Tao Ren]

On 8/8/19 4:03 PM, Andrew Lunn wrote:
>> After giving it more thought, I'm thinking about adding ncsi dt node
>> with following structure (mac/ncsi similar to mac/mdio/phy):
>>
>>  {
>> /* MAC properties... */
>>
>> use-ncsi;
> 
> This property seems to be specific to Faraday FTGMAC100. Are you going
> to make it more generic? 

I'm also using ftgmac100 on my platform, and I don't have plan to change this 
property.

>> ncsi {
>> /* ncsi level properties if any */
>>
>> package@0 {
> 
> You should get Rob Herring involved. This is not really describing
> hardware, so it might get rejected by the device tree maintainer.

Got it. Thank you for the sharing, and let me think it over :-)

>> 1) mac driver doesn't need to parse "mac-offset" stuff: these
>> ncsi-network-controller specific settings should be parsed in ncsi
>> stack.
> 
>> 2) get_bmc_mac_address command is a channel specific command, and
>> technically people can configure different offset/formula for
>> different channels.
> 
> Does that mean the NCSA code puts the interface into promiscuous mode?
> Or at least adds these unicast MAC addresses to the MAC receive
> filter? Humm, ftgmac100 only seems to support multicast address
> filtering, not unicast filters, so it must be using promisc mode, if
> you expect to receive frames using this MAC address.

Uhh, I actually didn't think too much about this: basically it's how to 
configure frame filtering when there are multiple packages/channels active: 
single BMC MAC or multiple BMC MAC is also allowed?
I don't have the answer yet, but will talk to NCSI expert and figure it out.


Thanks,

Tao

Re: [PATCH net-next v5 2/3] net: phy: add support for clause 37 auto-negotiation

[Tao Ren]

On 8/8/19 9:58 PM, Vladimir Oltean wrote:
> On Fri, 9 Aug 2019 at 02:48, Tao Ren  wrote:
>>
>> From: Heiner Kallweit 
>>
>> This patch adds support for clause 37 1000Base-X auto-negotiation.
>> It's compile-tested only as I don't have fiber equipment.
>>
>> Signed-off-by: Heiner Kallweit 
>> ---
> 
> This needs your signed-off-by as well.

I see. I didn't understand signed-off-by correctly and removed myself from the 
list explicitly. Adding it back now..


Thanks,

Tao

[PATCH net-next v5 3/3] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
example, on Facebook CMM BMC platform), mainly because genphy functions
are designed for copper links, and 1000Base-X (clause 37) auto negotiation
needs to be handled differently.

This patch enables 1000Base-X support for BCM54616S by customizing 3
driver callbacks:

  - probe: probe callback detects PHY's operation mode based on
INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
Control register.

  - config_aneg: calls genphy_c37_config_aneg when the PHY is running in
1000Base-X mode; otherwise, genphy_config_aneg will be called.

  - read_status: calls genphy_c37_read_status when the PHY is running in
1000Base-X mode; otherwise, genphy_read_status will be called.

Signed-off-by: Tao Ren 
---
 Changes in v5:
  - include Heiner's patch "net: phy: add support for clause 37
auto-negotiation" into the series.
  - use genphy_c37_config_aneg and genphy_c37_read_status in BCM54616S
PHY driver's callback when the PHY is running in 1000Base-X mode.
 Changes in v4:
  - add bcm54616s_config_aneg_1000bx() to deal with auto negotiation in
1000Base-X mode.
 Changes in v3:
  - rename bcm5482_read_status to bcm54xx_read_status so the callback can
be shared by BCM5482 and BCM54616S.
 Changes in v2:
  - Auto-detect PHY operation mode instead of passing DT node.
  - move PHY mode auto-detect logic from config_init to probe callback.
  - only set speed (not including duplex) in read_status callback.
  - update patch description with more background to avoid confusion.
  - patch #1 in the series ("net: phy: broadcom: set features explicitly
for BCM54616") is dropped: the fix should go to get_features callback
which may potentially depend on this patch.

 drivers/net/phy/broadcom.c | 62 ++
 include/linux/brcmphy.h| 10 --
 2 files changed, 64 insertions(+), 8 deletions(-)
 drivers/net/phy/broadcom.c | 54 +++---
 include/linux/brcmphy.h| 10 +--
 2 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 937d0059e8ac..fbd76a31c142 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device *phydev)
/*
 * Select 1000BASE-X register set (primary SerDes)
 */
-   reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
-   bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
-reg | BCM5482_SHD_MODE_1000BX);
+   reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
+   bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
+reg | BCM54XX_SHD_MODE_1000BX);
 
/*
 * LED1=ACTIVITYLED, LED3=LINKSPD[2]
@@ -451,12 +451,44 @@ static int bcm5481_config_aneg(struct phy_device *phydev)
return ret;
 }
 
+static int bcm54616s_probe(struct phy_device *phydev)
+{
+   int val, intf_sel;
+
+   val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
+   if (val < 0)
+   return val;
+
+   /* The PHY is strapped in RGMII to fiber mode when INTERF_SEL[1:0]
+* is 01b.
+*/
+   intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1;
+   if (intf_sel == 1) {
+   val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL);
+   if (val < 0)
+   return val;
+
+   /* Bit 0 of the SerDes 100-FX Control register, when set
+* to 1, sets the MII/RGMII -> 100BASE-FX configuration.
+* When this bit is set to 0, it sets the GMII/RGMII ->
+* 1000BASE-X configuration.
+*/
+   if (!(val & BCM54616S_100FX_MODE))
+   phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX;
+   }
+
+   return 0;
+}
+
 static int bcm54616s_config_aneg(struct phy_device *phydev)
 {
int ret;
 
/* Aneg firsly. */
-   ret = genphy_config_aneg(phydev);
+   if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX)
+   ret = genphy_c37_config_aneg(phydev);
+   else
+   ret = genphy_config_aneg(phydev);
 
/* Then we can set up the delay. */
bcm54xx_config_clock_delay(phydev);
@@ -464,6 +496,18 @@ static int bcm54616s_config_aneg(struct phy_device *phydev)
return ret;
 }
 
+static int bcm54616s_read_status(struct phy_device *phydev)
+{
+   int err;
+
+   if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX)
+   err = genphy_c37_read_status(phydev);
+   else
+   err = genphy_read_status(phydev);
+
+   return err;
+}
+
 static int brcm_phy_setbits(struct phy_device *phydev, int reg, int set

[PATCH net-next v5 1/3] net: phy: modify assignment to OR for dev_flags in phy_attach_direct

[Tao Ren]

Modify the assignment to OR when dealing with phydev->dev_flags in
phy_attach_direct function, and this is to make sure dev_flags set in
driver's probe callback won't be lost.

Suggested-by: Andrew Lunn 
CC: Heiner Kallweit 
CC: Vladimir Oltean 
Signed-off-by: Tao Ren 
Reviewed-by: Andrew Lunn 
---
 Changes:
  - nothing is changed in v1-v4: it's given v5 to align with v5 of the
patch series.

 drivers/net/phy/phy_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 7ddd91df99e3..252a712d1b2b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1270,7 +1270,7 @@ int phy_attach_direct(struct net_device *dev, struct 
phy_device *phydev,
phydev_err(phydev, "error creating 'phy_standalone' 
sysfs entry\n");
}
 
-   phydev->dev_flags = flags;
+   phydev->dev_flags |= flags;
 
phydev->interface = interface;
 
-- 
2.17.1

[PATCH net-next v5 2/3] net: phy: add support for clause 37 auto-negotiation

[Tao Ren]

From: Heiner Kallweit 

This patch adds support for clause 37 1000Base-X auto-negotiation.
It's compile-tested only as I don't have fiber equipment.

Signed-off-by: Heiner Kallweit 
---
 drivers/net/phy/phy_device.c | 139 +++
 include/linux/phy.h  |   5 ++
 2 files changed, 144 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 252a712d1b2b..7c5315302937 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1617,6 +1617,40 @@ static int genphy_config_advert(struct phy_device 
*phydev)
return changed;
 }
 
+/**
+ * genphy_c37_config_advert - sanitize and advertise auto-negotiation 
parameters
+ * @phydev: target phy_device struct
+ *
+ * Description: Writes MII_ADVERTISE with the appropriate values,
+ *   after sanitizing the values to make sure we only advertise
+ *   what is supported.  Returns < 0 on error, 0 if the PHY's advertisement
+ *   hasn't changed, and > 0 if it has changed. This function is intended
+ *   for Clause 37 1000Base-X mode.
+ */
+static int genphy_c37_config_advert(struct phy_device *phydev)
+{
+   u16 adv = 0;
+
+   /* Only allow advertising what this PHY supports */
+   linkmode_and(phydev->advertising, phydev->advertising,
+phydev->supported);
+
+   if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+ phydev->advertising))
+   adv |= ADVERTISE_1000XFULL;
+   if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+ phydev->advertising))
+   adv |= ADVERTISE_1000XPAUSE;
+   if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+ phydev->advertising))
+   adv |= ADVERTISE_1000XPSE_ASYM;
+
+   return phy_modify_changed(phydev, MII_ADVERTISE,
+ ADVERTISE_1000XFULL | ADVERTISE_1000XPAUSE |
+ ADVERTISE_1000XHALF | ADVERTISE_1000XPSE_ASYM,
+ adv);
+}
+
 /**
  * genphy_config_eee_advert - disable unwanted eee mode advertisement
  * @phydev: target phy_device struct
@@ -1726,6 +1760,54 @@ int genphy_config_aneg(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(genphy_config_aneg);
 
+/**
+ * genphy_c37_config_aneg - restart auto-negotiation or write BMCR
+ * @phydev: target phy_device struct
+ *
+ * Description: If auto-negotiation is enabled, we configure the
+ *   advertising, and then restart auto-negotiation.  If it is not
+ *   enabled, then we write the BMCR. This function is intended
+ *   for use with Clause 37 1000Base-X mode.
+ */
+int genphy_c37_config_aneg(struct phy_device *phydev)
+{
+   int err, changed;
+
+   if (AUTONEG_ENABLE != phydev->autoneg)
+   return genphy_setup_forced(phydev);
+
+   err = phy_modify(phydev, MII_BMCR, BMCR_SPEED1000 | BMCR_SPEED100,
+BMCR_SPEED1000);
+   if (err)
+   return err;
+
+   changed = genphy_c37_config_advert(phydev);
+   if (changed < 0) /* error */
+   return changed;
+
+   if (!changed) {
+   /* Advertisement hasn't changed, but maybe aneg was never on to
+* begin with?  Or maybe phy was isolated?
+*/
+   int ctl = phy_read(phydev, MII_BMCR);
+
+   if (ctl < 0)
+   return ctl;
+
+   if (!(ctl & BMCR_ANENABLE) || (ctl & BMCR_ISOLATE))
+   changed = 1; /* do restart aneg */
+   }
+
+   /* Only restart aneg if we are advertising something different
+* than we were before.
+*/
+   if (changed > 0)
+   return genphy_restart_aneg(phydev);
+
+   return 0;
+}
+EXPORT_SYMBOL(genphy_c37_config_aneg);
+
 /**
  * genphy_aneg_done - return auto-negotiation status
  * @phydev: target phy_device struct
@@ -1864,6 +1946,63 @@ int genphy_read_status(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(genphy_read_status);
 
+/**
+ * genphy_c37_read_status - check the link status and update current link state
+ * @phydev: target phy_device struct
+ *
+ * Description: Check the link, then figure out the current state
+ *   by comparing what we advertise with what the link partner
+ *   advertises. This function is for Clause 37 1000Base-X mode.
+ */
+int genphy_c37_read_status(struct phy_device *phydev)
+{
+   int lpa, err, old_link = phydev->link;
+
+   /* Update the link, but return if there was an error */
+   err = genphy_update_link(phydev);
+   if (err)
+   return err;
+
+   /* why bother the PHY if nothing can have changed */
+   if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
+   return 0;
+
+   phydev->duplex = DUPLEX_UNKNOWN;
+   phydev->pause = 0;
+   phydev->asym_pause = 0;
+
+   if (phydev->autoneg == AUTONEG_ENABLE && 

Re: [PATCH net-next v4 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 8/8/19 3:11 PM, Heiner Kallweit wrote:
> On 08.08.2019 23:47, Tao Ren wrote:
>> Hi Heiner,
>>
>> On 8/7/19 9:24 PM, Tao Ren wrote:
>>> Hi Heiner,
>>>
>>> On 8/7/19 12:18 PM, Heiner Kallweit wrote:
>>>> On 06.08.2019 23:42, Tao Ren wrote:
>>>>> Hi Andrew / Heiner / Vladimir,
>>>>>
>>>>> On 8/6/19 2:09 PM, Tao Ren wrote:
>>>>>> The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
>>>>>> example, on Facebook CMM BMC platform), mainly because genphy functions
>>>>>> are designed for copper links, and 1000Base-X (clause 37) auto 
>>>>>> negotiation
>>>>>> needs to be handled differently.
>>>>>>
>>>>>> This patch enables 1000Base-X support for BCM54616S by customizing 3
>>>>>> driver callbacks:
>>>>>>
>>>>>>   - probe: probe callback detects PHY's operation mode based on
>>>>>> INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>>>>>> Control register.
>>>>>>
>>>>>>   - config_aneg: bcm54616s_config_aneg_1000bx function is added for auto
>>>>>> negotiation in 1000Base-X mode.
>>>>>>
>>>>>>   - read_status: BCM54616S and BCM5482 PHY share the same read_status
>>>>>> callback which manually set link speed and duplex mode in 1000Base-X
>>>>>> mode.
>>>>>>
>>>>>> Signed-off-by: Tao Ren 
>>>>>
>>>>> I customized config_aneg function for BCM54616S 1000Base-X mode and 
>>>>> link-down issue is also fixed: the patch is tested on Facebook CMM and 
>>>>> Minipack BMC and everything looks normal. Please kindly review when you 
>>>>> have bandwidth and let me know if you have further suggestions.
>>>>>
>>>>> BTW, I would be happy to help if we decide to add a set of genphy 
>>>>> functions for clause 37, although that may mean I need more help/guidance 
>>>>> from you :-)
>>>>
>>>> You want to have standard clause 37 aneg and this should be generic in 
>>>> phylib.
>>>> I hacked together a first version that is compile-tested only:
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_1143631_=DwICaQ=5VD0RTtNlTh3ycd41b3MUw=iYElT7HC77pRZ3byVvW8ng=ZJArOJvHqNkqvs1x8l9HjfxjCN8e5xJpPz2YViBuKRA=EskpfBQtu9IBVeb96dv-sz76xIz4tJK5-lD4-qdIyWI=
>>>>  
>>>> It supports fixed mode too.
>>>>
>>>> It doesn't support half duplex mode because phylib doesn't know 1000BaseX 
>>>> HD yet.
>>>> Not sure whether half duplex mode is used at all in reality.
>>>>
>>>> You could test the new core functions in your own config_aneg and 
>>>> read_status
>>>> callback implementations.
>>>
>>> Thank you very much for the help! I'm planning to add these functions but I 
>>> haven't started yet because I'm still going through clause 37 :-)
>>>
>>> Let me apply your patch and run some test on my platform. Will share you 
>>> results tomorrow.
>>
>> The patch "net: phy: add support for clause 37 auto-negotiation" works on my 
>> CMM platform, with just 1 minor change in phy.h (I guess it's typo?). Thanks 
>> again for the help!
>>
>> -int genphy_c37_aneg_done(struct phy_device *phydev);
>> +int genphy_c37_config_aneg(struct phy_device *phydev);
>>
> Indeed, this was a typo. Thanks.
> 
>> BTW, shall I send out my patch v5 now (based on your patch)? Or I should 
>> wait till your patch is included in net-next and then send out my patch?
>>
> Adding new functions to the core is typically only acceptable if in the
> same patch series a user of the new functions is added. Therefore it's
> best if you include my patch in your series (just remove the RFC tag and
> set the From: properly).

Got it. Let me play with it (especially "From:" property) and will send out 
patch series soon.


Cheers,

Tao

Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Tao Ren]

On 8/8/19 2:16 PM, Andrew Lunn wrote:
> On Thu, Aug 08, 2019 at 07:02:54PM +0000, Tao Ren wrote:
>> Hi Andrew,
>>
>> On 8/8/19 6:32 AM, Andrew Lunn wrote:
>>>> Let me prepare patch v2 using device tree. I'm not sure if standard
>>>> "mac-address" fits this situation because all we need is an offset
>>>> (integer) and BMC MAC is calculated by adding the offset to NIC's
>>>> MAC address. Anyways, let me work out v2 patch we can discuss more
>>>> then.
>>>
>>> Hi Tao
>>>
>>> I don't know BMC terminology. By NICs MAC address, you are referring
>>> to the hosts MAC address? The MAC address the big CPU is using for its
>>> interface?  Where does this NIC get its MAC address from? If the BMCs
>>> bootloader has access to it, it can set the mac-address property in
>>> the device tree.
>>
>> Sorry for the confusion and let me clarify more:
>>
> 
>> The NIC here refers to the Network controller which provide network
>> connectivity for both BMC (via NC-SI) and Host (for example, via
>> PCIe).
>>
> 
>> On Facebook Yamp BMC, BMC sends NCSI_OEM_GET_MAC command (as an
>> ethernet packet) to the Network Controller while bringing up eth0,
>> and the (Broadcom) Network Controller replies with the Base MAC
>> Address reserved for the platform. As for Yamp, Base-MAC and
>> Base-MAC+1 are used by Host (big CPU) and Base-MAC+2 are assigned to
>> BMC. In my opinion, Base MAC and MAC address assignments are
>> controlled by Network Controller, which is transparent to both BMC
>> and Host.
> 
> Hi Tao
> 
> I've not done any work in the BMC field, so thanks for explaining
> this.
> 
> In a typical embedded system, each network interface is assigned a MAC
> address by the vendor. But here, things are different. The BMC SoC
> network interface has not been assigned a MAC address, it needs to ask
> the network controller for its MAC address, and then do some magical
> transformation on the answer to derive a MAC address for
> itself. Correct?

Yes. It's correct.

> It seems like a better design would of been, the BMC sends a
> NCSI_OEM_GET_BMC_MAC and the answer it gets back is the MAC address
> the BMC should use. No magic involved. But i guess it is too late to
> do that now.

Some NCSI Network Controllers support such OEM command (Get Provisioned BMC MAC 
Address), but unfortunately it's not supported on Yamp.

>> I'm not sure if I understand your suggestion correctly: do you mean
>> we should move the logic (GET_MAC from Network Controller, adding
>> offset and configuring BMC MAC) from kernel to boot loader?
> 
> In general, the kernel is generic. It probably boots on any ARM system
> which is has the needed modules for. The bootloader is often much more
> specific. It might not be fully platform specific, but it will be at
> least specific to the general family of BMC SoCs. If you consider the
> combination of the BMC bootloader and the device tree blob, you have
> something specific to the platform. This magical transformation of
> adding 2 seems to be very platform specific. So having this magic in
> the bootloader+DT seems like the best place to put it.

I understand your concern now. Thank you for the explanation.

> However, how you pass the resulting MAC address to the kernel should
> be as generic as possible. The DT "mac-address" property is very
> generic, many MAC drivers understand it. Using it also allows for
> vendors which actually assign a MAC address to the BMC to pass it to
> the BMC, avoiding all this NCSI_OEM_GET_MAC handshake. Having an API
> which just passing '2' is not generic at all.

After giving it more thought, I'm thinking about adding ncsi dt node with 
following structure (mac/ncsi similar to mac/mdio/phy):

 {
/* MAC properties... */

use-ncsi;
ncsi {
/* ncsi level properties if any */

package@0 {
/* package level properties if any */

channel@0 {
/* channel level properties if any */

bmc-mac-offset = <2>;
};

channel@1 {
/* channel #1 properties */
};
};

/* package #1 properties start here.. */
};
};

The reasons behind this are:

1) mac driver doesn't need to parse "mac-offset" stuff: these 
ncsi-network-controller specific settings should be parsed in ncsi stack.

2) get_bmc_mac_address command is a channel specific command, and technically 
people can configure different offset/formula for different channels.

Any concerns or suggestions?


Thanks,

Tao

Re: [PATCH net-next v4 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

Hi Heiner,

On 8/7/19 9:24 PM, Tao Ren wrote:
> Hi Heiner,
> 
> On 8/7/19 12:18 PM, Heiner Kallweit wrote:
>> On 06.08.2019 23:42, Tao Ren wrote:
>>> Hi Andrew / Heiner / Vladimir,
>>>
>>> On 8/6/19 2:09 PM, Tao Ren wrote:
>>>> The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
>>>> example, on Facebook CMM BMC platform), mainly because genphy functions
>>>> are designed for copper links, and 1000Base-X (clause 37) auto negotiation
>>>> needs to be handled differently.
>>>>
>>>> This patch enables 1000Base-X support for BCM54616S by customizing 3
>>>> driver callbacks:
>>>>
>>>>   - probe: probe callback detects PHY's operation mode based on
>>>> INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>>>> Control register.
>>>>
>>>>   - config_aneg: bcm54616s_config_aneg_1000bx function is added for auto
>>>> negotiation in 1000Base-X mode.
>>>>
>>>>   - read_status: BCM54616S and BCM5482 PHY share the same read_status
>>>> callback which manually set link speed and duplex mode in 1000Base-X
>>>> mode.
>>>>
>>>> Signed-off-by: Tao Ren 
>>>
>>> I customized config_aneg function for BCM54616S 1000Base-X mode and 
>>> link-down issue is also fixed: the patch is tested on Facebook CMM and 
>>> Minipack BMC and everything looks normal. Please kindly review when you 
>>> have bandwidth and let me know if you have further suggestions.
>>>
>>> BTW, I would be happy to help if we decide to add a set of genphy functions 
>>> for clause 37, although that may mean I need more help/guidance from you :-)
>>
>> You want to have standard clause 37 aneg and this should be generic in 
>> phylib.
>> I hacked together a first version that is compile-tested only:
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_1143631_=DwICaQ=5VD0RTtNlTh3ycd41b3MUw=iYElT7HC77pRZ3byVvW8ng=ZJArOJvHqNkqvs1x8l9HjfxjCN8e5xJpPz2YViBuKRA=EskpfBQtu9IBVeb96dv-sz76xIz4tJK5-lD4-qdIyWI=
>>  
>> It supports fixed mode too.
>>
>> It doesn't support half duplex mode because phylib doesn't know 1000BaseX HD 
>> yet.
>> Not sure whether half duplex mode is used at all in reality.
>>
>> You could test the new core functions in your own config_aneg and read_status
>> callback implementations.
> 
> Thank you very much for the help! I'm planning to add these functions but I 
> haven't started yet because I'm still going through clause 37 :-)
> 
> Let me apply your patch and run some test on my platform. Will share you 
> results tomorrow.

The patch "net: phy: add support for clause 37 auto-negotiation" works on my 
CMM platform, with just 1 minor change in phy.h (I guess it's typo?). Thanks 
again for the help!

-int genphy_c37_aneg_done(struct phy_device *phydev);
+int genphy_c37_config_aneg(struct phy_device *phydev);

BTW, shall I send out my patch v5 now (based on your patch)? Or I should wait 
till your patch is included in net-next and then send out my patch?


Cheers,

Tao

Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Tao Ren]

Hi Andrew,

On 8/8/19 6:32 AM, Andrew Lunn wrote:
>> Let me prepare patch v2 using device tree. I'm not sure if standard
>> "mac-address" fits this situation because all we need is an offset
>> (integer) and BMC MAC is calculated by adding the offset to NIC's
>> MAC address. Anyways, let me work out v2 patch we can discuss more
>> then.
> 
> Hi Tao
> 
> I don't know BMC terminology. By NICs MAC address, you are referring
> to the hosts MAC address? The MAC address the big CPU is using for its
> interface?  Where does this NIC get its MAC address from? If the BMCs
> bootloader has access to it, it can set the mac-address property in
> the device tree.

Sorry for the confusion and let me clarify more:

The NIC here refers to the Network controller which provide network 
connectivity for both BMC (via NC-SI) and Host (for example, via PCIe).

On Facebook Yamp BMC, BMC sends NCSI_OEM_GET_MAC command (as an ethernet 
packet) to the Network Controller while bringing up eth0, and the (Broadcom) 
Network Controller replies with the Base MAC Address reserved for the platform. 
As for Yamp, Base-MAC and Base-MAC+1 are used by Host (big CPU) and Base-MAC+2 
are assigned to BMC. In my opinion, Base MAC and MAC address assignments are 
controlled by Network Controller, which is transparent to both BMC and Host.

I'm not sure if I understand your suggestion correctly: do you mean we should 
move the logic (GET_MAC from Network Controller, adding offset and configuring 
BMC MAC) from kernel to boot loader?

Sam posted several ncsi patches for u-boot recently. Sam, do we support the 
work (implemented in this patch) in uboot? Or are we planning to do so?


Thanks,

Tao

Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Tao Ren]

On 8/7/19 10:36 AM, Vijay Khemka wrote:
> Lgtm except one small comment below.
> 
> On 8/6/19, 5:22 PM, "openbmc on behalf of Tao Ren" 
>  tao...@fb.com> wrote:
> 
> Currently BMC's MAC address is calculated by adding 1 to NCSI NIC's base
> MAC address when CONFIG_NCSI_OEM_CMD_GET_MAC option is enabled. The logic
> doesn't work for platforms with different BMC MAC offset: for example,
> Facebook Yamp BMC's MAC address is calculated by adding 2 to NIC's base
> MAC address ("BaseMAC + 1" is reserved for Host use).
> 
> This patch adds NET_NCSI_MC_MAC_OFFSET config option to customize offset
> between NIC's Base MAC address and BMC's MAC address. Its default value is
> set to 1 to avoid breaking existing users.
> 
> Signed-off-by: Tao Ren 
> ---
>  net/ncsi/Kconfig|  8 
>  net/ncsi/ncsi-rsp.c | 15 +--
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ncsi/Kconfig b/net/ncsi/Kconfig
> index 2f1e5756c03a..be8efe1ed99e 100644
> --- a/net/ncsi/Kconfig
> +++ b/net/ncsi/Kconfig
> @@ -17,3 +17,11 @@ config NCSI_OEM_CMD_GET_MAC
>   ---help---
> This allows to get MAC address from NCSI firmware and set them back to
>   controller.
> +config NET_NCSI_MC_MAC_OFFSET
> + int
> + prompt "Offset of Management Controller's MAC Address"
> + depends on NCSI_OEM_CMD_GET_MAC
> + default 1
> + help
> +   This defines the offset between Network Controller's (base) MAC
> +   address and Management Controller's MAC address.
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index 7581bf919885..24a791f9ebf5 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -656,6 +656,11 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct 
> ncsi_request *nr)
>   struct ncsi_rsp_oem_pkt *rsp;
>   struct sockaddr saddr;
>   int ret = 0;
> +#ifdef CONFIG_NET_NCSI_MC_MAC_OFFSET
> + int mac_offset = CONFIG_NET_NCSI_MC_MAC_OFFSET;
> +#else
> + int mac_offset = 1;
> +#endif
>  
>   /* Get the response header */
>   rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
> @@ -663,8 +668,14 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct 
> ncsi_request *nr)
>   saddr.sa_family = ndev->type;
>   ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
>   memcpy(saddr.sa_data, >data[BCM_MAC_ADDR_OFFSET], ETH_ALEN);
> - /* Increase mac address by 1 for BMC's address */
> - eth_addr_inc((u8 *)saddr.sa_data);
> +
> + /* Management Controller's MAC address is calculated by adding
> +  * the offset to Network Controller's (base) MAC address.
> +  * Note: negative offset is "ignored", and BMC will use the Base
> Just mention negative and zero offset is ignored. As you are ignoring 0 as 
> well.

Thank you Vijay for the review.

Zero offset is not ignored: users get what they want when setting offset to 0 
(BMC-MAC = Base-MAC).


Thanks,

Tao

Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Tao Ren]

On 8/7/19 11:41 AM, Andrew Lunn wrote:
> On Wed, Aug 07, 2019 at 11:25:18AM -0700, Jakub Kicinski wrote:
>> On Tue, 6 Aug 2019 17:21:18 -0700, Tao Ren wrote:
>>> Currently BMC's MAC address is calculated by adding 1 to NCSI NIC's base
>>> MAC address when CONFIG_NCSI_OEM_CMD_GET_MAC option is enabled. The logic
>>> doesn't work for platforms with different BMC MAC offset: for example,
>>> Facebook Yamp BMC's MAC address is calculated by adding 2 to NIC's base
>>> MAC address ("BaseMAC + 1" is reserved for Host use).
>>>
>>> This patch adds NET_NCSI_MC_MAC_OFFSET config option to customize offset
>>> between NIC's Base MAC address and BMC's MAC address. Its default value is
>>> set to 1 to avoid breaking existing users.
>>>
>>> Signed-off-by: Tao Ren 
>>
>> Maybe someone more knowledgeable like Andrew has an opinion here, 
>> but to me it seems a bit strange to encode what seems to be platfrom
>> information in the kernel config :(
> 
> Yes, this is not a good idea. It makes it impossible to have a 'BMC
> distro' kernel which you install on a number of different BMCs.
> 
> A device tree property would be better. Ideally it would be the
> standard local-mac-address, or mac-address.

Thank you Andrew and Jakub for the feedback. I picked up kconfig approach 
mainly because it's an OEM-only extention which is used only when 
NCSI_OEM_CMD_GET_MAC is enabled.

Let me prepare patch v2 using device tree. I'm not sure if standard 
"mac-address" fits this situation because all we need is an offset (integer) 
and BMC MAC is calculated by adding the offset to NIC's MAC address. Anyways, 
let me work out v2 patch we can discuss more then.


Thanks,

Tao

Re: [PATCH net-next v4 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

Hi Heiner,

On 8/7/19 12:18 PM, Heiner Kallweit wrote:
> On 06.08.2019 23:42, Tao Ren wrote:
>> Hi Andrew / Heiner / Vladimir,
>>
>> On 8/6/19 2:09 PM, Tao Ren wrote:
>>> The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
>>> example, on Facebook CMM BMC platform), mainly because genphy functions
>>> are designed for copper links, and 1000Base-X (clause 37) auto negotiation
>>> needs to be handled differently.
>>>
>>> This patch enables 1000Base-X support for BCM54616S by customizing 3
>>> driver callbacks:
>>>
>>>   - probe: probe callback detects PHY's operation mode based on
>>> INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>>> Control register.
>>>
>>>   - config_aneg: bcm54616s_config_aneg_1000bx function is added for auto
>>> negotiation in 1000Base-X mode.
>>>
>>>   - read_status: BCM54616S and BCM5482 PHY share the same read_status
>>> callback which manually set link speed and duplex mode in 1000Base-X
>>> mode.
>>>
>>> Signed-off-by: Tao Ren 
>>
>> I customized config_aneg function for BCM54616S 1000Base-X mode and 
>> link-down issue is also fixed: the patch is tested on Facebook CMM and 
>> Minipack BMC and everything looks normal. Please kindly review when you have 
>> bandwidth and let me know if you have further suggestions.
>>
>> BTW, I would be happy to help if we decide to add a set of genphy functions 
>> for clause 37, although that may mean I need more help/guidance from you :-)
> 
> You want to have standard clause 37 aneg and this should be generic in phylib.
> I hacked together a first version that is compile-tested only:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_1143631_=DwICaQ=5VD0RTtNlTh3ycd41b3MUw=iYElT7HC77pRZ3byVvW8ng=ZJArOJvHqNkqvs1x8l9HjfxjCN8e5xJpPz2YViBuKRA=EskpfBQtu9IBVeb96dv-sz76xIz4tJK5-lD4-qdIyWI=
>  
> It supports fixed mode too.
> 
> It doesn't support half duplex mode because phylib doesn't know 1000BaseX HD 
> yet.
> Not sure whether half duplex mode is used at all in reality.
> 
> You could test the new core functions in your own config_aneg and read_status
> callback implementations.

Thank you very much for the help! I'm planning to add these functions but I 
haven't started yet because I'm still going through clause 37 :-)

Let me apply your patch and run some test on my platform. Will share you 
results tomorrow.


Cheers,

Tao

[PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset

[Tao Ren]

Currently BMC's MAC address is calculated by adding 1 to NCSI NIC's base
MAC address when CONFIG_NCSI_OEM_CMD_GET_MAC option is enabled. The logic
doesn't work for platforms with different BMC MAC offset: for example,
Facebook Yamp BMC's MAC address is calculated by adding 2 to NIC's base
MAC address ("BaseMAC + 1" is reserved for Host use).

This patch adds NET_NCSI_MC_MAC_OFFSET config option to customize offset
between NIC's Base MAC address and BMC's MAC address. Its default value is
set to 1 to avoid breaking existing users.

Signed-off-by: Tao Ren 
---
 net/ncsi/Kconfig|  8 
 net/ncsi/ncsi-rsp.c | 15 +--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/net/ncsi/Kconfig b/net/ncsi/Kconfig
index 2f1e5756c03a..be8efe1ed99e 100644
--- a/net/ncsi/Kconfig
+++ b/net/ncsi/Kconfig
@@ -17,3 +17,11 @@ config NCSI_OEM_CMD_GET_MAC
---help---
  This allows to get MAC address from NCSI firmware and set them back to
controller.
+config NET_NCSI_MC_MAC_OFFSET
+   int
+   prompt "Offset of Management Controller's MAC Address"
+   depends on NCSI_OEM_CMD_GET_MAC
+   default 1
+   help
+ This defines the offset between Network Controller's (base) MAC
+ address and Management Controller's MAC address.
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 7581bf919885..24a791f9ebf5 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -656,6 +656,11 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct 
ncsi_request *nr)
struct ncsi_rsp_oem_pkt *rsp;
struct sockaddr saddr;
int ret = 0;
+#ifdef CONFIG_NET_NCSI_MC_MAC_OFFSET
+   int mac_offset = CONFIG_NET_NCSI_MC_MAC_OFFSET;
+#else
+   int mac_offset = 1;
+#endif
 
/* Get the response header */
rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
@@ -663,8 +668,14 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct 
ncsi_request *nr)
saddr.sa_family = ndev->type;
ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
memcpy(saddr.sa_data, >data[BCM_MAC_ADDR_OFFSET], ETH_ALEN);
-   /* Increase mac address by 1 for BMC's address */
-   eth_addr_inc((u8 *)saddr.sa_data);
+
+   /* Management Controller's MAC address is calculated by adding
+* the offset to Network Controller's (base) MAC address.
+* Note: negative offset is "ignored", and BMC will use the Base
+* MAC address in this case.
+*/
+   while (mac_offset-- > 0)
+   eth_addr_inc((u8 *)saddr.sa_data);
if (!is_valid_ether_addr((const u8 *)saddr.sa_data))
return -ENXIO;
 
-- 
2.17.1

Re: [PATCH net-next v4 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 8/6/19 3:00 PM, Heiner Kallweit wrote:
> On 06.08.2019 23:09, Tao Ren wrote:
>> The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
>> example, on Facebook CMM BMC platform), mainly because genphy functions
>> are designed for copper links, and 1000Base-X (clause 37) auto negotiation
>> needs to be handled differently.
>>
>> This patch enables 1000Base-X support for BCM54616S by customizing 3
>> driver callbacks:
>>
>>   - probe: probe callback detects PHY's operation mode based on
>> INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>> Control register.
>>
>>   - config_aneg: bcm54616s_config_aneg_1000bx function is added for auto
>> negotiation in 1000Base-X mode.
>>
>>   - read_status: BCM54616S and BCM5482 PHY share the same read_status
>>     callback which manually set link speed and duplex mode in 1000Base-X
>> mode.
>>
>> Signed-off-by: Tao Ren 
>> ---
>>  Changes in v4:
>>   - add bcm54616s_config_aneg_1000bx() to deal with auto negotiation in
>> 1000Base-X mode.
>>  Changes in v3:
>>   - rename bcm5482_read_status to bcm54xx_read_status so the callback can
>> be shared by BCM5482 and BCM54616S.
>>  Changes in v2:
>>   - Auto-detect PHY operation mode instead of passing DT node.
>>   - move PHY mode auto-detect logic from config_init to probe callback.
>>   - only set speed (not including duplex) in read_status callback.
>>   - update patch description with more background to avoid confusion.
>>   - patch #1 in the series ("net: phy: broadcom: set features explicitly
>> for BCM54616") is dropped: the fix should go to get_features callback
>> which may potentially depend on this patch.
>>
>>  drivers/net/phy/broadcom.c | 62 ++
>>  include/linux/brcmphy.h| 10 --
>>  2 files changed, 64 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
>> index 937d0059e8ac..bf61ed8451e5 100644
>> --- a/drivers/net/phy/broadcom.c
>> +++ b/drivers/net/phy/broadcom.c
>> @@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device *phydev)
>>  /*
>>   * Select 1000BASE-X register set (primary SerDes)
>>   */
>> -reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
>> -bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
>> - reg | BCM5482_SHD_MODE_1000BX);
>> +reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
>> +bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
>> + reg | BCM54XX_SHD_MODE_1000BX);
>>  
>>  /*
>>   * LED1=ACTIVITYLED, LED3=LINKSPD[2]
>> @@ -409,7 +409,7 @@ static int bcm5482_config_init(struct phy_device *phydev)
>>  return err;
>>  }
>>  
>> -static int bcm5482_read_status(struct phy_device *phydev)
>> +static int bcm54xx_read_status(struct phy_device *phydev)
>>  {
>>  int err;
>>  
>> @@ -451,12 +451,60 @@ static int bcm5481_config_aneg(struct phy_device 
>> *phydev)
>>  return ret;
>>  }
>>  
>> +static int bcm54616s_probe(struct phy_device *phydev)
>> +{
>> +int val, intf_sel;
>> +
>> +val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
>> +if (val < 0)
>> +return val;
>> +
>> +/* The PHY is strapped in RGMII to fiber mode when INTERF_SEL[1:0]
>> + * is 01b.
>> + */
>> +intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1;
>> +if (intf_sel == 1) {
>> +val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL);
>> +if (val < 0)
>> +return val;
>> +
>> +/* Bit 0 of the SerDes 100-FX Control register, when set
>> + * to 1, sets the MII/RGMII -> 100BASE-FX configuration.
>> + * When this bit is set to 0, it sets the GMII/RGMII ->
>> + * 1000BASE-X configuration.
>> + */
>> +if (!(val & BCM54616S_100FX_MODE))
>> +phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX;
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +static int bcm54616s_config_aneg_1000bx(struct phy_device *phydev)
>> +{
>> +int err;
>> +int adv = 0;
>> +
>> +if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Ful

Re: [PATCH net-next v4 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

Hi Andrew / Heiner / Vladimir,

On 8/6/19 2:09 PM, Tao Ren wrote:
> The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
> example, on Facebook CMM BMC platform), mainly because genphy functions
> are designed for copper links, and 1000Base-X (clause 37) auto negotiation
> needs to be handled differently.
> 
> This patch enables 1000Base-X support for BCM54616S by customizing 3
> driver callbacks:
> 
>   - probe: probe callback detects PHY's operation mode based on
> INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
> Control register.
> 
>   - config_aneg: bcm54616s_config_aneg_1000bx function is added for auto
> negotiation in 1000Base-X mode.
> 
>   - read_status: BCM54616S and BCM5482 PHY share the same read_status
> callback which manually set link speed and duplex mode in 1000Base-X
> mode.
> 
> Signed-off-by: Tao Ren 

I customized config_aneg function for BCM54616S 1000Base-X mode and link-down 
issue is also fixed: the patch is tested on Facebook CMM and Minipack BMC and 
everything looks normal. Please kindly review when you have bandwidth and let 
me know if you have further suggestions.

BTW, I would be happy to help if we decide to add a set of genphy functions for 
clause 37, although that may mean I need more help/guidance from you :-)


Cheers,

Tao

[PATCH net-next v4 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for
example, on Facebook CMM BMC platform), mainly because genphy functions
are designed for copper links, and 1000Base-X (clause 37) auto negotiation
needs to be handled differently.

This patch enables 1000Base-X support for BCM54616S by customizing 3
driver callbacks:

  - probe: probe callback detects PHY's operation mode based on
INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
Control register.

  - config_aneg: bcm54616s_config_aneg_1000bx function is added for auto
negotiation in 1000Base-X mode.

  - read_status: BCM54616S and BCM5482 PHY share the same read_status
callback which manually set link speed and duplex mode in 1000Base-X
mode.

Signed-off-by: Tao Ren 
---
 Changes in v4:
  - add bcm54616s_config_aneg_1000bx() to deal with auto negotiation in
1000Base-X mode.
 Changes in v3:
  - rename bcm5482_read_status to bcm54xx_read_status so the callback can
be shared by BCM5482 and BCM54616S.
 Changes in v2:
  - Auto-detect PHY operation mode instead of passing DT node.
  - move PHY mode auto-detect logic from config_init to probe callback.
  - only set speed (not including duplex) in read_status callback.
  - update patch description with more background to avoid confusion.
  - patch #1 in the series ("net: phy: broadcom: set features explicitly
for BCM54616") is dropped: the fix should go to get_features callback
which may potentially depend on this patch.

 drivers/net/phy/broadcom.c | 62 ++
 include/linux/brcmphy.h| 10 --
 2 files changed, 64 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 937d0059e8ac..bf61ed8451e5 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device *phydev)
/*
 * Select 1000BASE-X register set (primary SerDes)
 */
-   reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
-   bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
-reg | BCM5482_SHD_MODE_1000BX);
+   reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
+   bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
+reg | BCM54XX_SHD_MODE_1000BX);
 
/*
 * LED1=ACTIVITYLED, LED3=LINKSPD[2]
@@ -409,7 +409,7 @@ static int bcm5482_config_init(struct phy_device *phydev)
return err;
 }
 
-static int bcm5482_read_status(struct phy_device *phydev)
+static int bcm54xx_read_status(struct phy_device *phydev)
 {
int err;
 
@@ -451,12 +451,60 @@ static int bcm5481_config_aneg(struct phy_device *phydev)
return ret;
 }
 
+static int bcm54616s_probe(struct phy_device *phydev)
+{
+   int val, intf_sel;
+
+   val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
+   if (val < 0)
+   return val;
+
+   /* The PHY is strapped in RGMII to fiber mode when INTERF_SEL[1:0]
+* is 01b.
+*/
+   intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1;
+   if (intf_sel == 1) {
+   val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL);
+   if (val < 0)
+   return val;
+
+   /* Bit 0 of the SerDes 100-FX Control register, when set
+* to 1, sets the MII/RGMII -> 100BASE-FX configuration.
+* When this bit is set to 0, it sets the GMII/RGMII ->
+* 1000BASE-X configuration.
+*/
+   if (!(val & BCM54616S_100FX_MODE))
+   phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX;
+   }
+
+   return 0;
+}
+
+static int bcm54616s_config_aneg_1000bx(struct phy_device *phydev)
+{
+   int err;
+   int adv = 0;
+
+   if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+ phydev->supported))
+   adv |= ADVERTISE_1000XFULL;
+
+   err = phy_modify_changed(phydev, MII_ADVERTISE, 0, adv);
+   if (err > 0)
+   err = genphy_restart_aneg(phydev);
+
+   return err;
+}
+
 static int bcm54616s_config_aneg(struct phy_device *phydev)
 {
int ret;
 
/* Aneg firsly. */
-   ret = genphy_config_aneg(phydev);
+   if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX)
+   ret = bcm54616s_config_aneg_1000bx(phydev);
+   else
+   ret = genphy_config_aneg(phydev);
 
/* Then we can set up the delay. */
bcm54xx_config_clock_delay(phydev);
@@ -655,6 +703,8 @@ static struct phy_driver broadcom_drivers[] = {
.config_aneg= bcm54616s_config_aneg,
.ack_interrupt  = bcm_phy_ack_intr,
.config_intr= bcm_phy_config_intr,
+   .read_status= bcm54xx

[PATCH net-next v4 1/2] net: phy: modify assignment to OR for dev_flags in phy_attach_direct

[Tao Ren]

Modify the assignment to OR when dealing with phydev->dev_flags in
phy_attach_direct function, and this is to make sure dev_flags set in
driver's probe callback won't be lost.

Suggested-by: Andrew Lunn 
CC: Heiner Kallweit 
CC: Vladimir Oltean 
Signed-off-by: Tao Ren 
---
 Changes:
  - nothing is changed in v1/v2/v3: it's given v4 to align with v4 of the
2nd patch in the series.

 drivers/net/phy/phy_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 6b5cb87f3866..9259eb45c794 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1270,7 +1270,7 @@ int phy_attach_direct(struct net_device *dev, struct 
phy_device *phydev,
phydev_err(phydev, "error creating 'phy_standalone' 
sysfs entry\n");
}
 
-   phydev->dev_flags = flags;
+   phydev->dev_flags |= flags;
 
phydev->interface = interface;
 
-- 
2.17.1

Re: [PATCH net-next v3] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 8/5/19 1:45 PM, Heiner Kallweit wrote:
> On 04.08.2019 21:22, Vladimir Oltean wrote:
>> On Sun, 4 Aug 2019 at 19:07, Heiner Kallweit  wrote:
>>>
>>> On 04.08.2019 17:59, Vladimir Oltean wrote:
 On Sun, 4 Aug 2019 at 17:52, Andrew Lunn  wrote:
>
>>> The patchset looks better now. But is it ok, I wonder, to keep
>>> PHY_BCM_FLAGS_MODE_1000BX in phydev->dev_flags, considering that
>>> phy_attach_direct is overwriting it?
>>
>
>> I checked ftgmac100 driver (used on my machine) and it calls
>> phy_connect_direct which passes phydev->dev_flags when calling
>> phy_attach_direct: that explains why the flag is not cleared in my
>> case.
>
> Yes, that is the way it is intended to be used. The MAC driver can
> pass flags to the PHY. It is a fragile API, since the MAC needs to
> know what PHY is being used, since the flags are driver specific.
>
> One option would be to modify the assignment in phy_attach_direct() to
> OR in the flags passed to it with flags which are already in
> phydev->dev_flags.
>
> Andrew

 Even if that were the case (patching phy_attach_direct to apply a
 logical-or to dev_flags), it sounds fishy to me that the genphy code
 is unable to determine that this PHY is running in 1000Base-X mode.

 In my opinion it all boils down to this warning:

 "PHY advertising (0,0200,62c0) more modes than genphy
 supports, some modes not advertised".

>>> The genphy code deals with Clause 22 + Gigabit BaseT only.
>>> Question is whether you want aneg at all in 1000Base-X mode and
>>> what you want the config_aneg callback to do.
>>> There may be some inspiration in the Marvel PHY drivers.
>>>
>>
>> AN for 1000Base-X still gives you duplex and pause frame settings. I
>> thought the base page format for exchanging that info is standardized
>> in clause 37.
>> Does genphy cover only copper media by design, or is it desirable to
>> augment genphy_read_status?
>>
> So far we care about copper only in phylib. Some constants needed for
> Clause 37 support are defined, but used by few drivers only.
> 
> ADVERTISE_1000XHALF
> ADVERTISE_1000XFULL
> ADVERTISE_1000XPAUSE
> ADVERTISE_1000XPSE_ASYM
> 
> I think it would make sense to have something like genphy_c37_config_aneg.
> Similar for read_status.

Thank you all for the inputs on this patch.

If I understand correctly, we are going to create a set of genphy_c37_* 
functions for 1000x support so it can be used by phy drivers? Or are we 
considering other options? What's your recommendation on this specific patch?


Thanks,

Tao

[PATCH net-next] net: phy: modify assignment to OR for dev_flags in phy_attach_direct

[Tao Ren]

Modify the assignment to OR when dealing with phydev->dev_flags in
phy_attach_direct function, and this is to make sure dev_flags set in
driver's probe callback won't be lost.

Suggested-by: Andrew Lunn 
CC: Heiner Kallweit 
CC: Vladimir Oltean 
Signed-off-by: Tao Ren 
---
 drivers/net/phy/phy_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 6b5cb87f3866..9259eb45c794 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1270,7 +1270,7 @@ int phy_attach_direct(struct net_device *dev, struct 
phy_device *phydev,
phydev_err(phydev, "error creating 'phy_standalone' 
sysfs entry\n");
}
 
-   phydev->dev_flags = flags;
+   phydev->dev_flags |= flags;
 
phydev->interface = interface;
 
-- 
2.17.1

Re: [PATCH net-next v3] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

Hi Andrew,

On 8/4/19 7:51 AM, Andrew Lunn wrote:
>>> The patchset looks better now. But is it ok, I wonder, to keep
>>> PHY_BCM_FLAGS_MODE_1000BX in phydev->dev_flags, considering that
>>> phy_attach_direct is overwriting it?
>>
> 
>> I checked ftgmac100 driver (used on my machine) and it calls
>> phy_connect_direct which passes phydev->dev_flags when calling
>> phy_attach_direct: that explains why the flag is not cleared in my
>> case.
> 
> Yes, that is the way it is intended to be used. The MAC driver can
> pass flags to the PHY. It is a fragile API, since the MAC needs to
> know what PHY is being used, since the flags are driver specific.
> 
> One option would be to modify the assignment in phy_attach_direct() to
> OR in the flags passed to it with flags which are already in
> phydev->dev_flags.

It sounds like a reasonable fix/enhancement to replace overriding with OR, no 
matter which direction we are going to (either adding 1000bx aneg in genphy or 
providing phy-specific aneg callback).

Do you want me to send out the patch (I feel it's better to be in a separate 
patch?) or someone else will take care of it?


Thanks,

Tao

Re: [PATCH net-next v3] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

Hi Vladimir,

On 8/3/19 6:49 AM, Vladimir Oltean wrote:
> Hi Tao,
> 
> On Sat, 3 Aug 2019 at 00:56, Tao Ren  wrote:
>>
>> genphy_read_status() cannot report correct link speed when BCM54616S PHY
>> is configured in RGMII->1000Base-KX mode (for example, on Facebook CMM
>> BMC platform), and it is because speed-related fields in MII registers
>> are assigned different meanings in 1000X register set. Actually there
>> is no speed field in 1000X register set because link speed is always
>> 1000 Mb/s.
>>
>> The patch adds "probe" callback to detect PHY's operation mode based on
>> INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>> Control register. Besides, link speed is manually set to 1000 Mb/s in
>> "read_status" callback if PHY-switch link is 1000Base-X.
>>
>> Signed-off-by: Tao Ren 
>> ---
>>  Changes in v3:
>>   - rename bcm5482_read_status to bcm54xx_read_status so the callback can
>> be shared by BCM5482 and BCM54616S.
>>  Changes in v2:
>>   - Auto-detect PHY operation mode instead of passing DT node.
>>   - move PHY mode auto-detect logic from config_init to probe callback.
>>   - only set speed (not including duplex) in read_status callback.
>>   - update patch description with more background to avoid confusion.
>>   - patch #1 in the series ("net: phy: broadcom: set features explicitly
>> for BCM54616") is dropped: the fix should go to get_features callback
>> which may potentially depend on this patch.
>>
>>  drivers/net/phy/broadcom.c | 41 +-
>>  include/linux/brcmphy.h| 10 --
>>  2 files changed, 44 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
>> index 937d0059e8ac..ecad8a201a09 100644
>> --- a/drivers/net/phy/broadcom.c
>> +++ b/drivers/net/phy/broadcom.c
>> @@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device *phydev)
>> /*
>>  * Select 1000BASE-X register set (primary SerDes)
>>  */
>> -   reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
>> -   bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
>> -reg | BCM5482_SHD_MODE_1000BX);
>> +   reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
>> +   bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
>> +reg | BCM54XX_SHD_MODE_1000BX);
>>
>> /*
>>  * LED1=ACTIVITYLED, LED3=LINKSPD[2]
>> @@ -409,7 +409,7 @@ static int bcm5482_config_init(struct phy_device *phydev)
>> return err;
>>  }
>>
>> -static int bcm5482_read_status(struct phy_device *phydev)
>> +static int bcm54xx_read_status(struct phy_device *phydev)
>>  {
>> int err;
>>
>> @@ -464,6 +464,35 @@ static int bcm54616s_config_aneg(struct phy_device 
>> *phydev)
>> return ret;
>>  }
>>
>> +static int bcm54616s_probe(struct phy_device *phydev)
>> +{
>> +   int val, intf_sel;
>> +
>> +   val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
>> +   if (val < 0)
>> +   return val;
>> +
>> +   /* The PHY is strapped in RGMII to fiber mode when INTERF_SEL[1:0]
>> +* is 01b.
>> +*/
>> +   intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1;
>> +   if (intf_sel == 1) {
>> +   val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL);
>> +   if (val < 0)
>> +   return val;
>> +
>> +   /* Bit 0 of the SerDes 100-FX Control register, when set
>> +* to 1, sets the MII/RGMII -> 100BASE-FX configuration.
>> +* When this bit is set to 0, it sets the GMII/RGMII ->
>> +* 1000BASE-X configuration.
>> +*/
>> +   if (!(val & BCM54616S_100FX_MODE))
>> +   phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX;
>> +   }
>> +
>> +   return 0;
>> +}
>> +
>>  static int brcm_phy_setbits(struct phy_device *phydev, int reg, int set)
>>  {
>> int val;
>> @@ -655,6 +684,8 @@ static struct phy_driver broadcom_drivers[] = {
>> .config_aneg= bcm54616s_config_aneg,
>> .ack_interrupt  = bcm_phy_ack_intr,
>> .config_intr= bcm_phy_config_intr,
>> +   .read_status 

[PATCH net-next v3] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

genphy_read_status() cannot report correct link speed when BCM54616S PHY
is configured in RGMII->1000Base-KX mode (for example, on Facebook CMM
BMC platform), and it is because speed-related fields in MII registers
are assigned different meanings in 1000X register set. Actually there
is no speed field in 1000X register set because link speed is always
1000 Mb/s.

The patch adds "probe" callback to detect PHY's operation mode based on
INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
Control register. Besides, link speed is manually set to 1000 Mb/s in
"read_status" callback if PHY-switch link is 1000Base-X.

Signed-off-by: Tao Ren 
---
 Changes in v3:
  - rename bcm5482_read_status to bcm54xx_read_status so the callback can
be shared by BCM5482 and BCM54616S.
 Changes in v2:
  - Auto-detect PHY operation mode instead of passing DT node.
  - move PHY mode auto-detect logic from config_init to probe callback.
  - only set speed (not including duplex) in read_status callback.
  - update patch description with more background to avoid confusion.
  - patch #1 in the series ("net: phy: broadcom: set features explicitly
for BCM54616") is dropped: the fix should go to get_features callback
which may potentially depend on this patch.

 drivers/net/phy/broadcom.c | 41 +-
 include/linux/brcmphy.h| 10 --
 2 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 937d0059e8ac..ecad8a201a09 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device *phydev)
/*
 * Select 1000BASE-X register set (primary SerDes)
 */
-   reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
-   bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
-reg | BCM5482_SHD_MODE_1000BX);
+   reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
+   bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
+reg | BCM54XX_SHD_MODE_1000BX);
 
/*
 * LED1=ACTIVITYLED, LED3=LINKSPD[2]
@@ -409,7 +409,7 @@ static int bcm5482_config_init(struct phy_device *phydev)
return err;
 }
 
-static int bcm5482_read_status(struct phy_device *phydev)
+static int bcm54xx_read_status(struct phy_device *phydev)
 {
int err;
 
@@ -464,6 +464,35 @@ static int bcm54616s_config_aneg(struct phy_device *phydev)
return ret;
 }
 
+static int bcm54616s_probe(struct phy_device *phydev)
+{
+   int val, intf_sel;
+
+   val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
+   if (val < 0)
+   return val;
+
+   /* The PHY is strapped in RGMII to fiber mode when INTERF_SEL[1:0]
+* is 01b.
+*/
+   intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1;
+   if (intf_sel == 1) {
+   val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL);
+   if (val < 0)
+   return val;
+
+   /* Bit 0 of the SerDes 100-FX Control register, when set
+* to 1, sets the MII/RGMII -> 100BASE-FX configuration.
+* When this bit is set to 0, it sets the GMII/RGMII ->
+* 1000BASE-X configuration.
+*/
+   if (!(val & BCM54616S_100FX_MODE))
+   phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX;
+   }
+
+   return 0;
+}
+
 static int brcm_phy_setbits(struct phy_device *phydev, int reg, int set)
 {
int val;
@@ -655,6 +684,8 @@ static struct phy_driver broadcom_drivers[] = {
.config_aneg= bcm54616s_config_aneg,
.ack_interrupt  = bcm_phy_ack_intr,
.config_intr= bcm_phy_config_intr,
+   .read_status= bcm54xx_read_status,
+   .probe  = bcm54616s_probe,
 }, {
.phy_id = PHY_ID_BCM5464,
.phy_id_mask= 0xfff0,
@@ -689,7 +720,7 @@ static struct phy_driver broadcom_drivers[] = {
.name   = "Broadcom BCM5482",
/* PHY_GBIT_FEATURES */
.config_init= bcm5482_config_init,
-   .read_status= bcm5482_read_status,
+   .read_status= bcm54xx_read_status,
.ack_interrupt  = bcm_phy_ack_intr,
.config_intr= bcm_phy_config_intr,
 }, {
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index 6db2d9a6e503..b475e7f20d28 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -200,9 +200,15 @@
 #define BCM5482_SHD_SSD0x14/* 10100: Secondary SerDes 
control */
 #define BCM5482_SHD_SSD_LEDM   0x0008  /* SSD LED Mode enable */
 #define BCM5482_SHD_SSD_EN 0x0001  /* SSD enable */
-#define BCM5482_SHD_MODE   0x1f/* 1: Mode Contr

Re: [PATCH net-next v2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 8/2/19 7:50 AM, Andrew Lunn wrote:
>> +static int bcm54616s_read_status(struct phy_device *phydev)
>> +{
>> +int err;
>> +
>> +err = genphy_read_status(phydev);
>> +
>> +/* 1000Base-X register set doesn't provide speed fields: the
>> + * link speed is always 1000 Mb/s as long as link is up.
>> + */
>> +if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX &&
>> +phydev->link)
>> +phydev->speed = SPEED_1000;
>> +
>> +return err;
>> +}
> 
> This function is equivalent to bcm5482_read_status(). You should use
> it, rather than add a new function.

Thank you for pointing it out. Will fix the code.

BTW, should I update the patch subject to something more descriptive (such as 
"net: phy: broadcom: fix BCM54616S read_status in 1000X mode")? Or I should use 
the same title to avoid confusion?


Thanks,

Tao

Re: [PATCH v2] ARM: dts: aspeed: Add Facebook Wedge100 BMC

[Tao Ren]

On 8/1/19, 10:26 PM, "Joel Stanley"  wrote:

>  I've applied both of these to the aspeed tree for 5.4.

Thank you Joel.

Cheers,

Tao

Re: [PATCH v2] ARM: dts: aspeed: Add Facebook Wedge100 BMC

[Tao Ren]

On 8/1/19, 9:21 PM, "Joel Stanley"  wrote:

>  On Fri, 2 Aug 2019 at 04:10, Tao Ren  wrote:
>>
>> Add initial version of device tree for Facebook Wedge100 AST2400 BMC
>> platform.
>>
>> Signed-off-by: Tao Ren 
>> Reviewed-by: Andrew Jeffery 
>> ---
>>  Changes in v2:
>>  - remove "debug" from bootargs.
>
> Thanks. I applied wedge40 and then this one fails to apply due to
> conflicts in the Makefile. Next time you have two patches, send them
> as a series they apply one atop the other.

I thought about asking you if I should send them as a series although they are 
logically independent patches..
Sorry about that and I will do so for future patches.

>  The naming of these two files suggests they come from a family. I
>  noticed there's very minor differences, a pca9548 switch and the use
>  of a watchdog.
>  
>  Are these device trees complete? If yes, do you think it's worthwhile
>  to have a common wedge description in eg.
>  aspeed-bmc-facebook-wedge.dtsi, and put the unique description in
>  respective dts board files?
>   
>  The upside of this is reduced duplication.
>  
>  If you have a reason not to, then that is okay and we can leave it as
>  you submitted them.

Thank you for the suggestion. I'm also considering moving common stuff into 
"dtsi" file, but let me take care of it in a separate patch, mainly because:
  1) I have one more BMC platform (galaxy100) which is also similar to wedge.
  I haven't started the platform, but once I have galaxy100 device tree 
ready, it would be easier for me to extract common part.
  2) the device tree is not complete yet.
  For example, all the i2c devices are still created from userspace.
  I'm trying to move the logic from userspace to device tree but I haven't 
decided what to do with those cpld/fpga devices.


Cheers,

Tao

[PATCH v2] ARM: dts: aspeed: Add Facebook Wedge100 BMC

[Tao Ren]

Add initial version of device tree for Facebook Wedge100 AST2400 BMC
platform.

Signed-off-by: Tao Ren 
Reviewed-by: Andrew Jeffery 
---
 Changes in v2:
 - remove "debug" from bootargs.

 arch/arm/boot/dts/Makefile|   1 +
 .../boot/dts/aspeed-bmc-facebook-wedge100.dts | 149 ++
 2 files changed, 150 insertions(+)
 create mode 100644 arch/arm/boot/dts/aspeed-bmc-facebook-wedge100.dts

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 39a05a10a2a2..d71504ed82d3 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -1273,6 +1273,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
aspeed-bmc-facebook-cmm.dtb \
aspeed-bmc-facebook-minipack.dtb \
aspeed-bmc-facebook-tiogapass.dtb \
+   aspeed-bmc-facebook-wedge100.dtb \
aspeed-bmc-facebook-yamp.dtb \
aspeed-bmc-intel-s2600wf.dtb \
aspeed-bmc-inspur-fp5280g2.dtb \
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-wedge100.dts 
b/arch/arm/boot/dts/aspeed-bmc-facebook-wedge100.dts
new file mode 100644
index ..b1e10f0c85c9
--- /dev/null
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-wedge100.dts
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Copyright (c) 2018 Facebook Inc.
+/dts-v1/;
+
+#include "aspeed-g4.dtsi"
+
+/ {
+   model = "Facebook Wedge 100 BMC";
+   compatible = "facebook,wedge100-bmc", "aspeed,ast2400";
+
+   aliases {
+   /*
+* Override the default uart aliases to avoid breaking
+* the legacy applications.
+*/
+   serial0 = 
+   serial1 = 
+   serial2 = 
+   serial3 = 
+   };
+
+   chosen {
+   stdout-path = 
+   bootargs = "console=ttyS2,9600n8 root=/dev/ram rw";
+   };
+
+   memory@4000 {
+   reg = <0x4000 0x2000>;
+   };
+};
+
+ {
+   status = "okay";
+   aspeed,reset-type = "system";
+};
+
+ {
+   status = "okay";
+   aspeed,reset-type = "system";
+};
+
+ {
+   status = "okay";
+   flash@0 {
+   status = "okay";
+   m25p,fast-read;
+   label = "fmc0";
+#include "facebook-bmc-flash-layout.dtsi"
+   };
+};
+
+ {
+   status = "okay";
+   pinctrl-names = "default";
+   pinctrl-0 = <_txd1_default
+_rxd1_default>;
+};
+
+ {
+   status = "okay";
+   pinctrl-names = "default";
+   pinctrl-0 = <_txd3_default
+_rxd3_default>;
+};
+
+ {
+   status = "okay";
+   pinctrl-names = "default";
+   pinctrl-0 = <_txd4_default
+_rxd4_default>;
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+   no-hw-checksum;
+   pinctrl-names = "default";
+   pinctrl-0 = <_rgmii2_default _mdio2_default>;
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+
+   i2c-switch@70 {
+   compatible = "nxp,pca9548";
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <0x70>;
+   };
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
-- 
2.17.1

[PATCH v2] ARM: dts: aspeed: Add Facebook Wedge40 BMC

[Tao Ren]

Add initial version of device tree for Facebook Wedge40 AST2400 BMC
platform.

Signed-off-by: Tao Ren 
Reviewed-by: Andrew Jeffery 
---
 Changes in v2:
 - remove "debug" from bootargs.

 arch/arm/boot/dts/Makefile|   1 +
 .../boot/dts/aspeed-bmc-facebook-wedge40.dts  | 141 ++
 2 files changed, 142 insertions(+)
 create mode 100644 arch/arm/boot/dts/aspeed-bmc-facebook-wedge40.dts

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 39a05a10a2a2..dfc1011eb3f2 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -1273,6 +1273,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
aspeed-bmc-facebook-cmm.dtb \
aspeed-bmc-facebook-minipack.dtb \
aspeed-bmc-facebook-tiogapass.dtb \
+   aspeed-bmc-facebook-wedge40.dtb \
aspeed-bmc-facebook-yamp.dtb \
aspeed-bmc-intel-s2600wf.dtb \
aspeed-bmc-inspur-fp5280g2.dtb \
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-wedge40.dts 
b/arch/arm/boot/dts/aspeed-bmc-facebook-wedge40.dts
new file mode 100644
index ..aaa77a597d1a
--- /dev/null
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-wedge40.dts
@@ -0,0 +1,141 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Copyright (c) 2018 Facebook Inc.
+/dts-v1/;
+
+#include "aspeed-g4.dtsi"
+
+/ {
+   model = "Facebook Wedge 40 BMC";
+   compatible = "facebook,wedge40-bmc", "aspeed,ast2400";
+
+   aliases {
+   /*
+* Override the default uart aliases to avoid breaking
+* the legacy applications.
+*/
+   serial0 = 
+   serial1 = 
+   serial2 = 
+   serial3 = 
+   };
+
+   chosen {
+   stdout-path = 
+   bootargs = "console=ttyS2,9600n8 root=/dev/ram rw";
+   };
+
+   memory@4000 {
+   reg = <0x4000 0x2000>;
+   };
+};
+
+ {
+   status = "okay";
+   aspeed,reset-type = "system";
+};
+
+ {
+   status = "disabled";
+};
+
+ {
+   status = "okay";
+   flash@0 {
+   status = "okay";
+   m25p,fast-read;
+   label = "fmc0";
+#include "facebook-bmc-flash-layout.dtsi"
+   };
+};
+
+ {
+   status = "okay";
+   pinctrl-names = "default";
+   pinctrl-0 = <_txd1_default
+_rxd1_default>;
+};
+
+ {
+   status = "okay";
+   pinctrl-names = "default";
+   pinctrl-0 = <_txd3_default
+_rxd3_default>;
+};
+
+ {
+   status = "okay";
+   pinctrl-names = "default";
+   pinctrl-0 = <_txd4_default
+_rxd4_default>;
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+   no-hw-checksum;
+   pinctrl-names = "default";
+   pinctrl-0 = <_rgmii2_default _mdio2_default>;
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
-- 
2.17.1

Re: [PATCH] ARM: dts: aspeed: Add Facebook Wedge40 BMC

[Tao Ren]

On 8/1/19, 7:56 PM, "Andrew Jeffery"  wrote:

>On Fri, 2 Aug 2019, at 10:24, Tao Ren wrote: 
>> Add initial version of device tree for Facebook Wedge40 AST2400 BMC
>> platform.
>> 
>> Signed-off-by: Tao Ren 
>  
>  Reviewed-by: Andrew Jeffery 

Thank you Andrew for the quick review (on both patches)!

Cheers,

Tao
 

Re: [PATCH] ARM: dts: aspeed: Add Facebook Wedge100 BMC

[Tao Ren]

On 8/1/19, 8:02 PM, "Joel Stanley"  wrote:

>  On Fri, 2 Aug 2019 at 01:02, Tao Ren  wrote:
>> +
>> +   chosen {
>> +   stdout-path = 
>> +   bootargs = "debug console=ttyS2,9600n8 root=/dev/ram rw";
>  
>  Are you sure you want 'debug' in your boot arguments?
> 
> The rest lgtm. I can remove debug when applying, or leave it there if
> it was intentional.

Ahh, I copied bootargs from "/proc/cmdline" on my machine (running old kernel) 
but I don't think I need it.

Thank you for pointing it out. Let me send out v2 patches in a few minutes (so 
you could apply without extra changes).


Cheers,

Tao

Re: [PATCH net-next 1/2] net: phy: broadcom: set features explicitly for BCM54616S

[Tao Ren]

On 7/31/19 10:20 PM, Tao Ren wrote:
> On 7/30/19 11:00 PM, Tao Ren wrote:
>> On 7/30/19 10:53 PM, Heiner Kallweit wrote:
>>> On 31.07.2019 02:12, Tao Ren wrote:
>>>> On 7/29/19 11:00 PM, Heiner Kallweit wrote:
>>>>> On 30.07.2019 07:05, Tao Ren wrote:
>>>>>> On 7/29/19 8:35 PM, Andrew Lunn wrote:
>>>>>>> On Mon, Jul 29, 2019 at 05:25:32PM -0700, Tao Ren wrote:
>>>>>>>> BCM54616S feature "PHY_GBIT_FEATURES" was removed by commit 
>>>>>>>> dcdecdcfe1fc
>>>>>>>> ("net: phy: switch drivers to use dynamic feature detection"). As 
>>>>>>>> dynamic
>>>>>>>> feature detection doesn't work when BCM54616S is working in RGMII-Fiber
>>>>>>>> mode (different sets of MII Control/Status registers being used), let's
>>>>>>>> set "PHY_GBIT_FEATURES" for BCM54616S explicitly.
>>>>>>>
>>>>>>> Hi Tao
>>>>>>>
>>>>>>> What exactly does it get wrong?
>>>>>>>
>>>>>>>  Thanks
>>>>>>> Andrew
>>>>>>
>>>>>> Hi Andrew,
>>>>>>
>>>>>> BCM54616S is set to RGMII-Fiber (1000Base-X) mode on my platform, and 
>>>>>> none of the features (1000BaseT/100BaseT/10BaseT) can be detected by 
>>>>>> genphy_read_abilities(), because the PHY only reports 
>>>>>> 1000BaseX_Full|Half ability in this mode.
>>>>>>
>>>>> Are you going to use the PHY in copper or fibre mode?
>>>>> In case you use fibre mode, why do you need the copper modes set as 
>>>>> supported?
>>>>> Or does the PHY just start in fibre mode and you want to switch it to 
>>>>> copper mode?
>>>>
>>>> Hi Heiner,
>>>>
>>>> The phy starts in fiber mode and that's the mode I want.
>>>> My observation is: phydev->link is always 0 (Link status bit is never set 
>>>> in MII_BMSR) by using dynamic ability detection on my machine. I checked 
>>>> phydev->supported and it's set to "AutoNeg | TP | MII | Pause | 
>>>> Asym_Pause" by dynamic ability detection. Is it normal/expected? Or maybe 
>>>> the fix should go to different places? Thank you for your help.
>>>>
>>>
>>> Not sure whether you stated already which kernel version you're using.
>>> There's a brand-new extension to auto-detect 1000BaseX:
>>> f30e33bcdab9 ("net: phy: Add more 1000BaseX support detection")
>>> It's included in the 5.3-rc series.
>>
>> I'm running kernel 5.2.0. Thank you for the sharing and I didn't know the 
>> patch. Let me check it out.
> 
> I applied above patch and ca72efb6bdc7 ("net: phy: Add detection of 1000BaseX 
> link mode support") to my 5.2.0 tree but got following warning when booting 
> up my machine:
> 
> "PHY advertising (0,0200,62c0) more modes than genphy supports, some 
> modes not advertised".
> 
> The BCM54616S PHY on my machine only reports 1000-X features in 
> RGMII->1000Base-KX mode. Is it a known problem?
> 
> Anyways let me see if I missed some dependency/follow-up patches..

Let's ignore the patch ("net: phy: broadcom: set features explicitly for 
BCM54616S"): as Heiner pointed out, it doesn't make sense to turn on copper 
features for fiber mode (even though it "works" in my environment). I will work 
out new patch if 1000bx-auto-detection patches cannot solve my problem.

Thank you all for spending time on this.


Cheers,

Tao

[PATCH] ARM: dts: aspeed: Add Facebook Wedge100 BMC

[Tao Ren]

Add initial version of device tree for Facebook Wedge100 AST2400 BMC
platform.

Signed-off-by: Tao Ren 
---
 arch/arm/boot/dts/Makefile|   1 +
 .../boot/dts/aspeed-bmc-facebook-wedge100.dts | 149 ++
 2 files changed, 150 insertions(+)
 create mode 100644 arch/arm/boot/dts/aspeed-bmc-facebook-wedge100.dts

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 39a05a10a2a2..d71504ed82d3 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -1273,6 +1273,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
aspeed-bmc-facebook-cmm.dtb \
aspeed-bmc-facebook-minipack.dtb \
aspeed-bmc-facebook-tiogapass.dtb \
+   aspeed-bmc-facebook-wedge100.dtb \
aspeed-bmc-facebook-yamp.dtb \
aspeed-bmc-intel-s2600wf.dtb \
aspeed-bmc-inspur-fp5280g2.dtb \
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-wedge100.dts 
b/arch/arm/boot/dts/aspeed-bmc-facebook-wedge100.dts
new file mode 100644
index ..ccd700467ea7
--- /dev/null
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-wedge100.dts
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Copyright (c) 2018 Facebook Inc.
+/dts-v1/;
+
+#include "aspeed-g4.dtsi"
+
+/ {
+   model = "Facebook Wedge 100 BMC";
+   compatible = "facebook,wedge100-bmc", "aspeed,ast2400";
+
+   aliases {
+   /*
+* Override the default uart aliases to avoid breaking
+* the legacy applications.
+*/
+   serial0 = 
+   serial1 = 
+   serial2 = 
+   serial3 = 
+   };
+
+   chosen {
+   stdout-path = 
+   bootargs = "debug console=ttyS2,9600n8 root=/dev/ram rw";
+   };
+
+   memory@4000 {
+   reg = <0x4000 0x2000>;
+   };
+};
+
+ {
+   status = "okay";
+   aspeed,reset-type = "system";
+};
+
+ {
+   status = "okay";
+   aspeed,reset-type = "system";
+};
+
+ {
+   status = "okay";
+   flash@0 {
+   status = "okay";
+   m25p,fast-read;
+   label = "fmc0";
+#include "facebook-bmc-flash-layout.dtsi"
+   };
+};
+
+ {
+   status = "okay";
+   pinctrl-names = "default";
+   pinctrl-0 = <_txd1_default
+_rxd1_default>;
+};
+
+ {
+   status = "okay";
+   pinctrl-names = "default";
+   pinctrl-0 = <_txd3_default
+_rxd3_default>;
+};
+
+ {
+   status = "okay";
+   pinctrl-names = "default";
+   pinctrl-0 = <_txd4_default
+_rxd4_default>;
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+   no-hw-checksum;
+   pinctrl-names = "default";
+   pinctrl-0 = <_rgmii2_default _mdio2_default>;
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+
+   i2c-switch@70 {
+   compatible = "nxp,pca9548";
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <0x70>;
+   };
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
-- 
2.17.1

[PATCH] ARM: dts: aspeed: Add Facebook Wedge40 BMC

[Tao Ren]

Add initial version of device tree for Facebook Wedge40 AST2400 BMC
platform.

Signed-off-by: Tao Ren 
---
 arch/arm/boot/dts/Makefile|   1 +
 .../boot/dts/aspeed-bmc-facebook-wedge40.dts  | 141 ++
 2 files changed, 142 insertions(+)
 create mode 100644 arch/arm/boot/dts/aspeed-bmc-facebook-wedge40.dts

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 39a05a10a2a2..dfc1011eb3f2 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -1273,6 +1273,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
aspeed-bmc-facebook-cmm.dtb \
aspeed-bmc-facebook-minipack.dtb \
aspeed-bmc-facebook-tiogapass.dtb \
+   aspeed-bmc-facebook-wedge40.dtb \
aspeed-bmc-facebook-yamp.dtb \
aspeed-bmc-intel-s2600wf.dtb \
aspeed-bmc-inspur-fp5280g2.dtb \
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-wedge40.dts 
b/arch/arm/boot/dts/aspeed-bmc-facebook-wedge40.dts
new file mode 100644
index ..764633964ac1
--- /dev/null
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-wedge40.dts
@@ -0,0 +1,141 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Copyright (c) 2018 Facebook Inc.
+/dts-v1/;
+
+#include "aspeed-g4.dtsi"
+
+/ {
+   model = "Facebook Wedge 40 BMC";
+   compatible = "facebook,wedge40-bmc", "aspeed,ast2400";
+
+   aliases {
+   /*
+* Override the default uart aliases to avoid breaking
+* the legacy applications.
+*/
+   serial0 = 
+   serial1 = 
+   serial2 = 
+   serial3 = 
+   };
+
+   chosen {
+   stdout-path = 
+   bootargs = "debug console=ttyS2,9600n8 root=/dev/ram rw";
+   };
+
+   memory@4000 {
+   reg = <0x4000 0x2000>;
+   };
+};
+
+ {
+   status = "okay";
+   aspeed,reset-type = "system";
+};
+
+ {
+   status = "disabled";
+};
+
+ {
+   status = "okay";
+   flash@0 {
+   status = "okay";
+   m25p,fast-read;
+   label = "fmc0";
+#include "facebook-bmc-flash-layout.dtsi"
+   };
+};
+
+ {
+   status = "okay";
+   pinctrl-names = "default";
+   pinctrl-0 = <_txd1_default
+_rxd1_default>;
+};
+
+ {
+   status = "okay";
+   pinctrl-names = "default";
+   pinctrl-0 = <_txd3_default
+_rxd3_default>;
+};
+
+ {
+   status = "okay";
+   pinctrl-names = "default";
+   pinctrl-0 = <_txd4_default
+_rxd4_default>;
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+   no-hw-checksum;
+   pinctrl-names = "default";
+   pinctrl-0 = <_rgmii2_default _mdio2_default>;
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
-- 
2.17.1

[PATCH net-next v2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

genphy_read_status() cannot report correct link speed when BCM54616S PHY
is configured in RGMII->1000Base-KX mode (for example, on Facebook CMM
BMC platform), and it is because speed-related fields in MII registers
are assigned different meanings in 1000X register set. Actually there
is no speed field in 1000X register set because link speed is always
1000 Mb/s.

The patch adds "probe" callback to detect PHY's operation mode based on
INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
Control register. Besides, link speed is manually set to 1000 Mb/s in
"read_status" callback if PHY-switch link is 1000Base-X.

Signed-off-by: Tao Ren 
---
 Changes in v2:
  - Auto-detect PHY operation mode instead of passing DT node.
  - move PHY mode auto-detect logic from config_init to probe callback.
  - only set speed (not including duplex) in read_status callback.
  - update patch description with more background to avoid confusion.
  - patch #1 in the series ("net: phy: broadcom: set features explicitly
for BCM54616") is dropped: the fix should go to get_features callback
which may potentially depend on this patch.

 drivers/net/phy/broadcom.c | 53 +++---
 include/linux/brcmphy.h| 10 +--
 2 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 937d0059e8ac..b6debbc6054e 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device *phydev)
/*
 * Select 1000BASE-X register set (primary SerDes)
 */
-   reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
-   bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
-reg | BCM5482_SHD_MODE_1000BX);
+   reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
+   bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
+reg | BCM54XX_SHD_MODE_1000BX);
 
/*
 * LED1=ACTIVITYLED, LED3=LINKSPD[2]
@@ -464,6 +464,51 @@ static int bcm54616s_config_aneg(struct phy_device *phydev)
return ret;
 }
 
+static int bcm54616s_probe(struct phy_device *phydev)
+{
+   int val, intf_sel;
+
+   val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
+   if (val < 0)
+   return val;
+
+   /* The PHY is strapped in RGMII to fiber mode when INTERF_SEL[1:0]
+* is 01b.
+*/
+   intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1;
+   if (intf_sel == 1) {
+   val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL);
+   if (val < 0)
+   return val;
+
+   /* Bit 0 of the SerDes 100-FX Control register, when set
+* to 1, sets the MII/RGMII -> 100BASE-FX configuration.
+* When this bit is set to 0, it sets the GMII/RGMII ->
+* 1000BASE-X configuration.
+*/
+   if (!(val & BCM54616S_100FX_MODE))
+   phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX;
+   }
+
+   return 0;
+}
+
+static int bcm54616s_read_status(struct phy_device *phydev)
+{
+   int err;
+
+   err = genphy_read_status(phydev);
+
+   /* 1000Base-X register set doesn't provide speed fields: the
+* link speed is always 1000 Mb/s as long as link is up.
+*/
+   if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX &&
+   phydev->link)
+   phydev->speed = SPEED_1000;
+
+   return err;
+}
+
 static int brcm_phy_setbits(struct phy_device *phydev, int reg, int set)
 {
int val;
@@ -655,6 +700,8 @@ static struct phy_driver broadcom_drivers[] = {
.config_aneg= bcm54616s_config_aneg,
.ack_interrupt  = bcm_phy_ack_intr,
.config_intr= bcm_phy_config_intr,
+   .read_status= bcm54616s_read_status,
+   .probe  = bcm54616s_probe,
 }, {
.phy_id = PHY_ID_BCM5464,
.phy_id_mask= 0xfff0,
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index 6db2d9a6e503..b475e7f20d28 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -200,9 +200,15 @@
 #define BCM5482_SHD_SSD0x14/* 10100: Secondary SerDes 
control */
 #define BCM5482_SHD_SSD_LEDM   0x0008  /* SSD LED Mode enable */
 #define BCM5482_SHD_SSD_EN 0x0001  /* SSD enable */
-#define BCM5482_SHD_MODE   0x1f/* 1: Mode Control Register */
-#define BCM5482_SHD_MODE_1000BX0x0001  /* Enable 1000BASE-X registers 
*/
 
+/* 10011: SerDes 100-FX Control Register */
+#define BCM54616S_SHD_100FX_CTRL   0x13
+#defineBCM54616S_100FX_MODEBIT(0)  /* 100-FX SerDes Enable 
*/
+
+/* 1: Mode Control Register

Re: [PATCH net-next 1/2] net: phy: broadcom: set features explicitly for BCM54616S

[Tao Ren]

On 7/30/19 11:00 PM, Tao Ren wrote:
> On 7/30/19 10:53 PM, Heiner Kallweit wrote:
>> On 31.07.2019 02:12, Tao Ren wrote:
>>> On 7/29/19 11:00 PM, Heiner Kallweit wrote:
>>>> On 30.07.2019 07:05, Tao Ren wrote:
>>>>> On 7/29/19 8:35 PM, Andrew Lunn wrote:
>>>>>> On Mon, Jul 29, 2019 at 05:25:32PM -0700, Tao Ren wrote:
>>>>>>> BCM54616S feature "PHY_GBIT_FEATURES" was removed by commit dcdecdcfe1fc
>>>>>>> ("net: phy: switch drivers to use dynamic feature detection"). As 
>>>>>>> dynamic
>>>>>>> feature detection doesn't work when BCM54616S is working in RGMII-Fiber
>>>>>>> mode (different sets of MII Control/Status registers being used), let's
>>>>>>> set "PHY_GBIT_FEATURES" for BCM54616S explicitly.
>>>>>>
>>>>>> Hi Tao
>>>>>>
>>>>>> What exactly does it get wrong?
>>>>>>
>>>>>>  Thanks
>>>>>>  Andrew
>>>>>
>>>>> Hi Andrew,
>>>>>
>>>>> BCM54616S is set to RGMII-Fiber (1000Base-X) mode on my platform, and 
>>>>> none of the features (1000BaseT/100BaseT/10BaseT) can be detected by 
>>>>> genphy_read_abilities(), because the PHY only reports 1000BaseX_Full|Half 
>>>>> ability in this mode.
>>>>>
>>>> Are you going to use the PHY in copper or fibre mode?
>>>> In case you use fibre mode, why do you need the copper modes set as 
>>>> supported?
>>>> Or does the PHY just start in fibre mode and you want to switch it to 
>>>> copper mode?
>>>
>>> Hi Heiner,
>>>
>>> The phy starts in fiber mode and that's the mode I want.
>>> My observation is: phydev->link is always 0 (Link status bit is never set 
>>> in MII_BMSR) by using dynamic ability detection on my machine. I checked 
>>> phydev->supported and it's set to "AutoNeg | TP | MII | Pause | Asym_Pause" 
>>> by dynamic ability detection. Is it normal/expected? Or maybe the fix 
>>> should go to different places? Thank you for your help.
>>>
>>
>> Not sure whether you stated already which kernel version you're using.
>> There's a brand-new extension to auto-detect 1000BaseX:
>> f30e33bcdab9 ("net: phy: Add more 1000BaseX support detection")
>> It's included in the 5.3-rc series.
> 
> I'm running kernel 5.2.0. Thank you for the sharing and I didn't know the 
> patch. Let me check it out.

I applied above patch and ca72efb6bdc7 ("net: phy: Add detection of 1000BaseX 
link mode support") to my 5.2.0 tree but got following warning when booting up 
my machine:

"PHY advertising (0,0200,62c0) more modes than genphy supports, some 
modes not advertised".

The BCM54616S PHY on my machine only reports 1000-X features in 
RGMII->1000Base-KX mode. Is it a known problem?

Anyways let me see if I missed some dependency/follow-up patches..


Cheers,

Tao

Re: [PATCH net-next 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 7/30/19 10:55 PM, Tao Ren wrote:
> On 7/30/19 7:34 PM, Andrew Lunn wrote:
>>> Hi Andrew,
>>>
>>> The BCM54616S PHY on my machine is connected to a BCM5396 switch chip over 
>>> backplane (1000Base-KX).
>>
>> Ah, that is different. So the board is using it for RGMII to 1000Base-KX?
>>
>> phy-mode is about the MAC-PHY link. So in this case RGMII.
> 
> Yes. It's RGMII to 1000Base-KX.
> 
>> There is no DT way to configure the PHY-Switch link. However, it
>> sounds like you have the PHY strapped so it is doing 1000BaseX on the
>> PHY-Switch link. So do you actually need to configure this?
> 
> The PHY is strapped in RGMII-Fiber Mode (the term used in datasheet), but 
> besides 1000BaseX, 100Base-FX is also supported in this mode.
> The datasheet doesn't say which link type (1000BaseX or 100Base-FX) is active 
> after reset and I cannot find a way to auto-detect the link type, either.

I found bit 0 of 100-FX control register can be used to detect PHY-switch link 
type (means DT is not needed). Will run more testing and send out v2 patch 
soon. Thank you all for the input and help.


Cheers,

Tao

Re: [PATCH net-next 1/2] net: phy: broadcom: set features explicitly for BCM54616S

[Tao Ren]

On 7/30/19 10:53 PM, Heiner Kallweit wrote:
> On 31.07.2019 02:12, Tao Ren wrote:
>> On 7/29/19 11:00 PM, Heiner Kallweit wrote:
>>> On 30.07.2019 07:05, Tao Ren wrote:
>>>> On 7/29/19 8:35 PM, Andrew Lunn wrote:
>>>>> On Mon, Jul 29, 2019 at 05:25:32PM -0700, Tao Ren wrote:
>>>>>> BCM54616S feature "PHY_GBIT_FEATURES" was removed by commit dcdecdcfe1fc
>>>>>> ("net: phy: switch drivers to use dynamic feature detection"). As dynamic
>>>>>> feature detection doesn't work when BCM54616S is working in RGMII-Fiber
>>>>>> mode (different sets of MII Control/Status registers being used), let's
>>>>>> set "PHY_GBIT_FEATURES" for BCM54616S explicitly.
>>>>>
>>>>> Hi Tao
>>>>>
>>>>> What exactly does it get wrong?
>>>>>
>>>>>  Thanks
>>>>>   Andrew
>>>>
>>>> Hi Andrew,
>>>>
>>>> BCM54616S is set to RGMII-Fiber (1000Base-X) mode on my platform, and none 
>>>> of the features (1000BaseT/100BaseT/10BaseT) can be detected by 
>>>> genphy_read_abilities(), because the PHY only reports 1000BaseX_Full|Half 
>>>> ability in this mode.
>>>>
>>> Are you going to use the PHY in copper or fibre mode?
>>> In case you use fibre mode, why do you need the copper modes set as 
>>> supported?
>>> Or does the PHY just start in fibre mode and you want to switch it to 
>>> copper mode?
>>
>> Hi Heiner,
>>
>> The phy starts in fiber mode and that's the mode I want.
>> My observation is: phydev->link is always 0 (Link status bit is never set in 
>> MII_BMSR) by using dynamic ability detection on my machine. I checked 
>> phydev->supported and it's set to "AutoNeg | TP | MII | Pause | Asym_Pause" 
>> by dynamic ability detection. Is it normal/expected? Or maybe the fix should 
>> go to different places? Thank you for your help.
>>
> 
> Not sure whether you stated already which kernel version you're using.
> There's a brand-new extension to auto-detect 1000BaseX:
> f30e33bcdab9 ("net: phy: Add more 1000BaseX support detection")
> It's included in the 5.3-rc series.

I'm running kernel 5.2.0. Thank you for the sharing and I didn't know the 
patch. Let me check it out.

> If a feature can be read from a vendor-specific register only,
> then the preferred way is: Implement callback get_features in
> the PHY driver, call genphy_read_abilities for the basic features
> and complement it with reading the vendor-specific register(s).

Got it. Let me update my code and will come back soon.


Thanks,

Tao

Re: [PATCH net-next 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 7/30/19 7:34 PM, Andrew Lunn wrote:
>> Hi Andrew,
>>
>> The BCM54616S PHY on my machine is connected to a BCM5396 switch chip over 
>> backplane (1000Base-KX).
> 
> Ah, that is different. So the board is using it for RGMII to 1000Base-KX?
> 
> phy-mode is about the MAC-PHY link. So in this case RGMII.

Yes. It's RGMII to 1000Base-KX.

> There is no DT way to configure the PHY-Switch link. However, it
> sounds like you have the PHY strapped so it is doing 1000BaseX on the
> PHY-Switch link. So do you actually need to configure this?

The PHY is strapped in RGMII-Fiber Mode (the term used in datasheet), but 
besides 1000BaseX, 100Base-FX is also supported in this mode.
The datasheet doesn't say which link type (1000BaseX or 100Base-FX) is active 
after reset and I cannot find a way to auto-detect the link type, either.

Below are a few more details about 1000Base-X and 100Base-FX in BCM54616S 
datasheet.

- 1000BaseX: 
  The 1000BaseX register set (MII registers 00-0F) needs to be enabled by 
setting bit 0 of Mode Control Register.
  Although the register address is the same between 1000BaseX and 
1000BaseT/100BaseT/10BaseT registers, some bit fields in 1000BaseX registers 
are different: for example, speed field is not available in 1000BaseX status 
register.

- 100Base-FX:
  100Base-FX registers need to be enabled by writing 1 to SerDes 100FX Control 
Register.
  100Base-FX Control and Status registers are in shadow register 1Ch, instead 
of MII register 00h and 01h.

> You report you never see link up? So maybe the problem is actually in
> read_status? When in 1000BaseX mode, do you need to read the link
> status from some other register? The Marvell PHYs use a second page
> for 1000BaseX.

read_status callback needs to be updated to report correct link speed in my 
case. But as I cannot tell which link type (1000BaseX or 100Base-FX) is active, 
it becomes hard to access registers in read_status method. Any suggestions?

BTW, link-never-up issue seems to be caused by static/dynamic feature 
detection. I'm still tracing down the issue and it's being tracked in patch #1 
of the series.


Thanks,

Tao

Re: [PATCH net-next 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 7/30/19 6:36 PM, Andrew Lunn wrote:
>> The INTF_SEL pins report correct mode (RGMII-Fiber) on my machine,
>> but there are 2 "sub-modes" (1000Base-X and 100Base-FX) and I
>> couldn't find a proper/safe way to auto-detect which "sub-mode" is
>> active. The datasheet just describes instructions to enable a
>> specific mode, but it doesn't say 1000Base-X/100Base-FX mode will be
>> auto-selected. And that's why I came up with the patch to specify
>> 1000Base-X mode.
> 
> Fibre does not perform any sort of auto-negotiation. I assume you have
> an SFP connected? When using PHYLINK, the sfp driver will get the
> supported baud rate from SFP EEPROM to determine what speed could be
> used. However, there is currently no mainline support for having a
> chain MAC-PHY-SFP. For that you need Russells out of tree patches.

Hi Andrew,

The BCM54616S PHY on my machine is connected to a BCM5396 switch chip over 
backplane (1000Base-KX).


Thanks,

Tao

Re: [PATCH net-next 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 7/30/19 6:17 AM, Andrew Lunn wrote:
>> Again, I don't think Linux has generic support for overwriting (or
>> even describing) the operating mode of a PHY, although maybe that's a
>> direction we would want to push the discussion towards. RGMII to
>> copper, RGMII to fiber, SGMII to copper, copper to fiber (media
>> converter), even RGMII to SGMII (RTL8211FS supports this) - lots of
>> modes, and this is only for gigabit PHYs...
> 
> This is something Russell King has PHYLINK patches for, which have not
> yet been merged. There are some boards which use a PHY as a media
> converter, placed between the MAC and an SFP.

Thank you Andrew. Let me see if the operating mode can be auto-detected on 
BCM54616S chip, and I will update back soon.

Thanks,

Tao

Re: [PATCH net-next 1/2] net: phy: broadcom: set features explicitly for BCM54616S

[Tao Ren]

On 7/29/19 11:00 PM, Heiner Kallweit wrote:
> On 30.07.2019 07:05, Tao Ren wrote:
>> On 7/29/19 8:35 PM, Andrew Lunn wrote:
>>> On Mon, Jul 29, 2019 at 05:25:32PM -0700, Tao Ren wrote:
>>>> BCM54616S feature "PHY_GBIT_FEATURES" was removed by commit dcdecdcfe1fc
>>>> ("net: phy: switch drivers to use dynamic feature detection"). As dynamic
>>>> feature detection doesn't work when BCM54616S is working in RGMII-Fiber
>>>> mode (different sets of MII Control/Status registers being used), let's
>>>> set "PHY_GBIT_FEATURES" for BCM54616S explicitly.
>>>
>>> Hi Tao
>>>
>>> What exactly does it get wrong?
>>>
>>>  Thanks
>>> Andrew
>>
>> Hi Andrew,
>>
>> BCM54616S is set to RGMII-Fiber (1000Base-X) mode on my platform, and none 
>> of the features (1000BaseT/100BaseT/10BaseT) can be detected by 
>> genphy_read_abilities(), because the PHY only reports 1000BaseX_Full|Half 
>> ability in this mode.
>>
> Are you going to use the PHY in copper or fibre mode?
> In case you use fibre mode, why do you need the copper modes set as supported?
> Or does the PHY just start in fibre mode and you want to switch it to copper 
> mode?

Hi Heiner,

The phy starts in fiber mode and that's the mode I want.
My observation is: phydev->link is always 0 (Link status bit is never set in 
MII_BMSR) by using dynamic ability detection on my machine. I checked 
phydev->supported and it's set to "AutoNeg | TP | MII | Pause | Asym_Pause" by 
dynamic ability detection. Is it normal/expected? Or maybe the fix should go to 
different places? Thank you for your help.


Thanks,

Tao

Re: [PATCH net-next 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 7/30/19 3:15 AM, Vladimir Oltean wrote:
> On Tue, 30 Jul 2019 at 07:52, Tao Ren  wrote:
>>
>> On 7/29/19 6:32 PM, Vladimir Oltean wrote:
>>> Hi Tao,
>>>
>>> On Tue, 30 Jul 2019 at 03:31, Tao Ren  wrote:
>>>>
>>>> Configure the BCM54616S for 1000Base-X mode when "brcm-phy-mode-1000bx"
>>>> is set in device tree. This is needed when the PHY is used for fiber and
>>>> backplane connections.
>>>>
>>>> The patch is inspired by commit cd9af3dac6d1 ("PHYLIB: Add 1000Base-X
>>>> support for Broadcom bcm5482").
>>>
>>> As far as I can see, for the commit you referenced,
>>> PHY_BCM_FLAGS_MODE_1000BX is referenced from nowhere in the entire
>>> mainline kernel:
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_latest_ident_PHY-5FBCM-5FFLAGS-5FMODE-5F1000BX=DwIBaQ=5VD0RTtNlTh3ycd41b3MUw=iYElT7HC77pRZ3byVvW8ng=gy6Y-3Ylme-_GQcGF4fvOX10irgAT4xh253Weo0np38=KL__E2bvsmvUL-hBL9hUmOS5vyPQ92EMj6fEfByn8t8=
>>> (it is supposed to be put by the MAC driver in phydev->dev_flags prior
>>> to calling phy_connect). But I don't see the point to this - can't you
>>> check for phydev->interface == PHY_INTERFACE_MODE_1000BASEX?
>>> This has the advantage that no MAC driver will need to know that it's
>>> talking to a Broadcom PHY. Additionally, no custom DT bindings are
>>> needed.
>>> Also, for backplane connections you probably want 1000Base-KX which
>>> has its own AN/LT, not plain 1000Base-X.
>>
>> Thank you Vladimir for the quick review!
>> Perhaps I misunderstood the purpose of phydev->interface, and I thought it 
>> was usually used to defined the interface between MAC and PHY. For example, 
>> if I need to pass both "rgmii-id" and "1000base-x" from MAC to PHY driver, 
>> what would be the preferred way?
>>
> 
> Ohh, now I understand what you're trying to do, sorry, somehow I
> was too tired and I thought of something totally unrelated.

Thank you for spending time on the patch, and I really appreciate it.

> Let me see if I can explain: you've got the INTF_SEL pin strapping
> configured for something else (like RGMII to copper mode) and then
> you're changing the operating mode at runtime through MDIO? Is this
> intended to be for production code, or is it just some quick hack to
> fix a bad board design?

The INTF_SEL pins report correct mode (RGMII-Fiber) on my machine, but there 
are 2 "sub-modes" (1000Base-X and 100Base-FX) and I couldn't find a proper/safe 
way to auto-detect which "sub-mode" is active. The datasheet just describes 
instructions to enable a specific mode, but it doesn't say 
1000Base-X/100Base-FX mode will be auto-selected. And that's why I came up with 
the patch to specify 1000Base-X mode.

> I think what's supposed to happen (Heiner can comment) is that
> genphy_config_init will automatically read the out-of-reset PHY
> registers and figure out which link modes are supported. This includes
> the 1000Base-X media type, *if* the PHY is strapped correctly.
> But you are changing the strapping configuration too late (again, in
> .config_init), so phylib doesn't pick up the new Base-X modes. What
> happens if you do the switchover from the .probe callback of the
> driver, instead of .config_init?

I checked the 1000base-x mode control bit and it's already set on my machine 
when genphy_config_init is executed, means I'm writing the same value to the 
register.
I write the register explicitly because I'm suspecting the mode control bit is 
set by my boot loader and the value is not changed by software reset.
Let me see if I can disable net/phy in boot loader and see what happens. If the 
bit is still on, maybe we can use the bit to auto-detect sub-mode (although the 
datasheet doesn't mention it)?
Anyways, let me move the logic to .probe callback. Thank you.


> I think what got me confused was your "add support for 1000Base-X"
> commit message. If I understand correctly, you're not adding support,
> you're just forcing it.
> Again, I don't think Linux has generic support for overwriting (or
> even describing) the operating mode of a PHY, although maybe that's a
> direction we would want to push the discussion towards. RGMII to
> copper, RGMII to fiber, SGMII to copper, copper to fiber (media
> converter), even RGMII to SGMII (RTL8211FS supports this) - lots of
> modes, and this is only for gigabit PHYs...
> 
>>>> Signed-off-by: Tao Ren 
>>>> ---
>>>>  drivers/net/phy/broadcom.c | 58 +++---
>>>>  include/linux/brcmphy.h|  4 +--
>>>>  2 fi

Re: [PATCH net-next 1/2] net: phy: broadcom: set features explicitly for BCM54616S

[Tao Ren]

On 7/29/19 8:35 PM, Andrew Lunn wrote:
> On Mon, Jul 29, 2019 at 05:25:32PM -0700, Tao Ren wrote:
>> BCM54616S feature "PHY_GBIT_FEATURES" was removed by commit dcdecdcfe1fc
>> ("net: phy: switch drivers to use dynamic feature detection"). As dynamic
>> feature detection doesn't work when BCM54616S is working in RGMII-Fiber
>> mode (different sets of MII Control/Status registers being used), let's
>> set "PHY_GBIT_FEATURES" for BCM54616S explicitly.
> 
> Hi Tao
> 
> What exactly does it get wrong?
> 
>  Thanks
>   Andrew

Hi Andrew,

BCM54616S is set to RGMII-Fiber (1000Base-X) mode on my platform, and none of 
the features (1000BaseT/100BaseT/10BaseT) can be detected by 
genphy_read_abilities(), because the PHY only reports 1000BaseX_Full|Half 
ability in this mode.


Thanks,

Tao

Re: [PATCH net-next 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 7/29/19 6:32 PM, Vladimir Oltean wrote:
> Hi Tao,
> 
> On Tue, 30 Jul 2019 at 03:31, Tao Ren  wrote:
>>
>> Configure the BCM54616S for 1000Base-X mode when "brcm-phy-mode-1000bx"
>> is set in device tree. This is needed when the PHY is used for fiber and
>> backplane connections.
>>
>> The patch is inspired by commit cd9af3dac6d1 ("PHYLIB: Add 1000Base-X
>> support for Broadcom bcm5482").
> 
> As far as I can see, for the commit you referenced,
> PHY_BCM_FLAGS_MODE_1000BX is referenced from nowhere in the entire
> mainline kernel:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_latest_ident_PHY-5FBCM-5FFLAGS-5FMODE-5F1000BX=DwIBaQ=5VD0RTtNlTh3ycd41b3MUw=iYElT7HC77pRZ3byVvW8ng=gy6Y-3Ylme-_GQcGF4fvOX10irgAT4xh253Weo0np38=KL__E2bvsmvUL-hBL9hUmOS5vyPQ92EMj6fEfByn8t8=
>  
> (it is supposed to be put by the MAC driver in phydev->dev_flags prior
> to calling phy_connect). But I don't see the point to this - can't you
> check for phydev->interface == PHY_INTERFACE_MODE_1000BASEX?
> This has the advantage that no MAC driver will need to know that it's
> talking to a Broadcom PHY. Additionally, no custom DT bindings are
> needed.
> Also, for backplane connections you probably want 1000Base-KX which
> has its own AN/LT, not plain 1000Base-X.

Thank you Vladimir for the quick review!
Perhaps I misunderstood the purpose of phydev->interface, and I thought it was 
usually used to defined the interface between MAC and PHY. For example, if I 
need to pass both "rgmii-id" and "1000base-x" from MAC to PHY driver, what 
would be the preferred way?

>> Signed-off-by: Tao Ren 
>> ---
>>  drivers/net/phy/broadcom.c | 58 +++---
>>  include/linux/brcmphy.h|  4 +--
>>  2 files changed, 56 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
>> index 2b4e41a9d35a..6c22ac3a844b 100644
>> --- a/drivers/net/phy/broadcom.c
>> +++ b/drivers/net/phy/broadcom.c
>> @@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device *phydev)
>> /*
>>  * Select 1000BASE-X register set (primary SerDes)
>>  */
>> -   reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
>> -   bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
>> -reg | BCM5482_SHD_MODE_1000BX);
>> +   reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
>> +   bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
>> +reg | BCM54XX_SHD_MODE_1000BX);
>>
>> /*
>>  * LED1=ACTIVITYLED, LED3=LINKSPD[2]
>> @@ -451,6 +451,34 @@ static int bcm5481_config_aneg(struct phy_device 
>> *phydev)
>> return ret;
>>  }
>>
>> +static int bcm54616s_config_init(struct phy_device *phydev)
>> +{
>> +   int err, reg;
>> +   struct device_node *np = phydev->mdio.dev.of_node;
>> +
>> +   err = bcm54xx_config_init(phydev);
>> +
>> +   if (of_property_read_bool(np, "brcm-phy-mode-1000bx")) {
>> +   /* Select 1000BASE-X register set. */
>> +   reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
>> +   bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
>> +reg | BCM54XX_SHD_MODE_1000BX);
>> +
>> +   /* Auto-negotiation doesn't seem to work quite right
>> +* in this mode, so we disable it and force it to the
>> +* right speed/duplex setting.  Only 'link status'
>> +* is important.
>> +*/
>> +   phydev->autoneg = AUTONEG_DISABLE;
>> +   phydev->speed = SPEED_1000;
>> +   phydev->duplex = DUPLEX_FULL;
>> +
> 
> 1000Base-X AN does not include speed negotiation, so hardcoding
> SPEED_1000 is probably correct.
> What is wrong with the AN of duplex settings?

FULL_DUPLEX bit is set on my platform by default. Let me enable AN and test it 
out; will share you results tomorrow.

>> +   phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX;
>> +   }
>> +
>> +   return err;
>> +}
>> +
>>  static int bcm54616s_config_aneg(struct phy_device *phydev)
>>  {
>> int ret;
>> @@ -464,6 +492,27 @@ static int bcm54616s_config_aneg(struct phy_device 
>> *phydev)
>> return ret;
>>  }
>>
>

Re: [Potential Spoof] Re: [PATCH 1/2] i2c: aspeed: allow to customize base clock divisor

[Tao Ren]

On 6/20/19 1:13 AM, Tao Ren wrote:
> On 6/20/19 1:01 AM, Ryan Chen wrote:
>> Hello Tao,
>>  Let me more clear. When you set (3, 15, 14) the device sometimes 
>> response nack. 
>>  but when you set (4, 7, 7), the device always ack. Am I right? 
>> Ryan
> 
> Hello Ryan,
> 
> It's correct. We have seen the problem on 2 Facebook BMC platforms so far. 
> Given the other ~10 Facebook BMC platforms are still running kernel 4.1 (with 
> (4, 7, 7) settings), I'd assume more platforms will be impacted after 
> upgrading to the latest kernel.
> 
> Thank you for spending time on this!

Just heads up Ryan and I are working with ODM vendors to collect scope output; 
will update back when we have new findings. Thank you all for spending time on 
this.


Cheers,

Tao

Re: [PATCH 1/2] i2c: aspeed: allow to customize base clock divisor

[Tao Ren]

On 6/20/19 1:01 AM, Ryan Chen wrote:
> Hello Tao,
>   Let me more clear. When you set (3, 15, 14) the device sometimes 
> response nack. 
>   but when you set (4, 7, 7), the device always ack. Am I right? 
> Ryan

Hello Ryan,

It's correct. We have seen the problem on 2 Facebook BMC platforms so far. 
Given the other ~10 Facebook BMC platforms are still running kernel 4.1 (with 
(4, 7, 7) settings), I'd assume more platforms will be impacted after upgrading 
to the latest kernel.

Thank you for spending time on this!


Cheers,

Tao