[U-Boot] [PATCH] imx: Extend PCL063 support for phyCORE-i.MX6ULL SOM

[Martyn Welch <martyn.welch@collabora.com>]

On Wed, 2019-04-10 at 15:23 +0200, Parthiban Nallathambi wrote:
> Hello Wadim,
> 
> Thanks for sharing the details.
> 
> On 4/10/19 10:35 AM, Wadim Egorov wrote:
> > Martyn,
> > 
> > On 09.04.19 12:46, Martyn Welch wrote:
> > > On Tue, 2019-04-09 at 11:30 +0200, Parthiban Nallathambi wrote:
> > > > Hello Martyn,
> > > > 
> > > > On 4/9/19 10:49 AM, Martyn Welch wrote:
> > > > > On Mon, 2019-04-08 at 20:04 +0200, Parthiban wrote:
> > > > > > Hello Martyn,
> > > > > > 
> > > > > > On 4/8/19 7:45 PM, Martyn Welch wrote:
> > > > > > > On Sun, 2019-04-07 at 19:56 +0200, Parthiban Nallathambi
> > > > > > > wrote:
> > > > > > > > diff --git a/board/phytec/pcl063/spl.c
> > > > > > > > b/board/phytec/pcl063/spl.c
> > > > > > > > index b93cd493f2..73a774645d 100644
> > > > > > > > --- a/board/phytec/pcl063/spl.c
> > > > > > > > +++ b/board/phytec/pcl063/spl.c
> > > > > > > > @@ -13,6 +13,7 @@
> > > > > > > >    #include <asm/arch/mx6-ddr.h>
> > > > > > > >    #include <asm/arch/mx6-pins.h>
> > > > > > > >    #include <asm/arch/crm_regs.h>
> > > > > > > > +#include <asm/arch/sys_proto.h>
> > > > > > > >    #include <fsl_esdhc.h>
> > > > > > > >    
> > > > > > > >    /* Configuration for Micron MT41K256M16TW-107 IT:P,
> > > > > > > > 32M x
> > > > > > > > 16 x 8
> > > > > > > > ->
> > > > > > > > 256MiB */
> > > > > > > > @@ -117,11 +118,32 @@ static iomux_v3_cfg_t const
> > > > > > > > usdhc1_pads[] =
> > > > > > > > {
> > > > > > > >    	MX6_PAD_UART1_RTS_B__USDHC1_CD_B |
> > > > > > > > MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > > > > > > >    };
> > > > > > > >    
> > > > > > > > +#ifndef CONFIG_NAND_MXS
> > > > > > > > +static iomux_v3_cfg_t const usdhc2_pads[] = {
> > > > > > > > +	MX6_PAD_NAND_RE_B__USDHC2_CLK    |
> > > > > > > > MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > > > > > > > +	MX6_PAD_NAND_WE_B__USDHC2_CMD    |
> > > > > > > > MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > > > > > > > +	MX6_PAD_NAND_DATA00__USDHC2_DATA0 |
> > > > > > > > MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > > > > > > > +	MX6_PAD_NAND_DATA01__USDHC2_DATA1 |
> > > > > > > > MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > > > > > > > +	MX6_PAD_NAND_DATA02__USDHC2_DATA2 |
> > > > > > > > MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > > > > > > > +	MX6_PAD_NAND_DATA03__USDHC2_DATA3 |
> > > > > > > > MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > > > > > > > +	MX6_PAD_NAND_DATA04__USDHC2_DATA4 |
> > > > > > > > MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > > > > > > > +	MX6_PAD_NAND_DATA05__USDHC2_DATA5 |
> > > > > > > > MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > > > > > > > +	MX6_PAD_NAND_DATA06__USDHC2_DATA6 |
> > > > > > > > MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > > > > > > > +	MX6_PAD_NAND_DATA07__USDHC2_DATA7 |
> > > > > > > > MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > > > > > > > +};
> > > > > > > > +#endif
> > > > > > > > +
> > > > > > > Umm, these pins are already used a few lines up for the
> > > > > > > NAND,
> > > > > > > via
> > > > > > > gpmi:
> > > > > > I understand. But pcl063 can't co-exit with NAND and eMMC
> > > > > > together. I
> > > > > > comes
> > > > > > either with eMMC or NAND.
> > > > > Opps, sorry, just realised that I added this comment in the
> > > > > wrong
> > > > > place. This is in relation to the following being added to
> > > > > pcl063-common.dtsi:
> > > > > 
> > > > > +
> > > > > +       pinctrl_usdhc2: usdhc2grp {
> > > > > +               fsl,pins = <
> > > > > +                       MX6UL_PAD_NAND_WE_B__USDHC2_CMD      
> > > > >    0x1
> > > > > 70f9
> > > > > +                       MX6UL_PAD_NAND_RE_B__USDHC2_CLK      
> > > > >    0x1
> > > > > 00f9
> > > > > +                       MX6UL_PAD_NAND_DATA00__USDHC2_DATA0  
> > > > >    0x1
> > > > > 70f9
> > > > > +                       MX6UL_PAD_NAND_DATA01__USDHC2_DATA1  
> > > > >    0x1
> > > > > 70f9
> > > > > +                       MX6UL_PAD_NAND_DATA02__USDHC2_DATA2  
> > > > >    0x1
> > > > > 70f9
> > > > > +                       MX6UL_PAD_NAND_DATA03__USDHC2_DATA3  
> > > > >    0x1
> > > > > 70f9
> > > > > +                       MX6UL_PAD_NAND_DATA04__USDHC2_DATA4  
> > > > >    0x1
> > > > > 70f9
> > > > > +                       MX6UL_PAD_NAND_DATA05__USDHC2_DATA5  
> > > > >    0x1
> > > > > 70f9
> > > > > +                       MX6UL_PAD_NAND_DATA06__USDHC2_DATA6  
> > > > >    0x1
> > > > > 70f9
> > > > > +                       MX6UL_PAD_NAND_DATA07__USDHC2_DATA7  
> > > > >    0x1
> > > > > 70f9
> > > > > +               >;
> > > > > +       };
> > > > > 
> > > > > If there exists pcl063 modules that have eMMC and others that
> > > > > have
> > > > > NAND
> > > > > using the same pins, then this configuration is not common
> > > > > and
> > > > > therefore shouldn't be in pcl063-common.dtsi. Is it dependent
> > > > > on
> > > > > the
> > > > > flavour of i.MX used? If so I'd suggest the gpmi config needs
> > > > > to be
> > > > > pulled out into imx6ul-phycore-segin.dts and the usdhc2
> > > > > config
> > > > > needs to
> > > > > be in imx6ull-phycore-segin.dts.
> > > >   From phytec I understand that pcl063 SoM is a common platform
> > > > for
> > > > imx6UL
> > > > and imx6ULL. This can either be shipped with eMMC or NAND, but
> > > > not
> > > > both.
> > 
> > This is correct. There are PCL-063 SOMs with eMMC or NAND. And each
> > PCL-063 can be a 6UL or 6ULL.
> > 
> > 
> > > Looking a bit deeper, this seems a little odd as the product
> > > description suggests that NAND is provided onboard and 2
> > > SD/SDIO/MMC
> > > connections are provided to the edge connector of the pcl063 for
> > > expansion.
> > > 
> > > The schematic suggests the only way they could achieve eMMC
> > > onboard
> > > would be with an eMMC that is pin compatible with the NAND they
> > > use.
> > 
> > eMMC is connected via usdhc2. The usdhc2 pins conflict with the
> > gpmi pins.
> > 
> > 
> > > Additionally, looking at the DTBs for this board in Phytec's own
> > > kernel
> > > tree, the only use of usdhc2 that I can see is for their WLAN
> > > expansion
> > > board[1] and I would have expected their tree to have supported
> > > such an
> > > option if it was available (they seem to have gone to some length
> > > to
> > > support a lot of configurations there).
> > > 
> > > Are you sure that the eMMC is provided on the pcl063 and not off
> > > board?
> > 
> > eMMC is on the PCL-063 and not on a carrier board.
> > 
> > 
> > > (CCing Wadim who might be able to shed some light on this)
> > > 
> > > > So there exist a possibility that phytec can provide imx6UL
> > > > with eMMC
> > > > as
> > > > well. IMO, both pinmux detailing for NAND and eMMC should still
> > > > reside
> > > > in common.dtsi.
> > > > 
> > > Assuming Phytec do in fact sell a pcl0063 with eMMC on board, the
> > > DTB
> > > describes the hardware. You've said Phytec provide the board
> > > either with eMMC *or* NAND. The device tree, as used on a
> > > specific
> > > board, should show either the existence of NAND or eMMC.
> > 
> > AFAIK, the idea was to put the muxing for both flash devices in the
> > pcl063-som.dtsi and keep them disabled. eMMC or NAND will be then
> > enabled in a higher level carrier board dts file. The handling of
> > the
> > SOM variant is designed by the carrier board dts file name, e.g.
> > imx6ul-phytec-segin-ff-rdk-nand.dts and in theory a
> > imx6ul-phytec-segin-ff-rdk-emmc.dts (Right now there is no 6UL eMMC
> > SOM
> > variant, so you can not find it in our kernel repository).
> 
> I agree to place both gpmi and eMMC pinmux details in common.dtsi
> with
> default "disabled" and enable them only in board dts.
> 
> @Martyn, please advise.
> 

Yes, that seems sensible.

Martyn

> Thanks,
> Parthiban N
> 
> > I agree, that this does not describe the SOM hardware. This is just
> > the
> > way we handle the SOM variants.
> > 
> > It seems the upstream situation for kernel and u-boot differs to
> > what we
> > did quite a lot at the moment. I am just curious how you want to
> > handle
> > this now. Unfortunately, we did no had the time to bring our 6UL
> > boards
> > upstream and clarify the situation. But we can talk about it now
> > and
> > find out a way to handle the variants properly.
> > 
> > CC'ed Stefan who is maintaining the imx6 boards at Phytec.
> > 
> > I hope this helps :)
> > 
> > Regards,
> > Wadim
> > 
> > > I suspect having both options in the common file is going to lead
> > > to
> > > issues with the pinmuxing for one or the other option. The pins
> > > can't
> > > be muxed for both simultaneously, hence these need to be
> > > described in
> > > separate files.
> > > 
> > > > Creating multiple common.dtsi based on these variants is not
> > > > friendly.
> > > > So I suggest to keep these changes in common.dtsi as such and
> > > > decide
> > > > in
> > > > board dts whether to enable or disable usdhc1, usdhc2
> > > > explicitly.
> > > This isn't about the selection of usdhc1 or usdhc2, it's whether
> > > gpmi
> > > or usdhc2 is using the pins of the SOC.
> > > 
> > > Martyn
> > > 
> > > [1]
> > > https://git.phytec.de/linux-mainline/tree/arch/arm/boot/dts/imx6ul-phytec-peb-wlbt-01.dtsi?h=v4.14.39-phy#n51
> > > 
> > > 
> > > > > > >           pinctrl_gpmi_nand: gpminandgrp {
> > > > > > >                   fsl,pins = <
> > > > > > >                           MX6UL_PAD_NAND_CLE__RAWNAND_CLE
> > > > > > >    0x0
> > > > > > > b0b1
> > > > > > >                           MX6UL_PAD_NAND_ALE__RAWNAND_ALE
> > > > > > >    0x0
> > > > > > > b0b1
> > > > > > >                           MX6UL_PAD_NAND_WP_B__RAWNAND_WP
> > > > > > > _B
> > > > > > >    0x0
> > > > > > > b0b1
> > > > > > >                           MX6UL_PAD_NAND_READY_B__RAWNAND
> > > > > > > _READY_
> > > > > > > B
> > > > > > > 0x0b000
> > > > > > >                           MX6UL_PAD_NAND_CE0_B__RAWNAND_C
> > > > > > > E0_B
> > > > > > >    0x0
> > > > > > > b0b1
> > > > > > >                           MX6UL_PAD_NAND_RE_B__RAWNAND_RE
> > > > > > > _B
> > > > > > >    0x0
> > > > > > > b0b1
> > > > > > >                           MX6UL_PAD_NAND_WE_B__RAWNAND_WE
> > > > > > > _B
> > > > > > >    0x0
> > > > > > > b0b1
> > > > > > >                           MX6UL_PAD_NAND_DATA00__RAWNAND_
> > > > > > > DATA00
> > > > > > >    0x0
> > > > > > > b0b1
> > > > > > >                           MX6UL_PAD_NAND_DATA01__RAWNAND_
> > > > > > > DATA01
> > > > > > >    0x0
> > > > > > > b0b1
> > > > > > >                           MX6UL_PAD_NAND_DATA02__RAWNAND_
> > > > > > > DATA02
> > > > > > >    0x0
> > > > > > > b0b1
> > > > > > >                           MX6UL_PAD_NAND_DATA03__RAWNAND_
> > > > > > > DATA03
> > > > > > >    0x0
> > > > > > > b0b1
> > > > > > >                           MX6UL_PAD_NAND_DATA04__RAWNAND_
> > > > > > > DATA04
> > > > > > >    0x0
> > > > > > > b0b1
> > > > > > >                           MX6UL_PAD_NAND_DATA05__RAWNAND_
> > > > > > > DATA05
> > > > > > >    0x0
> > > > > > > b0b1
> > > > > > >                           MX6UL_PAD_NAND_DATA06__RAWNAND_
> > > > > > > DATA06
> > > > > > >    0x0
> > > > > > > b0b1
> > > > > > >                           MX6UL_PAD_NAND_DATA07__RAWNAND_
> > > > > > > DATA07
> > > > > > >    0x0
> > > > > > > b0b1
> > > > > > >                   >;
> > > > > > >           };
> > > > > > >