From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= Subject: Re: [PATCH V2 0/1] usb: add HCD providers Date: Fri, 15 Jul 2016 11:35:41 +0200 Message-ID: References: <1468326921-26485-1-git-send-email-zajec5@gmail.com> <1468413734-9569-1-git-send-email-zajec5@gmail.com> <87lh15isi7.fsf@linux.intel.com> <87inw9ir4k.fsf@linux.intel.com> <20160714094836.GB28730@shlinux2> <20160715022824.GA817@shlinux2> <20160715062209.GC817@shlinux2> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mail-oi0-f68.google.com ([209.85.218.68]:34583 "EHLO mail-oi0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751009AbcGOJfn convert rfc822-to-8bit (ORCPT ); Fri, 15 Jul 2016 05:35:43 -0400 In-Reply-To: <20160715062209.GC817@shlinux2> Sender: linux-leds-owner@vger.kernel.org List-Id: linux-leds@vger.kernel.org To: Peter Chen Cc: Felipe Balbi , Greg Kroah-Hartman , "linux-usb@vger.kernel.org" , "open list:LED SUBSYSTEM" , "devicetree@vger.kernel.org" On 15 July 2016 at 08:22, Peter Chen wrote: > On Fri, Jul 15, 2016 at 07:48:11AM +0200, Rafa=C5=82 Mi=C5=82ecki wro= te: >> >> > Below I supply another thought, please check if it is feasible. >> >> > In below design, you don't need to change any usb codes. >> >> > >> >> > dts: >> >> > >> >> > led_1 { >> >> > led_gpio_1; >> >> > usb_port =3D &ohci_port0, &ehci_port1; >> >> > } >> >> > >> >> > led_2 { >> >> > led_gpio_2; >> >> > usb_port =3D &xhci_port0, &xhci_port1; >> >> > } >> >> > >> >> > ohci@1000 { >> >> > compatible =3D "generic-ohci"; >> >> > reg =3D <0x00001000 0x1000>; >> >> > interrupts =3D ; >> >> > >> >> > ohci_port0: port@0 { >> >> > reg =3D <0>; >> >> > }; >> >> > >> >> > ohci_port1: port@1 { >> >> > reg =3D <1>; >> >> > }; >> >> > }; >> >> > >> >> > ehci@2000 { >> >> > compatible =3D "generic-ehci"; >> >> > reg =3D <0x00002000 0x1000>; >> >> > interrupts =3D ; >> >> > >> >> > ehci_port0: port@0 { >> >> > reg =3D <0>; >> >> > }; >> >> > >> >> > ehci_port1: port@1 { >> >> > reg =3D <1>; >> >> > }; >> >> > }; >> >> > >> >> > xhci@3000 { >> >> > compatible =3D "generic-xhci"; >> >> > reg =3D <0x00003000 0x1000>; >> >> > interrupts =3D ; >> >> > >> >> > /* for xhci, port 0 - [N-1] is USB3, N - [M-1] is USB2/= 1. >> >> > * The port 0 and port N is the same physical port >> >> > */ >> >> > xhci_port0: port@0 { >> >> > reg =3D <0>; >> >> > }; >> >> > >> >> > xhci_port1: port@1 { >> >> > reg =3D <1>; >> >> > }; >> >> > >> >> > }; >> >> > >> >> > At code, compare the usb_device's device_node at usbport_trig_n= otify >> >> > if it is at led_1's usb device list, light on it. >> >> >> >> This is quite interesting idea, thanks! >> >> >> >> So I got following checking code: >> >> >> >> count =3D of_count_phandle_with_args(np, "usb-ports", NULL); >> >> for (i =3D 0; i < count; i++) { >> >> of_parse_phandle_with_args(np, "usb-ports", NULL, i, &args); >> >> of_property_read_u32(args.np, "reg", &port); >> >> if (args.np->parent =3D=3D usb_dev->bus->controller->of_node = && >> >> port =3D=3D usb_dev->portnum) { >> >> of_node_put(args.np); >> >> return true; >> >> } >> >> of_node_put(args.np); >> >> } >> >> return false; >> > >> > No, compares the USB port directly. >> > >> > count =3D of_count_phandle_with_args(np, "usb-ports", NULL); >> > for (i =3D 0; i < count; i++) { >> > of_parse_phandle_with_args(np, "usb-ports", NULL, i, &args); >> > if (args.np =3D=3D usb_dev->dev.of_node) >> > of_node_put(args.np); >> > return true; >> > } >> > of_node_put(args.np); >> > } >> > return false; >> >> If we mean to use usb_dev->dev.of_node I *need* to modify USB >> subsystem, since this pointer is never being set by the current code= =2E >> >> [ 71.410505] usb 1-1: new high-speed USB device number 2 using ehc= i-platform >> [ 71.564928] [usbport_trig_notify] usb_dev:c6ac1000 &usb_dev->dev:= c6ac1068 >> [ 71.579874] [usbport_trig_notify] dev_name(&usb_dev->dev):1-1 >> [ 71.586580] [usbport_trig_notify] usb_dev->dev.of_node: (null) >> >> Or am I missing something? >> > > You may need below patches: > > commit 69bec725985324e79b1c47ea287815ac4ddb0521 > Author: Peter Chen > Date: Fri Feb 19 17:26:15 2016 +0800 > > USB: core: let USB device know device node > > commit 7222c832254a75dcd67d683df75753d4a4e125bb > Author: Nicolai Stange > Date: Thu Mar 17 23:53:02 2016 +0100 > > usb/core: usb_alloc_dev(): fix setting of ->portnum =46*k, I just implemented the same thing on my own and I was going to submit it :/ Thanks for pointing these commits. >> >> This works, but I see 3 more problems: >> >> >> >> 1) How to access list of available USB devices during activation? >> > >> > You mean during LED activation? eg your usbport_trig_activate? >> > Why do you need it? >> >> Yes, I mean usbport_trig_activate. If user plugs in USB device and >> *then* activates this trigger, we want to set a proper initial state= =2E >> We can't only depend on USB_DEVICE_ADD. >> > > Oh, I see, I asked it before. > > Either you need to register USB notifier before activation It won't work if someone builds usbport as a module and loads it after connecting USB devices. > Or you need to implement something like usb_node_to_dev > eg: like usb_for_each_dev. If device's state is USB_STATE_CONFIGURED > this USB device is available. I think I'll need that. >> >> 2) What about support for non-DT platforms in usbport driver? Sho= uld I >> >> still allow specifying ports manually? Are you OK with that? >> > >> > I am afraid I still don't know how to do it for non-DT platforms. >> > You can show your design. >> >> Please take a look at >> [PATCH] leds: trigger: Introduce an USB port trigger >> https://lkml.org/lkml/2016/7/11/305 >> >> Basically my idea was to support: >> echo usbport > trigger >> echo 4-1 > new_port >> echo 2-1 > new_port >> > > I know your patch, how you plan to support non-DT platforms before? I didn't have any better idea than just letting user space do some magic. I don't think we can develop any clever solution for non-DT but I still want to give these platforms some options. I think I can live with some more tricky code in user space. >> >> 3) What about devices with internal hubs? Should we describe thei= r USB >> >> ports in DT as well? Any idea how to do this? >> > >> > Well, the HUB must be hard-wired on board for this LED trigger cas= e. >> > So, you can described USB topology well at dts. Please note: the >> > USB port phandle at LED node is the physical connector on board wh= ich >> > the user can plug in USB device, it may be 2nd or more levels from= the >> > controller. >> > >> > Below is the example for how to describe 3 levels USB devices, the >> > USB ethernet port (axis) is one of the ports at internal HUB (gene= sys). >> > >> > http://www.spinics.net/lists/linux-usb/msg143698.html >> >> Thanks for this example. I'm only afraid there isn't a way to say wh= at >> port "ethernet: asix@1" is attached to. > > Why? You know your hardware design, you should know the topology, eg > This physical port is from ehci port, and which port number for its > internal hub port number. If not, how we describe USB devices on > dts. Please ignore that question. --=20 Rafa=C5=82