Devicetree
 help / color / mirror / Atom feed
* Re: [PATCH] drm/arcpgu: Get rid of "encoder-slave" property
From: Liviu.Dudau @ 2017-03-29 13:48 UTC (permalink / raw)
  To: Alexey Brodkin
  Cc: robh@kernel.org, linux-kernel@vger.kernel.org, robh+dt@kernel.org,
	devicetree@vger.kernel.org, linux-snps-arc@lists.infradead.org,
	Eugeniy Paltsev, daniel@ffwll.ch, dri-devel@lists.freedesktop.org,
	airlied@linux.ie
In-Reply-To: <1490794439.7754.88.camel@synopsys.com>

On Wed, Mar 29, 2017 at 01:34:00PM +0000, Alexey Brodkin wrote:
> Hi Liviu, Rob,

Hi Alexey,

> 
> On Fri, 2017-03-03 at 18:21 +0000, Liviu.Dudau@arm.com wrote:
> > On Fri, Mar 03, 2017 at 05:48:19PM +0000, Alexey Brodkin wrote:
> > > 
> > > Hi Liviu,
> > > 
> > > On Fri, 2017-03-03 at 16:28 +0000, Liviu Dudau wrote:
> > > > 
> > > > On Fri, Mar 03, 2017 at 06:19:24PM +0300, Alexey Brodkin wrote:
> > > > > 
> > > > > 
> > > > > -	/* find the encoder node and initialize it */
> > > > > -	encoder_node = of_parse_phandle(drm->dev->of_node, "encoder-slave", 0);
> > > > > -	if (encoder_node) {
> > > > > -		ret = arcpgu_drm_hdmi_init(drm, encoder_node);
> > > > > -		of_node_put(encoder_node);
> > > > > +	/* There is only one output port inside each device, find it */
> > > > > +	port = of_graph_get_next_endpoint(pdev->dev.of_node, NULL);
> > > > > +
> > > > > +	if (port) {
> > > > > +		if (of_device_is_available(port))
> > > > > +			encoder = of_graph_get_remote_port_parent(port);
> > > > > +		of_node_put(port);
> > > > > +	}
> > > > 
> > > > You must've been looking at some old version. Current version in -next uses
> > > > of_graph_get_remote_node() to replace all those lines you have added (see Rob
> > > > Herring's series to introduce of_graph_get_remote_node() function)
> > > 
> > > Hm, I'm not on Linus' master tree [1] and so I thought I was quite up to date :)
> > > Still I made a check of linux-next and don't see any changes in
> > > "drivers/gpu/drm/arm" compared to Linus' tree.
> > > 
> > > [1] https://urldefense.proofpoint.com/v2/url?u=http-3A__git.kernel.org_cgit_linux_kernel_git_torvalds_linux.git_commit_drivers_gpu_drm_arm-3Fid-3D
> > > e4563f6ba71792c77aeccb2092cc23149b44e642&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=lqdeeSSEes0GFDDl656eViXO7breS55ytWkhpk5R81I&m=SI66ngnnXy33ncb8m5H4La2
> > > T1SzSEiiP7hc_XsRahEc&s=uaswjVXcjYDrUosOkO_UpTMqJMWTT-LLPrg5JE6-t-8&e= 
> > > [2] https://urldefense.proofpoint.com/v2/url?u=http-3A__git.kernel.org_cgit_linux_kernel_git_next_linux-2Dnext.git_commit_drivers_gpu_drm_arm-3Fid
> > > -3De4563f6ba71792c77aeccb2092cc23149b44e642&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=lqdeeSSEes0GFDDl656eViXO7breS55ytWkhpk5R81I&m=SI66ngnnXy33ncb8m5H4
> > > La2T1SzSEiiP7hc_XsRahEc&s=hl9Y6s3K9LwLL1M2WnL3ODax_V-ZRh8k1iTiyctIqU4&e= 
> > > 
> > > Could you please clarify which exact tree did you mean?
> > 
> > Sorry, I thought the series got pulled by one of the DRM trees, but it looks like
> > I was wrong. I was carrying a private copy in my internal tree, waiting for the
> > moment when it got pulled into drm-next or drm-misc-next.
> > 
> > Rob, do you have an update on your series introducing of_graph_get_remote_node() ?
> 
> For some reason I cannot find any relevant commits in linux-next tree even today.
> Could you please point me to either any random git tree with mentioned above change or
> maybe just mailing list where this patch was sent?

Not sure why Rob hasn't added it to linux-next, but (according to Rob) this is the latest version:

https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/log/?h=of-graph-helpers

Best regards,
Liviu

> 
> I'd like to implement the same fix in ARCPGU and call it a day finally.
> 
> -Alexey

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

^ permalink raw reply

* Re: [PATCHv6 11/14] ov2640: convert from soc-camera to a standard subdev sensor driver.
From: Hugues FRUCHET @ 2017-03-29 13:50 UTC (permalink / raw)
  To: Hans Verkuil, linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
  Cc: Guennadi Liakhovetski, Songjun Wu, Sakari Ailus,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Hans Verkuil
In-Reply-To: <5623f5dd-88b3-10e6-5703-9400a893a530-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org>

Y're welcome, thanks Hans for this work, this allows us to drop our 
camera interface driver directly based on subdev.

BR,
Hugues.

On 03/29/2017 03:44 PM, Hans Verkuil wrote:
> On 29/03/17 15:42, Hugues FRUCHET wrote:
>> Acked-by: Hugues Fruchet <hugues.fruchet-qxv4g6HH51o@public.gmane.org>
>>
>> Tested successfully on STM324x9I-EVAL evaluation board embedding
>> an OV2640 camera sensor.
>>
>> I don't understand the comment around s_power op that has been dropped
>> (it is there in code), and no problem is observed doing several
>> open/close, tell me if I miss something.
>
> Darn, I forgot to remove that comment in the commit log. It's a leftover from
> an earlier version.
>
> Regards,
>
> 	Hans
>
>>
>> BR,
>> Hugues.
>>
>> On 03/28/2017 10:23 AM, Hans Verkuil wrote:
>>> From: Hans Verkuil <hans.verkuil-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
>>>
>>> Convert ov2640 to a standard subdev driver. The soc-camera driver no longer
>>> uses this driver, so it can safely be converted.
>>>
>>> Note: the s_power op has been dropped: this never worked. When the last open()
>>> is closed, then the power is turned off, and when it is opened again the power
>>> is turned on again, but the old state isn't restored.
>>>
>>> Someone else can figure out in the future how to get this working correctly,
>>> but I don't want to spend more time on this.
>>>
>>> Signed-off-by: Hans Verkuil <hans.verkuil-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
>>> Acked-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>> ---
>>>  drivers/media/i2c/Kconfig                   | 11 ++++
>>>  drivers/media/i2c/Makefile                  |  1 +
>>>  drivers/media/i2c/{soc_camera => }/ov2640.c | 89 +++++------------------------
>>>  drivers/media/i2c/soc_camera/Kconfig        |  6 --
>>>  drivers/media/i2c/soc_camera/Makefile       |  1 -
>>>  5 files changed, 27 insertions(+), 81 deletions(-)
>>>  rename drivers/media/i2c/{soc_camera => }/ov2640.c (94%)
>>>
>>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>>> index cee1dae6e014..db2c63f592c5 100644
>>> --- a/drivers/media/i2c/Kconfig
>>> +++ b/drivers/media/i2c/Kconfig
>>> @@ -520,6 +520,17 @@ config VIDEO_APTINA_PLL
>>>  config VIDEO_SMIAPP_PLL
>>>  	tristate
>>>
>>> +config VIDEO_OV2640
>>> +	tristate "OmniVision OV2640 sensor support"
>>> +	depends on VIDEO_V4L2 && I2C && GPIOLIB
>>> +	depends on MEDIA_CAMERA_SUPPORT
>>> +	help
>>> +	  This is a Video4Linux2 sensor-level driver for the OmniVision
>>> +	  OV2640 camera.
>>> +
>>> +	  To compile this driver as a module, choose M here: the
>>> +	  module will be called ov2640.
>>> +
>>>  config VIDEO_OV2659
>>>  	tristate "OmniVision OV2659 sensor support"
>>>  	depends on VIDEO_V4L2 && I2C
>>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>>> index 5bc7bbeb5499..50af1e11c85a 100644
>>> --- a/drivers/media/i2c/Makefile
>>> +++ b/drivers/media/i2c/Makefile
>>> @@ -57,6 +57,7 @@ obj-$(CONFIG_VIDEO_VP27SMPX) += vp27smpx.o
>>>  obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
>>>  obj-$(CONFIG_VIDEO_UPD64031A) += upd64031a.o
>>>  obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
>>> +obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
>>>  obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
>>>  obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
>>>  obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
>>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/ov2640.c
>>> similarity index 94%
>>> rename from drivers/media/i2c/soc_camera/ov2640.c
>>> rename to drivers/media/i2c/ov2640.c
>>> index b9a0069f5b33..83f88efbce69 100644
>>> --- a/drivers/media/i2c/soc_camera/ov2640.c
>>> +++ b/drivers/media/i2c/ov2640.c
>>> @@ -24,8 +24,8 @@
>>>  #include <linux/v4l2-mediabus.h>
>>>  #include <linux/videodev2.h>
>>>
>>> -#include <media/soc_camera.h>
>>>  #include <media/v4l2-clk.h>
>>> +#include <media/v4l2-device.h>
>>>  #include <media/v4l2-subdev.h>
>>>  #include <media/v4l2-ctrls.h>
>>>  #include <media/v4l2-image-sizes.h>
>>> @@ -287,7 +287,6 @@ struct ov2640_priv {
>>>  	struct v4l2_clk			*clk;
>>>  	const struct ov2640_win_size	*win;
>>>
>>> -	struct soc_camera_subdev_desc	ssdd_dt;
>>>  	struct gpio_desc *resetb_gpio;
>>>  	struct gpio_desc *pwdn_gpio;
>>>  };
>>> @@ -677,13 +676,8 @@ static int ov2640_reset(struct i2c_client *client)
>>>  }
>>>
>>>  /*
>>> - * soc_camera_ops functions
>>> + * functions
>>>   */
>>> -static int ov2640_s_stream(struct v4l2_subdev *sd, int enable)
>>> -{
>>> -	return 0;
>>> -}
>>> -
>>>  static int ov2640_s_ctrl(struct v4l2_ctrl *ctrl)
>>>  {
>>>  	struct v4l2_subdev *sd =
>>> @@ -744,10 +738,16 @@ static int ov2640_s_register(struct v4l2_subdev *sd,
>>>  static int ov2640_s_power(struct v4l2_subdev *sd, int on)
>>>  {
>>>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>>> -	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
>>>  	struct ov2640_priv *priv = to_ov2640(client);
>>>
>>> -	return soc_camera_set_power(&client->dev, ssdd, priv->clk, on);
>>> +	gpiod_direction_output(priv->pwdn_gpio, !on);
>>> +	if (on && priv->resetb_gpio) {
>>> +		/* Active the resetb pin to perform a reset pulse */
>>> +		gpiod_direction_output(priv->resetb_gpio, 1);
>>> +		usleep_range(3000, 5000);
>>> +		gpiod_direction_output(priv->resetb_gpio, 0);
>>> +	}
>>> +	return 0;
>>>  }
>>>
>>>  /* Select the nearest higher resolution for capture */
>>> @@ -994,26 +994,6 @@ static struct v4l2_subdev_core_ops ov2640_subdev_core_ops = {
>>>  	.s_power	= ov2640_s_power,
>>>  };
>>>
>>> -static int ov2640_g_mbus_config(struct v4l2_subdev *sd,
>>> -				struct v4l2_mbus_config *cfg)
>>> -{
>>> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
>>> -	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
>>> -
>>> -	cfg->flags = V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_MASTER |
>>> -		V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_HIGH |
>>> -		V4L2_MBUS_DATA_ACTIVE_HIGH;
>>> -	cfg->type = V4L2_MBUS_PARALLEL;
>>> -	cfg->flags = soc_camera_apply_board_flags(ssdd, cfg);
>>> -
>>> -	return 0;
>>> -}
>>> -
>>> -static struct v4l2_subdev_video_ops ov2640_subdev_video_ops = {
>>> -	.s_stream	= ov2640_s_stream,
>>> -	.g_mbus_config	= ov2640_g_mbus_config,
>>> -};
>>> -
>>>  static const struct v4l2_subdev_pad_ops ov2640_subdev_pad_ops = {
>>>  	.enum_mbus_code = ov2640_enum_mbus_code,
>>>  	.get_selection	= ov2640_get_selection,
>>> @@ -1023,40 +1003,9 @@ static const struct v4l2_subdev_pad_ops ov2640_subdev_pad_ops = {
>>>
>>>  static struct v4l2_subdev_ops ov2640_subdev_ops = {
>>>  	.core	= &ov2640_subdev_core_ops,
>>> -	.video	= &ov2640_subdev_video_ops,
>>>  	.pad	= &ov2640_subdev_pad_ops,
>>>  };
>>>
>>> -/* OF probe functions */
>>> -static int ov2640_hw_power(struct device *dev, int on)
>>> -{
>>> -	struct i2c_client *client = to_i2c_client(dev);
>>> -	struct ov2640_priv *priv = to_ov2640(client);
>>> -
>>> -	dev_dbg(&client->dev, "%s: %s the camera\n",
>>> -			__func__, on ? "ENABLE" : "DISABLE");
>>> -
>>> -	if (priv->pwdn_gpio)
>>> -		gpiod_direction_output(priv->pwdn_gpio, !on);
>>> -
>>> -	return 0;
>>> -}
>>> -
>>> -static int ov2640_hw_reset(struct device *dev)
>>> -{
>>> -	struct i2c_client *client = to_i2c_client(dev);
>>> -	struct ov2640_priv *priv = to_ov2640(client);
>>> -
>>> -	if (priv->resetb_gpio) {
>>> -		/* Active the resetb pin to perform a reset pulse */
>>> -		gpiod_direction_output(priv->resetb_gpio, 1);
>>> -		usleep_range(3000, 5000);
>>> -		gpiod_direction_output(priv->resetb_gpio, 0);
>>> -	}
>>> -
>>> -	return 0;
>>> -}
>>> -
>>>  static int ov2640_probe_dt(struct i2c_client *client,
>>>  		struct ov2640_priv *priv)
>>>  {
>>> @@ -1076,11 +1025,6 @@ static int ov2640_probe_dt(struct i2c_client *client,
>>>  	else if (IS_ERR(priv->pwdn_gpio))
>>>  		return PTR_ERR(priv->pwdn_gpio);
>>>
>>> -	/* Initialize the soc_camera_subdev_desc */
>>> -	priv->ssdd_dt.power = ov2640_hw_power;
>>> -	priv->ssdd_dt.reset = ov2640_hw_reset;
>>> -	client->dev.platform_data = &priv->ssdd_dt;
>>> -
>>>  	return 0;
>>>  }
>>>
>>> @@ -1091,7 +1035,6 @@ static int ov2640_probe(struct i2c_client *client,
>>>  			const struct i2c_device_id *did)
>>>  {
>>>  	struct ov2640_priv	*priv;
>>> -	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
>>>  	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
>>>  	int			ret;
>>>
>>> @@ -1112,17 +1055,15 @@ static int ov2640_probe(struct i2c_client *client,
>>>  	if (IS_ERR(priv->clk))
>>>  		return -EPROBE_DEFER;
>>>
>>> -	if (!ssdd && !client->dev.of_node) {
>>> +	if (!client->dev.of_node) {
>>>  		dev_err(&client->dev, "Missing platform_data for driver\n");
>>>  		ret = -EINVAL;
>>>  		goto err_clk;
>>>  	}
>>>
>>> -	if (!ssdd) {
>>> -		ret = ov2640_probe_dt(client, priv);
>>> -		if (ret)
>>> -			goto err_clk;
>>> -	}
>>> +	ret = ov2640_probe_dt(client, priv);
>>> +	if (ret)
>>> +		goto err_clk;
>>>
>>>  	v4l2_i2c_subdev_init(&priv->subdev, client, &ov2640_subdev_ops);
>>>  	v4l2_ctrl_handler_init(&priv->hdl, 2);
>>> @@ -1190,6 +1131,6 @@ static struct i2c_driver ov2640_i2c_driver = {
>>>
>>>  module_i2c_driver(ov2640_i2c_driver);
>>>
>>> -MODULE_DESCRIPTION("SoC Camera driver for Omni Vision 2640 sensor");
>>> +MODULE_DESCRIPTION("Driver for Omni Vision 2640 sensor");
>>>  MODULE_AUTHOR("Alberto Panizzo");
>>>  MODULE_LICENSE("GPL v2");
>>> diff --git a/drivers/media/i2c/soc_camera/Kconfig b/drivers/media/i2c/soc_camera/Kconfig
>>> index 7704bcf5cc25..96859f37cb1c 100644
>>> --- a/drivers/media/i2c/soc_camera/Kconfig
>>> +++ b/drivers/media/i2c/soc_camera/Kconfig
>>> @@ -41,12 +41,6 @@ config SOC_CAMERA_MT9V022
>>>  	help
>>>  	  This driver supports MT9V022 cameras from Micron
>>>
>>> -config SOC_CAMERA_OV2640
>>> -	tristate "ov2640 camera support"
>>> -	depends on SOC_CAMERA && I2C
>>> -	help
>>> -	  This is a ov2640 camera driver
>>> -
>>>  config SOC_CAMERA_OV5642
>>>  	tristate "ov5642 camera support"
>>>  	depends on SOC_CAMERA && I2C
>>> diff --git a/drivers/media/i2c/soc_camera/Makefile b/drivers/media/i2c/soc_camera/Makefile
>>> index 6f994f9353a0..974bdb721dbe 100644
>>> --- a/drivers/media/i2c/soc_camera/Makefile
>>> +++ b/drivers/media/i2c/soc_camera/Makefile
>>> @@ -3,7 +3,6 @@ obj-$(CONFIG_SOC_CAMERA_MT9M001)	+= mt9m001.o
>>>  obj-$(CONFIG_SOC_CAMERA_MT9T031)	+= mt9t031.o
>>>  obj-$(CONFIG_SOC_CAMERA_MT9T112)	+= mt9t112.o
>>>  obj-$(CONFIG_SOC_CAMERA_MT9V022)	+= mt9v022.o
>>> -obj-$(CONFIG_SOC_CAMERA_OV2640)		+= ov2640.o
>>>  obj-$(CONFIG_SOC_CAMERA_OV5642)		+= ov5642.o
>>>  obj-$(CONFIG_SOC_CAMERA_OV6650)		+= ov6650.o
>>>  obj-$(CONFIG_SOC_CAMERA_OV772X)		+= ov772x.o
>>>
>--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 2/2] mtd: spi-nor: add driver for STM32 quad spi flash controller
From: Marek Vasut @ 2017-03-29 13:57 UTC (permalink / raw)
  To: Ludovic BARRE, Cyrille Pitchen
  Cc: David Woodhouse, Brian Norris, Boris Brezillon,
	Richard Weinberger, Alexandre Torgue, Rob Herring, linux-mtd,
	linux-kernel, devicetree
In-Reply-To: <2410fcde-1d1d-57b5-01ee-bd6bc3bc863b@st.com>

On 03/29/2017 03:35 PM, Ludovic BARRE wrote:

[...]

>>>>> +    writel_relaxed(CR_PRESC(presc) | CR_FTHRES(3) | CR_TCEN |
>>>>> CR_SSHIFT
>>>>> +               | CR_EN, qspi->io_base + QUADSPI_CR);
>>>>> +
>>>>> +    /* a minimum fsize must be set to sent the command id */
>>>>> +    flash->fsize = 25;
>>>> I don't understand why this is needed and the comment doesn't make
>>>> sense. Please fix.
>>> fsize field defines the size of external memory.
>> What external memory ? Unclear
> oops, fsize field defined the size of "flash memory" in stm32 qspi
> controller.

Errr, now I am totally lost :) Is that some internal SPI NOR ? Shouldn't
the size be coming from DT or something ?

> Number of bytes in Flash memory = 2 ^[FSIZE+1].
> To sent a nor cmd this field must be set (hardware issue),
> but before "spi_nor_scan" the size of flash nor is not know.
> So I set a temporary value (workaround).

Is it needed before the scan ?

> After "spi_nor_scan" the fsize is overwritten by the right value
> flash->fsize = __fls(mtd->size) - 1;
>>> Normaly, this field is used only for memory map mode,
>>> but in fact is check in indirect mode.
>>> So while flash scan "spi_nor_scan":
>>> -I can't let 0.
>>> -I not know yet the size of flash.
>>> So I fix a temporary value
>>>
>>> I will update my comment
>> Please do, also please consider that I'm reading the comment and I
>> barely have any clue about this hardware , so make sure I can
>> understand it.
>>
>>>>> +    ret = spi_nor_scan(&flash->nor, NULL, flash_read);
>>>>> +    if (ret) {
>>>>> +        dev_err(qspi->dev, "device scan failed\n");
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>> +    flash->fsize = __fls(mtd->size) - 1;
>>>>> +
>>>>> +    writel_relaxed(DCR_CSHT(1), qspi->io_base + QUADSPI_DCR);
>>>>> +
>>>>> +    ret = mtd_device_register(mtd, NULL, 0);
>>>>> +    if (ret) {
>>>>> +        dev_err(qspi->dev, "mtd device parse failed\n");
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>> +    dev_dbg(qspi->dev, "read mm:%s cs:%d bus:%d\n",
>>>>> +        qspi->read_mode == CCR_FMODE_MM ? "yes" : "no", cs_num,
>>>>> width);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>> [...]
>>>>
>>
> 


-- 
Best regards,
Marek Vasut

^ permalink raw reply

* [PATCH] Add hardware PTP support.
From: Rafal Ozieblo @ 2017-03-29 14:00 UTC (permalink / raw)
  To: nicolas.ferre, netdev, linux-kernel, devicetree, linux-arm-kernel,
	davem, harinikatakamlinux, harini.katakam, richardcochran,
	Andrei.Pistirica
  Cc: Rafal Ozieblo

This patch is based on original Harini's patch and Andrei's patch,
implemented in aseparate file to ease the review/maintanance
and integration with other platforms.

In case that macb is compiled as a module, it has been renamed to
cadence-macb.ko to avoid naming confusion in Makefile.

This driver does support GEM-GXL:
- Enable HW time stamp
- Register ptp clock framework
- Initialize PTP related registers
- Updated dma buffer descriptor read/write mechanism
- HW time stamp on the PTP Ethernet packets are received using the
  SO_TIMESTAMPING API. Where timers are obtained from the dma buffer
  descriptors
- Added tsu_clk to device tree

Note: Patch on net-next, on March 15th.

Signed-off-by: Rafal Ozieblo <rafalo@cadence.com>
---
 Documentation/devicetree/bindings/net/macb.txt |   1 +
 drivers/net/ethernet/cadence/Kconfig           |  10 +-
 drivers/net/ethernet/cadence/Makefile          |   7 +-
 drivers/net/ethernet/cadence/macb.c            | 237 ++++++--
 drivers/net/ethernet/cadence/macb.h            | 176 +++++-
 drivers/net/ethernet/cadence/macb_ptp.c        | 724 +++++++++++++++++++++++++
 6 files changed, 1109 insertions(+), 46 deletions(-)
 create mode 100755 drivers/net/ethernet/cadence/macb_ptp.c

diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
index 1506e94..27966ae 100644
--- a/Documentation/devicetree/bindings/net/macb.txt
+++ b/Documentation/devicetree/bindings/net/macb.txt
@@ -22,6 +22,7 @@ Required properties:
 	Required elements: 'pclk', 'hclk'
 	Optional elements: 'tx_clk'
 	Optional elements: 'rx_clk' applies to cdns,zynqmp-gem
+	Optional elements: 'tsu_clk'
 - clocks: Phandles to input clocks.
 
 Optional properties for PHY child node:
diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
index 608bea1..427d65a 100644
--- a/drivers/net/ethernet/cadence/Kconfig
+++ b/drivers/net/ethernet/cadence/Kconfig
@@ -29,7 +29,15 @@ config MACB
 	  support for the MACB/GEM chip.
 
 	  To compile this driver as a module, choose M here: the module
-	  will be called macb.
+	  will be macb.
+
+config MACB_USE_HWSTAMP
+	bool "Use IEEE 1588 hwstamp"
+	depends on MACB
+	default y
+	imply PTP_1588_CLOCK
+	---help---
+	  Enable IEEE 1588 Precision Time Protocol (PTP) support for MACB.
 
 config MACB_PCI
 	tristate "Cadence PCI MACB/GEM support"
diff --git a/drivers/net/ethernet/cadence/Makefile b/drivers/net/ethernet/cadence/Makefile
index 4ba7559..a7f6e04 100644
--- a/drivers/net/ethernet/cadence/Makefile
+++ b/drivers/net/ethernet/cadence/Makefile
@@ -1,6 +1,11 @@
 #
 # Makefile for the Atmel network device drivers.
 #
+cadence-macb-y	:= macb.o
 
-obj-$(CONFIG_MACB) += macb.o
+ifeq ($(CONFIG_MACB_USE_HWSTAMP),y)
+cadence-macb-y	+= macb_ptp.o
+endif
+
+obj-$(CONFIG_MACB) += cadence-macb.o
 obj-$(CONFIG_MACB_PCI) += macb_pci.o
diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 30606b1..32af94e 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -79,33 +79,84 @@
 #define MACB_HALT_TIMEOUT	1230
 
 /* DMA buffer descriptor might be different size
- * depends on hardware configuration.
+ * depends on hardware configuration:
+ *
+ * 1. dma address width 32 bits:
+ *    word 1: 32 bit address of Data Buffer
+ *    word 2: control
+ *
+ * 2. dma address width 64 bits:
+ *    word 1: 32 bit address of Data Buffer
+ *    word 2: control
+ *    word 3: upper 32 bit address of Data Buffer
+ *    word 4: unused
+ *
+ * 3. dma address width 32 bits with hardware timestamping:
+ *    word 1: 32 bit address of Data Buffer
+ *    word 2: control
+ *    word 3: timestamp word 1
+ *    word 4: timestamp word 2
+ *
+ * 4. dma address width 64 bits with hardware timestamping:
+ *    word 1: 32 bit address of Data Buffer
+ *    word 2: control
+ *    word 3: upper 32 bit address of Data Buffer
+ *    word 4: unused
+ *    word 5: timestamp word 1
+ *    word 6: timestamp word 2
  */
 static unsigned int macb_dma_desc_get_size(struct macb *bp)
 {
-#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
-	if (bp->hw_dma_cap == HW_DMA_CAP_64B)
-		return sizeof(struct macb_dma_desc) + sizeof(struct macb_dma_desc_64);
+#ifdef MACB_EXT_DESC
+	unsigned int desc_size;
+
+	switch (bp->hw_dma_cap) {
+	case HW_DMA_CAP_64B:
+		desc_size = sizeof(struct macb_dma_desc)
+			+ sizeof(struct macb_dma_desc_64);
+		break;
+	case HW_DMA_CAP_PTP:
+		desc_size = sizeof(struct macb_dma_desc)
+			+ sizeof(struct macb_dma_desc_ptp);
+		break;
+	case HW_DMA_CAP_64B_PTP:
+		desc_size = sizeof(struct macb_dma_desc)
+			+ sizeof(struct macb_dma_desc_64)
+			+ sizeof(struct macb_dma_desc_ptp);
+		break;
+	default:
+		desc_size = sizeof(struct macb_dma_desc);
+	}
+	return desc_size;
 #endif
 	return sizeof(struct macb_dma_desc);
 }
 
-static unsigned int macb_adj_dma_desc_idx(struct macb *bp, unsigned int idx)
+static unsigned int macb_adj_dma_desc_idx(struct macb *bp, unsigned int desc_idx)
 {
-#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
-	/* Dma buffer descriptor is 4 words length (instead of 2 words)
-	 * for 64b GEM.
-	 */
-	if (bp->hw_dma_cap == HW_DMA_CAP_64B)
-		idx <<= 1;
+#ifdef MACB_EXT_DESC
+	switch (bp->hw_dma_cap) {
+	case HW_DMA_CAP_64B:
+	case HW_DMA_CAP_PTP:
+		desc_idx <<= 1;
+		break;
+	case HW_DMA_CAP_64B_PTP:
+		desc_idx *= 3;
+		break;
+	default:
+		break;
+	}
+	return desc_idx;
 #endif
-	return idx;
+	return desc_idx;
 }
 
 #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
 static struct macb_dma_desc_64 *macb_64b_desc(struct macb *bp, struct macb_dma_desc *desc)
 {
-	return (struct macb_dma_desc_64 *)((void *)desc + sizeof(struct macb_dma_desc));
+	if (bp->hw_dma_cap & HW_DMA_CAP_64B)
+		return (struct macb_dma_desc_64 *)((void *)desc + sizeof(struct macb_dma_desc));
+	return NULL;
 }
 #endif
 
@@ -600,7 +651,7 @@ static void macb_set_addr(struct macb *bp, struct macb_dma_desc *desc, dma_addr_
 #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
 	struct macb_dma_desc_64 *desc_64;
 
-	if (bp->hw_dma_cap == HW_DMA_CAP_64B) {
+	if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
 		desc_64 = macb_64b_desc(bp, desc);
 		desc_64->addrh = upper_32_bits(addr);
 	}
@@ -614,7 +665,7 @@ static dma_addr_t macb_get_addr(struct macb *bp, struct macb_dma_desc *desc)
 #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
 	struct macb_dma_desc_64 *desc_64;
 
-	if (bp->hw_dma_cap == HW_DMA_CAP_64B) {
+	if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
 		desc_64 = macb_64b_desc(bp, desc);
 		addr = ((u64)(desc_64->addrh) << 32);
 	}
@@ -713,7 +764,7 @@ static void macb_tx_error_task(struct work_struct *work)
 	/* Reinitialize the TX desc queue */
 	queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
 #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
-	if (bp->hw_dma_cap == HW_DMA_CAP_64B)
+	if (bp->hw_dma_cap & HW_DMA_CAP_64B)
 		queue_writel(queue, TBQPH, upper_32_bits(queue->tx_ring_dma));
 #endif
 	/* Make TX ring reflect state of hardware */
@@ -775,6 +826,16 @@ static void macb_tx_interrupt(struct macb_queue *queue)
 
 			/* First, update TX stats if needed */
 			if (skb) {
+#ifdef CONFIG_MACB_USE_HWSTAMP
+				if (gem_ptp_do_txstamp(queue, skb, desc) == 0) {
+					/* skb now belongs to timestamp buffer
+					 * and will be removed later
+					 */
+					tx_skb->skb = NULL;
+					schedule_work(&queue->tx_ts_task);
+
+				}
+#endif
 				netdev_vdbg(bp->dev, "skb %u (data %p) TX complete\n",
 					    macb_tx_ring_wrap(bp, tail),
 					    skb->data);
@@ -941,6 +1002,10 @@ static int gem_rx(struct macb *bp, int budget)
 		bp->stats.rx_packets++;
 		bp->stats.rx_bytes += skb->len;
 
+#ifdef CONFIG_MACB_USE_HWSTAMP
+		gem_ptp_do_rxstamp(bp, skb, desc);
+#endif
+
 #if defined(DEBUG) && defined(VERBOSE_DEBUG)
 		netdev_vdbg(bp->dev, "received skb of length %u, csum: %08x\n",
 			    skb->len, skb->csum);
@@ -1263,6 +1328,11 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
 				queue_writel(queue, ISR, MACB_BIT(HRESP));
 		}
 
+#ifdef CONFIG_MACB_USE_HWSTAMP
+		if (status & MACB_PTP_INT_MASK)
+			macb_ptp_int(queue, status);
+#endif
+
 		status = queue_readl(queue, ISR);
 	}
 
@@ -1592,8 +1662,10 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	/* Make newly initialized descriptor visible to hardware */
 	wmb();
-
-	skb_tx_timestamp(skb);
+#ifdef CONFIG_MACB_USE_HWSTAMP
+	if (!bp->ptp_hw_support)
+#endif
+		skb_tx_timestamp(skb);
 
 	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
 
@@ -1921,9 +1993,13 @@ static void macb_configure_dma(struct macb *bp)
 			dmacfg &= ~GEM_BIT(TXCOEN);
 
 #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
-		if (bp->hw_dma_cap == HW_DMA_CAP_64B)
+		if (bp->hw_dma_cap & HW_DMA_CAP_64B)
 			dmacfg |= GEM_BIT(ADDR64);
 #endif
+#ifdef CONFIG_MACB_USE_HWSTAMP
+		if (bp->hw_dma_cap & HW_DMA_CAP_PTP)
+			dmacfg |= GEM_BIT(RXEXT) | GEM_BIT(TXEXT);
+#endif
 		netdev_dbg(bp->dev, "Cadence configure DMA with 0x%08x\n",
 			   dmacfg);
 		gem_writel(bp, DMACFG, dmacfg);
@@ -1971,21 +2047,22 @@ static void macb_init_hw(struct macb *bp)
 	/* Initialize TX and RX buffers */
 	macb_writel(bp, RBQP, lower_32_bits(bp->rx_ring_dma));
 #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
-	if (bp->hw_dma_cap == HW_DMA_CAP_64B)
+	if (bp->hw_dma_cap & HW_DMA_CAP_64B)
 		macb_writel(bp, RBQPH, upper_32_bits(bp->rx_ring_dma));
 #endif
 	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
 		queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
 #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
-		if (bp->hw_dma_cap == HW_DMA_CAP_64B)
-			queue_writel(queue, TBQPH, upper_32_bits(queue->tx_ring_dma));
+		if (bp->hw_dma_cap & HW_DMA_CAP_64B)
+			queue_writel(queue, TBQPH,
+					upper_32_bits(queue->tx_ring_dma));
 #endif
 
 		/* Enable interrupts */
 		queue_writel(queue, IER,
-			     MACB_RX_INT_FLAGS |
-			     MACB_TX_INT_FLAGS |
-			     MACB_BIT(HRESP));
+				MACB_RX_INT_FLAGS |
+				MACB_TX_INT_FLAGS |
+				MACB_BIT(HRESP));
 	}
 
 	/* Enable TX and RX */
@@ -2446,6 +2523,71 @@ static int macb_set_ringparam(struct net_device *netdev,
 	return 0;
 }
 
+#ifdef CONFIG_MACB_USE_HWSTAMP
+static unsigned int gem_get_tsu_rate(struct macb *bp)
+{
+	struct clk *tsu_clk;
+	unsigned int tsu_rate;
+
+	tsu_clk = devm_clk_get(&bp->pdev->dev, "tsu_clk");
+	if (!IS_ERR(tsu_clk))
+		tsu_rate = clk_get_rate(tsu_clk);
+	/* try pclk instead */
+	else if (!IS_ERR(bp->pclk)) {
+		tsu_clk = bp->pclk;
+		tsu_rate = clk_get_rate(tsu_clk);
+	} else
+		return -ENOTSUPP;
+	return tsu_rate;
+}
+
+static s32 gem_get_ptp_max_adj(void)
+{
+	return 64E6;
+}
+
+static int gem_get_ts_info(struct net_device *dev,
+			   struct ethtool_ts_info *info)
+{
+	struct macb *bp = netdev_priv(dev);
+
+	ethtool_op_get_ts_info(dev, info);
+	if (!bp->ptp_hw_support)
+		return 0;
+
+	info->so_timestamping =
+		SOF_TIMESTAMPING_TX_SOFTWARE |
+		SOF_TIMESTAMPING_RX_SOFTWARE |
+		SOF_TIMESTAMPING_SOFTWARE |
+		SOF_TIMESTAMPING_TX_HARDWARE |
+		SOF_TIMESTAMPING_RX_HARDWARE |
+		SOF_TIMESTAMPING_RAW_HARDWARE;
+	info->tx_types =
+		(1 << HWTSTAMP_TX_ONESTEP_SYNC) |
+		(1 << HWTSTAMP_TX_OFF) |
+		(1 << HWTSTAMP_TX_ON);
+	info->rx_filters =
+		(1 << HWTSTAMP_FILTER_NONE) |
+		(1 << HWTSTAMP_FILTER_ALL);
+	info->phc_index = -1;
+
+	if (bp->ptp_clock)
+		info->phc_index = ptp_clock_index(bp->ptp_clock);
+
+	return 0;
+}
+
+static struct macb_ptp_info gem_ptp_info = {
+	.ptp_init	 = gem_ptp_init,
+	.ptp_remove	 = gem_ptp_remove,
+	.get_ptp_max_adj = gem_get_ptp_max_adj,
+	.get_tsu_rate	 = gem_get_tsu_rate,
+	.get_ts_info	 = gem_get_ts_info,
+	.get_hwtst	 = gem_get_hwtst,
+	.set_hwtst	 = gem_set_hwtst,
+};
+#endif
+
 static int macb_get_ts_info(struct net_device *netdev,
 			    struct ethtool_ts_info *info)
 {
@@ -2579,6 +2721,18 @@ static void macb_configure_caps(struct macb *bp,
 		dcfg = gem_readl(bp, DCFG2);
 		if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF))) == 0)
 			bp->caps |= MACB_CAPS_FIFO_MODE;
+		/* if HWSTAMP is configure and gem has the capability */
+#ifdef CONFIG_MACB_USE_HWSTAMP
+		bp->ptp_hw_support = false;
+		if (gem_has_ptp(bp)) {
+			if (!GEM_BFEXT(TSU, gem_readl(bp, DCFG5)))
+				pr_err("GEM doesn't support hardware ptp.\n");
+			else {
+				bp->ptp_hw_support = true;
+				bp->ptp_info = &gem_ptp_info;
+			}
+		}
+#endif
 	}
 
 	dev_dbg(&bp->pdev->dev, "Cadence caps 0x%08x\n", bp->caps);
@@ -2716,7 +2870,7 @@ static int macb_init(struct platform_device *pdev)
 			queue->IMR  = GEM_IMR(hw_q - 1);
 			queue->TBQP = GEM_TBQP(hw_q - 1);
 #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
-			if (bp->hw_dma_cap == HW_DMA_CAP_64B)
+			if (bp->hw_dma_cap & HW_DMA_CAP_64B)
 				queue->TBQPH = GEM_TBQPH(hw_q - 1);
 #endif
 		} else {
@@ -2727,7 +2881,7 @@ static int macb_init(struct platform_device *pdev)
 			queue->IMR  = MACB_IMR;
 			queue->TBQP = MACB_TBQP;
 #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
-			if (bp->hw_dma_cap == HW_DMA_CAP_64B)
+			if (bp->hw_dma_cap & HW_DMA_CAP_64B)
 				queue->TBQPH = MACB_TBQPH;
 #endif
 		}
@@ -3184,7 +3338,9 @@ static const struct macb_config np4_config = {
 };
 
 static const struct macb_config zynqmp_config = {
-	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO,
+	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
+			MACB_CAPS_JUMBO |
+			MACB_CAPS_GEM_HAS_PTP,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
@@ -3218,7 +3374,9 @@ MODULE_DEVICE_TABLE(of, macb_dt_ids);
 #endif /* CONFIG_OF */
 
 static const struct macb_config default_gem_config = {
-	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO,
+	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
+			MACB_CAPS_JUMBO |
+			MACB_CAPS_GEM_HAS_PTP,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
@@ -3307,19 +3465,24 @@ static int macb_probe(struct platform_device *pdev)
 		bp->wol |= MACB_WOL_HAS_MAGIC_PACKET;
 	device_init_wakeup(&pdev->dev, bp->wol & MACB_WOL_HAS_MAGIC_PACKET);
 
-#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
-	if (GEM_BFEXT(DAW64, gem_readl(bp, DCFG6))) {
-		dma_set_mask(&pdev->dev, DMA_BIT_MASK(44));
-		bp->hw_dma_cap = HW_DMA_CAP_64B;
-	} else
-		bp->hw_dma_cap = HW_DMA_CAP_32B;
-#endif
-
 	spin_lock_init(&bp->lock);
 
 	/* setup capabilities */
 	macb_configure_caps(bp, macb_config);
 
+#ifdef MACB_EXT_DESC
+	bp->hw_dma_cap = HW_DMA_CAP_32B;
+#endif
+#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+	if (GEM_BFEXT(DAW64, gem_readl(bp, DCFG6))) {
+		dma_set_mask(&pdev->dev, DMA_BIT_MASK(44));
+		bp->hw_dma_cap |= HW_DMA_CAP_64B;
+	}
+#endif
+#ifdef CONFIG_MACB_USE_HWSTAMP
+	if (bp->ptp_hw_support)
+		bp->hw_dma_cap |= HW_DMA_CAP_PTP;
+#endif
 	platform_set_drvdata(pdev, dev);
 
 	dev->irq = platform_get_irq(pdev, 0);
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 234a49e..07a1e8e 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -11,6 +11,13 @@
 #define _MACB_H
 
 #include <linux/phy.h>
+#include <linux/ptp_clock.h>
+#include <linux/ptp_clock_kernel.h>
+#include <linux/net_tstamp.h>
+
+#if defined(CONFIG_ARCH_DMA_ADDR_T_64BIT) || defined(CONFIG_MACB_USE_HWSTAMP)
+#define MACB_EXT_DESC
+#endif
 
 #define MACB_GREGS_NBR 16
 #define MACB_GREGS_VERSION 2
@@ -86,6 +93,10 @@
 #define GEM_SA3T		0x009C /* Specific3 Top */
 #define GEM_SA4B		0x00A0 /* Specific4 Bottom */
 #define GEM_SA4T		0x00A4 /* Specific4 Top */
+#define GEM_EFTSH		0x00e8 /* PTP Event Frame Transmitted Seconds Register 47:32 */
+#define GEM_EFRSH		0x00ec /* PTP Event Frame Received Seconds Register 47:32 */
+#define GEM_PEFTSH		0x00f0 /* PTP Peer Event Frame Transmitted Seconds Register 47:32 */
+#define GEM_PEFRSH		0x00f4 /* PTP Peer Event Frame Received Seconds Register 47:32 */
 #define GEM_OTX			0x0100 /* Octets transmitted */
 #define GEM_OCTTXL		0x0100 /* Octets transmitted [31:0] */
 #define GEM_OCTTXH		0x0104 /* Octets transmitted [47:32] */
@@ -155,6 +166,9 @@
 #define GEM_DCFG6		0x0294 /* Design Config 6 */
 #define GEM_DCFG7		0x0298 /* Design Config 7 */
 
+#define GEM_TXBDCTRL	0x04cc /* TX Buffer Descriptor control register */
+#define GEM_RXBDCTRL	0x04d0 /* RX Buffer Descriptor control register */
+
 #define GEM_ISR(hw_q)		(0x0400 + ((hw_q) << 2))
 #define GEM_TBQP(hw_q)		(0x0440 + ((hw_q) << 2))
 #define GEM_TBQPH(hw_q)		(0x04C8)
@@ -191,6 +205,8 @@
 #define MACB_TZQ_OFFSET		12 /* Transmit zero quantum pause frame */
 #define MACB_TZQ_SIZE		1
 #define MACB_SRTSM_OFFSET	15
+#define MACB_OSSMODE_OFFSET 24 /* Enable One Step Synchro Mode */
+#define MACB_OSSMODE_SIZE	1
 
 /* Bitfields in NCFGR */
 #define MACB_SPD_OFFSET		0 /* Speed */
@@ -269,6 +285,10 @@
 #define GEM_RXBS_SIZE		8
 #define GEM_DDRP_OFFSET		24 /* disc_when_no_ahb */
 #define GEM_DDRP_SIZE		1
+#define GEM_RXEXT_OFFSET	28 /* RX extended Buffer Descriptor mode */
+#define GEM_RXEXT_SIZE		1
+#define GEM_TXEXT_OFFSET	29 /* TX extended Buffer Descriptor mode */
+#define GEM_TXEXT_SIZE		1
 #define GEM_ADDR64_OFFSET	30 /* Address bus width - 64b or 32b */
 #define GEM_ADDR64_SIZE		1
 
@@ -354,6 +374,16 @@
 #define MACB_PDRSFT_SIZE	1
 #define MACB_SRI_OFFSET		26 /* TSU Seconds Register Increment */
 #define MACB_SRI_SIZE		1
+#define MACB_TCI_OFFSET		29 /* TSU timer comparison interrupt */
+#define MACB_TCI_SIZE		1
+#define MACB_PTP_INT_MASK	(MACB_BIT(DRQFR) \
+							| MACB_BIT(SFR) \
+							| MACB_BIT(DRQFT) \
+							| MACB_BIT(SFT) \
+							| MACB_BIT(PDRQFR) \
+							| MACB_BIT(PDRSFR) \
+							| MACB_BIT(PDRQFT) \
+							| MACB_BIT(PDRSFT))
 
 /* Timer increment fields */
 #define MACB_TI_CNS_OFFSET	0
@@ -425,6 +455,11 @@
 #define GEM_TX_PKT_BUFF_OFFSET			21
 #define GEM_TX_PKT_BUFF_SIZE			1
 
+
+/* Bitfields in DCFG5. */
+#define GEM_TSU_OFFSET				8
+#define GEM_TSU_SIZE				1
+
 /* Bitfields in DCFG6. */
 #define GEM_PBUF_LSO_OFFSET			27
 #define GEM_PBUF_LSO_SIZE			1
@@ -439,6 +474,52 @@
 #define GEM_NSINCR_OFFSET			0
 #define GEM_NSINCR_SIZE				8
 
+/* Bitfields in TSH */
+#define GEM_TSH_OFFSET				0 /* TSU timer value (s). MSB [47:32] of seconds timer count */
+#define GEM_TSH_SIZE				16
+
+/* Bitfields in TSL */
+#define GEM_TSL_OFFSET				0 /* TSU timer value (s). LSB [31:0] of seconds timer count */
+#define GEM_TSL_SIZE				32
+
+/* Bitfields in TN */
+#define GEM_TN_OFFSET				0 /* TSU timer value (ns) */
+#define GEM_TN_SIZE					30
+
+/* Bitfields in TXBDCTRL */
+#define GEM_TXTSMODE_OFFSET			4 /* TX Descriptor Timestamp Insertion mode */
+#define GEM_TXTSMODE_SIZE			2
+
+/* Bitfields in RXBDCTRL */
+#define GEM_RXTSMODE_OFFSET			4 /* RX Descriptor Timestamp Insertion mode */
+#define GEM_RXTSMODE_SIZE			2
+
+/* Transmit DMA buffer descriptor Word 1 */
+#define GEM_DMA_TXVALID_OFFSET		23 /* timestamp has been captured in the Buffer Descriptor */
+#define GEM_DMA_TXVALID_SIZE		1
+
+/* Receive DMA buffer descriptor Word 0 */
+#define GEM_DMA_RXVALID_OFFSET		2 /* indicates a valid timestamp in the Buffer Descriptor */
+#define GEM_DMA_RXVALID_SIZE		1
+
+/* DMA buffer descriptor Word 2 (32 bit addressing) or Word 4 (64 bit addressing) */
+#define GEM_DMA_SECL_OFFSET			30 /* Timestamp seconds[1:0]  */
+#define GEM_DMA_SECL_SIZE			2
+#define GEM_DMA_NSEC_OFFSET			0 /* Timestamp nanosecs [29:0] */
+#define GEM_DMA_NSEC_SIZE			30
+
+/* DMA buffer descriptor Word 3 (32 bit addressing) or Word 5 (64 bit addressing) */
+
+/* New hardware supports 12 bit precision of timestamp in DMA buffer descriptor.
+ * Old hardware supports only 6 bit precision but it is enough for PTP.
+ * Less accuracy is used always instead of checking hardware version.
+ */
+#define GEM_DMA_SECH_OFFSET			0 /* Timestamp seconds[5:2] */
+#define GEM_DMA_SECH_SIZE			4
+#define GEM_DMA_SEC_WIDTH			(GEM_DMA_SECH_SIZE + GEM_DMA_SECL_SIZE)
+#define GEM_DMA_SEC_TOP				(1 << GEM_DMA_SEC_WIDTH)
+#define GEM_DMA_SEC_MASK			(GEM_DMA_SEC_TOP - 1)
+
 /* Bitfields in ADJ */
 #define GEM_ADDSUB_OFFSET			31
 #define GEM_ADDSUB_SIZE				1
@@ -514,6 +595,8 @@
 #define queue_readl(queue, reg)		(queue)->bp->macb_reg_readl((queue)->bp, (queue)->reg)
 #define queue_writel(queue, reg, value)	(queue)->bp->macb_reg_writel((queue)->bp, (queue)->reg, (value))
 
+#define PTP_TS_BUFFER_SIZE		128 /* must be power of 2 */
+
 /* Conditional GEM/MACB macros.  These perform the operation to the correct
  * register dependent on whether the device is a GEM or a MACB.  For registers
  * and bitfields that are common across both devices, use macb_{read,write}l
@@ -546,16 +629,21 @@ struct macb_dma_desc {
 	u32	ctrl;
 };
 
-#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
-enum macb_hw_dma_cap {
-	HW_DMA_CAP_32B,
-	HW_DMA_CAP_64B,
-};
+#ifdef MACB_EXT_DESC
+#define HW_DMA_CAP_32B		0
+#define HW_DMA_CAP_64B		(1 << 0)
+#define HW_DMA_CAP_PTP		(1 << 1)
+#define HW_DMA_CAP_64B_PTP	(HW_DMA_CAP_64B | HW_DMA_CAP_PTP)
 
 struct macb_dma_desc_64 {
 	u32 addrh;
 	u32 resvd;
 };
+
+struct macb_dma_desc_ptp {
+	u32	ts_1;
+	u32	ts_2;
+};
 #endif
 
 /* DMA descriptor bitfields */
@@ -871,6 +959,18 @@ struct macb_config {
 	int	jumbo_max_len;
 };
 
+#ifdef CONFIG_MACB_USE_HWSTAMP
+struct tsu_incr {
+	u32 sub_ns;
+	u32 ns;
+};
+
+struct gem_tx_ts {
+	struct sk_buff *skb;
+	struct macb_dma_desc_ptp desc_ptp;
+};
+#endif
+
 struct macb_queue {
 	struct macb		*bp;
 	int			irq;
@@ -887,6 +987,12 @@ struct macb_queue {
 	struct macb_tx_skb	*tx_skb;
 	dma_addr_t		tx_ring_dma;
 	struct work_struct	tx_error_task;
+
+#ifdef CONFIG_MACB_USE_HWSTAMP
+	struct work_struct	tx_ts_task;
+	unsigned int		tx_ts_head, tx_ts_tail;
+	struct gem_tx_ts	tx_timestamps[PTP_TS_BUFFER_SIZE];
+#endif
 };
 
 struct macb {
@@ -955,11 +1061,67 @@ struct macb {
 	u32			wol;
 
 	struct macb_ptp_info	*ptp_info;	/* macb-ptp interface */
-#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
-	enum macb_hw_dma_cap hw_dma_cap;
+#ifdef MACB_EXT_DESC
+	uint8_t hw_dma_cap;
+#endif
+
+#ifdef CONFIG_MACB_USE_HWSTAMP
+	bool ptp_hw_support;
+	spinlock_t tsu_clk_lock; /* gem tsu clock locking */
+	unsigned int tsu_rate;
+	struct ptp_clock *ptp_clock;
+	struct ptp_clock_info ptp_clock_info;
+	struct tsu_incr tsu_incr;
+	struct hwtstamp_config tstamp_config;
 #endif
 };
 
+#ifdef CONFIG_MACB_USE_HWSTAMP
+#define GEM_TSEC_SIZE  (GEM_TSH_SIZE + GEM_TSL_SIZE)
+#define TSU_SEC_MAX_VAL (((u64)1 << GEM_TSEC_SIZE) - 1)
+#define TSU_NSEC_MAX_VAL ((1 << GEM_TN_SIZE) - 1)
+
+enum macb_bd_control {
+	TSTAMP_DISABLED,
+	TSTAMP_FRAME_PTP_EVENT_ONLY,
+	TSTAMP_ALL_PTP_FRAMES,
+	TSTAMP_ALL_FRAMES,
+};
+
+void gem_ptp_init(struct net_device *ndev);
+void gem_ptp_remove(struct net_device *ndev);
+int gem_ptp_txstamp(struct macb_queue *queue, struct sk_buff *skb, struct macb_dma_desc *des);
+void gem_ptp_rxstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc);
+void macb_ptp_int(struct macb_queue *queue, u32 status);
+static inline int gem_ptp_do_txstamp(struct macb_queue *queue, struct sk_buff *skb, struct macb_dma_desc *desc)
+{
+	if (queue->bp->tstamp_config.tx_type == TSTAMP_DISABLED)
+		return -ENOTSUPP;
+
+	return gem_ptp_txstamp(queue, skb, desc);
+}
+
+static inline void gem_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc)
+{
+	if (bp->tstamp_config.rx_filter == TSTAMP_DISABLED)
+		return;
+
+	gem_ptp_rxstamp(bp, skb, desc);
+}
+int gem_get_hwtst(struct net_device *dev, struct ifreq *rq);
+int gem_set_hwtst(struct net_device *dev, struct ifreq *ifr, int cmd);
+#else
+static inline void gem_ptp_init(struct net_device *ndev) { }
+static inline void gem_ptp_remove(struct net_device *ndev) { }
+
+static inline int gem_ptp_do_txstamp(struct macb_queue *queue, struct sk_buff *skb, struct macb_dma_desc *desc)
+{
+	return 0;
+}
+
+static inline void gem_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc) { }
+#endif
+
 static inline bool macb_is_gem(struct macb *bp)
 {
 	return !!(bp->caps & MACB_CAPS_MACB_IS_GEM);
diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
new file mode 100755
index 0000000..72a79c4
--- /dev/null
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -0,0 +1,724 @@
+/**
+ * 1588 PTP support for Cadence GEM device.
+ *
+ * Copyright (C) 2017 Cadence Design Systems - http://www.cadence.com
+ *
+ * Authors: Rafal Ozieblo <rafalo@cadence.com>
+ *          Bartosz Folta <bfolta@cadence.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2  of
+ * the License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/etherdevice.h>
+#include <linux/platform_device.h>
+#include <linux/time64.h>
+#include <linux/ptp_classify.h>
+#include <linux/if_ether.h>
+#include <linux/if_vlan.h>
+#include <linux/net_tstamp.h>
+#include <linux/circ_buf.h>
+#include <linux/spinlock.h>
+
+#include "macb.h"
+
+#define  GEM_PTP_TIMER_NAME "gem-ptp-timer"
+
+static struct macb_dma_desc_ptp *macb_ptp_desc(struct macb *bp,
+		struct macb_dma_desc *desc)
+{
+	if (bp->hw_dma_cap == HW_DMA_CAP_PTP)
+		return (struct macb_dma_desc_ptp *)
+				((u8 *)desc + sizeof(struct macb_dma_desc));
+	if (bp->hw_dma_cap == HW_DMA_CAP_64B_PTP)
+		return (struct macb_dma_desc_ptp *)
+				((u8 *)desc + sizeof(struct macb_dma_desc)
+				+ sizeof(struct macb_dma_desc_64));
+	return NULL;
+}
+
+static int gem_tsu_get_time(struct macb *bp, struct timespec64 *ts)
+{
+	long first, second;
+	u32 secl, sech;
+	unsigned long flags;
+
+	if (!bp || !ts)
+		return -EINVAL;
+
+	spin_lock_irqsave(&bp->tsu_clk_lock, flags);
+	first = gem_readl(bp, TN);
+	secl = gem_readl(bp, TSL);
+	sech = gem_readl(bp, TSH);
+	second = gem_readl(bp, TN);
+
+	/* test for nsec rollover */
+	if (first > second) {
+		/* if so, use later read & re-read seconds
+		 * (assume all done within 1s)
+		 */
+		ts->tv_nsec = gem_readl(bp, TN);
+		secl = gem_readl(bp, TSL);
+		sech = gem_readl(bp, TSH);
+	} else
+		ts->tv_nsec = first;
+
+	ts->tv_sec = (((u64)sech << GEM_TSL_SIZE) | secl)
+			& TSU_SEC_MAX_VAL;
+
+	spin_unlock_irqrestore(&bp->tsu_clk_lock, flags);
+	return 0;
+}
+
+static int gem_tsu_set_time(struct macb *bp, const struct timespec64 *ts)
+{
+	u32 ns, sech, secl;
+	unsigned long flags;
+
+	if (!bp || !ts)
+		return -EINVAL;
+
+	secl = (u32)ts->tv_sec;
+	sech = (ts->tv_sec >> GEM_TSL_SIZE) & ((1 << GEM_TSH_SIZE) - 1);
+	ns = ts->tv_nsec;
+
+	spin_lock_irqsave(&bp->tsu_clk_lock, flags);
+
+	/* TSH doesn't latch the time and no atomicity! */
+	gem_writel(bp, TN, 0); /* clear to avoid overflow */
+	gem_writel(bp, TSH, sech);
+	/* write lower bits 2nd, for synchronized secs update */
+	gem_writel(bp, TSL, secl);
+	gem_writel(bp, TN, ns);
+
+	spin_unlock_irqrestore(&bp->tsu_clk_lock, flags);
+
+	return 0;
+}
+
+static int gem_tsu_incr_set(struct macb *bp, struct tsu_incr *incr_spec)
+{
+	unsigned long flags;
+
+	if (!bp || !incr_spec)
+		return -EINVAL;
+
+	/* tsu_timer_incr register must be written after
+	 * the tsu_timer_incr_sub_ns register and the write operation
+	 * will cause the value written to the tsu_timer_incr_sub_ns register
+	 * to take effect.
+	 */
+	spin_lock_irqsave(&bp->tsu_clk_lock, flags);
+	gem_writel(bp, TISUBN, GEM_BF(SUBNSINCR, incr_spec->sub_ns));
+	gem_writel(bp, TI, GEM_BF(NSINCR, incr_spec->ns));
+	spin_unlock_irqrestore(&bp->tsu_clk_lock, flags);
+
+	return 0;
+}
+
+static int gem_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
+{
+	struct tsu_incr incr_spec;
+	struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);
+	u64 adj;
+	u32 word;
+	bool neg_adj = false;
+
+	if (!ptp)
+		return -EINVAL;
+
+	if (scaled_ppm < 0) {
+		neg_adj = true;
+		scaled_ppm = -scaled_ppm;
+	}
+
+	/* Adjustment is relative to base frequency */
+	incr_spec.sub_ns = bp->tsu_incr.sub_ns;
+	incr_spec.ns = bp->tsu_incr.ns;
+
+	/* scaling: unused(8bit) | ns(8bit) | fractions(16bit) */
+	word = ((u64)incr_spec.ns << GEM_SUBNSINCR_SIZE) + incr_spec.sub_ns;
+	adj = (u64)scaled_ppm * word;
+	/* Divide with rounding, equivalent to floating dividing:
+	 * (temp / USEC_PER_SEC) + 0.5
+	 */
+	adj += (USEC_PER_SEC >> 1);
+	adj >>= GEM_SUBNSINCR_SIZE; /* remove fractions */
+	adj = div_u64(adj, USEC_PER_SEC);
+	adj = neg_adj ? (word - adj) : (word + adj);
+
+	incr_spec.ns = (adj >> GEM_SUBNSINCR_SIZE)
+			& ((1 << GEM_NSINCR_SIZE) - 1);
+	incr_spec.sub_ns = adj & ((1 << GEM_SUBNSINCR_SIZE) - 1);
+	gem_tsu_incr_set(bp, &incr_spec);
+	return 0;
+}
+
+static int gem_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+	struct timespec64 now, then = ns_to_timespec64(delta);
+	struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);
+	u32 adj, sign = 0;
+
+	if (!ptp)
+		return -EINVAL;
+
+	if (delta < 0) {
+		sign = 1;
+		delta = -delta;
+	}
+
+	if (delta > TSU_NSEC_MAX_VAL) {
+		gem_tsu_get_time(bp, &now);
+		if (sign)
+			now = timespec64_sub(now, then);
+		else
+			now = timespec64_add(now, then);
+
+		gem_tsu_set_time(bp, (const struct timespec64 *)&now);
+	} else {
+		adj = (sign << GEM_ADDSUB_OFFSET) | delta;
+
+		gem_writel(bp, TA, adj);
+	}
+
+	return 0;
+}
+
+static int gem_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
+{
+	struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);
+
+	if (!ptp || !ts)
+		return -EINVAL;
+
+	gem_tsu_get_time(bp, ts);
+	return 0;
+}
+
+static int gem_ptp_settime(struct ptp_clock_info *ptp,
+		const struct timespec64 *ts)
+{
+	struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);
+
+	if (!ptp || !ts)
+		return -EINVAL;
+
+	gem_tsu_set_time(bp, ts);
+	return 0;
+}
+
+static int gem_ptp_enable(struct ptp_clock_info *ptp,
+			  struct ptp_clock_request *rq, int on)
+{
+	struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);
+
+	if (!ptp || !rq)
+		return -EINVAL;
+
+	switch (rq->type) {
+	case PTP_CLK_REQ_EXTTS:	/* Toggle TSU match interrupt */
+		if (on)
+			macb_writel(bp, IER, MACB_BIT(TCI));
+		else
+			macb_writel(bp, IDR, MACB_BIT(TCI));
+		break;
+	case PTP_CLK_REQ_PEROUT: /* Toggle Periodic output */
+		return -EOPNOTSUPP;
+		/* break; */
+	case PTP_CLK_REQ_PPS:	/* Toggle TSU periodic (second) interrupt */
+		if (on)
+			macb_writel(bp, IER, MACB_BIT(SRI));
+		else
+			macb_writel(bp, IDR, MACB_BIT(SRI));
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static struct ptp_clock_info gem_ptp_caps_template = {
+	.owner		= THIS_MODULE,
+	.name		= GEM_PTP_TIMER_NAME,
+	.max_adj	= 0,
+	.n_alarm	= 1,
+	.n_ext_ts	= 1,
+	.n_per_out	= 0,
+	.n_pins		= 0,
+	.pps		= 1,
+	.adjfine	= gem_ptp_adjfine,
+	.adjtime	= gem_ptp_adjtime,
+	.gettime64	= gem_ptp_gettime,
+	.settime64	= gem_ptp_settime,
+	.enable		= gem_ptp_enable,
+};
+
+static void gem_ptp_init_timer(struct macb *bp)
+{
+	u32 rem = 0;
+
+	bp->tsu_incr.ns = div_u64_rem(NSEC_PER_SEC, bp->tsu_rate, &rem);
+	if (rem) {
+		u64 adj = rem;
+
+		adj <<= GEM_SUBNSINCR_SIZE;
+		bp->tsu_incr.sub_ns = div_u64(adj, bp->tsu_rate);
+	} else {
+		bp->tsu_incr.sub_ns = 0;
+	}
+}
+
+static void gem_ptp_init_tsu(struct macb *bp)
+{
+	struct timespec64 ts;
+
+	/* 1. get current system time */
+	ts = ns_to_timespec64(ktime_to_ns(ktime_get_real()));
+
+	/* 2. set ptp timer */
+	gem_tsu_set_time(bp, &ts);
+
+	/* 3. set PTP timer increment value to BASE_INCREMENT */
+	gem_tsu_incr_set(bp, &bp->tsu_incr);
+
+	gem_writel(bp, TA, 0);
+}
+
+static void gem_ptp_clear_timer(struct macb *bp)
+{
+	bp->tsu_incr.ns = 0;
+	bp->tsu_incr.sub_ns = 0;
+
+	gem_writel(bp, TISUBN, GEM_BF(SUBNSINCR, 0));
+	gem_writel(bp, TI, GEM_BF(NSINCR, 0));
+	gem_writel(bp, TA, 0);
+}
+
+static int gem_hw_timestamp(struct macb *bp,
+		u32 dma_desc_ts_1, u32 dma_desc_ts_2, struct timespec64 *ts)
+{
+	struct timespec64 tsu;
+
+	ts->tv_sec = (GEM_BFEXT(DMA_SECH, dma_desc_ts_2) << GEM_DMA_SECL_SIZE) |
+			GEM_BFEXT(DMA_SECL, dma_desc_ts_1);
+	ts->tv_nsec = GEM_BFEXT(DMA_NSEC, dma_desc_ts_1);
+
+	/* TSU overlaping workaround
+	 * The timestamp only contains lower few bits of seconds,
+	 * so add value from 1588 timer
+	 */
+	gem_tsu_get_time(bp, &tsu);
+
+	/* If the top bit is set in the timestamp,
+	 * but not in 1588 timer, it has rolled over,
+	 * so subtract max size
+	 */
+	if ((ts->tv_sec & (GEM_DMA_SEC_TOP >> 1)) &&
+		!(tsu.tv_sec & (GEM_DMA_SEC_TOP >> 1)))
+		ts->tv_sec -= GEM_DMA_SEC_TOP;
+
+	ts->tv_sec += ((~GEM_DMA_SEC_MASK) & (tsu.tv_sec));
+
+	return 0;
+}
+
+void gem_ptp_rxstamp(struct macb *bp, struct sk_buff *skb,
+		struct macb_dma_desc *desc)
+{
+	struct timespec64 ts;
+	struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
+	struct macb_dma_desc_ptp *desc_ptp;
+
+	if (GEM_BFEXT(DMA_RXVALID, desc->addr)) {
+		desc_ptp = macb_ptp_desc(bp, desc);
+		gem_hw_timestamp(bp, desc_ptp->ts_1, desc_ptp->ts_2, &ts);
+		memset(shhwtstamps, 0, sizeof(struct skb_shared_hwtstamps));
+		shhwtstamps->hwtstamp = ktime_set(ts.tv_sec, ts.tv_nsec);
+	}
+}
+
+static void gem_tstamp_tx(struct macb *bp, struct sk_buff *skb,
+		struct macb_dma_desc_ptp *desc_ptp)
+{
+	struct skb_shared_hwtstamps shhwtstamps;
+	struct timespec64 ts;
+
+	gem_hw_timestamp(bp, desc_ptp->ts_1, desc_ptp->ts_2, &ts);
+	memset(&shhwtstamps, 0, sizeof(shhwtstamps));
+	shhwtstamps.hwtstamp = ktime_set(ts.tv_sec, ts.tv_nsec);
+	skb_tstamp_tx(skb, &shhwtstamps);
+}
+
+int gem_ptp_txstamp(struct macb_queue *queue, struct sk_buff *skb,
+		struct macb_dma_desc *desc)
+{
+	struct gem_tx_ts *tx_timestamp;
+	struct macb_dma_desc_ptp *desc_ptp;
+	unsigned long head = queue->tx_ts_head;
+	unsigned long tail = READ_ONCE(queue->tx_ts_tail);
+
+	if (!GEM_BFEXT(DMA_TXVALID, desc->ctrl))
+		return -EINVAL;
+
+	if (CIRC_SPACE(head, tail, PTP_TS_BUFFER_SIZE) == 0)
+		return -ENOMEM;
+
+	desc_ptp = macb_ptp_desc(queue->bp, desc);
+	tx_timestamp = &queue->tx_timestamps[head];
+	tx_timestamp->skb = skb;
+	tx_timestamp->desc_ptp.ts_1 = desc_ptp->ts_1;
+	tx_timestamp->desc_ptp.ts_2 = desc_ptp->ts_2;
+	/* move head */
+	smp_store_release(&queue->tx_ts_head,
+			(head + 1) & (PTP_TS_BUFFER_SIZE - 1));
+	return 0;
+}
+
+static void gem_tx_timestamp_flush(struct work_struct *work)
+{
+	struct macb_queue *queue =
+			container_of(work, struct macb_queue, tx_ts_task);
+	struct gem_tx_ts *tx_ts;
+	unsigned long head, tail;
+
+	/* take current head */
+	head = smp_load_acquire(&queue->tx_ts_head);
+	tail = queue->tx_ts_tail;
+
+	while (CIRC_CNT(head, tail, PTP_TS_BUFFER_SIZE)) {
+		tx_ts = &queue->tx_timestamps[tail];
+		gem_tstamp_tx(queue->bp, tx_ts->skb, &tx_ts->desc_ptp);
+		/* cleanup */
+		dev_kfree_skb_any(tx_ts->skb);
+		/* remove old tail */
+		smp_store_release(&queue->tx_ts_tail,
+				(tail + 1) & (PTP_TS_BUFFER_SIZE - 1));
+		tail = queue->tx_ts_tail;
+	}
+}
+
+void gem_ptp_init(struct net_device *dev)
+{
+	struct macb *bp = netdev_priv(dev);
+	unsigned int q;
+	struct macb_queue *queue;
+
+	bp->ptp_clock_info = gem_ptp_caps_template;
+
+	/* nominal frequency and maximum adjustment in ppb */
+	bp->tsu_rate = bp->ptp_info->get_tsu_rate(bp);
+	bp->ptp_clock_info.max_adj = bp->ptp_info->get_ptp_max_adj();
+	gem_ptp_init_timer(bp);
+	bp->ptp_clock = ptp_clock_register(&bp->ptp_clock_info, &dev->dev);
+	if (IS_ERR(&bp->ptp_clock)) {
+		bp->ptp_clock = NULL;
+		pr_err("ptp clock register failed\n");
+		return;
+	}
+
+	spin_lock_init(&bp->tsu_clk_lock);
+	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
+		queue->tx_ts_head = 0;
+		queue->tx_ts_tail = 0;
+		INIT_WORK(&queue->tx_ts_task, gem_tx_timestamp_flush);
+		queue_writel(queue, IER, MACB_PTP_INT_MASK);
+	}
+
+	gem_ptp_init_tsu(bp);
+
+	dev_info(&bp->pdev->dev, "%s ptp clock registered.\n",
+		 GEM_PTP_TIMER_NAME);
+}
+
+void gem_ptp_remove(struct net_device *ndev)
+{
+	struct macb *bp = netdev_priv(ndev);
+
+	if (bp->ptp_clock)
+		ptp_clock_unregister(bp->ptp_clock);
+
+	gem_ptp_clear_timer(bp);
+
+	dev_info(&bp->pdev->dev, "%s ptp clock unregistered.\n",
+		 GEM_PTP_TIMER_NAME);
+}
+
+static int gem_ptp_set_ts_mode(struct macb *bp,
+			     enum macb_bd_control tx_bd_control,
+			     enum macb_bd_control rx_bd_control)
+{
+	if (!bp)
+		return -EINVAL;
+
+	gem_writel(bp, TXBDCTRL, GEM_BF(TXTSMODE, tx_bd_control));
+	gem_writel(bp, RXBDCTRL, GEM_BF(RXTSMODE, rx_bd_control));
+
+	return 0;
+}
+
+int gem_get_hwtst(struct net_device *dev, struct ifreq *rq)
+{
+	struct macb *bp = netdev_priv(dev);
+	struct hwtstamp_config *tstamp_config = &bp->tstamp_config;
+
+	if (!bp->ptp_hw_support)
+		return -EFAULT;
+
+	if (copy_to_user(rq->ifr_data, tstamp_config, sizeof(*tstamp_config)))
+		return -EFAULT;
+	else
+		return 0;
+}
+
+static int gem_ptp_set_one_step_sync(struct macb *bp, u8 enable)
+{
+	u32 reg_val;
+
+	if (!bp || enable > 1)
+		return -EINVAL;
+
+	reg_val = macb_readl(bp, NCR);
+
+	if (enable)
+		macb_writel(bp, NCR, reg_val | MACB_BIT(OSSMODE));
+	else
+		macb_writel(bp, NCR, reg_val & ~MACB_BIT(OSSMODE));
+
+	return 0;
+}
+
+int gem_set_hwtst(struct net_device *dev, struct ifreq *ifr, int cmd)
+{
+	struct macb *bp = netdev_priv(dev);
+	struct hwtstamp_config *tstamp_config = &bp->tstamp_config;
+	enum macb_bd_control tx_bd_control = TSTAMP_DISABLED;
+	enum macb_bd_control rx_bd_control = TSTAMP_DISABLED;
+	u32 regval;
+
+	if (!bp->ptp_hw_support)
+		return -EFAULT;
+
+	if (copy_from_user(tstamp_config, ifr->ifr_data,
+			sizeof(*tstamp_config)))
+		return -EFAULT;
+
+	/* reserved for future extensions */
+	if (tstamp_config->flags)
+		return -EINVAL;
+
+	switch (tstamp_config->tx_type) {
+	case HWTSTAMP_TX_OFF:
+		break;
+	case HWTSTAMP_TX_ONESTEP_SYNC:
+		if (gem_ptp_set_one_step_sync(bp, 1) != 0)
+			return -ERANGE;
+	case HWTSTAMP_TX_ON:
+		tx_bd_control = TSTAMP_ALL_FRAMES;
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	switch (tstamp_config->rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		break;
+	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
+		break;
+	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
+	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
+		rx_bd_control =  TSTAMP_ALL_PTP_FRAMES;
+		tstamp_config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
+		regval = macb_readl(bp, NCR);
+		macb_writel(bp, NCR, (regval | MACB_BIT(SRTSM)));
+		break;
+	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
+	case HWTSTAMP_FILTER_ALL:
+		rx_bd_control = TSTAMP_ALL_FRAMES;
+		tstamp_config->rx_filter = HWTSTAMP_FILTER_ALL;
+		break;
+	default:
+		tstamp_config->rx_filter = HWTSTAMP_FILTER_NONE;
+		return -ERANGE;
+	}
+
+	if (gem_ptp_set_ts_mode(bp, tx_bd_control, rx_bd_control) != 0)
+		return -ERANGE;
+
+	if (copy_to_user(ifr->ifr_data, tstamp_config, sizeof(*tstamp_config)))
+		return -EFAULT;
+	else
+		return 0;
+}
+
+static int gem_ptp_time_peer_frame_tx_get(struct macb *bp,
+		struct timespec64 *ts)
+{
+	if (!bp || !ts)
+		return -EINVAL;
+
+	ts->tv_sec = (((u64)gem_readl(bp, PEFTSH) << 32) |
+		gem_readl(bp, PEFTSL)) & TSU_SEC_MAX_VAL;
+	ts->tv_nsec = gem_readl(bp, PEFTN);
+
+	return 0;
+}
+
+static int gem_ptp_time_peer_frame_rx_get(struct macb *bp,
+		struct timespec64 *ts)
+{
+	if (!bp || !ts)
+		return -EINVAL;
+
+	ts->tv_sec = (((u64)gem_readl(bp, PEFRSH) << 32) |
+		gem_readl(bp, PEFRSL)) & TSU_SEC_MAX_VAL;
+	ts->tv_nsec = gem_readl(bp, PEFRN);
+
+	return 0;
+}
+
+static int gem_ptp_time_frame_tx_get(struct macb *bp, struct timespec64 *ts)
+{
+	if (!bp || !ts)
+		return -EINVAL;
+
+	ts->tv_sec = (((u64)gem_readl(bp, EFTSH) << 32) |
+		gem_readl(bp, EFTSL)) & TSU_SEC_MAX_VAL;
+	ts->tv_nsec = gem_readl(bp, EFTN);
+
+	return 0;
+}
+
+static int gem_ptp_time_frame_rx_get(struct macb *bp, struct timespec64 *ts)
+{
+	if (!bp || !ts)
+		return -EINVAL;
+
+	ts->tv_sec = (((u64)gem_readl(bp, EFRSH) << 32) |
+		      gem_readl(bp, EFRSL)) & TSU_SEC_MAX_VAL;
+	ts->tv_nsec = gem_readl(bp, EFRN);
+
+	return 0;
+}
+
+static int gem_ptp_event(struct macb *bp, struct timespec64 *ts)
+{
+	struct ptp_clock_event event;
+
+	event.type = PTP_CLOCK_EXTTS;
+	event.index = 0;
+	event.timestamp = ts->tv_sec * NSEC_PER_SEC + ts->tv_nsec;
+
+	ptp_clock_event(bp->ptp_clock, &event);
+
+	return 0;
+}
+
+void macb_ptp_int(struct macb_queue *queue, u32 status)
+{
+	struct macb *bp = queue->bp;
+	struct timespec64 ts;
+
+	if (status & MACB_BIT(DRQFR)) {
+		if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+			queue_writel(queue, ISR, MACB_BIT(DRQFR));
+		if (gem_ptp_time_frame_rx_get(bp, &ts) != 0) {
+			ts.tv_sec = 0;
+			ts.tv_nsec = 0;
+		}
+		gem_ptp_event(bp, &ts);
+	}
+
+	if (status & MACB_BIT(SFR)) {
+		if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+			queue_writel(queue, ISR, MACB_BIT(SFR));
+		if (gem_ptp_time_frame_rx_get(bp, &ts) != 0) {
+			ts.tv_sec = 0;
+			ts.tv_nsec = 0;
+		}
+		gem_ptp_event(bp, &ts);
+	}
+
+	if (status & MACB_BIT(DRQFT)) {
+		if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+			queue_writel(queue, ISR, MACB_BIT(DRQFT));
+		if (gem_ptp_time_frame_tx_get(bp, &ts) != 0) {
+			ts.tv_sec = 0;
+			ts.tv_nsec = 0;
+		}
+		gem_ptp_event(bp, &ts);
+	}
+
+	if (status & MACB_BIT(SFT)) {
+		if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+			queue_writel(queue, ISR, MACB_BIT(SFT));
+		if (gem_ptp_time_frame_tx_get(bp, &ts) != 0) {
+			ts.tv_sec = 0;
+			ts.tv_nsec = 0;
+		}
+		gem_ptp_event(bp, &ts);
+	}
+
+	if (status & MACB_BIT(PDRQFR)) {
+		if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+			queue_writel(queue, ISR, MACB_BIT(PDRQFR));
+		if (gem_ptp_time_peer_frame_rx_get(bp, &ts) != 0) {
+			ts.tv_sec = 0;
+			ts.tv_nsec = 0;
+		}
+		gem_ptp_event(bp, &ts);
+	}
+
+	if (status & MACB_BIT(PDRSFR)) {
+		if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+			queue_writel(queue, ISR, MACB_BIT(PDRSFR));
+		if (gem_ptp_time_peer_frame_rx_get(bp, &ts) != 0) {
+			ts.tv_sec = 0;
+			ts.tv_nsec = 0;
+		}
+		gem_ptp_event(bp, &ts);
+	}
+
+	if (status & MACB_BIT(PDRQFT)) {
+		if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+			queue_writel(queue, ISR,
+					MACB_BIT(PDRQFT));
+		if (gem_ptp_time_peer_frame_tx_get(bp, &ts) != 0) {
+			ts.tv_sec = 0;
+			ts.tv_nsec = 0;
+		}
+		gem_ptp_event(bp, &ts);
+	}
+
+	if (status & MACB_BIT(PDRSFT)) {
+		if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+			queue_writel(queue, ISR,
+					MACB_BIT(PDRSFT));
+		if (gem_ptp_time_peer_frame_tx_get(bp, &ts) != 0) {
+			ts.tv_sec = 0;
+			ts.tv_nsec = 0;
+		}
+		gem_ptp_event(bp, &ts);
+	}
+}
-- 
2.4.5

^ permalink raw reply related

* [PATCH] i2c: add sc18is600 driver
From: Sebastian Reichel @ 2017-03-29 14:03 UTC (permalink / raw)
  To: Sebastian Reichel, Wolfram Sang
  Cc: Rob Herring, Mark Rutland, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

This adds an I²C master driver for SPI -> I²C bus bridge chips.
It currently supports NXP's SC18IS600 and SC18IS601, as well as
Silicon Labs' CP2120. The driver was only tested on SC18IS600.

Signed-off-By: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 .../devicetree/bindings/i2c/i2c-cp2120.txt         |   1 +
 .../devicetree/bindings/i2c/i2c-sc18is600.txt      |  62 +++
 drivers/i2c/busses/Kconfig                         |  10 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-sc18is600.c                 | 572 +++++++++++++++++++++
 5 files changed, 646 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-cp2120.txt
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-sc18is600.txt
 create mode 100644 drivers/i2c/busses/i2c-sc18is600.c

diff --git a/Documentation/devicetree/bindings/i2c/i2c-cp2120.txt b/Documentation/devicetree/bindings/i2c/i2c-cp2120.txt
new file mode 100644
index 000000000000..95e06e74f288
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-cp2120.txt
@@ -0,0 +1 @@
+Please see binding for i2c-sc18is600
diff --git a/Documentation/devicetree/bindings/i2c/i2c-sc18is600.txt b/Documentation/devicetree/bindings/i2c/i2c-sc18is600.txt
new file mode 100644
index 000000000000..d0d9e680a5d6
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-sc18is600.txt
@@ -0,0 +1,62 @@
+NXP SC18IS600 and Silabs CP2120 - SPI to I2C bus bridge
+
+NXP SC18IS600 is a SPI slave chip, which implements an I²C host,
+also known as SPI to I²C bus bridge. SC18IS601 is the same chip,
+but has an external clock input instead of using a builtin
+oscillator. CP2120 is a similar chip from Silabs, which implements
+the same interface as NXP's SC18IS600.
+
+Required properties:
+  - compatible: Should contain one of
+      * "nxp,sc18is600"
+      * "nxp,sc18is601"
+      * "silabs,cp2120"
+  - reg: address of the chip on SPI bus
+  - interrupts: Interrupt specifier. Refer to interrupt bindings.
+  - #address-cells: Should be 1.
+  - #size-cells: Should be 0.
+
+Required properties for sc18is601:
+  - clkin: Clock specifier for CLKIN pin
+
+Optional properties:
+  - clock-frequency:
+    Desired I2C bus frequency in Hz, otherwise defaults to 100 KHz
+  - reset-gpios
+    GPIO specifier for reset pin, which is active low.
+  - vdd-supply
+    Regulator specifier for VDD supply (3.3V).
+  - Child nodes conforming to i2c bus binding
+
+Example:
+
+&spi_controller {
+	sc18is600: i2c@0 {
+		compatible = "nxp,sc18is600";
+		spi-max-frequency = <700000>; /* 700KHz */
+		spi-cpol;
+		spi-cpha;
+		reg = <0>;
+
+		vdd-supply = <&regulator_v33>;
+
+		interrupt-parent = <&socgpio>;
+		interrupts = <25 0x2>;
+
+		reset-gpios = <&i2cgpio1 9 GPIO_ACTIVE_LOW>;
+
+		clock-frequency = <100000>; /* 100KHz */
+
+		#address-cells = <0x1>;
+		#size-cells = <0x0>;
+	};
+};
+
+&sc18is600 {
+	i2c_device@42 {
+		compatible = "some,i2c-device";
+		reg = <0x42>;
+	};
+
+	...
+};
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 8adc0f1d7ad0..3e6386ff8de3 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -210,6 +210,16 @@ config I2C_NFORCE2_S4985
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-nforce2-s4985.
 
+config I2C_SC18IS600
+	tristate "NXP SC18IS600"
+	depends on SPI && REGMAP
+	help
+	  If you say yes to this option, support will be included for the
+	  NXP SC18IS600 SPI to I2C-bus interface.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called i2c-sc18is600.
+
 config I2C_SIS5595
 	tristate "SiS 5595"
 	depends on PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 30b60855fbcd..29971aebd238 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_I2C_ISMT)		+= i2c-ismt.o
 obj-$(CONFIG_I2C_NFORCE2)	+= i2c-nforce2.o
 obj-$(CONFIG_I2C_NFORCE2_S4985)	+= i2c-nforce2-s4985.o
 obj-$(CONFIG_I2C_PIIX4)		+= i2c-piix4.o
+obj-$(CONFIG_I2C_SC18IS600)	+= i2c-sc18is600.o
 obj-$(CONFIG_I2C_SIS5595)	+= i2c-sis5595.o
 obj-$(CONFIG_I2C_SIS630)	+= i2c-sis630.o
 obj-$(CONFIG_I2C_SIS96X)	+= i2c-sis96x.o
diff --git a/drivers/i2c/busses/i2c-sc18is600.c b/drivers/i2c/busses/i2c-sc18is600.c
new file mode 100644
index 000000000000..e4d4b3caf3a9
--- /dev/null
+++ b/drivers/i2c/busses/i2c-sc18is600.c
@@ -0,0 +1,572 @@
+/*
+ * NXP SC18IS600 SPI to I2C bus interface driver
+ *
+ * Copyright (C) 2017 Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * Datasheets:
+ *  - http://www.nxp.com/documents/data_sheet/SC18IS600.pdf
+ *  - https://www.silabs.com/documents/public/data-sheets/CP2120.pdf
+ */
+
+#include <linux/clk.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#define SC18IS600_I2C_PM_TIMEOUT 1000 /* ms */
+#define SC18IS600_DEFAULT_FREQ 100000
+
+#define SC18IS600_CMD_WR	0x00 /* write */
+#define SC18IS600_CMD_RD	0x01 /* read */
+#define SC18IS600_CMD_WR_RD	0x02 /* read after write */
+#define SC18IS600_CMD_WR_WR	0x03 /* write after write */
+#define SC18IS600_CMD_RDBUF	0x06 /* read buffer */
+#define CP2120_CMD_WRMULTI	0x09 /* write to multiple slaves */
+#define SC18IS600_CMD_SPICON	0x18 /* spi endianess configuration */
+#define SC18IS600_CMD_REG_WR	0x20 /* write register */
+#define SC18IS600_CMD_REG_RD	0x21 /* read register */
+#define SC18IS600_CMD_PWRDWN	0x30 /* power down */
+#define CP2120_CMD_REVISION	0x40 /* read revision */
+
+#define SC18IS600_REG_IO_CONFIG		0x00
+#define SC18IS600_REG_IO_STATE		0x01
+#define SC18IS600_REG_I2C_CLOCK		0x02
+#define SC18IS600_REG_I2C_TIMEOUT	0x03
+#define SC18IS600_REG_I2C_STAT		0x04
+#define SC18IS600_REG_I2C_ADDR		0x05
+#define SC18IS600_REG_I2C_BUFFER	0x06 /* only cp2120 */
+#define SC18IS600_REG_IO_CONFIG2	0x07 /* only cp2120 */
+#define SC18IS600_REG_EDGEINT		0x08 /* only cp2120 */
+#define SC18IS600_REG_I2C_TIMEOUT2	0x09 /* only cp2120 */
+
+#define SC18IS600_STAT_OK		0xF0
+#define SC18IS600_STAT_NAK_ADDR		0xF1
+#define SC18IS600_STAT_NAK_DATA		0xF2
+#define SC18IS600_STAT_BUSY		0xF3
+#define SC18IS600_STAT_TIMEOUT		0xF8
+#define SC18IS600_STAT_SIZE		0xF9
+#define SC18IS600_STAT_TIMEOUT2		0xFA /* only cp2120 */
+#define SC18IS600_STAT_BLOCKED		0xFB /* only cp2120 */
+
+#define CMD_BUFFER_SIZE 5
+
+enum chiptype {
+	SPI2I2C_SC18IS600,
+	SPI2I2C_SC18IS601,
+	SPI2I2C_CP2120,
+};
+
+struct chipdesc {
+	u8  type;
+	u32 max_spi_speed;
+	u32 buffer_size;
+	u32 clock_base;
+	u32 timeout_base;
+	const struct regmap_config *regmap_cfg;
+};
+
+static bool sc18is600_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case SC18IS600_REG_I2C_STAT:
+	case SC18IS600_REG_I2C_BUFFER:
+		return false;
+	default:
+		return true;
+	}
+}
+
+static const struct regmap_config sc18is600_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = 0x05,
+	.writeable_reg = sc18is600_writeable_reg,
+};
+
+static const struct regmap_config cp2120_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = 0x09,
+	.writeable_reg = sc18is600_writeable_reg,
+};
+
+/*
+ * Note: The sc18is600's datasheet promises 1.2MHz SPI support, but my chip did
+ * not behave correctly at that speed. It received the bytes correctly, but
+ * just sent them back instead of interpreting them correctly. At 800 KHz I
+ * still got a few errors (about 1%) and at 700 KHz everything works smoothly.
+ */
+static const struct chipdesc chip_sc18is600 = {
+	.type = SPI2I2C_SC18IS600,
+	.max_spi_speed = 700000,
+	.buffer_size = 96,
+	.clock_base = 1843200,
+	.buffer_size = 96,
+	.clock_base = 1843200,
+	.timeout_base = 1125, /* 112.5 Hz */
+	.regmap_cfg = &sc18is600_regmap_config,
+};
+
+static const struct chipdesc chip_sc18is601 = {
+	.type = SPI2I2C_SC18IS601,
+	.max_spi_speed = 3000000,
+	.buffer_size = 96,
+	.clock_base = 0,
+	.timeout_base = 1125, /* 112.5 Hz */
+	.regmap_cfg = &sc18is600_regmap_config,
+};
+
+static const struct chipdesc chip_cp2120 = {
+	.type = SPI2I2C_CP2120,
+	.max_spi_speed = 1000000,
+	.buffer_size = 255,
+	.clock_base = 2000000,
+	.timeout_base = 1280, /* 128 Hz */
+	.regmap_cfg = &cp2120_regmap_config,
+};
+
+struct sc18is600dev {
+	struct i2c_adapter adapter;
+	struct completion completion;
+	struct spi_device *spi;
+	struct regmap *regmap;
+	const struct chipdesc *chip;
+	struct gpio_desc *reset;
+	struct regulator *vdd;
+	struct clk *clk;
+	u32 clock_base;
+	u32 i2c_clock_frequency;
+	int state;
+};
+
+static irqreturn_t sc18is600_irq_handler(int this_irq, void *data)
+{
+	struct sc18is600dev *dev = data;
+	int err;
+
+	err = regmap_read(dev->regmap, SC18IS600_REG_I2C_STAT, &dev->state);
+	if (err)
+		return IRQ_NONE;
+
+	dev_vdbg(&dev->spi->dev, "irq received, stat=%08x", dev->state);
+
+	/* no irq is generated for busy state, so ignore this irq */
+	if (dev->state == SC18IS600_STAT_BUSY)
+		return IRQ_NONE;
+
+	complete(&dev->completion);
+	return IRQ_HANDLED;
+}
+
+static int reg_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct device *dev = context;
+	struct spi_device *spi = to_spi_device(dev);
+	u8 txbuffer[2] = { SC18IS600_CMD_REG_RD, reg & 0xff };
+	u8 rxbuffer[1];
+	int err;
+
+	err = spi_write_then_read(spi, txbuffer, sizeof(txbuffer),
+				       rxbuffer, sizeof(rxbuffer));
+	if (err)
+		return err;
+
+	*val = rxbuffer[0];
+
+	return 0;
+}
+
+static int reg_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct device *dev = context;
+	struct spi_device *spi = to_spi_device(dev);
+	u8 txbuffer[3] = { SC18IS600_CMD_REG_WR, reg & 0xff, val & 0xff };
+
+	return spi_write(spi, txbuffer, sizeof(txbuffer));
+}
+
+static struct regmap_bus regmap_sc18is600_bus = {
+	.reg_write = reg_write,
+	.reg_read = reg_read,
+	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
+	.val_format_endian_default = REGMAP_ENDIAN_BIG,
+};
+
+static void sc18is600_setup_clock_frequency(struct sc18is600dev *dev)
+{
+	int reg = DIV_ROUND_UP(dev->clock_base, dev->i2c_clock_frequency);
+
+	if (reg < 5)
+		reg = 5;
+	if (reg > 255)
+		reg = 255;
+
+	dev_dbg(&dev->spi->dev, "i2c clock frequency: %08x", reg);
+	regmap_write(dev->regmap, SC18IS600_REG_I2C_CLOCK, reg);
+}
+
+static void sc18is600_setup_timeout(struct sc18is600dev *dev,
+				    bool enable, int timeout_ms)
+{
+	int timeout = DIV_ROUND_UP(timeout_ms * dev->chip->timeout_base, 10000);
+	u8 reg;
+
+	if (timeout <= 0)
+		timeout = 1;
+	if (timeout > 255)
+		timeout = 255;
+
+	reg = (timeout & 0x7F) << 1;
+	reg |= (!!enable);
+
+	dev_dbg(&dev->spi->dev, "i2c timeout: %08x", reg);
+	regmap_write(dev->regmap, SC18IS600_REG_I2C_TIMEOUT, reg);
+}
+
+static void sc18is600_reset(struct sc18is600dev *dev)
+{
+	if (dev->reset) {
+		gpiod_set_value_cansleep(dev->reset, 1);
+		usleep_range(50, 100);
+		gpiod_set_value_cansleep(dev->reset, 0);
+		usleep_range(50, 100);
+	}
+
+	sc18is600_setup_clock_frequency(dev);
+	sc18is600_setup_timeout(dev, true, 500);
+}
+
+static int sc18is600_read(struct sc18is600dev *dev, struct i2c_msg *msg)
+{
+	u8 header[] = { SC18IS600_CMD_RD, msg->len, msg->addr << 1 };
+	struct spi_transfer xfer[1] = { 0 };
+
+	xfer[0].tx_buf = header;
+	xfer[0].len = sizeof(header);
+
+	dev_dbg(&dev->spi->dev, "r(addr=%x, len=%d)", msg->addr, msg->len);
+	return spi_sync_transfer(dev->spi, xfer, 1);
+}
+
+static int sc18is600_write(struct sc18is600dev *dev, struct i2c_msg *msg)
+{
+	u8 header[] = { SC18IS600_CMD_WR, msg->len, msg->addr << 1 };
+	struct spi_transfer xfer[2] = { 0 };
+
+	xfer[0].tx_buf = header;
+	xfer[0].len = sizeof(header);
+
+	xfer[1].tx_buf = msg->buf;
+	xfer[1].len = msg->len;
+
+	dev_dbg(&dev->spi->dev, "w(addr=%x, len=%d)", msg->addr, msg->len);
+	return spi_sync_transfer(dev->spi, xfer, 2);
+}
+
+static int sc18is600_read_after_write(struct sc18is600dev *dev,
+				      struct i2c_msg *msg1,
+				      struct i2c_msg *msg2)
+{
+	u8 header1[] =
+		{ SC18IS600_CMD_WR_RD, msg1->len, msg2->len, msg1->addr << 1 };
+	u8 header2[] = { msg2->addr << 1 };
+	struct spi_transfer xfer[3] = { 0 };
+
+	xfer[0].tx_buf = header1;
+	xfer[0].len = sizeof(header1);
+
+	xfer[1].tx_buf = msg1->buf;
+	xfer[1].len = msg1->len;
+
+	xfer[2].tx_buf = header2;
+	xfer[2].len = sizeof(header2);
+
+	dev_dbg(&dev->spi->dev, "w(addr=%x, len=%d) + r(addr=%x, len=%d)",
+		msg1->addr, msg1->len, msg2->addr, msg2->len);
+	return spi_sync_transfer(dev->spi, xfer, 3);
+}
+
+static int sc18is600_write_after_write(struct sc18is600dev *dev,
+				       struct i2c_msg *msg1,
+				       struct i2c_msg *msg2)
+{
+	u8 header1[] =
+		{ SC18IS600_CMD_WR_WR, msg1->len, msg2->len, msg1->addr << 1 };
+	u8 header2[] = { msg2->addr << 1 };
+	struct spi_transfer xfer[4] = { 0 };
+
+	xfer[0].tx_buf = header1;
+	xfer[0].len = sizeof(header1);
+
+	xfer[1].tx_buf = msg1->buf;
+	xfer[1].len = msg1->len;
+
+	xfer[2].tx_buf = header2;
+	xfer[2].len = sizeof(header2);
+
+	xfer[3].tx_buf = msg2->buf;
+	xfer[3].len = msg2->len;
+
+	dev_dbg(&dev->spi->dev, "w(addr=%x, len=%d) + w(addr=%x, len=%d)",
+		msg1->addr, msg1->len, msg2->addr, msg2->len);
+	return spi_sync_transfer(dev->spi, xfer, 4);
+}
+
+static int sc18is600_read_buffer(struct sc18is600dev *dev, struct i2c_msg *msg)
+{
+	static const u8 read_buffer_cmd = SC18IS600_REG_I2C_BUFFER;
+
+	return spi_write_then_read(dev->spi, &read_buffer_cmd, 1,
+				   msg->buf, msg->len);
+}
+
+static int sc18is600_xfer(struct i2c_adapter *adapter,
+			  struct i2c_msg *msgs, int num)
+{
+	struct sc18is600dev *dev = adapter->algo_data;
+	int read_operations = 0;
+	int i, err;
+
+	for (i = 0; i < num; i++) {
+		if (msgs[i].len > dev->chip->buffer_size)
+			return -EOPNOTSUPP;
+
+		/* chip only support standard read & write */
+		if (msgs[i].flags & ~I2C_M_RD)
+			return -EOPNOTSUPP;
+
+		if (msgs[i].flags & I2C_M_RD)
+			read_operations++;
+	}
+
+	reinit_completion(&dev->completion);
+
+	if (num == 1 && read_operations == 1)
+		err = sc18is600_read(dev, &msgs[0]);
+	else if (num == 1)
+		err = sc18is600_write(dev, &msgs[0]);
+	else if (num == 2 && read_operations == 1)
+		err = sc18is600_read_after_write(dev, &msgs[0], &msgs[1]);
+	else if (num == 2)
+		err = sc18is600_write_after_write(dev, &msgs[0], &msgs[1]);
+	else
+		return -EOPNOTSUPP;
+
+	if (err) {
+		dev_err(&dev->spi->dev, "spi transfer failed: %d", err);
+		return err;
+	}
+
+	err = wait_for_completion_timeout(&dev->completion, adapter->timeout);
+	if (!err) {
+		dev_warn(&dev->spi->dev,
+			 "timeout waiting for irq, poll status register");
+		dev->state = SC18IS600_STAT_BUSY;
+		regmap_read(dev->regmap, SC18IS600_REG_I2C_STAT, &dev->state);
+	}
+
+	switch (dev->state) {
+	case SC18IS600_STAT_OK:
+		break;
+	case SC18IS600_STAT_NAK_ADDR:
+		return -EIO;
+	case SC18IS600_STAT_NAK_DATA:
+		return -EREMOTEIO;
+	case SC18IS600_STAT_SIZE:
+		return -EINVAL;
+	case SC18IS600_STAT_TIMEOUT:
+		return -ETIMEDOUT;
+	case SC18IS600_STAT_TIMEOUT2:
+		return -ETIMEDOUT;
+	case SC18IS600_STAT_BLOCKED:
+		return -ETIMEDOUT;
+	default:
+	case SC18IS600_STAT_BUSY:
+		dev_err(&dev->spi->dev, "device hangup detected, reset!");
+		sc18is600_reset(dev);
+		return -EAGAIN;
+	}
+
+	if (!read_operations)
+		return 0;
+
+	err = sc18is600_read_buffer(dev, &msgs[num-1]);
+	if (err)
+		return err;
+
+	return num;
+}
+
+static u32 sc18is600_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm sc18is600_algorithm = {
+	.master_xfer	= sc18is600_xfer,
+	.functionality	= sc18is600_func,
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id sc18is600_of_match[] = {
+	{ .compatible = "nxp,sc18is600", .data = &chip_sc18is600 },
+	{ .compatible = "nxp,sc18is601", .data = &chip_sc18is601 },
+	{ .compatible = "silabs,cp2120", .data = &chip_cp2120 },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sc18is600_of_match);
+#endif
+
+static int sc18is600_probe(struct spi_device *spi)
+{
+	const struct of_device_id *of_id;
+	struct sc18is600dev *dev;
+	int err;
+
+	of_id = of_match_device(sc18is600_of_match, &spi->dev);
+	if (!of_id)
+		return -ENODEV;
+
+	dev = devm_kzalloc(&spi->dev, sizeof(*dev), GFP_KERNEL);
+	if (dev == NULL)
+		return -ENOMEM;
+	spi_set_drvdata(spi, dev);
+
+	init_completion(&dev->completion);
+
+	dev->spi = spi;
+	dev->adapter.owner = THIS_MODULE;
+	dev->adapter.class = I2C_CLASS_DEPRECATED;
+	dev->adapter.algo = &sc18is600_algorithm;
+	dev->adapter.algo_data = dev;
+	dev->adapter.dev.parent = &spi->dev;
+	dev->chip = of_id->data;
+
+	snprintf(dev->adapter.name, sizeof(dev->adapter.name),
+		 "SC18IS600 at SPI %02d device %02d",
+		 spi->master->bus_num, spi->chip_select);
+
+	spi->bits_per_word = 8;
+	spi->mode = SPI_MODE_3;
+	spi->max_speed_hz = dev->chip->max_spi_speed;
+
+	err = spi_setup(spi);
+	if (err)
+		return err;
+
+	dev->reset = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(dev->reset)) {
+		err = PTR_ERR(dev->reset);
+		dev_err(&spi->dev, "Failed to reset gpio, err: %d\n", err);
+		return err;
+	}
+
+	err = devm_request_threaded_irq(&spi->dev, spi->irq, NULL,
+					sc18is600_irq_handler,
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					"sc18is600", dev);
+	if (err) {
+		dev_err(&spi->dev, "Failed to request irq, err: %d\n", err);
+		return err;
+	}
+
+	dev->regmap = devm_regmap_init(&dev->spi->dev,
+			       &regmap_sc18is600_bus, &dev->spi->dev,
+			       dev->chip->regmap_cfg);
+	if (IS_ERR(dev->regmap)) {
+		err = PTR_ERR(dev->regmap);
+		dev_err(&spi->dev, "Failed to init regmap, err: %d\n", err);
+		return err;
+	}
+
+	err = device_property_read_u32(&spi->dev, "clock-frequency",
+				       &dev->i2c_clock_frequency);
+	if (err) {
+		dev->i2c_clock_frequency = SC18IS600_DEFAULT_FREQ;
+		dev_dbg(&spi->dev, "using default frequency %u\n",
+			dev->i2c_clock_frequency);
+	}
+
+	dev->vdd = devm_regulator_get(&spi->dev, "vdd");
+	if (IS_ERR(dev->vdd)) {
+		err = PTR_ERR(dev->vdd);
+		dev_err(&spi->dev, "could not acquire vdd: %d\n", err);
+		return err;
+	}
+
+	if (!dev->chip->clock_base) {
+		dev->clk = devm_clk_get(&spi->dev, "clkin");
+		if (IS_ERR(dev->clk)) {
+			err = PTR_ERR(dev->clk);
+			dev_err(&spi->dev, "could not acquire vdd: %d\n", err);
+			return err;
+		}
+
+		clk_prepare_enable(dev->clk);
+
+		dev->clock_base = clk_get_rate(dev->clk) / 4;
+	} else {
+		dev->clock_base = dev->chip->clock_base;
+	}
+
+	err = regulator_enable(dev->vdd);
+	if (err) {
+		dev_err(&spi->dev, "could not enable vdd: %d\n", err);
+		return err;
+	}
+
+	sc18is600_reset(dev);
+
+	err = i2c_add_adapter(&dev->adapter);
+	if (err)
+		goto out_disable_regulator;
+
+	return 0;
+
+out_disable_regulator:
+	regulator_disable(dev->vdd);
+	return err;
+}
+
+static int sc18is600_remove(struct spi_device *spi)
+{
+	struct sc18is600dev *dev = spi_get_drvdata(spi);
+
+	i2c_del_adapter(&dev->adapter);
+
+	regulator_disable(dev->vdd);
+
+	return 0;
+}
+
+static struct spi_driver sc18is600_driver = {
+	.probe		= sc18is600_probe,
+	.remove		= sc18is600_remove,
+	.driver		= {
+		.name	= "i2c-sc18is600",
+		.of_match_table = of_match_ptr(sc18is600_of_match),
+	},
+};
+module_spi_driver(sc18is600_driver);
+
+MODULE_AUTHOR("Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>");
+MODULE_DESCRIPTION("NXP SC18IS600 I2C bus adapter");
+MODULE_LICENSE("GPL");
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* RE: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc
From: Chris Brandt @ 2017-03-29 14:09 UTC (permalink / raw)
  To: Linus Walleij
  Cc: jacopo, Geert Uytterhoeven, Jacopo Mondi, Geert Uytterhoeven,
	Laurent Pinchart, Rob Herring, Mark Rutland, Russell King,
	Linux-Renesas, linux-gpio@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CACRpkdb+k82OUbi1y1SNC09asHsNSqbtvCkYyT=1eqjpdMt_cg@mail.gmail.com>

On Wednesday, March 29, 2017, Linus Walleij:
> If you prefer to use preprocessor macros or whatever to make the bitmasks
> or how you want to organize the constants in your include files is not my
> concern, do whatever you seem fit, just pack it into a 32bit thing somehow
> which makes sense from a maintenance point of view.

OK, I think everyone agrees that a single 32-bit value is fine because macros
will also for good readability and maintenance.


> >> Not only because it will save use from having a loong list(*) of
> >> macros that has to be kept up to date when/if new RZ hardware will
> >> arrive, but also because of readability and simplicity for down-stream
> and BSP users.
> >> Speaking of which, I would like to know what does Chris think of this.
> >
> > The list of macros would be very long, especially against the
> > different packaging version of the RZ/A1 series. 11 ports, 16-pins for
> > each port, 8 different function options for each pin....2 different
> package/pin variations.
> >
> > And at the end of the day....there is no benefit for the user over
> > just using a macro.
> 
> I don't know who has this idea that you could not use macros, certainly
> not me. Some misunderstanding must be going on. For what I'm concerned you
> can write hex numbers in the pinmux = <0x12345678>;
> 
> > The reason for the "FLAGS" is to work around a quirky hardware design
> (in my opinion).
> 
> The flags I don't like at all, and think they should be converted to
> generic pin config because they have nothing to do with muxing.
> 
> But I will point that out in the specific patch adding them.

OK, I think I understand your issue a little better of mixing user-defined config
with generic pinmux.

From our perspective, the FLAGS (BI_DIR, SWIO_IN, and SWIO_OUT ) were not really
optional when selecting what you want the pin to do....so we considered it part
of the pin-mux.

In the hardware manual, there are tables that say that for 'some' certain
pins/functions "just setting the pin to a function is not enough...you need to make
another register setting (that may or may not make sense), otherwise it's not going
to work".

So, trying to get the driver to be that smart across all the different pin/package
variations seems to be way too ugly (and a maintenance nightmare). Simply putting
that in the DT binding was much more cleaner.


As for how to pass this HW info into the driver, I'll move over to the other email
thread where you started to give some suggestions...


Chris


^ permalink raw reply

* RE: [PATCH v2 2/7] rtc: Add rtc-sh
From: Chris Brandt @ 2017-03-29 14:14 UTC (permalink / raw)
  To: Geert Uytterhoeven, Rob Herring
  Cc: Alessandro Zummo, Alexandre Belloni, Mark Rutland, Simon Horman,
	RTCLINUX, devicetree@vger.kernel.org, Linux-Renesas
In-Reply-To: <CAMuHMdVmwJYMzspVeDqToBrkfK+zGLMNhxGvEbqCA-vuCUXqoQ@mail.gmail.com>

On Wednesday, March 29, 2017, Geert Uytterhoeven wrote:
> >> +Example:
> >> +rtc: rtc@fcff1000 {
> >> +     compatible = "renesas,r7s72100-rtc", "renesas,sh-rtc";
> >> +     reg = <0xfcff1000 0x2e>;
> >> +     interrupts = <GIC_SPI 276 IRQ_TYPE_EDGE_RISING
> >> +                   GIC_SPI 277 IRQ_TYPE_EDGE_RISING
> >> +                   GIC_SPI 278 IRQ_TYPE_EDGE_RISING>;
> >> +     interrupt-names = "alarm", "period", "carry";
> >> +     clocks = <&mstp6_clks R7S72100_CLK_RTC>, <&rtc_x1_clk>,
> >> +              <&rtc_x3_clk>, <&extal_clk>;
> >> +     clock-names = "fck", "rtc_x1", "rtc_x3", "extal";
> >> +     power-domains = <&cpg_clocks>;
> >
> > Not documented.
> 
> "power-domains" is a platform property.
> 
> All hardware components need power.
> All synchronous hardware components need a clock.
> Most hardware components have a reset signal.
> 
> Whether these are exposed and can be controlled depends on the
> platform/SoC.
> So documenting them in each and every device binding looks overkill to me.
> I think this is something to be addressed by devicetree-specification
> (which doesn't handle clocks, power-domains, resets yet).
> 
> If you prefer, the property can be removed from the example, though.

I'll go ahead and pull it out (since I have to update the commit title
anyway as per Rob's request)

Besides...it's probably the most least important part of the example ;)

Thanks,
Chris

^ permalink raw reply

* [PATCHv5 05/11] ARM: dts: exynos: add HDMI controller phandle to exynos4.dtsi
From: Hans Verkuil @ 2017-03-29 14:15 UTC (permalink / raw)
  To: linux-media
  Cc: Daniel Vetter, Russell King, dri-devel, linux-samsung-soc,
	Krzysztof Kozlowski, Inki Dae, Marek Szyprowski,
	Javier Martinez Canillas, Benjamin Gaignard, Hans Verkuil,
	devicetree
In-Reply-To: <20170329141543.32935-1-hverkuil@xs4all.nl>

From: Hans Verkuil <hans.verkuil@cisco.com>

Add the new hdmi phandle to exynos4.dtsi. This phandle is needed by the
s5p-cec driver to initialize the CEC notifier framework.

Tested with my Odroid U3.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
CC: linux-samsung-soc@vger.kernel.org
CC: devicetree@vger.kernel.org
CC: Krzysztof Kozlowski <krzk@kernel.org>
---
 arch/arm/boot/dts/exynos4.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/exynos4.dtsi b/arch/arm/boot/dts/exynos4.dtsi
index 18def1c774d5..84fcdff140ae 100644
--- a/arch/arm/boot/dts/exynos4.dtsi
+++ b/arch/arm/boot/dts/exynos4.dtsi
@@ -771,6 +771,7 @@
 		clocks = <&clock CLK_HDMI_CEC>;
 		clock-names = "hdmicec";
 		samsung,syscon-phandle = <&pmu_system_controller>;
+		hdmi-phandle = <&hdmi>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&hdmi_cec>;
 		status = "disabled";
-- 
2.11.0

^ permalink raw reply related

* [PATCHv5 06/11] s5p-cec.txt: document the HDMI controller phandle
From: Hans Verkuil @ 2017-03-29 14:15 UTC (permalink / raw)
  To: linux-media
  Cc: devicetree, linux-samsung-soc, Russell King, Krzysztof Kozlowski,
	Javier Martinez Canillas, Hans Verkuil, dri-devel, Daniel Vetter,
	Marek Szyprowski
In-Reply-To: <20170329141543.32935-1-hverkuil@xs4all.nl>

From: Hans Verkuil <hans.verkuil@cisco.com>

Update the bindings documenting the new hdmi phandle.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
CC: linux-samsung-soc@vger.kernel.org
CC: devicetree@vger.kernel.org
CC: Krzysztof Kozlowski <krzk@kernel.org>
---
 Documentation/devicetree/bindings/media/s5p-cec.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/s5p-cec.txt b/Documentation/devicetree/bindings/media/s5p-cec.txt
index 925ab4d72eaa..4bb08d9d940b 100644
--- a/Documentation/devicetree/bindings/media/s5p-cec.txt
+++ b/Documentation/devicetree/bindings/media/s5p-cec.txt
@@ -15,6 +15,7 @@ Required properties:
   - clock-names : from common clock binding: must contain "hdmicec",
 		  corresponding to entry in the clocks property.
   - samsung,syscon-phandle - phandle to the PMU system controller
+  - hdmi-phandle - phandle to the HDMI controller
 
 Example:
 
@@ -25,6 +26,7 @@ hdmicec: cec@100B0000 {
 	clocks = <&clock CLK_HDMI_CEC>;
 	clock-names = "hdmicec";
 	samsung,syscon-phandle = <&pmu_system_controller>;
+	hdmi-phandle = <&hdmi>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&hdmi_cec>;
 	status = "okay";
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related

* [PATCHv5 09/11] stih-cec.txt: document new hdmi phandle
From: Hans Verkuil @ 2017-03-29 14:15 UTC (permalink / raw)
  To: linux-media
  Cc: Daniel Vetter, Russell King, dri-devel, linux-samsung-soc,
	Krzysztof Kozlowski, Inki Dae, Marek Szyprowski,
	Javier Martinez Canillas, Benjamin Gaignard, Hans Verkuil,
	devicetree
In-Reply-To: <20170329141543.32935-1-hverkuil@xs4all.nl>

From: Benjamin Gaignard <benjamin.gaignard@linaro.org>

Update the bindings documentation with the new hdmi phandle.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Rob Herring <robh@kernel.org>
CC: devicetree@vger.kernel.org
---
 Documentation/devicetree/bindings/media/stih-cec.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/stih-cec.txt b/Documentation/devicetree/bindings/media/stih-cec.txt
index 71c4b2f4bcef..289a08b33651 100644
--- a/Documentation/devicetree/bindings/media/stih-cec.txt
+++ b/Documentation/devicetree/bindings/media/stih-cec.txt
@@ -9,6 +9,7 @@ Required properties:
  - pinctrl-names: Contains only one value - "default"
  - pinctrl-0: Specifies the pin control groups used for CEC hardware.
  - resets: Reference to a reset controller
+ - hdmi-phandle: Phandle to the HDMI controller
 
 Example for STIH407:
 
@@ -22,4 +23,5 @@ sti-cec@094a087c {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_cec0_default>;
 	resets = <&softreset STIH407_LPM_SOFTRESET>;
+	hdmi-phandle = <&hdmi>;
 };
-- 
2.11.0

^ permalink raw reply related

* [PATCHv5 10/11] stih-cec: add CEC notifier support
From: Hans Verkuil @ 2017-03-29 14:15 UTC (permalink / raw)
  To: linux-media
  Cc: devicetree, linux-samsung-soc, Russell King, Krzysztof Kozlowski,
	Javier Martinez Canillas, Hans Verkuil, dri-devel, Daniel Vetter,
	Marek Szyprowski
In-Reply-To: <20170329141543.32935-1-hverkuil@xs4all.nl>

From: Benjamin Gaignard <benjamin.gaignard@linaro.org>

By using the CEC notifier framework there is no longer any reason
to manually set the physical address. This was the one blocking
issue that prevented this driver from going out of staging, so do
this move as well.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
CC: devicetree@vger.kernel.org
---
 drivers/media/platform/Kconfig                     | 10 +++++++
 drivers/media/platform/Makefile                    |  1 +
 .../st-cec => media/platform/sti/cec}/Makefile     |  0
 .../st-cec => media/platform/sti/cec}/stih-cec.c   | 31 +++++++++++++++++++---
 drivers/staging/media/Kconfig                      |  2 --
 drivers/staging/media/Makefile                     |  1 -
 drivers/staging/media/st-cec/Kconfig               |  8 ------
 drivers/staging/media/st-cec/TODO                  |  7 -----
 8 files changed, 39 insertions(+), 21 deletions(-)
 rename drivers/{staging/media/st-cec => media/platform/sti/cec}/Makefile (100%)
 rename drivers/{staging/media/st-cec => media/platform/sti/cec}/stih-cec.c (93%)
 delete mode 100644 drivers/staging/media/st-cec/Kconfig
 delete mode 100644 drivers/staging/media/st-cec/TODO

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 2c449b88fc94..7321f6123659 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -476,6 +476,16 @@ config VIDEO_SAMSUNG_S5P_CEC
          CEC bus is present in the HDMI connector and enables communication
          between compatible devices.
 
+config VIDEO_STI_HDMI_CEC
+       tristate "STMicroelectronics STiH4xx HDMI CEC driver"
+       depends on VIDEO_DEV && MEDIA_CEC_SUPPORT && (ARCH_STI || COMPILE_TEST)
+       select MEDIA_CEC_NOTIFIER
+       ---help---
+         This is a driver for STIH4xx HDMI CEC interface. It uses the
+         generic CEC framework interface.
+         CEC bus is present in the HDMI connector and enables communication
+         between compatible devices.
+
 endif #V4L_CEC_DRIVERS
 
 menuconfig V4L_TEST_DRIVERS
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 2f94d82afa4c..940724ab9b70 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS_GSC)	+= exynos-gsc/
 obj-$(CONFIG_VIDEO_STI_BDISP)		+= sti/bdisp/
 obj-$(CONFIG_VIDEO_STI_HVA)		+= sti/hva/
 obj-$(CONFIG_DVB_C8SECTPFE)		+= sti/c8sectpfe/
+obj-$(CONFIG_VIDEO_STI_HDMI_CEC) 	+= sti/cec/
 
 obj-$(CONFIG_VIDEO_STI_DELTA)		+= sti/delta/
 
diff --git a/drivers/staging/media/st-cec/Makefile b/drivers/media/platform/sti/cec/Makefile
similarity index 100%
rename from drivers/staging/media/st-cec/Makefile
rename to drivers/media/platform/sti/cec/Makefile
diff --git a/drivers/staging/media/st-cec/stih-cec.c b/drivers/media/platform/sti/cec/stih-cec.c
similarity index 93%
rename from drivers/staging/media/st-cec/stih-cec.c
rename to drivers/media/platform/sti/cec/stih-cec.c
index 3c25638a9610..636281c64c04 100644
--- a/drivers/staging/media/st-cec/stih-cec.c
+++ b/drivers/media/platform/sti/cec/stih-cec.c
@@ -1,6 +1,4 @@
 /*
- * drivers/staging/media/st-cec/stih-cec.c
- *
  * STIH4xx CEC driver
  * Copyright (C) STMicroelectronic SA 2016
  *
@@ -15,9 +13,11 @@
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 
 #include <media/cec.h>
+#include <media/cec-notifier.h>
 
 #define CEC_NAME	"stih-cec"
 
@@ -129,6 +129,7 @@ struct stih_cec {
 	void __iomem		*regs;
 	int			irq;
 	u32			irq_status;
+	struct cec_notifier	*notifier;
 };
 
 static int stih_cec_adap_enable(struct cec_adapter *adap, bool enable)
@@ -303,12 +304,29 @@ static int stih_cec_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct resource *res;
 	struct stih_cec *cec;
+	struct device_node *np;
+	struct platform_device *hdmi_dev;
 	int ret;
 
 	cec = devm_kzalloc(dev, sizeof(*cec), GFP_KERNEL);
 	if (!cec)
 		return -ENOMEM;
 
+	np = of_parse_phandle(pdev->dev.of_node, "hdmi-phandle", 0);
+
+	if (!np) {
+		dev_err(&pdev->dev, "Failed to find hdmi node in device tree\n");
+		return -ENODEV;
+	}
+
+	hdmi_dev = of_find_device_by_node(np);
+	if (!hdmi_dev)
+		return -EPROBE_DEFER;
+
+	cec->notifier = cec_notifier_get(&hdmi_dev->dev);
+	if (!cec->notifier)
+		return -ENOMEM;
+
 	cec->dev = dev;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -335,7 +353,7 @@ static int stih_cec_probe(struct platform_device *pdev)
 	cec->adap = cec_allocate_adapter(&sti_cec_adap_ops, cec,
 			CEC_NAME,
 			CEC_CAP_LOG_ADDRS | CEC_CAP_PASSTHROUGH |
-			CEC_CAP_PHYS_ADDR | CEC_CAP_TRANSMIT, 1);
+			CEC_CAP_TRANSMIT, 1);
 	ret = PTR_ERR_OR_ZERO(cec->adap);
 	if (ret)
 		return ret;
@@ -346,12 +364,19 @@ static int stih_cec_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	cec_register_cec_notifier(cec->adap, cec->notifier);
+
 	platform_set_drvdata(pdev, cec);
 	return 0;
 }
 
 static int stih_cec_remove(struct platform_device *pdev)
 {
+	struct stih_cec *cec = platform_get_drvdata(pdev);
+
+	cec_unregister_adapter(cec->adap);
+	cec_notifier_put(cec->notifier);
+
 	return 0;
 }
 
diff --git a/drivers/staging/media/Kconfig b/drivers/staging/media/Kconfig
index c0d83cecf528..8ed8202da57a 100644
--- a/drivers/staging/media/Kconfig
+++ b/drivers/staging/media/Kconfig
@@ -32,6 +32,4 @@ source "drivers/staging/media/platform/bcm2835/Kconfig"
 # Keep LIRC at the end, as it has sub-menus
 source "drivers/staging/media/lirc/Kconfig"
 
-source "drivers/staging/media/st-cec/Kconfig"
-
 endif
diff --git a/drivers/staging/media/Makefile b/drivers/staging/media/Makefile
index 97b29ece9a2c..3a6adeabede1 100644
--- a/drivers/staging/media/Makefile
+++ b/drivers/staging/media/Makefile
@@ -4,4 +4,3 @@ obj-$(CONFIG_LIRC_STAGING)	+= lirc/
 obj-$(CONFIG_VIDEO_BCM2835)	+= platform/bcm2835/
 obj-$(CONFIG_VIDEO_DM365_VPFE)	+= davinci_vpfe/
 obj-$(CONFIG_VIDEO_OMAP4)	+= omap4iss/
-obj-$(CONFIG_VIDEO_STI_HDMI_CEC) += st-cec/
diff --git a/drivers/staging/media/st-cec/Kconfig b/drivers/staging/media/st-cec/Kconfig
deleted file mode 100644
index c04283db58d6..000000000000
--- a/drivers/staging/media/st-cec/Kconfig
+++ /dev/null
@@ -1,8 +0,0 @@
-config VIDEO_STI_HDMI_CEC
-       tristate "STMicroelectronics STiH4xx HDMI CEC driver"
-       depends on VIDEO_DEV && MEDIA_CEC_SUPPORT && (ARCH_STI || COMPILE_TEST)
-       ---help---
-         This is a driver for STIH4xx HDMI CEC interface. It uses the
-         generic CEC framework interface.
-         CEC bus is present in the HDMI connector and enables communication
-         between compatible devices.
diff --git a/drivers/staging/media/st-cec/TODO b/drivers/staging/media/st-cec/TODO
deleted file mode 100644
index c61289742c5c..000000000000
--- a/drivers/staging/media/st-cec/TODO
+++ /dev/null
@@ -1,7 +0,0 @@
-This driver requires that userspace sets the physical address.
-However, this should be passed on from the corresponding
-ST HDMI driver.
-
-We have to wait until the HDMI notifier framework has been merged
-in order to handle this gracefully, until that time this driver
-has to remain in staging.
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related

* [PATCHv5 11/11] arm: sti: update sti-cec for CEC notifier support
From: Hans Verkuil @ 2017-03-29 14:15 UTC (permalink / raw)
  To: linux-media
  Cc: Daniel Vetter, Russell King, dri-devel, linux-samsung-soc,
	Krzysztof Kozlowski, Inki Dae, Marek Szyprowski,
	Javier Martinez Canillas, Benjamin Gaignard, Hans Verkuil,
	devicetree
In-Reply-To: <20170329141543.32935-1-hverkuil@xs4all.nl>

From: Benjamin Gaignard <benjamin.gaignard@linaro.org>

To use CEC notifier sti CEC driver needs to get phandle
of the hdmi device.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
CC: devicetree@vger.kernel.org
---
 arch/arm/boot/dts/stih407-family.dtsi | 12 ------------
 arch/arm/boot/dts/stih410.dtsi        | 13 +++++++++++++
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index d753ac36788f..044184580326 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -742,18 +742,6 @@
 				 <&clk_s_c0_flexgen CLK_ETH_PHY>;
 		};
 
-		cec: sti-cec@094a087c {
-			compatible = "st,stih-cec";
-			reg = <0x94a087c 0x64>;
-			clocks = <&clk_sysin>;
-			clock-names = "cec-clk";
-			interrupts = <GIC_SPI 140 IRQ_TYPE_NONE>;
-			interrupt-names = "cec-irq";
-			pinctrl-names = "default";
-			pinctrl-0 = <&pinctrl_cec0_default>;
-			resets = <&softreset STIH407_LPM_SOFTRESET>;
-		};
-
 		rng10: rng@08a89000 {
 			compatible      = "st,rng";
 			reg		= <0x08a89000 0x1000>;
diff --git a/arch/arm/boot/dts/stih410.dtsi b/arch/arm/boot/dts/stih410.dtsi
index 3c9672c5b09f..21fe72b183d8 100644
--- a/arch/arm/boot/dts/stih410.dtsi
+++ b/arch/arm/boot/dts/stih410.dtsi
@@ -281,5 +281,18 @@
 				 <&clk_s_c0_flexgen CLK_ST231_DMU>,
 				 <&clk_s_c0_flexgen CLK_FLASH_PROMIP>;
 		};
+
+		sti-cec@094a087c {
+			compatible = "st,stih-cec";
+			reg = <0x94a087c 0x64>;
+			clocks = <&clk_sysin>;
+			clock-names = "cec-clk";
+			interrupts = <GIC_SPI 140 IRQ_TYPE_NONE>;
+			interrupt-names = "cec-irq";
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_cec0_default>;
+			resets = <&softreset STIH407_LPM_SOFTRESET>;
+			hdmi-phandle = <&sti_hdmi>;
+		};
 	};
 };
-- 
2.11.0

^ permalink raw reply related

* Re: [PATCH v3] ASoC: Add support for Maxim Integrated MAX98927 Amplifier
From: Mark Brown @ 2017-03-29 14:23 UTC (permalink / raw)
  To: Ryan Lee
  Cc: mark.rutland, alsa-devel, kuninori.morimoto.gx, lgirdwood, tiwai,
	srinivas.kandagatla, romain.perier, bardliao, lars, axel.lin,
	Paul.Handrigan, devicetree, arnd, nh6z, robh+dt, ckeepax, dgreid,
	oder_chiou, ryan.lee.maxim, KCHSU0, linux-kernel
In-Reply-To: <1490748828-31701-1-git-send-email-ryans.lee@maximintegrated.com>


[-- Attachment #1.1: Type: text/plain, Size: 3776 bytes --]

On Wed, Mar 29, 2017 at 09:53:48AM +0900, Ryan Lee wrote:

> +	case SND_SOC_DAIFMT_CBS_CFM:
> +		mode = MAX98927_PCM_MASTER_MODE_HYBRID;
> +	default:
> +		dev_err(codec->dev, "DAI clock mode unsupported");

Either we don't support _CBS_CFM or there's a missing break there.

> +	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> +	case SND_SOC_DAIFMT_I2S:
> +		max98927->iface |= SND_SOC_DAIFMT_I2S;
> +
> +	break;

Please use the kernel coding style as you do in the rest of this
function.

> +	int reg = MAX98927_R0022_PCM_CLK_SETUP;
> +	int mask = MAX98927_PCM_CLK_SETUP_BSEL_MASK;

> +	regmap_update_bits(max98927->regmap, reg, mask, value);

reg and mask have exactly one user (as you'd expect), just use the
constants directly here to make things clearer.

> +	switch (snd_pcm_format_width(params_format(params))) {
> +	case 16:
> +		chan_sz = MAX98927_PCM_MODE_CFG_CHANSZ_16;
> +		max98927->ch_size = 16;

You could just assign ch_size directly.

> +	/* sampling rate configuration */
> +	switch (params_rate(params)) {
> +	case 8000:
> +		sampling_rate |= MAX98927_PCM_SR_SET1_SR_8000;

sampling_rate is only ever set to a real value in this switch statement,
you're not oring with anything else so you can just use an assignment
which would be a lot clearer (it's not obvious what else might go in
there) and the initial assignment to 0 for this and chan_sz can only
mask errors.

> +	/* set sampling rate of IV */
> +	if (max98927->interleave_mode &&
> +		sampling_rate > MAX98927_PCM_SR_SET1_SR_16000)
> +		regmap_update_bits(max98927->regmap,

Please use the kernel coding style, indent the second line of the if
with the ( so that it doesn't look like part of the conditional code.

> +	switch (reg) {
> +	case MAX98927_R0001_INT_RAW1 ... MAX98927_R0028_ICC_RX_EN_B:


> +		return true;
> +	}
> +	return false;

It'd be clearer to put the return false in the switch statement like the
return true, and also consistent with the _volatile_reg() function.

> +static const char * const max98927_speaker_source_text[] = {
> +	"i2s", "reserved", "tone", "pdm"
> +};

This looks like it should be a DAPM control.

> +static const char * const max98927_monomix_output_text[] = {
> +	"ch_0", "ch_1", "ch_1_2_div"
> +};

Similarly here.

> +	SOC_SINGLE_TLV("Digital Gain", MAX98927_R0036_AMP_VOL_CTRL,
> +		0, (1<<MAX98927_AMP_VOL_WIDTH)-1, 0,
> +		max98927_digital_tlv),

All volume controls should end with Volume as per control-names.rst.

> +	SOC_SINGLE("Amp DSP Enable", MAX98927_R0052_BROWNOUT_EN,
> +		MAX98927_BROWNOUT_DSP_SHIFT, 1, 0),

All on/off switches should end with Switch as per control-names.rst.

> +static int max98927_probe(struct snd_soc_codec *codec)
> +{

> +       /* Check Revision ID */
> +       ret = regmap_read(max98927->regmap,
> +               MAX98927_R01FF_REV_ID, &reg);

Basic device identification and setup should be done at the chip level
probe not at the CODEC level so that if there are problems we fail as
early as possible and so that diagnostic information is available to
users as soon as possible, even if there's no sound card for the device
in the system.

> +	/* Set inital volume (+13dB) */

As with all other CODEC drivers you should leave the hardware defaults
alone, what makes sense for your system may not make sense for other
systems and the hardware defaults are a fixed thing.

> +	/* Boost Output Voltage & Current limit */
> +	regmap_write(max98927->regmap,
> +		MAX98927_R0040_BOOST_CTRL0,
> +		0x1C);
> +	regmap_write(max98927->regmap,
> +		MAX98927_R0042_BOOST_CTRL1,
> +		0x3E);

This should be system specific, these values might be unsafe in some
systems.

> +err:
> +	if (max98927)
> +		devm_kfree(&i2c->dev, max98927);

There is no need to explicitly free devm_ allocated memory, that's the
point of devm.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply

* Re: [PATCH v3 2/2] PCI: Add tango PCIe host bridge support
From: Robin Murphy @ 2017-03-29 14:33 UTC (permalink / raw)
  To: Mason
  Cc: Marc Gonzalez, Bjorn Helgaas, Marc Zyngier, Thomas Gleixner,
	Lorenzo Pieralisi, Liviu Dudau, David Laight, linux-pci,
	Linux ARM, Thibaud Cornic, Phuong Nguyen, LKML, DT
In-Reply-To: <96da696c-141a-3e8d-1fb7-2c024a295f58@free.fr>

On 29/03/17 13:53, Mason wrote:
> On 29/03/2017 14:19, Robin Murphy wrote:
> 
>> On 29/03/17 12:34, Marc Gonzalez wrote:
>>
>>> +	/*
>>> +	 * QUIRK #3
>>> +	 * Unfortunately, config and mem spaces are muxed.
>>> +	 * Linux does not support such a setting, since drivers are free
>>> +	 * to access mem space directly, at any time.
>>> +	 * Therefore, we can only PRAY that config and mem space accesses
>>> +	 * NEVER occur concurrently.
>>> +	 */
>>
>> What about David's suggestion of using an IPI for safe mutual exclusion?
> 
> I was left with the impression that this wouldn't solve the problem.
> If a mem space access is "in flight" on core0 when core1 starts a
> config space access, an IPI will not prevent breakage.
> 
> Did I misunderstand?
> 
> For my education, what is the API to send an IPI?
> And the API to handle an IPI?

There are a few ways you could implement some custom cross-call,
although in this case I think stop_machine() would probably be the most
appropriate candidate. However, you're right that in general it may not
actually help enough to be worthwhile - a DSB SY would ensure that
in-flight transactions have at least been observed by the CPUs and any
other coherent masters, but for any writes with a memory type allowing
early acknowledgement (i.e. a Normal or Device mapping of a BAR) that
doesn't necessarily correlate with them having reached their ultimate
destination. For a PCI destination in particular, I think the normal way
to ensure all posted writes have completed would be to read from config
space; ah...

>>> +	if (of_device_is_compatible(dev->of_node, "sigma,smp8759-pcie"))
>>> +		smp8759_init(pcie, base);
>>
>> ...then retrieve it with of_device_get_match_data() here. No need to
>> reinvent the wheel (or have to worry about the ordering of multiple
>> compatibles once rev. n+1 comes around).
> 
> I actually asked about this on IRC. The consensus was "use what
> best fits your use case". I need to do some processing based on
> the revision, so I thought
> 
>   if (chip_x)
> 	do_chip_x_init()
> 
> was a good way to express my intent. Did I misunderstand?

No, I'm in no way disputing that; what I'm pointing out is that you
already have an explicitly provided way to associate a value of "chip_x"
with a given compatible string - see other callers of
of_device_get_match_data() for inspiration. I don't have much of an
opinion as to whether it's an enum, a static structure of offsets and
callbacks, or you embrace the nasal demons and just wedge the init
function pointer in there directly (this'll never run on
IA-64/M68k/etc., right? :P). The point is that not only is it cleaner
and scales better as the driver grows, it stops you having to worry at
all about setting this trap for yourself:

	compatible = "rev3-with-extra-fun", "rev3";
	...

	if (of_device_is_compatible(dev, "rev3"))
		boring_init_without_extra_fun();	/* :( */

because once you've made your code robust against that, you'll realise
that what you've done is wasted your time open-coding a creaky
approximation of of_match_device().

Robin.

> For example, the init function for rev2 currently looks like this:
> 
> static void rev2_init(struct tango_pcie *pcie, void __iomem *base)
> {
> 	void __iomem *misc_irq	= base + 0x40;
> 	void __iomem *doorbell	= base + 0x8c;
> 
> 	pcie->mux		= base + 0x2c;
> 	pcie->msi_status	= base + 0x4c;
> 	pcie->msi_mask		= base + 0x6c;
> 	pcie->msi_doorbell	= 0x80000000;
> 
> 	writel(lower_32_bits(pcie->msi_doorbell), doorbell + 0);
> 	writel(upper_32_bits(pcie->msi_doorbell), doorbell + 4);
> 
> 	/* Enable legacy PCI interrupts */
> 	writel(BIT(15), misc_irq);
> 	writel(0xf << 4, misc_irq + 4);
> }
> 
>>> +#define VENDOR_SIGMA	0x1105
>>
>> Should this not be in include/linux/pci_ids.h?
> 
> Doh! Very likely. Thanks.
> 
> Regards.
> 

^ permalink raw reply

* Re: [PATCH v7 1/7] clocksource/drivers/clksrc-evt-probe: Describe with the DT both the clocksource and the clockevent
From: Mark Rutland @ 2017-03-29 14:34 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: Rob Herring, Alexander Kochetkov, Heiko Stuebner,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Thomas Gleixner,
	Russell King, Caesar Wang, Huang Tao
In-Reply-To: <20170329134134.GJ2123@mai>

On Wed, Mar 29, 2017 at 03:41:34PM +0200, Daniel Lezcano wrote:
> On Wed, Mar 29, 2017 at 01:57:14PM +0100, Mark Rutland wrote:
> > On Wed, Mar 29, 2017 at 02:36:38PM +0200, Daniel Lezcano wrote:

> > > arch/arc/boot/dts/abilis_tb10x.dtsi:
> > > 
> > >         /* TIMER0 with interrupt for clockevent */
> > >         timer0 {
> > >                 compatible = "snps,arc-timer";
> > >                 interrupts = <3>;
> > >                 interrupt-parent = <&intc>;
> > >                 clocks = <&cpu_clk>;
> > >         };
> > > 
> > >         /* TIMER1 for free running clocksource */
> > >         timer1 {
> > >                 compatible = "snps,arc-timer";
> > >                 clocks = <&cpu_clk>;
> > >         };
> > > 
> > > drivers/clocksource/arc_timer.c:
> > > 
> > > static int __init arc_of_timer_init(struct device_node *np)
> > > {
> > >         static int init_count = 0;
> > >         int ret;
> > > 
> > >         if (!init_count) {
> > >                 init_count = 1;
> > >                 ret = arc_clockevent_setup(np);
> > >         } else {
> > >                 ret = arc_cs_setup_timer1(np);
> > >         }
> > > 
> > >         return ret;
> > > }
> > > 
> > > So the purpose of these changes is to provide a stronger timer declaration in
> > > order to clearly split in the kernel a clocksource and a clockevent
> > > initialization.
> > 
> > I agree that this pattern is not nice. However, I think that splitting
> > devices as this level makes the problem *worse*.
> > 
> > Users care that they have a clocksource and a clockevent device. They
> > do not care *which* particular device is used as either. The comments in
> > the DT above are at best misleading.
> 
> Agree.
> 
> And the driver is assuming the first node is the clockevent and the second one
> is the clocksource. If the DT invert these nodes, that breaks the driver.

Sure, but that is something we can and should fix within Linux.

> > What we need is for the kernel to understand that devices can be both
> > clockevent and clocksource (perhaps mutually exclusively), such that the
> > kernel can decide how to make use of devices.
> > 
> > That way, for the above the kernel can figure out that timer0 could be
> > used as clocksource or clockevent, while timer1 can only be used as a
> > clocksource due to the lack of an interrupt. Thus, it can choose to use
> > timer0 as a clockevent, and timer1 and a clocksource.
> 
> Well, 'interrupt' gives an indication the timer can be used as a clockevent and
> clocksource, not the clockevent only.

Which is exactly what I said above, when I said:

	the kernel can figure out that timer0 could be used as
	clocksource or clockevent

Considering both timer0 and timer1 is how we can figure out timer0 must
be the clockevent, since timer1 cannot be.

> If we take the case of the rockchip, the arm_arch_timer clocksource is stopped
> when the CPU is clock gated. So specifically, we don't want to use this
> clocksource but we want to use the arch clockevents because they are better.

Sure. As I pointed out, we want to consider the holistic details to make
the right decision. i.e. the infrastructure should make the choice, not
the individual drivers.

Consider that the kernel may need to make decisions that differ it a
kernel is built wihout certain drivers. That cannot work if the use is
allocated in the DT.

[...]

> > > 2. A node can have a clockevent and|or a clocksource attributes
> > 
> > As above, this should not be in the DT given it's describing a
> > (Linux-specific) SW policy and not a HW detail.
> > 
> > So I must disagree with this.
> 
> IIUC my discussion with Rob, an attribute is acceptable (btw if
> 'clocksource'|'clockevent' names are too Linux specific (+1), what
> about a more generic name like 'tick' and 'time' ?).

The *meaning* of these is Linux specific. The naming is irrelevant.

> > > 3. The timer_probe pass a flag to the driver's init function, so this one knows
> > >    if it should invoke the clockevent/clocksource init functions.
> > >    No attribute defaults to clocksource|clockevent.
> > > 
> > > That would be backward compatible and will let to create drivers with clutch
> > > activated device via DT. Also, it will give the opportunity to the existing
> > > drivers to change consolidate their initialization routines.
> > 
> > I think that if anything, we need a combined clocksource+clockevent
> > device that we register to the core code. That means all
> > clocksource/clockevent drivers have a consolidated routine.
> > 
> > Subsequently, core code should determine how specifically to use the
> > device (e.g. based on what other devices are registered, and their
> > capabilities).
> 
> IMO, the core code is complex enough and that may imply more heuristics.

Given that the majority of cases are going to be multiple instances of
the same IP, I cannot imagine it is complex to get something that works.

A generally optimal configuration may require some heuristics, but
that's a different matter to a correct and functional configuration.

I must disagree with trying to push that complexity into the DT by means
of static SW policy, rather than solving the problem in SW where we have
all the information to do so.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* RE: [PATCH v3 2/2] PCI: Add tango PCIe host bridge support
From: David Laight @ 2017-03-29 14:38 UTC (permalink / raw)
  To: 'Robin Murphy', Mason
  Cc: Marc Gonzalez, Bjorn Helgaas, Marc Zyngier, Thomas Gleixner,
	Lorenzo Pieralisi, Liviu Dudau, linux-pci, Linux ARM,
	Thibaud Cornic, Phuong Nguyen, LKML, DT
In-Reply-To: <84a8b26d-7d24-df29-3959-45fce880916d@arm.com>

> > For my education, what is the API to send an IPI?
> > And the API to handle an IPI?
> 
> There are a few ways you could implement some custom cross-call,
> although in this case I think stop_machine() would probably be the most
> appropriate candidate. However, you're right that in general it may not
> actually help enough to be worthwhile - a DSB SY would ensure that
> in-flight transactions have at least been observed by the CPUs and any
> other coherent masters, but for any writes with a memory type allowing
> early acknowledgement (i.e. a Normal or Device mapping of a BAR) that
> doesn't necessarily correlate with them having reached their ultimate
> destination. For a PCI destination in particular, I think the normal way
> to ensure all posted writes have completed would be to read from config
> space; ah...

He almost certainly doesn't need to wait for the cycle to complete,
just long enough for the cycle to have been sent.

	David

^ permalink raw reply

* RE: [PATCH v3 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl header
From: Chris Brandt @ 2017-03-29 14:55 UTC (permalink / raw)
  To: Linus Walleij, Jacopo Mondi, Bjorn Andersson
  Cc: Geert Uytterhoeven, Laurent Pinchart, Rob Herring, Mark Rutland,
	Russell King, Linux-Renesas, linux-gpio@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CACRpkdZ4Z6c2YDu-e6kZ8-K7Dv_tRGxZj0vAqgPBx7rWwmHU-g@mail.gmail.com>

On Wednesday, March 29, 2017, Linus Walleij wrote:
> On Fri, Mar 24, 2017 at 4:22 PM, Jacopo Mondi <jacopo+renesas@jmondi.org>
> wrote:
> 
> > Add dt-bindings for Renesas r7s72100 pin controller header file.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> > +/*
> > + * Pin is bi-directional.
> > + * An alternate function that needs both input/output functionalities
> > +shall
> > + * be configured as bidirectional.
> > + * Eg. SDA/SCL pins of an I2c interface.
> > + */
> > +#define BI_DIR                 (1 << 3)
> 
> Any specific reason why this should not simply be added to
> include/linux/pinctrl/pinconf-generic.h
> as PIN_CONFIG_BIDIRECTIONAL and parsed in drivers/pinctrl/pinconf-
> generic.c from the (new) DT property "bidirectional" simply?

I see your point. It would cut down from every driver out there
inventing some new property or config instead of everyone just sharing
a fixed set.
Maybe someone else out there will end up having a need for a
"bidirectional" option.


> > +/*
> > + * Flags used to ask software to drive the pin I/O direction
> > +overriding the
> > + * alternate function configuration.
> > + * Some alternate functions require software to force I/O direction
> > +of a pin,
> > + * overriding the designated one.
> > + * Refer to the HW manual to know when this flag shall be used.
> > + */
> > +#define SWIO_IN                        (1 << 4)
> > +#define SWIO_OUT               (1 << 5)
> 
> What is wrong in doing this with generic pin config using
> PIN_CONFIG_INPUT_ENABLE and PIN_CONFIG_OUTPUT (ignoring the argument)?
> 
> In the device tree use input-enable and add a new output-enable (with
> unspecified value) with proper description and DT bindings?

Again, that's probably fine. It seems we are still doing the same thing
which is using the DT to pass extra config information to the driver.
And, we can do whatever we want with that info.


> And if you think these have no general applicability, by the end of the
> day they are *still* pin config, not magic flags we can choose to toss in
> with the muxing, so you can do what the Qualcomm driver does and add
> custom pin configurations extending the generic pin config, see
> drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> qcom,pull-up-strength etc.

But, it seems that when you set a config option, it applies to everything
in "pins"?

I2C Example: (seem OK)
	/* P1_6 = RIIC3SCL (bi dir) */
	/* P1_7 = RIIC3SDA (bi dir) */
	i2c3_pins: i2c3 {
		pins = <PIN(1, 6) | FUNC_1>,
		       <PIN(1, 7) | FUNC_1>;
		bidirectional;
	};


But, what do we do for Ethernet? All the pins are "normal" except just
the MDIO pin needs to be bidirectional.
That's the part I'm confused by.
How do we flag that just the ET_MDIO needs "bidirectional"?

	/* Ethernet */
	ether_pins: ether {
		/* Ethernet on Ports 1,2,3,5 */
		pins = <PIN(1, 14) | FUNC_4)>, /* P1_14 = ET_COL */
			<PIN(5, 9) | FUNC_2)>,	/* P5_9 = ET_MDC */
			<PIN(3, 3) | FUNC_2)>,	/* P3_3 = ET_MDIO !!!! (bi dir) !!!!!! */
			<PIN(3, 4) | FUNC_2)>,	/* P3_4 = ET_RXCLK */
			<PIN(3, 5) | FUNC_2)>,	/* P3_5 = ET_RXER */
			<PIN(3, 6) | FUNC_2)>,	/* P3_6 = ET_RXDV */
			<PIN(2, 0) | FUNC_2)>,	/* P2_0 = ET_TXCLK */
			<PIN(2, 1) | FUNC_2)>,	/* P2_1 = ET_TXER */
			<PIN(2, 2) | FUNC_2)>,	/* P2_2 = ET_TXEN */
			<PIN(2, 3) | FUNC_2)>,	/* P2_3 = ET_CRS */
			<PIN(2, 4) | FUNC_2)>,	/* P2_4 = ET_TXD0 */
			<PIN(2, 5) | FUNC_2)>,	/* P2_5 = ET_TXD1 */
			<PIN(2, 6) | FUNC_2)>,	/* P2_6 = ET_TXD2 */
			<PIN(2, 7) | FUNC_2)>,	/* P2_7 = ET_TXD3 */
			<PIN(2, 8) | FUNC_2)>,	/* P2_8 = ET_RXD0 */
			<PIN(2, 9) | FUNC_2)>,	/* P2_9 = ET_RXD1 */
			<PIN(2, 10) | FUNC_2)>, /* P2_10 = ET_RXD2 */
			<PIN(2, 11) | FUNC_2)>; /* P2_11 = ET_RXD3 */
	};


Chris

^ permalink raw reply

* Re: [PATCH v5 6/9] coresight: add support for CPU debug module
From: Mike Leach @ 2017-03-29 14:56 UTC (permalink / raw)
  To: Leo Yan
  Cc: Mathieu Poirier, Jonathan Corbet, Rob Herring, Mark Rutland,
	Wei Xu, Catalin Marinas, Will Deacon, Andy Gross, David Brown,
	Michael Turquette, Stephen Boyd, Guodong Xu, John Stultz,
	linux-doc-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	linux-soc-u79uwXL29TY76Z2rM5mHXA,
	linux-clk-u79uwXL29TY76Z2rM5mHXA, Suzuki K. Poulose, Sudeep
In-Reply-To: <20170329015423.GA5035@leoy-linaro>

 Hi Leo

On 29 March 2017 at 02:54, Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Hi Mathieu,
>
> On Tue, Mar 28, 2017 at 10:50:10AM -0600, Mathieu Poirier wrote:
>> On Sun, Mar 26, 2017 at 02:23:14AM +0800, Leo Yan wrote:
>
> [...]
>
>> > +static void debug_force_cpu_powered_up(struct debug_drvdata *drvdata)
>> > +{
>> > +   int timeout = DEBUG_WAIT_TIMEOUT;
>> > +
>> > +   drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
>> > +
>> > +   CS_UNLOCK(drvdata->base);
>> > +
>> > +   /* Bail out if CPU is powered up yet */
>> > +   if (drvdata->edprsr & EDPRSR_PU)
>> > +           goto out_powered_up;
>> > +
>> > +   /*
>> > +    * Send request to power management controller and assert
>> > +    * DBGPWRUPREQ signal; if power management controller has
>> > +    * sane implementation, it should enable CPU power domain
>> > +    * in case CPU is in low power state.
>> > +    */
>> > +   drvdata->edprsr = readl(drvdata->base + EDPRCR);
>> > +   drvdata->edprsr |= EDPRCR_COREPURQ;
>> > +   writel(drvdata->edprsr, drvdata->base + EDPRCR);
>>
>> Here ->edprsr is used but EDPRCR is accessed.  Is this intentional or a
>> copy/paste error?  The same is true for accesses in the out_powered_up section.
>
> Thanks for pointing out. This is a typo error and will fix.
>
>> > +
>> > +   /* Wait for CPU to be powered up (timeout~=32ms) */
>> > +   while (timeout--) {
>> > +           drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
>> > +           if (drvdata->edprsr & EDPRSR_PU)
>> > +                   break;
>> > +
>> > +           usleep_range(1000, 2000);
>> > +   }
>>
>> See if function readx_poll_timeout() can be used.
>
> Will use it.
>
>> > +
>> > +   /*
>> > +    * Unfortunately the CPU cannot be powered up, so return
>> > +    * back and later has no permission to access other
>> > +    * registers. For this case, should set 'idle_constraint'
>> > +    * to ensure CPU power domain is enabled!
>> > +    */
>> > +   if (!(drvdata->edprsr & EDPRSR_PU)) {
>> > +           pr_err("%s: power up request for CPU%d failed\n",
>> > +                   __func__, drvdata->cpu);
>> > +           goto out;
>> > +   }
>> > +
>> > +out_powered_up:
>> > +   debug_os_unlock(drvdata);
>> > +
>> > +   /*
>> > +    * At this point the CPU is powered up, so set the no powerdown
>> > +    * request bit so we don't lose power and emulate power down.
>> > +    */
>> > +   drvdata->edprsr = readl(drvdata->base + EDPRCR);
>> > +   drvdata->edprsr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;
>>
>> If we are here the core is already up.  Shouldn't we need to set
>> EDPRCR_CORENPDRQ only?
>
> Yeah. Will fix.

No - EDPRCR_COREPURQ and EDPRCR_CORENPDRQ have different semantics and purposes

EDPRCR_COREPURQ is in the debug power domain an is tied to an external
debug request that should be an input to the external (to the PE)
system power controller.
The requirement is that the system power controller powers up the core
domain and does not power it down while it remains asserted.

EDPRCR_CORENPDRQ is in the core power domain and thus to the specific
core only. This ensures that any power control software running on
that core should emulate a power down if this is set to one.

We cannot know the power control design of the system, so the safe
solution is to set both bits.

Mike
>
>> > +   writel(drvdata->edprsr, drvdata->base + EDPRCR);
>>
>> This section is a little racy - between the time the PU bit has been
>> checked and the time COREPDRQ has been flipped, the state of PU may have
>> changed.  You can probably get around this by checking edprsr.PU rigth here.  If
>> it is not set you go through the process again.  Note that doing this will
>> probably force a refactoring of the whole function.
>
> Agree. Will handle this.
>
> [...]
>
>> > +static ssize_t debug_func_knob_write(struct file *f,
>> > +           const char __user *buf, size_t count, loff_t *ppos)
>> > +{
>> > e   u8 on;
>> > +   int ret;
>> > +
>> > +   ret = kstrtou8_from_user(buf, count, 2, &on);
>> > +   if (ret)
>> > +           return ret;
>> > +
>> > +   mutex_lock(&debug_lock);
>> > +
>> > +   if (!on ^ debug_enable)
>> > +           goto out;
>>
>> I had to read this condition too many times - please refactor.
>
> Will do it.
>
>> > +
>> > +   if (on) {
>> > +           ret = debug_enable_func();
>> > +           if (ret) {
>> > +                   pr_err("%s: unable to disable debug function: %d\n",
>> > +                          __func__, ret);
>>
>> Based on the semantic this is the wrong error message.
>
> Yeah. Will fix.
>
>> > +                   goto err;
>> > +           }
>> > +   } else
>> > +           debug_disable_func();
>>
>> Did checkpatch.pl complain about extra curly braces?  If not please add them.
>
> checkpatch.pl doesn't report for this. Will add.
>
>> > +
>> > +   debug_enable = on;
>>
>> Here we can't set debug_enable if we just called debug_disable_func().  Maybe
>> I'm missing something.  If that's the case a comment in the code would be worth
>> it.
>
> After called debug_disable_func(), debug_enable is set to 0 (on = 0).
>
>> > +
>> > +out:
>> > +   ret = count;
>> > +err:
>> > +   mutex_unlock(&debug_lock);
>> > +   return ret;
>> > +}
>> > +
>> > +static ssize_t debug_func_knob_read(struct file *f,
>> > +           char __user *ubuf, size_t count, loff_t *ppos)
>> > +{
>> > +   char val[] = { '0' + debug_enable, '\n' };
>> > +
>> > +   return simple_read_from_buffer(ubuf, count, ppos, val, sizeof(val));
>>
>> Use the debug_lock to avoid race conditions.
>
> Will do it.
>
>> > +}
>> > +
>> > +static ssize_t debug_idle_constraint_write(struct file *f,
>> > +           const char __user *buf, size_t count, loff_t *ppos)
>> > +{
>> > +   int val;
>> > +   ssize_t ret;
>> > +
>> > +   ret = kstrtoint_from_user(buf, count, 10, &val);
>> > +   if (ret)
>> > +           return ret;
>> > +
>> > +   mutex_lock(&debug_lock);
>> > +   idle_constraint = val;
>> > +
>> > +   if (debug_enable)
>> > +           pm_qos_update_request(&debug_qos_req, idle_constraint);
>> > +
>> > +   mutex_unlock(&debug_lock);
>> > +   return count;
>> > +}
>> > +
>> > +static ssize_t debug_idle_constraint_read(struct file *f,
>> > +           char __user *ubuf, size_t count, loff_t *ppos)
>> > +{
>> > +   char buf[32];
>> > +   int len;
>> > +
>> > +   if (*ppos)
>> > +           return 0;
>> > +
>> > +   len = sprintf(buf, "%d\n", idle_constraint);
>> > +   return simple_read_from_buffer(ubuf, count, ppos, buf, len);
>>
>> Use the debug_lock to avoid race conditions.
>
> Will do it.
>
>> > +}
>> > +
>> > +static const struct file_operations debug_func_knob_fops = {
>> > +   .open   = simple_open,
>> > +   .read   = debug_func_knob_read,
>> > +   .write  = debug_func_knob_write,
>> > +};
>> > +
>> > +static const struct file_operations debug_idle_constraint_fops = {
>> > +   .open   = simple_open,
>> > +   .read   = debug_idle_constraint_read,
>> > +   .write  = debug_idle_constraint_write,
>> > +};
>> > +
>> > +static int debug_func_init(void)
>> > +{
>> > +   struct dentry *file;
>> > +   int ret;
>> > +
>> > +   /* Create debugfs node */
>> > +   debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL);
>> > +   if (!debug_debugfs_dir) {
>> > +           pr_err("%s: unable to create debugfs directory\n", __func__);
>> > +           return -ENOMEM;
>>
>> return PTR_ERR(debug_debugfs_dir);
>
> Here cannot use PTR_ERR(debug_debugfs_dir). If create debugfs failed
> the pointer is NULL value, so finally we will return zero value for
> PTR_ERR(debug_debugfs_dir).
>
> [...]
>
>> > +   }
>> > +
>> > +   file = debugfs_create_file("enable", S_IRUGO | S_IWUSR,
>> > +                   debug_debugfs_dir, NULL, &debug_func_knob_fops);
>> > +   if (!file) {
>> > +           pr_err("%s: unable to create enable knob file\n", __func__);
>> > +           ret = -ENOMEM;
>>
>> Same as above.
>>
>> > +           goto err;
>> > +   }
>> > +
>> > +   file = debugfs_create_file("idle_constraint", S_IRUGO | S_IWUSR,
>> > +                   debug_debugfs_dir, NULL, &debug_idle_constraint_fops);
>> > +   if (!file) {
>> > +           pr_err("%s: unable to create idle constraint file\n", __func__);
>> > +           ret = -ENOMEM;
>>
>> Same as above.
>>
>> > +           goto err;
>> > +   }
>> > +
>> > +   /* Use sysfs node to enable functionality */
>> > +   if (!debug_enable)
>> > +           return 0;
>> > +
>> > +   /* Enable debug module at boot time */
>> > +   ret = debug_enable_func();
>> > +   if (ret) {
>> > +           pr_err("%s: unable to disable debug function: %d\n",
>> > +                  __func__, ret);
>> > +           goto err;
>> > +   }
>>
>> Use the debug_lock to avoid race conditions.
>
> I'm struggling to understand what's race condition at here? The
> function pairs debug_func_init()/debug_func_exit() are used for
> module's probing and removing, so naturally module's probing and
> removing are sequential, right?
>
>> > +
>> > +   return 0;
>> > +
>> > +err:
>> > +   debugfs_remove_recursive(debug_debugfs_dir);
>> > +   return ret;
>> > +}
>> > +
>> > +static void debug_func_exit(void)
>> > +{
>> > +   debugfs_remove_recursive(debug_debugfs_dir);
>> > +
>> > +   /* Disable functionality if has enabled */
>> > +   if (debug_enable)
>> > +           debug_disable_func();
>> > +}
>> > +
>> > +static int debug_probe(struct amba_device *adev, const struct amba_id *id)
>> > +{
>> > +   void __iomem *base;
>> > +   struct device *dev = &adev->dev;
>> > +   struct debug_drvdata *drvdata;
>> > +   struct resource *res = &adev->res;
>> > +   struct device_node *np = adev->dev.of_node;
>> > +   int ret;
>> > +
>> > +   drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>> > +   if (!drvdata)
>> > +           return -ENOMEM;
>> > +
>> > +   drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
>> > +   if (per_cpu(debug_drvdata, drvdata->cpu)) {
>> > +           dev_err(dev, "CPU's drvdata has been initialized\n");
>>
>> Might be worth adding the CPU number in the error message.
>
> Yeah, will add it.
>
> [...]
>
>> This driver doesn't call the pm_runtime_put/get() operations needed to handle the
>> debug power domain.  See the other CoreSight drivers for details.
>
> Sure, will do it.
>
>> Also, from the conversation that followed the previous post we agreed that we wouldn't
>> deal with CPUidle issues in this driver.  We deal with the CPU power domain
>> using the EDPRCR register (like you did) and that's it.  System that don't honor that register
>> can use other (external) means to solve this.  As such please remove the
>> pm_qos_xyz() functions.
>
> From previous discussion, Mike reminds the CPU power domain design is
> quite SoC specific and usually the SoC has many different low power
> states, e.g. except CPU level and cluster level low power states, they
> also can define SoC level low power states. Any SoC with any power
> state is possible finally impact CPU power domain, so this is why I add
> this interface to let user can have the final decision based on their
> working platform.
>
> We can rely on "nohlt" and "cpuidle.off=1" in kernel command line to
> disable whole SoC low power states at boot time; or we can use sysfs
> node "echo 1 > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disble" to
> disable CPU low power states at runtime. But that means we need use
> different interfaces to control CPU power domain for booting and
> runtime, it's not nice for usage.
>
> So this is why add "idle_constraint" as a central place to control
> power domain for CPU debug purpose and I also think this is more
> friendly for hardware design, e.g. some platforms can enable partial
> low power states to save power and avoid overheat after using this
> driver.
>
> How about you think for this?
>
> Thanks,
> Leo Yan



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Blackburn Design Centre. UK
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v3 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl header
From: Geert Uytterhoeven @ 2017-03-29 14:59 UTC (permalink / raw)
  To: Chris Brandt
  Cc: Linus Walleij, Jacopo Mondi, Bjorn Andersson, Geert Uytterhoeven,
	Laurent Pinchart, Rob Herring, Mark Rutland, Russell King,
	Linux-Renesas, linux-gpio@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <SG2PR06MB11659AB956F83E8DCE2C9AA18A350@SG2PR06MB1165.apcprd06.prod.outlook.com>

Hi Chris,

On Wed, Mar 29, 2017 at 4:55 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Wednesday, March 29, 2017, Linus Walleij wrote:
>> On Fri, Mar 24, 2017 at 4:22 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
>> > +/*
>> > + * Flags used to ask software to drive the pin I/O direction
>> > +overriding the
>> > + * alternate function configuration.
>> > + * Some alternate functions require software to force I/O direction
>> > +of a pin,
>> > + * overriding the designated one.
>> > + * Refer to the HW manual to know when this flag shall be used.
>> > + */
>> > +#define SWIO_IN                        (1 << 4)
>> > +#define SWIO_OUT               (1 << 5)
>>
>> What is wrong in doing this with generic pin config using
>> PIN_CONFIG_INPUT_ENABLE and PIN_CONFIG_OUTPUT (ignoring the argument)?
>>
>> In the device tree use input-enable and add a new output-enable (with
>> unspecified value) with proper description and DT bindings?
>
> Again, that's probably fine. It seems we are still doing the same thing
> which is using the DT to pass extra config information to the driver.
> And, we can do whatever we want with that info.
>
>
>> And if you think these have no general applicability, by the end of the
>> day they are *still* pin config, not magic flags we can choose to toss in
>> with the muxing, so you can do what the Qualcomm driver does and add
>> custom pin configurations extending the generic pin config, see
>> drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
>> qcom,pull-up-strength etc.
>
> But, it seems that when you set a config option, it applies to everything
> in "pins"?
>
> I2C Example: (seem OK)
>         /* P1_6 = RIIC3SCL (bi dir) */
>         /* P1_7 = RIIC3SDA (bi dir) */
>         i2c3_pins: i2c3 {
>                 pins = <PIN(1, 6) | FUNC_1>,
>                        <PIN(1, 7) | FUNC_1>;
>                 bidirectional;
>         };

Correct.

> But, what do we do for Ethernet? All the pins are "normal" except just
> the MDIO pin needs to be bidirectional.
> That's the part I'm confused by.
> How do we flag that just the ET_MDIO needs "bidirectional"?

You add subnodes, cfr. arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts:

        avb_pins: avb {
                mux {
                        groups = "avb_link", "avb_phy_int", "avb_mdc",
                                 "avb_mii";
                        function = "avb";
                };

                pins_mdc {
                        groups = "avb_mdc";
                        drive-strength = <24>;
                };

                pins_mii_tx {
                        pins = "PIN_AVB_TX_CTL", "PIN_AVB_TXC", "PIN_AVB_TD0",
                               "PIN_AVB_TD1", "PIN_AVB_TD2", "PIN_AVB_TD3";
                        drive-strength = <12>;
                };
        };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply

* Re: [PATCH v5 6/9] coresight: add support for CPU debug module
From: Mike Leach @ 2017-03-29 15:17 UTC (permalink / raw)
  To: Leo Yan
  Cc: Suzuki K Poulose, Jonathan Corbet, Rob Herring, Mark Rutland,
	Wei Xu, Catalin Marinas, Will Deacon, Andy Gross, David Brown,
	Michael Turquette, Stephen Boyd, Mathieu Poirier, Guodong Xu,
	John Stultz, linux-doc, linux-kernel, devicetree,
	linux-arm-kernel, linux-arm-msm, linux-soc, linux-clk,
	Sudeep Holla <sudee>
In-Reply-To: <20170329030735.GA23889@leoy-linaro>

On 29 March 2017 at 04:07, Leo Yan <leo.yan@linaro.org> wrote:
> Hi Suzuki,
>
> On Mon, Mar 27, 2017 at 05:34:57PM +0100, Suzuki K Poulose wrote:
>> On 25/03/17 18:23, Leo Yan wrote:
>
> [...]
>
>> Leo,
>>
>> Thanks a lot for the quick rework. I don't fully understand (yet!) why we need the
>> idle_constraint. I will leave it for Sudeep to comment on it, as he is the expert
>> in that area. Some minor comments below.
>
> Thanks a lot for quick reviewing :)
>
>> >Signed-off-by: Leo Yan <leo.yan@linaro.org>
>> >---
>> > drivers/hwtracing/coresight/Kconfig               |  11 +
>> > drivers/hwtracing/coresight/Makefile              |   1 +
>> > drivers/hwtracing/coresight/coresight-cpu-debug.c | 704 ++++++++++++++++++++++
>> > 3 files changed, 716 insertions(+)
>> > create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c
>> >
>> >diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>> >index 130cb21..18d7931 100644
>> >--- a/drivers/hwtracing/coresight/Kconfig
>> >+++ b/drivers/hwtracing/coresight/Kconfig
>> >@@ -89,4 +89,15 @@ config CORESIGHT_STM
>> >       logging useful software events or data coming from various entities
>> >       in the system, possibly running different OSs
>> >
>> >+config CORESIGHT_CPU_DEBUG
>> >+    tristate "CoreSight CPU Debug driver"
>> >+    depends on ARM || ARM64
>> >+    depends on DEBUG_FS
>> >+    help
>> >+      This driver provides support for coresight debugging module. This
>> >+      is primarily used to dump sample-based profiling registers when
>> >+      system triggers panic, the driver will parse context registers so
>> >+      can quickly get to know program counter (PC), secure state,
>> >+      exception level, etc.
>>
>> May be we should mention/warn the user about the possible caveats of using
>> this feature to help him make a better decision ? And / Or we should add a documentation
>> for it. We have collected some real good information over the discussions and
>> it is a good idea to capture it somewhere.
>
> Sure, I will add a documentation for this.
>
> [...]
>
>> >+static struct pm_qos_request debug_qos_req;
>> >+static int idle_constraint = PM_QOS_DEFAULT_VALUE;
>> >+module_param(idle_constraint, int, 0600);
>> >+MODULE_PARM_DESC(idle_constraint, "Latency requirement in microseconds for CPU "
>> >+             "idle states (default is -1, which means have no limiation "
>> >+             "to CPU idle states; 0 means disabling all idle states; user "
>> >+             "can choose other platform dependent values so can disable "
>> >+             "specific idle states for the platform)");
>>
>> Correct me if I am wrong,
>>
>> All we want to do is disable the CPUIdle explicitly if the user knows that this
>> could be a problem to use CPU debug on his platform. So, in effect, we should
>> only be using idle_constraint = 0 or -1.
>>
>> In which case, we could make it easier for the user to tell us, either
>>
>>  0 - Don't do anything with CPUIdle (default)
>>  1 - Disable CPUIdle for me as I know the platform has issues with CPU debug and CPUidle.
>
> The reason for not using bool flag is: usually SoC may have many idle
> states, so if user wants to partially enable some states then can set
> the latency to constraint.
>
> But of course, we can change this to binary value as you suggested,
> this means turn on of turn off all states. The only one reason to use
> latency value is it is more friendly for hardware design, e.g. some
> platforms can enable partial states to save power and avoid overheat
> after using this driver.
>
> If you guys think this is a bit over design, I will follow up your
> suggestion. I also have some replying in Mathieu's reviewing, please
> help review as well.
>
>> than explaining the miscrosecond latency etc and make the appropriate calls underneath.
>> something like (not necessarily the same name) :
>>
>> module_param(broken_with_cpuidle, bool, 0600);
>> MODULE_PARAM_DESC(broken_with_cpuidle, "Specifies whether the CPU debug has issues with CPUIdle on"
>>                                      " the platform. Non-zero value implies CPUIdle has to be"
>>                                      " explicitly disabled.",);
>
> [...]
>
>> >+    /*
>> >+     * Unfortunately the CPU cannot be powered up, so return
>> >+     * back and later has no permission to access other
>> >+     * registers. For this case, should set 'idle_constraint'
>> >+     * to ensure CPU power domain is enabled!
>> >+     */
>> >+    if (!(drvdata->edprsr & EDPRSR_PU)) {
>> >+            pr_err("%s: power up request for CPU%d failed\n",
>> >+                    __func__, drvdata->cpu);
>> >+            goto out;
>> >+    }
>> >+
>> >+out_powered_up:
>> >+    debug_os_unlock(drvdata);
>>
>> Question: Do we need a matching debug_os_lock() once we are done ?
>
> I have checked ARM ARMv8, but there have no detailed description for
> this. I refered coresight-etmv4 code and Mike's pseudo code, ther have
> no debug_os_lock() related operations.
>
> Mike, Mathieu, could you also help confirm this?
>

Debug OS lock / unlock allows the power management code running on the
core to lock out the external debugger while the debug registers are
saved/restored during a core power event.

e.g. A sequence such as this might occur in a correctly programmed system....

debug_os_lock()
save_debug_regs() // visible from core power domain - incl breakpoints etc
save_etm_regs()
... // other stuff prior to core power down,
<power_down_core>

Followed by...

<power_up_core>
restore_etm_regs()
restore_debug_regs() // visible from core power domain - incl breakpoints etc
debug_os_unlock()

The value is 1 (locked) if cold resetting into AArch64 - it is
expected that some system software will set this to 0 as part of the
boot process.
The lock prevents write access to the external debug registers so we
need to clear it to set up the external debug registers we are using.
This suggests that it should be restored as we found it when done.

Mike

> [...]
>
>> >+static void debug_init_arch_data(void *info)
>> >+{
>> >+    struct debug_drvdata *drvdata = info;
>> >+    u32 mode, pcsr_offset;
>> >+
>> >+    CS_UNLOCK(drvdata->base);
>> >+
>> >+    debug_os_unlock(drvdata);
>> >+
>> >+    /* Read device info */
>> >+    drvdata->eddevid  = readl_relaxed(drvdata->base + EDDEVID);
>> >+    drvdata->eddevid1 = readl_relaxed(drvdata->base + EDDEVID1);
>>
>> As mentioned above, both of these registers are only need at init time to
>> figure out the flags we set here. So we could remove them.
>>
>> >+
>> >+    CS_LOCK(drvdata->base);
>> >+
>> >+    /* Parse implementation feature */
>> >+    mode = drvdata->eddevid & EDDEVID_PCSAMPLE_MODE;
>> >+    pcsr_offset = drvdata->eddevid1 & EDDEVID1_PCSR_OFFSET_MASK;
>>
>>
>> >+
>> >+    if (mode == EDDEVID_IMPL_NONE) {
>> >+            drvdata->edpcsr_present  = false;
>> >+            drvdata->edcidsr_present = false;
>> >+            drvdata->edvidsr_present = false;
>> >+    } else if (mode == EDDEVID_IMPL_EDPCSR) {
>> >+            drvdata->edpcsr_present  = true;
>> >+            drvdata->edcidsr_present = false;
>> >+            drvdata->edvidsr_present = false;
>> >+    } else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) {
>> >+            if (!IS_ENABLED(CONFIG_64BIT) &&
>> >+                    (pcsr_offset == EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32))
>> >+                    drvdata->edpcsr_present = false;
>> >+            else
>> >+                    drvdata->edpcsr_present = true;
>>
>> Sorry, I forgot why we do this check only in this mode. Shouldn't this be
>> common to all modes (of course which implies PCSR is present) ?
>
> No. PCSROffset is defined differently in ARMv7 and ARMv8; So finally we
> simplize PCSROffset value :
> 0000 - Sample offset applies based on the instruction state (indicated by PCSR[0])
> 0001 - No offset applies.
> 0010 - No offset applies, but do not use in AArch32 mode!
>
> So we need handle the corner case is when CPU runs AArch32 mode and
> PCSRoffset = 'b0010. Other cases the pcsr should be present.
>
> [...]
>
> Other suggestions are good for me, will take them in next version.
>
> Thanks,
> Leo Yan



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Blackburn Design Centre. UK

^ permalink raw reply

* RE: [PATCH v3 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl header
From: Chris Brandt @ 2017-03-29 15:18 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Linus Walleij, Jacopo Mondi, Bjorn Andersson, Geert Uytterhoeven,
	Laurent Pinchart, Rob Herring, Mark Rutland, Russell King,
	Linux-Renesas, linux-gpio@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CAMuHMdVx_fyGsw0p-s4ZAoe2k+r0RuwxjwNV_-8fXY7Yv8y3kQ@mail.gmail.com>

Hi Geert,

On Wednesday, March 29, 2017, Geert Uytterhoeven wrote:
> > But, what do we do for Ethernet? All the pins are "normal" except just
> > the MDIO pin needs to be bidirectional.
> > That's the part I'm confused by.
> > How do we flag that just the ET_MDIO needs "bidirectional"?
> 
> You add subnodes, cfr. arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts:
> 
>         avb_pins: avb {
>                 mux {
>                         groups = "avb_link", "avb_phy_int", "avb_mdc",
>                                  "avb_mii";
>                         function = "avb";
>                 };
> 
>                 pins_mdc {
>                         groups = "avb_mdc";
>                         drive-strength = <24>;
>                 };
> 
>                 pins_mii_tx {
>                         pins = "PIN_AVB_TX_CTL", "PIN_AVB_TXC",
> "PIN_AVB_TD0",
>                                "PIN_AVB_TD1", "PIN_AVB_TD2",
> "PIN_AVB_TD3";
>                         drive-strength = <12>;
>                 };
>         };

Oh, so there is a way!

Thank you.


So, for clarity:

	/* Ethernet */
	ether_pins: ether {
		/* Ethernet on Ports 1,2,3,5 */
		mux {
			pins = <PIN(1, 14) | FUNC_4)>, /* P1_14 = ET_COL */
				<PIN(5, 9) | FUNC_2)>,	/* P5_9 = ET_MDC */
				<PIN(3, 4) | FUNC_2)>,	/* P3_4 = ET_RXCLK */
				<PIN(3, 5) | FUNC_2)>,	/* P3_5 = ET_RXER */
				<PIN(3, 6) | FUNC_2)>,	/* P3_6 = ET_RXDV */
				<PIN(2, 0) | FUNC_2)>,	/* P2_0 = ET_TXCLK */
				<PIN(2, 1) | FUNC_2)>,	/* P2_1 = ET_TXER */
				<PIN(2, 2) | FUNC_2)>,	/* P2_2 = ET_TXEN */
				<PIN(2, 3) | FUNC_2)>,	/* P2_3 = ET_CRS */
				<PIN(2, 4) | FUNC_2)>,	/* P2_4 = ET_TXD0 */
				<PIN(2, 5) | FUNC_2)>,	/* P2_5 = ET_TXD1 */
				<PIN(2, 6) | FUNC_2)>,	/* P2_6 = ET_TXD2 */
				<PIN(2, 7) | FUNC_2)>,	/* P2_7 = ET_TXD3 */
				<PIN(2, 8) | FUNC_2)>,	/* P2_8 = ET_RXD0 */
				<PIN(2, 9) | FUNC_2)>,	/* P2_9 = ET_RXD1 */
				<PIN(2, 10) | FUNC_2)>, /* P2_10 = ET_RXD2 */
				<PIN(2, 11) | FUNC_2)>; /* P2_11 = ET_RXD3 */
		};
		pins_bidir {
			pins = <PIN(3, 3) | FUNC_2)>,	/* P3_3 = ET_MDIO */
			bidirectional;
		};
	};



Chris


^ permalink raw reply

* Re: [PATCH v3 2/5] clk: sunxi-ng: add support for PRCM CCUs
From: Icenowy Zheng @ 2017-03-29 15:27 UTC (permalink / raw)
  To: Michael Turquette, Maxime Ripard, Rob Herring, Chen-Yu Tsai
  Cc: linux-clk-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Icenowy Zheng
In-Reply-To: <20170329104246.34204-3-icenowy-h8G6r0blFSE@public.gmane.org>

On Wed, 29 Mar 2017 18:42:43 +0800
Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org> wrote:

> From: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
> 
> SoCs after A31 has a clock controller module in the PRCM part.
> 
> Support the clock controller module on H3/5 and A64 now.
> 
> Signed-off-by: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
> ---
> Changes in v3:
> - Change osc32000 mux to iosc, as its frequency varies between SoCs.
> (And none of them is really 32000Hz)
> Changes in v2:
> - Replace all sun6i to sun8i, as this driver currently doesn't really
>   support sun6i(A31).
> - Add osc32000 mux for ar100 clk. (Note: the frequency is proven to
> be wrong during the development of PATCH v3).
> - Rename some clocks.
> - Add gate of TWD (Trusted Watchdog). There's no reset for TWD.
> - Removed reset of PIO, which doesn't exist really.
> 
>  drivers/clk/sunxi-ng/Kconfig            |   6 +
>  drivers/clk/sunxi-ng/Makefile           |   1 +
>  drivers/clk/sunxi-ng/ccu-sun8i-r.c      | 211
> ++++++++++++++++++++++++++++++++
> drivers/clk/sunxi-ng/ccu-sun8i-r.h      |  27 ++++
> include/dt-bindings/clock/sun8i-r-ccu.h |  59 +++++++++
> include/dt-bindings/reset/sun8i-r-ccu.h |  53 ++++++++ 6 files
> changed, 357 insertions(+) create mode 100644
> drivers/clk/sunxi-ng/ccu-sun8i-r.c create mode 100644
> drivers/clk/sunxi-ng/ccu-sun8i-r.h create mode 100644
> include/dt-bindings/clock/sun8i-r-ccu.h create mode 100644
> include/dt-bindings/reset/sun8i-r-ccu.h
> 
> diff --git a/drivers/clk/sunxi-ng/Kconfig
> b/drivers/clk/sunxi-ng/Kconfig index 8af8f4be8e3b..fbd3f8cd5c22 100644
> --- a/drivers/clk/sunxi-ng/Kconfig
> +++ b/drivers/clk/sunxi-ng/Kconfig
> @@ -151,4 +151,10 @@ config SUN9I_A80_CCU
>  	default MACH_SUN9I
>  	depends on MACH_SUN9I || COMPILE_TEST
>  
> +config SUN8I_R_CCU
> +	bool "Support for Allwinner SoCs' PRCM CCUs"
> +	select SUNXI_CCU_DIV
> +	select SUNXI_CCU_GATE
> +	default MACH_SUN8I || (ARCH_SUNXI && ARM64)
> +
>  endif
> diff --git a/drivers/clk/sunxi-ng/Makefile
> b/drivers/clk/sunxi-ng/Makefile index 6feaac0c5600..0ec02fe14c50
> 100644 --- a/drivers/clk/sunxi-ng/Makefile
> +++ b/drivers/clk/sunxi-ng/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_SUN8I_A23_CCU)	+=
> ccu-sun8i-a23.o obj-$(CONFIG_SUN8I_A33_CCU)	+= ccu-sun8i-a33.o
>  obj-$(CONFIG_SUN8I_H3_CCU)	+= ccu-sun8i-h3.o
>  obj-$(CONFIG_SUN8I_V3S_CCU)	+= ccu-sun8i-v3s.o
> +obj-$(CONFIG_SUN8I_R_CCU)	+= ccu-sun8i-r.o
>  obj-$(CONFIG_SUN9I_A80_CCU)	+= ccu-sun9i-a80.o
>  obj-$(CONFIG_SUN9I_A80_CCU)	+= ccu-sun9i-a80-de.o
>  obj-$(CONFIG_SUN9I_A80_CCU)	+= ccu-sun9i-a80-usb.o
> diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-r.c
> b/drivers/clk/sunxi-ng/ccu-sun8i-r.c new file mode 100644
> index 000000000000..ddbb34018086
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/ccu-sun8i-r.c
> @@ -0,0 +1,211 @@
> +/*
> + * Copyright (c) 2016 Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
> + *
> + * This software is licensed under the terms of the GNU General
> Public
> + * License version 2, as published by the Free Software Foundation,
> and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +
> +#include "ccu_common.h"
> +#include "ccu_reset.h"
> +
> +#include "ccu_div.h"
> +#include "ccu_gate.h"
> +#include "ccu_mp.h"
> +#include "ccu_nm.h"
> +
> +#include "ccu-sun8i-r.h"
> +
> +static const char * const ar100_parents[] = { "osc32k", "osc24M",
> +					     "pll-periph0", "iosc" };
> +
> +static struct ccu_div ar100_clk = {
> +	.div		= _SUNXI_CCU_DIV_FLAGS(4, 2,
> CLK_DIVIDER_POWER_OF_TWO), +
> +	.mux		= {
> +		.shift	= 16,
> +		.width	= 2,
> +
> +		.variable_prediv	= {
> +			.index	= 2,
> +			.shift	= 8,
> +			.width	= 5,
> +		},
> +	},
> +
> +	.common		= {
> +		.reg		= 0x00,
> +		.features	= CCU_FEATURE_VARIABLE_PREDIV,
> +		.hw.init	= CLK_HW_INIT_PARENTS("ar100",
> +						      ar100_parents,
> +						      &ccu_div_ops,
> +						      0),
> +	},
> +};
> +
> +static CLK_FIXED_FACTOR(ahb0_clk, "ahb0", "ar100", 1, 1, 0);
> +
> +static struct ccu_div apb0_clk = {
> +	.div		= _SUNXI_CCU_DIV_FLAGS(0, 2,
> CLK_DIVIDER_POWER_OF_TWO), +
> +	.common		= {
> +		.reg		= 0x0c,
> +		.hw.init	= CLK_HW_INIT("apb0",
> +					      "ahb0",
> +					      &ccu_div_ops,
> +					      0),
> +	},
> +};
> +
> +static SUNXI_CCU_GATE(apb0_pio_clk,	"apb0-pio",	"apb0",
> +		      0x28, BIT(0), 0);
> +static SUNXI_CCU_GATE(apb0_ir_clk,	"apb0-ir",	"apb0",
> +		      0x28, BIT(1), 0);
> +static
> SUNXI_CCU_GATE(apb0_timer_clk,	"apb0-timer",	"apb0",
> +		      0x28, BIT(2), 0);
> +static SUNXI_CCU_GATE(apb0_rsb_clk,	"apb0-rsb",	"apb0",
> +		      0x28, BIT(3), 0);
> +static
> SUNXI_CCU_GATE(apb0_uart_clk,	"apb0-uart",	"apb0",
> +		      0x28, BIT(4), 0);
> +static SUNXI_CCU_GATE(apb0_i2c_clk,	"apb0-i2c",	"apb0",
> +		      0x28, BIT(6), 0);
> +static SUNXI_CCU_GATE(apb0_twd_clk,	"apb0-twd",	"apb0",
> +		      0x28, BIT(7), 0);
> +
> +static const char * const r_mod0_default_parents[] = { "osc32K",
> "osc24M" }; +static SUNXI_CCU_MP_WITH_MUX_GATE(ir_clk, "ir",
> +				  r_mod0_default_parents, 0x54,
> +				  0, 4,		/* M */
> +				  16, 2,	/* P */
> +				  24, 2,	/* mux */
> +				  BIT(31),	/* gate */
> +				  0);
> +
> +static struct ccu_common *sun8i_h3_r_ccu_clks[] = {
> +	&ar100_clk.common,
> +	&apb0_clk.common,
> +	&apb0_pio_clk.common,
> +	&apb0_ir_clk.common,
> +	&apb0_timer_clk.common,
> +	&apb0_uart_clk.common,
> +	&apb0_i2c_clk.common,
I'm sorry but I missed something here:

+	&apb0_twd_clk.common,

> +	&ir_clk.common,
> +};
> +
> +static struct ccu_common *sun50i_a64_r_ccu_clks[] = {
> +	&ar100_clk.common,
> +	&apb0_clk.common,
> +	&apb0_pio_clk.common,
> +	&apb0_ir_clk.common,
> +	&apb0_timer_clk.common,
> +	&apb0_rsb_clk.common,
> +	&apb0_uart_clk.common,
> +	&apb0_i2c_clk.common,
+	&apb0_twd_clk.common,
> +	&ir_clk.common,
> +};
> +
> +static struct clk_hw_onecell_data sun8i_h3_r_hw_clks = {
> +	.hws	= {
> +		[CLK_AR100]		= &ar100_clk.common.hw,
> +		[CLK_AHB0]		= &ahb0_clk.hw,
> +		[CLK_APB0]		= &apb0_clk.common.hw,
> +		[CLK_APB0_PIO]		=
> &apb0_pio_clk.common.hw,
> +		[CLK_APB0_IR]		=
> &apb0_ir_clk.common.hw,
> +		[CLK_APB0_TIMER]	= &apb0_timer_clk.common.hw,
> +		[CLK_APB0_UART]		=
> &apb0_uart_clk.common.hw,
> +		[CLK_APB0_I2C]		=
> &apb0_i2c_clk.common.hw,
> +		[CLK_APB0_TWD]		=
> &apb0_twd_clk.common.hw,
> +		[CLK_IR]		= &ir_clk.common.hw,
> +	},
> +	.num	= CLK_NUMBER,
> +};
> +
> +static struct clk_hw_onecell_data sun50i_a64_r_hw_clks = {
> +	.hws	= {
> +		[CLK_AR100]		= &ar100_clk.common.hw,
> +		[CLK_AHB0]		= &ahb0_clk.hw,
> +		[CLK_APB0]		= &apb0_clk.common.hw,
> +		[CLK_APB0_PIO]		=
> &apb0_pio_clk.common.hw,
> +		[CLK_APB0_IR]		=
> &apb0_ir_clk.common.hw,
> +		[CLK_APB0_TIMER]	= &apb0_timer_clk.common.hw,
> +		[CLK_APB0_RSB]		=
> &apb0_rsb_clk.common.hw,
> +		[CLK_APB0_UART]		=
> &apb0_uart_clk.common.hw,
> +		[CLK_APB0_I2C]		=
> &apb0_i2c_clk.common.hw,
> +		[CLK_APB0_TWD]		=
> &apb0_twd_clk.common.hw,
> +		[CLK_IR]		= &ir_clk.common.hw,
> +	},
> +	.num	= CLK_NUMBER,
> +};
> +
> +static struct ccu_reset_map sun8i_h3_r_ccu_resets[] = {
> +	[RST_APB0_IR]		=  { 0xb0, BIT(1) },
> +	[RST_APB0_TIMER]	=  { 0xb0, BIT(2) },
> +	[RST_APB0_UART]		=  { 0xb0, BIT(4) },
> +	[RST_APB0_I2C]		=  { 0xb0, BIT(6) },
> +};
> +
> +static struct ccu_reset_map sun50i_a64_r_ccu_resets[] = {
> +	[RST_APB0_IR]		=  { 0xb0, BIT(1) },
> +	[RST_APB0_TIMER]	=  { 0xb0, BIT(2) },
> +	[RST_APB0_RSB]		=  { 0xb0, BIT(3) },
> +	[RST_APB0_UART]		=  { 0xb0, BIT(4) },
> +	[RST_APB0_I2C]		=  { 0xb0, BIT(6) },
> +};
> +
> +static const struct sunxi_ccu_desc sun8i_h3_r_ccu_desc = {
> +	.ccu_clks	= sun8i_h3_r_ccu_clks,
> +	.num_ccu_clks	= ARRAY_SIZE(sun8i_h3_r_ccu_clks),
> +
> +	.hw_clks	= &sun8i_h3_r_hw_clks,
> +
> +	.resets		= sun8i_h3_r_ccu_resets,
> +	.num_resets	= ARRAY_SIZE(sun8i_h3_r_ccu_resets),
> +};
> +
> +static const struct sunxi_ccu_desc sun50i_a64_r_ccu_desc = {
> +	.ccu_clks	= sun50i_a64_r_ccu_clks,
> +	.num_ccu_clks	= ARRAY_SIZE(sun50i_a64_r_ccu_clks),
> +
> +	.hw_clks	= &sun50i_a64_r_hw_clks,
> +
> +	.resets		= sun50i_a64_r_ccu_resets,
> +	.num_resets	= ARRAY_SIZE(sun50i_a64_r_ccu_resets),
> +};
> +
> +static void __init sunxi_r_ccu_init(struct device_node *node,
> +				    const struct sunxi_ccu_desc
> *desc) +{
> +	void __iomem *reg;
> +
> +	reg = of_io_request_and_map(node, 0,
> of_node_full_name(node));
> +	if (IS_ERR(reg)) {
> +		pr_err("%s: Could not map the clock registers\n",
> +		       of_node_full_name(node));
> +		return;
> +	}
> +
> +	sunxi_ccu_probe(node, reg, desc);
> +}
> +
> +static void __init sun8i_h3_r_ccu_setup(struct device_node *node)
> +{
> +	sunxi_r_ccu_init(node, &sun8i_h3_r_ccu_desc);
> +}
> +CLK_OF_DECLARE(sun8i_h3_r_ccu, "allwinner,sun8i-h3-r-ccu",
> +	       sun8i_h3_r_ccu_setup);
> +
> +static void __init sun50i_a64_r_ccu_setup(struct device_node *node)
> +{
> +	sunxi_r_ccu_init(node, &sun50i_a64_r_ccu_desc);
> +}
> +CLK_OF_DECLARE(sun50i_a64_r_ccu, "allwinner,sun50i-a64-r-ccu",
> +	       sun50i_a64_r_ccu_setup);
> diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-r.h
> b/drivers/clk/sunxi-ng/ccu-sun8i-r.h new file mode 100644
> index 000000000000..eaa431fd1d8f
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/ccu-sun8i-r.h
> @@ -0,0 +1,27 @@
> +/*
> + * Copyright 2016 Icenowy <icenowy-ymACFijhrKM@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or
> modify
> + * it under the terms of the GNU General Public License as published
> by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef _CCU_SUN8I_R_H
> +#define _CCU_SUN8I_R_H_
> +
> +#include <dt-bindings/clock/sun8i-r-ccu.h>
> +#include <dt-bindings/reset/sun8i-r-ccu.h>
> +
> +/* AHB/APB bus clocks are not exported */
> +#define CLK_AHB0	1
> +#define CLK_APB0	2
> +
> +#define CLK_NUMBER	(CLK_APB0_TWD + 1)
> +
> +#endif /* _CCU_SUN8I_R_H */
> diff --git a/include/dt-bindings/clock/sun8i-r-ccu.h
> b/include/dt-bindings/clock/sun8i-r-ccu.h new file mode 100644
> index 000000000000..779d20aa0d05
> --- /dev/null
> +++ b/include/dt-bindings/clock/sun8i-r-ccu.h
> @@ -0,0 +1,59 @@
> +/*
> + * Copyright (c) 2016 Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + *  a) This file is free software; you can redistribute it and/or
> + *     modify it under the terms of the GNU General Public License as
> + *     published by the Free Software Foundation; either version 2
> of the
> + *     License, or (at your option) any later version.
> + *
> + *     This file is distributed in the hope that it will be useful,
> + *     but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *     GNU General Public License for more details.
> + *
> + * Or, alternatively,
> + *
> + *  b) Permission is hereby granted, free of charge, to any person
> + *     obtaining a copy of this software and associated documentation
> + *     files (the "Software"), to deal in the Software without
> + *     restriction, including without limitation the rights to use,
> + *     copy, modify, merge, publish, distribute, sublicense, and/or
> + *     sell copies of the Software, and to permit persons to whom the
> + *     Software is furnished to do so, subject to the following
> + *     conditions:
> + *
> + *     The above copyright notice and this permission notice shall be
> + *     included in all copies or substantial portions of the
> Software.
> + *
> + *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
> KIND,
> + *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
> WARRANTIES
> + *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + *     OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef _DT_BINDINGS_CLK_SUN8I_R_CCU_H_
> +#define _DT_BINDINGS_CLK_SUN8I_R_CCU_H_
> +
> +#define CLK_AR100		0
> +
> +#define CLK_APB0_PIO		3
> +#define CLK_APB0_IR		4
> +#define CLK_APB0_TIMER		5
> +#define CLK_APB0_RSB		6
> +#define CLK_APB0_UART		7
> +/* 8 is reserved for CLK_APB0_W1 on A31 */
> +#define CLK_APB0_I2C		9
> +#define CLK_APB0_TWD		10
> +
> +#define CLK_IR			11
> +
> +#endif /* _DT_BINDINGS_CLK_SUN8I_R_CCU_H_ */
> diff --git a/include/dt-bindings/reset/sun8i-r-ccu.h
> b/include/dt-bindings/reset/sun8i-r-ccu.h new file mode 100644
> index 000000000000..4ba64f3d6fc9
> --- /dev/null
> +++ b/include/dt-bindings/reset/sun8i-r-ccu.h
> @@ -0,0 +1,53 @@
> +/*
> + * Copyright (C) 2016 Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + *  a) This file is free software; you can redistribute it and/or
> + *     modify it under the terms of the GNU General Public License as
> + *     published by the Free Software Foundation; either version 2
> of the
> + *     License, or (at your option) any later version.
> + *
> + *     This file is distributed in the hope that it will be useful,
> + *     but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *     GNU General Public License for more details.
> + *
> + * Or, alternatively,
> + *
> + *  b) Permission is hereby granted, free of charge, to any person
> + *     obtaining a copy of this software and associated documentation
> + *     files (the "Software"), to deal in the Software without
> + *     restriction, including without limitation the rights to use,
> + *     copy, modify, merge, publish, distribute, sublicense, and/or
> + *     sell copies of the Software, and to permit persons to whom the
> + *     Software is furnished to do so, subject to the following
> + *     conditions:
> + *
> + *     The above copyright notice and this permission notice shall be
> + *     included in all copies or substantial portions of the
> Software.
> + *
> + *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
> KIND,
> + *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
> WARRANTIES
> + *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + *     OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef _DT_BINDINGS_RST_SUN8I_R_CCU_H_
> +#define _DT_BINDINGS_RST_SUN8I_R_CCU_H_
> +
> +#define RST_APB0_IR		0
> +#define RST_APB0_TIMER		1
> +#define RST_APB0_RSB		2
> +#define RST_APB0_UART		3
> +/* 4 is reserved for RST_APB0_W1 on A31 */
> +#define RST_APB0_I2C		5
> +
> +#endif /* _DT_BINDINGS_RST_SUN8I_R_CCU_H_ */

^ permalink raw reply

* RE: [PATCH v3 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl header
From: Chris Brandt @ 2017-03-29 15:39 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Linus Walleij, Jacopo Mondi, Bjorn Andersson, Geert Uytterhoeven,
	Laurent Pinchart, Rob Herring, Mark Rutland, Russell King,
	Linux-Renesas, linux-gpio@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <SG2PR06MB1165F5D4CFC50CADC609451C8A350@SG2PR06MB1165.apcprd06.prod.outlook.com>

On Wednesday, March 29, 2017, Chris Brandt wrote:
> On Wednesday, March 29, 2017, Geert Uytterhoeven wrote:
> > > But, what do we do for Ethernet? All the pins are "normal" except
> > > just the MDIO pin needs to be bidirectional.
> > > That's the part I'm confused by.
> > > How do we flag that just the ET_MDIO needs "bidirectional"?
> >
> > You add subnodes, cfr. arch/arm64/boot/dts/renesas/r8a7795-salvator-
> x.dts:
> >
> >         avb_pins: avb {
> >                 mux {
> >                         groups = "avb_link", "avb_phy_int", "avb_mdc",
> >                                  "avb_mii";
> >                         function = "avb";
> >                 };
> >
> >                 pins_mdc {
> >                         groups = "avb_mdc";
> >                         drive-strength = <24>;
> >                 };
> >
> >                 pins_mii_tx {
> >                         pins = "PIN_AVB_TX_CTL", "PIN_AVB_TXC",
> > "PIN_AVB_TD0",
> >                                "PIN_AVB_TD1", "PIN_AVB_TD2",
> > "PIN_AVB_TD3";
> >                         drive-strength = <12>;
> >                 };
> >         };
> 
> Oh, so there is a way!
> 
> Thank you.
> 
> 
> So, for clarity:


Actually, Linus's request was to use "pinmux", not "pins".

So, it should be:

	/* P1_6 = RIIC3SCL (bi dir) */
	/* P1_7 = RIIC3SDA (bi dir) */
	i2c3_pins: i2c3 {
		pinmux = <PIN(1, 6) | FUNC_1>,
		         <PIN(1, 7) | FUNC_1>;
		bidirectional;
	};

 
	/* Ethernet */
	ether_pins: ether {
		/* Ethernet on Ports 1,2,3,5 */
		mux {
			pinmux = <PIN(1, 14) | FUNC_4)>, /* P1_14 = ET_COL */
				<PIN(5, 9) | FUNC_2)>,	/* P5_9 = ET_MDC */
				<PIN(3, 4) | FUNC_2)>,	/* P3_4 = ET_RXCLK */
				<PIN(3, 5) | FUNC_2)>,	/* P3_5 = ET_RXER */
				<PIN(3, 6) | FUNC_2)>,	/* P3_6 = ET_RXDV */
				<PIN(2, 0) | FUNC_2)>,	/* P2_0 = ET_TXCLK */
				<PIN(2, 1) | FUNC_2)>,	/* P2_1 = ET_TXER */
				<PIN(2, 2) | FUNC_2)>,	/* P2_2 = ET_TXEN */
				<PIN(2, 3) | FUNC_2)>,	/* P2_3 = ET_CRS */
				<PIN(2, 4) | FUNC_2)>,	/* P2_4 = ET_TXD0 */
				<PIN(2, 5) | FUNC_2)>,	/* P2_5 = ET_TXD1 */
				<PIN(2, 6) | FUNC_2)>,	/* P2_6 = ET_TXD2 */
				<PIN(2, 7) | FUNC_2)>,	/* P2_7 = ET_TXD3 */
				<PIN(2, 8) | FUNC_2)>,	/* P2_8 = ET_RXD0 */
				<PIN(2, 9) | FUNC_2)>,	/* P2_9 = ET_RXD1 */
				<PIN(2, 10) | FUNC_2)>, /* P2_10 = ET_RXD2 */
				<PIN(2, 11) | FUNC_2)>; /* P2_11 = ET_RXD3 */
		};
		pins_bidir {
			pinmux = <PIN(3, 3) | FUNC_2)>,	/* P3_3 = ET_MDIO */
			bidirectional;
		};
	};


  NOTE: "FUNC_2" can just be "2" as per Geert's request.



Chris



^ permalink raw reply

* Re: [PATCH v5 6/9] coresight: add support for CPU debug module
From: Mathieu Poirier @ 2017-03-29 15:41 UTC (permalink / raw)
  To: Leo Yan
  Cc: Mark Rutland, linux-doc, Catalin Marinas, Michael Turquette,
	Will Deacon, David Brown, linux-clk, Jonathan Corbet, Wei Xu,
	Andy Gross, mike.leach, devicetree, Suzuki K Poulose,
	linux-arm-msm, Rob Herring, John Stultz, linux-soc,
	linux-arm-kernel, Guodong Xu, Stephen Boyd, linux-kernel,
	sudeep.holla
In-Reply-To: <20170329030735.GA23889@leoy-linaro>

On Wed, Mar 29, 2017 at 11:07:35AM +0800, Leo Yan wrote:
> Hi Suzuki,
> 
> On Mon, Mar 27, 2017 at 05:34:57PM +0100, Suzuki K Poulose wrote:
> > On 25/03/17 18:23, Leo Yan wrote:
> 
> [...]
> 
> > Leo,
> > 
> > Thanks a lot for the quick rework. I don't fully understand (yet!) why we need the
> > idle_constraint. I will leave it for Sudeep to comment on it, as he is the expert
> > in that area. Some minor comments below.
> 
> Thanks a lot for quick reviewing :)
> 
> > >Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > >---
> > > drivers/hwtracing/coresight/Kconfig               |  11 +
> > > drivers/hwtracing/coresight/Makefile              |   1 +
> > > drivers/hwtracing/coresight/coresight-cpu-debug.c | 704 ++++++++++++++++++++++
> > > 3 files changed, 716 insertions(+)
> > > create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c
> > >
> > >diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> > >index 130cb21..18d7931 100644
> > >--- a/drivers/hwtracing/coresight/Kconfig
> > >+++ b/drivers/hwtracing/coresight/Kconfig
> > >@@ -89,4 +89,15 @@ config CORESIGHT_STM
> > > 	  logging useful software events or data coming from various entities
> > > 	  in the system, possibly running different OSs
> > >
> > >+config CORESIGHT_CPU_DEBUG
> > >+	tristate "CoreSight CPU Debug driver"
> > >+	depends on ARM || ARM64
> > >+	depends on DEBUG_FS
> > >+	help
> > >+	  This driver provides support for coresight debugging module. This
> > >+	  is primarily used to dump sample-based profiling registers when
> > >+	  system triggers panic, the driver will parse context registers so
> > >+	  can quickly get to know program counter (PC), secure state,
> > >+	  exception level, etc.
> > 
> > May be we should mention/warn the user about the possible caveats of using
> > this feature to help him make a better decision ? And / Or we should add a documentation
> > for it. We have collected some real good information over the discussions and
> > it is a good idea to capture it somewhere.
> 
> Sure, I will add a documentation for this.
> 
> [...]
> 
> > >+static struct pm_qos_request debug_qos_req;
> > >+static int idle_constraint = PM_QOS_DEFAULT_VALUE;
> > >+module_param(idle_constraint, int, 0600);
> > >+MODULE_PARM_DESC(idle_constraint, "Latency requirement in microseconds for CPU "
> > >+		 "idle states (default is -1, which means have no limiation "
> > >+		 "to CPU idle states; 0 means disabling all idle states; user "
> > >+		 "can choose other platform dependent values so can disable "
> > >+		 "specific idle states for the platform)");
> > 
> > Correct me if I am wrong,
> > 
> > All we want to do is disable the CPUIdle explicitly if the user knows that this
> > could be a problem to use CPU debug on his platform. So, in effect, we should
> > only be using idle_constraint = 0 or -1.
> > 
> > In which case, we could make it easier for the user to tell us, either
> > 
> >  0 - Don't do anything with CPUIdle (default)
> >  1 - Disable CPUIdle for me as I know the platform has issues with CPU debug and CPUidle.
> 
> The reason for not using bool flag is: usually SoC may have many idle
> states, so if user wants to partially enable some states then can set
> the latency to constraint.
> 
> But of course, we can change this to binary value as you suggested,
> this means turn on of turn off all states. The only one reason to use
> latency value is it is more friendly for hardware design, e.g. some
> platforms can enable partial states to save power and avoid overheat
> after using this driver.
> 
> If you guys think this is a bit over design, I will follow up your
> suggestion. I also have some replying in Mathieu's reviewing, please
> help review as well.
> 
> > than explaining the miscrosecond latency etc and make the appropriate calls underneath.
> > something like (not necessarily the same name) :
> > 
> > module_param(broken_with_cpuidle, bool, 0600);
> > MODULE_PARAM_DESC(broken_with_cpuidle, "Specifies whether the CPU debug has issues with CPUIdle on"
> > 				       " the platform. Non-zero value implies CPUIdle has to be"
> > 				       " explicitly disabled.",);
> 
> [...]
> 
> > >+	/*
> > >+	 * Unfortunately the CPU cannot be powered up, so return
> > >+	 * back and later has no permission to access other
> > >+	 * registers. For this case, should set 'idle_constraint'
> > >+	 * to ensure CPU power domain is enabled!
> > >+	 */
> > >+	if (!(drvdata->edprsr & EDPRSR_PU)) {
> > >+		pr_err("%s: power up request for CPU%d failed\n",
> > >+			__func__, drvdata->cpu);
> > >+		goto out;
> > >+	}
> > >+
> > >+out_powered_up:
> > >+	debug_os_unlock(drvdata);
> > 
> > Question: Do we need a matching debug_os_lock() once we are done ?
> 
> I have checked ARM ARMv8, but there have no detailed description for
> this. I refered coresight-etmv4 code and Mike's pseudo code, ther have
> no debug_os_lock() related operations.
> 
> Mike, Mathieu, could you also help confirm this?

I'm not strongly opiniated on the usage of the OS lock, hence being a little
nonchalent in the coresight-etmv4 driver.   

> 
> [...]
> 
> > >+static void debug_init_arch_data(void *info)
> > >+{
> > >+	struct debug_drvdata *drvdata = info;
> > >+	u32 mode, pcsr_offset;
> > >+
> > >+	CS_UNLOCK(drvdata->base);
> > >+
> > >+	debug_os_unlock(drvdata);
> > >+
> > >+	/* Read device info */
> > >+	drvdata->eddevid  = readl_relaxed(drvdata->base + EDDEVID);
> > >+	drvdata->eddevid1 = readl_relaxed(drvdata->base + EDDEVID1);
> > 
> > As mentioned above, both of these registers are only need at init time to
> > figure out the flags we set here. So we could remove them.
> > 
> > >+
> > >+	CS_LOCK(drvdata->base);
> > >+
> > >+	/* Parse implementation feature */
> > >+	mode = drvdata->eddevid & EDDEVID_PCSAMPLE_MODE;
> > >+	pcsr_offset = drvdata->eddevid1 & EDDEVID1_PCSR_OFFSET_MASK;
> > 
> > 
> > >+
> > >+	if (mode == EDDEVID_IMPL_NONE) {
> > >+		drvdata->edpcsr_present  = false;
> > >+		drvdata->edcidsr_present = false;
> > >+		drvdata->edvidsr_present = false;
> > >+	} else if (mode == EDDEVID_IMPL_EDPCSR) {
> > >+		drvdata->edpcsr_present  = true;
> > >+		drvdata->edcidsr_present = false;
> > >+		drvdata->edvidsr_present = false;
> > >+	} else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) {
> > >+		if (!IS_ENABLED(CONFIG_64BIT) &&
> > >+			(pcsr_offset == EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32))
> > >+			drvdata->edpcsr_present = false;
> > >+		else
> > >+			drvdata->edpcsr_present = true;
> > 
> > Sorry, I forgot why we do this check only in this mode. Shouldn't this be
> > common to all modes (of course which implies PCSR is present) ?
> 
> No. PCSROffset is defined differently in ARMv7 and ARMv8; So finally we
> simplize PCSROffset value :
> 0000 - Sample offset applies based on the instruction state (indicated by PCSR[0])
> 0001 - No offset applies.
> 0010 - No offset applies, but do not use in AArch32 mode!
> 
> So we need handle the corner case is when CPU runs AArch32 mode and
> PCSRoffset = 'b0010. Other cases the pcsr should be present.
> 
> [...]
> 
> Other suggestions are good for me, will take them in next version.
> 
> Thanks,
> Leo Yan

^ permalink raw reply

* Re: [PATCH v5 6/9] coresight: add support for CPU debug module
From: Suzuki K Poulose @ 2017-03-29 15:50 UTC (permalink / raw)
  To: Leo Yan
  Cc: Mark Rutland, linux-doc, Catalin Marinas, Michael Turquette,
	Will Deacon, David Brown, linux-clk, Jonathan Corbet, Wei Xu,
	Andy Gross, mike.leach, devicetree, linux-arm-msm, Rob Herring,
	John Stultz, linux-soc, linux-arm-kernel, Mathieu Poirier,
	Guodong Xu, Stephen Boyd, linux-kernel, sudeep.holla
In-Reply-To: <20170329103712.GA22480@leoy-linaro>

On 29/03/17 11:37, Leo Yan wrote:
> On Wed, Mar 29, 2017 at 11:31:03AM +0100, Suzuki K Poulose wrote:
>> On 29/03/17 11:27, Leo Yan wrote:
>>> On Wed, Mar 29, 2017 at 10:07:07AM +0100, Suzuki K Poulose wrote:
>>>
>>> [...]
>>>
>>>>>>> +	if (mode == EDDEVID_IMPL_NONE) {
>>>>>>> +		drvdata->edpcsr_present  = false;
>>>>>>> +		drvdata->edcidsr_present = false;
>>>>>>> +		drvdata->edvidsr_present = false;
>>>>>>> +	} else if (mode == EDDEVID_IMPL_EDPCSR) {
>>>>>>> +		drvdata->edpcsr_present  = true;
>>>>>>> +		drvdata->edcidsr_present = false;
>>>>>>> +		drvdata->edvidsr_present = false;
>>>>>>> +	} else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) {
>>>>>>> +		if (!IS_ENABLED(CONFIG_64BIT) &&
>>>>>>> +			(pcsr_offset == EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32))
>>>>>>> +			drvdata->edpcsr_present = false;
>>>>>>> +		else
>>>>>>> +			drvdata->edpcsr_present = true;
>>>>>>
>>>>>> Sorry, I forgot why we do this check only in this mode. Shouldn't this be
>>>>>> common to all modes (of course which implies PCSR is present) ?
>>>>>
>>>>> No. PCSROffset is defined differently in ARMv7 and ARMv8; So finally we
>>>>> simplize PCSROffset value :
>>>>> 0000 - Sample offset applies based on the instruction state (indicated by PCSR[0])
>>>>> 0001 - No offset applies.
>>>>> 0010 - No offset applies, but do not use in AArch32 mode!
>>>>>
>>>>> So we need handle the corner case is when CPU runs AArch32 mode and
>>>>> PCSRoffset = 'b0010. Other cases the pcsr should be present.
>>>>
>>>> I understand that reasoning. But my question is, why do we check for PCSROffset
>>>> only when mode == EDDEVID_IMPL_EDPCSR_EDCIDSR and not for say mode == EDDEVID_IMPL_EDPCSR or
>>>> any other mode where PCSR is present.
>>>
>>> Sorry I misunderstood your question.
>>>
>>> I made mistake when I analyzed the possbile combination for mode and
>>> PCSROffset so I thought it's the only case should handle:
>>> { EDDEVID_IMPL_EDPCSR_EDCIDSR, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }
>>>
>>> Below three combinations are possible to exist; so you are right, I
>>> should move this out for the checking:
>>> { EDDEVID_IMPL_NONE,           EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }
>>
>> That need not be covered, as IMPL_NONE says PCSR is not implemented hence you
>> don't worry about anything as the functionality is missing. This should rather be:
>> EDDEVID_IMPL_EDPCSR, where only PCSR is implemented.
>
> I think below combination doesn't really exist:
> { EDDEVID_IMPL_EDPCSR, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 };
>
> EDDEVID_IMPL_EDPCSR is only defined in ARMv7 ARM, and
> EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 is only defined in ARMv8 ARM.

It is not wrong to check the PCSROffset in all cases where PCSR is available, as if
we hit PCSR on ARMv7 then PCSROffset shouldn't be DIS_AARCH32. And in fact that
would make the code a bit more cleaner. Anyways, I am not particular about this.

Suzuki

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox