On Thu, Aug 25, 2016 at 3:44 AM, Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx> wrote: > On Mon, Aug 22, 2016 at 11:29:59PM +0200, Rask Ingemann Lambertsen wrote: >> On Mon, Aug 22, 2016 at 08:57:45PM +0200, Maxime Ripard wrote: >> > Hi, >> > >> > On Sat, Aug 13, 2016 at 12:03:57AM +0200, Rask Ingemann Lambertsen wrote: >> > > The Suncip CX-A99 board is found in at least four brands of TV boxes. >> > > It features an Allwinner A80 SOC, with either 2 GiB DDR3 DRAM and >> > > 16 GB eMMC or 4 GiB DDR3 DRAM and 32 GB eMMC, as well as several support >> > > chips for Ethernet, wireless networking, video output, SATA and power >> > > management. For details, see the linux-sunxi page about the board >> > > <URL:https://linux-sunxi.org/Sunchip_CX-A99>. >> > > >> > > This patch only adds support for the SD and eMMC storage, real-time clock, >> > > USB 2.0 ports (and by extension the SATA port), the UART port and the LEDs. >> > > All of this relies on the boot loader to leave those parts powered up, as >> > > I'm still working on a driver for the AXP808 PMIC. >> > > >> > > Signed-off-by: Rask Ingemann Lambertsen <ccc94453@xxxxxxxxxxxxxxxx> >> > >> > It looks mostly good, but I have a couple of comments though, see below. >> > >> > > --- >> > > >> > > Although the vendor U-Boot lets you boot the kernel on one of the >> > > Cortex-A15 cores, the kernel gpio-regulator driver currently glitches >> > > the GPIO lines to the OZ80120 regulator such that the system crashes >> > > during startup. This part needs further work to be useful. >> > >> > So it doesn't power the CPU through one of the AXP regulators? >> > Interesting design. >> >> The Cortex-A7 cores are powered by the dcdca regulator on the AXP 808 PMIC. >> Right now, I'm using the AXP 806 driver that Chen-Yu Tsai posted to drive >> the AXP 808 and so far, it looks good. > > Ok. Could you look around for AXP808 datasheets? > >> > > + leds { >> > > + compatible = "gpio-leds"; >> > > + pinctrl-names = "default"; >> > > + pinctrl-0 = <&led_pins_cx_a99>; >> > > + >> > > + blue { >> > > + gpios = <&pio 6 10 GPIO_ACTIVE_HIGH>; /* PG10 */ >> > > + label = "cx-a99:blue:normal"; >> > > + default-state = "on"; >> > > + }; >> > > + >> > > + red { >> > > + gpios = <&pio 6 11 GPIO_ACTIVE_HIGH>; /* PG11 */ >> > > + label = "cx-a99:red:standby"; >> > >> > The last part of the label should be the function. >> >> I'm not sure what else to label them. They form a bi-colour LED emitting >> through the front of the device. The stock OS lights the blue LED when >> the device is powered on and lights the red LED when the device is >> suspended. There is no label on the front of the device telling what the >> colours mean. Documentation/devicetree/bindings/leds/common.txt and >> Documentation/devicetree/bindings/leds/leds-gpio.txt don't provide much in >> the way of examples. Suggestions are welcome. > > Hmmmm. status for both then? > >> [snip] >> > > + >> > > +/* Each GPIO controls VBUS for a port on the GL850G hub connected to ehci0; >> > > + * PL7 for port 1, the USB connector closest to the 12 V power connector, and >> > > + * PL8 for port 2, the USB connector next to the (micro)SD card slot. >> > > + * Note: The regulators are not chained like this in reality, but >> > > + * regulator-fixed doesn't support a gpio list, and allwinner,sun9i-a80-usb-phy >> > > + * doesn't support more than one supply, so this will have to do (for now). >> > > + */ >> > > +®_usb1_vbus { >> > > + pinctrl-0 = <&usb1_vbus_r_pin_cx_a99>; >> > > + gpio = <&r_pio 0 7 GPIO_ACTIVE_HIGH>; /* PL7 */ >> > > + status = "okay"; >> > > +}; >> > > + >> > > +®_usb2_vbus { >> > > + pinctrl-0 = <&usb2_vbus_r_pin_cx_a99>; >> > > + gpio = <&r_pio 0 8 GPIO_ACTIVE_HIGH>; /* PL8 */ >> > >> > I'd really prefer it to be modelled properly, and not attached to the >> > wrong device. >> > >> > I have some work pending to allow multiple regulators in the same >> > property, but that won't be ready soon. >> > >> > For the time being, I would suggest having two usb1 regulators >> > defined, each with their respective GPIOs, and one set to always on >> > (and keep that great comment). >> >> Will do if I don't come up with something better. I gave it a shot to >> describe the hub as a child of ehci0 with a child node for each of the >> two ports, but it didn't work. >> >> Also, using the phy-supply property for the vbus-supply is an ugly hack, >> in the first place, isn't it? Shouldn't it be more like this? Is it. Back then when I added USB support I didn't notice the PHYs had separate power supplies, and 2 even. This will be reworked in the future. >> >> &usbphy1 { >> phy-supply = <®_vcc33_usbh>; >> }; >> >> &ehci0 { >> vcc-supply = <®_vdd09_usbh>; >> phy = <&usbphy1>; >> >> hub@1 { >> port@1 { >> vbus-supply = <®_usb1_vbus>; >> } >> >> port@2 { >> vbus-supply = <®_usb2_vbus>; >> } >> }; >> }; > > It looks great to me. I'm not really sure how happy the DT maintainers > are going to be about it, and how easy it would be to support without > breaking the existing users. The hub and device power sequencing stuff is being worked on, though I've lost track of the progress. I don't think regulators were included in the initial proposal though. ChenYu >> It would generally be great to be able to describe regulators that should >> be kept in sync, because the Wifi+BT module's Vbat pin is supplied by two >> regulators in parallel. IIRC, Hans de Goede ran into that as well on one >> of his boards. > > Yes, we got the same issue on the CHIP. I'm the one with the hot > potato, but that would require a significant rework of the regulator > framework, that I haven't had the time to do yet. > > Thanks! > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
