Re: [PATCH v3 4/8] memory: Add STM32 Octo Memory Manager driver

[Patrice CHOTARD <patrice.chotard@xxxxxxxxxxx>]

On 2/11/25 19:16, Christophe JAILLET wrote:
> Le 10/02/2025 à 14:18, patrice.chotard-rj0Iel/JR4NBDgjK7y7TUQ@xxxxxxxxxxxxxxxx a écrit :
>> From: Patrice Chotard <patrice.chotard-rj0Iel/JR4NBDgjK7y7TUQ@xxxxxxxxxxxxxxxx>
>>
>> Octo Memory Manager driver (OMM) manages:
>>    - the muxing between 2 OSPI busses and 2 output ports.
>>      There are 4 possible muxing configurations:
>>        - direct mode (no multiplexing): OSPI1 output is on port 1 and OSPI2
>>          output is on port 2
>>        - OSPI1 and OSPI2 are multiplexed over the same output port 1
>>        - swapped mode (no multiplexing), OSPI1 output is on port 2,
>>          OSPI2 output is on port 1
>>        - OSPI1 and OSPI2 are multiplexed over the same output port 2
>>    - the split of the memory area shared between the 2 OSPI instances.
>>    - chip select selection override.
>>    - the time between 2 transactions in multiplexed mode.
>>    - check firewall access.
> 
> ...
> 
>> diff --git a/drivers/memory/stm32_omm.c b/drivers/memory/stm32_omm.c
>> new file mode 100644
>> index 000000000000..af69137bfba2
>> --- /dev/null
>> +++ b/drivers/memory/stm32_omm.c
>> @@ -0,0 +1,520 @@
>> +// SPDX-License-Identifier: GPL
> 
> Not sure this SPDX-License-Identifier exists.

Right, i will fix that.

> 
>> +/*
>> + * Copyright (C) STMicroelectronics 2024 - All Rights Reserved
> 
> ...
> 
>> +    pm_runtime_enable(dev);
>> +
>> +    /* check if OMM's resource access is granted */
>> +    ret = stm32_omm_check_access(dev, dev->of_node);
>> +    if (ret < 0 && ret != -EACCES)
>> +        goto err_clk_release;
> 
> Should we call, here and below, pm_runtime_disable() in the error handling path, as done in the remove function?

right, i will add it.

> 
>> +
>> +    if (!ret && child_access_granted == OMM_CHILD_NB) {
>> +        /* Ensure both OSPI instance are disabled before configuring OMM */
>> +        ret = stm32_omm_disable_child(dev);
>> +        if (ret)
>> +            goto err_clk_release;
>> +
>> +        ret = stm32_omm_configure(dev);
>> +        if (ret)
>> +            goto err_clk_release;
>> +    } else {
>> +        dev_dbg(dev, "Octo Memory Manager resource's access not granted\n");
>> +        /*
>> +         * AMCR can't be set, so check if current value is coherent
>> +         * with memory-map areas defined in DT
>> +         */
>> +        ret = stm32_omm_set_amcr(dev, false);
>> +        if (ret)
>> +            goto err_clk_release;
>> +    }
>> +
>> +    /* for each child, if resource access is granted and status "okay", probe it */
>> +    for (i = 0; i < omm->nb_child; i++) {
>> +        if (!child_access[i] || !of_device_is_available(omm->child[i].node))
>> +            continue;
>> +
>> +        vdev = of_platform_device_create(omm->child[i].node, NULL, NULL);
>> +        if (!vdev) {
>> +            dev_err(dev, "Failed to create Octo Memory Manager child\n");
>> +            for (j = i; j > 0; --j) {
>> +                if (omm->child[j].dev)
>> +                    of_platform_device_destroy(omm->child[j].dev, NULL);
>> +            }
>> +
>> +            ret = -EINVAL;
>> +            goto err_clk_release;
>> +        }
>> +        omm->child[i].dev = &vdev->dev;
>> +    }
>> +
>> +err_clk_release:
>> +    for (i = 0; i < omm->nb_child; i++)
>> +        clk_put(omm->child[i].clk);
>> +
>> +    return ret;
>> +}
>> +
>> +static void stm32_omm_remove(struct platform_device *pdev)
>> +{
>> +    struct stm32_omm *omm = platform_get_drvdata(pdev);
>> +    int i;
>> +
>> +    for (i = 0; i < omm->nb_child; i++)
>> +        if (omm->child[i].dev)
>> +            of_platform_device_destroy(omm->child[i].dev, NULL);
>> +
>> +    if (omm->cr & CR_MUXEN)
>> +        stm32_omm_enable_child_clock(&pdev->dev, false);
>> +
>> +    pm_runtime_disable(&pdev->dev);
> 
> Should we have:
>     for (i = 0; i < omm->nb_child; i++)
>         clk_put(omm->child[i].clk);
> as done in the error handling path of the probe?

no need, as child's clock are always freed in stm32_omm_probe() in all cases.

> 
>> +}
>> +
>> +static const struct of_device_id stm32_omm_of_match[] = {
>> +    { .compatible = "st,stm32mp25-omm", },
>> +    {},
> 
> Nitpick: Unneeded , after a terminator.

ok

> 
>> +};
>> +MODULE_DEVICE_TABLE(of, stm32_omm_of_match);
> 
> ...
> 
> CJ
> 
> 
>