From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Graf Date: Fri, 12 Aug 2016 20:38:58 +0200 Subject: [U-Boot] [PATCH v2] serial: bcm283x_mu: Detect disabled serial device In-Reply-To: References: <1470294682-159882-1-git-send-email-agraf@suse.de> <88a6384e-ddd1-6a5b-4e82-82c565b53036@wwwdotorg.org> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 12.08.16 19:21, Simon Glass wrote: > Hi Alex, > > On 11 August 2016 at 23:27, Alexander Graf wrote: >> >> >>> Am 12.08.2016 um 00:38 schrieb Simon Glass : >>> >>> Hi Alex, >>> >>>> On 11 August 2016 at 05:33, Alexander Graf wrote: >>>> >>>> >>>>> On 09.08.16 06:28, Stephen Warren wrote: >>>>>> On 08/04/2016 05:15 PM, Alexander Graf wrote: >>>>>> >>>>>>> On 04 Aug 2016, at 20:11, Stephen Warren wrote: >>>>>>> >>>>>>> On 08/04/2016 01:11 AM, Alexander Graf wrote: >>>>>>>> On the raspberry pi, you can disable the serial port to gain dynamic >>>>>>>> frequency >>>>>>>> scaling which can get handy at times. >>>>>>>> >>>>>>>> However, in such a configuration the serial controller gets its rx >>>>>>>> queue filled >>>>>>>> up with zero bytes which then happily get transmitted on to whoever >>>>>>>> calls >>>>>>>> getc() today. >>>>>>>> >>>>>>>> This patch adds detection logic for that case by checking whether >>>>>>>> the RX pin is >>>>>>>> mapped to GPIO15 and disables the mini uart if it is not mapped >>>>>>>> properly. >>>>>>>> >>>>>>>> That way we can leave the driver enabled in the tree and can >>>>>>>> determine during >>>>>>>> runtime whether serial is usable or not, having a single binary that >>>>>>>> allows for >>>>>>>> uart and non-uart operation. >>>>>>> >>>>>>>> diff --git a/drivers/serial/serial_bcm283x_mu.c >>>>>>>> b/drivers/serial/serial_bcm283x_mu.c >>>>>>> >>>>>>>> @@ -72,9 +87,18 @@ static int bcm283x_mu_serial_probe(struct udevice >>>>>>>> *dev) >>>>>>>> { >>>>>>>> struct bcm283x_mu_serial_platdata *plat = dev_get_platdata(dev); >>>>>>>> struct bcm283x_mu_priv *priv = dev_get_priv(dev); >>>>>>>> + struct bcm283x_gpio_regs *gpio = (struct bcm283x_gpio_regs >>>>>>>> *)plat->gpio; >>>>>>>> >>>>>>>> priv->regs = (struct bcm283x_mu_regs *)plat->base; >>>>>>>> >>>>>>>> + /* >>>>>>>> + * The RPi3 disables the mini uart by default. The easiest way >>>>>>>> to find >>>>>>>> + * out whether it is available is to check if the pin is muxed. >>>>>>>> + */ >>>>>>>> + if (((readl(&gpio->gpfsel1) >> BCM283X_GPIO_GPFSEL1_F15_SHIFT) & >>>>>>>> + BCM283X_GPIO_ALTFUNC_MASK) != BCM283X_GPIO_ALTFUNC_5) >>>>>>>> + priv->disabled = true; >>>>>>>> + >>>>>>>> return 0; >>>>>>> >>>>>>> Comment on the current implementation: Can't probe() return an error >>>>>>> if the device should be disabled? That would avoid the need to check >>>>>>> priv->disabled in all the other functions. >>>>>> >>>>>> I guess I should?ve put that in a comment somewhere. Unfortunately we >>>>>> can?t. If I just return an error on probe, U-Boot will panic because >>>>>> we tell it in a CONFIG define that we require a serial port (grep for >>>>>> CONFIG_REQUIRE_SERIAL_CONSOLE). >>>>>> >>>>>> We could maybe try to unset that define instead? >>>>> >>>>> Yes, assuming that U-Boot runs just fine with HDMI console only, I think >>>>> it's fine to unset CONFIG_REQUIRE_SERIAL_CONSOLE. >>>>> >>>>>>> Overall comment: I'd rather not put this logic into the UART driver >>>>>>> itself; it is system-specific rather than device-specific. I'd also >>>>>>> rather not have the UART driver touching GPIO registers; that's not >>>>>>> very modular, and could cause problems if the Pi is converted to use >>>>>>> DT to instantiate devices. >>>>>>> >>>>>>> Instead, can we put the logic into board/raspberrypi/rpi/rpi.c? I.e. >>>>>>> have some early function come along and enable/disable the >>>>>>> bcm2837_serials device object as appropriate? That way it isolates >>>>>>> the code to the Pi specifically, and not any other bcm283x board. >>>>>>> We'd want to wrap that code in #ifdef CONFIG_PL01X_SERIAL. >>>>>> >>>>>> We can do that if we can fail at probe time. If we absolutely must >>>>>> have a serial driver to work in the first place, that doesn?t work. I >>>>>> can try to poke at it, but it?ll be a few days I think :). >>>> >>>> So I couldn't find a sane way to fail probing based on something defined >>>> in the board file, reusing the existing gpio device. >>> >>> Would it be possible to move this code into the serial driver? >> >> You mean like in v2 which Stephen nacked? :) > > Yes :-( > > Well you can put what you like in the board code, and if this is only > on the rpi, then it makes sense. > > Really though, this is a pinctrl thing, so if there were a pinctrl > driver you could just use it. The GPIO driver should not deal with pin > muxing. It's the same IP block on the RPi :). > >> >>> >>>> >>>> However, there's an easy alternative. We can make the console code just >>>> ignore our serial device if we set its pointer to NULL. That way we >>>> still have the device, but can contain all logic to disable usage of the >>>> mini uart to the board file. >>> >>> I'm not very keen on that - feels like a hack. What is stopping >>> Stephen's idea from working? I could perhaps help with dm plumbing is >>> that is the issue... >> >> The problem is that we need the gpio device to determine whether the pin is muxed. There is no temporal control that I could see that would allow me to be in a place where the gpio device exists, the serial device does not exist, and where I could then not spawn the serial device based on board logic. > > Can you use board_early_init_f() ? How? I guess we would need to a) Create the GPIO device b) Ask the GPIO device whether the pin is muxed correctly c) Create serial device based on outcome of b Is that possible? Alex