Re: [PATCH v6 5/5] ARM: dts: sun9i: Initial support for the Sunchip CX-A99 board

[Maxime Ripard]

On Sun, Feb 19, 2017 at 09:12:32PM +0100, Rask Ingemann Lambertsen wrote:
> On Thu, Feb 16, 2017 at 07:29:57PM +0100, Maxime Ripard wrote:
> > On Thu, Feb 16, 2017 at 07:17:54AM +0100, Rask Ingemann Lambertsen wrote:
> > > > > Supported features (+ means tested):
> [...]
> > > > > + SATA port on on-board SATA-to-USB bridge *
> [...]
> > > > > * Only shows up when a SATA device is connected. Also, if a power source
> > > > >   is connected to the USB 3.0 connector across power cycles (e.g. FEL
> > > > >   boot), the bridge may not properly reset and not show up on the USB bus.
> > > > >   The vendor U-Boot performs some unknown magic which resets the bridge.
> > > > 
> > > > Is that magic at the USB level, or specific to the bridge itself?
> > > 
> > > I don't know, but since the other USB port connected to the hub comes up
> > > fine, I'm assuming it's something to do with the SATA-to-USB bridge.
> > 
> > But where is that magic written to then? the USB controller registers,
> > or is it an USB packet?
> 
> I don't know. Like I said, unknown magic. I haven't found the source code to
> the U-Boot version that the board uses.

Then how do you know there's a magic in the first place?

> For all I know, it could be a bug in the Linux USB stack. I've always had
> to unplug and replug my USB keyboards after Linux has booted, otherwise
> they are not detected. If the keyboard was connected to a hub, Linux doesn't
> detect that port on the hub, so the hub must be unplugged and replugged.
> If it's an internal hub, tough luck.

I have a laptop that certainly doesn't show that behaviour, so a bug
in the USB stack seems weird.

> > > > > +	reg_usb0_vbus: regulator-usb0-vbus {
> > > > > +		compatible = "regulator-fixed";
> > > > > +		regulator-name = "usb0-vbus";
> > > > > +		regulator-min-microvolt = <5000000>;
> > > > > +		regulator-max-microvolt = <5000000>;
> > > > > +		gpio = <&pio 7 15 GPIO_ACTIVE_HIGH>;	/* PH15 */
> > > > > +		enable-active-high;
> > > > 
> > > > This is redundant with the GPIO flag
> 
> The GPIO flag is overridden by the regulator core, which makes it misleading.
> I will remove the GPIO flag and add a comment there that GPIO flags are not
> supported. This is to prevent someone from thinking that
> 
> gpio = <... GPIO_ACTIVE_HIGH>;
> enable-active-high;
> 
> and
> 
> gpio = <... GPIO_ACTIVE_LOW>;
> 
> are equivalent. It would be an easy mistake to make, because in mmc nodes,
> 
> cd-gpios = <...  GPIO_ACTIVE_HIGH>;
> 
> and
> 
> cd-gpios = <...  GPIO_ACTIVE_LOW>;
> cd-inverted;
> 
> _are_ equivalent.

And your point is? The gpio flags are now properly passed through to
their respective consumers, which was not the case before. Everything
works, without the workaround that we had to use before. Why would you
want to use anything else than the property that just works without
any modification and with the minimal amount of "code" ?

> 
> > > No. I have now tested without "enable-active-high" and then Vbus stays off.
> > > This is also in line with the binding documentation:
> > > 
> > > "- enable-active-high: Polarity of GPIO is Active high
> > >  If this property is missing, the default assumed is Active low."
> > >
> > > It also seems to me that the way this is implemented in Linux, the GPIO
> > > polarity flag is ignored.
> > 
> > It works just fine if the driver uses the gpiod API. If that driver
> > doesn't, then that driver needs some fixing :)
> 
> Take it up with the maintainer of the regulator core, then. :)

Or you can also submit a patch fixing it.

> From of_get_fixed_voltage_config() in drivers/regulator/fixed.c:
> [...]
> 	config->gpio = of_get_named_gpio(np, "gpio", 0);
> [...]
> 	config->enable_high = of_property_read_bool(np, "enable-active-high");
> 	config->gpio_is_open_drain = of_property_read_bool(np,
> 							   "gpio-open-drain");
> [...]
> 
> From reg_fixed_voltage_probe() in drivers/regulator/fixed.c:
> [...]
> 		config = of_get_fixed_voltage_config(&pdev->dev,
> 						     &drvdata->desc);
> [...]
> 	if (gpio_is_valid(config->gpio)) {
> 		cfg.ena_gpio = config->gpio;
> 		if (pdev->dev.of_node)
> 			cfg.ena_gpio_initialized = true;
> 	}
> 	cfg.ena_gpio_invert = !config->enable_high;
> 	if (config->enabled_at_boot) {
> 		if (config->enable_high)
> 			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
> 		else
> 			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
> 	} else {
> 		if (config->enable_high)
> 			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
> 		else
> 			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
> 	}
> 	if (config->gpio_is_open_drain)
> 		cfg.ena_gpio_flags |= GPIOF_OPEN_DRAIN;
> [...]
> 
> From regulator_ena_gpio_request() in drivers/regulator/core.c:
> [...]
> 	gpiod = gpio_to_desc(config->ena_gpio);
> [...]
> 	ret = gpio_request_one(config->ena_gpio,
> 				GPIOF_DIR_OUT | config->ena_gpio_flags,
> 				rdev_get_name(rdev));
> [...]
> 	pin->gpiod = gpiod;
> 	pin->ena_gpio_invert = config->ena_gpio_invert;
> 
> And finally, gpio_request_one() in drivers/gpio/gpiolib-legacy.c:
> 
> int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
> {
> 	struct gpio_desc *desc;
> 	int err;
> 
> 	desc = gpio_to_desc(gpio);
> [...]
> 	if (flags & GPIOF_OPEN_DRAIN)
> 		set_bit(FLAG_OPEN_DRAIN, &desc->flags);
> 
> 	if (flags & GPIOF_OPEN_SOURCE)
> 		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> 
> 	if (flags & GPIOF_ACTIVE_LOW)
> 		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> 
> 	if (flags & GPIOF_DIR_IN)
> 		err = gpiod_direction_input(desc);
> 	else
> 		err = gpiod_direction_output_raw(desc,
> 				(flags & GPIOF_INIT_HIGH) ? 1 : 0);
> [...]
> 
> > > > > +		regulator-always-on;
> > > > 
> > > > And it shouldn't be always on. The USB driver will enable it if needs
> > > > be.
> > > 
> > > Yes, we just don't have a driver for the A80 USB 3.0 port yet.
> > 
> > Aaah, so that's what you meant. Leave it out of the DT then.
> 
> OK.
> 
> > > > You're listing a minimum state of 750mv, yet your minimum voltage is
> > > > 800mV.
> > > 
> > > Yes. The regulator is capable of outputting the listed voltages in the
> > > 750 mV to 1200 mV range, but the permissible range of the consumer is only
> > > 800 mV to 1100 mV according to the A80 data sheet. Likewise, the AXP806
> > > regulators support a larger voltage range than that of the connected
> > > consumers.
> > 
> > Adding a comment stating that would be nice then.
> 
> OK.
> 
> > > > > +	pmic@745 {
> > > > > +		compatible = "x-powers,axp808", "x-powers,axp806";
> > > > > +		reg = <0x745>;
> > > > > +		interrupt-parent = <&nmi_intc>;
> > > > > +		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > > > > +		interrupt-controller;
> > > > > +		#interrupt-cells = <1>;
> > > > > +
> > > > > +		swin-supply = <&reg_dcdce>;
> > > > 
> > > > Please use an incude for that PMIC.
> > > 
> > > I don't understand. What would that include file contain? Very few lines
> > > would be shared between .dts files. You could save < 8 lines total if you
> > > #include the 5 lines that are common between the Cubieboard4, the Optimus
> > > and this board.
> > 
> > You also have to take into account that the PMIC is barely supported
> > at the moment, you'll also need to define the GPIO controllers, the
> > various power supplies, adc, etc. nodes that are going (hopefully to
> > be added) in the future.
> > 
> > If the include is shared, once it is enabled, every one benefits from
> > it. Otherwise, you have to duplicate that change across all DTs. This
> > is much likely to error, harder to maintain, and you end up in a
> > situation where none of the boards really support the same feature set
> > while being based on the same SoC / PMIC combination.
> 
> Fair enough, I'll add an include file the next time around. Like you
> mentioned youself, though, we will only really know how compatible the two
> PMICs are when we have the code to support the other features (or a data
> sheet/user's manual is found). I'm hopeful based on what has been found so
> far.

Adding a new compatible early on is cheap. Dealing with a device that
was supposed to be fully compatible but turned out not to be is really
painful, so yeah, it's better to just add a new compatible from the
very beginning, even if we'll always behave in the exact same way.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attachment: signature.asc
Description: PGP signature