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

Groups > linux.kernel > #1444167 > unrolled thread

Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support

Started byFelipe Balbi <felipe.balbi@linux.intel.com>
First post2016-07-15 12:40 +0200
Last post2016-07-19 10:40 +0200
Articles 7 — 3 participants

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: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-07-15 12:40 +0200
    Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-07-15 13:20 +0200
      Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-07-15 13:30 +0200
        Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support Bin Gao <bin.gao@linux.intel.com> - 2016-07-16 00:40 +0200
          Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-07-16 02:00 +0200
            Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support Bin Gao <bin.gao@linux.intel.com> - 2016-07-19 07:30 +0200
              Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-07-19 10:40 +0200

#1444167 — Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support

FromFelipe Balbi <felipe.balbi@linux.intel.com>
Date2016-07-15 12:40 +0200
SubjectRe: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
Message-ID<rV8Hg-5Li-39@gated-at.bofh.it>

[Multipart message — attachments visible in raw view] — view raw

Hi,

Bin Gao <bin.gao@linux.intel.com> writes:
> +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> +{
> +	pr_info("sink port %d: %s message %s %s\n", port,
> +				is_cmsg ? "Control" : "Data",
> +				msg_to_string(is_cmsg, msg),
> +		 recv ? "received" : "sent(wait GOODCRC)");
> +}

this is problematic. By default, we're all using 115200 8N1 baud
rate. This message alone prints anywhere from 50 to 100 characters (I
didn't really count properly, these are rough numbers), and that takes:

n50chars_time = 50 / (115200 / 10) = 4.3ms
n100chars_time = 100 / (115200 / 10) = 8.6ms

Considering you have 30ms to reply with Power Request after GoodCRC, and
considering you're printing several of these messages, they become
really expensive and eat up valuable time from tSenderReply.

This should really be a pr_debug() or, better yet, a tracepoint.

-- 
balbi

[toc] | [next] | [standalone]

#1444196

FromGreg Kroah-Hartman <gregkh@linuxfoundation.org>
Date2016-07-15 13:20 +0200
Message-ID<rV9jX-6dN-7@gated-at.bofh.it>
In reply to#1444167
On Fri, Jul 15, 2016 at 01:38:12PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Bin Gao <bin.gao@linux.intel.com> writes:
> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> > +{
> > +	pr_info("sink port %d: %s message %s %s\n", port,
> > +				is_cmsg ? "Control" : "Data",
> > +				msg_to_string(is_cmsg, msg),
> > +		 recv ? "received" : "sent(wait GOODCRC)");
> > +}
> 
> this is problematic. By default, we're all using 115200 8N1 baud
> rate. This message alone prints anywhere from 50 to 100 characters (I
> didn't really count properly, these are rough numbers), and that takes:
> 
> n50chars_time = 50 / (115200 / 10) = 4.3ms
> n100chars_time = 100 / (115200 / 10) = 8.6ms
> 
> Considering you have 30ms to reply with Power Request after GoodCRC, and
> considering you're printing several of these messages, they become
> really expensive and eat up valuable time from tSenderReply.

printk() should be async, so it shouldn't be that big of a deal.

What is wrong is that this isn't using dev_info().

> This should really be a pr_debug() or, better yet, a tracepoint.

Yes, that would be best (dev_dbg() or a tracepoint.)

thanks,

greg k-h

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

#1444202

FromFelipe Balbi <felipe.balbi@linux.intel.com>
Date2016-07-15 13:30 +0200
Message-ID<rV9tE-6hc-35@gated-at.bofh.it>
In reply to#1444196

[Multipart message — attachments visible in raw view] — view raw

Hi,

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> On Fri, Jul 15, 2016 at 01:38:12PM +0300, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Bin Gao <bin.gao@linux.intel.com> writes:
>> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
>> > +{
>> > +	pr_info("sink port %d: %s message %s %s\n", port,
>> > +				is_cmsg ? "Control" : "Data",
>> > +				msg_to_string(is_cmsg, msg),
>> > +		 recv ? "received" : "sent(wait GOODCRC)");
>> > +}
>> 
>> this is problematic. By default, we're all using 115200 8N1 baud
>> rate. This message alone prints anywhere from 50 to 100 characters (I
>> didn't really count properly, these are rough numbers), and that takes:
>> 
>> n50chars_time = 50 / (115200 / 10) = 4.3ms
>> n100chars_time = 100 / (115200 / 10) = 8.6ms
>> 
>> Considering you have 30ms to reply with Power Request after GoodCRC, and
>> considering you're printing several of these messages, they become
>> really expensive and eat up valuable time from tSenderReply.
>
> printk() should be async, so it shouldn't be that big of a deal.

I can actually see this causing problems ;-) With this pr_info(),
sometimes tSenderReply times out and Source gives a HardReset. Without
pr_info(), type-c analyzer tells me we reply in less than 1ms.

> What is wrong is that this isn't using dev_info().

right, that too.

-- 
balbi

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

#1444655

FromBin Gao <bin.gao@linux.intel.com>
Date2016-07-16 00:40 +0200
Message-ID<rVjW2-4an-11@gated-at.bofh.it>
In reply to#1444202
On Fri, Jul 15, 2016 at 02:21:48PM +0300, Felipe Balbi wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> > On Fri, Jul 15, 2016 at 01:38:12PM +0300, Felipe Balbi wrote:
> >> 
> >> Hi,
> >> 
> >> Bin Gao <bin.gao@linux.intel.com> writes:
> >> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> >> > +{
> >> > +	pr_info("sink port %d: %s message %s %s\n", port,
> >> > +				is_cmsg ? "Control" : "Data",
> >> > +				msg_to_string(is_cmsg, msg),
> >> > +		 recv ? "received" : "sent(wait GOODCRC)");
> >> > +}
> >> 
> >> this is problematic. By default, we're all using 115200 8N1 baud
> >> rate. This message alone prints anywhere from 50 to 100 characters (I
> >> didn't really count properly, these are rough numbers), and that takes:
> >> 
> >> n50chars_time = 50 / (115200 / 10) = 4.3ms
> >> n100chars_time = 100 / (115200 / 10) = 8.6ms
> >> 
> >> Considering you have 30ms to reply with Power Request after GoodCRC, and
> >> considering you're printing several of these messages, they become
> >> really expensive and eat up valuable time from tSenderReply.
> >
> > printk() should be async, so it shouldn't be that big of a deal.
> 
> I can actually see this causing problems ;-) With this pr_info(),
> sometimes tSenderReply times out and Source gives a HardReset. Without
> pr_info(), type-c analyzer tells me we reply in less than 1ms.
> 
> > What is wrong is that this isn't using dev_info().
> 
> right, that too.
> 
> -- 
> balbi

When we don't have a struct device pointer for this driver,
a dev_info(NULL, fmt, ...) is equivalent to pr_info(). So we have to
use dev_info() here?
But I agree at least it should be pr_debug().

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

#1444686

FromGreg Kroah-Hartman <gregkh@linuxfoundation.org>
Date2016-07-16 02:00 +0200
Message-ID<rVlbs-4PI-9@gated-at.bofh.it>
In reply to#1444655
On Fri, Jul 15, 2016 at 03:41:10PM -0700, Bin Gao wrote:
> On Fri, Jul 15, 2016 at 02:21:48PM +0300, Felipe Balbi wrote:
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> > > On Fri, Jul 15, 2016 at 01:38:12PM +0300, Felipe Balbi wrote:
> > >> 
> > >> Hi,
> > >> 
> > >> Bin Gao <bin.gao@linux.intel.com> writes:
> > >> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> > >> > +{
> > >> > +	pr_info("sink port %d: %s message %s %s\n", port,
> > >> > +				is_cmsg ? "Control" : "Data",
> > >> > +				msg_to_string(is_cmsg, msg),
> > >> > +		 recv ? "received" : "sent(wait GOODCRC)");
> > >> > +}
> > >> 
> > >> this is problematic. By default, we're all using 115200 8N1 baud
> > >> rate. This message alone prints anywhere from 50 to 100 characters (I
> > >> didn't really count properly, these are rough numbers), and that takes:
> > >> 
> > >> n50chars_time = 50 / (115200 / 10) = 4.3ms
> > >> n100chars_time = 100 / (115200 / 10) = 8.6ms
> > >> 
> > >> Considering you have 30ms to reply with Power Request after GoodCRC, and
> > >> considering you're printing several of these messages, they become
> > >> really expensive and eat up valuable time from tSenderReply.
> > >
> > > printk() should be async, so it shouldn't be that big of a deal.
> > 
> > I can actually see this causing problems ;-) With this pr_info(),
> > sometimes tSenderReply times out and Source gives a HardReset. Without
> > pr_info(), type-c analyzer tells me we reply in less than 1ms.
> > 
> > > What is wrong is that this isn't using dev_info().
> > 
> > right, that too.
> > 
> > -- 
> > balbi
> 
> When we don't have a struct device pointer for this driver,

Then you should fix that, as this is a driver for hardware :)

thanks,

greg k-h

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

#1446027

FromBin Gao <bin.gao@linux.intel.com>
Date2016-07-19 07:30 +0200
Message-ID<rWvLr-8iu-3@gated-at.bofh.it>
In reply to#1444686
On Sat, Jul 16, 2016 at 08:49:53AM +0900, Greg Kroah-Hartman wrote:
> On Fri, Jul 15, 2016 at 03:41:10PM -0700, Bin Gao wrote:
> > On Fri, Jul 15, 2016 at 02:21:48PM +0300, Felipe Balbi wrote:
> > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> > > > On Fri, Jul 15, 2016 at 01:38:12PM +0300, Felipe Balbi wrote:
> > > >> 
> > > >> Hi,
> > > >> 
> > > >> Bin Gao <bin.gao@linux.intel.com> writes:
> > > >> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> > > >> > +{
> > > >> > +	pr_info("sink port %d: %s message %s %s\n", port,
> > > >> > +				is_cmsg ? "Control" : "Data",
> > > >> > +				msg_to_string(is_cmsg, msg),
> > > >> > +		 recv ? "received" : "sent(wait GOODCRC)");
> > > >> > +}
> > > >> 
> > > >> this is problematic. By default, we're all using 115200 8N1 baud
> > > >> rate. This message alone prints anywhere from 50 to 100 characters (I
> > > >> didn't really count properly, these are rough numbers), and that takes:
> > > >> 
> > > >> n50chars_time = 50 / (115200 / 10) = 4.3ms
> > > >> n100chars_time = 100 / (115200 / 10) = 8.6ms
> > > >> 
> > > >> Considering you have 30ms to reply with Power Request after GoodCRC, and
> > > >> considering you're printing several of these messages, they become
> > > >> really expensive and eat up valuable time from tSenderReply.
> > > >
> > > > printk() should be async, so it shouldn't be that big of a deal.
> > > 
> > > I can actually see this causing problems ;-) With this pr_info(),
> > > sometimes tSenderReply times out and Source gives a HardReset. Without
> > > pr_info(), type-c analyzer tells me we reply in less than 1ms.
> > > 
> > > > What is wrong is that this isn't using dev_info().
> > > 
> > > right, that too.
> > > 
> > > -- 
> > > balbi
> > 
> > When we don't have a struct device pointer for this driver,
> 
> Then you should fix that, as this is a driver for hardware :)
This is actualy a software stack to implement the USB PD spec.
Only the USB Type-C phy driver has a device pointer.
The PD stack vs. USB Type-C phy driver is similar to TCP/IP stack
vs. ethernet driver in the kernel. We don't have a device pointer
for TCP/IP stack code either.

Thanks,
Bin

> 
> thanks,
> 
> greg k-h

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

#1446239

FromFelipe Balbi <felipe.balbi@linux.intel.com>
Date2016-07-19 10:40 +0200
Message-ID<rWyJk-1Ja-13@gated-at.bofh.it>
In reply to#1446027

[Multipart message — attachments visible in raw view] — view raw

Hi,

Bin Gao <bin.gao@linux.intel.com> writes:
> On Sat, Jul 16, 2016 at 08:49:53AM +0900, Greg Kroah-Hartman wrote:
>> On Fri, Jul 15, 2016 at 03:41:10PM -0700, Bin Gao wrote:
>> > On Fri, Jul 15, 2016 at 02:21:48PM +0300, Felipe Balbi wrote:
>> > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> > > > On Fri, Jul 15, 2016 at 01:38:12PM +0300, Felipe Balbi wrote:
>> > > >> 
>> > > >> Hi,
>> > > >> 
>> > > >> Bin Gao <bin.gao@linux.intel.com> writes:
>> > > >> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
>> > > >> > +{
>> > > >> > +	pr_info("sink port %d: %s message %s %s\n", port,
>> > > >> > +				is_cmsg ? "Control" : "Data",
>> > > >> > +				msg_to_string(is_cmsg, msg),
>> > > >> > +		 recv ? "received" : "sent(wait GOODCRC)");
>> > > >> > +}
>> > > >> 
>> > > >> this is problematic. By default, we're all using 115200 8N1 baud
>> > > >> rate. This message alone prints anywhere from 50 to 100 characters (I
>> > > >> didn't really count properly, these are rough numbers), and that takes:
>> > > >> 
>> > > >> n50chars_time = 50 / (115200 / 10) = 4.3ms
>> > > >> n100chars_time = 100 / (115200 / 10) = 8.6ms
>> > > >> 
>> > > >> Considering you have 30ms to reply with Power Request after GoodCRC, and
>> > > >> considering you're printing several of these messages, they become
>> > > >> really expensive and eat up valuable time from tSenderReply.
>> > > >
>> > > > printk() should be async, so it shouldn't be that big of a deal.
>> > > 
>> > > I can actually see this causing problems ;-) With this pr_info(),
>> > > sometimes tSenderReply times out and Source gives a HardReset. Without
>> > > pr_info(), type-c analyzer tells me we reply in less than 1ms.
>> > > 
>> > > > What is wrong is that this isn't using dev_info().
>> > > 
>> > > right, that too.
>> > > 
>> > > -- 
>> > > balbi
>> > 
>> > When we don't have a struct device pointer for this driver,
>> 
>> Then you should fix that, as this is a driver for hardware :)
> This is actualy a software stack to implement the USB PD spec.
> Only the USB Type-C phy driver has a device pointer.

what Greg is saying is that you should register yourself to the PD stack
with something that passes along a pointer to the actual device.

> The PD stack vs. USB Type-C phy driver is similar to TCP/IP stack
> vs. ethernet driver in the kernel. We don't have a device pointer
> for TCP/IP stack code either.

This is the wrong analogy.

-- 
balbi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web