* Re: [PATCH v4 09/13] net: ethernet: ti: cpts: rework initialization/deinitialization
From: Richard Cochran @ 2016-12-06 18:04 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori, linux-kernel,
linux-omap, devicetree, Murali Karicheri, Wingman Kwok,
Thomas Gleixner
In-Reply-To: <c0d6d670-cf44-c113-3443-3b5209e68ee2@ti.com>
On Tue, Dec 06, 2016 at 11:49:14AM -0600, Grygorii Strashko wrote:
> But we do reset whole cpsw :( and that's required to support PM use cases as
> suspend/resume.
The code is resetting the clock unconditionally on ifup/down. That
sucks. If you reset the clock *only* after resume, that would be ok.
> There are also PM requirement to shutdown cpsw in case all interfaces are down.
Well, those requirements are not too smart. As an end user, I expect
that ifdown/up does not change the time. There isn't any reason to
reset the clock in this case.
> More over, there are requirement to minimize cpsw power consumption in case all links are
> disconnected (and cpts is special case here).
>
> So, at least resetting of the timecounter still required.
Only if you follow that poorly conceived PM plan. Anyhow, I agree
that it isn't the task of your present series to fix that.
> Ok. I'll try to optimize it following your directions.
What I would like to see is: initialize the cyclecounter fields
exactly once.
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH v4 09/13] net: ethernet: ti: cpts: rework initialization/deinitialization
From: Grygorii Strashko @ 2016-12-06 17:49 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev-u79uwXL29TY76Z2rM5mHXA, Mugunthan V N,
Sekhar Nori, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Murali Karicheri, Wingman Kwok,
Thomas Gleixner
In-Reply-To: <20161206171802.GA19646-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
Hi Richard,
On 12/06/2016 11:18 AM, Richard Cochran wrote:
> On Tue, Dec 06, 2016 at 10:45:55AM -0600, Grygorii Strashko wrote:
>> On 12/06/2016 07:40 AM, Richard Cochran wrote:
>>> [ BTW, resetting the timecounter here makes no sense either. Why
>>> reset the clock just because the interface goes down? ]
>>>
>>
>> Huh. This is how it works now (even before my changes) - this is just refactoring!
>> (really new thing is the only cpts_calc_mult_shift()).
>
> The cpts_register() used to be called from probe(), but this changed
> without any review in f280e89ad. That wasn't your fault, but still...
>
>> Also there are additional questions such as:
>> - is there guarantee that cpsw port will be connected to the same network after ifup?
>
> The network is not relevant. PTP time is a global standard, just like
> with NTP. We don't reset the NTP clock with ifup/down, do we?
But we do reset whole cpsw :( and that's required to support PM use cases as
suspend/resume.
There are also PM requirement to shutdown cpsw in case all interfaces are down.
More over, there are requirement to minimize cpsw power consumption in case all links are
disconnected (and cpts is special case here).
So, at least resetting of the timecounter still required.
>
>> - should there be possibility to reset cc.mult if it's value will be kept from the previous run?
>
> cc.mult will change naturally according to the operation of the user
> space PTP stack. There is no need to reset it that I can see.
>
>>> Secondly, you have made the initialization order of these fields hard
>>> to follow. With the whole series applied:
>>>
>>> probe()
>>> cpts_create()
>>> cpts_of_parse()
>>> {
>>> /* Set cc_mult but not cc.mult! */
>>> set cc_mult
>>> set cc.shift
>>> }
>>> cpts_calc_mult_shift()
>>> {
>>> /* Set them both. */
>>> cpts->cc_mult = mult;
>>> cpts->cc.mult = mult;
>>
>> ^^ this assignment of cpts->cc.mult not required.
>
> You wrote the code, not me.
Sry, I meant, thank for catching this.
>
>>> cpts->cc.shift = shift;
>>
>>
>> only in case there were not set in DT before
>> (I have a requirement to support both - DT and cpts_calc_mult_shift and
>> that introduces a bit of complexity)
>>
>> Also, I've tried not to add more fields in struct cpts.
>>
>>> }
>>> /* later on */
>>> cpts_register()
>>> cpts->cc.mult = cpts->cc_mult;
>>>
>>> There is no need for such complexity. Simply set cc.mult in
>>> cpts_create() _once_, immediately after the call to
>>> cpts_calc_mult_shift().
>>>
>>> You can remove the assignment from cpts_calc_mult_shift() and
>>> cpts_register().
>>
>> Just to clarify: do you propose to get rid of cpts->cc_mult at all?
>
> No. Read what I said before:
>
> There is no need for such complexity. Simply set cc.mult in
> cpts_create() _once_, immediately after the call to
> cpts_calc_mult_shift().
>
>> Honestly, i'd not prefer to change functional behavior of ptp clock as part of
>> this series.
>
> Fair enough. The bogus clock reset existed before your series.
>
> But please don't obfuscate simple initialization routines. The way
> you set cc.mult and cc_mult is illogical and convoluted.
>
Ok. I'll try to optimize it following your directions.
Thanks.
--
regards,
-grygorii
--
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: [RFC] New Device Tree property - "bonding"
From: Ramesh Shanmugasundaram @ 2016-12-06 17:45 UTC (permalink / raw)
To: Rob Herring
Cc: Laurent Pinchart,
frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
mark.rutland-5wv7dgnIgG8@public.gmane.org,
pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org,
Chris Paterson, Geert Uytterhoeven,
laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Maxime Ripard
In-Reply-To: <CAL_JsqKJcEmNUzOm-3j3FODkN1faXMAMmURRxfRpHfiGs_a+qA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 6188 bytes --]
Hi Rob,
> >> On Monday 05 Dec 2016 09:57:32 Rob Herring wrote:
> >> > On Mon, Dec 5, 2016 at 8:40 AM, Laurent Pinchart wrote:
> >> > > On Monday 05 Dec 2016 08:18:34 Rob Herring wrote:
> >> > >> On Fri, Nov 25, 2016 at 10:55 AM, Ramesh Shanmugasundaram wrote:
> >> > >>> Hello DT maintainers,
> >> > >>>
> >> > >>> In one of the Renesas SoCs we have a device called DRIF
> >> > >>> (Digital Radio
> >> > >>> Interface) controller. A DRIF channel contains 4 external pins
> >> > >>> - SCK, SYNC, Data pins D0 & D1.
> >> > >>>
> >> > >>> Internally a DRIF channel is made up of two SPI slave devices
> >> > >>> (also called sub-channels here) that share common CLK & SYNC
> >> > >>> signals but have their own resource set. The DRIF channel can
> >> > >>> have either one of the sub-channel active at a time or both.
> >> > >>> When both sub-channels are active, they need to be managed
> >> > >>> together as one device as they share same CLK & SYNC. We plan
> >> > >>> to tie these two sub-channels together with a new property called
> "renesas,bonding".
> >> > >>
> >> > >> Is there no need to describe the master device? No GPIOs,
> >> > >> regulators or other sideband controls needed? If that's never
> >> > >> needed (which seems doubtful), then I would do something
> >> > >> different here probably with the master device as a child of one
> >> > >> DRIF and then phandles to master from the other DRIFs.
> >> > >> Otherwise, this looks
> >> fine to me.
> >> > >
> >> > > Here's a bit of background.
> >> > >
> >> > > The DRIF is an SPI receiver. It has three input pins, a clock
> >> > > line, a data line and a sync signal. The device is designed to be
> >> > > connected to a variety of data sources, usually plain SPI (1 data
> >> > > line), IIS (1 data
> >> > > line) but also radio tuners that output I/Q data
> >> > > (http://www.ni.com/tutorial/4805/en/) over two data lines.
> >> > >
> >> > > In the case of IQ each data sample is split in two I and Q values
> >> > > (typically 16 to 20 bits each in this case), and the values are
> >> > > transmitted serially over one data line each. The synchronization
> >> > > and clock signals are common to both data lines. The DRIF is
> >> > > optimized for this use case as the DRIF instances in the SoC
> >> > > (each of them having independent clocks, interrupts and control
> >> > > registers) are grouped by two, and the two instances in a group
> >> > > handle a single data line each but share the same clock and sync
> input.
> >> > >
> >> > > On the software side we need to group the I and Q values, which
> >> > > are DMA'ed to memory by the two DRIF instances, and make them
> >> > > available to userspace. The V4L2 API used here in SDR (Software
> >> > > Defined Radio) mode supports such use cases and exposes a single
> >> > > device node to userspace that allows control of the two DRIF
> >> > > instances as a single device. To be able to implement this we
> >> > > need kernel code to be aware of DRIF groups and, while binding to
> >> > > the DRIF instances separately, expose only one V4L2 device to
> userspace for each group.
> >> > >
> >> > > There's no master or slave instance from a hardware point of
> >> > > view, but the two instances are not interchangeable as they carry
> >> > > separate
> >> information.
> >> > > They must thus be identified at the driver level.
> >> >
> >> > By master, I meant the external master device that generates the IQ
> >> > data, not which of the internal DRIF blocks is a master of the other.
> >> > So back to my question, does the external master device need to be
> >> > described? I worry the answer now for a simple case is no, but then
> >> > later people are going to have cases needing to describe more. We
> >> > need to answer this question first before we can decide what this
> >> > binding should look like.
> >>
> >> Oh yes the external device certainly needs to be described. As it is
> >> controlled through a separate, general-purpose I2C or SPI controller,
> >> it should be a child node of that controller. The DRIF handles the
> >> data interface only, not the control interface of the external device.
> >
> > Yes, as Laurent mentioned, the external master will be described
> separately. The data interface with the master is described through port
> nodes. E.g.
> >
> > port {
> > drif0_ep: endpoint {
> > remote-endpoint = <&tuner_ep>;
> > };
> > };
> >
> > Do we agree on this model please?
>
> Well, that's not complete as you should have both DRIF0 and DRIF1 having
> connections to the tuner. Then you can walk the graph and find everything,
> and you then don't need the bonding property.
Assuming the third party tuner exposes it's two data lines as two endpoints, it seems possible with of_graph.h apis to walk through tuner end points and get the phandle of the other DRIF device. However, there are couple of points coming to mind.
- The ctrl pins shared between two DRIFs needs to be enabled in one of the DRIF device. Do we choose this device arbitrarily? Do we expose the CTRL signal properties (msb/lsb first, polarity etc) on both DRIF devices? Should we think about scalability?
- It mandates the third party tuner device to expose it's two data lines as two endpoints. It assumes that a single third party master device controls both the data lines coming to each DRIF device.
The bonding property looks a bit cleaner on these aspects because it describes only the DRIF device.
Thanks,
Ramesh
[https://www2.renesas.eu/media/email/unicef.gif]
This Christmas, instead of sending out cards, Renesas Electronics Europe have decided to support Unicef with a donation. For further details click here<https://www.unicef.org/> to find out about the valuable work they do, helping children all over the world.
We would like to take this opportunity to wish you a Merry Christmas and a prosperous New Year.
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i
^ permalink raw reply
* Re: [PATCH v3 3/4] [media] davinci: vpif_capture: get subdevs from DT
From: Kevin Hilman @ 2016-12-06 17:40 UTC (permalink / raw)
To: Hans Verkuil
Cc: Laurent Pinchart, Sakari Ailus,
linux-media-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Sekhar Nori, Axel Haslam,
Bartosz Gołaszewski, Alexandre Bailon, David Lechner
In-Reply-To: <d7aaa1d5-f11a-e361-b2fe-f0cf86d92008-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org>
Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org> writes:
> On 12/01/2016 10:16 AM, Laurent Pinchart wrote:
>> Hello,
>>
>> On Thursday 01 Dec 2016 09:57:31 Sakari Ailus wrote:
>>> On Wed, Nov 30, 2016 at 04:14:11PM -0800, Kevin Hilman wrote:
>>>> Sakari Ailus <sakari.ailus-X3B1VOXEql0@public.gmane.org> writes:
>>>>> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
>>>>>> Sakari Ailus <sakari.ailus-X3B1VOXEql0@public.gmane.org> writes:
>>>>>>> On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
>>>>>>>> Allow getting of subdevs from DT ports and endpoints.
>>>>>>>>
>>>>>>>> The _get_pdata() function was larely inspired by (i.e. stolen from)
>>>>>>>> am437x-vpfe.c
>>>>>>>>
>>>>>>>> Signed-off-by: Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++-
>>>>>>>> include/media/davinci/vpif_types.h
>>>>>>>> | 9 +-
>>>>>>>> 2 files changed, 133 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/media/platform/davinci/vpif_capture.c
>>>>>>>> b/drivers/media/platform/davinci/vpif_capture.c index
>>>>>>>> 94ee6cf03f02..47a4699157e7 100644
>>>>>>>> --- a/drivers/media/platform/davinci/vpif_capture.c
>>>>>>>> +++ b/drivers/media/platform/davinci/vpif_capture.c
>>>>>>>> @@ -26,6 +26,8 @@
>>>>>>>> #include <linux/slab.h>
>>>>>>>>
>>>>>>>> #include <media/v4l2-ioctl.h>
>>>>>>>> +#include <media/v4l2-of.h>
>>>>>>>> +#include <media/i2c/tvp514x.h>
>>>>>>>
>>>>>>> Do you need this header?
>>>>>>
>>>>>> Yes, based on discussion with Hans, since there is no DT binding for
>>>>>> selecting the input pins of the TVP514x, I have to select it in the
>>>>>> driver, so I need the defines from this header. More on this below...
>>
>> That's really ugly :-( The problem should be fixed properly instead of adding
>> one more offender.
>
> Do you have time for that, Laurent? I don't. Until that time we just need to
> make do with this workaround.
>
>>
>>>>>>>> #include "vpif.h"
>>>>>>>> #include "vpif_capture.h"
>>>>>>>> @@ -650,6 +652,10 @@ static int vpif_input_to_subdev(
>>>>>>>>
>>>>>>>> vpif_dbg(2, debug, "vpif_input_to_subdev\n");
>>>>>>>>
>>>>>>>> + if (!chan_cfg)
>>>>>>>> + return -1;
>>>>>>>> + if (input_index >= chan_cfg->input_count)
>>>>>>>> + return -1;
>>>>>>>> subdev_name = chan_cfg->inputs[input_index].subdev_name;
>>>>>>>> if (subdev_name == NULL)
>>>>>>>> return -1;
>>>>>>>> @@ -657,7 +663,7 @@ static int vpif_input_to_subdev(
>>>>>>>> /* loop through the sub device list to get the sub device info
>>>>>>>> */
>>>>>>>> for (i = 0; i < vpif_cfg->subdev_count; i++) {
>>>>>>>> subdev_info = &vpif_cfg->subdev_info[i];
>>>>>>>> - if (!strcmp(subdev_info->name, subdev_name))
>>>>>>>> + if (subdev_info && !strcmp(subdev_info->name,
>>>>>>>> subdev_name))
>>>>>>>> return i;
>>>>>>>> }
>>>>>>>> return -1;
>>>>>>>> @@ -1327,6 +1333,21 @@ static int vpif_async_bound(struct
>>>>>>>> v4l2_async_notifier *notifier,> >> >>
>>>>>>>> {
>>>>>>>> int i;
>>>>>>>>
>>>>>>>> + for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
>>>>>>>> + struct v4l2_async_subdev *_asd = vpif_obj.config
>>>>>>>> ->asd[i];
>>>>>>>> + const struct device_node *node = _asd->match.of.node;
>>>>>>>> +
>>>>>>>> + if (node == subdev->of_node) {
>>>>>>>> + vpif_obj.sd[i] = subdev;
>>>>>>>> + vpif_obj.config->chan_config
>>>>>>>> ->inputs[i].subdev_name =
>>>>>>>> + (char *)subdev->of_node->full_name;
>>
>> Can subdev_name be made const instead of blindly casting the full_name pointer
>> ? If not this is probably unsafe, and if yes it should be done :-)
>>
>>>>>>>> + vpif_dbg(2, debug,
>>>>>>>> + "%s: setting input %d subdev_name =
>>>>>>>> %s\n",
>>>>>>>> + __func__, i, subdev->of_node
>>>>>>>> ->full_name);
>>>>>>>> + return 0;
>>>>>>>> + }
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> for (i = 0; i < vpif_obj.config->subdev_count; i++)
>>>>>>>> if (!strcmp(vpif_obj.config->subdev_info[i].name,
>>>>>>>> subdev->name)) {
>>>>>>>> @@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct
>>>>>>>> v4l2_async_notifier *notifier)
>>>>>>>> return vpif_probe_complete();
>>>>>>>> }
>>>>>>>>
>>>>>>>> +static struct vpif_capture_config *
>>>>>>>> +vpif_capture_get_pdata(struct platform_device *pdev)
>>>>>>>> +{
>>>>>>>> + struct device_node *endpoint = NULL;
>>>>>>>> + struct v4l2_of_endpoint bus_cfg;
>>>>>>>> + struct vpif_capture_config *pdata;
>>>>>>>> + struct vpif_subdev_info *sdinfo;
>>>>>>>> + struct vpif_capture_chan_config *chan;
>>>>>>>> + unsigned int i;
>>>>>>>> +
>>>>>>>> + dev_dbg(&pdev->dev, "vpif_get_pdata\n");
>>>>>>>> +
>>>>>>>> + if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
>>>>>>>> + return pdev->dev.platform_data;
>>>>>>>> +
>>>>>>>> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>>>>>>>> + if (!pdata)
>>>>>>>> + return NULL;
>>>>>>>> + pdata->subdev_info =
>>>>>>>> + devm_kzalloc(&pdev->dev, sizeof(*pdata->subdev_info) *
>>>>>>>> + VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
>>>>>>>> +
>>>>>>>> + if (!pdata->subdev_info)
>>>>>>>> + return NULL;
>>>>>>>> + dev_dbg(&pdev->dev, "%s\n", __func__);
>>>>>>>> +
>>>>>>>> + for (i = 0; ; i++) {
>>>>>>>> + struct device_node *rem;
>>>>>>>> + unsigned int flags;
>>>>>>>> + int err;
>>>>>>>> +
>>>>>>>> + endpoint = of_graph_get_next_endpoint(pdev
>>>>>>>> ->dev.of_node,
>>>>>>>> + endpoint);
>>>>>>>> + if (!endpoint)
>>>>>>>> + break;
>>>>>>>> +
>>>>>>>> + sdinfo = &pdata->subdev_info[i];
>>>>>>>
>>>>>>> subdev_info[] has got VPIF_CAPTURE_MAX_CHANNELS entries only.
>>>>>>
>>>>>> Right, I need to make the loop only go for a max of
>>>>>> VPIF_CAPTURE_MAX_CHANNELS iterations.
>>>>>>
>>>>>>>> + chan = &pdata->chan_config[i];
>>>>>>>> + chan->inputs = devm_kzalloc(&pdev->dev,
>>>>>>>> + sizeof(*chan->inputs) *
>>>>>>>> + VPIF_DISPLAY_MAX_CHANNELS,
>>>>>>>> + GFP_KERNEL);
>>>>>>>> +
>>>>>>>> + chan->input_count++;
>>>>>>>> + chan->inputs[i].input.type = V4L2_INPUT_TYPE_CAMERA;
>>>>>>>
>>>>>>> I wonder what's the purpose of using index i on this array as well.
>>>>>>
>>>>>> The number of endpoints in DT is the number of input channels
>>>>>> configured (up to a max of VPIF_CAPTURE_MAX_CHANNELS.)
>>>>>>
>>>>>>> If you use that to access a corresponding entry in a different array,
>>>>>>> I'd just create a struct that contains the port configuration and the
>>>>>>> async sub-device. The omap3isp driver does that, for instance; see
>>>>>>> isp_of_parse_nodes() in drivers/media/platform/omap3isp/isp.c if
>>>>>>> you're interested. Up to you.
>>>>>>
>>>>>> OK, I'll have a look at that driver. The goal here with this series is
>>>>>> just to get this working with DT, but also not break the existing
>>>>>> legacy platform_device support, so I'm trying not to mess with the
>>>>>> driver-interal data structures too much.
>>>>>
>>>>> Ack.
>>>>>
>>>>>>>> + chan->inputs[i].input.std = V4L2_STD_ALL;
>>>>>>>> + chan->inputs[i].input.capabilities = V4L2_IN_CAP_STD;
>>>>>>>> +
>>>>>>>> + /* FIXME: need a new property? ch0:composite ch1:
>>>>>>>> s-video */
>>>>>>>> + if (i == 0)
>>>>>>>
>>>>>>> Can you assume that the first endopoint has got a particular kind of
>>>>>>> input? What if it's not connected?
>>>>>>
>>>>>> On all the boards I know of (there aren't many using this SoC), it's a
>>>>>> safe assumption.
>>>>>>
>>>>>>> If this is a different physical port (not in the meaning another) in
>>>>>>> the device, I'd use the reg property for this. Please see
>>>>>>> Documentation/devicetree/bindings/media/video-interfaces.txt .
>>>>>>
>>>>>> My understanding (which is admittedly somewhat fuzzy) of the TVP514x is
>>>>>> that it's not physically a different port. Instead, it's just telling
>>>>>> the TVP514x which pin(s) will be active inputs (and what kind of signal
>>>>>> will be present.)
>>>>>>
>>>>>> I'm open to a better way to describe this input select from DT, but
>>>>>> based on what I heard from Hans, there isn't currently a good way to do
>>>>>> that except for in the driver:
>>>>>> (c.f. https://marc.info/?l=linux-arm-kernel&m=147887871615788)
>>>>>>
>>>>>> Based on further discussion in that thread, it sounds like there may be
>>>>>> a way forward coming soon, and I'll be glad to switch to that when it
>>>>>> arrives.
>>
>> I'm afraid I have to disappoint Hans here, I don't have code for that yet.
>>
>>>>> I'm not sure that properly supporting connectors will provide any help
>>>>> here.
>>>>>
>>>>> Looking at the s_routing() API, it's the calling driver that has to be
>>>>> aware of sub-device specific function parameters. As such it's not a
>>>>> very good idea to require that a driver is aware of the value range of
>>>>> another driver's parameter. I wonder if a simple enumeration interface
>>>>> would help here --- if I understand correctly, the purpose is just to
>>>>> provide a way to choose the input using VIDIOC_S_INPUT.
>>>>>
>>>>> I guess that's somehow ok as long as you have no other combinations of
>>>>> these devices but this is hardly future-proof. (And certainly not a
>>>>> problem created by this patch.)
>>>>
>>>> Yeah, this is far from future proof.
>>>>
>>>>> It'd be still nice to fix that as presumably we don't have the option of
>>>>> reworking how we expect the device tree to look like.
>>>>
>>>> Agreed.
>>>>
>>>> I'm just hoping someone can shed som light on "how we expect the device
>>>> tree to look". ;)
>>>
>>> :-)
>>>
>>> For the tvp514x, do you need more than a single endpoint on the receiver
>>> side? Does the input that's selected affect the bus parameters?
>>>
>>> If it doesn't, you could create a custom endpoint property for the possible
>>> input values. The s_routing() really should be fixed though, but that could
>>> be postponed I guess. There are quite a few drivers using it.
>>
>> There's two ways to look at s_routing() in my opinion, as the calling driver
>> should really not hardcode any knowledge specific to a particular subdev. We
>> can either have the calling driver discover the possible routing options at
>> runtime through the subdev API, or modify the s_routing() API.
>>
>
> Some historical perspective: s_routing was added well before the device tree
> was ever used for ARM. And at that time the vast majority of drivers were PCI
> or USB drivers, very few platform drivers existed (and those typically used
> sensors, not video receivers).
>
> Before s_routing existed the situation was even worse.
>
> Basically what s_routing does is a poor-man's device tree entry, telling the
> subdev how to route video or audio from connector to the output of the chip.
> Typically the card tables in PCI or USB drivers contain the correct arguments
> for s_routing. Of course, today we'd do that with the DT, but that was not an
> option years ago.
So I'm still confused on the path forward here.
I do not have the time (or the V4L2 knowledge/experience) to rework the
V4L2 internals to make this work, but I'm happy to test if someone else
is working on it.
In the meantime, what do we do with this series? I have a couple minor
things to fixup based on review comments, but other than that, the
s_routing decision is blocking this from getting an update for use on DT
platforms.
The alternative is to go the OMAP route for legacy drivers like this and
just use pdata quirks for passing the legacy pdata (which has the input
and output routes hard-coded in platform_data).
Kevin
--
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/5] ARM: BCM5301X: Specify USB controllers in DT
From: Ray Jui @ 2016-12-06 17:36 UTC (permalink / raw)
To: Rafał Miłecki
Cc: Florian Fainelli, Arnd Bergmann, Rob Herring, Mark Rutland,
Russell King, Hauke Mehrtens, bcm-kernel-feedback-list,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Linux Kernel Mailing List, Rafał Miłecki
In-Reply-To: <CACna6rw4iY_xhEUyWkGrchs0SOn3nLBKYMuA7qa0UHAbxpGN0A@mail.gmail.com>
On 12/6/2016 9:31 AM, Rafał Miłecki wrote:
> On 6 December 2016 at 18:28, Ray Jui <ray.jui@broadcom.com> wrote:
>> On 12/6/2016 9:17 AM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> There are 3 separated controllers, one per USB /standard/. With PHY
>>> drivers in place they can be simply supported with generic drivers.
>>>
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>> ---
>>> arch/arm/boot/dts/bcm5301x.dtsi | 33 ++++++++++++++++++++++++++++++++-
>>> 1 file changed, 32 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
>>> index f09a2bb..bfc98d19 100644
>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>> @@ -248,8 +248,26 @@
>>>
>>> #address-cells = <1>;
>>> #size-cells = <1>;
>>> + ranges;
>>>
>>> - phys = <&usb2_phy>;
>>> + interrupt-parent = <&gic>;
>>> +
>>> + ohci: ohci@21000 {
>>> + #usb-cells = <0>;
>>> +
>>> + compatible = "generic-ohci";
>>> + reg = <0x00022000 0x1000>;
>>
>> Your label ohci@21000 does not match the 'reg' at 0x22000.
>>
>>> + interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>;
>>> + };
>>> +
>>> + ehci: ehci@22000 {
>>> + #usb-cells = <0>;
>>> +
>>> + compatible = "generic-ehci";
>>> + reg = <0x00021000 0x1000>;
>>
>> Looks like you got the label of ohci and ehci reversed?
>
> Nice catch, thanks! I'll fix it in V2 (just let me wait a day to see
> if there will be other comments).
>
In V2, please remember to put the nodes in ascending order based on the
base address of the registers, i.e., ehci@21000, and then followed by
ohci@22000.
Thanks,
Ray
^ permalink raw reply
* Re: [PATCH 2/5] ARM: BCM5301X: Specify USB controllers in DT
From: Rafał Miłecki @ 2016-12-06 17:31 UTC (permalink / raw)
To: Ray Jui
Cc: Florian Fainelli, Arnd Bergmann, Rob Herring, Mark Rutland,
Russell King, Hauke Mehrtens, bcm-kernel-feedback-list,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Linux Kernel Mailing List, Rafał Miłecki
In-Reply-To: <c721c71c-117f-185d-0544-954981dbcf04-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
On 6 December 2016 at 18:28, Ray Jui <ray.jui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
> On 12/6/2016 9:17 AM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>
>> There are 3 separated controllers, one per USB /standard/. With PHY
>> drivers in place they can be simply supported with generic drivers.
>>
>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>> ---
>> arch/arm/boot/dts/bcm5301x.dtsi | 33 ++++++++++++++++++++++++++++++++-
>> 1 file changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
>> index f09a2bb..bfc98d19 100644
>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>> @@ -248,8 +248,26 @@
>>
>> #address-cells = <1>;
>> #size-cells = <1>;
>> + ranges;
>>
>> - phys = <&usb2_phy>;
>> + interrupt-parent = <&gic>;
>> +
>> + ohci: ohci@21000 {
>> + #usb-cells = <0>;
>> +
>> + compatible = "generic-ohci";
>> + reg = <0x00022000 0x1000>;
>
> Your label ohci@21000 does not match the 'reg' at 0x22000.
>
>> + interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>;
>> + };
>> +
>> + ehci: ehci@22000 {
>> + #usb-cells = <0>;
>> +
>> + compatible = "generic-ehci";
>> + reg = <0x00021000 0x1000>;
>
> Looks like you got the label of ohci and ehci reversed?
Nice catch, thanks! I'll fix it in V2 (just let me wait a day to see
if there will be other comments).
--
Rafał
--
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/5] ARM: BCM5301X: Specify USB controllers in DT
From: Ray Jui @ 2016-12-06 17:28 UTC (permalink / raw)
To: Rafał Miłecki, Florian Fainelli
Cc: Arnd Bergmann, Rob Herring, Mark Rutland, Russell King,
Hauke Mehrtens, bcm-kernel-feedback-list, devicetree,
linux-arm-kernel, linux-kernel, Rafał Miłecki
In-Reply-To: <20161206171714.22738-2-zajec5@gmail.com>
On 12/6/2016 9:17 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> There are 3 separated controllers, one per USB /standard/. With PHY
> drivers in place they can be simply supported with generic drivers.
>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> arch/arm/boot/dts/bcm5301x.dtsi | 33 ++++++++++++++++++++++++++++++++-
> 1 file changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
> index f09a2bb..bfc98d19 100644
> --- a/arch/arm/boot/dts/bcm5301x.dtsi
> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
> @@ -248,8 +248,26 @@
>
> #address-cells = <1>;
> #size-cells = <1>;
> + ranges;
>
> - phys = <&usb2_phy>;
> + interrupt-parent = <&gic>;
> +
> + ohci: ohci@21000 {
> + #usb-cells = <0>;
> +
> + compatible = "generic-ohci";
> + reg = <0x00022000 0x1000>;
Your label ohci@21000 does not match the 'reg' at 0x22000.
> + interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>;
> + };
> +
> + ehci: ehci@22000 {
> + #usb-cells = <0>;
> +
> + compatible = "generic-ehci";
> + reg = <0x00021000 0x1000>;
Looks like you got the label of ohci and ehci reversed?
> + interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>;
> + phys = <&usb2_phy>;
> + };
> };
>
> usb3: usb3@23000 {
> @@ -257,6 +275,19 @@
>
> #address-cells = <1>;
> #size-cells = <1>;
> + ranges;
> +
> + interrupt-parent = <&gic>;
> +
> + xhci: xhci@23000 {
> + #usb-cells = <0>;
> +
> + compatible = "generic-xhci";
> + reg = <0x00023000 0x1000>;
> + interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>;
> + phys = <&usb3_phy>;
> + phy-names = "usb";
> + };
> };
>
> spi@29000 {
>
^ permalink raw reply
* Re: [PATCH v4 09/13] net: ethernet: ti: cpts: rework initialization/deinitialization
From: Richard Cochran @ 2016-12-06 17:18 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev-u79uwXL29TY76Z2rM5mHXA, Mugunthan V N,
Sekhar Nori, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Murali Karicheri, Wingman Kwok,
Thomas Gleixner
In-Reply-To: <4e7888b6-9f1c-ca31-e83e-15109bf1df3f-l0cyMroinI0@public.gmane.org>
On Tue, Dec 06, 2016 at 10:45:55AM -0600, Grygorii Strashko wrote:
> On 12/06/2016 07:40 AM, Richard Cochran wrote:
> > [ BTW, resetting the timecounter here makes no sense either. Why
> > reset the clock just because the interface goes down? ]
> >
>
> Huh. This is how it works now (even before my changes) - this is just refactoring!
> (really new thing is the only cpts_calc_mult_shift()).
The cpts_register() used to be called from probe(), but this changed
without any review in f280e89ad. That wasn't your fault, but still...
> Also there are additional questions such as:
> - is there guarantee that cpsw port will be connected to the same network after ifup?
The network is not relevant. PTP time is a global standard, just like
with NTP. We don't reset the NTP clock with ifup/down, do we?
> - should there be possibility to reset cc.mult if it's value will be kept from the previous run?
cc.mult will change naturally according to the operation of the user
space PTP stack. There is no need to reset it that I can see.
> > Secondly, you have made the initialization order of these fields hard
> > to follow. With the whole series applied:
> >
> > probe()
> > cpts_create()
> > cpts_of_parse()
> > {
> > /* Set cc_mult but not cc.mult! */
> > set cc_mult
> > set cc.shift
> > }
> > cpts_calc_mult_shift()
> > {
> > /* Set them both. */
> > cpts->cc_mult = mult;
> > cpts->cc.mult = mult;
>
> ^^ this assignment of cpts->cc.mult not required.
You wrote the code, not me.
> > cpts->cc.shift = shift;
>
>
> only in case there were not set in DT before
> (I have a requirement to support both - DT and cpts_calc_mult_shift and
> that introduces a bit of complexity)
>
> Also, I've tried not to add more fields in struct cpts.
>
> > }
> > /* later on */
> > cpts_register()
> > cpts->cc.mult = cpts->cc_mult;
> >
> > There is no need for such complexity. Simply set cc.mult in
> > cpts_create() _once_, immediately after the call to
> > cpts_calc_mult_shift().
> >
> > You can remove the assignment from cpts_calc_mult_shift() and
> > cpts_register().
>
> Just to clarify: do you propose to get rid of cpts->cc_mult at all?
No. Read what I said before:
There is no need for such complexity. Simply set cc.mult in
cpts_create() _once_, immediately after the call to
cpts_calc_mult_shift().
> Honestly, i'd not prefer to change functional behavior of ptp clock as part of
> this series.
Fair enough. The bogus clock reset existed before your series.
But please don't obfuscate simple initialization routines. The way
you set cc.mult and cc_mult is illogical and convoluted.
Thanks,
Richard
--
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
* [PATCH 5/5] ARM: BCM53573: Specify USB ports of on-SoC controllers
From: Rafał Miłecki @ 2016-12-06 17:17 UTC (permalink / raw)
To: Florian Fainelli
Cc: Arnd Bergmann, Rob Herring, Mark Rutland, Russell King,
Hauke Mehrtens, bcm-kernel-feedback-list, devicetree,
linux-arm-kernel, linux-kernel, Rafał Miłecki
In-Reply-To: <20161206171714.22738-1-zajec5@gmail.com>
From: Rafał Miłecki <rafal@milecki.pl>
Broadcom OHCI and EHCI controllers always have 2 ports each on the root
hub. Describe them in DT to allow specifying extra info or referencing
port nodes.
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
arch/arm/boot/dts/bcm53573.dtsi | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/arch/arm/boot/dts/bcm53573.dtsi b/arch/arm/boot/dts/bcm53573.dtsi
index e2c496a..2da04d0 100644
--- a/arch/arm/boot/dts/bcm53573.dtsi
+++ b/arch/arm/boot/dts/bcm53573.dtsi
@@ -124,6 +124,17 @@
reg = <0x4000 0x1000>;
interrupt-parent = <&gic>;
interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ehci_port1: port@1 {
+ reg = <1>;
+ };
+
+ ehci_port2: port@2 {
+ reg = <2>;
+ };
};
ohci: ohci@d000 {
@@ -133,6 +144,17 @@
reg = <0xd000 0x1000>;
interrupt-parent = <&gic>;
interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ohci_port1: port@1 {
+ reg = <1>;
+ };
+
+ ohci_port2: port@2 {
+ reg = <2>;
+ };
};
};
--
2.10.1
^ permalink raw reply related
* [PATCH 4/5] ARM: BCM5301X: Specify all RAM by including extra block
From: Rafał Miłecki @ 2016-12-06 17:17 UTC (permalink / raw)
To: Florian Fainelli
Cc: Arnd Bergmann, Rob Herring, Mark Rutland, Russell King,
Hauke Mehrtens, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rafał Miłecki
In-Reply-To: <20161206171714.22738-1-zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
So far we were specifying only the first block which is always limited
up to 128 MiB. There are many devices with 256 MiB and few with 512 MiB.
Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
---
arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts | 3 ++-
arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts | 3 ++-
arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dts | 3 ++-
arch/arm/boot/dts/bcm4708-netgear-r6250.dts | 3 ++-
arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts | 3 ++-
arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dts | 3 ++-
arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts | 3 ++-
arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts | 3 ++-
arch/arm/boot/dts/bcm47081-buffalo-wzr-900dhp.dts | 3 ++-
arch/arm/boot/dts/bcm4709-asus-rt-ac87u.dts | 3 ++-
arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts | 3 ++-
arch/arm/boot/dts/bcm4709-netgear-r7000.dts | 3 ++-
arch/arm/boot/dts/bcm4709-netgear-r8000.dts | 3 ++-
arch/arm/boot/dts/bcm47094-dlink-dir-885l.dts | 3 ++-
arch/arm/boot/dts/bcm47094-netgear-r8500.dts | 3 ++-
15 files changed, 30 insertions(+), 15 deletions(-)
diff --git a/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts b/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts
index 112a5a8..d241cee 100644
--- a/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts
+++ b/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts
@@ -21,7 +21,8 @@
};
memory {
- reg = <0x00000000 0x08000000>;
+ reg = <0x00000000 0x08000000
+ 0x88000000 0x08000000>;
};
leds {
diff --git a/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts b/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts
index 3600f56..b0e6204 100644
--- a/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts
+++ b/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts
@@ -21,7 +21,8 @@
};
memory {
- reg = <0x00000000 0x08000000>;
+ reg = <0x00000000 0x08000000
+ 0x88000000 0x08000000>;
};
leds {
diff --git a/arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dts b/arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dts
index d49afec0..c9ba6b9 100644
--- a/arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dts
+++ b/arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dts
@@ -21,7 +21,8 @@
};
memory {
- reg = <0x00000000 0x08000000>;
+ reg = <0x00000000 0x08000000
+ 0x88000000 0x18000000>;
};
spi {
diff --git a/arch/arm/boot/dts/bcm4708-netgear-r6250.dts b/arch/arm/boot/dts/bcm4708-netgear-r6250.dts
index 8519548..b9f66c0 100644
--- a/arch/arm/boot/dts/bcm4708-netgear-r6250.dts
+++ b/arch/arm/boot/dts/bcm4708-netgear-r6250.dts
@@ -21,7 +21,8 @@
};
memory {
- reg = <0x00000000 0x08000000>;
+ reg = <0x00000000 0x08000000
+ 0x88000000 0x08000000>;
};
leds {
diff --git a/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts b/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts
index 6229ef2..ae0199f 100644
--- a/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts
+++ b/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts
@@ -21,7 +21,8 @@
};
memory {
- reg = <0x00000000 0x08000000>;
+ reg = <0x00000000 0x08000000
+ 0x88000000 0x08000000>;
};
leds {
diff --git a/arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dts b/arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dts
index 74cfcd3..36b628b1 100644
--- a/arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dts
+++ b/arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dts
@@ -21,7 +21,8 @@
};
memory {
- reg = <0x00000000 0x08000000>;
+ reg = <0x00000000 0x08000000
+ 0x88000000 0x08000000>;
};
leds {
diff --git a/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts b/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts
index 71b98cf..db8608b 100644
--- a/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts
+++ b/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts
@@ -21,7 +21,8 @@
};
memory {
- reg = <0x00000000 0x08000000>;
+ reg = <0x00000000 0x08000000
+ 0x88000000 0x08000000>;
};
leds {
diff --git a/arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts b/arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts
index 2922536..d51586d 100644
--- a/arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts
+++ b/arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts
@@ -21,7 +21,8 @@
};
memory {
- reg = <0x00000000 0x08000000>;
+ reg = <0x00000000 0x08000000
+ 0x88000000 0x08000000>;
};
spi {
diff --git a/arch/arm/boot/dts/bcm47081-buffalo-wzr-900dhp.dts b/arch/arm/boot/dts/bcm47081-buffalo-wzr-900dhp.dts
index 184fd92..de041b8 100644
--- a/arch/arm/boot/dts/bcm47081-buffalo-wzr-900dhp.dts
+++ b/arch/arm/boot/dts/bcm47081-buffalo-wzr-900dhp.dts
@@ -21,7 +21,8 @@
};
memory {
- reg = <0x00000000 0x08000000>;
+ reg = <0x00000000 0x08000000
+ 0x88000000 0x08000000>;
};
gpio-keys {
diff --git a/arch/arm/boot/dts/bcm4709-asus-rt-ac87u.dts b/arch/arm/boot/dts/bcm4709-asus-rt-ac87u.dts
index eac0f52..eaca687 100644
--- a/arch/arm/boot/dts/bcm4709-asus-rt-ac87u.dts
+++ b/arch/arm/boot/dts/bcm4709-asus-rt-ac87u.dts
@@ -21,7 +21,8 @@
};
memory {
- reg = <0x00000000 0x08000000>;
+ reg = <0x00000000 0x08000000
+ 0x88000000 0x08000000>;
};
leds {
diff --git a/arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts b/arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts
index aab39c9..b32957c 100644
--- a/arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts
+++ b/arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts
@@ -21,7 +21,8 @@
};
memory {
- reg = <0x00000000 0x08000000>;
+ reg = <0x00000000 0x08000000
+ 0x88000000 0x18000000>;
};
leds {
diff --git a/arch/arm/boot/dts/bcm4709-netgear-r7000.dts b/arch/arm/boot/dts/bcm4709-netgear-r7000.dts
index 7ab1176..f459a98 100644
--- a/arch/arm/boot/dts/bcm4709-netgear-r7000.dts
+++ b/arch/arm/boot/dts/bcm4709-netgear-r7000.dts
@@ -21,7 +21,8 @@
};
memory {
- reg = <0x00000000 0x08000000>;
+ reg = <0x00000000 0x08000000
+ 0x88000000 0x08000000>;
};
leds {
diff --git a/arch/arm/boot/dts/bcm4709-netgear-r8000.dts b/arch/arm/boot/dts/bcm4709-netgear-r8000.dts
index 56d38a3..cd13534 100644
--- a/arch/arm/boot/dts/bcm4709-netgear-r8000.dts
+++ b/arch/arm/boot/dts/bcm4709-netgear-r8000.dts
@@ -21,7 +21,8 @@
};
memory {
- reg = <0x00000000 0x08000000>;
+ reg = <0x00000000 0x08000000
+ 0x88000000 0x08000000>;
};
leds {
diff --git a/arch/arm/boot/dts/bcm47094-dlink-dir-885l.dts b/arch/arm/boot/dts/bcm47094-dlink-dir-885l.dts
index 7fb9270..64ded76 100644
--- a/arch/arm/boot/dts/bcm47094-dlink-dir-885l.dts
+++ b/arch/arm/boot/dts/bcm47094-dlink-dir-885l.dts
@@ -21,7 +21,8 @@
};
memory {
- reg = <0x00000000 0x08000000>;
+ reg = <0x00000000 0x08000000
+ 0x88000000 0x08000000>;
};
nand: nand@18028000 {
diff --git a/arch/arm/boot/dts/bcm47094-netgear-r8500.dts b/arch/arm/boot/dts/bcm47094-netgear-r8500.dts
index 7ecd57c..600795e 100644
--- a/arch/arm/boot/dts/bcm47094-netgear-r8500.dts
+++ b/arch/arm/boot/dts/bcm47094-netgear-r8500.dts
@@ -18,7 +18,8 @@
};
memory {
- reg = <0x00000000 0x08000000>;
+ reg = <0x00000000 0x08000000
+ 0x88000000 0x18000000>;
};
leds {
--
2.10.1
--
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
* [PATCH 3/5] ARM: BCM5301X: Set GPIO enabling USB power on Netgear R7000
From: Rafał Miłecki @ 2016-12-06 17:17 UTC (permalink / raw)
To: Florian Fainelli
Cc: Arnd Bergmann, Rob Herring, Mark Rutland, Russell King,
Hauke Mehrtens, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rafał Miłecki,
Rafał Miłecki
In-Reply-To: <20161206171714.22738-1-zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
There is one GPIO controlling power for both USB ports.
Signed-off-by: Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
---
arch/arm/boot/dts/bcm4709-netgear-r7000.dts | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/bcm4709-netgear-r7000.dts b/arch/arm/boot/dts/bcm4709-netgear-r7000.dts
index 0225d82..7ab1176 100644
--- a/arch/arm/boot/dts/bcm4709-netgear-r7000.dts
+++ b/arch/arm/boot/dts/bcm4709-netgear-r7000.dts
@@ -100,3 +100,11 @@
};
};
};
+
+&usb2 {
+ vcc-gpio = <&chipcommon 0 GPIO_ACTIVE_HIGH>;
+};
+
+&usb3 {
+ vcc-gpio = <&chipcommon 0 GPIO_ACTIVE_HIGH>;
+};
--
2.10.1
--
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
* [PATCH 2/5] ARM: BCM5301X: Specify USB controllers in DT
From: Rafał Miłecki @ 2016-12-06 17:17 UTC (permalink / raw)
To: Florian Fainelli
Cc: Arnd Bergmann, Rob Herring, Mark Rutland, Russell King,
Hauke Mehrtens, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rafał Miłecki
In-Reply-To: <20161206171714.22738-1-zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
There are 3 separated controllers, one per USB /standard/. With PHY
drivers in place they can be simply supported with generic drivers.
Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
---
arch/arm/boot/dts/bcm5301x.dtsi | 33 ++++++++++++++++++++++++++++++++-
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
index f09a2bb..bfc98d19 100644
--- a/arch/arm/boot/dts/bcm5301x.dtsi
+++ b/arch/arm/boot/dts/bcm5301x.dtsi
@@ -248,8 +248,26 @@
#address-cells = <1>;
#size-cells = <1>;
+ ranges;
- phys = <&usb2_phy>;
+ interrupt-parent = <&gic>;
+
+ ohci: ohci@21000 {
+ #usb-cells = <0>;
+
+ compatible = "generic-ohci";
+ reg = <0x00022000 0x1000>;
+ interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>;
+ };
+
+ ehci: ehci@22000 {
+ #usb-cells = <0>;
+
+ compatible = "generic-ehci";
+ reg = <0x00021000 0x1000>;
+ interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>;
+ phys = <&usb2_phy>;
+ };
};
usb3: usb3@23000 {
@@ -257,6 +275,19 @@
#address-cells = <1>;
#size-cells = <1>;
+ ranges;
+
+ interrupt-parent = <&gic>;
+
+ xhci: xhci@23000 {
+ #usb-cells = <0>;
+
+ compatible = "generic-xhci";
+ reg = <0x00023000 0x1000>;
+ interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>;
+ phys = <&usb3_phy>;
+ phy-names = "usb";
+ };
};
spi@29000 {
--
2.10.1
--
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
* [PATCH 1/5] ARM: BCM5301X: Fix LAN LED labels for Luxul XWR-3100
From: Rafał Miłecki @ 2016-12-06 17:17 UTC (permalink / raw)
To: Florian Fainelli
Cc: Arnd Bergmann, Rob Herring, Mark Rutland, Russell King,
Hauke Mehrtens, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rafał Miłecki
From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
They were named incorrectly most likely due to copy & paste mistake.
Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
---
arch/arm/boot/dts/bcm47094-luxul-xwr-3100.dts | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/bcm47094-luxul-xwr-3100.dts b/arch/arm/boot/dts/bcm47094-luxul-xwr-3100.dts
index 2f4a651..93cc91d 100644
--- a/arch/arm/boot/dts/bcm47094-luxul-xwr-3100.dts
+++ b/arch/arm/boot/dts/bcm47094-luxul-xwr-3100.dts
@@ -31,13 +31,13 @@
};
lan3 {
- label = "bcm53xx:green:lan1";
+ label = "bcm53xx:green:lan3";
gpios = <&chipcommon 1 GPIO_ACTIVE_LOW>;
linux,default-trigger = "default-off";
};
lan4 {
- label = "bcm53xx:green:lan0";
+ label = "bcm53xx:green:lan4";
gpios = <&chipcommon 2 GPIO_ACTIVE_LOW>;
linux,default-trigger = "default-off";
};
@@ -49,7 +49,7 @@
};
lan1 {
- label = "bcm53xx:green:lan3";
+ label = "bcm53xx:green:lan1";
gpios = <&chipcommon 4 GPIO_ACTIVE_LOW>;
linux,default-trigger = "default-off";
};
--
2.10.1
--
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 v4 1/4] [media] davinci: vpif_capture: don't lock over s_stream
From: Kevin Hilman @ 2016-12-06 16:49 UTC (permalink / raw)
To: Laurent Pinchart
Cc: devicetree, Sekhar Nori, Hans Verkuil, Sakari Ailus,
linux-arm-kernel, linux-media
In-Reply-To: <4747860.QGGHSuFRpz@avalon>
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> Hi Kevin,
>
> Thank you for the patch.
>
> On Tuesday 29 Nov 2016 15:57:09 Kevin Hilman wrote:
>> Video capture subdevs may be over I2C and may sleep during xfer, so we
>> cannot do IRQ-disabled locking when calling the subdev.
>>
>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>> ---
>> drivers/media/platform/davinci/vpif_capture.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/media/platform/davinci/vpif_capture.c
>> b/drivers/media/platform/davinci/vpif_capture.c index
>> 5104cc0ee40e..9f8f41c0f251 100644
>> --- a/drivers/media/platform/davinci/vpif_capture.c
>> +++ b/drivers/media/platform/davinci/vpif_capture.c
>> @@ -193,7 +193,10 @@ static int vpif_start_streaming(struct vb2_queue *vq,
>> unsigned int count) }
>> }
>>
>> + spin_unlock_irqrestore(&common->irqlock, flags);
>> ret = v4l2_subdev_call(ch->sd, video, s_stream, 1);
>> + spin_lock_irqsave(&common->irqlock, flags);
>
> I always get anxious when I see a spinlock being released randomly with an
> operation in the middle of a protected section. Looking at the code it looks
> like the spinlock is abused here. irqlock should only protect the dma_queue
> and should thus only be taken around the following code:
>
> spin_lock_irqsave(&common->irqlock, flags);
> /* Get the next frame from the buffer queue */
> common->cur_frm = common->next_frm = list_entry(common->dma_queue.next,
> struct vpif_cap_buffer, list);
> /* Remove buffer from the buffer queue */
> list_del(&common->cur_frm->list);
> spin_unlock_irqrestore(&common->irqlock, flags);
Yes, that looks correct. Will update.
> The code that is currently protected by the lock in the start and stop
> streaming functions should be protected by a mutex instead.
I tried taking the mutex here, but lockdep pointed out a deadlock. I
may not be fully understanding the V4L2 internals here, but it seems
that the ioctl is already taking a mutex, so taking it again in
start/stop streaming is a deadlock. Unless you think the locking should
be nested here, it seems to me that the mutex isn't needed.
Kevin
^ permalink raw reply
* Re: [PATCH v4 09/13] net: ethernet: ti: cpts: rework initialization/deinitialization
From: Grygorii Strashko @ 2016-12-06 16:45 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev-u79uwXL29TY76Z2rM5mHXA, Mugunthan V N,
Sekhar Nori, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Murali Karicheri, Wingman Kwok,
Thomas Gleixner
In-Reply-To: <20161206134037.GA15946-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
On 12/06/2016 07:40 AM, Richard Cochran wrote:
> On Mon, Dec 05, 2016 at 02:05:21PM -0600, Grygorii Strashko wrote:
>> @@ -372,34 +354,27 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>> }
>> EXPORT_SYMBOL_GPL(cpts_tx_timestamp);
>>
>> -int cpts_register(struct device *dev, struct cpts *cpts,
>> - u32 mult, u32 shift)
>> +int cpts_register(struct cpts *cpts)
>> {
>> int err, i;
>>
>> - cpts->info = cpts_info;
>> - spin_lock_init(&cpts->lock);
>> -
>> - cpts->cc.read = cpts_systim_read;
>> - cpts->cc.mask = CLOCKSOURCE_MASK(32);
>> - cpts->cc_mult = mult;
>> - cpts->cc.mult = mult;
>> - cpts->cc.shift = shift;
>> -
>> INIT_LIST_HEAD(&cpts->events);
>> INIT_LIST_HEAD(&cpts->pool);
>> for (i = 0; i < CPTS_MAX_EVENTS; i++)
>> list_add(&cpts->pool_data[i].list, &cpts->pool);
>>
>> - cpts_clk_init(dev, cpts);
>> + clk_enable(cpts->refclk);
>> +
>> cpts_write32(cpts, CPTS_EN, control);
>> cpts_write32(cpts, TS_PEND_EN, int_enable);
>>
>> + /* reinitialize cc.mult to original value as it can be modified
>> + * by cpts_ptp_adjfreq().
>> + */
>> + cpts->cc.mult = cpts->cc_mult;
>
> This still isn't quite right. First of all, you shouldn't clobber the
> learned cc.mult value in cpts_register(). Presumably, if PTP had been
> run on this port before, then the learned frequency is approximately
> correct, and it should be left alone.
>
> [ BTW, resetting the timecounter here makes no sense either. Why
> reset the clock just because the interface goes down? ]
>
Huh. This is how it works now (even before my changes) - this is just refactoring!
(really new thing is the only cpts_calc_mult_shift()).
Also, this is how cpts is supported now as part of cpsw (and keystone):
configure cpsw (cpts)
- ifup
cpsw (*soft_reset*, full reconfiguration of cpsw)
(start cpts) - cpts/ptp active
- ifdown
if last netdev - shutdown/poweroff cpsw (cpts)
in other words, cpts/ptp is expected to work once and until at least one cpsw netdev is active.
Also there are additional questions such as:
- is there guarantee that cpsw port will be connected to the same network after ifup?
- should there be possibility to reset cc.mult if it's value will be kept from the previous run?
> Secondly, you have made the initialization order of these fields hard
> to follow. With the whole series applied:
>
> probe()
> cpts_create()
> cpts_of_parse()
> {
> /* Set cc_mult but not cc.mult! */
> set cc_mult
> set cc.shift
> }
> cpts_calc_mult_shift()
> {
> /* Set them both. */
> cpts->cc_mult = mult;
> cpts->cc.mult = mult;
^^ this assignment of cpts->cc.mult not required.
> cpts->cc.shift = shift;
only in case there were not set in DT before
(I have a requirement to support both - DT and cpts_calc_mult_shift and
that introduces a bit of complexity)
Also, I've tried not to add more fields in struct cpts.
> }
> /* later on */
> cpts_register()
> cpts->cc.mult = cpts->cc_mult;
>
> There is no need for such complexity. Simply set cc.mult in
> cpts_create() _once_, immediately after the call to
> cpts_calc_mult_shift().
>
> You can remove the assignment from cpts_calc_mult_shift() and
> cpts_register().
Just to clarify: do you propose to get rid of cpts->cc_mult at all?
static int cpts_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
{
...
if (ppb < 0) {
neg_adj = 1;
ppb = -ppb;
}
mult = cpts->cc_mult;
^^^^^^^^^^^^^^
adj = mult;
adj *= ppb;
diff = div_u64(adj, 1000000000ULL);
...
cpts->cc.mult = neg_adj ? mult - diff : mult + diff;
Honestly, i'd not prefer to change functional behavior of ptp clock as part of
this series.
--
regards,
-grygorii
--
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] i2c: rk3x: keep i2c irq ON in suspend
From: Doug Anderson @ 2016-12-06 16:31 UTC (permalink / raw)
To: David.Wu
Cc: Heiko Stuebner, Wolfram Sang,
linux-arm-kernel@lists.infradead.org,
open list:ARM/Rockchip SoC..., linux-i2c@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <1d570232-af87-467e-e6f4-5b514d583a02@rock-chips.com>
Hi,
On Tue, Dec 6, 2016 at 12:12 AM, David.Wu <david.wu@rock-chips.com> wrote:
> Hi Heiko,
>
> 在 2016/12/5 18:54, Heiko Stuebner 写道:
>>
>> Hi David,
>>
>> Am Montag, 5. Dezember 2016, 16:02:59 CET schrieb David Wu:
>>>
>>> During suspend there may still be some i2c access happening.
>>> And if we don't keep i2c irq ON, there may be i2c access timeout if
>>> i2c is in irq mode of operation.
>>
>>
>> can you describe the issue you're trying to fix a bit more please?
>
>
> Sometimes we could see the i2c timeout errors during suspend/resume, which
> makes the duration of suspend/resume too longer.
>
> [ 484.171541] CPU4: Booted secondary processor [410fd082]
> [ 485.172777] rk3x-i2c ff3c0000.i2c: timeout, ipd: 0x10, state: 1
> [ 486.172760] rk3x-i2c ff3c0000.i2c: timeout, ipd: 0x10, state: 1
> [ 487.172759] rk3x-i2c ff3c0000.i2c: timeout, ipd: 0x10, state: 1
> [ 487.172840] cpu cpu4: _set_opp_voltage: failed to set voltage (800000
> 800000 800000 mV): -110
> [ 487.172874] cpu cpu4: failed to set volt 800000
>
>>
>> I.e. I'd think the i2c-core does suspend i2c-client devices first, so that
>> these should be able to finish up their ongoing transfers and not start
>> any
>> new ones instead?
>>
>> Your irq can still happen slightly after the system started going to
>> actually
>> sleep, so to me it looks like you just widened the window where irqs can
>> be
>> handled. Especially as your irq could also just simply stem from the start
>> state, so you cannot even be sure if your transaction actually is
>> finished.
>
>
> Okay, you are right. I want to give it a double insurance at first, but it
> may hide the unhappend issue.
>
>>
>> So to me it looks like the i2c-connected device driver should be fixed
>> instead?
>
>
> I tell them to fix it in rk808 driver.
To me it seems like perhaps cpufreq should not be changing frequencies
until it is resumed properly. Presumably if all the ordering is done
right then cpufreq should be resumed _after_ the i2c regulator so you
should be OK. ...or am I somehow confused about that?
Also note that previous i2c busses I worked with simply returned -EIO
in the case where they were called when suspended. See
"i2c-exynos5.c" and "i2c-s3c2410.c".
-Doug
^ permalink raw reply
* Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer + regulator support
From: Doug Anderson @ 2016-12-06 16:18 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Jiri Kosina, Brian Norris,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
open list:ARM/Rockchip SoC..., Benjamin Tissoires,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Dmitry Torokhov, Caesar Wang
In-Reply-To: <CAL_Jsq+C1MWrN4Sg_xOgpTzQh9gfL12G5Uxb1ya2MTkoa=7fMA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi,
On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
> <benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> On Dec 05 2016 or thereabouts, Rob Herring wrote:
>>> On Thu, Dec 01, 2016 at 09:24:50AM -0800, Brian Norris wrote:
>>> > Hi Benjamin and Rob,
>>> >
>>> > On Thu, Dec 01, 2016 at 03:34:34PM +0100, Benjamin Tissoires wrote:
>>> > > On Nov 30 2016 or thereabouts, Brian Norris wrote:
>>> > > > From: Caesar Wang <wxt-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>> > > >
>>> > > > Add a compatible string and regulator property for Wacom W9103
>>> > > > digitizer. Its VDD supply may need to be enabled before using it.
>>> > > >
>>> > > > Signed-off-by: Caesar Wang <wxt-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>> > > > Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> > > > Cc: Jiri Kosina <jikos-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> > > > Cc: linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> > > > Signed-off-by: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>> > > > ---
>>> > > > v1 was a few months back. I finally got around to rewriting it based on
>>> > > > DT binding feedback.
>>> > > >
>>> > > > v2:
>>> > > > * add compatible property for wacom
>>> > > > * name the regulator property specifically (VDD)
>>> > > >
>>> > > > Documentation/devicetree/bindings/input/hid-over-i2c.txt | 6 +++++-
>>> > > > 1 file changed, 5 insertions(+), 1 deletion(-)
>>> > > >
>>> > > > diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
>>> > > > index 488edcb264c4..eb98054e60c9 100644
>>> > > > --- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
>>> > > > +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
>>> > > > @@ -11,12 +11,16 @@ If this binding is used, the kernel module i2c-hid will handle the communication
>>> > > > with the device and the generic hid core layer will handle the protocol.
>>> > > >
>>> > > > Required properties:
>>> > > > -- compatible: must be "hid-over-i2c"
>>> > > > +- compatible: must be "hid-over-i2c", or a device-specific string like:
>>> > > > + * "wacom,w9013"
>>> > >
>>> > > NACK on this one.
>>> > >
>>> > > After re-reading the v1 submission I realized Rob asked for this change,
>>> > > but I strongly disagree.
>>> > >
>>> > > HID over I2C is a generic protocol, in the same way HID over USB is. We
>>> > > can not start adding device specifics here, this is opening the can of
>>> > > worms. If the device is a HID one, nothing else should matter. The rest
>>> > > (description of the device, name, etc...) is all provided by the
>>> > > protocol.
>>> >
>>> > I should have spoken up when Rob made the suggestion, because I more or
>>> > less agree with Benjamin here. I don't really see why this needs to have
>>> > a specialized compatible string, as the property is still fairly
>>> > generic, and the entire device handling is via a generic protocol. The
>>> > fact that we manage its power via a regulator is not very
>>> > device-specific.
>>>
>>> It doesn't matter that the protocol is generic. The device attached and
>>> the implementation is not. Implementations have been known to have
>>> bugs/quirks (generally speaking, not HID over I2C in particular). There
>>> are also things outside the scope of what is 'hid-over-i2c' like what's
>>> needed to power-on the device which this patch clearly show.
>>
>> Yes, there are bugs, quirks, even with HID. But the HID declares within
>> the protocol the Vendor ID and the Product ID, which means once we pass
>> the initial "device is ready" step and can do a single i2c write/read,
>> we don't give a crap about device tree anymore.
>>
>> This is just about setting the device in shape so that it can answer a
>> single write/read.
>>
>>>
>>> This is no different than a panel attached via LVDS, eDP, etc., or
>>> USB/PCIe device hard-wired on a board. They all use standard protocols
>>> and all need additional data to describe them. Of course, adding a
>>> single property for a delay would not be a big deal, but it's never
>>> ending. Next you need multiple supplies, GPIO controls, mutiple
>>> delays... This has been discussed to death already. As Thierry Reding
>>> said, you're not special[1].
>>
>> I can somewhat understand what you mean. The official specification is
>> for ACPI. And ACPI allows to calls various settings while querying the
>> _STA method for instance. So in the ACPI world, we don't need to care
>> about regulators or GPIOs because the OEM deals with this in its own
>> blob.
>>
>> Now, coming back to our issue. We are not special, maybe, if he says so.
>> But this really feels like a design choice between putting the burden on
>> device tree and OEMs or in the module maintainers. And I'd rather have
>> the OEM deal with their device than me having to update the module for
>> each generations of hardware. Indeed, this looks like an "endless"
>> amount of quirks, but I'd rather have this endless amount of quirks than
>> having to maintain an endless amount of list of new devices that behaves
>> the same way. We are talking here about "wacom,w9013", but then comes
>> "wacom,w9014" and we need to upgrade the kernel.
>
> No. If the w9014 can claim compatibility with then w9013, then you
> don't need a kernel change. The DT should list the w9014 AND w9013,
> but the kernel only needs to know about the w9013. That is until there
> is some difference which is why the DT should list w9014 to start
> with.
>
> If you don't have any power control to deal with, then the kernel can
> always just match on "hid-over-i2c" compatible.
Just my $0.02. Feel free to ignore.
One thought is that I would say that the need to power on the device
explicitly seems more like a board level difference and less like a
difference associated with a particular digitizer. Said another way,
it seems likely there will be boards with a w9013 without explicit
control of the regulator in software and it seems like there will be
boards with other digitizers where suddenly a new board will come out
that needs explicit control of the regulator.
In this particular case I feel like we can draw a lot of parallels to
an SDIO bus.
When you specify an SDIO bus you don't specify what kind of card will
be present, you just say "I've got an SDIO bus" and then the specific
device underneath is probed. Here we've say "I've got an i2c
connection intended for HID" and then you probe for the HID device
that's on the connection.
Also for an SDIO bus, you've possibly got a regulators / GPIOs /
resets that need to be controlled, but the specific details of these
regulator / GPIOs / resets are specific to a given board and not
necessarily a given SDIO device.
-Doug
^ permalink raw reply
* Re: [RFC] New Device Tree property - "bonding"
From: Laurent Pinchart @ 2016-12-06 16:11 UTC (permalink / raw)
To: Ramesh Shanmugasundaram
Cc: Rob Herring, frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
mark.rutland-5wv7dgnIgG8@public.gmane.org,
pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org,
Chris Paterson, Geert Uytterhoeven,
laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Maxime Ripard
In-Reply-To: <SG2PR06MB1038A0DADECF2EB7795A487AC3820-ESzmfEwOt/zfc7TNChRnj20DtJ1/0DrXvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
Hi Ramesh,
On Tuesday 06 Dec 2016 15:41:06 Ramesh Shanmugasundaram wrote:
> > On Monday 05 Dec 2016 09:57:32 Rob Herring wrote:
> >> On Mon, Dec 5, 2016 at 8:40 AM, Laurent Pinchart wrote:
> >>> On Monday 05 Dec 2016 08:18:34 Rob Herring wrote:
> >>>> On Fri, Nov 25, 2016 at 10:55 AM, Ramesh Shanmugasundaram wrote:
> >>>>> Hello DT maintainers,
> >>>>>
> >>>>> In one of the Renesas SoCs we have a device called DRIF (Digital
> >>>>> Radio Interface) controller. A DRIF channel contains 4 external pins
> >>>>> - SCK, SYNC, Data pins D0 & D1.
> >>>>>
> >>>>> Internally a DRIF channel is made up of two SPI slave devices
> >>>>> (also called sub-channels here) that share common CLK & SYNC
> >>>>> signals but have their own resource set. The DRIF channel can have
> >>>>> either one of the sub-channel active at a time or both. When both
> >>>>> sub-channels are active, they need to be managed together as one
> >>>>> device as they share same CLK & SYNC. We plan to tie these two
> >>>>> sub-channels together with a new property called "renesas,bonding".
> >>>>
> >>>> Is there no need to describe the master device? No GPIOs,
> >>>> regulators or other sideband controls needed? If that's never
> >>>> needed (which seems doubtful), then I would do something different
> >>>> here probably with the master device as a child of one DRIF and
> >>>> then phandles to master from the other DRIFs. Otherwise, this looks
> >>>> fine to me.
> >>>
> >>> Here's a bit of background.
> >>>
> >>> The DRIF is an SPI receiver. It has three input pins, a clock line,
> >>> a data line and a sync signal. The device is designed to be
> >>> connected to a variety of data sources, usually plain SPI (1 data
> >>> line), IIS (1 data line) but also radio tuners that output I/Q data
> >>> (http://www.ni.com/tutorial/4805/en/) over two data lines.
> >>>
> >>> In the case of IQ each data sample is split in two I and Q values
> >>> (typically 16 to 20 bits each in this case), and the values are
> >>> transmitted serially over one data line each. The synchronization
> >>> and clock signals are common to both data lines. The DRIF is
> >>> optimized for this use case as the DRIF instances in the SoC (each
> >>> of them having independent clocks, interrupts and control registers)
> >>> are grouped by two, and the two instances in a group handle a single
> >>> data line each but share the same clock and sync input.
> >>>
> >>> On the software side we need to group the I and Q values, which are
> >>> DMA'ed to memory by the two DRIF instances, and make them available
> >>> to userspace. The V4L2 API used here in SDR (Software Defined Radio)
> >>> mode supports such use cases and exposes a single device node to
> >>> userspace that allows control of the two DRIF instances as a single
> >>> device. To be able to implement this we need kernel code to be aware
> >>> of DRIF groups and, while binding to the DRIF instances separately,
> >>> expose only one V4L2 device to userspace for each group.
> >>>
> >>> There's no master or slave instance from a hardware point of view,
> >>> but the two instances are not interchangeable as they carry separate
> >>> information. They must thus be identified at the driver level.
> >>
> >> By master, I meant the external master device that generates the IQ
> >> data, not which of the internal DRIF blocks is a master of the other.
> >> So back to my question, does the external master device need to be
> >> described? I worry the answer now for a simple case is no, but then
> >> later people are going to have cases needing to describe more. We need
> >> to answer this question first before we can decide what this binding
> >> should look like.
> >
> > Oh yes the external device certainly needs to be described. As it is
> > controlled through a separate, general-purpose I2C or SPI controller, it
> > should be a child node of that controller. The DRIF handles the data
> > interface only, not the control interface of the external device.
>
> Yes, as Laurent mentioned, the external master will be described separately.
> The data interface with the master is described through port nodes. E.g.
>
> port {
> drif0_ep: endpoint {
> remote-endpoint = <&tuner_ep>;
> };
> };
>
> Do we agree on this model please?
That looks good to me.
> >>> Back to the DT bindings, the need to expose relationships between
> >>> (mostly) independent devices is quite common nowadays. It has been
> >>> solved in some cases by creating a separate DT node that does not
> >>> correspond to any physical hardware and whose sole purpose is to
> >>> contain phandles to devices that need to be grouped. Drivers then
> >>> bind to the compatible string of that "virtual" DT node. The
> >>> proposed bonding property is a different approach to solve a similar
> >>> problem. Would it be worth it addressing the problem at a more
> >>> general level and try to design a common solution ?
> >>
> >> We already have somewhat standard ways of grouping, but they are per
> >> type of device (display, sound card, etc.). I'm not sure we gain much
> >> standardizing across these devices and to some extent that ship has
> >> sailed. Even within display subsystems, I don't think there is one
> >> style that fits all. Sometimes a parent subsystem node with children
> >> makes sense for the h/w. Sometimes that doesn't make sense and we have
> >> the virtual node with a "ports" property (like sun8i did). I've never
> >> been too happy with that style largely just because it feels like
> >> we're letting DRM define the binding. However, it's generally extra
> >> data that an OS could just ignore. There have also been many display
> >> bindings that started out with a virtual node and we got rid of it. So
> >> it's not something I like to encourage and we need to be clear when it
> >> is okay or not.
> >>
> >> To state the problem more generally (at least when using OF graph), I
> >> think the problem is simply how do we define and group multiple entry
> >> points to an OF graph. Maybe these should be graph nodes themselves
> >> rather than phandles to the nodes with the entry points of the graph.
> >
> > CC'ing Maxime Ripard for the sun8i side.
>
> Can we use this "bonding" property as another way of grouping similar
> devices? Should I make it generic instead of "renesas,bonding"?
--
Regards,
Laurent Pinchart
--
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 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings
From: Mark Brown @ 2016-12-06 16:11 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Bjorn Andersson, Archit Taneja, linux-arm-msm, robh, dri-devel,
devicetree
In-Reply-To: <4504382.pqKQWZshtj@avalon>
[-- Attachment #1: Type: text/plain, Size: 383 bytes --]
On Tue, Dec 06, 2016 at 06:08:55PM +0200, Laurent Pinchart wrote:
> On Tuesday 06 Dec 2016 13:20:20 Mark Brown wrote:
> > The tiny amount of extra typing involved doesn't seem like much of a
> > cost for keeping things consistent with every other regulator user out
> > there.
> I'm not concerned by that at all, but by the additional runtime complexity :-/
regulator_bulk_()! :)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: drm/bridge: adv7511: Add regulator bindings
From: Laurent Pinchart @ 2016-12-06 16:08 UTC (permalink / raw)
To: Mark Brown
Cc: Bjorn Andersson, Archit Taneja, linux-arm-msm, robh, dri-devel,
devicetree
In-Reply-To: <20161206132020.uzchq63pfpaujg6h@sirena.org.uk>
Hi Mark,
On Tuesday 06 Dec 2016 13:20:20 Mark Brown wrote:
> On Tue, Dec 06, 2016 at 02:46:55PM +0200, Laurent Pinchart wrote:
> > On Tuesday 06 Dec 2016 10:05:17 Mark Brown wrote:
> > > On Mon, Dec 05, 2016 at 11:16:22PM +0200, Laurent Pinchart wrote:
> > > > This has been discussed previously, and Rob agreed that if the
> > > > datasheet recommends to power all supplies from the same regulator we
> > > > can take that as a good hint that a single supply should be enough. In
> > > > the very
> > >
> > > No, don't do this - introducing special snowflake bindings just makes
> > > things more complex at the system level and tells everyone else that
> > > they too can have special snowflake bindings. Someone should be able to
> > > connect up the regulators based purely on a schematic. Just describe
> > > the hardware, it's just one extra line in the DT per regulator.
> >
> > There are power supply pin that have different names but documented as
> > having to be connected to the same supply. I really see no point in
> > having multiple regulators for them.
>
> The tiny amount of extra typing involved doesn't seem like much of a
> cost for keeping things consistent with every other regulator user out
> there.
I'm not concerned by that at all, but by the additional runtime complexity :-/
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v4 2/3] doc: dt: add cyclone-spi binding document
From: Rob Herring @ 2016-12-06 16:05 UTC (permalink / raw)
To: Joshua Clayton
Cc: Alan Tull, Moritz Fischer, Mark Rutland, Russell King,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <137f03de76e5f865430b17ca247e8f73e3315c8d.1480551148.git.stillcompiling-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Thu, Dec 01, 2016 at 09:04:51AM -0800, Joshua Clayton wrote:
> Describe a cyclonei-ps-spi devicetree entry, required features
cyclonei?
>
> Signed-off-by: Joshua Clayton <stillcompiling-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>
> .../bindings/fpga/cyclone-ps-spi-fpga-mgr.txt | 25 ++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/fpga/cyclone-ps-spi-fpga-mgr.txt
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
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
* [PATCH v4 i2c/for-next] i2c: rcar: Add per-Generation fallback bindings
From: Simon Horman @ 2016-12-06 16:01 UTC (permalink / raw)
To: Wolfram Sang
Cc: Magnus Damm, linux-i2c, linux-renesas-soc, Rob Herring,
devicetree, Simon Horman
In the case of Renesas R-Car hardware we know that there are generations of
SoCs, e.g. Gen 2 and Gen 3. But beyond that it's not clear what the
relationship between IP blocks might be. For example, I believe that
r8a7790 is older than r8a7791 but that doesn't imply that the latter is a
descendant of the former or vice versa.
We can, however, by examining the documentation and behaviour of the
hardware at run-time observe that the current driver implementation appears
to be compatible with the IP blocks on SoCs within a given generation.
For the above reasons and convenience when enabling new SoCs a
per-generation fallback compatibility string scheme is being adopted for
drivers for Renesas SoCs.
Also:
* Deprecate renesas,i2c-rcar. It seems poorly named as it is only
compatible with R-Car Gen 1. It also appears unused in mainline.
* Add some text to describe per-SoC bindings
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v5
* s/R8A7797/R8A777/
v4
* Correct grammar in changelog
v3
* Consistently use renesas,<family>-<module> for new compat strings
* Drop RFC designation
v2
* Include accidently omitted i2c-rcar.c portion of patch
---
Documentation/devicetree/bindings/i2c/i2c-rcar.txt | 32 ++++++++++++++--------
drivers/i2c/busses/i2c-rcar.c | 5 +++-
2 files changed, 24 insertions(+), 13 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-rcar.txt b/Documentation/devicetree/bindings/i2c/i2c-rcar.txt
index 239632a0d709..2b8bd33dbf8d 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-rcar.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-rcar.txt
@@ -1,17 +1,25 @@
I2C for R-Car platforms
Required properties:
-- compatible: Must be one of
- "renesas,i2c-rcar"
- "renesas,i2c-r8a7778"
- "renesas,i2c-r8a7779"
- "renesas,i2c-r8a7790"
- "renesas,i2c-r8a7791"
- "renesas,i2c-r8a7792"
- "renesas,i2c-r8a7793"
- "renesas,i2c-r8a7794"
- "renesas,i2c-r8a7795"
- "renesas,i2c-r8a7796"
+- compatible:
+ "renesas,i2c-r8a7778" if the device is a part of a R8A7778 SoC.
+ "renesas,i2c-r8a7779" if the device is a part of a R8A7779 SoC.
+ "renesas,i2c-r8a7790" if the device is a part of a R8A7790 SoC.
+ "renesas,i2c-r8a7791" if the device is a part of a R8A7791 SoC.
+ "renesas,i2c-r8a7792" if the device is a part of a R8A7792 SoC.
+ "renesas,i2c-r8a7793" if the device is a part of a R8A7793 SoC.
+ "renesas,i2c-r8a7794" if the device is a part of a R8A7794 SoC.
+ "renesas,i2c-r8a7795" if the device is a part of a R8A7795 SoC.
+ "renesas,i2c-r8a7796" if the device is a part of a R8A7796 SoC.
+ "renesas,rcar-gen1-i2c" for a generic R-Car Gen1 compatible device.
+ "renesas,rcar-gen2-i2c" for a generic R-Car Gen2 compatible device.
+ "renesas,rcar-gen3-i2c" for a generic R-Car Gen3 compatible device.
+ "renesas,i2c-rcar" (deprecated)
+
+ When compatible with the generic version, nodes must list the
+ SoC-specific version corresponding to the platform first followed
+ by the generic version.
+
- reg: physical base address of the controller and length of memory mapped
region.
- interrupts: interrupt specifier.
@@ -33,7 +41,7 @@ Examples :
i2c0: i2c@e6508000 {
#address-cells = <1>;
#size-cells = <0>;
- compatible = "renesas,i2c-r8a7791";
+ compatible = "renesas,i2c-r8a7791", "renesas,rcar-gen2-i2c";
reg = <0 0xe6508000 0 0x40>;
interrupts = <0 287 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&mstp9_clks R8A7791_CLK_I2C0>;
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 726615e54f2a..622def6b43e2 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -793,7 +793,6 @@ static const struct i2c_algorithm rcar_i2c_algo = {
};
static const struct of_device_id rcar_i2c_dt_ids[] = {
- { .compatible = "renesas,i2c-rcar", .data = (void *)I2C_RCAR_GEN1 },
{ .compatible = "renesas,i2c-r8a7778", .data = (void *)I2C_RCAR_GEN1 },
{ .compatible = "renesas,i2c-r8a7779", .data = (void *)I2C_RCAR_GEN1 },
{ .compatible = "renesas,i2c-r8a7790", .data = (void *)I2C_RCAR_GEN2 },
@@ -803,6 +802,10 @@ static const struct of_device_id rcar_i2c_dt_ids[] = {
{ .compatible = "renesas,i2c-r8a7794", .data = (void *)I2C_RCAR_GEN2 },
{ .compatible = "renesas,i2c-r8a7795", .data = (void *)I2C_RCAR_GEN3 },
{ .compatible = "renesas,i2c-r8a7796", .data = (void *)I2C_RCAR_GEN3 },
+ { .compatible = "renesas,i2c-rcar", .data = (void *)I2C_RCAR_GEN1 }, // Deprecated
+ { .compatible = "renesas,rcar-gen1-i2c", .data = (void *)I2C_RCAR_GEN1 },
+ { .compatible = "renesas,rcar-gen2-i2c", .data = (void *)I2C_RCAR_GEN2 },
+ { .compatible = "renesas,rcar-gen3-i2c", .data = (void *)I2C_RCAR_GEN3 },
{},
};
MODULE_DEVICE_TABLE(of, rcar_i2c_dt_ids);
--
2.7.0.rc3.207.g0ac5344
^ permalink raw reply related
* Re: [RFC] New Device Tree property - "bonding"
From: Rob Herring @ 2016-12-06 15:56 UTC (permalink / raw)
To: Ramesh Shanmugasundaram
Cc: Laurent Pinchart,
frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
mark.rutland-5wv7dgnIgG8@public.gmane.org,
pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org,
Chris Paterson, Geert Uytterhoeven,
laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Maxime Ripard
In-Reply-To: <SG2PR06MB1038A0DADECF2EB7795A487AC3820-ESzmfEwOt/zfc7TNChRnj20DtJ1/0DrXvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
On Tue, Dec 6, 2016 at 9:41 AM, Ramesh Shanmugasundaram
<ramesh.shanmugasundaram-kTT6dE0pTRh9uiUsa/gSgQ@public.gmane.org> wrote:
> Hi Rob, Laurent,
>
> Thanks for the response.
>
>> On Monday 05 Dec 2016 09:57:32 Rob Herring wrote:
>> > On Mon, Dec 5, 2016 at 8:40 AM, Laurent Pinchart wrote:
>> > > On Monday 05 Dec 2016 08:18:34 Rob Herring wrote:
>> > >> On Fri, Nov 25, 2016 at 10:55 AM, Ramesh Shanmugasundaram wrote:
>> > >>> Hello DT maintainers,
>> > >>>
>> > >>> In one of the Renesas SoCs we have a device called DRIF (Digital
>> > >>> Radio
>> > >>> Interface) controller. A DRIF channel contains 4 external pins -
>> > >>> SCK, SYNC, Data pins D0 & D1.
>> > >>>
>> > >>> Internally a DRIF channel is made up of two SPI slave devices
>> > >>> (also called sub-channels here) that share common CLK & SYNC
>> > >>> signals but have their own resource set. The DRIF channel can have
>> > >>> either one of the sub-channel active at a time or both. When both
>> > >>> sub-channels are active, they need to be managed together as one
>> > >>> device as they share same CLK & SYNC. We plan to tie these two
>> > >>> sub-channels together with a new property called "renesas,bonding".
>> > >>
>> > >> Is there no need to describe the master device? No GPIOs,
>> > >> regulators or other sideband controls needed? If that's never
>> > >> needed (which seems doubtful), then I would do something different
>> > >> here probably with the master device as a child of one DRIF and
>> > >> then phandles to master from the other DRIFs. Otherwise, this looks
>> fine to me.
>> > >
>> > > Here's a bit of background.
>> > >
>> > > The DRIF is an SPI receiver. It has three input pins, a clock line,
>> > > a data line and a sync signal. The device is designed to be
>> > > connected to a variety of data sources, usually plain SPI (1 data
>> > > line), IIS (1 data
>> > > line) but also radio tuners that output I/Q data
>> > > (http://www.ni.com/tutorial/4805/en/) over two data lines.
>> > >
>> > > In the case of IQ each data sample is split in two I and Q values
>> > > (typically 16 to 20 bits each in this case), and the values are
>> > > transmitted serially over one data line each. The synchronization
>> > > and clock signals are common to both data lines. The DRIF is
>> > > optimized for this use case as the DRIF instances in the SoC (each
>> > > of them having independent clocks, interrupts and control registers)
>> > > are grouped by two, and the two instances in a group handle a single
>> > > data line each but share the same clock and sync input.
>> > >
>> > > On the software side we need to group the I and Q values, which are
>> > > DMA'ed to memory by the two DRIF instances, and make them available
>> > > to userspace. The V4L2 API used here in SDR (Software Defined Radio)
>> > > mode supports such use cases and exposes a single device node to
>> > > userspace that allows control of the two DRIF instances as a single
>> > > device. To be able to implement this we need kernel code to be aware
>> > > of DRIF groups and, while binding to the DRIF instances separately,
>> > > expose only one V4L2 device to userspace for each group.
>> > >
>> > > There's no master or slave instance from a hardware point of view,
>> > > but the two instances are not interchangeable as they carry separate
>> information.
>> > > They must thus be identified at the driver level.
>> >
>> > By master, I meant the external master device that generates the IQ
>> > data, not which of the internal DRIF blocks is a master of the other.
>> > So back to my question, does the external master device need to be
>> > described? I worry the answer now for a simple case is no, but then
>> > later people are going to have cases needing to describe more. We need
>> > to answer this question first before we can decide what this binding
>> > should look like.
>>
>> Oh yes the external device certainly needs to be described. As it is
>> controlled through a separate, general-purpose I2C or SPI controller, it
>> should be a child node of that controller. The DRIF handles the data
>> interface only, not the control interface of the external device.
>
> Yes, as Laurent mentioned, the external master will be described separately. The data interface with the master is described through port nodes. E.g.
>
> port {
> drif0_ep: endpoint {
> remote-endpoint = <&tuner_ep>;
> };
> };
>
> Do we agree on this model please?
Well, that's not complete as you should have both DRIF0 and DRIF1
having connections to the tuner. Then you can walk the graph and find
everything, and you then don't need the bonding property.
Rob
--
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] ASoC: atmel: tse850: rely on the ssc to register as a cpu dai by itself
From: Rob Herring @ 2016-12-06 15:52 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Rutland, Liam Girdwood,
Mark Brown, Nicolas Ferre, Arnd Bergmann, Greg Kroah-Hartman,
Jaroslav Kysela, Takashi Iwai, devicetree-u79uwXL29TY76Z2rM5mHXA,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1480593549-6464-3-git-send-email-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
On Thu, Dec 01, 2016 at 12:59:09PM +0100, Peter Rosin wrote:
> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> ---
> .../bindings/sound/axentia,tse850-pcm5142.txt | 5 ++---
> sound/soc/atmel/tse850-pcm5142.c | 23 +++-------------------
> 2 files changed, 5 insertions(+), 23 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/sound/axentia,tse850-pcm5142.txt b/Documentation/devicetree/bindings/sound/axentia,tse850-pcm5142.txt
> index 5b9b38f578bb..fd12ecb35b5c 100644
> --- a/Documentation/devicetree/bindings/sound/axentia,tse850-pcm5142.txt
> +++ b/Documentation/devicetree/bindings/sound/axentia,tse850-pcm5142.txt
> @@ -2,8 +2,7 @@ Devicetree bindings for the Axentia TSE-850 audio complex
>
> Required properties:
> - compatible: "axentia,tse850-pcm5142"
> - - axentia,ssc-controller: The phandle of the atmel SSC controller used as
> - cpu dai.
> + - axentia,cpu-dai: The phandle of the cpu dai.
You are breaking backwards compatibility with old DTBs. You either need
to not do that or explain in the commit why that is okay.
Rob
--
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
* [PATCH pci/next v3 3/3] PCI: rcar: Add gen3 fallback compatibility string for pcie-rcar
From: Simon Horman @ 2016-12-06 15:51 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Phil Edworthy, Magnus Damm, linux-pci, linux-renesas-soc,
Rob Herring, devicetree, Simon Horman
In-Reply-To: <1481039491-30364-1-git-send-email-horms+renesas@verge.net.au>
Add fallback compatibility string for the R-Car Gen 3 family. This is in
keeping with the both the existing fallback compatibility string for the
R-Car Gen 2 family and the fallback scheme being adopted wherever
appropriate for drivers for Renesas SoCs.
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
v3
* s/rcar_pcie_hw_init_hw_init/rcar_pcie_hw_init/
v2
* Move fallback binding to below SoC specific bindings it covers
in implementation
---
Documentation/devicetree/bindings/pci/rcar-pci.txt | 1 +
drivers/pci/host/pcie-rcar.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/rcar-pci.txt b/Documentation/devicetree/bindings/pci/rcar-pci.txt
index 6cf99690eef9..eee518db90b9 100644
--- a/Documentation/devicetree/bindings/pci/rcar-pci.txt
+++ b/Documentation/devicetree/bindings/pci/rcar-pci.txt
@@ -7,6 +7,7 @@ compatible: "renesas,pcie-r8a7779" for the R8A7779 SoC;
"renesas,pcie-r8a7793" for the R8A7793 SoC;
"renesas,pcie-r8a7795" for the R8A7795 SoC;
"renesas,pcie-rcar-gen2" for a generic R-Car Gen2 compatible device.
+ "renesas,pcie-rcar-gen3" for a generic R-Car Gen3 compatible device.
When compatible with the generic version, nodes must list the
SoC-specific version corresponding to the platform first
diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
index 85971bc276c6..aca85be101f8 100644
--- a/drivers/pci/host/pcie-rcar.c
+++ b/drivers/pci/host/pcie-rcar.c
@@ -1078,6 +1078,7 @@ static const struct of_device_id rcar_pcie_of_match[] = {
{ .compatible = "renesas,pcie-rcar-gen2",
.data = rcar_pcie_hw_init_gen2 },
{ .compatible = "renesas,pcie-r8a7795", .data = rcar_pcie_hw_init },
+ { .compatible = "renesas,pcie-rcar-gen3", .data = rcar_pcie_hw_init },
{},
};
--
2.7.0.rc3.207.g0ac5344
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox