Re: [u-boot][PATCH 10/14] mtd: rawnand: omap_gpmc: support u-boot driver model

[Roger Quadros <rogerq@kernel.org>]

On 13/10/2022 14:17, Adam Ford wrote:
> On Wed, Oct 12, 2022 at 6:42 AM Adam Ford <aford173@gmail.com> wrote:
>>
>> On Wed, Oct 12, 2022 at 1:22 AM Roger Quadros <rogerq@kernel.org> wrote:
>>>
>>> Hi Adam,
>>>
>>> On 11/10/2022 18:01, Adam Ford wrote:
>>>> On Tue, Oct 11, 2022 at 6:52 AM Roger Quadros <rogerq@kernel.org> wrote:
>>>>>
>>>>> Adds driver model support.
>>>>>
>>>>> We need to be able to self initialize the NAND controller/chip
>>>>> at probe and so enable CONFIG_SYS_NAND_SELF_INIT.
>>>>>
>>>>> Doing so requires nand_register() API which is provided by nand.c
>>>>> and needs to be enabled during SPL build via CONFIG_SPL_NAND_INIT.
>>>>> But nand.c also provides nand_init() so we need to get rid of nand_init()
>>>>> in omap_gpmc driver if CONFIG_SPL_NAND_INIT is set.
>>>>>
>>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>>>>> ---
>>>>>  drivers/mtd/nand/raw/Kconfig     |  1 +
>>>>>  drivers/mtd/nand/raw/omap_gpmc.c | 55 +++++++++++++++++++++++++++++++-
>>>>>  2 files changed, 55 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
>>>>> index bc5cabdfc2..1d23144ce4 100644
>>>>> --- a/drivers/mtd/nand/raw/Kconfig
>>>>> +++ b/drivers/mtd/nand/raw/Kconfig
>>>>> @@ -190,6 +190,7 @@ config NAND_LPC32XX_SLC
>>>>>  config NAND_OMAP_GPMC
>>>>>         bool "Support OMAP GPMC NAND controller"
>>>>>         depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || ARCH_K3
>>>>> +       select SYS_NAND_SELF_INIT if ARCH_K3
>>>>
>>>> I have a question about this down below.
>>>>
>>>>>         help
>>>>>           Enables omap_gpmc.c driver for OMAPx and AMxxxx platforms.
>>>>>           GPMC controller is used for parallel NAND flash devices, and can
>>>>> diff --git a/drivers/mtd/nand/raw/omap_gpmc.c b/drivers/mtd/nand/raw/omap_gpmc.c
>>>>> index e772a914c8..7192ca9e5a 100644
>>>>> --- a/drivers/mtd/nand/raw/omap_gpmc.c
>>>>> +++ b/drivers/mtd/nand/raw/omap_gpmc.c
>>>>> @@ -7,6 +7,7 @@
>>>>>  #include <common.h>
>>>>>  #include <log.h>
>>>>>  #include <asm/io.h>
>>>>> +#include <dm/uclass.h>
>>>>>  #include <linux/errno.h>
>>>>>
>>>>>  #ifdef CONFIG_ARCH_OMAP2PLUS
>>>>> @@ -1121,7 +1122,7 @@ int __maybe_unused omap_nand_switch_ecc(uint32_t hardware, uint32_t eccstrength)
>>>>>   *   nand_scan about special functionality. See the defines for further
>>>>>   *   explanation
>>>>>   */
>>>>> -int board_nand_init(struct nand_chip *nand)
>>>>> +int gpmc_nand_init(struct nand_chip *nand)
>>>>>  {
>>>>>         int32_t gpmc_config = 0;
>>>>>         int cs = cs_next++;
>>>>> @@ -1201,3 +1202,55 @@ int board_nand_init(struct nand_chip *nand)
>>>>>
>>>>>         return 0;
>>>>>  }
>>>>> +
>>>>> +static struct nand_chip *nand_chip;    /* First NAND chip for SPL use only */
>>>>> +
>>>>> +#if CONFIG_IS_ENABLED(SYS_NAND_SELF_INIT)
>>>>> +
>>>>> +static int gpmc_nand_probe(struct udevice *dev)
>>>>> +{
>>>>> +       struct nand_chip *nand = dev_get_priv(dev);
>>>>> +       struct mtd_info *mtd = nand_to_mtd(nand);
>>>>> +       int ret;
>>>>> +
>>>>> +       gpmc_nand_init(nand);
>>>>> +
>>>>> +       ret = nand_scan(mtd, CONFIG_SYS_NAND_MAX_CHIPS);
>>>>> +       if (ret)
>>>>> +               return ret;
>>>>> +
>>>>> +       ret = nand_register(0, mtd);
>>>>> +       if (ret)
>>>>> +               return ret;
>>>>> +
>>>>> +       if (!nand_chip)
>>>>> +               nand_chip = nand;
>>>>> +
>>>>> +       return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct udevice_id gpmc_nand_ids[] = {
>>>>> +       { .compatible = "ti,am64-nand" },
>>>>> +       { .compatible = "ti,omap2-nand" },
>>>>
>>>> The gpmc_nand_ids reference to omap2, but it's encapsulated inside the
>>>> SYS_NAND_SELF_INIT ifdef which appears to only be set if K3.  Should
>>>> this code be expected to work on OMAP2?  I don't think K3 is set for
>>>> OMAP2+.  If so, should the SYS_NAND_SELF_INIT be selected if OMAP2 is
>>>> selected?
>>>
>>> We want to eventually get this working using driver model and SYS_NAND_SELF_INIT
>>> for OMAP2 as well but just that I didn't work on it yet or test it.
>>>
>>> One challenge is that OMAP2 boards tend to either select nand_spl_simple.c
>>> or am335x_spl_bch.c for NAND support at SPL.
>>>
>>> We will need to figure out if it is possible to use CONFIG_SPL_NAND_INIT
>>> and this driver instead.
>>> One issue might be that everything doesn't fit in resources available at SPL?
>>
>> On my board the GPMC runs more than just NAND.  I am hoping to get the
>> GPMC driver working in U-Boot first then in SPL (assuming it fits).  I
>> have LTO enabled on my DM3730, so I am hoping it might help make some
>> room.  I am hoping that once the NAND is working that the GPMC driver
>> can be used in U-Boot to handle the configuration of the bus for
>> handling Ethernet, so some of the quirks and manual board file
>> configuration can be removed.
>>>
>>>>
>>>> I have a DM3730 that I can test with this.  Do you have a repo I can
>>>
>>> That would be great. Thanks!
> 
> I haven't spend a lot of time, but here is what I have so far:
> 
> U-Boot 2022.10-rc4-gf499f45d18-dirty (Oct 13 2022 - 05:33:33 -0500)
> 
> OMAP3630/3730-GP ES1.2, CPU-OPP2, L3-200MHz, Max CPU Clock 1 GHz
> Model: LogicPD Zoom DM3730 Torpedo + Wireless Development Kit
> DRAM:  256 MiB
> Error binding driver 'gpmc-nand': -96
> Some drivers failed to bind
> Error binding driver 'ti-gpmc': -96
> Some drivers failed to bind
> Error binding driver 'simple_bus': -96
> Some drivers failed to bind
> initcall sequence 8ffde84c failed at call 8011705d (err=-96)
> ### ERROR ### Please RESET the board ###
> 
> There was a small conflict with arch/arm/mach-omap2/mem-common.c
>  where I needed to remove a reference to gpmc_cfg to let it compile.
> 
> +#ifndef CONFIG_TI_GPMC
>  const struct gpmc *gpmc_cfg = (struct gpmc *)GPMC_BASE;
> +#endif

OK. But eventually we don't want to include mem-common.c
when TI_GPMC driver is enabled.

> 
> After that, I had to enable CONFIG_CLK and CONFIG_CLK_TI_GATE, but I
> am not sure it's sufficient.  I haven't had enough time to check to
> see if the OMAP3 clock drivers are fully supported in U-Boot or not.

Tom could answer this question.

> I am guessing they'll need to be functional enough to fetch the fck so
> the GPMC controller knows how fast it's running to properly calculate
> the timings.

Even if fck is not available we could assume a default value for now so
driver can proceed.

> 
> Do you have any suggestions on what I should do to diagnose the -96
> errors on all the GPMC sub-nodes?

-96 is EPFNOSUPPORT and it seems to be coming from uclass_add()

Did you enable CONFIG_DM_MEMORY and CONFIG_DM_MTD?

If that doesn't work then try to add some prints in device_bind_common() to see where it fails.

Please see this patch to see what all I had to enable to get NAND working on AM64
https://github.com/rogerq/u-boot/commit/f831875b6551588f7c83314d5770e2b73b18bc37

The following 2 patches might also be helpful to get NAND to work
https://github.com/rogerq/u-boot/commit/2c64a620e2a65eef3bbe3aa1e74488daea9e019d

https://github.com/rogerq/u-boot/commit/50d7ed49bc4737cccecc5ce2e3f2c3b17d540a5f

> 
> adam
>>>
>>>> point to to test?  If not, I'll pull the series from patchwork, but I
>>>> need to know what branch to use as a starting point.
>>>
>>> You can use this Repo as reference.
>>> https://github.com/rogerq/u-boot/commits/for-v2023.01/am64-nand-base-1.0-test
>>>
>>
>> Thanks!
>>
>>> It has a few patches on top consisting of device tree and u-boot configuration
>>> for AM64 platform. You can ignore the last 2 patches as they are only for a
>>> workaround on early AM64 boards.
>>>
>>> If you hit any hurdles, we can discuss how to resolve.
>>
>> I'll just pull in the branch and build for my DM3730 then start
>> enabling and disabling stuff.  I haven't checked how big SPL is, but
>> SPL keeps growing instead of shrinking.  OF_PLATDATA might be an
>> option if it doesn't fit in SPL, but I was hoping to avoid that.
>>
>> adam
>>>
>>>>
>>>> thanks,
>>>>
>>>> adam
>>>>
>>>>> +       { }
>>>>> +};
>>>>> +
>>>>> +U_BOOT_DRIVER(gpmc_nand) = {
>>>>> +       .name           = "gpmc-nand",
>>>>> +       .id             = UCLASS_MTD,
>>>>> +       .of_match       = gpmc_nand_ids,
>>>>> +       .probe          = gpmc_nand_probe,
>>>>> +       .priv_auto      = sizeof(struct nand_chip),
>>>>> +};
>>>>> +
>>>>> +void board_nand_init(void)
>>>>> +{
>>>>> +       struct udevice *dev;
>>>>> +       int ret;
>>>>> +
>>>>> +       ret = uclass_get_device_by_driver(UCLASS_MTD,
>>>>> +                                         DM_DRIVER_GET(gpmc_nand), &dev);
>>>>> +       if (ret && ret != -ENODEV)
>>>>> +               pr_err("%s: Failed to get GPMC device: %d\n", __func__, ret);
>>>>> +}
>>>>> +#endif /* CONFIG_SYS_NAND_SELF_INIT */
>>>>> --
>>>>> 2.17.1
>>>>>
>>>
>>> cheers,
>>> -roger

cheers,
-roger