RE: [PATCH v9 2/2] media: i2c: Add the ov7740 image sensor driver

[Wenyou.Yang]

Hi Sakari,

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: 2017年12月19日 17:23
> To: Wenyou Yang - A41535 
> Cc: Mauro Carvalho Chehab ; Rob Herring
> ; Mark Rutland ; linux-
> ker...@vger.kernel.org; Nicolas Ferre - M43238 ;
> devicet...@vger.kernel.org; Jonathan Corbet ; Hans Verkuil
> ; linux-arm-ker...@lists.infradead.org; Linux Media 
> Mailing List
> ; Songjun Wu 
> Subject: Re: [PATCH v9 2/2] media: i2c: Add the ov7740 image sensor driver
> 
> On Mon, Dec 11, 2017 at 09:31:46AM +0800, Wenyou Yang wrote:
> > The ov7740 (color) image sensor is a high performance VGA CMOS image
> > snesor, which supports for output formats: RAW RGB and YUV and image
> > sizes: VGA, and QVGA, CIF and any size smaller.
> >
> > Signed-off-by: Songjun Wu 
> > Signed-off-by: Wenyou Yang 
> 
> Applied with this diff:

Thank you very much.

> 
> diff --git a/drivers/media/i2c/ov7740.c b/drivers/media/i2c/ov7740.c index
> 0308ba437bbb..041a77039d70 100644
> --- a/drivers/media/i2c/ov7740.c
> +++ b/drivers/media/i2c/ov7740.c
> @@ -1,5 +1,7 @@
> -// SPDX-License-Identifier: GPL-2.0
> -// Copyright (c) 2017 Microchip Corporation.
> +/*
> + * SPDX-License-Identifier: GPL-2.0
> + * Copyright (c) 2017 Microchip Corporation.
> + */
> 
>  #include 
>  #include 
> 
> --
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi


Best Regards,
Wenyou Yang

RE: [PATCH v9 2/2] media: i2c: Add the ov7740 image sensor driver

[Wenyou.Yang]

Hi Sakari,

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: 2017年12月19日 17:23
> To: Wenyou Yang - A41535 
> Cc: Mauro Carvalho Chehab ; Rob Herring
> ; Mark Rutland ; linux-
> ker...@vger.kernel.org; Nicolas Ferre - M43238 ;
> devicet...@vger.kernel.org; Jonathan Corbet ; Hans Verkuil
> ; linux-arm-ker...@lists.infradead.org; Linux Media 
> Mailing List
> ; Songjun Wu 
> Subject: Re: [PATCH v9 2/2] media: i2c: Add the ov7740 image sensor driver
> 
> On Mon, Dec 11, 2017 at 09:31:46AM +0800, Wenyou Yang wrote:
> > The ov7740 (color) image sensor is a high performance VGA CMOS image
> > snesor, which supports for output formats: RAW RGB and YUV and image
> > sizes: VGA, and QVGA, CIF and any size smaller.
> >
> > Signed-off-by: Songjun Wu 
> > Signed-off-by: Wenyou Yang 
> 
> Applied with this diff:

Thank you very much.

> 
> diff --git a/drivers/media/i2c/ov7740.c b/drivers/media/i2c/ov7740.c index
> 0308ba437bbb..041a77039d70 100644
> --- a/drivers/media/i2c/ov7740.c
> +++ b/drivers/media/i2c/ov7740.c
> @@ -1,5 +1,7 @@
> -// SPDX-License-Identifier: GPL-2.0
> -// Copyright (c) 2017 Microchip Corporation.
> +/*
> + * SPDX-License-Identifier: GPL-2.0
> + * Copyright (c) 2017 Microchip Corporation.
> + */
> 
>  #include 
>  #include 
> 
> --
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi


Best Regards,
Wenyou Yang

RE: [PATCH v9 2/2] media: i2c: Add the ov7740 image sensor driver

[Wenyou.Yang]

Hi Sakari,

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: 2017年12月14日 4:06
> To: Wenyou Yang - A41535 ; Mauro Carvalho
> Chehab ; Rob Herring ;
> Mark Rutland 
> Cc: linux-kernel@vger.kernel.org; Nicolas Ferre - M43238
> ; devicet...@vger.kernel.org; Jonathan Corbet
> ; Hans Verkuil ; linux-arm-
> ker...@lists.infradead.org; Linux Media Mailing List  me...@vger.kernel.org>; Songjun Wu 
> Subject: Re: [PATCH v9 2/2] media: i2c: Add the ov7740 image sensor driver
> 
> Hi Wenyou,
> 
> Wenyou Yang wrote:
> ...
> > +static int ov7740_start_streaming(struct ov7740 *ov7740) {
> > +   int ret;
> > +
> > +   if (ov7740->fmt) {
> > +   ret = regmap_multi_reg_write(ov7740->regmap,
> > +ov7740->fmt->regs,
> > +ov7740->fmt->reg_num);
> > +   if (ret)
> > +   return ret;
> > +   }
> > +
> > +   if (ov7740->frmsize) {
> > +   ret = regmap_multi_reg_write(ov7740->regmap,
> > +ov7740->frmsize->regs,
> > +ov7740->frmsize->reg_num);
> > +   if (ret)
> > +   return ret;
> > +   }
> > +
> > +   return __v4l2_ctrl_handler_setup(ov7740->subdev.ctrl_handler);
> 
> I believe you're still setting the controls after starting streaming.

Yes, it sees it does so.

The OV7740 sensor generates the stream pixel data at the constant frame rate, 
no such start or stop control. 

> 
> --
> Sakari Ailus
> sakari.ai...@iki.fi

Wenyou Yang


imx7d-sdb.dtb
Description: imx7d-sdb.dtb

RE: [PATCH v9 2/2] media: i2c: Add the ov7740 image sensor driver

[Wenyou.Yang]

Hi Sakari,

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: 2017年12月14日 4:06
> To: Wenyou Yang - A41535 ; Mauro Carvalho
> Chehab ; Rob Herring ;
> Mark Rutland 
> Cc: linux-kernel@vger.kernel.org; Nicolas Ferre - M43238
> ; devicet...@vger.kernel.org; Jonathan Corbet
> ; Hans Verkuil ; linux-arm-
> ker...@lists.infradead.org; Linux Media Mailing List  me...@vger.kernel.org>; Songjun Wu 
> Subject: Re: [PATCH v9 2/2] media: i2c: Add the ov7740 image sensor driver
> 
> Hi Wenyou,
> 
> Wenyou Yang wrote:
> ...
> > +static int ov7740_start_streaming(struct ov7740 *ov7740) {
> > +   int ret;
> > +
> > +   if (ov7740->fmt) {
> > +   ret = regmap_multi_reg_write(ov7740->regmap,
> > +ov7740->fmt->regs,
> > +ov7740->fmt->reg_num);
> > +   if (ret)
> > +   return ret;
> > +   }
> > +
> > +   if (ov7740->frmsize) {
> > +   ret = regmap_multi_reg_write(ov7740->regmap,
> > +ov7740->frmsize->regs,
> > +ov7740->frmsize->reg_num);
> > +   if (ret)
> > +   return ret;
> > +   }
> > +
> > +   return __v4l2_ctrl_handler_setup(ov7740->subdev.ctrl_handler);
> 
> I believe you're still setting the controls after starting streaming.

Yes, it sees it does so.

The OV7740 sensor generates the stream pixel data at the constant frame rate, 
no such start or stop control. 

> 
> --
> Sakari Ailus
> sakari.ai...@iki.fi

Wenyou Yang


imx7d-sdb.dtb
Description: imx7d-sdb.dtb

RE: [PATCH v9 0/2] media: ov7740: Add a V4L2 sensor-level driver

[Wenyou.Yang]

Hi Sakari,

Do you have some comments on this version?


Best Regards,
Wenyou Yang

> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Wenyou Yang
> Sent: 2017年12月11日 9:32
> To: Mauro Carvalho Chehab ; Rob Herring
> ; Mark Rutland 
> Cc: linux-kernel@vger.kernel.org; Nicolas Ferre - M43238
> ; devicet...@vger.kernel.org; Sakari Ailus
> ; Jonathan Corbet ; Hans Verkuil
> ; linux-arm-ker...@lists.infradead.org; Linux Media 
> Mailing List
> ; Wenyou Yang - A41535
> 
> Subject: [PATCH v9 0/2] media: ov7740: Add a V4L2 sensor-level driver
> 
> Add a Video4Linux2 sensor-level driver for the OmniVision OV7740 VGA camera
> image sensor.
> 
> Changes in v9:
>  - Use the new SPDX ids.
> 
> Changes in v8:
>  - As the registers are written at stream start, remove the written
>code from the set fmt function.
> 
> Changes in v7:
>  - Add Acked-by tag.
>  - Fix the wrong handle of default register configuration.
>  - Add the missed assignment of ov7740->frmsize.
> 
> Changes in v6:
>  - Remove unnecessary #include .
>  - Remove unnecessary comments and extra newline.
>  - Add const for some structures.
>  - Add the check of the return value from regmap_write().
>  - Simplify the calling of __v4l2_ctrl_handler_setup().
>  - Add the default format initialization function.
>  - Integrate the set_power() and enable/disable the clock into
>one function.
> 
> Changes in v5:
>  - Squash the driver and MAINTAINERS entry patches to one.
>  - Precede the driver patch with the bindings patch.
> 
> Changes in v4:
>  - Assign 'val' a initial value to avoid warning: 'val' may be
>used uninitialized.
>  - Rename REG_REG15 to avoid warning: "REG_REG15" redefined.
> 
> Changes in v3:
>  - Explicitly document the "remote-endpoint" property.
>  - Put the MAINTAINERS change to a separate patch.
> 
> Changes in v2:
>  - Split off the bindings into a separate patch.
>  - Add a new entry to the MAINTAINERS file.
> 
> Wenyou Yang (2):
>   media: ov7740: Document device tree bindings
>   media: i2c: Add the ov7740 image sensor driver
> 
>  .../devicetree/bindings/media/i2c/ov7740.txt   |   47 +
>  MAINTAINERS|8 +
>  drivers/media/i2c/Kconfig  |8 +
>  drivers/media/i2c/Makefile |1 +
>  drivers/media/i2c/ov7740.c | 1216 
> 
>  5 files changed, 1280 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov7740.txt
>  create mode 100644 drivers/media/i2c/ov7740.c
> 
> --
> 2.15.0

RE: [PATCH v9 0/2] media: ov7740: Add a V4L2 sensor-level driver

[Wenyou.Yang]

Hi Sakari,

Do you have some comments on this version?


Best Regards,
Wenyou Yang

> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Wenyou Yang
> Sent: 2017年12月11日 9:32
> To: Mauro Carvalho Chehab ; Rob Herring
> ; Mark Rutland 
> Cc: linux-kernel@vger.kernel.org; Nicolas Ferre - M43238
> ; devicet...@vger.kernel.org; Sakari Ailus
> ; Jonathan Corbet ; Hans Verkuil
> ; linux-arm-ker...@lists.infradead.org; Linux Media 
> Mailing List
> ; Wenyou Yang - A41535
> 
> Subject: [PATCH v9 0/2] media: ov7740: Add a V4L2 sensor-level driver
> 
> Add a Video4Linux2 sensor-level driver for the OmniVision OV7740 VGA camera
> image sensor.
> 
> Changes in v9:
>  - Use the new SPDX ids.
> 
> Changes in v8:
>  - As the registers are written at stream start, remove the written
>code from the set fmt function.
> 
> Changes in v7:
>  - Add Acked-by tag.
>  - Fix the wrong handle of default register configuration.
>  - Add the missed assignment of ov7740->frmsize.
> 
> Changes in v6:
>  - Remove unnecessary #include .
>  - Remove unnecessary comments and extra newline.
>  - Add const for some structures.
>  - Add the check of the return value from regmap_write().
>  - Simplify the calling of __v4l2_ctrl_handler_setup().
>  - Add the default format initialization function.
>  - Integrate the set_power() and enable/disable the clock into
>one function.
> 
> Changes in v5:
>  - Squash the driver and MAINTAINERS entry patches to one.
>  - Precede the driver patch with the bindings patch.
> 
> Changes in v4:
>  - Assign 'val' a initial value to avoid warning: 'val' may be
>used uninitialized.
>  - Rename REG_REG15 to avoid warning: "REG_REG15" redefined.
> 
> Changes in v3:
>  - Explicitly document the "remote-endpoint" property.
>  - Put the MAINTAINERS change to a separate patch.
> 
> Changes in v2:
>  - Split off the bindings into a separate patch.
>  - Add a new entry to the MAINTAINERS file.
> 
> Wenyou Yang (2):
>   media: ov7740: Document device tree bindings
>   media: i2c: Add the ov7740 image sensor driver
> 
>  .../devicetree/bindings/media/i2c/ov7740.txt   |   47 +
>  MAINTAINERS|8 +
>  drivers/media/i2c/Kconfig  |8 +
>  drivers/media/i2c/Makefile |1 +
>  drivers/media/i2c/ov7740.c | 1216 
> 
>  5 files changed, 1280 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov7740.txt
>  create mode 100644 drivers/media/i2c/ov7740.c
> 
> --
> 2.15.0

RE: [PATCH 1/3] media: atmel-isc: Not support RBG format from sensor.

[Wenyou.Yang]

Hi Hans,

> -Original Message-
> From: Hans Verkuil [mailto:hverk...@xs4all.nl]
> Sent: 2017年8月22日 15:00
> To: Wenyou Yang - A41535 ; Mauro Carvalho
> Chehab 
> Cc: Nicolas Ferre - M43238 ; linux-
> ker...@vger.kernel.org; Sakari Ailus ; Jonathan Corbet
> ; linux-arm-ker...@lists.infradead.org; Linux Media Mailing 
> List
> 
> Subject: Re: [PATCH 1/3] media: atmel-isc: Not support RBG format from sensor.
> 
> On 08/22/2017 03:18 AM, Yang, Wenyou wrote:
> > Hi Hans,
> >
> > On 2017/8/21 22:07, Hans Verkuil wrote:
> >> On 08/17/2017 09:16 AM, Wenyou Yang wrote:
> >>> The 12-bit parallel interface supports the Raw Bayer, YCbCr,
> >>> Monochrome and JPEG Compressed pixel formats from the external
> >>> sensor, not support RBG pixel format.
> >>>
> >>> Signed-off-by: Wenyou Yang 
> >>> ---
> >>>
> >>>   drivers/media/platform/atmel/atmel-isc.c | 5 +
> >>>   1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/drivers/media/platform/atmel/atmel-isc.c
> >>> b/drivers/media/platform/atmel/atmel-isc.c
> >>> index d4df3d4ccd85..535bb03783fe 100644
> >>> --- a/drivers/media/platform/atmel/atmel-isc.c
> >>> +++ b/drivers/media/platform/atmel/atmel-isc.c
> >>> @@ -1478,6 +1478,11 @@ static int isc_formats_init(struct isc_device *isc)
> >>>   while (!v4l2_subdev_call(subdev, pad, enum_mbus_code,
> >>>  NULL, _code)) {
> >>>   mbus_code.index++;
> >>> +
> >>> + /* Not support the RGB pixel formats from sensor */
> >>> + if ((mbus_code.code & 0xf000) == 0x1000)
> >>> + continue;
> >> Am I missing something? Here you skip any RGB mediabus formats, but
> >> in patch 3/3 you add RGB mediabus formats. But this patch prevents
> >> those new formats from being selected, right?
> > This patch prevents getting the RGB format from the sensor directly.
> > The RGB format can be produced by ISC controller by itself.
> 
> OK, I think I see what is going on here. The isc_formats array really is two 
> arrays
> in one: up to RAW_FMT_IND_END it describes what it can receive from the
> source, and after that it describes what it can convert it to.

Not exactly.

Yes, up to RAW_FMT_IND_END, these formats must be got from the senor, they are 
RAW formats.
From ISC_FMT_IND_START to ISC_FMT_IND_END, they can be generated by the ISC 
controller.
It is possible they can be got from the sensor too, the driver will check it. 
If it can be got from both the sensor and the ISC controller, the user can use 
the "sensor_preferred" parameter to decide from which one to get.
The RBG formats are the exception.

> 
> But if you can't handle RGB formats from the sensor, then why not make sure
> none of the mbus codes in isc_formats uses RGB? That makes much more sense.
> 
> E.g.:
> 
> { V4L2_PIX_FMT_RGB565, MEDIA_BUS_FMT_RGB565_2X8_LE, 16,
>   ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG,
> ISC_RLP_CFG_MODE_RGB565,
>   ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x7b,
>   false, false },
> 
> Why use MEDIA_BUS_FMT_RGB565_2X8_LE if this apparently is not supported?

This array is also the lists of all formats supported by the ISC(including got 
from the sensor).
The RGB formats are only generated by the ISC controller, not from the sensor.

> 
> Regards,
> 
>   Hans
> 
> >
> >> Regards,
> >>
> >>Hans
> >>
> >>> +
> >>>   fmt = find_format_by_code(mbus_code.code, );
> >>>   if (!fmt)
> >>>   continue;
> >>>
> >
> > Best Regards,
> > Wenyou Yang
> >

Best Regards,
Wenyou Yang

RE: [PATCH 1/3] media: atmel-isc: Not support RBG format from sensor.

[Wenyou.Yang]

Hi Hans,

> -Original Message-
> From: Hans Verkuil [mailto:hverk...@xs4all.nl]
> Sent: 2017年8月22日 15:00
> To: Wenyou Yang - A41535 ; Mauro Carvalho
> Chehab 
> Cc: Nicolas Ferre - M43238 ; linux-
> ker...@vger.kernel.org; Sakari Ailus ; Jonathan Corbet
> ; linux-arm-ker...@lists.infradead.org; Linux Media Mailing 
> List
> 
> Subject: Re: [PATCH 1/3] media: atmel-isc: Not support RBG format from sensor.
> 
> On 08/22/2017 03:18 AM, Yang, Wenyou wrote:
> > Hi Hans,
> >
> > On 2017/8/21 22:07, Hans Verkuil wrote:
> >> On 08/17/2017 09:16 AM, Wenyou Yang wrote:
> >>> The 12-bit parallel interface supports the Raw Bayer, YCbCr,
> >>> Monochrome and JPEG Compressed pixel formats from the external
> >>> sensor, not support RBG pixel format.
> >>>
> >>> Signed-off-by: Wenyou Yang 
> >>> ---
> >>>
> >>>   drivers/media/platform/atmel/atmel-isc.c | 5 +
> >>>   1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/drivers/media/platform/atmel/atmel-isc.c
> >>> b/drivers/media/platform/atmel/atmel-isc.c
> >>> index d4df3d4ccd85..535bb03783fe 100644
> >>> --- a/drivers/media/platform/atmel/atmel-isc.c
> >>> +++ b/drivers/media/platform/atmel/atmel-isc.c
> >>> @@ -1478,6 +1478,11 @@ static int isc_formats_init(struct isc_device *isc)
> >>>   while (!v4l2_subdev_call(subdev, pad, enum_mbus_code,
> >>>  NULL, _code)) {
> >>>   mbus_code.index++;
> >>> +
> >>> + /* Not support the RGB pixel formats from sensor */
> >>> + if ((mbus_code.code & 0xf000) == 0x1000)
> >>> + continue;
> >> Am I missing something? Here you skip any RGB mediabus formats, but
> >> in patch 3/3 you add RGB mediabus formats. But this patch prevents
> >> those new formats from being selected, right?
> > This patch prevents getting the RGB format from the sensor directly.
> > The RGB format can be produced by ISC controller by itself.
> 
> OK, I think I see what is going on here. The isc_formats array really is two 
> arrays
> in one: up to RAW_FMT_IND_END it describes what it can receive from the
> source, and after that it describes what it can convert it to.

Not exactly.

Yes, up to RAW_FMT_IND_END, these formats must be got from the senor, they are 
RAW formats.
From ISC_FMT_IND_START to ISC_FMT_IND_END, they can be generated by the ISC 
controller.
It is possible they can be got from the sensor too, the driver will check it. 
If it can be got from both the sensor and the ISC controller, the user can use 
the "sensor_preferred" parameter to decide from which one to get.
The RBG formats are the exception.

> 
> But if you can't handle RGB formats from the sensor, then why not make sure
> none of the mbus codes in isc_formats uses RGB? That makes much more sense.
> 
> E.g.:
> 
> { V4L2_PIX_FMT_RGB565, MEDIA_BUS_FMT_RGB565_2X8_LE, 16,
>   ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG,
> ISC_RLP_CFG_MODE_RGB565,
>   ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x7b,
>   false, false },
> 
> Why use MEDIA_BUS_FMT_RGB565_2X8_LE if this apparently is not supported?

This array is also the lists of all formats supported by the ISC(including got 
from the sensor).
The RGB formats are only generated by the ISC controller, not from the sensor.

> 
> Regards,
> 
>   Hans
> 
> >
> >> Regards,
> >>
> >>Hans
> >>
> >>> +
> >>>   fmt = find_format_by_code(mbus_code.code, );
> >>>   if (!fmt)
> >>>   continue;
> >>>
> >
> > Best Regards,
> > Wenyou Yang
> >

Best Regards,
Wenyou Yang

RE: [PATCH v3] ARM: dts: at91: sama5d2: add m_can nodes

[Wenyou.Yang]

Hi Alexandre,

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年5月15日 17:44
> To: Wenyou Yang - A41535 
> Cc: Nicolas Ferre - M43238 ; Rob Herring
> ; Pawel Moll ; Mark Rutland
> ; Ian Campbell ; Kumar
> Gala ; Russell King ; linux-
> ker...@vger.kernel.org; linux-...@vger.kernel.org; Oliver Hartkopp
> ; devicet...@vger.kernel.org; Quentin Schulz
> ; Wenyou Yang - A41535
> ; linux-arm-ker...@lists.infradead.org
> Subject: Re: [PATCH v3] ARM: dts: at91: sama5d2: add m_can nodes
> 
> On 24/04/2017 at 09:12:17 +0800, Wenyou Yang wrote:
> > Add nodes to support the Controller Area Network(M_CAN) on SAMA5D2.
> > The version of M_CAN IP core is 3.1.0 (CREL = 0x31040730).
> >
> > As said in SAMA5D2 datasheet, the CAN clock is recommended to use
> > frequencies of 20, 40 or 80 MHz. To achieve these frequencies, PMC
> > GCLK3 must select the UPLLCK(480 MHz) as source clock and divide by
> > 24, 12, or 6. So, the "assigned-clock-rates" property has three
> > options: 2000, 4000, and 8000.
> > The "assigned-clock-parents" property should be referred to utmi
> > fixedly.
> >
> > The MSBs [bits 31:16] of the CAN Message RAM for CAN0 and CAN1 are
> > default configured in 0x0020. To avoid conflict with SRAM map for
> > PM, change them to 0x0021 in the AT91Bootstrap via setting the CAN
> > Memories Address-based Register(SFR_CAN) of SFR.
> >
> > Signed-off-by: Wenyou Yang 
> > Tested-by: Quentin Schulz 
> > ---
> > The patch is tested on SAMA5D2 Xplained and based on the patch set,
> > 1. [PATCH v4 1/7] can: m_can: Disabled Interrupt Line 1
> > http://marc.info/?l=linux-can=149165343604033=2
> >
> > Changes in v3:
> >  - Add Tested-by tag.
> >  - Change the number of Rx Rx Buffers, Tx Buffers and Tx Event FIFO
> >to maximum.
> >
> > Changes in v2:
> >  - Configures 10 TX Event FIFO elements and 10 TX Buffers/FIFO slots,
> >because the TXE FIFO is needed to be configured.
> >  - Configure the offset of Message RAM for CAN1 followed from CAN0's.
> >
> >  arch/arm/boot/dts/at91-sama5d2_xplained.dts | 24 +
> >  arch/arm/boot/dts/sama5d2.dtsi  | 56
> +
> >  2 files changed, 80 insertions(+)
> >
> 
> It didn't apply cleanly, can you please verify
> https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git/log/?h=at91-dt

Verified, it works. 

Thank you for your effort.


BTW, it will be better take the patch to add the config.
[PATCH v2] ARM: at91/defconfig: add MCAN driver to sama5_defconfig
I just submitted.

> 
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com


Best Regards,
Wenyou Yang

RE: [PATCH v3] ARM: dts: at91: sama5d2: add m_can nodes

[Wenyou.Yang]

Hi Alexandre,

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年5月15日 17:44
> To: Wenyou Yang - A41535 
> Cc: Nicolas Ferre - M43238 ; Rob Herring
> ; Pawel Moll ; Mark Rutland
> ; Ian Campbell ; Kumar
> Gala ; Russell King ; linux-
> ker...@vger.kernel.org; linux-...@vger.kernel.org; Oliver Hartkopp
> ; devicet...@vger.kernel.org; Quentin Schulz
> ; Wenyou Yang - A41535
> ; linux-arm-ker...@lists.infradead.org
> Subject: Re: [PATCH v3] ARM: dts: at91: sama5d2: add m_can nodes
> 
> On 24/04/2017 at 09:12:17 +0800, Wenyou Yang wrote:
> > Add nodes to support the Controller Area Network(M_CAN) on SAMA5D2.
> > The version of M_CAN IP core is 3.1.0 (CREL = 0x31040730).
> >
> > As said in SAMA5D2 datasheet, the CAN clock is recommended to use
> > frequencies of 20, 40 or 80 MHz. To achieve these frequencies, PMC
> > GCLK3 must select the UPLLCK(480 MHz) as source clock and divide by
> > 24, 12, or 6. So, the "assigned-clock-rates" property has three
> > options: 2000, 4000, and 8000.
> > The "assigned-clock-parents" property should be referred to utmi
> > fixedly.
> >
> > The MSBs [bits 31:16] of the CAN Message RAM for CAN0 and CAN1 are
> > default configured in 0x0020. To avoid conflict with SRAM map for
> > PM, change them to 0x0021 in the AT91Bootstrap via setting the CAN
> > Memories Address-based Register(SFR_CAN) of SFR.
> >
> > Signed-off-by: Wenyou Yang 
> > Tested-by: Quentin Schulz 
> > ---
> > The patch is tested on SAMA5D2 Xplained and based on the patch set,
> > 1. [PATCH v4 1/7] can: m_can: Disabled Interrupt Line 1
> > http://marc.info/?l=linux-can=149165343604033=2
> >
> > Changes in v3:
> >  - Add Tested-by tag.
> >  - Change the number of Rx Rx Buffers, Tx Buffers and Tx Event FIFO
> >to maximum.
> >
> > Changes in v2:
> >  - Configures 10 TX Event FIFO elements and 10 TX Buffers/FIFO slots,
> >because the TXE FIFO is needed to be configured.
> >  - Configure the offset of Message RAM for CAN1 followed from CAN0's.
> >
> >  arch/arm/boot/dts/at91-sama5d2_xplained.dts | 24 +
> >  arch/arm/boot/dts/sama5d2.dtsi  | 56
> +
> >  2 files changed, 80 insertions(+)
> >
> 
> It didn't apply cleanly, can you please verify
> https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git/log/?h=at91-dt

Verified, it works. 

Thank you for your effort.


BTW, it will be better take the patch to add the config.
[PATCH v2] ARM: at91/defconfig: add MCAN driver to sama5_defconfig
I just submitted.

> 
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com


Best Regards,
Wenyou Yang

RE: [PATCH v2 07/11] ARM: at91: pm: Tie the memory controller type to the ramc id

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月28日 19:20
> To: Nicolas Ferre - M43238 
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Boris
> Brezillon ; Wenyou Yang - A41535
> ; Alexandre Belloni  electrons.com>
> Subject: [PATCH v2 07/11] ARM: at91: pm: Tie the memory controller type to the
> ramc id
> 
> Instead of relying on the SoC type to select the memory controller type, use 
> the
> device tree ids as they are parsed anyway.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Wenyou Yang 

> ---
>  arch/arm/mach-at91/pm.c | 30 --
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> 488549bc2bed..ddf62a006635 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -329,11 +329,23 @@ static void at91sam9_sdram_standby(void)
>   at91_ramc_write(1, AT91_SDRAMC_LPR, saved_lpr1);  }
> 
> +struct ramc_info {
> + void (*idle)(void);
> + unsigned int memctrl;
> +};
> +
> +static const struct ramc_info ramc_infos[] __initconst = {
> + { .idle = at91rm9200_standby, .memctrl = AT91_MEMCTRL_MC},
> + { .idle = at91sam9_sdram_standby, .memctrl =
> AT91_MEMCTRL_SDRAMC},
> + { .idle = at91_ddr_standby, .memctrl = AT91_MEMCTRL_DDRSDR},
> + { .idle = sama5d3_ddr_standby, .memctrl =
> AT91_MEMCTRL_DDRSDR}, };
> +
>  static const struct of_device_id const ramc_ids[] __initconst = {
> - { .compatible = "atmel,at91rm9200-sdramc", .data =
> at91rm9200_standby },
> - { .compatible = "atmel,at91sam9260-sdramc", .data =
> at91sam9_sdram_standby },
> - { .compatible = "atmel,at91sam9g45-ddramc", .data = at91_ddr_standby },
> - { .compatible = "atmel,sama5d3-ddramc", .data = sama5d3_ddr_standby },
> + { .compatible = "atmel,at91rm9200-sdramc", .data = _infos[0] },
> + { .compatible = "atmel,at91sam9260-sdramc", .data = _infos[1] },
> + { .compatible = "atmel,at91sam9g45-ddramc", .data = _infos[2] },
> + { .compatible = "atmel,sama5d3-ddramc", .data = _infos[3] },
>   { /*sentinel*/ }
>  };
> 
> @@ -343,14 +355,17 @@ static __init void at91_dt_ramc(void)
>   const struct of_device_id *of_id;
>   int idx = 0;
>   const void *standby = NULL;
> + const struct ramc_info *ramc;
> 
>   for_each_matching_node_and_match(np, ramc_ids, _id) {
>   pm_data.ramc[idx] = of_iomap(np, 0);
>   if (!pm_data.ramc[idx])
>   panic(pr_fmt("unable to map ramc[%d] cpu registers\n"),
> idx);
> 
> + ramc = of_id->data;
>   if (!standby)
> - standby = of_id->data;
> + standby = ramc->idle;
> + pm_data.memctrl = ramc->memctrl;
> 
>   idx++;
>   }
> @@ -473,7 +488,6 @@ void __init at91rm9200_pm_init(void)
>   at91_ramc_write(0, AT91_MC_SDRAMC_LPR, 0);
> 
>   pm_data.uhp_udp_mask = AT91RM9200_PMC_UHP |
> AT91RM9200_PMC_UDP;
> - pm_data.memctrl = AT91_MEMCTRL_MC;
> 
>   at91_pm_init(at91rm9200_idle);
>  }
> @@ -481,7 +495,6 @@ void __init at91rm9200_pm_init(void)  void __init
> at91sam9260_pm_init(void)  {
>   at91_dt_ramc();
> - pm_data.memctrl = AT91_MEMCTRL_SDRAMC;
>   pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP |
> AT91SAM926x_PMC_UDP;
>   at91_pm_init(at91sam9_idle);
>  }
> @@ -490,7 +503,6 @@ void __init at91sam9g45_pm_init(void)  {
>   at91_dt_ramc();
>   pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP;
> - pm_data.memctrl = AT91_MEMCTRL_DDRSDR;
>   at91_pm_init(at91sam9_idle);
>  }
> 
> @@ -498,7 +510,6 @@ void __init at91sam9x5_pm_init(void)  {
>   at91_dt_ramc();
>   pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP |
> AT91SAM926x_PMC_UDP;
> - pm_data.memctrl = AT91_MEMCTRL_DDRSDR;
>   at91_pm_init(at91sam9_idle);
>  }
> 
> @@ -506,6 +517,5 @@ void __init sama5_pm_init(void)  {
>   at91_dt_ramc();
>   pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP |
> AT91SAM926x_PMC_UDP;
> - pm_data.memctrl = AT91_MEMCTRL_DDRSDR;
>   at91_pm_init(NULL);
>  }
> --
> 2.11.0


Best Regards,
Wenyou Yang

RE: [PATCH v2 07/11] ARM: at91: pm: Tie the memory controller type to the ramc id

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月28日 19:20
> To: Nicolas Ferre - M43238 
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Boris
> Brezillon ; Wenyou Yang - A41535
> ; Alexandre Belloni  electrons.com>
> Subject: [PATCH v2 07/11] ARM: at91: pm: Tie the memory controller type to the
> ramc id
> 
> Instead of relying on the SoC type to select the memory controller type, use 
> the
> device tree ids as they are parsed anyway.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Wenyou Yang 

> ---
>  arch/arm/mach-at91/pm.c | 30 --
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> 488549bc2bed..ddf62a006635 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -329,11 +329,23 @@ static void at91sam9_sdram_standby(void)
>   at91_ramc_write(1, AT91_SDRAMC_LPR, saved_lpr1);  }
> 
> +struct ramc_info {
> + void (*idle)(void);
> + unsigned int memctrl;
> +};
> +
> +static const struct ramc_info ramc_infos[] __initconst = {
> + { .idle = at91rm9200_standby, .memctrl = AT91_MEMCTRL_MC},
> + { .idle = at91sam9_sdram_standby, .memctrl =
> AT91_MEMCTRL_SDRAMC},
> + { .idle = at91_ddr_standby, .memctrl = AT91_MEMCTRL_DDRSDR},
> + { .idle = sama5d3_ddr_standby, .memctrl =
> AT91_MEMCTRL_DDRSDR}, };
> +
>  static const struct of_device_id const ramc_ids[] __initconst = {
> - { .compatible = "atmel,at91rm9200-sdramc", .data =
> at91rm9200_standby },
> - { .compatible = "atmel,at91sam9260-sdramc", .data =
> at91sam9_sdram_standby },
> - { .compatible = "atmel,at91sam9g45-ddramc", .data = at91_ddr_standby },
> - { .compatible = "atmel,sama5d3-ddramc", .data = sama5d3_ddr_standby },
> + { .compatible = "atmel,at91rm9200-sdramc", .data = _infos[0] },
> + { .compatible = "atmel,at91sam9260-sdramc", .data = _infos[1] },
> + { .compatible = "atmel,at91sam9g45-ddramc", .data = _infos[2] },
> + { .compatible = "atmel,sama5d3-ddramc", .data = _infos[3] },
>   { /*sentinel*/ }
>  };
> 
> @@ -343,14 +355,17 @@ static __init void at91_dt_ramc(void)
>   const struct of_device_id *of_id;
>   int idx = 0;
>   const void *standby = NULL;
> + const struct ramc_info *ramc;
> 
>   for_each_matching_node_and_match(np, ramc_ids, _id) {
>   pm_data.ramc[idx] = of_iomap(np, 0);
>   if (!pm_data.ramc[idx])
>   panic(pr_fmt("unable to map ramc[%d] cpu registers\n"),
> idx);
> 
> + ramc = of_id->data;
>   if (!standby)
> - standby = of_id->data;
> + standby = ramc->idle;
> + pm_data.memctrl = ramc->memctrl;
> 
>   idx++;
>   }
> @@ -473,7 +488,6 @@ void __init at91rm9200_pm_init(void)
>   at91_ramc_write(0, AT91_MC_SDRAMC_LPR, 0);
> 
>   pm_data.uhp_udp_mask = AT91RM9200_PMC_UHP |
> AT91RM9200_PMC_UDP;
> - pm_data.memctrl = AT91_MEMCTRL_MC;
> 
>   at91_pm_init(at91rm9200_idle);
>  }
> @@ -481,7 +495,6 @@ void __init at91rm9200_pm_init(void)  void __init
> at91sam9260_pm_init(void)  {
>   at91_dt_ramc();
> - pm_data.memctrl = AT91_MEMCTRL_SDRAMC;
>   pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP |
> AT91SAM926x_PMC_UDP;
>   at91_pm_init(at91sam9_idle);
>  }
> @@ -490,7 +503,6 @@ void __init at91sam9g45_pm_init(void)  {
>   at91_dt_ramc();
>   pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP;
> - pm_data.memctrl = AT91_MEMCTRL_DDRSDR;
>   at91_pm_init(at91sam9_idle);
>  }
> 
> @@ -498,7 +510,6 @@ void __init at91sam9x5_pm_init(void)  {
>   at91_dt_ramc();
>   pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP |
> AT91SAM926x_PMC_UDP;
> - pm_data.memctrl = AT91_MEMCTRL_DDRSDR;
>   at91_pm_init(at91sam9_idle);
>  }
> 
> @@ -506,6 +517,5 @@ void __init sama5_pm_init(void)  {
>   at91_dt_ramc();
>   pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP |
> AT91SAM926x_PMC_UDP;
> - pm_data.memctrl = AT91_MEMCTRL_DDRSDR;
>   at91_pm_init(NULL);
>  }
> --
> 2.11.0


Best Regards,
Wenyou Yang

RE: [PATCH v2 06/11] ARM: at91: pm: Workaround DDRSDRC self-refresh bug with LPDDR1 memories.

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月28日 19:20
> To: Nicolas Ferre - M43238 
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Boris
> Brezillon ; Wenyou Yang - A41535
> ; Alexandre Belloni  electrons.com>
> Subject: [PATCH v2 06/11] ARM: at91: pm: Workaround DDRSDRC self-refresh
> bug with LPDDR1 memories.
> 
> As already explained for pm_suspend.S, the DDRSDR controller fails to put
> LPDDR1 memories in self-refresh. Force the controller to think it has DDR2
> memories during the self-refresh period, as the DDR2 self-refresh spec is
> equivalent to LPDDR1, and is correctly implemented in the controller.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Wenyou Yang 

> ---
>  arch/arm/mach-at91/pm.c | 20 +++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> 3d68d93c11c7..488549bc2bed 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -241,12 +241,27 @@ static void at91_ddr_standby(void)
>   /* Those two values allow us to delay self-refresh activation
>* to the maximum. */
>   u32 lpr0, lpr1 = 0;
> + u32 mdr, saved_mdr0, saved_mdr1 = 0;
>   u32 saved_lpr0, saved_lpr1 = 0;
> 
> + /* LPDDR1 --> force DDR2 mode during self-refresh */
> + saved_mdr0 = at91_ramc_read(0, AT91_DDRSDRC_MDR);
> + if ((saved_mdr0 & AT91_DDRSDRC_MD) ==
> AT91_DDRSDRC_MD_LOW_POWER_DDR) {
> + mdr = saved_mdr0 & ~AT91_DDRSDRC_MD;
> + mdr |= AT91_DDRSDRC_MD_DDR2;
> + at91_ramc_write(0, AT91_DDRSDRC_MDR, mdr);
> + }
> +
>   if (pm_data.ramc[1]) {
>   saved_lpr1 = at91_ramc_read(1, AT91_DDRSDRC_LPR);
>   lpr1 = saved_lpr1 & ~AT91_DDRSDRC_LPCB;
>   lpr1 |= AT91_DDRSDRC_LPCB_SELF_REFRESH;
> + saved_mdr1 = at91_ramc_read(1, AT91_DDRSDRC_MDR);
> + if ((saved_mdr1 & AT91_DDRSDRC_MD) ==
> AT91_DDRSDRC_MD_LOW_POWER_DDR) {
> + mdr = saved_mdr1 & ~AT91_DDRSDRC_MD;
> + mdr |= AT91_DDRSDRC_MD_DDR2;
> + at91_ramc_write(1, AT91_DDRSDRC_MDR, mdr);
> + }
>   }
> 
>   saved_lpr0 = at91_ramc_read(0, AT91_DDRSDRC_LPR); @@ -260,9
> +275,12 @@ static void at91_ddr_standby(void)
> 
>   cpu_do_idle();
> 
> + at91_ramc_write(0, AT91_DDRSDRC_MDR, saved_mdr0);
>   at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr0);
> - if (pm_data.ramc[1])
> + if (pm_data.ramc[1]) {
> + at91_ramc_write(0, AT91_DDRSDRC_MDR, saved_mdr1);
>   at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
> + }
>  }
> 
>  static void sama5d3_ddr_standby(void)
> --
> 2.11.0

RE: [PATCH v2 06/11] ARM: at91: pm: Workaround DDRSDRC self-refresh bug with LPDDR1 memories.

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月28日 19:20
> To: Nicolas Ferre - M43238 
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Boris
> Brezillon ; Wenyou Yang - A41535
> ; Alexandre Belloni  electrons.com>
> Subject: [PATCH v2 06/11] ARM: at91: pm: Workaround DDRSDRC self-refresh
> bug with LPDDR1 memories.
> 
> As already explained for pm_suspend.S, the DDRSDR controller fails to put
> LPDDR1 memories in self-refresh. Force the controller to think it has DDR2
> memories during the self-refresh period, as the DDR2 self-refresh spec is
> equivalent to LPDDR1, and is correctly implemented in the controller.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Wenyou Yang 

> ---
>  arch/arm/mach-at91/pm.c | 20 +++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> 3d68d93c11c7..488549bc2bed 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -241,12 +241,27 @@ static void at91_ddr_standby(void)
>   /* Those two values allow us to delay self-refresh activation
>* to the maximum. */
>   u32 lpr0, lpr1 = 0;
> + u32 mdr, saved_mdr0, saved_mdr1 = 0;
>   u32 saved_lpr0, saved_lpr1 = 0;
> 
> + /* LPDDR1 --> force DDR2 mode during self-refresh */
> + saved_mdr0 = at91_ramc_read(0, AT91_DDRSDRC_MDR);
> + if ((saved_mdr0 & AT91_DDRSDRC_MD) ==
> AT91_DDRSDRC_MD_LOW_POWER_DDR) {
> + mdr = saved_mdr0 & ~AT91_DDRSDRC_MD;
> + mdr |= AT91_DDRSDRC_MD_DDR2;
> + at91_ramc_write(0, AT91_DDRSDRC_MDR, mdr);
> + }
> +
>   if (pm_data.ramc[1]) {
>   saved_lpr1 = at91_ramc_read(1, AT91_DDRSDRC_LPR);
>   lpr1 = saved_lpr1 & ~AT91_DDRSDRC_LPCB;
>   lpr1 |= AT91_DDRSDRC_LPCB_SELF_REFRESH;
> + saved_mdr1 = at91_ramc_read(1, AT91_DDRSDRC_MDR);
> + if ((saved_mdr1 & AT91_DDRSDRC_MD) ==
> AT91_DDRSDRC_MD_LOW_POWER_DDR) {
> + mdr = saved_mdr1 & ~AT91_DDRSDRC_MD;
> + mdr |= AT91_DDRSDRC_MD_DDR2;
> + at91_ramc_write(1, AT91_DDRSDRC_MDR, mdr);
> + }
>   }
> 
>   saved_lpr0 = at91_ramc_read(0, AT91_DDRSDRC_LPR); @@ -260,9
> +275,12 @@ static void at91_ddr_standby(void)
> 
>   cpu_do_idle();
> 
> + at91_ramc_write(0, AT91_DDRSDRC_MDR, saved_mdr0);
>   at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr0);
> - if (pm_data.ramc[1])
> + if (pm_data.ramc[1]) {
> + at91_ramc_write(0, AT91_DDRSDRC_MDR, saved_mdr1);
>   at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
> + }
>  }
> 
>  static void sama5d3_ddr_standby(void)
> --
> 2.11.0

RE: [PATCH v2 04/11] ARM: at91: pm: Use struct at91_pm_data in pm_suspend.S

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月28日 19:20
> To: Nicolas Ferre - M43238 
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Boris
> Brezillon ; Wenyou Yang - A41535
> ; Alexandre Belloni  electrons.com>
> Subject: [PATCH v2 04/11] ARM: at91: pm: Use struct at91_pm_data in
> pm_suspend.S
> 
> The number of register we can safely pass to at91_pm_suspend_in_sram is
> limited. Instead, pass the address to the at91_pm_data structure.
> 
> The offsets are automatically generated to avoid hardcoding them.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Wenyou Yang 

> ---
>  arch/arm/mach-at91/Makefile  | 33 +++
>  arch/arm/mach-at91/pm.c  | 78 
> +++-
>  arch/arm/mach-at91/pm.h  | 16 +---
>  arch/arm/mach-at91/pm_data-offsets.c | 13 ++
>  arch/arm/mach-at91/pm_suspend.S  | 29 ++
>  5 files changed, 102 insertions(+), 67 deletions(-)  create mode 100644
> arch/arm/mach-at91/pm_data-offsets.c
> 
> diff --git a/arch/arm/mach-at91/Makefile b/arch/arm/mach-at91/Makefile index
> c5bbf8bb8c0f..858ef14f961c 100644
> --- a/arch/arm/mach-at91/Makefile
> +++ b/arch/arm/mach-at91/Makefile
> @@ -18,3 +18,36 @@ endif
>  ifeq ($(CONFIG_PM_DEBUG),y)
>  CFLAGS_pm.o += -DDEBUG
>  endif
> +
> +# Default sed regexp - multiline due to syntax constraints define sed-y
> + "/^->/{s:->#\(.*\):/* \1 */:; \
> + s:^->\([^ ]*\) [\$$#]*\([-0-9]*\) \(.*\):#define \1 \2 /* \3 */:; \
> + s:^->\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; \
> + s:->::; p;}"
> +endef
> +
> +# Use filechk to avoid rebuilds when a header changes, but the
> +resulting file # does not define filechk_offsets
> + (set -e; \
> +  echo "#ifndef $2"; \
> +  echo "#define $2"; \
> +  echo "/*"; \
> +  echo " * DO NOT MODIFY."; \
> +  echo " *"; \
> +  echo " * This file was generated by Kbuild"; \
> +  echo " */"; \
> +  echo ""; \
> +  sed -ne $(sed-y); \
> +  echo ""; \
> +  echo "#endif" )
> +endef
> +
> +arch/arm/mach-at91/pm_data-offsets.s: arch/arm/mach-at91/pm_data-offsets.c
> + $(call if_changed_dep,cc_s_c)
> +
> +include/generated/at91_pm_data-offsets.h: arch/arm/mach-at91/pm_data-
> offsets.s FORCE
> + $(call filechk,offsets,__PM_DATA_OFFSETS_H__)
> +
> +arch/arm/mach-at91/pm_suspend.o:
> +include/generated/at91_pm_data-offsets.h
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> c780dda3b604..a35b1541b328 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -37,18 +37,13 @@ extern void at91_pinctrl_gpio_suspend(void);  extern void
> at91_pinctrl_gpio_resume(void);  #endif
> 
> -static struct {
> - void __iomem *pmc;
> - void __iomem *ramc[2];
> - unsigned long uhp_udp_mask;
> - int memctrl;
> -} at91_pm_data;
> +static struct at91_pm_data pm_data;
> 
>  #define at91_ramc_read(id, field) \
> - __raw_readl(at91_pm_data.ramc[id] + field)
> + __raw_readl(pm_data.ramc[id] + field)
> 
>  #define at91_ramc_write(id, field, value) \
> - __raw_writel(value, at91_pm_data.ramc[id] + field)
> + __raw_writel(value, pm_data.ramc[id] + field)
> 
>  static int at91_pm_valid_state(suspend_state_t state)  { @@ -84,10 +79,10 @@
> static int at91_pm_verify_clocks(void)
>   unsigned long scsr;
>   int i;
> 
> - scsr = readl(at91_pm_data.pmc + AT91_PMC_SCSR);
> + scsr = readl(pm_data.pmc + AT91_PMC_SCSR);
> 
>   /* USB must not be using PLLB */
> - if ((scsr & at91_pm_data.uhp_udp_mask) != 0) {
> + if ((scsr & pm_data.uhp_udp_mask) != 0) {
>   pr_err("AT91: PM - Suspend-to-RAM with USB still active\n");
>   return 0;
>   }
> @@ -98,7 +93,7 @@ static int at91_pm_verify_clocks(void)
> 
>   if ((scsr & (AT91_PMC_PCK0 << i)) == 0)
>   continue;
> - css = readl(at91_pm_data.pmc + AT91_PMC_PCKR(i)) &
> AT91_PMC_CSS;
> + css = readl(pm_data.pmc + AT91_PMC_PCKR(i)) &
> AT91_PMC_CSS;
>   if (css != AT91_PMC_CSS_SLOW) {
>   pr_err("AT91: PM - Suspend-to-RAM with PCK%d
> src %d\n", i, css);
>   return 0;
> @@ -124,25 +119,18 @@ int at91_suspend_entering_slow_clock(void)
>  }
>  EXPORT_SYMBOL(at91_suspend_entering_slow_clock);
> 
> -static void (*at91_suspend_sram_fn)(void __iomem *pmc, void __iomem *ramc0,
> -   void __iomem *ramc1, int memctrl);
> -
> -extern void at91_pm_suspend_in_sram(void __iomem *pmc, void __iomem
> *ramc0,
> - void __iomem *ramc1, int memctrl);
> +static void (*at91_suspend_sram_fn)(struct at91_pm_data *); extern void
> 

RE: [PATCH v2 04/11] ARM: at91: pm: Use struct at91_pm_data in pm_suspend.S

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月28日 19:20
> To: Nicolas Ferre - M43238 
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Boris
> Brezillon ; Wenyou Yang - A41535
> ; Alexandre Belloni  electrons.com>
> Subject: [PATCH v2 04/11] ARM: at91: pm: Use struct at91_pm_data in
> pm_suspend.S
> 
> The number of register we can safely pass to at91_pm_suspend_in_sram is
> limited. Instead, pass the address to the at91_pm_data structure.
> 
> The offsets are automatically generated to avoid hardcoding them.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Wenyou Yang 

> ---
>  arch/arm/mach-at91/Makefile  | 33 +++
>  arch/arm/mach-at91/pm.c  | 78 
> +++-
>  arch/arm/mach-at91/pm.h  | 16 +---
>  arch/arm/mach-at91/pm_data-offsets.c | 13 ++
>  arch/arm/mach-at91/pm_suspend.S  | 29 ++
>  5 files changed, 102 insertions(+), 67 deletions(-)  create mode 100644
> arch/arm/mach-at91/pm_data-offsets.c
> 
> diff --git a/arch/arm/mach-at91/Makefile b/arch/arm/mach-at91/Makefile index
> c5bbf8bb8c0f..858ef14f961c 100644
> --- a/arch/arm/mach-at91/Makefile
> +++ b/arch/arm/mach-at91/Makefile
> @@ -18,3 +18,36 @@ endif
>  ifeq ($(CONFIG_PM_DEBUG),y)
>  CFLAGS_pm.o += -DDEBUG
>  endif
> +
> +# Default sed regexp - multiline due to syntax constraints define sed-y
> + "/^->/{s:->#\(.*\):/* \1 */:; \
> + s:^->\([^ ]*\) [\$$#]*\([-0-9]*\) \(.*\):#define \1 \2 /* \3 */:; \
> + s:^->\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; \
> + s:->::; p;}"
> +endef
> +
> +# Use filechk to avoid rebuilds when a header changes, but the
> +resulting file # does not define filechk_offsets
> + (set -e; \
> +  echo "#ifndef $2"; \
> +  echo "#define $2"; \
> +  echo "/*"; \
> +  echo " * DO NOT MODIFY."; \
> +  echo " *"; \
> +  echo " * This file was generated by Kbuild"; \
> +  echo " */"; \
> +  echo ""; \
> +  sed -ne $(sed-y); \
> +  echo ""; \
> +  echo "#endif" )
> +endef
> +
> +arch/arm/mach-at91/pm_data-offsets.s: arch/arm/mach-at91/pm_data-offsets.c
> + $(call if_changed_dep,cc_s_c)
> +
> +include/generated/at91_pm_data-offsets.h: arch/arm/mach-at91/pm_data-
> offsets.s FORCE
> + $(call filechk,offsets,__PM_DATA_OFFSETS_H__)
> +
> +arch/arm/mach-at91/pm_suspend.o:
> +include/generated/at91_pm_data-offsets.h
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> c780dda3b604..a35b1541b328 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -37,18 +37,13 @@ extern void at91_pinctrl_gpio_suspend(void);  extern void
> at91_pinctrl_gpio_resume(void);  #endif
> 
> -static struct {
> - void __iomem *pmc;
> - void __iomem *ramc[2];
> - unsigned long uhp_udp_mask;
> - int memctrl;
> -} at91_pm_data;
> +static struct at91_pm_data pm_data;
> 
>  #define at91_ramc_read(id, field) \
> - __raw_readl(at91_pm_data.ramc[id] + field)
> + __raw_readl(pm_data.ramc[id] + field)
> 
>  #define at91_ramc_write(id, field, value) \
> - __raw_writel(value, at91_pm_data.ramc[id] + field)
> + __raw_writel(value, pm_data.ramc[id] + field)
> 
>  static int at91_pm_valid_state(suspend_state_t state)  { @@ -84,10 +79,10 @@
> static int at91_pm_verify_clocks(void)
>   unsigned long scsr;
>   int i;
> 
> - scsr = readl(at91_pm_data.pmc + AT91_PMC_SCSR);
> + scsr = readl(pm_data.pmc + AT91_PMC_SCSR);
> 
>   /* USB must not be using PLLB */
> - if ((scsr & at91_pm_data.uhp_udp_mask) != 0) {
> + if ((scsr & pm_data.uhp_udp_mask) != 0) {
>   pr_err("AT91: PM - Suspend-to-RAM with USB still active\n");
>   return 0;
>   }
> @@ -98,7 +93,7 @@ static int at91_pm_verify_clocks(void)
> 
>   if ((scsr & (AT91_PMC_PCK0 << i)) == 0)
>   continue;
> - css = readl(at91_pm_data.pmc + AT91_PMC_PCKR(i)) &
> AT91_PMC_CSS;
> + css = readl(pm_data.pmc + AT91_PMC_PCKR(i)) &
> AT91_PMC_CSS;
>   if (css != AT91_PMC_CSS_SLOW) {
>   pr_err("AT91: PM - Suspend-to-RAM with PCK%d
> src %d\n", i, css);
>   return 0;
> @@ -124,25 +119,18 @@ int at91_suspend_entering_slow_clock(void)
>  }
>  EXPORT_SYMBOL(at91_suspend_entering_slow_clock);
> 
> -static void (*at91_suspend_sram_fn)(void __iomem *pmc, void __iomem *ramc0,
> -   void __iomem *ramc1, int memctrl);
> -
> -extern void at91_pm_suspend_in_sram(void __iomem *pmc, void __iomem
> *ramc0,
> - void __iomem *ramc1, int memctrl);
> +static void (*at91_suspend_sram_fn)(struct at91_pm_data *); extern void
> +at91_pm_suspend_in_sram(struct at91_pm_data *pm_data);
>  extern u32 at91_pm_suspend_in_sram_sz;
> 
>  static void at91_pm_suspend(suspend_state_t state)  {
> - unsigned int 

RE: [PATCH v2 09/11] ARM: at91: pm: Merge all at91sam9*_pm_init

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月28日 19:20
> To: Nicolas Ferre - M43238 
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Boris
> Brezillon ; Wenyou Yang - A41535
> ; Alexandre Belloni  electrons.com>
> Subject: [PATCH v2 09/11] ARM: at91: pm: Merge all at91sam9*_pm_init
> 
> The PM initialization is now identical for all at91sam9. Merge the functions.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Wenyou Yang 

> ---
>  arch/arm/mach-at91/at91sam9.c | 45 
> +++
>  arch/arm/mach-at91/generic.h  |  8 ++--
>  arch/arm/mach-at91/pm.c   | 14 +-
>  3 files changed, 6 insertions(+), 61 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/at91sam9.c b/arch/arm/mach-at91/at91sam9.c
> index ba28e9cc584d..c089bfd0dc2f 100644
> --- a/arch/arm/mach-at91/at91sam9.c
> +++ b/arch/arm/mach-at91/at91sam9.c
> @@ -52,7 +52,7 @@ static const struct at91_soc at91sam9_socs[] = {
>   { /* sentinel */ },
>  };
> 
> -static void __init at91sam9_common_init(void)
> +static void __init at91sam9_init(void)
>  {
>   struct soc_device *soc;
>   struct device *soc_dev = NULL;
> @@ -62,12 +62,8 @@ static void __init at91sam9_common_init(void)
>   soc_dev = soc_device_to_device(soc);
> 
>   of_platform_default_populate(NULL, NULL, soc_dev); -}
> 
> -static void __init at91sam9_dt_device_init(void) -{
> - at91sam9_common_init();
> - at91sam9260_pm_init();
> + at91sam9_pm_init();
>  }
> 
>  static const char *const at91_dt_board_compat[] __initconst = { @@ -77,41
> +73,6 @@ static const char *const at91_dt_board_compat[] __initconst = {
> 
>  DT_MACHINE_START(at91sam_dt, "Atmel AT91SAM9")
>   /* Maintainer: Atmel */
> - .init_machine   = at91sam9_dt_device_init,
> + .init_machine   = at91sam9_init,
>   .dt_compat  = at91_dt_board_compat,
>  MACHINE_END
> -
> -static void __init at91sam9g45_dt_device_init(void) -{
> - at91sam9_common_init();
> - at91sam9g45_pm_init();
> -}
> -
> -static const char *const at91sam9g45_board_compat[] __initconst = {
> - "atmel,at91sam9g45",
> - NULL
> -};
> -
> -DT_MACHINE_START(at91sam9g45_dt, "Atmel AT91SAM9G45")
> - /* Maintainer: Atmel */
> - .init_machine   = at91sam9g45_dt_device_init,
> - .dt_compat  = at91sam9g45_board_compat,
> -MACHINE_END
> -
> -static void __init at91sam9x5_dt_device_init(void) -{
> - at91sam9_common_init();
> - at91sam9x5_pm_init();
> -}
> -
> -static const char *const at91sam9x5_board_compat[] __initconst = {
> - "atmel,at91sam9x5",
> - "atmel,at91sam9n12",
> - NULL
> -};
> -
> -DT_MACHINE_START(at91sam9x5_dt, "Atmel AT91SAM9")
> - /* Maintainer: Atmel */
> - .init_machine   = at91sam9x5_dt_device_init,
> - .dt_compat  = at91sam9x5_board_compat,
> -MACHINE_END
> diff --git a/arch/arm/mach-at91/generic.h b/arch/arm/mach-at91/generic.h index
> 28ca57a2060f..f1ead0f13c19 100644
> --- a/arch/arm/mach-at91/generic.h
> +++ b/arch/arm/mach-at91/generic.h
> @@ -13,15 +13,11 @@
> 
>  #ifdef CONFIG_PM
>  extern void __init at91rm9200_pm_init(void); -extern void __init
> at91sam9260_pm_init(void); -extern void __init at91sam9g45_pm_init(void); -
> extern void __init at91sam9x5_pm_init(void);
> +extern void __init at91sam9_pm_init(void);
>  extern void __init sama5_pm_init(void);  #else  static inline void __init
> at91rm9200_pm_init(void) { } -static inline void __init 
> at91sam9260_pm_init(void)
> { } -static inline void __init at91sam9g45_pm_init(void) { } -static inline 
> void __init
> at91sam9x5_pm_init(void) { }
> +static inline void __init at91sam9_pm_init(void) { }
>  static inline void __init sama5_pm_init(void) { }  #endif
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> a7c047f0d21f..dedfe9038336 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -505,19 +505,7 @@ void __init at91rm9200_pm_init(void)
>   at91_pm_init(at91rm9200_idle);
>  }
> 
> -void __init at91sam9260_pm_init(void)
> -{
> - at91_dt_ramc();
> - at91_pm_init(at91sam9_idle);
> -}
> -
> -void __init at91sam9g45_pm_init(void)
> -{
> - at91_dt_ramc();
> - at91_pm_init(at91sam9_idle);
> -}
> -
> -void __init at91sam9x5_pm_init(void)
> +void __init at91sam9_pm_init(void)
>  {
>   at91_dt_ramc();
>   at91_pm_init(at91sam9_idle);
> --
> 2.11.0


Best Regards,
Wenyou Yang

RE: [PATCH v2 09/11] ARM: at91: pm: Merge all at91sam9*_pm_init

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月28日 19:20
> To: Nicolas Ferre - M43238 
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Boris
> Brezillon ; Wenyou Yang - A41535
> ; Alexandre Belloni  electrons.com>
> Subject: [PATCH v2 09/11] ARM: at91: pm: Merge all at91sam9*_pm_init
> 
> The PM initialization is now identical for all at91sam9. Merge the functions.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Wenyou Yang 

> ---
>  arch/arm/mach-at91/at91sam9.c | 45 
> +++
>  arch/arm/mach-at91/generic.h  |  8 ++--
>  arch/arm/mach-at91/pm.c   | 14 +-
>  3 files changed, 6 insertions(+), 61 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/at91sam9.c b/arch/arm/mach-at91/at91sam9.c
> index ba28e9cc584d..c089bfd0dc2f 100644
> --- a/arch/arm/mach-at91/at91sam9.c
> +++ b/arch/arm/mach-at91/at91sam9.c
> @@ -52,7 +52,7 @@ static const struct at91_soc at91sam9_socs[] = {
>   { /* sentinel */ },
>  };
> 
> -static void __init at91sam9_common_init(void)
> +static void __init at91sam9_init(void)
>  {
>   struct soc_device *soc;
>   struct device *soc_dev = NULL;
> @@ -62,12 +62,8 @@ static void __init at91sam9_common_init(void)
>   soc_dev = soc_device_to_device(soc);
> 
>   of_platform_default_populate(NULL, NULL, soc_dev); -}
> 
> -static void __init at91sam9_dt_device_init(void) -{
> - at91sam9_common_init();
> - at91sam9260_pm_init();
> + at91sam9_pm_init();
>  }
> 
>  static const char *const at91_dt_board_compat[] __initconst = { @@ -77,41
> +73,6 @@ static const char *const at91_dt_board_compat[] __initconst = {
> 
>  DT_MACHINE_START(at91sam_dt, "Atmel AT91SAM9")
>   /* Maintainer: Atmel */
> - .init_machine   = at91sam9_dt_device_init,
> + .init_machine   = at91sam9_init,
>   .dt_compat  = at91_dt_board_compat,
>  MACHINE_END
> -
> -static void __init at91sam9g45_dt_device_init(void) -{
> - at91sam9_common_init();
> - at91sam9g45_pm_init();
> -}
> -
> -static const char *const at91sam9g45_board_compat[] __initconst = {
> - "atmel,at91sam9g45",
> - NULL
> -};
> -
> -DT_MACHINE_START(at91sam9g45_dt, "Atmel AT91SAM9G45")
> - /* Maintainer: Atmel */
> - .init_machine   = at91sam9g45_dt_device_init,
> - .dt_compat  = at91sam9g45_board_compat,
> -MACHINE_END
> -
> -static void __init at91sam9x5_dt_device_init(void) -{
> - at91sam9_common_init();
> - at91sam9x5_pm_init();
> -}
> -
> -static const char *const at91sam9x5_board_compat[] __initconst = {
> - "atmel,at91sam9x5",
> - "atmel,at91sam9n12",
> - NULL
> -};
> -
> -DT_MACHINE_START(at91sam9x5_dt, "Atmel AT91SAM9")
> - /* Maintainer: Atmel */
> - .init_machine   = at91sam9x5_dt_device_init,
> - .dt_compat  = at91sam9x5_board_compat,
> -MACHINE_END
> diff --git a/arch/arm/mach-at91/generic.h b/arch/arm/mach-at91/generic.h index
> 28ca57a2060f..f1ead0f13c19 100644
> --- a/arch/arm/mach-at91/generic.h
> +++ b/arch/arm/mach-at91/generic.h
> @@ -13,15 +13,11 @@
> 
>  #ifdef CONFIG_PM
>  extern void __init at91rm9200_pm_init(void); -extern void __init
> at91sam9260_pm_init(void); -extern void __init at91sam9g45_pm_init(void); -
> extern void __init at91sam9x5_pm_init(void);
> +extern void __init at91sam9_pm_init(void);
>  extern void __init sama5_pm_init(void);  #else  static inline void __init
> at91rm9200_pm_init(void) { } -static inline void __init 
> at91sam9260_pm_init(void)
> { } -static inline void __init at91sam9g45_pm_init(void) { } -static inline 
> void __init
> at91sam9x5_pm_init(void) { }
> +static inline void __init at91sam9_pm_init(void) { }
>  static inline void __init sama5_pm_init(void) { }  #endif
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> a7c047f0d21f..dedfe9038336 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -505,19 +505,7 @@ void __init at91rm9200_pm_init(void)
>   at91_pm_init(at91rm9200_idle);
>  }
> 
> -void __init at91sam9260_pm_init(void)
> -{
> - at91_dt_ramc();
> - at91_pm_init(at91sam9_idle);
> -}
> -
> -void __init at91sam9g45_pm_init(void)
> -{
> - at91_dt_ramc();
> - at91_pm_init(at91sam9_idle);
> -}
> -
> -void __init at91sam9x5_pm_init(void)
> +void __init at91sam9_pm_init(void)
>  {
>   at91_dt_ramc();
>   at91_pm_init(at91sam9_idle);
> --
> 2.11.0


Best Regards,
Wenyou Yang

RE: [PATCH v2 08/11] ARM: at91: pm: Tie the USB clock mask to the pmc

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月28日 19:20
> To: Nicolas Ferre - M43238 
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Boris
> Brezillon ; Wenyou Yang - A41535
> ; Alexandre Belloni  electrons.com>
> Subject: [PATCH v2 08/11] ARM: at91: pm: Tie the USB clock mask to the pmc
> 
> The USB clocks mask (uhp_udp_mask) depends on the pmc. Tie it to the pmc id
> instead of the SoC.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Wenyou Yang 

> ---
>  arch/arm/mach-at91/pm.c | 37 +++--
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> ddf62a006635..a7c047f0d21f 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -442,31 +442,46 @@ static void __init at91_pm_sram_init(void)
>   _pm_suspend_in_sram,
> at91_pm_suspend_in_sram_sz);  }
> 
> +struct pmc_info {
> + unsigned long uhp_udp_mask;
> +};
> +
> +static const struct pmc_info pmc_infos[] __initconst = {
> + { .uhp_udp_mask = AT91RM9200_PMC_UHP |
> AT91RM9200_PMC_UDP },
> + { .uhp_udp_mask = AT91SAM926x_PMC_UHP |
> AT91SAM926x_PMC_UDP },
> + { .uhp_udp_mask = AT91SAM926x_PMC_UHP }, };
> +
>  static const struct of_device_id atmel_pmc_ids[] __initconst = {
> - { .compatible = "atmel,at91rm9200-pmc"  },
> - { .compatible = "atmel,at91sam9260-pmc" },
> - { .compatible = "atmel,at91sam9g45-pmc" },
> - { .compatible = "atmel,at91sam9n12-pmc" },
> - { .compatible = "atmel,at91sam9x5-pmc" },
> - { .compatible = "atmel,sama5d3-pmc" },
> - { .compatible = "atmel,sama5d2-pmc" },
> + { .compatible = "atmel,at91rm9200-pmc", .data = _infos[0] },
> + { .compatible = "atmel,at91sam9260-pmc", .data = _infos[1] },
> + { .compatible = "atmel,at91sam9g45-pmc", .data = _infos[2] },
> + { .compatible = "atmel,at91sam9n12-pmc", .data = _infos[1] },
> + { .compatible = "atmel,at91sam9x5-pmc", .data = _infos[1] },
> + { .compatible = "atmel,sama5d3-pmc", .data = _infos[1] },
> + { .compatible = "atmel,sama5d2-pmc", .data = _infos[1] },
>   { /* sentinel */ },
>  };
> 
>  static void __init at91_pm_init(void (*pm_idle)(void))  {
>   struct device_node *pmc_np;
> + const struct of_device_id *of_id;
> + const struct pmc_info *pmc;
> 
>   if (at91_cpuidle_device.dev.platform_data)
>   platform_device_register(_cpuidle_device);
> 
> - pmc_np = of_find_matching_node(NULL, atmel_pmc_ids);
> + pmc_np = of_find_matching_node_and_match(NULL, atmel_pmc_ids,
> _id);
>   pm_data.pmc = of_iomap(pmc_np, 0);
>   if (!pm_data.pmc) {
>   pr_err("AT91: PM not supported, PMC not found\n");
>   return;
>   }
> 
> + pmc = of_id->data;
> + pm_data.uhp_udp_mask = pmc->uhp_udp_mask;
> +
>   if (pm_idle)
>   arm_pm_idle = pm_idle;
> 
> @@ -487,35 +502,29 @@ void __init at91rm9200_pm_init(void)
>*/
>   at91_ramc_write(0, AT91_MC_SDRAMC_LPR, 0);
> 
> - pm_data.uhp_udp_mask = AT91RM9200_PMC_UHP |
> AT91RM9200_PMC_UDP;
> -
>   at91_pm_init(at91rm9200_idle);
>  }
> 
>  void __init at91sam9260_pm_init(void)
>  {
>   at91_dt_ramc();
> - pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP |
> AT91SAM926x_PMC_UDP;
>   at91_pm_init(at91sam9_idle);
>  }
> 
>  void __init at91sam9g45_pm_init(void)
>  {
>   at91_dt_ramc();
> - pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP;
>   at91_pm_init(at91sam9_idle);
>  }
> 
>  void __init at91sam9x5_pm_init(void)
>  {
>   at91_dt_ramc();
> - pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP |
> AT91SAM926x_PMC_UDP;
>   at91_pm_init(at91sam9_idle);
>  }
> 
>  void __init sama5_pm_init(void)
>  {
>   at91_dt_ramc();
> - pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP |
> AT91SAM926x_PMC_UDP;
>   at91_pm_init(NULL);
>  }
> --
> 2.11.0


Best Regards,
Wenyou Yang

RE: [PATCH v2 08/11] ARM: at91: pm: Tie the USB clock mask to the pmc

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月28日 19:20
> To: Nicolas Ferre - M43238 
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Boris
> Brezillon ; Wenyou Yang - A41535
> ; Alexandre Belloni  electrons.com>
> Subject: [PATCH v2 08/11] ARM: at91: pm: Tie the USB clock mask to the pmc
> 
> The USB clocks mask (uhp_udp_mask) depends on the pmc. Tie it to the pmc id
> instead of the SoC.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Wenyou Yang 

> ---
>  arch/arm/mach-at91/pm.c | 37 +++--
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> ddf62a006635..a7c047f0d21f 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -442,31 +442,46 @@ static void __init at91_pm_sram_init(void)
>   _pm_suspend_in_sram,
> at91_pm_suspend_in_sram_sz);  }
> 
> +struct pmc_info {
> + unsigned long uhp_udp_mask;
> +};
> +
> +static const struct pmc_info pmc_infos[] __initconst = {
> + { .uhp_udp_mask = AT91RM9200_PMC_UHP |
> AT91RM9200_PMC_UDP },
> + { .uhp_udp_mask = AT91SAM926x_PMC_UHP |
> AT91SAM926x_PMC_UDP },
> + { .uhp_udp_mask = AT91SAM926x_PMC_UHP }, };
> +
>  static const struct of_device_id atmel_pmc_ids[] __initconst = {
> - { .compatible = "atmel,at91rm9200-pmc"  },
> - { .compatible = "atmel,at91sam9260-pmc" },
> - { .compatible = "atmel,at91sam9g45-pmc" },
> - { .compatible = "atmel,at91sam9n12-pmc" },
> - { .compatible = "atmel,at91sam9x5-pmc" },
> - { .compatible = "atmel,sama5d3-pmc" },
> - { .compatible = "atmel,sama5d2-pmc" },
> + { .compatible = "atmel,at91rm9200-pmc", .data = _infos[0] },
> + { .compatible = "atmel,at91sam9260-pmc", .data = _infos[1] },
> + { .compatible = "atmel,at91sam9g45-pmc", .data = _infos[2] },
> + { .compatible = "atmel,at91sam9n12-pmc", .data = _infos[1] },
> + { .compatible = "atmel,at91sam9x5-pmc", .data = _infos[1] },
> + { .compatible = "atmel,sama5d3-pmc", .data = _infos[1] },
> + { .compatible = "atmel,sama5d2-pmc", .data = _infos[1] },
>   { /* sentinel */ },
>  };
> 
>  static void __init at91_pm_init(void (*pm_idle)(void))  {
>   struct device_node *pmc_np;
> + const struct of_device_id *of_id;
> + const struct pmc_info *pmc;
> 
>   if (at91_cpuidle_device.dev.platform_data)
>   platform_device_register(_cpuidle_device);
> 
> - pmc_np = of_find_matching_node(NULL, atmel_pmc_ids);
> + pmc_np = of_find_matching_node_and_match(NULL, atmel_pmc_ids,
> _id);
>   pm_data.pmc = of_iomap(pmc_np, 0);
>   if (!pm_data.pmc) {
>   pr_err("AT91: PM not supported, PMC not found\n");
>   return;
>   }
> 
> + pmc = of_id->data;
> + pm_data.uhp_udp_mask = pmc->uhp_udp_mask;
> +
>   if (pm_idle)
>   arm_pm_idle = pm_idle;
> 
> @@ -487,35 +502,29 @@ void __init at91rm9200_pm_init(void)
>*/
>   at91_ramc_write(0, AT91_MC_SDRAMC_LPR, 0);
> 
> - pm_data.uhp_udp_mask = AT91RM9200_PMC_UHP |
> AT91RM9200_PMC_UDP;
> -
>   at91_pm_init(at91rm9200_idle);
>  }
> 
>  void __init at91sam9260_pm_init(void)
>  {
>   at91_dt_ramc();
> - pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP |
> AT91SAM926x_PMC_UDP;
>   at91_pm_init(at91sam9_idle);
>  }
> 
>  void __init at91sam9g45_pm_init(void)
>  {
>   at91_dt_ramc();
> - pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP;
>   at91_pm_init(at91sam9_idle);
>  }
> 
>  void __init at91sam9x5_pm_init(void)
>  {
>   at91_dt_ramc();
> - pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP |
> AT91SAM926x_PMC_UDP;
>   at91_pm_init(at91sam9_idle);
>  }
> 
>  void __init sama5_pm_init(void)
>  {
>   at91_dt_ramc();
> - pm_data.uhp_udp_mask = AT91SAM926x_PMC_UHP |
> AT91SAM926x_PMC_UDP;
>   at91_pm_init(NULL);
>  }
> --
> 2.11.0


Best Regards,
Wenyou Yang

RE: [PATCH v2 10/11] ARM: at91: pm: Remove at91_pm_set_standby

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月28日 19:20
> To: Nicolas Ferre - M43238 
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Boris
> Brezillon ; Wenyou Yang - A41535
> ; Alexandre Belloni  electrons.com>
> Subject: [PATCH v2 10/11] ARM: at91: pm: Remove at91_pm_set_standby
> 
> Merge at91_pm_set_standby() in at91_dt_ramc as this is the only callsite.
> That moves it to the init section.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Wenyou Yang 

> ---
>  arch/arm/mach-at91/pm.c | 10 ++
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> dedfe9038336..2cd27c830ab6 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -205,12 +205,6 @@ static struct platform_device at91_cpuidle_device = {
>   .name = "cpuidle-at91",
>  };
> 
> -static void at91_pm_set_standby(void (*at91_standby)(void)) -{
> - if (at91_standby)
> - at91_cpuidle_device.dev.platform_data = at91_standby;
> -}
> -
>  /*
>   * The AT91RM9200 goes into self-refresh mode with this command, and will
>   * terminate self-refresh automatically on the next SDRAM access.
> @@ -354,7 +348,7 @@ static __init void at91_dt_ramc(void)
>   struct device_node *np;
>   const struct of_device_id *of_id;
>   int idx = 0;
> - const void *standby = NULL;
> + void *standby = NULL;
>   const struct ramc_info *ramc;
> 
>   for_each_matching_node_and_match(np, ramc_ids, _id) { @@ -378,7
> +372,7 @@ static __init void at91_dt_ramc(void)
>   return;
>   }
> 
> - at91_pm_set_standby(standby);
> + at91_cpuidle_device.dev.platform_data = standby;
>  }
> 
>  static void at91rm9200_idle(void)
> --
> 2.11.0

Best Regards,
Wenyou Yang

RE: [PATCH v2 10/11] ARM: at91: pm: Remove at91_pm_set_standby

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月28日 19:20
> To: Nicolas Ferre - M43238 
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Boris
> Brezillon ; Wenyou Yang - A41535
> ; Alexandre Belloni  electrons.com>
> Subject: [PATCH v2 10/11] ARM: at91: pm: Remove at91_pm_set_standby
> 
> Merge at91_pm_set_standby() in at91_dt_ramc as this is the only callsite.
> That moves it to the init section.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Wenyou Yang 

> ---
>  arch/arm/mach-at91/pm.c | 10 ++
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> dedfe9038336..2cd27c830ab6 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -205,12 +205,6 @@ static struct platform_device at91_cpuidle_device = {
>   .name = "cpuidle-at91",
>  };
> 
> -static void at91_pm_set_standby(void (*at91_standby)(void)) -{
> - if (at91_standby)
> - at91_cpuidle_device.dev.platform_data = at91_standby;
> -}
> -
>  /*
>   * The AT91RM9200 goes into self-refresh mode with this command, and will
>   * terminate self-refresh automatically on the next SDRAM access.
> @@ -354,7 +348,7 @@ static __init void at91_dt_ramc(void)
>   struct device_node *np;
>   const struct of_device_id *of_id;
>   int idx = 0;
> - const void *standby = NULL;
> + void *standby = NULL;
>   const struct ramc_info *ramc;
> 
>   for_each_matching_node_and_match(np, ramc_ids, _id) { @@ -378,7
> +372,7 @@ static __init void at91_dt_ramc(void)
>   return;
>   }
> 
> - at91_pm_set_standby(standby);
> + at91_cpuidle_device.dev.platform_data = standby;
>  }
> 
>  static void at91rm9200_idle(void)
> --
> 2.11.0

Best Regards,
Wenyou Yang

RE: [PATCH v2 03/11] ARM: at91: pm: Move global variables into at91_pm_data

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月28日 19:20
> To: Nicolas Ferre - M43238 
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Boris
> Brezillon ; Wenyou Yang - A41535
> ; Alexandre Belloni  electrons.com>
> Subject: [PATCH v2 03/11] ARM: at91: pm: Move global variables into
> at91_pm_data
> 
> Instead of having separate global variables to hold IP addresses, move them to
> struct at91_pm_data.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Wenyou Yang 

> ---
>  arch/arm/mach-at91/pm.c | 44 +---
>  1 file changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> 41789aa4df86..c780dda3b604 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -26,8 +26,6 @@
>  #include "generic.h"
>  #include "pm.h"
> 
> -static void __iomem *pmc;
> -
>  /*
>   * FIXME: this is needed to communicate between the pinctrl driver and
>   * the PM implementation in the machine. Possibly part of the PM @@ -40,17
> +38,17 @@ extern void at91_pinctrl_gpio_resume(void);  #endif
> 
>  static struct {
> + void __iomem *pmc;
> + void __iomem *ramc[2];
>   unsigned long uhp_udp_mask;
>   int memctrl;
>  } at91_pm_data;
> 
> -static void __iomem *at91_ramc_base[2];  #define at91_ramc_read(id, field) \
> - __raw_readl(at91_ramc_base[id] + field)
> + __raw_readl(at91_pm_data.ramc[id] + field)
> 
>  #define at91_ramc_write(id, field, value) \
> - __raw_writel(value, at91_ramc_base[id] + field)
> -
> + __raw_writel(value, at91_pm_data.ramc[id] + field)
> 
>  static int at91_pm_valid_state(suspend_state_t state)  { @@ -86,7 +84,7 @@
> static int at91_pm_verify_clocks(void)
>   unsigned long scsr;
>   int i;
> 
> - scsr = readl(pmc + AT91_PMC_SCSR);
> + scsr = readl(at91_pm_data.pmc + AT91_PMC_SCSR);
> 
>   /* USB must not be using PLLB */
>   if ((scsr & at91_pm_data.uhp_udp_mask) != 0) { @@ -100,7 +98,7 @@
> static int at91_pm_verify_clocks(void)
> 
>   if ((scsr & (AT91_PMC_PCK0 << i)) == 0)
>   continue;
> - css = readl(pmc + AT91_PMC_PCKR(i)) & AT91_PMC_CSS;
> + css = readl(at91_pm_data.pmc + AT91_PMC_PCKR(i)) &
> AT91_PMC_CSS;
>   if (css != AT91_PMC_CSS_SLOW) {
>   pr_err("AT91: PM - Suspend-to-RAM with PCK%d
> src %d\n", i, css);
>   return 0;
> @@ -143,8 +141,8 @@ static void at91_pm_suspend(suspend_state_t state)
>   flush_cache_all();
>   outer_disable();
> 
> - at91_suspend_sram_fn(pmc, at91_ramc_base[0],
> -  at91_ramc_base[1], pm_data);
> + at91_suspend_sram_fn(at91_pm_data.pmc, at91_pm_data.ramc[0],
> +  at91_pm_data.ramc[1], pm_data);
> 
>   outer_resume();
>  }
> @@ -247,7 +245,7 @@ static void at91rm9200_standby(void)
>   "mcrp15, 0, %0, c7, c0, 4\n\t"
>   "str%5, [%1, %2]"
>   :
> - : "r" (0), "r" (at91_ramc_base[0]), "r" (AT91_MC_SDRAMC_LPR),
> + : "r" (0), "r" (at91_pm_data.ramc[0]), "r"
> (AT91_MC_SDRAMC_LPR),
> "r" (1), "r" (AT91_MC_SDRAMC_SRR),
> "r" (lpr));
>  }
> @@ -262,7 +260,7 @@ static void at91_ddr_standby(void)
>   u32 lpr0, lpr1 = 0;
>   u32 saved_lpr0, saved_lpr1 = 0;
> 
> - if (at91_ramc_base[1]) {
> + if (at91_pm_data.ramc[1]) {
>   saved_lpr1 = at91_ramc_read(1, AT91_DDRSDRC_LPR);
>   lpr1 = saved_lpr1 & ~AT91_DDRSDRC_LPCB;
>   lpr1 |= AT91_DDRSDRC_LPCB_SELF_REFRESH; @@ -274,13
> +272,13 @@ static void at91_ddr_standby(void)
> 
>   /* self-refresh mode now */
>   at91_ramc_write(0, AT91_DDRSDRC_LPR, lpr0);
> - if (at91_ramc_base[1])
> + if (at91_pm_data.ramc[1])
>   at91_ramc_write(1, AT91_DDRSDRC_LPR, lpr1);
> 
>   cpu_do_idle();
> 
>   at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr0);
> - if (at91_ramc_base[1])
> + if (at91_pm_data.ramc[1])
>   at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);  }
> 
> @@ -308,7 +306,7 @@ static void at91sam9_sdram_standby(void)
>   u32 lpr0, lpr1 = 0;
>   u32 saved_lpr0, saved_lpr1 = 0;
> 
> - if (at91_ramc_base[1]) {
> + if (at91_pm_data.ramc[1]) {
>   saved_lpr1 = at91_ramc_read(1, AT91_SDRAMC_LPR);
>   lpr1 = saved_lpr1 & ~AT91_SDRAMC_LPCB;
>   lpr1 |= AT91_SDRAMC_LPCB_SELF_REFRESH; @@ -320,13
> +318,13 @@ static void at91sam9_sdram_standby(void)
> 
>   /* self-refresh mode now */
>   at91_ramc_write(0, AT91_SDRAMC_LPR, lpr0);
> - if (at91_ramc_base[1])
> + 

RE: [PATCH v2 03/11] ARM: at91: pm: Move global variables into at91_pm_data

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月28日 19:20
> To: Nicolas Ferre - M43238 
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Boris
> Brezillon ; Wenyou Yang - A41535
> ; Alexandre Belloni  electrons.com>
> Subject: [PATCH v2 03/11] ARM: at91: pm: Move global variables into
> at91_pm_data
> 
> Instead of having separate global variables to hold IP addresses, move them to
> struct at91_pm_data.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Wenyou Yang 

> ---
>  arch/arm/mach-at91/pm.c | 44 +---
>  1 file changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> 41789aa4df86..c780dda3b604 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -26,8 +26,6 @@
>  #include "generic.h"
>  #include "pm.h"
> 
> -static void __iomem *pmc;
> -
>  /*
>   * FIXME: this is needed to communicate between the pinctrl driver and
>   * the PM implementation in the machine. Possibly part of the PM @@ -40,17
> +38,17 @@ extern void at91_pinctrl_gpio_resume(void);  #endif
> 
>  static struct {
> + void __iomem *pmc;
> + void __iomem *ramc[2];
>   unsigned long uhp_udp_mask;
>   int memctrl;
>  } at91_pm_data;
> 
> -static void __iomem *at91_ramc_base[2];  #define at91_ramc_read(id, field) \
> - __raw_readl(at91_ramc_base[id] + field)
> + __raw_readl(at91_pm_data.ramc[id] + field)
> 
>  #define at91_ramc_write(id, field, value) \
> - __raw_writel(value, at91_ramc_base[id] + field)
> -
> + __raw_writel(value, at91_pm_data.ramc[id] + field)
> 
>  static int at91_pm_valid_state(suspend_state_t state)  { @@ -86,7 +84,7 @@
> static int at91_pm_verify_clocks(void)
>   unsigned long scsr;
>   int i;
> 
> - scsr = readl(pmc + AT91_PMC_SCSR);
> + scsr = readl(at91_pm_data.pmc + AT91_PMC_SCSR);
> 
>   /* USB must not be using PLLB */
>   if ((scsr & at91_pm_data.uhp_udp_mask) != 0) { @@ -100,7 +98,7 @@
> static int at91_pm_verify_clocks(void)
> 
>   if ((scsr & (AT91_PMC_PCK0 << i)) == 0)
>   continue;
> - css = readl(pmc + AT91_PMC_PCKR(i)) & AT91_PMC_CSS;
> + css = readl(at91_pm_data.pmc + AT91_PMC_PCKR(i)) &
> AT91_PMC_CSS;
>   if (css != AT91_PMC_CSS_SLOW) {
>   pr_err("AT91: PM - Suspend-to-RAM with PCK%d
> src %d\n", i, css);
>   return 0;
> @@ -143,8 +141,8 @@ static void at91_pm_suspend(suspend_state_t state)
>   flush_cache_all();
>   outer_disable();
> 
> - at91_suspend_sram_fn(pmc, at91_ramc_base[0],
> -  at91_ramc_base[1], pm_data);
> + at91_suspend_sram_fn(at91_pm_data.pmc, at91_pm_data.ramc[0],
> +  at91_pm_data.ramc[1], pm_data);
> 
>   outer_resume();
>  }
> @@ -247,7 +245,7 @@ static void at91rm9200_standby(void)
>   "mcrp15, 0, %0, c7, c0, 4\n\t"
>   "str%5, [%1, %2]"
>   :
> - : "r" (0), "r" (at91_ramc_base[0]), "r" (AT91_MC_SDRAMC_LPR),
> + : "r" (0), "r" (at91_pm_data.ramc[0]), "r"
> (AT91_MC_SDRAMC_LPR),
> "r" (1), "r" (AT91_MC_SDRAMC_SRR),
> "r" (lpr));
>  }
> @@ -262,7 +260,7 @@ static void at91_ddr_standby(void)
>   u32 lpr0, lpr1 = 0;
>   u32 saved_lpr0, saved_lpr1 = 0;
> 
> - if (at91_ramc_base[1]) {
> + if (at91_pm_data.ramc[1]) {
>   saved_lpr1 = at91_ramc_read(1, AT91_DDRSDRC_LPR);
>   lpr1 = saved_lpr1 & ~AT91_DDRSDRC_LPCB;
>   lpr1 |= AT91_DDRSDRC_LPCB_SELF_REFRESH; @@ -274,13
> +272,13 @@ static void at91_ddr_standby(void)
> 
>   /* self-refresh mode now */
>   at91_ramc_write(0, AT91_DDRSDRC_LPR, lpr0);
> - if (at91_ramc_base[1])
> + if (at91_pm_data.ramc[1])
>   at91_ramc_write(1, AT91_DDRSDRC_LPR, lpr1);
> 
>   cpu_do_idle();
> 
>   at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr0);
> - if (at91_ramc_base[1])
> + if (at91_pm_data.ramc[1])
>   at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);  }
> 
> @@ -308,7 +306,7 @@ static void at91sam9_sdram_standby(void)
>   u32 lpr0, lpr1 = 0;
>   u32 saved_lpr0, saved_lpr1 = 0;
> 
> - if (at91_ramc_base[1]) {
> + if (at91_pm_data.ramc[1]) {
>   saved_lpr1 = at91_ramc_read(1, AT91_SDRAMC_LPR);
>   lpr1 = saved_lpr1 & ~AT91_SDRAMC_LPCB;
>   lpr1 |= AT91_SDRAMC_LPCB_SELF_REFRESH; @@ -320,13
> +318,13 @@ static void at91sam9_sdram_standby(void)
> 
>   /* self-refresh mode now */
>   at91_ramc_write(0, AT91_SDRAMC_LPR, lpr0);
> - if (at91_ramc_base[1])
> + if (at91_pm_data.ramc[1])
>   at91_ramc_write(1, AT91_SDRAMC_LPR, lpr1);
> 
>   cpu_do_idle();
> 
>   at91_ramc_write(0, AT91_SDRAMC_LPR, saved_lpr0);
> - 

RE: [PATCH v2 02/11] ARM: at91: pm: Move at91_ramc_read/write to pm.c

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月28日 19:19
> To: Nicolas Ferre - M43238 
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Boris
> Brezillon ; Wenyou Yang - A41535
> ; Alexandre Belloni  electrons.com>
> Subject: [PATCH v2 02/11] ARM: at91: pm: Move at91_ramc_read/write to pm.c
> 
> Those macros are only used in pm.c, move them there so we can remove the test
> on __ASSEMBLY__.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Wenyou Yang 

> ---
>  arch/arm/mach-at91/pm.c | 6 ++
>  arch/arm/mach-at91/pm.h | 8 
>  2 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> 9e2b5c1e503e..41789aa4df86 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -45,6 +45,12 @@ static struct {
>  } at91_pm_data;
> 
>  static void __iomem *at91_ramc_base[2];
> +#define at91_ramc_read(id, field) \
> + __raw_readl(at91_ramc_base[id] + field)
> +
> +#define at91_ramc_write(id, field, value) \
> + __raw_writel(value, at91_ramc_base[id] + field)
> +
> 
>  static int at91_pm_valid_state(suspend_state_t state)  { diff --git
> a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h index
> bf980c6ef294..8eed156ef19a 100644
> --- a/arch/arm/mach-at91/pm.h
> +++ b/arch/arm/mach-at91/pm.h
> @@ -17,14 +17,6 @@
>  #include 
>  #include 
> 
> -#ifndef __ASSEMBLY__
> -#define at91_ramc_read(id, field) \
> - __raw_readl(at91_ramc_base[id] + field)
> -
> -#define at91_ramc_write(id, field, value) \
> - __raw_writel(value, at91_ramc_base[id] + field)
> -#endif
> -
>  #define AT91_MEMCTRL_MC  0
>  #define AT91_MEMCTRL_SDRAMC  1
>  #define AT91_MEMCTRL_DDRSDR  2
> --
> 2.11.0

RE: [PATCH v2 02/11] ARM: at91: pm: Move at91_ramc_read/write to pm.c

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月28日 19:19
> To: Nicolas Ferre - M43238 
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Boris
> Brezillon ; Wenyou Yang - A41535
> ; Alexandre Belloni  electrons.com>
> Subject: [PATCH v2 02/11] ARM: at91: pm: Move at91_ramc_read/write to pm.c
> 
> Those macros are only used in pm.c, move them there so we can remove the test
> on __ASSEMBLY__.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Wenyou Yang 

> ---
>  arch/arm/mach-at91/pm.c | 6 ++
>  arch/arm/mach-at91/pm.h | 8 
>  2 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> 9e2b5c1e503e..41789aa4df86 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -45,6 +45,12 @@ static struct {
>  } at91_pm_data;
> 
>  static void __iomem *at91_ramc_base[2];
> +#define at91_ramc_read(id, field) \
> + __raw_readl(at91_ramc_base[id] + field)
> +
> +#define at91_ramc_write(id, field, value) \
> + __raw_writel(value, at91_ramc_base[id] + field)
> +
> 
>  static int at91_pm_valid_state(suspend_state_t state)  { diff --git
> a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h index
> bf980c6ef294..8eed156ef19a 100644
> --- a/arch/arm/mach-at91/pm.h
> +++ b/arch/arm/mach-at91/pm.h
> @@ -17,14 +17,6 @@
>  #include 
>  #include 
> 
> -#ifndef __ASSEMBLY__
> -#define at91_ramc_read(id, field) \
> - __raw_readl(at91_ramc_base[id] + field)
> -
> -#define at91_ramc_write(id, field, value) \
> - __raw_writel(value, at91_ramc_base[id] + field)
> -#endif
> -
>  #define AT91_MEMCTRL_MC  0
>  #define AT91_MEMCTRL_SDRAMC  1
>  #define AT91_MEMCTRL_DDRSDR  2
> --
> 2.11.0

RE: [PATCH v2 01/11] ARM: at91: pm: Cleanup headers

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月28日 19:19
> To: Nicolas Ferre - M43238 
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Boris
> Brezillon ; Wenyou Yang - A41535
> ; Alexandre Belloni  electrons.com>
> Subject: [PATCH v2 01/11] ARM: at91: pm: Cleanup headers
> 
> Remove unnecessary header inclusions and reorder the remaining ones.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Wenyou Yang 

> ---
>  arch/arm/mach-at91/pm.c | 21 +
>  1 file changed, 5 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> a277981f414d..9e2b5c1e503e 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -10,28 +10,17 @@
>   * (at your option) any later version.
>   */
> 
> -#include 
> -#include 
> -#include 
> -#include 
>  #include 
> -#include 
> -#include 
> -#include 
> +#include 
> +#include 
>  #include 
>  #include 
> -#include 
> -#include 
> -#include 
> -#include 
> +#include 
> +
>  #include 
> 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
>  #include 
> +#include 
>  #include 
> 
>  #include "generic.h"
> --
> 2.11.0

RE: [PATCH v2 01/11] ARM: at91: pm: Cleanup headers

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月28日 19:19
> To: Nicolas Ferre - M43238 
> Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Boris
> Brezillon ; Wenyou Yang - A41535
> ; Alexandre Belloni  electrons.com>
> Subject: [PATCH v2 01/11] ARM: at91: pm: Cleanup headers
> 
> Remove unnecessary header inclusions and reorder the remaining ones.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Wenyou Yang 

> ---
>  arch/arm/mach-at91/pm.c | 21 +
>  1 file changed, 5 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> a277981f414d..9e2b5c1e503e 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -10,28 +10,17 @@
>   * (at your option) any later version.
>   */
> 
> -#include 
> -#include 
> -#include 
> -#include 
>  #include 
> -#include 
> -#include 
> -#include 
> +#include 
> +#include 
>  #include 
>  #include 
> -#include 
> -#include 
> -#include 
> -#include 
> +#include 
> +
>  #include 
> 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
>  #include 
> +#include 
>  #include 
> 
>  #include "generic.h"
> --
> 2.11.0

RE: [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月3日 1:31
> To: Guenter Roeck <li...@roeck-us.net>
> Cc: Wim Van Sebroeck <w...@iguana.be>; Nicolas Ferre - M43238
> <nicolas.fe...@microchip.com>; Wenyou Yang - A41535
> <wenyou.y...@microchip.com>; linux-watch...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org; Alexandre Belloni
> <alexandre.bell...@free-electrons.com>
> Subject: [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling
> 
> The datasheet states: "When setting the WDDIS bit, and while it is set, the 
> fields
> WDV and WDD must not be modified."
> 
> Because the whole configuration is already cached inside .mr, wait for the 
> user to
> enable the watchdog to configure it so it is enabled and configured at the 
> same
> time (what the IP is actually expecting).
> 
> When the watchdog is already enabled, it is not an issue to reconfigure it.

Indeed, thanks.
Acked-by: Wenyou.Yang <wenyou.y...@microchip.com>

> 
> Signed-off-by: Alexandre Belloni <alexandre.bell...@free-electrons.com>
> ---
>  drivers/watchdog/sama5d4_wdt.c | 48 ++--
> --
>  1 file changed, 30 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index f709962018ac..5cee20caca78 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -44,6 +44,8 @@ MODULE_PARM_DESC(nowayout,
>   "Watchdog cannot be stopped once started (default="
>   __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> 
> +#define wdt_enabled (!(wdt->mr & AT91_WDT_WDDIS))
> +
>  #define wdt_read(wdt, field) \
>   readl_relaxed((wdt)->reg_base + (field))
> 
> @@ -89,7 +91,16 @@ static int sama5d4_wdt_set_timeout(struct
> watchdog_device *wdd,
>   wdt->mr &= ~AT91_WDT_WDD;
>   wdt->mr |= AT91_WDT_SET_WDV(value);
>   wdt->mr |= AT91_WDT_SET_WDD(value);
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> +
> + /*
> +  * WDDIS has to be 0 when updating WDD/WDV. The datasheet states:
> When
> +  * setting the WDDIS bit, and while it is set, the fields WDV and WDD
> +  * must not be modified.
> +  * If the watchdog is enabled, then the timeout can be updated. Else,
> +  * wait that the user enables it.
> +  */
> + if (wdt_enabled)
> + wdt_write(wdt, AT91_WDT_MR, wdt->mr &
> ~AT91_WDT_WDDIS);
> 
>   wdd->timeout = timeout;
> 
> @@ -145,23 +156,20 @@ static int of_sama5d4_wdt_init(struct device_node *np,
> struct sama5d4_wdt *wdt)
> 
>  static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)  {
> - struct watchdog_device *wdd = >wdd;
> - u32 value = WDT_SEC2TICKS(wdd->timeout);
>   u32 reg;
> -
>   /*
> -  * Because the fields WDV and WDD must not be modified when the
> WDDIS
> -  * bit is set, so clear the WDDIS bit before writing the WDT_MR.
> +  * When booting and resuming, the bootloader may have changed the
> +  * watchdog configuration.
> +  * If the watchdog is already running, we can safely update it.
> +  * Else, we have to disable it properly.
>*/
> - reg = wdt_read(wdt, AT91_WDT_MR);
> - reg &= ~AT91_WDT_WDDIS;
> - wdt_write(wdt, AT91_WDT_MR, reg);
> -
> - wdt->mr |= AT91_WDT_SET_WDD(value);
> - wdt->mr |= AT91_WDT_SET_WDV(value);
> -
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> -
> + if (wdt_enabled) {
> + wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> + } else {
> + reg = wdt_read(wdt, AT91_WDT_MR);
> + if (!(reg & AT91_WDT_WDDIS))
> + wdt_write(wdt, AT91_WDT_MR, reg |
> AT91_WDT_WDDIS);
> + }
>   return 0;
>  }
> 
> @@ -172,6 +180,7 @@ static int sama5d4_wdt_probe(struct platform_device
> *pdev)
>   struct resource *res;
>   void __iomem *regs;
>   u32 irq = 0;
> + u32 timeout;
>   int ret;
> 
>   wdt = devm_kzalloc(>dev, sizeof(*wdt), GFP_KERNEL); @@ -
> 221,6 +230,11 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
>   return ret;
>   }
> 
> + timeout = WDT_SEC2TICKS(wdd->timeout);
> +
> + wdt->mr |= AT91_WDT_SET_WDD(timeout);
> + wdt->mr |= AT91_WDT_SET_WDV(timeout);
> +
>   ret = sama5d4_wdt_init(wdt);
>   if (ret)
>   return ret;
> @@ -263,9 +277,7 @@ static int sama5d4_wdt_resume(struct device *dev)  {
>   struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
> 
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
> - if (wdt->mr & AT91_WDT_WDDIS)
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> + sama5d4_wdt_init(wdt);
> 
>   return 0;
>  }
> --
> 2.11.0

Best Regards,
Wenyou Yang

RE: [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月3日 1:31
> To: Guenter Roeck 
> Cc: Wim Van Sebroeck ; Nicolas Ferre - M43238
> ; Wenyou Yang - A41535
> ; linux-watch...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org; Alexandre Belloni
> 
> Subject: [PATCH 1/4] watchdog: sama5d4: fix WDDIS handling
> 
> The datasheet states: "When setting the WDDIS bit, and while it is set, the 
> fields
> WDV and WDD must not be modified."
> 
> Because the whole configuration is already cached inside .mr, wait for the 
> user to
> enable the watchdog to configure it so it is enabled and configured at the 
> same
> time (what the IP is actually expecting).
> 
> When the watchdog is already enabled, it is not an issue to reconfigure it.

Indeed, thanks.
Acked-by: Wenyou.Yang 

> 
> Signed-off-by: Alexandre Belloni 
> ---
>  drivers/watchdog/sama5d4_wdt.c | 48 ++--
> --
>  1 file changed, 30 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index f709962018ac..5cee20caca78 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -44,6 +44,8 @@ MODULE_PARM_DESC(nowayout,
>   "Watchdog cannot be stopped once started (default="
>   __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> 
> +#define wdt_enabled (!(wdt->mr & AT91_WDT_WDDIS))
> +
>  #define wdt_read(wdt, field) \
>   readl_relaxed((wdt)->reg_base + (field))
> 
> @@ -89,7 +91,16 @@ static int sama5d4_wdt_set_timeout(struct
> watchdog_device *wdd,
>   wdt->mr &= ~AT91_WDT_WDD;
>   wdt->mr |= AT91_WDT_SET_WDV(value);
>   wdt->mr |= AT91_WDT_SET_WDD(value);
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> +
> + /*
> +  * WDDIS has to be 0 when updating WDD/WDV. The datasheet states:
> When
> +  * setting the WDDIS bit, and while it is set, the fields WDV and WDD
> +  * must not be modified.
> +  * If the watchdog is enabled, then the timeout can be updated. Else,
> +  * wait that the user enables it.
> +  */
> + if (wdt_enabled)
> + wdt_write(wdt, AT91_WDT_MR, wdt->mr &
> ~AT91_WDT_WDDIS);
> 
>   wdd->timeout = timeout;
> 
> @@ -145,23 +156,20 @@ static int of_sama5d4_wdt_init(struct device_node *np,
> struct sama5d4_wdt *wdt)
> 
>  static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)  {
> - struct watchdog_device *wdd = >wdd;
> - u32 value = WDT_SEC2TICKS(wdd->timeout);
>   u32 reg;
> -
>   /*
> -  * Because the fields WDV and WDD must not be modified when the
> WDDIS
> -  * bit is set, so clear the WDDIS bit before writing the WDT_MR.
> +  * When booting and resuming, the bootloader may have changed the
> +  * watchdog configuration.
> +  * If the watchdog is already running, we can safely update it.
> +  * Else, we have to disable it properly.
>*/
> - reg = wdt_read(wdt, AT91_WDT_MR);
> - reg &= ~AT91_WDT_WDDIS;
> - wdt_write(wdt, AT91_WDT_MR, reg);
> -
> - wdt->mr |= AT91_WDT_SET_WDD(value);
> - wdt->mr |= AT91_WDT_SET_WDV(value);
> -
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> -
> + if (wdt_enabled) {
> + wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> + } else {
> + reg = wdt_read(wdt, AT91_WDT_MR);
> + if (!(reg & AT91_WDT_WDDIS))
> + wdt_write(wdt, AT91_WDT_MR, reg |
> AT91_WDT_WDDIS);
> + }
>   return 0;
>  }
> 
> @@ -172,6 +180,7 @@ static int sama5d4_wdt_probe(struct platform_device
> *pdev)
>   struct resource *res;
>   void __iomem *regs;
>   u32 irq = 0;
> + u32 timeout;
>   int ret;
> 
>   wdt = devm_kzalloc(>dev, sizeof(*wdt), GFP_KERNEL); @@ -
> 221,6 +230,11 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
>   return ret;
>   }
> 
> + timeout = WDT_SEC2TICKS(wdd->timeout);
> +
> + wdt->mr |= AT91_WDT_SET_WDD(timeout);
> + wdt->mr |= AT91_WDT_SET_WDV(timeout);
> +
>   ret = sama5d4_wdt_init(wdt);
>   if (ret)
>   return ret;
> @@ -263,9 +277,7 @@ static int sama5d4_wdt_resume(struct device *dev)  {
>   struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
> 
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
> - if (wdt->mr & AT91_WDT_WDDIS)
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> + sama5d4_wdt_init(wdt);
> 
>   return 0;
>  }
> --
> 2.11.0

Best Regards,
Wenyou Yang

RE: [PATCH 2/4] watchdog: sama5d4: fix race condition

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月3日 1:31
> To: Guenter Roeck <li...@roeck-us.net>
> Cc: Wim Van Sebroeck <w...@iguana.be>; Nicolas Ferre - M43238
> <nicolas.fe...@microchip.com>; Wenyou Yang - A41535
> <wenyou.y...@microchip.com>; linux-watch...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org; Alexandre Belloni
> <alexandre.bell...@free-electrons.com>
> Subject: [PATCH 2/4] watchdog: sama5d4: fix race condition
> 
> WDT_MR and WDT_CR must not updated within three slow clock periods after
> the last ping (write to WDT_CR or WDT_MR). Ensure enough time has elapsed
> before writing those registers.
> wdt_write() waits for 4 periods to ensure at least 3 edges are seen by the IP.

Indeed.
Acked-by: Wenyou.Yang <wenyou.y...@microchip.com>

Thanks

> 
> Signed-off-by: Alexandre Belloni <alexandre.bell...@free-electrons.com>
> ---
>  drivers/watchdog/sama5d4_wdt.c | 33 +
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 5cee20caca78..362fd229786d 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -6,6 +6,7 @@
>   * Licensed under GPLv2.
>   */
> 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -29,6 +30,7 @@ struct sama5d4_wdt {
>   struct watchdog_device  wdd;
>   void __iomem*reg_base;
>   u32 mr;
> + unsigned long   last_ping;
>  };
> 
>  static int wdt_timeout = WDT_DEFAULT_TIMEOUT; @@ -49,8 +51,29 @@
> MODULE_PARM_DESC(nowayout,  #define wdt_read(wdt, field) \
>   readl_relaxed((wdt)->reg_base + (field))
> 
> -#define wdt_write(wtd, field, val) \
> - writel_relaxed((val), (wdt)->reg_base + (field))
> +/* 4 slow clock periods is 4/32768 = 122.07µs*/
> +#define WDT_DELAYusecs_to_jiffies(123)
> +
> +static void wdt_write(struct sama5d4_wdt *wdt, u32 field, u32 val) {
> + /*
> +  * WDT_CR and WDT_MR must not be modified within three slow clock
> +  * periods following a restart of the watchdog performed by a write
> +  * access in WDT_CR.
> +  */
> + while (time_before(jiffies, wdt->last_ping + WDT_DELAY))
> + usleep_range(30, 125);
> + writel_relaxed(val, wdt->reg_base + field);
> + wdt->last_ping = jiffies;
> +}
> +
> +static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32
> +val) {
> + if (time_before(jiffies, wdt->last_ping + WDT_DELAY))
> + udelay(123);
> + writel_relaxed(val, wdt->reg_base + field);
> + wdt->last_ping = jiffies;
> +}
> 
>  static int sama5d4_wdt_start(struct watchdog_device *wdd)  { @@ -164,11
> +187,12 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
>* Else, we have to disable it properly.
>*/
>   if (wdt_enabled) {
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> + wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
>   } else {
>   reg = wdt_read(wdt, AT91_WDT_MR);
>   if (!(reg & AT91_WDT_WDDIS))
> - wdt_write(wdt, AT91_WDT_MR, reg |
> AT91_WDT_WDDIS);
> + wdt_write_nosleep(wdt, AT91_WDT_MR,
> +   reg | AT91_WDT_WDDIS);
>   }
>   return 0;
>  }
> @@ -193,6 +217,7 @@ static int sama5d4_wdt_probe(struct platform_device
> *pdev)
>   wdd->ops = _wdt_ops;
>   wdd->min_timeout = MIN_WDT_TIMEOUT;
>   wdd->max_timeout = MAX_WDT_TIMEOUT;
> + wdt->last_ping = jiffies;
> 
>   watchdog_set_drvdata(wdd, wdt);
> 
> --
> 2.11.0


Best Regards,
Wenyou Yang

RE: [PATCH 2/4] watchdog: sama5d4: fix race condition

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年3月3日 1:31
> To: Guenter Roeck 
> Cc: Wim Van Sebroeck ; Nicolas Ferre - M43238
> ; Wenyou Yang - A41535
> ; linux-watch...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org; Alexandre Belloni
> 
> Subject: [PATCH 2/4] watchdog: sama5d4: fix race condition
> 
> WDT_MR and WDT_CR must not updated within three slow clock periods after
> the last ping (write to WDT_CR or WDT_MR). Ensure enough time has elapsed
> before writing those registers.
> wdt_write() waits for 4 periods to ensure at least 3 edges are seen by the IP.

Indeed.
Acked-by: Wenyou.Yang 

Thanks

> 
> Signed-off-by: Alexandre Belloni 
> ---
>  drivers/watchdog/sama5d4_wdt.c | 33 +
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 5cee20caca78..362fd229786d 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -6,6 +6,7 @@
>   * Licensed under GPLv2.
>   */
> 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -29,6 +30,7 @@ struct sama5d4_wdt {
>   struct watchdog_device  wdd;
>   void __iomem*reg_base;
>   u32 mr;
> + unsigned long   last_ping;
>  };
> 
>  static int wdt_timeout = WDT_DEFAULT_TIMEOUT; @@ -49,8 +51,29 @@
> MODULE_PARM_DESC(nowayout,  #define wdt_read(wdt, field) \
>   readl_relaxed((wdt)->reg_base + (field))
> 
> -#define wdt_write(wtd, field, val) \
> - writel_relaxed((val), (wdt)->reg_base + (field))
> +/* 4 slow clock periods is 4/32768 = 122.07µs*/
> +#define WDT_DELAYusecs_to_jiffies(123)
> +
> +static void wdt_write(struct sama5d4_wdt *wdt, u32 field, u32 val) {
> + /*
> +  * WDT_CR and WDT_MR must not be modified within three slow clock
> +  * periods following a restart of the watchdog performed by a write
> +  * access in WDT_CR.
> +  */
> + while (time_before(jiffies, wdt->last_ping + WDT_DELAY))
> + usleep_range(30, 125);
> + writel_relaxed(val, wdt->reg_base + field);
> + wdt->last_ping = jiffies;
> +}
> +
> +static void wdt_write_nosleep(struct sama5d4_wdt *wdt, u32 field, u32
> +val) {
> + if (time_before(jiffies, wdt->last_ping + WDT_DELAY))
> + udelay(123);
> + writel_relaxed(val, wdt->reg_base + field);
> + wdt->last_ping = jiffies;
> +}
> 
>  static int sama5d4_wdt_start(struct watchdog_device *wdd)  { @@ -164,11
> +187,12 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
>* Else, we have to disable it properly.
>*/
>   if (wdt_enabled) {
> - wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> + wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
>   } else {
>   reg = wdt_read(wdt, AT91_WDT_MR);
>   if (!(reg & AT91_WDT_WDDIS))
> - wdt_write(wdt, AT91_WDT_MR, reg |
> AT91_WDT_WDDIS);
> + wdt_write_nosleep(wdt, AT91_WDT_MR,
> +   reg | AT91_WDT_WDDIS);
>   }
>   return 0;
>  }
> @@ -193,6 +217,7 @@ static int sama5d4_wdt_probe(struct platform_device
> *pdev)
>   wdd->ops = _wdt_ops;
>   wdd->min_timeout = MIN_WDT_TIMEOUT;
>   wdd->max_timeout = MAX_WDT_TIMEOUT;
> + wdt->last_ping = jiffies;
> 
>   watchdog_set_drvdata(wdd, wdt);
> 
> --
> 2.11.0


Best Regards,
Wenyou Yang

RE: [PATCH v2] can: m_can: enable transmission of FD frame on latest version

[Wenyou.Yang]

HI Oliver, 

> -Original Message-
> From: Oliver Hartkopp [mailto:socket...@hartkopp.net]
> Sent: 2017年3月7日 5:26
> To: Marc Kleine-Budde ; Wenyou Yang - A41535
> ; Wolfgang Grandegger 
> Cc: Alexandre Belloni ; Florian Fainelli
> ; Quentin Schulz ;
> Wenyou Yang - A41535 ; Nicolas Ferre
> ; linux-...@vger.kernel.org; net...@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] can: m_can: enable transmission of FD frame on latest
> version
> 
> @Wenyou Yang: Can you please test the two patches posted here:

Tested on SAMA5D2 SoC, It works.

> 
> [PATCH 1/2] can: m_can: handle bitrate setup on IP core >= 3.1.x
> http://marc.info/?l=linux-can=148883529927720=2
> 
> [PATCH 2/2] can: m_can: handle frame transmission on IP core >= 3.1.x
> http://marc.info/?l=linux-can=148883529927718=2
> 
> Tnx & regards,
> Oliver

Thank you.

Best Regards,
Wenyou Yang

RE: [PATCH v2] can: m_can: enable transmission of FD frame on latest version

[Wenyou.Yang]

HI Oliver, 

> -Original Message-
> From: Oliver Hartkopp [mailto:socket...@hartkopp.net]
> Sent: 2017年3月7日 5:26
> To: Marc Kleine-Budde ; Wenyou Yang - A41535
> ; Wolfgang Grandegger 
> Cc: Alexandre Belloni ; Florian Fainelli
> ; Quentin Schulz ;
> Wenyou Yang - A41535 ; Nicolas Ferre
> ; linux-...@vger.kernel.org; net...@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] can: m_can: enable transmission of FD frame on latest
> version
> 
> @Wenyou Yang: Can you please test the two patches posted here:

Tested on SAMA5D2 SoC, It works.

> 
> [PATCH 1/2] can: m_can: handle bitrate setup on IP core >= 3.1.x
> http://marc.info/?l=linux-can=148883529927720=2
> 
> [PATCH 2/2] can: m_can: handle frame transmission on IP core >= 3.1.x
> http://marc.info/?l=linux-can=148883529927718=2
> 
> Tnx & regards,
> Oliver

Thank you.

Best Regards,
Wenyou Yang

RE: [PATCH] can: m_can: support transmit frame in CAN FD format

[Wenyou.Yang]

Hi Oliver,

Thank you for your review.

> -Original Message-
> From: Oliver Hartkopp [mailto:socket...@hartkopp.net]
> Sent: 2017年3月6日 3:34
> To: Wenyou Yang - A41535 ; Wolfgang
> Grandegger ; Marc Kleine-Budde 
> Cc: Alexandre Belloni ; Florian Fainelli
> ; Quentin Schulz ;
> Wenyou Yang - A41535 ; Nicolas Ferre
> ; linux-...@vger.kernel.org; net...@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] can: m_can: support transmit frame in CAN FD format
> 
> The patch has some issues:
> 
> On 03/03/2017 06:33 AM, Wenyou Yang wrote:
> > Add support to transmit the frame in the CAN FD format and with the
> > bit rate switching.
> 
> This is a misleading comment.
> 
> "can: m_can: support transmit frame in CAN FD format"
> is misleading too. You were able to send CAN FD frames before.
> 
> This patch enables the transmission of CAN FD frames on M_CAN IP cores  >=
> v3.1.x, right?

The revision of M_CAN IP on SAMA5D2 is 3.1.0 (CREL = 0x31040730).

> 
> So this should be mentioned properly.
> 
> > Tested on SAMA5D2 Xplained board.
> 
> Tested on which M_CAN IP core revision would be useful here.
> 
> > Signed-off-by: Wenyou Yang 
> > ---
> > The testing is based on
> > [RESEND PATCH 1/1] can: m_can: fix bitrate setup on latest silicon
> > http://lkml.iu.edu/hypermail/linux/kernel/1702.1/05347.html
> >
> >  drivers/net/can/m_can/m_can.c | 21 +
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/can/m_can/m_can.c
> > b/drivers/net/can/m_can/m_can.c index 195f15edb32e..9ef9b337d25b
> > 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -266,8 +266,12 @@ enum m_can_mram_cfg {
> >
> >  /* Tx Buffer Element */
> >  /* R0 */
> > +#define TX_BUF_ESI BIT(31)
> >  #define TX_BUF_XTD BIT(30)
> >  #define TX_BUF_RTR BIT(29)
> > +#define TX_BUF_EFC BIT(23)
> > +#define TX_BUF_EDL BIT(21)
> 
> This bit is named FDF (FD Format bit).
> 
> It has to be:
> 
> #define TX_BUF_FDFBIT(21)
> 
> > +#define TX_BUF_BRS BIT(20)
> >
> >  /* address offset and element number for each FIFO/Buffer in the
> > Message RAM */  struct mram_cfg { @@ -884,7 +888,7 @@ static void
> > m_can_chip_config(struct net_device *dev)
> > }
> >
> > if (priv->can.ctrlmode & CAN_CTRLMODE_FD)
> > -   cccr |= CCCR_CME_CANFD_BRS << CCCR_CME_SHIFT;
> > +   cccr |= (CCCR_CME_CANFD_BRS | CCCR_CME_CANFD) <<
> CCCR_CME_SHIFT;
> >
> > m_can_write(priv, M_CAN_CCCR, cccr);
> > m_can_write(priv, M_CAN_TEST, test); @@ -1047,6 +1051,7 @@ static
> > netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
> > struct canfd_frame *cf = (struct canfd_frame *)skb->data;
> > u32 id, cccr;
> > int i;
> > +   u32 dlc;
> >
> > if (can_dropped_invalid_skb(dev, skb))
> > return NETDEV_TX_OK;
> > @@ -1065,7 +1070,6 @@ static netdev_tx_t m_can_start_xmit(struct
> > sk_buff *skb,
> >
> > /* message ram configuration */
> > m_can_fifo_write(priv, 0, M_CAN_FIFO_ID, id);
> > -   m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, can_len2dlc(cf->len) << 16);
> >
> > for (i = 0; i < cf->len; i += 4)
> > m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4), @@ -
> 1073,20
> > +1077,29 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
> >
> > can_put_echo_skb(skb, dev, 0);
> >
> > +   dlc = can_len2dlc(cf->len) << 16;
> > +
> > if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> > cccr = m_can_read(priv, M_CAN_CCCR);
> > cccr &= ~(CCCR_CMR_MASK << CCCR_CMR_SHIFT);
> > if (can_is_canfd_skb(skb)) {
> > -   if (cf->flags & CANFD_BRS)
> > +   dlc |= TX_BUF_EDL;
> 
> dito FDF
> 
> > +   if (cf->flags & CANFD_ESI)
> > +   dlc |= TX_BUF_ESI;
> > +   if (cf->flags & CANFD_BRS) {
> > +   dlc |= TX_BUF_BRS;
> > cccr |= CCCR_CMR_CANFD_BRS <<
> CCCR_CMR_SHIFT;
> > -   else
> > +   } else {
> > cccr |= CCCR_CMR_CANFD <<
> CCCR_CMR_SHIFT;
> > +   }
> > } else {
> > cccr |= CCCR_CMR_CAN << CCCR_CMR_SHIFT;
> > }
> > m_can_write(priv, M_CAN_CCCR, cccr);
> > }
> >
> > +   m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, dlc);
> > +
> > /* enable first TX buffer to start transfer  */
> > m_can_write(priv, M_CAN_TXBTIE, 0x1);
> > m_can_write(priv, M_CAN_TXBAR, 0x1);
> >
> 
> Beside the documentation and the wrong bit naming the patch looks ok.
> Please resend.

I will send a version 2. Thanks

> 
> Regards,
> 

RE: [PATCH] can: m_can: support transmit frame in CAN FD format

[Wenyou.Yang]

Hi Oliver,

Thank you for your review.

> -Original Message-
> From: Oliver Hartkopp [mailto:socket...@hartkopp.net]
> Sent: 2017年3月6日 3:34
> To: Wenyou Yang - A41535 ; Wolfgang
> Grandegger ; Marc Kleine-Budde 
> Cc: Alexandre Belloni ; Florian Fainelli
> ; Quentin Schulz ;
> Wenyou Yang - A41535 ; Nicolas Ferre
> ; linux-...@vger.kernel.org; net...@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] can: m_can: support transmit frame in CAN FD format
> 
> The patch has some issues:
> 
> On 03/03/2017 06:33 AM, Wenyou Yang wrote:
> > Add support to transmit the frame in the CAN FD format and with the
> > bit rate switching.
> 
> This is a misleading comment.
> 
> "can: m_can: support transmit frame in CAN FD format"
> is misleading too. You were able to send CAN FD frames before.
> 
> This patch enables the transmission of CAN FD frames on M_CAN IP cores  >=
> v3.1.x, right?

The revision of M_CAN IP on SAMA5D2 is 3.1.0 (CREL = 0x31040730).

> 
> So this should be mentioned properly.
> 
> > Tested on SAMA5D2 Xplained board.
> 
> Tested on which M_CAN IP core revision would be useful here.
> 
> > Signed-off-by: Wenyou Yang 
> > ---
> > The testing is based on
> > [RESEND PATCH 1/1] can: m_can: fix bitrate setup on latest silicon
> > http://lkml.iu.edu/hypermail/linux/kernel/1702.1/05347.html
> >
> >  drivers/net/can/m_can/m_can.c | 21 +
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/can/m_can/m_can.c
> > b/drivers/net/can/m_can/m_can.c index 195f15edb32e..9ef9b337d25b
> > 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -266,8 +266,12 @@ enum m_can_mram_cfg {
> >
> >  /* Tx Buffer Element */
> >  /* R0 */
> > +#define TX_BUF_ESI BIT(31)
> >  #define TX_BUF_XTD BIT(30)
> >  #define TX_BUF_RTR BIT(29)
> > +#define TX_BUF_EFC BIT(23)
> > +#define TX_BUF_EDL BIT(21)
> 
> This bit is named FDF (FD Format bit).
> 
> It has to be:
> 
> #define TX_BUF_FDFBIT(21)
> 
> > +#define TX_BUF_BRS BIT(20)
> >
> >  /* address offset and element number for each FIFO/Buffer in the
> > Message RAM */  struct mram_cfg { @@ -884,7 +888,7 @@ static void
> > m_can_chip_config(struct net_device *dev)
> > }
> >
> > if (priv->can.ctrlmode & CAN_CTRLMODE_FD)
> > -   cccr |= CCCR_CME_CANFD_BRS << CCCR_CME_SHIFT;
> > +   cccr |= (CCCR_CME_CANFD_BRS | CCCR_CME_CANFD) <<
> CCCR_CME_SHIFT;
> >
> > m_can_write(priv, M_CAN_CCCR, cccr);
> > m_can_write(priv, M_CAN_TEST, test); @@ -1047,6 +1051,7 @@ static
> > netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
> > struct canfd_frame *cf = (struct canfd_frame *)skb->data;
> > u32 id, cccr;
> > int i;
> > +   u32 dlc;
> >
> > if (can_dropped_invalid_skb(dev, skb))
> > return NETDEV_TX_OK;
> > @@ -1065,7 +1070,6 @@ static netdev_tx_t m_can_start_xmit(struct
> > sk_buff *skb,
> >
> > /* message ram configuration */
> > m_can_fifo_write(priv, 0, M_CAN_FIFO_ID, id);
> > -   m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, can_len2dlc(cf->len) << 16);
> >
> > for (i = 0; i < cf->len; i += 4)
> > m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4), @@ -
> 1073,20
> > +1077,29 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
> >
> > can_put_echo_skb(skb, dev, 0);
> >
> > +   dlc = can_len2dlc(cf->len) << 16;
> > +
> > if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> > cccr = m_can_read(priv, M_CAN_CCCR);
> > cccr &= ~(CCCR_CMR_MASK << CCCR_CMR_SHIFT);
> > if (can_is_canfd_skb(skb)) {
> > -   if (cf->flags & CANFD_BRS)
> > +   dlc |= TX_BUF_EDL;
> 
> dito FDF
> 
> > +   if (cf->flags & CANFD_ESI)
> > +   dlc |= TX_BUF_ESI;
> > +   if (cf->flags & CANFD_BRS) {
> > +   dlc |= TX_BUF_BRS;
> > cccr |= CCCR_CMR_CANFD_BRS <<
> CCCR_CMR_SHIFT;
> > -   else
> > +   } else {
> > cccr |= CCCR_CMR_CANFD <<
> CCCR_CMR_SHIFT;
> > +   }
> > } else {
> > cccr |= CCCR_CMR_CAN << CCCR_CMR_SHIFT;
> > }
> > m_can_write(priv, M_CAN_CCCR, cccr);
> > }
> >
> > +   m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, dlc);
> > +
> > /* enable first TX buffer to start transfer  */
> > m_can_write(priv, M_CAN_TXBTIE, 0x1);
> > m_can_write(priv, M_CAN_TXBAR, 0x1);
> >
> 
> Beside the documentation and the wrong bit naming the patch looks ok.
> Please resend.

I will send a version 2. Thanks

> 
> Regards,
> Oliver


Best Regards,
Wenyou Yang

RE: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle

[Wenyou.Yang]

> -Original Message-
> From: Russell King - ARM Linux [mailto:li...@armlinux.org.uk]
> Sent: 2017年1月11日 19:18
> To: Jean-Jacques Hiblot 
> Cc: Wenyou Yang - A41535 ; Alexandre Belloni
> ; Mark Rutland ;
> devicetree ; Nicolas Ferre
> ; Linux Kernel Mailing List  ker...@vger.kernel.org>; robh+dt ; linux-arm-
> ker...@lists.infradead.org
> Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu 
> idle
> 
> On Wed, Jan 11, 2017 at 12:05:05PM +0100, Jean-Jacques Hiblot wrote:
> > 2017-01-11 9:15 GMT+01:00  :
> > > Hi Jean-Jacques,
> > >
> > >> -Original Message-
> > >> From: Jean-Jacques Hiblot [mailto:jjhib...@gmail.com]
> > >> Sent: 2017年1月11日 0:51
> > >> To: Alexandre Belloni 
> > >> Cc: Wenyou Yang - A41535 ; Mark Rutland
> > >> ; devicetree ;
> > >> Russell King ; Wenyou Yang - A41535
> > >> ; Nicolas Ferre
> > >> ; Linux Kernel Mailing List
> > >> ; Rob Herring ;
> > >> linux-arm-ker...@lists.infradead.org
> > >> Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before
> > >> entering cpu idle
> > >>
> > >> 2017-01-10 17:18 GMT+01:00 Alexandre Belloni
> > >> :
> > >> > I though a bit more about it, and I don't really like the new
> > >> > compatible string. I don't feel this should be necessary.
> > >> >
> > >> > What about the following:
> > >> >
> > >> > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> > >> > index
> > >> > b4332b727e9c..0333aca63e44 100644
> > >> > --- a/arch/arm/mach-at91/pm.c
> > >> > +++ b/arch/arm/mach-at91/pm.c
> > >> > @@ -53,6 +53,7 @@ extern void at91_pinctrl_gpio_resume(void);
> > >> > static struct {
> > >> > unsigned long uhp_udp_mask;
> > >> > int memctrl;
> > >> > +   bool has_l2_cache;
> > >> >  } at91_pm_data;
> > >> >
> > >> >  void __iomem *at91_ramc_base[2]; @@ -267,6 +268,11 @@ static
> > >> > void at91_ddr_standby(void)
> > >> > u32 lpr0, lpr1 = 0;
> > >> > u32 saved_lpr0, saved_lpr1 = 0;
> > >> >
> > >>
> > >> > +   if (at91_pm_data.has_l2_cache) {
> > >> > +   flush_cache_all();
> > >> what is the point of calling flush_cache_all() here ? Do we really
> > >> care that dirty data in L1 is written to DDR ? I may be missing
> > >> something but to me it's just extra latency.
> > >
> > > Are you mean use outer_flush_all() to flush all cache lines in the outer 
> > > cache
> only?
> >
> > Yes that's what I meant. You see, you don't flush the cache for
> > sama5d3 so it shouldn't be required either for sam5d4. You should be
> > able to test it quickly and see if L1 flush is indeed required by
> > replacing  flush_cache_all() with outer_flush_all(). BTW is highly
> > probable that L2 cache flush is done in outer_disable() so calling
> > outer_flush_all() is probably no required.
> 
> Please don't.  Read the comments in the code, and understand the APIs that
> you're suggesting people use _before_ making the suggestion:
> 
> /**
>  * outer_flush_all - clean and invalidate all cache lines in the outer cache
>  *
>  * Note: depending on implementation, this may not be atomic - it must
>  * only be called with interrupts disabled and no other active outer
>  * cache masters.
>  *
>  * It is intended that this function is only used by implementations
>  * needing to override the outer_cache.disable() method due to security.
>  * (Some implementations perform this as a clean followed by an invalidate.)  
> */
> 
> So, outer_flush_all() should not be called except from L2 cache code
> implementing the outer_disable() function - it's not intended for platforms 
> to use.
> 
> There are, however, sadly three users of outer_flush_all() which have crept in
> through arm-soc, that should be outer_disable() instead.

Here, outer_flush_all() should not be called, calling outer_disable() is 
enough. Is it right?

In the implementation of l2c_disable(void) of in mm/cache-l2x0.c, the 
outer_cache.flush_all() is called.

> 
> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.


Best Regards,
Wenyou Yang

RE: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle

[Wenyou.Yang]

> -Original Message-
> From: Russell King - ARM Linux [mailto:li...@armlinux.org.uk]
> Sent: 2017年1月11日 19:18
> To: Jean-Jacques Hiblot 
> Cc: Wenyou Yang - A41535 ; Alexandre Belloni
> ; Mark Rutland ;
> devicetree ; Nicolas Ferre
> ; Linux Kernel Mailing List  ker...@vger.kernel.org>; robh+dt ; linux-arm-
> ker...@lists.infradead.org
> Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu 
> idle
> 
> On Wed, Jan 11, 2017 at 12:05:05PM +0100, Jean-Jacques Hiblot wrote:
> > 2017-01-11 9:15 GMT+01:00  :
> > > Hi Jean-Jacques,
> > >
> > >> -Original Message-
> > >> From: Jean-Jacques Hiblot [mailto:jjhib...@gmail.com]
> > >> Sent: 2017年1月11日 0:51
> > >> To: Alexandre Belloni 
> > >> Cc: Wenyou Yang - A41535 ; Mark Rutland
> > >> ; devicetree ;
> > >> Russell King ; Wenyou Yang - A41535
> > >> ; Nicolas Ferre
> > >> ; Linux Kernel Mailing List
> > >> ; Rob Herring ;
> > >> linux-arm-ker...@lists.infradead.org
> > >> Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before
> > >> entering cpu idle
> > >>
> > >> 2017-01-10 17:18 GMT+01:00 Alexandre Belloni
> > >> :
> > >> > I though a bit more about it, and I don't really like the new
> > >> > compatible string. I don't feel this should be necessary.
> > >> >
> > >> > What about the following:
> > >> >
> > >> > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> > >> > index
> > >> > b4332b727e9c..0333aca63e44 100644
> > >> > --- a/arch/arm/mach-at91/pm.c
> > >> > +++ b/arch/arm/mach-at91/pm.c
> > >> > @@ -53,6 +53,7 @@ extern void at91_pinctrl_gpio_resume(void);
> > >> > static struct {
> > >> > unsigned long uhp_udp_mask;
> > >> > int memctrl;
> > >> > +   bool has_l2_cache;
> > >> >  } at91_pm_data;
> > >> >
> > >> >  void __iomem *at91_ramc_base[2]; @@ -267,6 +268,11 @@ static
> > >> > void at91_ddr_standby(void)
> > >> > u32 lpr0, lpr1 = 0;
> > >> > u32 saved_lpr0, saved_lpr1 = 0;
> > >> >
> > >>
> > >> > +   if (at91_pm_data.has_l2_cache) {
> > >> > +   flush_cache_all();
> > >> what is the point of calling flush_cache_all() here ? Do we really
> > >> care that dirty data in L1 is written to DDR ? I may be missing
> > >> something but to me it's just extra latency.
> > >
> > > Are you mean use outer_flush_all() to flush all cache lines in the outer 
> > > cache
> only?
> >
> > Yes that's what I meant. You see, you don't flush the cache for
> > sama5d3 so it shouldn't be required either for sam5d4. You should be
> > able to test it quickly and see if L1 flush is indeed required by
> > replacing  flush_cache_all() with outer_flush_all(). BTW is highly
> > probable that L2 cache flush is done in outer_disable() so calling
> > outer_flush_all() is probably no required.
> 
> Please don't.  Read the comments in the code, and understand the APIs that
> you're suggesting people use _before_ making the suggestion:
> 
> /**
>  * outer_flush_all - clean and invalidate all cache lines in the outer cache
>  *
>  * Note: depending on implementation, this may not be atomic - it must
>  * only be called with interrupts disabled and no other active outer
>  * cache masters.
>  *
>  * It is intended that this function is only used by implementations
>  * needing to override the outer_cache.disable() method due to security.
>  * (Some implementations perform this as a clean followed by an invalidate.)  
> */
> 
> So, outer_flush_all() should not be called except from L2 cache code
> implementing the outer_disable() function - it's not intended for platforms 
> to use.
> 
> There are, however, sadly three users of outer_flush_all() which have crept in
> through arm-soc, that should be outer_disable() instead.

Here, outer_flush_all() should not be called, calling outer_disable() is 
enough. Is it right?

In the implementation of l2c_disable(void) of in mm/cache-l2x0.c, the 
outer_cache.flush_all() is called.

> 
> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.


Best Regards,
Wenyou Yang

RE: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle

[Wenyou.Yang]

Hi Jean-Jacques,

> -Original Message-
> From: Jean-Jacques Hiblot [mailto:jjhib...@gmail.com]
> Sent: 2017年1月11日 0:51
> To: Alexandre Belloni 
> Cc: Wenyou Yang - A41535 ; Mark Rutland
> ; devicetree ; Russell
> King ; Wenyou Yang - A41535
> ; Nicolas Ferre ;
> Linux Kernel Mailing List ; Rob Herring
> ; linux-arm-ker...@lists.infradead.org
> Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu 
> idle
> 
> 2017-01-10 17:18 GMT+01:00 Alexandre Belloni
> :
> > I though a bit more about it, and I don't really like the new
> > compatible string. I don't feel this should be necessary.
> >
> > What about the following:
> >
> > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> > b4332b727e9c..0333aca63e44 100644
> > --- a/arch/arm/mach-at91/pm.c
> > +++ b/arch/arm/mach-at91/pm.c
> > @@ -53,6 +53,7 @@ extern void at91_pinctrl_gpio_resume(void);  static
> > struct {
> > unsigned long uhp_udp_mask;
> > int memctrl;
> > +   bool has_l2_cache;
> >  } at91_pm_data;
> >
> >  void __iomem *at91_ramc_base[2];
> > @@ -267,6 +268,11 @@ static void at91_ddr_standby(void)
> > u32 lpr0, lpr1 = 0;
> > u32 saved_lpr0, saved_lpr1 = 0;
> >
> 
> > +   if (at91_pm_data.has_l2_cache) {
> > +   flush_cache_all();
> what is the point of calling flush_cache_all() here ? Do we really care that 
> dirty
> data in L1 is written to DDR ? I may be missing something but to me it's just 
> extra
> latency.

Are you mean use outer_flush_all() to flush all cache lines in the outer cache 
only?

> > +   outer_disable();
> It seems to me that if there's no L2 cache, then outer_disable()  is a no-op. 
> It
> could be called unconditionally.
> > +   }
> > +
> > if (at91_ramc_base[1]) {
> > saved_lpr1 = at91_ramc_read(1, AT91_DDRSDRC_LPR);
> > lpr1 = saved_lpr1 & ~AT91_DDRSDRC_LPCB; @@ -287,6
> > +293,9 @@ static void at91_ddr_standby(void)
> > at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr0);
> > if (at91_ramc_base[1])
> > at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
> > +
> > +   if (at91_pm_data.has_l2_cache)
> > +   outer_resume();
> 
> same remark as for outer_disable()
> 
> Jean-Jacques
> 
> >  }
> >
> >  /* We manage both DDRAM/SDRAM controllers, we need more than one
> > value
> >  * to
> > @@ -353,6 +362,11 @@ static __init void at91_dt_ramc(void)
> > return;
> > }
> >
> > +   np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
> > +   if (np)
> > +   at91_pm_data.has_l2_cache = true;
> > +   of_node_put(np);
> > +
> > at91_pm_set_standby(standby);
> >  }
> >
> >
> > This has the following benefits:
> >  - everybody will have the fix, regardless of whether the dtb is
> > updated
> >  - has_l2_cache can be used later in at91_pm_suspend instead of calling
> >it unconditionnaly (I'll send a patch)
> >
> >
> > On 06/01/2017 at 14:59:45 +0800, Wenyou Yang wrote :
> >> For the SoCs such as SAMA5D2 and SAMA5D4 which have L2 cache, flush
> >> the L2 cache first before entering the cpu idle.
> >>
> >> Signed-off-by: Wenyou Yang 
> >> ---
> >>
> >>  arch/arm/mach-at91/pm.c   | 19 +++
> >>  drivers/memory/atmel-sdramc.c |  1 +
> >>  2 files changed, 20 insertions(+)
> >>
> >> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> >> b4332b727e9c..1a60dede1a01 100644
> >> --- a/arch/arm/mach-at91/pm.c
> >> +++ b/arch/arm/mach-at91/pm.c
> >> @@ -289,6 +289,24 @@ static void at91_ddr_standby(void)
> >>   at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);  }
> >>
> >> +static void at91_ddr_cache_standby(void) {
> >> + u32 saved_lpr;
> >> +
> >> + flush_cache_all();
> >> + outer_disable();
> >> +
> >> + saved_lpr = at91_ramc_read(0, AT91_DDRSDRC_LPR);
> >> + at91_ramc_write(0, AT91_DDRSDRC_LPR, (saved_lpr &
> >> + (~AT91_DDRSDRC_LPCB)) |
> >> + AT91_DDRSDRC_LPCB_SELF_REFRESH);
> >> +
> >> + cpu_do_idle();
> >> +
> >> + at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr);
> >> +
> >> + outer_resume();
> >> +}
> >> +
> >>  /* We manage both DDRAM/SDRAM controllers, we need more than one
> value to
> >>   * remember.
> >>   */
> >> @@ -324,6 +342,7 @@ static const struct of_device_id const ramc_ids[]
> __initconst = {
> >>   { .compatible = "atmel,at91sam9260-sdramc", .data =
> at91sam9_sdram_standby },
> >>   { .compatible = "atmel,at91sam9g45-ddramc", .data = at91_ddr_standby 
> >> },
> >>   { .compatible = "atmel,sama5d3-ddramc", .data =
> >> at91_ddr_standby },
> >> + { 

RE: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle

[Wenyou.Yang]

Hi Jean-Jacques,

> -Original Message-
> From: Jean-Jacques Hiblot [mailto:jjhib...@gmail.com]
> Sent: 2017年1月11日 0:51
> To: Alexandre Belloni 
> Cc: Wenyou Yang - A41535 ; Mark Rutland
> ; devicetree ; Russell
> King ; Wenyou Yang - A41535
> ; Nicolas Ferre ;
> Linux Kernel Mailing List ; Rob Herring
> ; linux-arm-ker...@lists.infradead.org
> Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu 
> idle
> 
> 2017-01-10 17:18 GMT+01:00 Alexandre Belloni
> :
> > I though a bit more about it, and I don't really like the new
> > compatible string. I don't feel this should be necessary.
> >
> > What about the following:
> >
> > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> > b4332b727e9c..0333aca63e44 100644
> > --- a/arch/arm/mach-at91/pm.c
> > +++ b/arch/arm/mach-at91/pm.c
> > @@ -53,6 +53,7 @@ extern void at91_pinctrl_gpio_resume(void);  static
> > struct {
> > unsigned long uhp_udp_mask;
> > int memctrl;
> > +   bool has_l2_cache;
> >  } at91_pm_data;
> >
> >  void __iomem *at91_ramc_base[2];
> > @@ -267,6 +268,11 @@ static void at91_ddr_standby(void)
> > u32 lpr0, lpr1 = 0;
> > u32 saved_lpr0, saved_lpr1 = 0;
> >
> 
> > +   if (at91_pm_data.has_l2_cache) {
> > +   flush_cache_all();
> what is the point of calling flush_cache_all() here ? Do we really care that 
> dirty
> data in L1 is written to DDR ? I may be missing something but to me it's just 
> extra
> latency.

Are you mean use outer_flush_all() to flush all cache lines in the outer cache 
only?

> > +   outer_disable();
> It seems to me that if there's no L2 cache, then outer_disable()  is a no-op. 
> It
> could be called unconditionally.
> > +   }
> > +
> > if (at91_ramc_base[1]) {
> > saved_lpr1 = at91_ramc_read(1, AT91_DDRSDRC_LPR);
> > lpr1 = saved_lpr1 & ~AT91_DDRSDRC_LPCB; @@ -287,6
> > +293,9 @@ static void at91_ddr_standby(void)
> > at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr0);
> > if (at91_ramc_base[1])
> > at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
> > +
> > +   if (at91_pm_data.has_l2_cache)
> > +   outer_resume();
> 
> same remark as for outer_disable()
> 
> Jean-Jacques
> 
> >  }
> >
> >  /* We manage both DDRAM/SDRAM controllers, we need more than one
> > value
> >  * to
> > @@ -353,6 +362,11 @@ static __init void at91_dt_ramc(void)
> > return;
> > }
> >
> > +   np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
> > +   if (np)
> > +   at91_pm_data.has_l2_cache = true;
> > +   of_node_put(np);
> > +
> > at91_pm_set_standby(standby);
> >  }
> >
> >
> > This has the following benefits:
> >  - everybody will have the fix, regardless of whether the dtb is
> > updated
> >  - has_l2_cache can be used later in at91_pm_suspend instead of calling
> >it unconditionnaly (I'll send a patch)
> >
> >
> > On 06/01/2017 at 14:59:45 +0800, Wenyou Yang wrote :
> >> For the SoCs such as SAMA5D2 and SAMA5D4 which have L2 cache, flush
> >> the L2 cache first before entering the cpu idle.
> >>
> >> Signed-off-by: Wenyou Yang 
> >> ---
> >>
> >>  arch/arm/mach-at91/pm.c   | 19 +++
> >>  drivers/memory/atmel-sdramc.c |  1 +
> >>  2 files changed, 20 insertions(+)
> >>
> >> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> >> b4332b727e9c..1a60dede1a01 100644
> >> --- a/arch/arm/mach-at91/pm.c
> >> +++ b/arch/arm/mach-at91/pm.c
> >> @@ -289,6 +289,24 @@ static void at91_ddr_standby(void)
> >>   at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);  }
> >>
> >> +static void at91_ddr_cache_standby(void) {
> >> + u32 saved_lpr;
> >> +
> >> + flush_cache_all();
> >> + outer_disable();
> >> +
> >> + saved_lpr = at91_ramc_read(0, AT91_DDRSDRC_LPR);
> >> + at91_ramc_write(0, AT91_DDRSDRC_LPR, (saved_lpr &
> >> + (~AT91_DDRSDRC_LPCB)) |
> >> + AT91_DDRSDRC_LPCB_SELF_REFRESH);
> >> +
> >> + cpu_do_idle();
> >> +
> >> + at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr);
> >> +
> >> + outer_resume();
> >> +}
> >> +
> >>  /* We manage both DDRAM/SDRAM controllers, we need more than one
> value to
> >>   * remember.
> >>   */
> >> @@ -324,6 +342,7 @@ static const struct of_device_id const ramc_ids[]
> __initconst = {
> >>   { .compatible = "atmel,at91sam9260-sdramc", .data =
> at91sam9_sdram_standby },
> >>   { .compatible = "atmel,at91sam9g45-ddramc", .data = at91_ddr_standby 
> >> },
> >>   { .compatible = "atmel,sama5d3-ddramc", .data =
> >> at91_ddr_standby },
> >> + { .compatible = "atmel,sama5d4-ddramc", .data =
> >> + at91_ddr_cache_standby },
> >>   { /*sentinel*/ }
> >>  };
> >>
> >> diff --git a/drivers/memory/atmel-sdramc.c
> >> b/drivers/memory/atmel-sdramc.c index b418b39af180..7e5c5c6c1348
> >> 100644
> >> --- a/drivers/memory/atmel-sdramc.c
> >> +++ 

RE: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle

[Wenyou.Yang]

Hi Alexandre,

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年1月6日 17:05
> To: Wenyou Yang - A41535 
> Cc: Russell King ; Nicolas Ferre
> ; Rob Herring ; Mark Rutland
> ; linux-kernel@vger.kernel.org; Wenyou Yang - A41535
> ; devicet...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org
> Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu 
> idle
> 
> Hi,
> 
> On 06/01/2017 at 14:59:45 +0800, Wenyou Yang wrote :
> > For the SoCs such as SAMA5D2 and SAMA5D4 which have L2 cache, flush
> > the L2 cache first before entering the cpu idle.
> >
> > Signed-off-by: Wenyou Yang 
> > ---
> >
> >  arch/arm/mach-at91/pm.c   | 19 +++
> >  drivers/memory/atmel-sdramc.c |  1 +
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> > b4332b727e9c..1a60dede1a01 100644
> > --- a/arch/arm/mach-at91/pm.c
> > +++ b/arch/arm/mach-at91/pm.c
> > @@ -289,6 +289,24 @@ static void at91_ddr_standby(void)
> > at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);  }
> >
> > +static void at91_ddr_cache_standby(void) {
> > +   u32 saved_lpr;
> > +
> > +   flush_cache_all();
> > +   outer_disable();
> > +
> > +   saved_lpr = at91_ramc_read(0, AT91_DDRSDRC_LPR);
> > +   at91_ramc_write(0, AT91_DDRSDRC_LPR, (saved_lpr &
> > +   (~AT91_DDRSDRC_LPCB)) |
> AT91_DDRSDRC_LPCB_SELF_REFRESH);
> > +
> > +   cpu_do_idle();
> > +
> > +   at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr);
> > +
> > +   outer_resume();
> > +}
> > +
> 
> Seems good to me. Did you measure the added latency on sama5d3 if you add the
> cache operations in at91_ddr_standby instead of having a new function?

No, I didn't. How to measure it?


Best Regards,
Wenyou Yang

RE: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle

[Wenyou.Yang]

Hi Alexandre,

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2017年1月6日 17:05
> To: Wenyou Yang - A41535 
> Cc: Russell King ; Nicolas Ferre
> ; Rob Herring ; Mark Rutland
> ; linux-kernel@vger.kernel.org; Wenyou Yang - A41535
> ; devicet...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org
> Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu 
> idle
> 
> Hi,
> 
> On 06/01/2017 at 14:59:45 +0800, Wenyou Yang wrote :
> > For the SoCs such as SAMA5D2 and SAMA5D4 which have L2 cache, flush
> > the L2 cache first before entering the cpu idle.
> >
> > Signed-off-by: Wenyou Yang 
> > ---
> >
> >  arch/arm/mach-at91/pm.c   | 19 +++
> >  drivers/memory/atmel-sdramc.c |  1 +
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index
> > b4332b727e9c..1a60dede1a01 100644
> > --- a/arch/arm/mach-at91/pm.c
> > +++ b/arch/arm/mach-at91/pm.c
> > @@ -289,6 +289,24 @@ static void at91_ddr_standby(void)
> > at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);  }
> >
> > +static void at91_ddr_cache_standby(void) {
> > +   u32 saved_lpr;
> > +
> > +   flush_cache_all();
> > +   outer_disable();
> > +
> > +   saved_lpr = at91_ramc_read(0, AT91_DDRSDRC_LPR);
> > +   at91_ramc_write(0, AT91_DDRSDRC_LPR, (saved_lpr &
> > +   (~AT91_DDRSDRC_LPCB)) |
> AT91_DDRSDRC_LPCB_SELF_REFRESH);
> > +
> > +   cpu_do_idle();
> > +
> > +   at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr);
> > +
> > +   outer_resume();
> > +}
> > +
> 
> Seems good to me. Did you measure the added latency on sama5d3 if you add the
> cache operations in at91_ddr_standby instead of having a new function?

No, I didn't. How to measure it?


Best Regards,
Wenyou Yang

RE: [PATCH v2 REGRESSION RESEND] usb: ohci-at91: use descriptor-based gpio APIs correctly

[Wenyou.Yang]

> -Original Message-
> From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
> Sent: 2016年12月24日 2:05
> To: Peter Rosin 
> Cc: linux-kernel@vger.kernel.org; Alan Stern ;
> linux-...@vger.kernel.org; Wenyou Yang - A41535
> 
> Subject: Re: [PATCH v2 REGRESSION RESEND] usb: ohci-at91: use descriptor-
> based gpio APIs correctly
> 
> On Thu, Dec 22, 2016 at 09:38:08PM +0100, Peter Rosin wrote:
> > On 2016-12-22 18:27, Greg Kroah-Hartman wrote:
> > > On Thu, Dec 22, 2016 at 08:43:55AM +0100, Peter Rosin wrote:
> > >> The gpiod_get* function family does not want the -gpio suffix.
> > >> Use devm_gpiod_get_index_optional instead of devm_gpiod_get_optional.
> > >> The descriptor based APIs handle active high/low automatically.
> > >> The vbus-gpios are output, request enable while getting the gpio.
> > >> Don't try to get any vbus-gpios for ports outside num-ports.
> > >>
> > >> WTF? Big sigh.
> > >>
> > >> Fixes: 054d4b7b577d ("usb: ohci-at91: Use descriptor-based gpio
> > >> APIs")
> > >> Signed-off-by: Peter Rosin 
> > >> ---
> > >>
> > >> Hi!
> > >>
> > >> Resending this, since the only response I've got is that the merge
> > >> window is open and that this patch has been put on hold due to that.
> > >> But I think this regression (which happend between v4.9 and current
> > >> master) should be fixed before the merge window closes.
> > >
> > > I don't merge patches before -rc1 comes out, sorry, people should
> > > have tested linux-next better :)
> >
> > Neat, shift the blame for the shit patch over to the messenger :)
> 
> Not at all, I blame the original developer :)

I am very very sorry. It is my ignorance. Sorry.

I tested this patch on linux-next branch on the SAMA5D3 and SAMA5D4 Xplained 
board.

> 
> > > I'll catch up the first week of January, relax.
> >
> > As we all know, unrelated regressions are painful when trying to
> > locate other problems. It's seems silly to have a few extra for no good 
> > reason.
> 
> I am supposed to be on vacation and not reading email until the 3rd of 
> January,
> relax, we will catch up on stuff like this, and other minor things, soon 
> enough, in
> plenty of time for 4.10-final.
> 
> thanks,
> 
> greg k-h


Best Regards,
Wenyou Yang

RE: [PATCH v2 REGRESSION RESEND] usb: ohci-at91: use descriptor-based gpio APIs correctly

[Wenyou.Yang]

> -Original Message-
> From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
> Sent: 2016年12月24日 2:05
> To: Peter Rosin 
> Cc: linux-kernel@vger.kernel.org; Alan Stern ;
> linux-...@vger.kernel.org; Wenyou Yang - A41535
> 
> Subject: Re: [PATCH v2 REGRESSION RESEND] usb: ohci-at91: use descriptor-
> based gpio APIs correctly
> 
> On Thu, Dec 22, 2016 at 09:38:08PM +0100, Peter Rosin wrote:
> > On 2016-12-22 18:27, Greg Kroah-Hartman wrote:
> > > On Thu, Dec 22, 2016 at 08:43:55AM +0100, Peter Rosin wrote:
> > >> The gpiod_get* function family does not want the -gpio suffix.
> > >> Use devm_gpiod_get_index_optional instead of devm_gpiod_get_optional.
> > >> The descriptor based APIs handle active high/low automatically.
> > >> The vbus-gpios are output, request enable while getting the gpio.
> > >> Don't try to get any vbus-gpios for ports outside num-ports.
> > >>
> > >> WTF? Big sigh.
> > >>
> > >> Fixes: 054d4b7b577d ("usb: ohci-at91: Use descriptor-based gpio
> > >> APIs")
> > >> Signed-off-by: Peter Rosin 
> > >> ---
> > >>
> > >> Hi!
> > >>
> > >> Resending this, since the only response I've got is that the merge
> > >> window is open and that this patch has been put on hold due to that.
> > >> But I think this regression (which happend between v4.9 and current
> > >> master) should be fixed before the merge window closes.
> > >
> > > I don't merge patches before -rc1 comes out, sorry, people should
> > > have tested linux-next better :)
> >
> > Neat, shift the blame for the shit patch over to the messenger :)
> 
> Not at all, I blame the original developer :)

I am very very sorry. It is my ignorance. Sorry.

I tested this patch on linux-next branch on the SAMA5D3 and SAMA5D4 Xplained 
board.

> 
> > > I'll catch up the first week of January, relax.
> >
> > As we all know, unrelated regressions are painful when trying to
> > locate other problems. It's seems silly to have a few extra for no good 
> > reason.
> 
> I am supposed to be on vacation and not reading email until the 3rd of 
> January,
> relax, we will catch up on stuff like this, and other minor things, soon 
> enough, in
> plenty of time for 4.10-final.
> 
> thanks,
> 
> greg k-h


Best Regards,
Wenyou Yang

RE: [PATCH v3] ARM: at91/dt: add dts file for sama5d36ek CMP board

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2016年11月21日 22:03
> To: Wenyou Yang - A41535 
> Cc: Nicolas Ferre ; Russell King
> ; Rob Herring ; Pawel Moll
> ; Mark Rutland ; Ian Campbell
> ; Kumar Gala ; linux-
> ker...@vger.kernel.org; Wenyou Yang - A41535
> ; devicet...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org
> Subject: Re: [PATCH v3] ARM: at91/dt: add dts file for sama5d36ek CMP board
> 
> Hi,
> 
> I fixed it up this time but please use the proper subject prefix next time 
> (ARM: dts:
> at91:).

Thank you.

I keep it in mind.

> 
> On 21/11/2016 at 13:14:42 +0800, Wenyou Yang wrote :
> > The sama5d36ek CMP board is the variant of sama5d3xek board.
> > It is equipped with the low-power DDR2 SDRAM, PMIC ACT8865 and some
> > power rail. Its main purpose is used to measure the power consumption.
> > The difference of the sama5d36ek CMP dts from sama5d36ek dts is listed
> > as below.
> >  1. The USB host nodes are removed, that is, the USB host is disabled.
> >  2. The gpio_keys node is added to wake up from the sleep.
> >  3. The LCD isn't supported due to the pins for LCD are conflicted
> > with gpio_keys.
> >  4. The adc0 node support the pinctrl sleep state to fix the over
> > consumption on VDDANA.
> >
> > As said in errata, "When the USB host ports are used in high speed
> > mode (EHCI), it is not possible to suspend the ports if no device is
> > attached on each port. This leads to increased power consumption even
> > if the system is in a low power mode." That is why the the USB host is
> > disabled.
> >
> > Signed-off-by: Wenyou Yang 
> > ---
> >
> > Changes in v3:
> >  - Use a dual license scheme for DT files.
> >  - Use the proper model name and the compatible string to reflect
> >the nature of this new "CMP" board.
> >  - Change name of wakeup property to "wakeup-source".
> >  - Remove unnecessary comments.
> >  - Remove bootargs.
> >
> > Changes in v2:
> >  - Add the pinctrl sleep state for adc0 node to fix the over
> >consumption on VDDANA.
> >  - Improve the commit log.
> >
> >  arch/arm/boot/dts/sama5d36ek_cmp.dts  |  87 ++
> > arch/arm/boot/dts/sama5d3xcm_cmp.dtsi | 201 +++
> > arch/arm/boot/dts/sama5d3xmb_cmp.dtsi | 301
> > ++
> >  3 files changed, 589 insertions(+)
> >  create mode 100644 arch/arm/boot/dts/sama5d36ek_cmp.dts
> >  create mode 100644 arch/arm/boot/dts/sama5d3xcm_cmp.dtsi
> >  create mode 100644 arch/arm/boot/dts/sama5d3xmb_cmp.dtsi
> >
> Applied, thanks.
> 
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Best Regards,
Wenyou Yang

RE: [PATCH v3] ARM: at91/dt: add dts file for sama5d36ek CMP board

[Wenyou.Yang]

> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2016年11月21日 22:03
> To: Wenyou Yang - A41535 
> Cc: Nicolas Ferre ; Russell King
> ; Rob Herring ; Pawel Moll
> ; Mark Rutland ; Ian Campbell
> ; Kumar Gala ; linux-
> ker...@vger.kernel.org; Wenyou Yang - A41535
> ; devicet...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org
> Subject: Re: [PATCH v3] ARM: at91/dt: add dts file for sama5d36ek CMP board
> 
> Hi,
> 
> I fixed it up this time but please use the proper subject prefix next time 
> (ARM: dts:
> at91:).

Thank you.

I keep it in mind.

> 
> On 21/11/2016 at 13:14:42 +0800, Wenyou Yang wrote :
> > The sama5d36ek CMP board is the variant of sama5d3xek board.
> > It is equipped with the low-power DDR2 SDRAM, PMIC ACT8865 and some
> > power rail. Its main purpose is used to measure the power consumption.
> > The difference of the sama5d36ek CMP dts from sama5d36ek dts is listed
> > as below.
> >  1. The USB host nodes are removed, that is, the USB host is disabled.
> >  2. The gpio_keys node is added to wake up from the sleep.
> >  3. The LCD isn't supported due to the pins for LCD are conflicted
> > with gpio_keys.
> >  4. The adc0 node support the pinctrl sleep state to fix the over
> > consumption on VDDANA.
> >
> > As said in errata, "When the USB host ports are used in high speed
> > mode (EHCI), it is not possible to suspend the ports if no device is
> > attached on each port. This leads to increased power consumption even
> > if the system is in a low power mode." That is why the the USB host is
> > disabled.
> >
> > Signed-off-by: Wenyou Yang 
> > ---
> >
> > Changes in v3:
> >  - Use a dual license scheme for DT files.
> >  - Use the proper model name and the compatible string to reflect
> >the nature of this new "CMP" board.
> >  - Change name of wakeup property to "wakeup-source".
> >  - Remove unnecessary comments.
> >  - Remove bootargs.
> >
> > Changes in v2:
> >  - Add the pinctrl sleep state for adc0 node to fix the over
> >consumption on VDDANA.
> >  - Improve the commit log.
> >
> >  arch/arm/boot/dts/sama5d36ek_cmp.dts  |  87 ++
> > arch/arm/boot/dts/sama5d3xcm_cmp.dtsi | 201 +++
> > arch/arm/boot/dts/sama5d3xmb_cmp.dtsi | 301
> > ++
> >  3 files changed, 589 insertions(+)
> >  create mode 100644 arch/arm/boot/dts/sama5d36ek_cmp.dts
> >  create mode 100644 arch/arm/boot/dts/sama5d3xcm_cmp.dtsi
> >  create mode 100644 arch/arm/boot/dts/sama5d3xmb_cmp.dtsi
> >
> Applied, thanks.
> 
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Best Regards,
Wenyou Yang

RE: [PATCH v2] ARM: at91/dt: add dts file for sama5d36ek CMP board

[Wenyou.Yang]

> -Original Message-
> From: kbuild test robot [mailto:l...@intel.com]
> Sent: 2016年11月2日 13:28
> To: Wenyou Yang - A41535 
> Cc: kbuild-...@01.org; Nicolas Ferre ; Alexandre
> Belloni ; Russell King
> ; Rob Herring ; Pawel Moll
> ; Mark Rutland ; Ian Campbell
> ; Kumar Gala ; linux-
> ker...@vger.kernel.org; Wenyou Yang - A41535
> ; devicet...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; Wenyou Yang - A41535
> 
> Subject: Re: [PATCH v2] ARM: at91/dt: add dts file for sama5d36ek CMP board
> 
> Hi Wenyou,
> 
> [auto build test ERROR on at91/at91-next] [also build test ERROR on v4.9-rc3
> next-20161028] [if your patch is applied to the wrong git tree, please drop 
> us a
> note to help improve the system]
> 
> url:https://github.com/0day-ci/linux/commits/Wenyou-Yang/ARM-at91-dt-add-
> dts-file-for-sama5d36ek-CMP-board/20161102-52
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/nferre/linux-at91.git 
> at91-
> next
> config: arm-at91_dt_defconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
> wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-
> tests.git/plain/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=arm
> 
> All errors (new ones prefixed by >>):
> 
> >> Error: arch/arm/boot/dts/sama5d3xmb_cmp.dtsi:143.17-18 syntax error
>FATAL ERROR: Unable to parse input tree

Oh, this patch is based on the patch I sent before.
[PATCH] pinctrl: at91: add support for OUTPUT config
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/464354.html

I forgot adding this information.

Sorry. 


Best Regards,
Wenyou Yang

RE: [PATCH v2] ARM: at91/dt: add dts file for sama5d36ek CMP board

[Wenyou.Yang]

> -Original Message-
> From: kbuild test robot [mailto:l...@intel.com]
> Sent: 2016年11月2日 13:28
> To: Wenyou Yang - A41535 
> Cc: kbuild-...@01.org; Nicolas Ferre ; Alexandre
> Belloni ; Russell King
> ; Rob Herring ; Pawel Moll
> ; Mark Rutland ; Ian Campbell
> ; Kumar Gala ; linux-
> ker...@vger.kernel.org; Wenyou Yang - A41535
> ; devicet...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; Wenyou Yang - A41535
> 
> Subject: Re: [PATCH v2] ARM: at91/dt: add dts file for sama5d36ek CMP board
> 
> Hi Wenyou,
> 
> [auto build test ERROR on at91/at91-next] [also build test ERROR on v4.9-rc3
> next-20161028] [if your patch is applied to the wrong git tree, please drop 
> us a
> note to help improve the system]
> 
> url:https://github.com/0day-ci/linux/commits/Wenyou-Yang/ARM-at91-dt-add-
> dts-file-for-sama5d36ek-CMP-board/20161102-52
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/nferre/linux-at91.git 
> at91-
> next
> config: arm-at91_dt_defconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
> wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-
> tests.git/plain/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=arm
> 
> All errors (new ones prefixed by >>):
> 
> >> Error: arch/arm/boot/dts/sama5d3xmb_cmp.dtsi:143.17-18 syntax error
>FATAL ERROR: Unable to parse input tree

Oh, this patch is based on the patch I sent before.
[PATCH] pinctrl: at91: add support for OUTPUT config
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/464354.html

I forgot adding this information.

Sorry. 


Best Regards,
Wenyou Yang

RE: [PATCH v9 3/4] doc: bindings: mfd: act8945a: Update the example

[Wenyou.Yang]

> -Original Message-
> From: Sebastian Reichel [mailto:s...@kernel.org]
> Sent: 2016年9月1日 20:48
> To: Lee Jones 
> Cc: Wenyou Yang - A41535 ; Dmitry Eremin-
> Solenikov ; David Woodhouse
> ; Rob Herring ; Pawel Moll
> ; Mark Rutland ; Ian Campbell
> ; Kumar Gala ; Nicolas
> Ferre ; Alexandre Belloni  electrons.com>; linux-kernel@vger.kernel.org; Wenyou Yang - A41535
> ; devicet...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux...@vger.kernel.org
> Subject: Re: [PATCH v9 3/4] doc: bindings: mfd: act8945a: Update the example
> 
> Hi Lee,
> 
> On Thu, Sep 01, 2016 at 12:22:51PM +0100, Lee Jones wrote:
> > On Thu, 01 Sep 2016, Wenyou Yang wrote:
> >
> > > Since the act8945a-charger is regarded as a sub-device and it using
> > > "interrupts" property, update the examples section.
> > >
> > > Signed-off-by: Wenyou Yang 
> > > ---
> > >
> > > Changes in v9: None
> > > Changes in v8: None
> > > Changes in v7: None
> > > Changes in v6: None
> > > Changes in v4: None
> > >
> > >  Documentation/devicetree/bindings/mfd/act8945a.txt | 22
> > > +++---
> > >  1 file changed, 15 insertions(+), 7 deletions(-)
> >
> > This is a functional change.  I'll require a DT Ack.
> 
> Wenyou Yang forgot to take over Rob's Acked-By from the previous
> version: https://patchwork.kernel.org/patch/9298871/

Yes, sorry. I forgot.


Best Regards,
Wenyou Yang

RE: [PATCH v9 3/4] doc: bindings: mfd: act8945a: Update the example

[Wenyou.Yang]

> -Original Message-
> From: Sebastian Reichel [mailto:s...@kernel.org]
> Sent: 2016年9月1日 20:48
> To: Lee Jones 
> Cc: Wenyou Yang - A41535 ; Dmitry Eremin-
> Solenikov ; David Woodhouse
> ; Rob Herring ; Pawel Moll
> ; Mark Rutland ; Ian Campbell
> ; Kumar Gala ; Nicolas
> Ferre ; Alexandre Belloni  electrons.com>; linux-kernel@vger.kernel.org; Wenyou Yang - A41535
> ; devicet...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux...@vger.kernel.org
> Subject: Re: [PATCH v9 3/4] doc: bindings: mfd: act8945a: Update the example
> 
> Hi Lee,
> 
> On Thu, Sep 01, 2016 at 12:22:51PM +0100, Lee Jones wrote:
> > On Thu, 01 Sep 2016, Wenyou Yang wrote:
> >
> > > Since the act8945a-charger is regarded as a sub-device and it using
> > > "interrupts" property, update the examples section.
> > >
> > > Signed-off-by: Wenyou Yang 
> > > ---
> > >
> > > Changes in v9: None
> > > Changes in v8: None
> > > Changes in v7: None
> > > Changes in v6: None
> > > Changes in v4: None
> > >
> > >  Documentation/devicetree/bindings/mfd/act8945a.txt | 22
> > > +++---
> > >  1 file changed, 15 insertions(+), 7 deletions(-)
> >
> > This is a functional change.  I'll require a DT Ack.
> 
> Wenyou Yang forgot to take over Rob's Acked-By from the previous
> version: https://patchwork.kernel.org/patch/9298871/

Yes, sorry. I forgot.


Best Regards,
Wenyou Yang

RE: [PATCH v5] usb: ohci-at91: Forcibly suspend ports while USB suspend

[Wenyou.Yang]

Hi Alan,

As you saw, I think the version 4 is better than this, can we take the version 
4?


Best Regards,
Wenyou Yang

> -Original Message-
> From: wenyou.y...@microchip.com [mailto:wenyou.y...@microchip.com]
> Sent: 2016年8月5日 11:46
> To: st...@rowland.harvard.edu; wenyou.y...@atmel.com
> Cc: gre...@linuxfoundation.org; nicolas.fe...@atmel.com;
> alexandre.bell...@free-electrons.com; linux-kernel@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux-...@vger.kernel.org
> Subject: RE: [PATCH v5] usb: ohci-at91: Forcibly suspend ports while USB
> suspend
> 
> Hi Alan,
> 
> > -Original Message-
> > From: Alan Stern [mailto:st...@rowland.harvard.edu]
> > Sent: 2016年8月4日 23:02
> > To: Wenyou Yang 
> > Cc: Greg Kroah-Hartman ; Nicolas Ferre
> > ; Alexandre Belloni  > electrons.com>; linux-kernel@vger.kernel.org; linux-arm-
> > ker...@lists.infradead.org; linux-...@vger.kernel.org
> > Subject: Re: [PATCH v5] usb: ohci-at91: Forcibly suspend ports while
> > USB suspend
> >
> > On Thu, 4 Aug 2016, Wenyou Yang wrote:
> >
> > > The usb controller does not managed correctly the suspend mode for
> > > the ehci. In echi mode, there is no way to have utmi_suspend_o_n[1]
> > > suspend without any device connected to it. This is why this
> > > specific control is added to fix this issue. The suspend mode works
> > > in ohci mode.
> >
> > Why are you talking about EHCI mode?  This patch is only for the
> > ohci-at91 driver.
> 
> Actually I described the issue according to the documents from our IP, and 
> this
> specific control is recommended to do in ohci mode.
> 
> >
> > > This specific control is by setting the SUSPEND_A/B/C fields of
> > > SFR_OHCIICR(OHCI Interrupt Configuration Register) in the SFR while
> > > OHCI USB suspend.
> > >
> > > This setting operation must be done before the USB clock disabled,
> > > clear them after the USB clock enabled.
> > >
> > > Signed-off-by: Wenyou Yang 
> > > Reviewed-by: Alexandre Belloni
> > > 
> > > Acked-by: Nicolas Ferre 
> >
> > I don't know if this is any better than before!  See the comments below.
> 
> Yes, I think so.
> 
> What else advice?
> 
> >
> > > ---
> > >
> > > Changes in v5:
> > >  - Use the USB_PORT_FEAT_SUSPEND subcase of the SetPortFeature case
> > >to take care it.
> > >  - Update the commit log.
> > >
> > > Changes in v4:
> > >  - To check whether the SFR node with "atmel,sama5d2-sfr" compatible
> > >is present or not to decide if this feature is applied or not
> > >when USB OHCI suspend/resume, instead of new compatible.
> > >  - Drop the compatible "atmel,sama5d2-ohci".
> > >  - Drop [PATCH 2/2] ARM: at91/dt: sama5d2: Use new compatible for
> > >ohci node.
> > >  - Drop include/soc/at91/at91_sfr.h, move the macro definitions to
> > >atmel-sfr.h which already exists.
> > >  - Change the defines to align the exists.
> > >
> > > Changes in v3:
> > >  - Change the compatible description for more precise.
> > >
> > > Changes in v2:
> > >  - Add compatible to support forcibly suspend the ports.
> > >  - Add soc/at91/at91_sfr.h to accommodate the defines.
> > >  - Add error checking for .sfr_regmap.
> > >  - Remove unnecessary regmap_read() statement.
> >
> >
> > > @@ -282,6 +301,28 @@ static int ohci_at91_hub_status_data(struct
> > > usb_hcd
> > *hcd, char *buf)
> > >   return length;
> > >  }
> > >
> > > +static int ohci_at91_port_ctrl(struct regmap *regmap, u16 port, u8
> > > +set) {
> > > + u32 regval;
> > > + int ret;
> > > +
> > > + if (!regmap)
> > > + return 0;
> > > +
> > > + ret = regmap_read(regmap, AT91_SFR_OHCIICR, );
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (set)
> > > + regval |= AT91_OHCIICR_SUSPEND(port);
> > > + else
> > > + regval &= ~AT91_OHCIICR_SUSPEND(port);
> >
> > In the earlier versions of this patch, you did not use the port number.
> > Why has this changed?
> 
> This function is called by ohci_at91_hub_control(), which is invoked on basis 
> of
> port number.
> 
> So, I think it is more reasonable of adding the port argument.
> 
> >
> > How many ports does this controller have?
> 
> This controller has three ports.
> 
> >
> > > +
> > > + regmap_write(regmap, AT91_SFR_OHCIICR, regval);
> > > +
> > > + return 0;
> > > +}
> > > +
> > >  /*
> > >   * Look at the control requests to the root hub and see if we need to 
> > > override.
> > >   */
> > > @@ -289,6 +330,7 @@ static int ohci_at91_hub_control(struct usb_hcd
> > > *hcd,
> > u16 typeReq, u16 wValue,
> > >u16 wIndex, char *buf, u16 wLength)  {
> > >   struct at91_usbh_data *pdata =
> > > dev_get_platdata(hcd->self.controller);
> > > + struct ohci_at91_priv *ohci_at91 = hcd_to_ohci_at91_priv(hcd);
> > >   struct usb_hub_descriptor *desc;
> > >   int ret = -EINVAL;
> > >   u32 

RE: [PATCH v5] usb: ohci-at91: Forcibly suspend ports while USB suspend

[Wenyou.Yang]

Hi Alan,

As you saw, I think the version 4 is better than this, can we take the version 
4?


Best Regards,
Wenyou Yang

> -Original Message-
> From: wenyou.y...@microchip.com [mailto:wenyou.y...@microchip.com]
> Sent: 2016年8月5日 11:46
> To: st...@rowland.harvard.edu; wenyou.y...@atmel.com
> Cc: gre...@linuxfoundation.org; nicolas.fe...@atmel.com;
> alexandre.bell...@free-electrons.com; linux-kernel@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux-...@vger.kernel.org
> Subject: RE: [PATCH v5] usb: ohci-at91: Forcibly suspend ports while USB
> suspend
> 
> Hi Alan,
> 
> > -Original Message-
> > From: Alan Stern [mailto:st...@rowland.harvard.edu]
> > Sent: 2016年8月4日 23:02
> > To: Wenyou Yang 
> > Cc: Greg Kroah-Hartman ; Nicolas Ferre
> > ; Alexandre Belloni  > electrons.com>; linux-kernel@vger.kernel.org; linux-arm-
> > ker...@lists.infradead.org; linux-...@vger.kernel.org
> > Subject: Re: [PATCH v5] usb: ohci-at91: Forcibly suspend ports while
> > USB suspend
> >
> > On Thu, 4 Aug 2016, Wenyou Yang wrote:
> >
> > > The usb controller does not managed correctly the suspend mode for
> > > the ehci. In echi mode, there is no way to have utmi_suspend_o_n[1]
> > > suspend without any device connected to it. This is why this
> > > specific control is added to fix this issue. The suspend mode works
> > > in ohci mode.
> >
> > Why are you talking about EHCI mode?  This patch is only for the
> > ohci-at91 driver.
> 
> Actually I described the issue according to the documents from our IP, and 
> this
> specific control is recommended to do in ohci mode.
> 
> >
> > > This specific control is by setting the SUSPEND_A/B/C fields of
> > > SFR_OHCIICR(OHCI Interrupt Configuration Register) in the SFR while
> > > OHCI USB suspend.
> > >
> > > This setting operation must be done before the USB clock disabled,
> > > clear them after the USB clock enabled.
> > >
> > > Signed-off-by: Wenyou Yang 
> > > Reviewed-by: Alexandre Belloni
> > > 
> > > Acked-by: Nicolas Ferre 
> >
> > I don't know if this is any better than before!  See the comments below.
> 
> Yes, I think so.
> 
> What else advice?
> 
> >
> > > ---
> > >
> > > Changes in v5:
> > >  - Use the USB_PORT_FEAT_SUSPEND subcase of the SetPortFeature case
> > >to take care it.
> > >  - Update the commit log.
> > >
> > > Changes in v4:
> > >  - To check whether the SFR node with "atmel,sama5d2-sfr" compatible
> > >is present or not to decide if this feature is applied or not
> > >when USB OHCI suspend/resume, instead of new compatible.
> > >  - Drop the compatible "atmel,sama5d2-ohci".
> > >  - Drop [PATCH 2/2] ARM: at91/dt: sama5d2: Use new compatible for
> > >ohci node.
> > >  - Drop include/soc/at91/at91_sfr.h, move the macro definitions to
> > >atmel-sfr.h which already exists.
> > >  - Change the defines to align the exists.
> > >
> > > Changes in v3:
> > >  - Change the compatible description for more precise.
> > >
> > > Changes in v2:
> > >  - Add compatible to support forcibly suspend the ports.
> > >  - Add soc/at91/at91_sfr.h to accommodate the defines.
> > >  - Add error checking for .sfr_regmap.
> > >  - Remove unnecessary regmap_read() statement.
> >
> >
> > > @@ -282,6 +301,28 @@ static int ohci_at91_hub_status_data(struct
> > > usb_hcd
> > *hcd, char *buf)
> > >   return length;
> > >  }
> > >
> > > +static int ohci_at91_port_ctrl(struct regmap *regmap, u16 port, u8
> > > +set) {
> > > + u32 regval;
> > > + int ret;
> > > +
> > > + if (!regmap)
> > > + return 0;
> > > +
> > > + ret = regmap_read(regmap, AT91_SFR_OHCIICR, );
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (set)
> > > + regval |= AT91_OHCIICR_SUSPEND(port);
> > > + else
> > > + regval &= ~AT91_OHCIICR_SUSPEND(port);
> >
> > In the earlier versions of this patch, you did not use the port number.
> > Why has this changed?
> 
> This function is called by ohci_at91_hub_control(), which is invoked on basis 
> of
> port number.
> 
> So, I think it is more reasonable of adding the port argument.
> 
> >
> > How many ports does this controller have?
> 
> This controller has three ports.
> 
> >
> > > +
> > > + regmap_write(regmap, AT91_SFR_OHCIICR, regval);
> > > +
> > > + return 0;
> > > +}
> > > +
> > >  /*
> > >   * Look at the control requests to the root hub and see if we need to 
> > > override.
> > >   */
> > > @@ -289,6 +330,7 @@ static int ohci_at91_hub_control(struct usb_hcd
> > > *hcd,
> > u16 typeReq, u16 wValue,
> > >u16 wIndex, char *buf, u16 wLength)  {
> > >   struct at91_usbh_data *pdata =
> > > dev_get_platdata(hcd->self.controller);
> > > + struct ohci_at91_priv *ohci_at91 = hcd_to_ohci_at91_priv(hcd);
> > >   struct usb_hub_descriptor *desc;
> > >   int ret = -EINVAL;
> > >   u32 *data = (u32 *)buf;
> > > @@ -301,7 +343,8 @@ static int ohci_at91_hub_control(struct usb_hcd
> > > *hcd, u16 typeReq, u16 wValue,
> > >
> > >   switch (typeReq) {
> > >   case 

RE: [PATCH v3 7/7] doc: bindings: act8945a-charger: Update properties

[Wenyou.Yang]

Hi Rob,

> -Original Message-
> From: Rob Herring [mailto:r...@kernel.org]
> Sent: 2016年7月30日 5:40
> To: Wenyou Yang 
> Cc: Sebastian Reichel ; Dmitry Eremin-Solenikov
> ; David Woodhouse ; Pawel
> Moll ; Mark Brown ; Ian Campbell
> ; Kumar Gala ; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; Nicolas Ferre ; linux-
> p...@vger.kernel.org
> Subject: Re: [PATCH v3 7/7] doc: bindings: act8945a-charger: Update properties
> 
> On Fri, Jul 29, 2016 at 09:25:28AM +0800, Wenyou Yang wrote:
> > Due the driver improvements, update the properties,
> >  - Remove "active-semi,check-battery-temperature" property.
> >  - Add the properties, "active-semi,irq_gpio"
> >and "active-semi,lbo-gpios".
> >
> > Signed-off-by: Wenyou Yang 
> > ---
> >
> > Changes in v3: None
> > Changes in v2: None
> >
> >  Documentation/devicetree/bindings/power/act8945a-charger.txt | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/power/act8945a-charger.txt
> b/Documentation/devicetree/bindings/power/act8945a-charger.txt
> > index bea254c..d7cf05b 100644
> > --- a/Documentation/devicetree/bindings/power/act8945a-charger.txt
> > +++ b/Documentation/devicetree/bindings/power/act8945a-charger.txt
> > @@ -4,10 +4,12 @@ Required properties:
> >   - compatible: "active-semi,act8945a", please refer to ../mfd/act8945a.txt.
> >   - active-semi,chglev-gpios: charge current level phandle with args
> > as described in ../gpio/gpio.txt.
> > + - active-semi,irq_gpios: specify the charger interrupt input phanle
> > +   with args as as described in ../gpio/gpio.txt.
> 
> As I said in v2, this should be an interrupt property instead.

Sorry for forgetting to change.

Accepted, will be in next version. Thanks.

> 
> > + - active-semi,lbo-gpios: specify the low battery voltage detect phandle
> > +   with args as as described in ../gpio/gpio.txt.
> 
> Maybe this one too.

The capacity level function need to read this pin level, so this one should not
use "interrupts" property.

> 
> >
> >  Optional properties:
> > - - active-semi,check-battery-temperature: boolean to check the battery
> > -   temperature or not.
> >   - active-semi,input-voltage-threshold-microvolt: unit: mV;
> > Specifies the charger's input over-voltage threshold value;
> > The value can be: 6600, 7000, 7500, 8000; default: 6600
> > --
> > 2.7.4
> >

Best Regards,
Wenyou Yang

RE: [PATCH v3 7/7] doc: bindings: act8945a-charger: Update properties

[Wenyou.Yang]

Hi Rob,

> -Original Message-
> From: Rob Herring [mailto:r...@kernel.org]
> Sent: 2016年7月30日 5:40
> To: Wenyou Yang 
> Cc: Sebastian Reichel ; Dmitry Eremin-Solenikov
> ; David Woodhouse ; Pawel
> Moll ; Mark Brown ; Ian Campbell
> ; Kumar Gala ; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; Nicolas Ferre ; linux-
> p...@vger.kernel.org
> Subject: Re: [PATCH v3 7/7] doc: bindings: act8945a-charger: Update properties
> 
> On Fri, Jul 29, 2016 at 09:25:28AM +0800, Wenyou Yang wrote:
> > Due the driver improvements, update the properties,
> >  - Remove "active-semi,check-battery-temperature" property.
> >  - Add the properties, "active-semi,irq_gpio"
> >and "active-semi,lbo-gpios".
> >
> > Signed-off-by: Wenyou Yang 
> > ---
> >
> > Changes in v3: None
> > Changes in v2: None
> >
> >  Documentation/devicetree/bindings/power/act8945a-charger.txt | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/power/act8945a-charger.txt
> b/Documentation/devicetree/bindings/power/act8945a-charger.txt
> > index bea254c..d7cf05b 100644
> > --- a/Documentation/devicetree/bindings/power/act8945a-charger.txt
> > +++ b/Documentation/devicetree/bindings/power/act8945a-charger.txt
> > @@ -4,10 +4,12 @@ Required properties:
> >   - compatible: "active-semi,act8945a", please refer to ../mfd/act8945a.txt.
> >   - active-semi,chglev-gpios: charge current level phandle with args
> > as described in ../gpio/gpio.txt.
> > + - active-semi,irq_gpios: specify the charger interrupt input phanle
> > +   with args as as described in ../gpio/gpio.txt.
> 
> As I said in v2, this should be an interrupt property instead.

Sorry for forgetting to change.

Accepted, will be in next version. Thanks.

> 
> > + - active-semi,lbo-gpios: specify the low battery voltage detect phandle
> > +   with args as as described in ../gpio/gpio.txt.
> 
> Maybe this one too.

The capacity level function need to read this pin level, so this one should not
use "interrupts" property.

> 
> >
> >  Optional properties:
> > - - active-semi,check-battery-temperature: boolean to check the battery
> > -   temperature or not.
> >   - active-semi,input-voltage-threshold-microvolt: unit: mV;
> > Specifies the charger's input over-voltage threshold value;
> > The value can be: 6600, 7000, 7500, 8000; default: 6600
> > --
> > 2.7.4
> >

Best Regards,
Wenyou Yang

RE: [PATCH v1] net: phy: micrel: Add specific suspend

[Wenyou.Yang]

Hi Florian,

> -Original Message-
> From: Florian Fainelli [mailto:f.faine...@gmail.com]
> Sent: 2016年8月4日 11:33
> To: Wenyou Yang 
> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; Alexandre Belloni  electrons.com>; Nicolas Ferre ; Andrew Lunn
> 
> Subject: Re: [PATCH v1] net: phy: micrel: Add specific suspend
> 
> On 03/08/2016 17:21, Wenyou Yang wrote:
> > Disable all interrupts when suspend, they will be enabled when resume.
> > Otherwise, the suspend/resume process will be blocked occasionally.
> 
> This seems like something fairly generic actually, we could imagine having the
> core library do something like this:
> 
> - if interrupts are valid and enabled for the PHY, call
> phydrv->config_intr() with PHY_INTERRUPT_DISABLED
> - call genphy_suspend

Accepted. I will send a new version.

> 
> Of course if none of this fits the generic model, the PHY driver can still 
> provide a
> suspend callback. Might be worth auditing other drivers for that pattern and 
> look at
> those that need specific handling like keeping specific interrupt sources 
> active for
> e.g: wake-on-LAN.
> 
> >
> > Signed-off-by: Wenyou Yang 
> > Acked-by: Nicolas Ferre 
> > ---
> >
> >  drivers/net/phy/micrel.c | 19 ++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index
> > 5a8fefc..8cb778a 100644
> > --- a/drivers/net/phy/micrel.c
> > +++ b/drivers/net/phy/micrel.c
> > @@ -647,6 +647,23 @@ static void kszphy_get_stats(struct phy_device
> *phydev,
> > data[i] = kszphy_get_stat(phydev, i);  }
> >
> > +int kszphy_suspend(struct phy_device *phydev) {
> > +   int value;
> > +
> > +   mutex_lock(>lock);
> > +
> > +   /* disable interrupts */
> > +   phy_write(phydev, MII_KSZPHY_INTCS, 0x0);
> > +
> > +   value = phy_read(phydev, MII_BMCR);
> > +   phy_write(phydev, MII_BMCR, value | BMCR_PDOWN);
> > +
> > +   mutex_unlock(>lock);
> > +
> > +   return 0;
> > +}
> > +
> >  static int kszphy_resume(struct phy_device *phydev)  {
> > int value;
> > @@ -870,7 +887,7 @@ static struct phy_driver ksphy_driver[] = {
> > .get_sset_count = kszphy_get_sset_count,
> > .get_strings= kszphy_get_strings,
> > .get_stats  = kszphy_get_stats,
> > -   .suspend= genphy_suspend,
> > +   .suspend= kszphy_suspend,
> > .resume = kszphy_resume,
> >  }, {
> > .phy_id = PHY_ID_KSZ8061,
> >


Best Regards,
Wenyou Yang

RE: [PATCH v1] net: phy: micrel: Add specific suspend

[Wenyou.Yang]

Hi Florian,

> -Original Message-
> From: Florian Fainelli [mailto:f.faine...@gmail.com]
> Sent: 2016年8月4日 11:33
> To: Wenyou Yang 
> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; Alexandre Belloni  electrons.com>; Nicolas Ferre ; Andrew Lunn
> 
> Subject: Re: [PATCH v1] net: phy: micrel: Add specific suspend
> 
> On 03/08/2016 17:21, Wenyou Yang wrote:
> > Disable all interrupts when suspend, they will be enabled when resume.
> > Otherwise, the suspend/resume process will be blocked occasionally.
> 
> This seems like something fairly generic actually, we could imagine having the
> core library do something like this:
> 
> - if interrupts are valid and enabled for the PHY, call
> phydrv->config_intr() with PHY_INTERRUPT_DISABLED
> - call genphy_suspend

Accepted. I will send a new version.

> 
> Of course if none of this fits the generic model, the PHY driver can still 
> provide a
> suspend callback. Might be worth auditing other drivers for that pattern and 
> look at
> those that need specific handling like keeping specific interrupt sources 
> active for
> e.g: wake-on-LAN.
> 
> >
> > Signed-off-by: Wenyou Yang 
> > Acked-by: Nicolas Ferre 
> > ---
> >
> >  drivers/net/phy/micrel.c | 19 ++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index
> > 5a8fefc..8cb778a 100644
> > --- a/drivers/net/phy/micrel.c
> > +++ b/drivers/net/phy/micrel.c
> > @@ -647,6 +647,23 @@ static void kszphy_get_stats(struct phy_device
> *phydev,
> > data[i] = kszphy_get_stat(phydev, i);  }
> >
> > +int kszphy_suspend(struct phy_device *phydev) {
> > +   int value;
> > +
> > +   mutex_lock(>lock);
> > +
> > +   /* disable interrupts */
> > +   phy_write(phydev, MII_KSZPHY_INTCS, 0x0);
> > +
> > +   value = phy_read(phydev, MII_BMCR);
> > +   phy_write(phydev, MII_BMCR, value | BMCR_PDOWN);
> > +
> > +   mutex_unlock(>lock);
> > +
> > +   return 0;
> > +}
> > +
> >  static int kszphy_resume(struct phy_device *phydev)  {
> > int value;
> > @@ -870,7 +887,7 @@ static struct phy_driver ksphy_driver[] = {
> > .get_sset_count = kszphy_get_sset_count,
> > .get_strings= kszphy_get_strings,
> > .get_stats  = kszphy_get_stats,
> > -   .suspend= genphy_suspend,
> > +   .suspend= kszphy_suspend,
> > .resume = kszphy_resume,
> >  }, {
> > .phy_id = PHY_ID_KSZ8061,
> >


Best Regards,
Wenyou Yang

RE: [PATCH v5] usb: ohci-at91: Forcibly suspend ports while USB suspend

[Wenyou.Yang]

Hi Alan,

> -Original Message-
> From: Alan Stern [mailto:st...@rowland.harvard.edu]
> Sent: 2016年8月4日 23:02
> To: Wenyou Yang 
> Cc: Greg Kroah-Hartman ; Nicolas Ferre
> ; Alexandre Belloni  electrons.com>; linux-kernel@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux-...@vger.kernel.org
> Subject: Re: [PATCH v5] usb: ohci-at91: Forcibly suspend ports while USB
> suspend
> 
> On Thu, 4 Aug 2016, Wenyou Yang wrote:
> 
> > The usb controller does not managed correctly the suspend mode for the
> > ehci. In echi mode, there is no way to have utmi_suspend_o_n[1]
> > suspend without any device connected to it. This is why this specific
> > control is added to fix this issue. The suspend mode works in ohci
> > mode.
> 
> Why are you talking about EHCI mode?  This patch is only for the
> ohci-at91 driver.

Actually I described the issue according to the documents from our IP,
and this specific control is recommended to do in ohci mode.

> 
> > This specific control is by setting the SUSPEND_A/B/C fields of
> > SFR_OHCIICR(OHCI Interrupt Configuration Register) in the SFR while
> > OHCI USB suspend.
> >
> > This setting operation must be done before the USB clock disabled,
> > clear them after the USB clock enabled.
> >
> > Signed-off-by: Wenyou Yang 
> > Reviewed-by: Alexandre Belloni 
> > Acked-by: Nicolas Ferre 
> 
> I don't know if this is any better than before!  See the comments below.

Yes, I think so.

What else advice?

> 
> > ---
> >
> > Changes in v5:
> >  - Use the USB_PORT_FEAT_SUSPEND subcase of the SetPortFeature case
> >to take care it.
> >  - Update the commit log.
> >
> > Changes in v4:
> >  - To check whether the SFR node with "atmel,sama5d2-sfr" compatible
> >is present or not to decide if this feature is applied or not
> >when USB OHCI suspend/resume, instead of new compatible.
> >  - Drop the compatible "atmel,sama5d2-ohci".
> >  - Drop [PATCH 2/2] ARM: at91/dt: sama5d2: Use new compatible for
> >ohci node.
> >  - Drop include/soc/at91/at91_sfr.h, move the macro definitions to
> >atmel-sfr.h which already exists.
> >  - Change the defines to align the exists.
> >
> > Changes in v3:
> >  - Change the compatible description for more precise.
> >
> > Changes in v2:
> >  - Add compatible to support forcibly suspend the ports.
> >  - Add soc/at91/at91_sfr.h to accommodate the defines.
> >  - Add error checking for .sfr_regmap.
> >  - Remove unnecessary regmap_read() statement.
> 
> 
> > @@ -282,6 +301,28 @@ static int ohci_at91_hub_status_data(struct usb_hcd
> *hcd, char *buf)
> > return length;
> >  }
> >
> > +static int ohci_at91_port_ctrl(struct regmap *regmap, u16 port, u8
> > +set) {
> > +   u32 regval;
> > +   int ret;
> > +
> > +   if (!regmap)
> > +   return 0;
> > +
> > +   ret = regmap_read(regmap, AT91_SFR_OHCIICR, );
> > +   if (ret)
> > +   return ret;
> > +
> > +   if (set)
> > +   regval |= AT91_OHCIICR_SUSPEND(port);
> > +   else
> > +   regval &= ~AT91_OHCIICR_SUSPEND(port);
> 
> In the earlier versions of this patch, you did not use the port number.
> Why has this changed?

This function is called by ohci_at91_hub_control(), which is invoked on basis 
of port number.

So, I think it is more reasonable of adding the port argument. 

> 
> How many ports does this controller have?

This controller has three ports.

> 
> > +
> > +   regmap_write(regmap, AT91_SFR_OHCIICR, regval);
> > +
> > +   return 0;
> > +}
> > +
> >  /*
> >   * Look at the control requests to the root hub and see if we need to 
> > override.
> >   */
> > @@ -289,6 +330,7 @@ static int ohci_at91_hub_control(struct usb_hcd *hcd,
> u16 typeReq, u16 wValue,
> >  u16 wIndex, char *buf, u16 wLength)  {
> > struct at91_usbh_data *pdata =
> > dev_get_platdata(hcd->self.controller);
> > +   struct ohci_at91_priv *ohci_at91 = hcd_to_ohci_at91_priv(hcd);
> > struct usb_hub_descriptor *desc;
> > int ret = -EINVAL;
> > u32 *data = (u32 *)buf;
> > @@ -301,7 +343,8 @@ static int ohci_at91_hub_control(struct usb_hcd
> > *hcd, u16 typeReq, u16 wValue,
> >
> > switch (typeReq) {
> > case SetPortFeature:
> > -   if (wValue == USB_PORT_FEAT_POWER) {
> > +   switch (wValue) {
> > +   case USB_PORT_FEAT_POWER:
> > dev_dbg(hcd->self.controller, "SetPortFeat: POWER\n");
> > if (valid_port(wIndex)) {
> > ohci_at91_usb_set_power(pdata, wIndex, 1); @@
> -309,6 +352,11 @@
> > static int ohci_at91_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 
> > wValue,
> > }
> >
> > goto out;
> > +
> > +   case USB_PORT_FEAT_SUSPEND:
> > +   dev_dbg(hcd->self.controller, "SetPortFeat: 

RE: [PATCH v5] usb: ohci-at91: Forcibly suspend ports while USB suspend

[Wenyou.Yang]

Hi Alan,

> -Original Message-
> From: Alan Stern [mailto:st...@rowland.harvard.edu]
> Sent: 2016年8月4日 23:02
> To: Wenyou Yang 
> Cc: Greg Kroah-Hartman ; Nicolas Ferre
> ; Alexandre Belloni  electrons.com>; linux-kernel@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux-...@vger.kernel.org
> Subject: Re: [PATCH v5] usb: ohci-at91: Forcibly suspend ports while USB
> suspend
> 
> On Thu, 4 Aug 2016, Wenyou Yang wrote:
> 
> > The usb controller does not managed correctly the suspend mode for the
> > ehci. In echi mode, there is no way to have utmi_suspend_o_n[1]
> > suspend without any device connected to it. This is why this specific
> > control is added to fix this issue. The suspend mode works in ohci
> > mode.
> 
> Why are you talking about EHCI mode?  This patch is only for the
> ohci-at91 driver.

Actually I described the issue according to the documents from our IP,
and this specific control is recommended to do in ohci mode.

> 
> > This specific control is by setting the SUSPEND_A/B/C fields of
> > SFR_OHCIICR(OHCI Interrupt Configuration Register) in the SFR while
> > OHCI USB suspend.
> >
> > This setting operation must be done before the USB clock disabled,
> > clear them after the USB clock enabled.
> >
> > Signed-off-by: Wenyou Yang 
> > Reviewed-by: Alexandre Belloni 
> > Acked-by: Nicolas Ferre 
> 
> I don't know if this is any better than before!  See the comments below.

Yes, I think so.

What else advice?

> 
> > ---
> >
> > Changes in v5:
> >  - Use the USB_PORT_FEAT_SUSPEND subcase of the SetPortFeature case
> >to take care it.
> >  - Update the commit log.
> >
> > Changes in v4:
> >  - To check whether the SFR node with "atmel,sama5d2-sfr" compatible
> >is present or not to decide if this feature is applied or not
> >when USB OHCI suspend/resume, instead of new compatible.
> >  - Drop the compatible "atmel,sama5d2-ohci".
> >  - Drop [PATCH 2/2] ARM: at91/dt: sama5d2: Use new compatible for
> >ohci node.
> >  - Drop include/soc/at91/at91_sfr.h, move the macro definitions to
> >atmel-sfr.h which already exists.
> >  - Change the defines to align the exists.
> >
> > Changes in v3:
> >  - Change the compatible description for more precise.
> >
> > Changes in v2:
> >  - Add compatible to support forcibly suspend the ports.
> >  - Add soc/at91/at91_sfr.h to accommodate the defines.
> >  - Add error checking for .sfr_regmap.
> >  - Remove unnecessary regmap_read() statement.
> 
> 
> > @@ -282,6 +301,28 @@ static int ohci_at91_hub_status_data(struct usb_hcd
> *hcd, char *buf)
> > return length;
> >  }
> >
> > +static int ohci_at91_port_ctrl(struct regmap *regmap, u16 port, u8
> > +set) {
> > +   u32 regval;
> > +   int ret;
> > +
> > +   if (!regmap)
> > +   return 0;
> > +
> > +   ret = regmap_read(regmap, AT91_SFR_OHCIICR, );
> > +   if (ret)
> > +   return ret;
> > +
> > +   if (set)
> > +   regval |= AT91_OHCIICR_SUSPEND(port);
> > +   else
> > +   regval &= ~AT91_OHCIICR_SUSPEND(port);
> 
> In the earlier versions of this patch, you did not use the port number.
> Why has this changed?

This function is called by ohci_at91_hub_control(), which is invoked on basis 
of port number.

So, I think it is more reasonable of adding the port argument. 

> 
> How many ports does this controller have?

This controller has three ports.

> 
> > +
> > +   regmap_write(regmap, AT91_SFR_OHCIICR, regval);
> > +
> > +   return 0;
> > +}
> > +
> >  /*
> >   * Look at the control requests to the root hub and see if we need to 
> > override.
> >   */
> > @@ -289,6 +330,7 @@ static int ohci_at91_hub_control(struct usb_hcd *hcd,
> u16 typeReq, u16 wValue,
> >  u16 wIndex, char *buf, u16 wLength)  {
> > struct at91_usbh_data *pdata =
> > dev_get_platdata(hcd->self.controller);
> > +   struct ohci_at91_priv *ohci_at91 = hcd_to_ohci_at91_priv(hcd);
> > struct usb_hub_descriptor *desc;
> > int ret = -EINVAL;
> > u32 *data = (u32 *)buf;
> > @@ -301,7 +343,8 @@ static int ohci_at91_hub_control(struct usb_hcd
> > *hcd, u16 typeReq, u16 wValue,
> >
> > switch (typeReq) {
> > case SetPortFeature:
> > -   if (wValue == USB_PORT_FEAT_POWER) {
> > +   switch (wValue) {
> > +   case USB_PORT_FEAT_POWER:
> > dev_dbg(hcd->self.controller, "SetPortFeat: POWER\n");
> > if (valid_port(wIndex)) {
> > ohci_at91_usb_set_power(pdata, wIndex, 1); @@
> -309,6 +352,11 @@
> > static int ohci_at91_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 
> > wValue,
> > }
> >
> > goto out;
> > +
> > +   case USB_PORT_FEAT_SUSPEND:
> > +   dev_dbg(hcd->self.controller, "SetPortFeat: SUSPEND\n");
> > +   ohci_at91_port_ctrl(ohci_at91->sfr_regmap, wIndex, 1);
> > +   break;
> > }
> > break;
> >
> > @@ -342,6 

RE: [PATCH] mtd: spi-nor: Add at25df321 spi-nor flash support

[Wenyou.Yang]

> -Original Message-
> From: Jagan Teki [mailto:jt...@openedev.com]
> Sent: 2016年7月26日 16:38
> To: linux-...@lists.infradead.org
> Cc: David Woodhouse ; linux-kernel@vger.kernel.org;
> Jagan Teki ; Brian Norris
> ; Wenyou Yang 
> Subject: [PATCH] mtd: spi-nor: Add at25df321 spi-nor flash support
> 
> Add Atmel at25df321 spi-nor flash to the list of spi_nor_ids.
> 
> Cc: Brian Norris 
> Cc: Wenyou Yang 
> Signed-off-by: Jagan Teki 

Acked-by Wenyou Yang 

> ---
>  drivers/mtd/spi-nor/spi-nor.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c 
> index
> d0fc165..ec47451 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -799,6 +799,7 @@ static const struct flash_info spi_nor_ids[] = {
>   { "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K) },
> 
>   { "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K) },
> + { "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64, SECT_4K) },
>   { "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K) },
>   { "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K) },
> 
> --
> 2.7.4


Best Regards,
Wenyou Yang

RE: [PATCH] mtd: spi-nor: Add at25df321 spi-nor flash support

[Wenyou.Yang]

> -Original Message-
> From: Jagan Teki [mailto:jt...@openedev.com]
> Sent: 2016年7月26日 16:38
> To: linux-...@lists.infradead.org
> Cc: David Woodhouse ; linux-kernel@vger.kernel.org;
> Jagan Teki ; Brian Norris
> ; Wenyou Yang 
> Subject: [PATCH] mtd: spi-nor: Add at25df321 spi-nor flash support
> 
> Add Atmel at25df321 spi-nor flash to the list of spi_nor_ids.
> 
> Cc: Brian Norris 
> Cc: Wenyou Yang 
> Signed-off-by: Jagan Teki 

Acked-by Wenyou Yang 

> ---
>  drivers/mtd/spi-nor/spi-nor.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c 
> index
> d0fc165..ec47451 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -799,6 +799,7 @@ static const struct flash_info spi_nor_ids[] = {
>   { "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K) },
> 
>   { "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K) },
> + { "at25df321",  INFO(0x1f4700, 0, 64 * 1024,  64, SECT_4K) },
>   { "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K) },
>   { "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K) },
> 
> --
> 2.7.4


Best Regards,
Wenyou Yang