Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1483505 > unrolled thread

Re: [GIT PULL] Greybus driver subsystem for 4.9-rc1

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [GIT PULL] Greybus driver subsystem for 4.9-rc1 Mark Rutland <mark.rutland@arm.com> - 2016-09-14 19:40 +0200
    Re: [GIT PULL] Greybus driver subsystem for 4.9-rc1 Greg KH <gregkh@linuxfoundation.org> - 2016-09-14 20:10 +0200
      Re: [GIT PULL] Greybus driver subsystem for 4.9-rc1 Joe Perches <joe@perches.com> - 2016-09-14 21:10 +0200
      Re: [GIT PULL] Greybus driver subsystem for 4.9-rc1 Rob Herring <robh@kernel.org> - 2016-09-14 22:10 +0200
        Re: [GIT PULL] Greybus driver subsystem for 4.9-rc1 Greg KH <gregkh@linuxfoundation.org> - 2016-09-15 12:20 +0200
          Re: [GIT PULL] Greybus driver subsystem for 4.9-rc1 Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2016-09-15 13:00 +0200
      Re: [GIT PULL] Greybus driver subsystem for 4.9-rc1 Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2016-09-15 11:40 +0200
        Re: [GIT PULL] Greybus driver subsystem for 4.9-rc1 Mark Rutland <mark.rutland@arm.com> - 2016-09-15 12:20 +0200
          Re: [GIT PULL] Greybus driver subsystem for 4.9-rc1 Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2016-09-15 12:40 +0200
            Re: [GIT PULL] Greybus driver subsystem for 4.9-rc1 Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2016-09-15 12:50 +0200
            Re: [GIT PULL] Greybus driver subsystem for 4.9-rc1 Mark Rutland <mark.rutland@arm.com> - 2016-09-15 13:30 +0200
              Re: [GIT PULL] Greybus driver subsystem for 4.9-rc1 Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2016-09-15 13:50 +0200
                Re: [GIT PULL] Greybus driver subsystem for 4.9-rc1 Mark Rutland <mark.rutland@arm.com> - 2016-09-15 14:50 +0200
                  Re: [GIT PULL] Greybus driver subsystem for 4.9-rc1 Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2016-09-15 17:40 +0200
                    Re: [GIT PULL] Greybus driver subsystem for 4.9-rc1 Mark Rutland <mark.rutland@arm.com> - 2016-09-15 17:50 +0200
                      Re: [GIT PULL] Greybus driver subsystem for 4.9-rc1 Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2016-09-15 18:10 +0200

#1483505 — Re: [GIT PULL] Greybus driver subsystem for 4.9-rc1

Hi Greg,

On Wed, Sep 14, 2016 at 12:09:49PM +0200, Greg KH wrote:
> Given that it's never a good idea to keep subsystems out of the mainline
> kernel, I've put together this pull request that adds the greybus driver
> layer to drivers/greybus/.  Because this was 2 1/2 years of work, with
> many many developers contributing, I didn't want to flatten all of their
> effort into a few small patches, as that wouldn't be very fair.  So I've
> built a git tree with all of the changes going back to the first commit,
> and merged it into the kernel tree, just like btrfs was merged into the
> kernel.

> Unless people point out some major problems with this, I'd like to get
> it merged into 4.9-rc1.

I'm extremely concerned that these patches have *never* seen upstream
review, and this pull request gives no real opportunity for people to
make a judgement regarding the code, as many relevant parties have not
been Cc'd.

From a quick scan of the git tree, I can see code (that isn't even
placed under staging/) for which I have fundamental objections to as a
maintainer, and has not been Cc'd to a relevant list.

For example, I see commit 5a450477311fbfe2 ("greybus: timesync: Add
timesync core driver"). This states that it directly accesses the ARMv7
architected timer, though it's unclear as to precisely what it's doing
since it introduces an (undocumented) compatible string, and what should
be an unnecessary devicetree property.

That's never gone to the linux-arm-kernel mainline list, myself or Marc
(as maintainers of the arch timer driver), nor has the binding seen any
review on the devicetree mailing list.

Given that, for at least that patch, NAK.

Thanks,
Mark.

[toc] | [next] | [standalone]

#1483519

On Wed, Sep 14, 2016 at 06:36:26PM +0100, Mark Rutland wrote:
> Hi Greg,
> 
> On Wed, Sep 14, 2016 at 12:09:49PM +0200, Greg KH wrote:
> > Given that it's never a good idea to keep subsystems out of the mainline
> > kernel, I've put together this pull request that adds the greybus driver
> > layer to drivers/greybus/.  Because this was 2 1/2 years of work, with
> > many many developers contributing, I didn't want to flatten all of their
> > effort into a few small patches, as that wouldn't be very fair.  So I've
> > built a git tree with all of the changes going back to the first commit,
> > and merged it into the kernel tree, just like btrfs was merged into the
> > kernel.
> 
> > Unless people point out some major problems with this, I'd like to get
> > it merged into 4.9-rc1.
> 
> I'm extremely concerned that these patches have *never* seen upstream
> review, and this pull request gives no real opportunity for people to
> make a judgement regarding the code, as many relevant parties have not
> been Cc'd.

As I said, I will send a set of simple patches, I wanted to get this out
as soon as possible and other things came up today.  Will do it in the
morning, sorry.

> From a quick scan of the git tree, I can see code (that isn't even
> placed under staging/) for which I have fundamental objections to as a
> maintainer, and has not been Cc'd to a relevant list.
> 
> For example, I see commit 5a450477311fbfe2 ("greybus: timesync: Add
> timesync core driver"). This states that it directly accesses the ARMv7
> architected timer, though it's unclear as to precisely what it's doing
> since it introduces an (undocumented) compatible string, and what should
> be an unnecessary devicetree property.
> 
> That's never gone to the linux-arm-kernel mainline list, myself or Marc
> (as maintainers of the arch timer driver), nor has the binding seen any
> review on the devicetree mailing list.

Hm, odd, I thought we had Rob review all of the device tree bindings,
but maybe the timesync stuff missed him.  And timesync is "odd" to say
the least, wait until you see the firmware side of it :)

Let me post the patches tomorrow and then we can review them.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1483559

On Wed, 2016-09-14 at 20:29 +0200, Greg KH wrote:

trivial note:

> +static size_t gb_timesync_log_frame_time(struct gb_timesync_svc *timesync_svc,
> +					 char *buf, size_t buflen)
> +{
> +	struct gb_svc *svc = timesync_svc->svc;
> +	struct gb_host_device *hd;
> +	struct gb_timesync_interface *timesync_interface;
> +	struct gb_interface *interface;
> +	unsigned int len;
> +	size_t off;
> +
> +	/* AP/SVC */
> +	off = snprintf(buf, buflen, "%s frametime: ap=%llu %s=%llu ",
> +		       greybus_bus_type.name,
> +		       timesync_svc->ap_ping_frame_time, dev_name(&svc->dev),
> +		       timesync_svc->svc_ping_frame_time);
> +	len = buflen - off;
> +
> +	/* APB/GPB */
> +	if (len < buflen) {
> +		hd = timesync_svc->timesync_hd->hd;
> +		off += snprintf(&buf[off], len, "%s=%llu ", dev_name(&hd->dev),
> +				timesync_svc->timesync_hd->ping_frame_time);
> +		len = buflen - off;
> +	}
> +
> +	list_for_each_entry(timesync_interface,
> +			    &timesync_svc->interface_list, list) {
> +		if (len < buflen) {
> +			interface = timesync_interface->interface;
> +			off += snprintf(&buf[off], len, "%s=%llu ",
> +					dev_name(&interface->dev),
> +					timesync_interface->ping_frame_time);
> +			len = buflen - off;
> +		}
> +	}
> +	if (len < buflen)
> +		off += snprintf(&buf[off], len, "\n");
> +	return off;
> +}

The unnecessary trailing blank can be avoided by converting

	snprintf(, "format ', ...);
	for (...)
		snprintf(, "format ", ...);
	snprintf(, "\n");

to

	snprintf(, "format', ...);
	for (...)
		snprintf(, " format", ...);
	snprintf(, "\n");

> 

[toc] | [prev] | [next] | [standalone]

#1483603

On Wed, Sep 14, 2016 at 1:07 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Sep 14, 2016 at 06:36:26PM +0100, Mark Rutland wrote:
>> Hi Greg,
>>
>> On Wed, Sep 14, 2016 at 12:09:49PM +0200, Greg KH wrote:
>> > Given that it's never a good idea to keep subsystems out of the mainline
>> > kernel, I've put together this pull request that adds the greybus driver
>> > layer to drivers/greybus/.  Because this was 2 1/2 years of work, with
>> > many many developers contributing, I didn't want to flatten all of their
>> > effort into a few small patches, as that wouldn't be very fair.  So I've
>> > built a git tree with all of the changes going back to the first commit,
>> > and merged it into the kernel tree, just like btrfs was merged into the
>> > kernel.
>>
>> > Unless people point out some major problems with this, I'd like to get
>> > it merged into 4.9-rc1.
>>
>> I'm extremely concerned that these patches have *never* seen upstream
>> review, and this pull request gives no real opportunity for people to
>> make a judgement regarding the code, as many relevant parties have not
>> been Cc'd.
>
> As I said, I will send a set of simple patches, I wanted to get this out
> as soon as possible and other things came up today.  Will do it in the
> morning, sorry.
>
>> From a quick scan of the git tree, I can see code (that isn't even
>> placed under staging/) for which I have fundamental objections to as a
>> maintainer, and has not been Cc'd to a relevant list.
>>
>> For example, I see commit 5a450477311fbfe2 ("greybus: timesync: Add
>> timesync core driver"). This states that it directly accesses the ARMv7
>> architected timer, though it's unclear as to precisely what it's doing
>> since it introduces an (undocumented) compatible string, and what should
>> be an unnecessary devicetree property.
>>
>> That's never gone to the linux-arm-kernel mainline list, myself or Marc
>> (as maintainers of the arch timer driver), nor has the binding seen any
>> review on the devicetree mailing list.
>
> Hm, odd, I thought we had Rob review all of the device tree bindings,
> but maybe the timesync stuff missed him.  And timesync is "odd" to say
> the least, wait until you see the firmware side of it :)

I have not. There weren't any when I was involved (other than SOC
related bindings). Some like USB devices were discussed at least, but
at the time there was no common definition of how to deal with
soldered, on-board USB devices. And that was also what's good enough
to move forward with development, not ready for mainline. There's a
binding now for USB, but what's there for arche predates it IIRC. This
is the first I've heard of timesync having a binding. I can't imagine
why it needs one.

I was going to stay out of this, but now that I'm here...

I'm not all that worried about the quality of the code, but do
question whether this really makes sense to merge at this time. While
I worked on Ara and would like to see all this code be used, I'm
pretty doubtful it will be. Yes, Motorola is using a version of it,
but they forked it some time back and changed who knows what. So what
is upstream won't likely even work with any publicly available device.
And how long that Motorola phone lasts is unknown. Sure it is self
contained, but maintenance to keep it in tree is not 0.

There's also things that never got solved. Like how do you describe
devices on I2C, SPI, UART, etc. behind a greybus device? The plan was
to use DT overlays, but that was never solved and brings a whole set
of problems to solve upstream.

Rob

[toc] | [prev] | [next] | [standalone]

#1483952

On Wed, Sep 14, 2016 at 03:07:22PM -0500, Rob Herring wrote:
> On Wed, Sep 14, 2016 at 1:07 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Sep 14, 2016 at 06:36:26PM +0100, Mark Rutland wrote:
> >> Hi Greg,
> >>
> >> On Wed, Sep 14, 2016 at 12:09:49PM +0200, Greg KH wrote:
> >> > Given that it's never a good idea to keep subsystems out of the mainline
> >> > kernel, I've put together this pull request that adds the greybus driver
> >> > layer to drivers/greybus/.  Because this was 2 1/2 years of work, with
> >> > many many developers contributing, I didn't want to flatten all of their
> >> > effort into a few small patches, as that wouldn't be very fair.  So I've
> >> > built a git tree with all of the changes going back to the first commit,
> >> > and merged it into the kernel tree, just like btrfs was merged into the
> >> > kernel.
> >>
> >> > Unless people point out some major problems with this, I'd like to get
> >> > it merged into 4.9-rc1.
> >>
> >> I'm extremely concerned that these patches have *never* seen upstream
> >> review, and this pull request gives no real opportunity for people to
> >> make a judgement regarding the code, as many relevant parties have not
> >> been Cc'd.
> >
> > As I said, I will send a set of simple patches, I wanted to get this out
> > as soon as possible and other things came up today.  Will do it in the
> > morning, sorry.
> >
> >> From a quick scan of the git tree, I can see code (that isn't even
> >> placed under staging/) for which I have fundamental objections to as a
> >> maintainer, and has not been Cc'd to a relevant list.
> >>
> >> For example, I see commit 5a450477311fbfe2 ("greybus: timesync: Add
> >> timesync core driver"). This states that it directly accesses the ARMv7
> >> architected timer, though it's unclear as to precisely what it's doing
> >> since it introduces an (undocumented) compatible string, and what should
> >> be an unnecessary devicetree property.
> >>
> >> That's never gone to the linux-arm-kernel mainline list, myself or Marc
> >> (as maintainers of the arch timer driver), nor has the binding seen any
> >> review on the devicetree mailing list.
> >
> > Hm, odd, I thought we had Rob review all of the device tree bindings,
> > but maybe the timesync stuff missed him.  And timesync is "odd" to say
> > the least, wait until you see the firmware side of it :)
> 
> I have not. There weren't any when I was involved (other than SOC
> related bindings). Some like USB devices were discussed at least, but
> at the time there was no common definition of how to deal with
> soldered, on-board USB devices. And that was also what's good enough
> to move forward with development, not ready for mainline. There's a
> binding now for USB, but what's there for arche predates it IIRC. This
> is the first I've heard of timesync having a binding. I can't imagine
> why it needs one.

Ah, I'll let Bryan answer that one :)

> I was going to stay out of this, but now that I'm here...
> 
> I'm not all that worried about the quality of the code, but do
> question whether this really makes sense to merge at this time. While
> I worked on Ara and would like to see all this code be used, I'm
> pretty doubtful it will be. Yes, Motorola is using a version of it,
> but they forked it some time back and changed who knows what. So what
> is upstream won't likely even work with any publicly available device.
> And how long that Motorola phone lasts is unknown. Sure it is self
> contained, but maintenance to keep it in tree is not 0.

We have people who are willing to maintain it (me and Johan) and others
on the cc: have expressed interest in doing things with it (Rui has
great ideas about extending gbsim in ways to make it easier to test and
use).

And there is never a restriction on accepting kernel code for "publicly
availble devices only", remember, we have ripped CPU code out of the
kernel for chips that never shipped.  It just has to not affect others
in ways that would affect them.

I merge new driver subsystems to the kernel every release that are much
larger than this, and have almost no users out there, so this shouldn't
be a surprise to anyone (who here has a most-bus device?  A unisys
visorbus device?  MCB device?  I can go on :)

And with gbsim, you can run this code today on your laptop, or directly
on a target system like a beagle bone black or minnowboard.  There's a
talk about how to use greybus over IP at ELC in a few weeks, and someone
is working on porting the firmware side to an arduino to make it simper
to control the devices on such a target from a host computer using the
greybus protocol.

And getting Motorola to merge back in with this upstream code is a good
goal, having it upstream is a neutral place where everyone can work
together on it.  Keeping it in a random github project makes forks
almost inevitable, as we all know.

> There's also things that never got solved. Like how do you describe
> devices on I2C, SPI, UART, etc. behind a greybus device? The plan was
> to use DT overlays, but that was never solved and brings a whole set
> of problems to solve upstream.

That is only an issue if you want to bind a kernel driver for an
existing i2c/spi chip to an i2c/spi greybus device.  With the code we
have today, we do it for a specific SPI chip (for firmware download),
but rely on everything to be userspace-only accesses to make it simpler
at this point in time.

When DT overlays get more settled down, yes, I want to revisit this idea
of how to do it for greybus devices, but that's a long-term goal and is
not required at all right now to have a working system and devices.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1483989

On Thu, 2016-09-15 at 12:17 +0200, Greg KH wrote:
> This
> > is the first I've heard of timesync having a binding. I can't
> imagine
> > why it needs one.
> 
> Ah, I'll let Bryan answer that one :)

It's possible we could drop the binding. It was needed to describe the
register location of the MMIO architectural register on MSM8994. I have
this binding ATM the describe fact that get_cycles() on MSM8994 returns
a free-running counter, clocked by refclk and that refclk is provided
to each processor that want to do FrameTime - i.e. the clock driving
get_cycles() comes from PMIC and drives the relevant PLLs on the
downstream processors clocking their respective TMR blocks.

static const struct of_device_id arch_timer_of_match[] = {
        { .compatible   = "google,greybus-frame-time-counter", },
        {},
};

I'm not aware of a corresponding kernel API that describes the
frequency get_cycles() operates at but if there is one then there's no
need for this binding.

[toc] | [prev] | [next] | [standalone]

#1483933

On Wed, 2016-09-14 at 20:29 +0200, Greg KH wrote:
> On Wed, Sep 14, 2016 at 08:07:54PM +0200, Greg KH wrote:
> > 
> > On Wed, Sep 14, 2016 at 06:36:26PM +0100, Mark Rutland wrote:
> > > 
> > > Hi Greg,
> > > 
> > > On Wed, Sep 14, 2016 at 12:09:49PM +0200, Greg KH wrote:
> > > > 
> > > > Given that it's never a good idea to keep subsystems out of the
> > > > mainline
> > > > kernel, I've put together this pull request that adds the
> > > > greybus driver
> > > > layer to drivers/greybus/.  Because this was 2 1/2 years of
> > > > work, with
> > > > many many developers contributing, I didn't want to flatten all
> > > > of their
> > > > effort into a few small patches, as that wouldn't be very
> > > > fair.  So I've
> > > > built a git tree with all of the changes going back to the
> > > > first commit,
> > > > and merged it into the kernel tree, just like btrfs was merged
> > > > into the
> > > > kernel.
> > > > 
> > > > Unless people point out some major problems with this, I'd like
> > > > to get
> > > > it merged into 4.9-rc1.
> > > I'm extremely concerned that these patches have *never* seen
> > > upstream
> > > review, and this pull request gives no real opportunity for
> > > people to
> > > make a judgement regarding the code, as many relevant parties
> > > have not
> > > been Cc'd.
> > As I said, I will send a set of simple patches, I wanted to get
> > this out
> > as soon as possible and other things came up today.  Will do it in
> > the
> > morning, sorry.
> Here's the timesync code pulled out into a simple patch if you want
> to
> see it.
> 
> Bryan, any explanations you want to provide that would help in
> clarifying Mark's issues?

As Douglas Adams would say - "don't panic".

If you look at the final state the code ends up in - we're doing
get_cycles(); as opposed to reading an architectural timer directly.

u64 gb_timesync_platform_get_counter(void)
{
        return (u64)get_cycles();
}

You have the entire git history - from the early days where we were reading one of the unused ARMv8 timers the MSM8994 has to the later days where we just do get_cycles()...

At the time when we first started writing the code it wasn't 100% clear if get_cycles() would do, so it was safer to allocate an unused architectural timer and read it directly. Later on and with some experimentation it was possible to switch to get_cycles().

---
bod

[toc] | [prev] | [next] | [standalone]

#1483949

On Thu, Sep 15, 2016 at 10:35:33AM +0100, Bryan O'Donoghue wrote:
> On Wed, 2016-09-14 at 20:29 +0200, Greg KH wrote:
> > Bryan, any explanations you want to provide that would help in
> > clarifying Mark's issues?
> 
> As Douglas Adams would say - "don't panic".
> 
> If you look at the final state the code ends up in - we're doing
> get_cycles(); as opposed to reading an architectural timer directly.
>
> u64 gb_timesync_platform_get_counter(void)
> {
>         return (u64)get_cycles();
> }

I did in fact notice this, though my wording was somewhat unclear on
that part.

> You have the entire git history - from the early days where we were
> reading one of the unused ARMv8 timers the MSM8994 has to the later
> days where we just do get_cycles()...
>
> At the time when we first started writing the code it wasn't 100%
> clear if get_cycles() would do, so it was safer to allocate an unused
> architectural timer and read it directly. Later on and with some
> experimentation it was possible to switch to get_cycles().

I don't think the history matters, and I don't think that one can rely
on get_cycles() in this manner outside of arch code. Looking at the
state of the tree [1] as of the final commit [2] in the greybus branch,
my points still stand:

* The "google,greybus-frame-time-counter" node is superfluous. It does
  not describe a particular device, and duplicates information we have
  elsewhere. It does not explicitly define the relationship with the
  underlying clocksource.

* The clock-frequency property isn't necessary. The architected timer
  drivers know the frequency of the architected timers (MMIO or sysreg),
  and they should be queried as to the frequency.

* In general, get_cycles() isn't guaranteed to be any clocksource or
  cycle counter in particular, so even if the clock-frequency property
  matches the architected timer, that doesn't help.

Beyond that, the fallback code using cpufreq and presumably an actual
cycle counter will be broken in a number of cases -- cycle counts will
not match across CPUs, and on CPUs whic gate clocks in WFI/WFE, they'll
drift randomly.

Per the comment at the top of the file, it looks like you want a
system-wide stable clocksource. If you want that, you need to use a
generic API that allows drivers and arch code to actually provide that,
rather than building one yourself that doesn't work.

If you're trying to synchronise with other agents in the system that are
reading from the MMIO arch timers, then that relationship should be
described explicitly in the DT.

Thanks,
Mark.
 
[1] https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/tree/drivers/greybus/timesync_platform.c?h=greybus&id=f86bfc90a401681838866b5f6826d86cc4c7010c
[2] https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=greybus&id=f86bfc90a401681838866b5f6826d86cc4c7010c

[toc] | [prev] | [next] | [standalone]

#1483959

On Thu, 2016-09-15 at 11:13 +0100, Mark Rutland wrote:
> On Thu, Sep 15, 2016 at 10:35:33AM +0100, Bryan O'Donoghue wrote:
> > 
> I don't think the history matters, 

Your comment seemed to indicate you thought we were reading a
architectural timer directly - which we aren't.

> and I don't think that one can rely
> on get_cycles() in this manner outside of arch code.

I don't follow your meaning. What's wrong with get_cycles() ? You've
already said you don't think reading an architectural timer directly is
correct.

The objective is to read one of the free-running counters in MSM8994,
clocked by the PMIC. The refclk provided by PMIC is distributed to each
processor in the system.

>  Looking at the
> state of the tree [1] as of the final commit [2] in the greybus
> branch,
> my points still stand:
> 
> * The "google,greybus-frame-time-counter" node is superfluous. It
> does
>   not describe a particular device,

It describes a timer running @ 19.2MHz, clocked by PMIC refclk.

>  and duplicates information we have
>   elsewhere.

Can you give an example ?

>  It does not explicitly define the relationship with the
>   underlying clocksource.


> * The clock-frequency property isn't necessary. The architected timer
>   drivers know the frequency of the architected timers (MMIO or
> sysreg),
>   and they should be queried as to the frequency.

OK so if I'm understanding you. You think get_cycles() is fine but that
instead of encoding a "greybus-frame-time-counter" the platform code
should interrogate the frequency provided - is that correct ?

> Beyond that, the fallback code using cpufreq and presumably an actual
> cycle counter will be broken in a number of cases

Of course the fallback will be broken... it's not supposed to work if
you don't have a timer that can be used - just compile, run and print a
complaint - i.e., this won't really do FrameTime on an x86... then
again since so much of the underlying greybus/unipro hardware -
requires a 19.2MHz refclk - if you were to try to do greybus on x86
you'd need to solve that problem.

> 
> Per the comment at the top of the file, it looks like you want a
> system-wide stable clocksource. If you want that, you need to use a
> generic API that allows drivers and arch code to actually provide
> that,
> rather than building one yourself that doesn't work.

Hmm. The objective is to read one of the timers clocked by the PMIC
refclk input. refclk is provided to each processor in the system - and
on MSM8994 clocks the MMIO timers. It's used to drive the PLL on the
other processors - which in turn drive the timers that the Modules use
to read their own local counters. We want to read that counter on MSM
directly - get_cycles() has worked nicely so far.

> 
> If you're trying to synchronise with other agents in the system that
> are
> reading from the MMIO arch timers,

No. The MMIO timers are useful only to the MSM. We don't have any type
of parallel (or serial) bus that can access that on-chip resource.

MSM8994 -- > USB
             APBridge (timer) -> UniPro bus
                                            -> Module with a UART
                                            -> Module with a GPIO
                                            -> Module with an etc, etc
                              -> SPI bus
                                            -> SVC
                                               Owns FrameTime

So the SVC owns FrameTime and diseminates that to other entities in the
system by way of a GPIO and greybus. It's up to the MSM8994 to select a
timer that works for it - the other processors in the system are
responsible for their own timers.

---
bod

[toc] | [prev] | [next] | [standalone]

#1483964

On Thu, 2016-09-15 at 11:35 +0100, Bryan O'Donoghue wrote:
> 

Here's a slightly better diagram.

PMIC -> refclk provided to each (timer) element below.

MSM8994(timer) -- > USB
WD8a
               APBridgeA (timer) -> UniPro bus
               WD8a
                                            -> Module(timer) with UART
                                               WD1
                                            -> Module(timer) with GPIO
                                               WD2
                                            -> Module(timer) with blah
                                               WD3
                              -> SPI bus
                                            -> SVC(timer)
                                               Owns FrameTime
                                               GPIO {WD0...WDn}

So yes, each processor has it's own timer. We aren't trying to read the
MSM's FrameTime.

---
bod

[toc] | [prev] | [next] | [standalone]

#1484001

Hi,

More questions below. Perhaps some of these will be implicitly answered
when the linearised patches appear, and I'm happy to wait until then to
continue the discussion, as I suspect otherwise we're all likely to end
up exasperated.

Please do Cc me on those.

Regardless, until those appear and a reasonable time has been given for
replies, my comments regarding the lack of review stand, as does my NAK
for the series.

On Thu, Sep 15, 2016 at 11:35:56AM +0100, Bryan O'Donoghue wrote:
> On Thu, 2016-09-15 at 11:13 +0100, Mark Rutland wrote:
> > On Thu, Sep 15, 2016 at 10:35:33AM +0100, Bryan O'Donoghue wrote:
> > > 
> > I don't think the history matters, 
> 
> Your comment seemed to indicate you thought we were reading a
> architectural timer directly - which we aren't.

Sure, and as I pointed out, the comment in the HEAD commit still claims
it does, even if the code doesn't. This is at best, confusing, and the
history of how it came to be there doesn't really matter...

> > and I don't think that one can rely
> > on get_cycles() in this manner outside of arch code.
> 
> I don't follow your meaning. What's wrong with get_cycles() ? You've
> already said you don't think reading an architectural timer directly is
> correct.

I pointed out a number of issues in my previous reply.

For example, you have absolutely no guarantee as to what backs
get_cycles(). Despite this, the code assumes that get_cycles() is backed
by something running at the frequency described in a
"google,greybus-frame-time-counter" node.

Even if this *happens* to match what some piece of arch code provides
today on some platform, it is in no way *guaranteed*.

> The objective is to read one of the free-running counters in MSM8994,
> clocked by the PMIC. The refclk provided by PMIC is distributed to each
> processor in the system.
> 
> >  Looking at the
> > state of the tree [1] as of the final commit [2] in the greybus
> > branch,
> > my points still stand:
> > 
> > * The "google,greybus-frame-time-counter" node is superfluous. It
> > does
> >   not describe a particular device,
> 
> It describes a timer running @ 19.2MHz, clocked by PMIC refclk.

... which you assume is whatever backs get_cycles(), which you in
practice assume is the architected timer. For which we *already* have a
binding and driver.

Given that, as far as I can tell, "google,greybus-frame-time-counter"
describes a software construct that uses this, not an actual piece of
hardware.

> >  and duplicates information we have   elsewhere.
> 
> Can you give an example ?

Trivially, the CNTFRQ register in the architected timer (which is common
across MMIO/sysreg), which you can query with arch_timer_get_rate().

Note that isn't guaranteed to match get_cycles() either. You need a
better API to call.

> > * The clock-frequency property isn't necessary. The architected timer
> >   drivers know the frequency of the architected timers (MMIO or
> > sysreg),
> >   and they should be queried as to the frequency.
> 
> OK so if I'm understanding you. You think get_cycles() is fine but that
> instead of encoding a "greybus-frame-time-counter" the platform code
> should interrogate the frequency provided - is that correct ?

You should definitely interrogate the relevant driver, somehow.

Without a higher-level view of what you're trying to achieve, it's not
clear to me whether get_cycles() is the right interface.

> > Beyond that, the fallback code using cpufreq and presumably an actual
> > cycle counter will be broken in a number of cases
> 
> Of course the fallback will be broken... it's not supposed to work if
> you don't have a timer that can be used - just compile, run and print a
> complaint - i.e., this won't really do FrameTime on an x86... then
> again since so much of the underlying greybus/unipro hardware -
> requires a 19.2MHz refclk - if you were to try to do greybus on x86
> you'd need to solve that problem.

If it's never going to work, why give the illusion that it might?

If you don't have the necessary prerequisites, fail to probe entirely.
Prevent drivers that depend on the non-existent functionality from
probing, or have them avoid the facility which is not available.

Don't provide them with something that can appear to work for a while,
yet will fall over in a breeze.

> > Per the comment at the top of the file, it looks like you want a
> > system-wide stable clocksource. If you want that, you need to use a
> > generic API that allows drivers and arch code to actually provide
> > that, rather than building one yourself that doesn't work.
> 
> Hmm. The objective is to read one of the timers clocked by the PMIC
> refclk input. refclk is provided to each processor in the system - and
> on MSM8994 clocks the MMIO timers. It's used to drive the PLL on the
> other processors - which in turn drive the timers that the Modules use
> to read their own local counters. We want to read that counter on MSM
> directly - get_cycles() has worked nicely so far.

This is too low-level for me to see what you're actually trying to
achieve. The fact that you want to read a specific timer is an
implementation detail.

Why can't you use any other timer? Correctness? Performance? (Why) is
the fact that the PLL drives module timers important?

> > If you're trying to synchronise with other agents in the system that
> > are reading from the MMIO arch timers,
> 
> No. The MMIO timers are useful only to the MSM. We don't have any type
> of parallel (or serial) bus that can access that on-chip resource.

To be clear, by "other agents in the system", I'm also asking about
other devices within the SoC (e.g. anything other than the CPUs running
this instance of Linux).

> MSM8994 -- > USB
>              APBridge (timer) -> UniPro bus
>                                             -> Module with a UART
>                                             -> Module with a GPIO
>                                             -> Module with an etc, etc
>                               -> SPI bus
>                                             -> SVC
>                                                Owns FrameTime
> 
> So the SVC owns FrameTime and diseminates that to other entities in the
> system by way of a GPIO and greybus. It's up to the MSM8994 to select a
> timer that works for it - the other processors in the system are
> responsible for their own timers.

Sorry, but this doesn't clarify much from my PoV.

* What are the requirements for that timer? 

* Is there_any_ implicit relationship with the module timers derived
  from the fact they share a parent PLL? Is that a requirement somehow?

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1484023

On Thu, 2016-09-15 at 12:20 +0100, Mark Rutland wrote:
> Hi,
> 
> More questions below. Perhaps some of these will be implicitly
> answered
> when the linearised patches appear, and I'm happy to wait until then
> to
> continue the discussion, as I suspect otherwise we're all likely to
> end
> up exasperated.
> 
> Please do Cc me on those.
> 
> Regardless, until those appear and a reasonable time has been given
> for
> replies, my comments regarding the lack of review stand, as does my
> NAK
> for the series.
> 
> On Thu, Sep 15, 2016 at 11:35:56AM +0100, Bryan O'Donoghue wrote:
> > 
> > On Thu, 2016-09-15 at 11:13 +0100, Mark Rutland wrote:
> > > 
> > > On Thu, Sep 15, 2016 at 10:35:33AM +0100, Bryan O'Donoghue wrote:
> > > > 
> > > >  
> > > I don't think the history matters, 
> > Your comment seemed to indicate you thought we were reading a
> > architectural timer directly - which we aren't.
> Sure, and as I pointed out, the comment in the HEAD commit still
> claims
> it does, even if the code doesn't. This is at best, confusing, and
> the
> history of how it came to be there doesn't really matter...

TBH a whole git history will invariably contain things developers did,
thought better of and then backed out as is the case here.

> 
> > 
> > > 
> > > and I don't think that one can rely
> > > on get_cycles() in this manner outside of arch code.
> > I don't follow your meaning. What's wrong with get_cycles() ?
> > You've
> > already said you don't think reading an architectural timer
> > directly is
> > correct.
> I pointed out a number of issues in my previous reply.

On MSM8994 the timer backing get_cycles() is one of the MMIO
architectural timers (which is why I switched over in the end). There's
not much else that can be done bar custom silicon - this particular
timer is as good as it gets, more of a "how do we synchronise time with
the hardware we have" than a "lets design in a feature to synchronise
time" - which was something we were focusing in on for later
silicon... 

> 
> For example, you have absolutely no guarantee as to what backs
> get_cycles(). Despite this, the code assumes that get_cycles() is
> backed
> by something running at the frequency described in a
> "google,greybus-frame-time-counter" node.
> 
> Even if this *happens* to match what some piece of arch code provides
> today on some platform, it is in no way *guaranteed*.

That's the point though, if you declare "google,greybus-frame-time-
counter" in your platform code - then you can use 'get_cycles()' in
this manner - if not - then you need to take steps in your own new
platform to provide that same level of functionality. You could switch
to an MSM8996 or an MSM8998 declare this node and bob's your uncle.

OTOH declaring this node on x86 would be a bit pointless. You'd be
better off providing a timer on a PCI bar, and binding that into
greybus with some x86/x86-platform code...

> 
> > 
> > The objective is to read one of the free-running counters in
> > MSM8994,
> > clocked by the PMIC. The refclk provided by PMIC is distributed to
> > each
> > processor in the system.
> > 
> > > 
> > >  Looking at the
> > > state of the tree [1] as of the final commit [2] in the greybus
> > > branch,
> > > my points still stand:
> > > 
> > > * The "google,greybus-frame-time-counter" node is superfluous. It
> > > does
> > >   not describe a particular device,
> > It describes a timer running @ 19.2MHz, clocked by PMIC refclk.
> ... which you assume is whatever backs get_cycles(), which you in
> practice assume is the architected timer. For which we *already* have
> a
> binding and driver.

It's a requirement rather than assumption. If you declare that node,
it's assumed the timer driving get_cycles() does what it says on the
greybus-frame-time-counter tin.

> > >  and duplicates information we have   elsewhere.
> > Can you give an example ?
> Trivially, the CNTFRQ register in the architected timer (which is
> common
> across MMIO/sysreg), which you can query with arch_timer_get_rate().
> 
> Note that isn't guaranteed to match get_cycles() either. You need a
> better API to call.

In that case a DT entry makes sense I'd say.

> You should definitely interrogate the relevant driver, somehow.

Hrmm. TBH if we are ruling out arch_timer_get_rate() then I think a DT
entry (which BTW is greybus specific) is the more intelligent way
forward. The greybus platform implementer needs to understand the
dependencies and take action to meet those dependencies should he or
she wish to support this feature.

I'm not opposed necessarily to calling arch_timer_get_rate() instead of
a DT binding, assuming it works, and drawing a line under it for
MSM8994. As I've said it's up to a system architect for other platforms
to go and do the necessary design to support this feature and this will
almost certainly require new platform code both here and in other
places anyway.

> 
> Without a higher-level view of what you're trying to achieve, it's
> not
> clear to me whether get_cycles() is the right interface.

I appreciate that.

---
bod

[toc] | [prev] | [next] | [standalone]

#1484129

On Thu, Sep 15, 2016 at 12:48:08PM +0100, Bryan O'Donoghue wrote:
> On Thu, 2016-09-15 at 12:20 +0100, Mark Rutland wrote:
> > For example, you have absolutely no guarantee as to what backs
> > get_cycles(). Despite this, the code assumes that get_cycles() is
> > backed by something running at the frequency described in a
> > "google,greybus-frame-time-counter" node.
> > 
> > Even if this *happens* to match what some piece of arch code provides
> > today on some platform, it is in no way *guaranteed*.
> 
> That's the point though, if you declare "google,greybus-frame-time-
> counter" in your platform code - then you can use 'get_cycles()' in
> this manner

To be clear, *some* properties (and perhaps additional nodes) may need
to be in the DT, in order to capture the hardware property or
relationship that you are reliant upon.

However, the DT cannot possibly know anything about get_cycles(), as
get_cycles() is a kernel implementation details that's subject to
arbitrary change at any point in time, independent of the DT.

The "google,greybus-frame-time-counter" node, as it stands, does not
capture any relevant hardware detail, and does not belong in the DT.

> > Without a higher-level view of what you're trying to achieve, it's
> > not clear to me whether get_cycles() is the right interface.
> 
> I appreciate that.

Until that's clarified, we won't make any progress here.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1484329

On Thu, 2016-09-15 at 13:46 +0100, Mark Rutland wrote:
> On Thu, Sep 15, 2016 at 12:48:08PM +0100, Bryan O'Donoghue wrote:
> > 
> > On Thu, 2016-09-15 at 12:20 +0100, Mark Rutland wrote:
> > > 
> > > For example, you have absolutely no guarantee as to what backs
> > > get_cycles(). Despite this, the code assumes that get_cycles() is
> > > backed by something running at the frequency described in a
> > > "google,greybus-frame-time-counter" node.
> > > 
> > > Even if this *happens* to match what some piece of arch code
> > > provides
> > > today on some platform, it is in no way *guaranteed*.
> > That's the point though, if you declare "google,greybus-frame-time-
> > counter" in your platform code - then you can use 'get_cycles()' in
> > this manner
> To be clear, *some* properties (and perhaps additional nodes) may
> need
> to be in the DT, in order to capture the hardware property or
> relationship that you are reliant upon.

Sure but on the relevant platform we know 

1. get_cycles() is derived from an architectured MMIO timer
2. What is clocking that timer

So any platform that declares that property must be aware of what its
doing.

For clarity this is the alternative to reading another register
directly bypassing get_cycles().


> > 
> > > 
> > > Without a higher-level view of what you're trying to achieve,
> > > it's
> > > not clear to me whether get_cycles() is the right interface.
> > I appreciate that.
> Until that's clarified, we won't make any progress here.

Let's see. We're synchronizing a set of distributed timers via a GPIO
pulse. Each processor on the system has at least one timer block
directly driven by the refclk@19.2MHz or a PLL driven by refclk@19.2MHz

FrameTime is a 64 bit free-running counter running @19.2MHz that is
used as a common source of reference for events. On the MSM side this
implies reading one of the timers driven by that refclk directly.

The choices on MSM8994 to access one of those timer blocks are

1. Read the register directly as is done in earlier patches or
2. Read get_cycles() as is done in later patches.

It's no more complex than that.

---
bod

[toc] | [prev] | [next] | [standalone]

#1484338

On Thu, Sep 15, 2016 at 04:40:00PM +0100, Bryan O'Donoghue wrote:
> On Thu, 2016-09-15 at 13:46 +0100, Mark Rutland wrote:
> > On Thu, Sep 15, 2016 at 12:48:08PM +0100, Bryan O'Donoghue wrote:
> > > On Thu, 2016-09-15 at 12:20 +0100, Mark Rutland wrote:
> > > > For example, you have absolutely no guarantee as to what backs
> > > > get_cycles(). Despite this, the code assumes that get_cycles() is
> > > > backed by something running at the frequency described in a
> > > > "google,greybus-frame-time-counter" node.
> > > > 
> > > > Even if this *happens* to match what some piece of arch code
> > > > provides
> > > > today on some platform, it is in no way *guaranteed*.
> > > That's the point though, if you declare "google,greybus-frame-time-
> > > counter" in your platform code - then you can use 'get_cycles()' in
> > > this manner
> > To be clear, *some* properties (and perhaps additional nodes) may
> > need
> > to be in the DT, in order to capture the hardware property or
> > relationship that you are reliant upon.
> 
> Sure but on the relevant platform we know 
> 
> 1. get_cycles() is derived from an architectured MMIO timer
> 2. What is clocking that timer
> 
> So any platform that declares that property must be aware of what its
> doing.

I can't say this any more explicitly:

****************************************************
* The DT *cannot* know anything about get_cycles() *
****************************************************

It's no more complex than that.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1484375

On Thu, 2016-09-15 at 16:47 +0100, Mark Rutland wrote:
> On 
> I can't say this any more explicitly:
> 
> ****************************************************
> * The DT *cannot* know anything about get_cycles() *
> ****************************************************
> 
> It's no more complex than that.

Sure think Mark I understand your point.

I think to satisfy your objection the most sensible change to make is
using arch_timer_get_rate() compared to a desired rate of 19200000.

---
bod

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web