[U-Boot] [PATCH v4] x86: baytrail: Configure FSP UPD from device tree

[Andrew Bradford]

Hi Simon,

On 08/11 21:54, Simon Glass wrote:
> +Gabriel
> 
> Hi Andrew,
> 
> On 11 August 2015 at 09:20, Andrew Bradford <andrew at bradfordembedded.com> wrote:
> > Hi Simon,
> >
> > On 08/11 08:06, Simon Glass wrote:
> >> Hi Andrew,
> >>
> >> On 11 August 2015 at 06:08, Andrew Bradford <andrew at bradfordembedded.com> wrote:
> >> > Hi Simon,
> >> >
> >> > On 08/10 21:07, Simon Glass wrote:
> >> >> Hi Bin,
> >> >>
> >> >> On 10 August 2015 at 20:53, Bin Meng <bmeng.cn at gmail.com> wrote:
> >> >> > Hi Andrew,
> >> >> >
> >> >> > On Mon, Aug 10, 2015 at 7:32 PM, Andrew Bradford
> >> >> > <andrew at bradfordembedded.com> wrote:
> >> >> >> Hi Bin,
> >> >> >>
> >> >> >> On 08/09 10:52, Bin Meng wrote:
> >> >> >>> Hi Andrew,
> >> >> >>>
> >> >> >>> On Sun, Aug 9, 2015 at 9:08 AM, Andrew Bradford
> >> >> >>> <andrew at bradfordembedded.com> wrote:
> >> >> >>> > Hi Simon,
> >> >> >>> >
> >> >> >>> > On 08/08 10:18, Simon Glass wrote:
> >> >> >>> >> Hi,
> >> >> >>> >>
> >> >> >>> >> On 7 August 2015 at 06:44, Bin Meng <bmeng.cn at gmail.com> wrote:
> >> >> >>> >> > On Fri, Aug 7, 2015 at 8:36 PM, Andrew Bradford
> >> >> >>> >> > <andrew at bradfordembedded.com> wrote:
> >> >> >>> >> >> From: Andrew Bradford <andrew.bradford at kodakalaris.com>
> >> >> >>> >> >>
> >> >> >>> >> >> Allow for configuration of FSP UPD from the device tree which will
> >> >> >>> >> >> override any settings which the FSP was built with itself.
> >> >> >>> >> >>
> >> >> >>> >> >> Modify the MinnowMax and BayleyBay boards to transfer sensible UPD
> >> >> >>> >> >> settings from the Intel FSPv4 Gold release to the respective dts files,
> >> >> >>> >> >> with the condition that the memory-down parameters for MinnowMax are
> >> >> >>> >> >> also used.
> >> >> >>> >> >>
> >> >> >>> >> >> Signed-off-by: Andrew Bradford <andrew.bradford at kodakalaris.com>
> >> >> >>> >> >> ---
> >> >> >>> >> >>
> >> >> >>> >> >
> >> >> >>> >> > Reviewed-by: Bin Meng <bmeng.cn at gmail.com>
> >> >> >>> >> > Tested-by: Bin Meng <bmeng.cn at gmail.com>
> >> >> >>> >> >
> >> >> >>> >>
> >> >> >>> >> Acked-by: Simon Glass <sjg at chromium.org>
> >> >> >>> >> Tested on minnowmax:
> >> >> >>> >> Tested-by: Simon Glass <sjg at chromium.org>
> >> >> >>> >>
> >> >> >>> >> I found that I need to remove two properties from the minnowmax.dts:
> >> >> >>> >>
> >> >> >>> >> - fsp,enable-xhci needs to be removed as this does not work in U-Boot
> >> >> >>> >> at present and stops EHCI from working
> >> >> >>> >> - fsp,mrc-debug-msg needs to be removed to prevent debug information
> >> >> >>> >> being displayed
> >> >> >>> >>
> >> >> >>> >> I plan to apply this with these changes - please let me know if this
> >> >> >>> >> doesn't suit.
> >> >> >>> >
> >> >> >>> > I'm OK with disabling xhci and the MRC debug output in the FSP.
> >> >> >>> >
> >> >> >>> > But if xhci is disabled then I believe when Linux boots that the USB 3.0
> >> >> >>> > port on Minnow Max will only act as a USB 2.0 port.  That u-boot doesn't
> >> >> >>> > yet have working XHCI on E3800 means there is a tradeoff and I wasn't
> >> >> >>> > sure which was a better choice.
> >> >> >>>
> >> >> >>> Does these xHCI ports on MinnowMax work fully on Linux kernel? If it
> >> >> >>> works, I'd rather we keep fsp,enable-xhci in the U-Boot.
> >> >> >>
> >> >> >> I believe that the xhci port does work on Minnow Max in Linux but I do
> >> >> >> not have a board so I'm unable to test, sorry.
> >> >> >>
> >> >> >
> >> >> > OK, my test shows that ehci works fine in U-Boot on Bayley Bay.
> >> >> >
> >> >> > Hi Simon,
> >> >> >
> >> >> > What do you think regarding to xhci vs. ehci in U-Boot?
> >> >>
> >> >> The problem is that USB is then broken in U-Boot. I think it is better
> >> >> to limit the speed for the moment until we have that fixed. It is
> >> >> quite useful to be able to use a keyboard or USB stick in U-Boot.
> >> >>
> >> >> With my testing the bottom (blue) port works fine but the top port
> >> >> does not. This happens regardless of the xhci setting.
> >> >
> >> > Minnowmax has a USB power switch enable which determines if the VBUS2
> >> > net is turned on or not, which will supply or not supply power to the
> >> > USB 2.0 port.  The blue USB 3.0 port also has an enable.  Both enables
> >> > and the USB ports themselves are located on page 16 of the schematic
> >> > that I have.
> >> >
> >> > The switches to enable the VBUS for each port are active high and pulled
> >> > down.  So to turn them on, I believe that's the point of the pinmuxing
> >> > and GPIO settings which are used in the minnowmax.dts.  Can you verify
> >> > if these are correctly turning on VBUS2 (since it sounds like they are
> >> > turning on VBUS1)?  If not, when you turn these GPIO on manually, does
> >> > the USB 2.0 port work correctly?
> >>
> >> I see power on the bottom port but not the top one.
> >
> > OK, that's odd, but likely can be fixed with changes to the pin mux and
> > pad settings in the dts.  I believe that the "pad-offset" for
> > pin_usb_host_en1 at 0 is wrong and that it should be 0x250 instead of
> > 0x258.
> >
> > diff --git a/arch/x86/dts/minnowmax.dts b/arch/x86/dts/minnowmax.dts
> > index d0c0fe6..4988163 100644
> > --- a/arch/x86/dts/minnowmax.dts
> > +++ b/arch/x86/dts/minnowmax.dts
> > @@ -39,7 +39,7 @@
> >
> >                 pin_usb_host_en1 at 0 {
> >                         gpio-offset = <0x80 9>;
> > -                       pad-offset = <0x258>;
> > +                       pad-offset = <0x250>;
> >                         mode-gpio;
> >                         output-value = <1>;
> >                         direction = <PIN_OUTPUT>;
> >
> > Does that fix it?
> 
> I dug into this a bit and it all seems quite broken:
> 
> - PCI bus configuration does not work in gpio_ich6_pinctrl_init(). I
> will send a patch for this
> - gpio_ich6_get_base() returns a 16-bit word but function is also used
> to read the memory address which holds a 32-bit word
> - Intel's hardware won't let you read the status of an output!

I thought that if you cleared bit 2 in the "pad value" register (iinenb)
for a GPIO that you could read back outputs.  Or am I misunderstanding?

> The last point means that _gpio_ich6_pinctrl_cfg_pin cannot work. We
> need to mirror the lvl register in order to be able to read it.
> 
> I confirmed that with the 'correct' value in the lvl registers both
> USB ports work.

OK, well that's good at least.

Thanks,
Andrew