Message-ID: <a4efb3d4-f734-4900-9a16-a809908b67ce@foss.st.com>
Date: Wed, 23 Apr 2025 08:07:41 +0200
From: Patrice CHOTARD <patrice.chotard@...s.st.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
CC: <alexandre.torgue@...s.st.com>, <catalin.marinas@....com>,
        <christophe.kerello@...s.st.com>, <conor+dt@...nel.org>,
        <devicetree@...r.kernel.org>, <krzk+dt@...nel.org>, <krzk@...nel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <mcoquelin.stm32@...il.com>, <p.zabel@...gutronix.de>,
        <robh@...nel.org>, <will@...nel.org>
Subject: Re: [PATCH v10 2/3] memory: Add STM32 Octo Memory Manager driver



On 4/22/25 19:55, Christophe JAILLET wrote:
> Le 22/04/2025 à 10:44, Patrice Chotard a écrit :
>> 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.
> 
> Hi,
> 
> 2 small questions below.
> 
>> +static int stm32_omm_disable_child(struct device *dev)
>> +{
>> +    struct stm32_omm *omm = dev_get_drvdata(dev);
>> +    struct reset_control *reset;
>> +    int ret;
>> +    u8 i;
>> +
>> +    ret = stm32_omm_toggle_child_clock(dev, true);
>> +    if (!ret)
>> +        return ret;
>> +
>> +    for (i = 0; i < omm->nb_child; i++) {
>> +        /* reset OSPI to ensure CR_EN bit is set to 0 */
>> +        reset = omm->child_reset[i];
>> +        ret = reset_control_acquire(reset);
>> +        if (ret) {
>> +            dev_err(dev, "Can not acquire resset %d\n", ret);
> 
> Should stm32_omm_toggle_child_clock(dev, false);
> be called here?

Hi Christophe

Right, stm32_omm_toggle_child_clock(dev, false) can be called here too.

> 
>> +            return ret;
>> +        }
>> +
>> +        reset_control_assert(reset);
>> +        udelay(2);
>> +        reset_control_deassert(reset);
>> +
>> +        reset_control_release(reset);
>> +    }
>> +
>> +    return stm32_omm_toggle_child_clock(dev, false);
>> +}
> 
> ...
> 
>> +static int stm32_omm_probe(struct platform_device *pdev)
>> +{
>> +    static const char * const resets_name[] = {"ospi1", "ospi2"};
>> +    struct device *dev = &pdev->dev;
>> +    u8 child_access_granted = 0;
>> +    struct stm32_omm *omm;
>> +    int i, ret;
>> +
>> +    omm = devm_kzalloc(dev, sizeof(*omm), GFP_KERNEL);
>> +    if (!omm)
>> +        return -ENOMEM;
>> +
>> +    omm->io_base = devm_platform_ioremap_resource_byname(pdev, "regs");
>> +    if (IS_ERR(omm->io_base))
>> +        return PTR_ERR(omm->io_base);
>> +
>> +    omm->mm_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory_map");
>> +    if (IS_ERR(omm->mm_res))
>> +        return PTR_ERR(omm->mm_res);
>> +
>> +    /* check child's access */
>> +    for_each_child_of_node_scoped(dev->of_node, child) {
>> +        if (omm->nb_child >= OMM_CHILD_NB) {
>> +            dev_err(dev, "Bad DT, found too much children\n");
>> +            return -E2BIG;
>> +        }
>> +
>> +        ret = stm32_omm_check_access(child);
>> +        if (ret < 0 && ret != -EACCES)
>> +            return ret;
>> +
>> +        if (!ret)
>> +            child_access_granted++;
>> +
>> +        omm->nb_child++;
>> +    }
>> +
>> +    if (omm->nb_child != OMM_CHILD_NB)
>> +        return -EINVAL;
>> +
>> +    platform_set_drvdata(pdev, omm);
>> +
>> +    devm_pm_runtime_enable(dev);
>> +
>> +    /* check if OMM's resource access is granted */
>> +    ret = stm32_omm_check_access(dev->of_node);
>> +    if (ret < 0 && ret != -EACCES)
>> +        return ret;
>> +
>> +    for (i = 0; i < omm->nb_child; i++) {
>> +        omm->child_reset[i] = devm_reset_control_get_exclusive_released(dev,
>> +                                        resets_name[i]);
>> +
>> +        if (IS_ERR(omm->child_reset[i]))
>> +            return dev_err_probe(dev, PTR_ERR(omm->child_reset[i]),
>> +                         "Can't get %s reset\n", resets_name[i]);
>> +    }
>> +
>> +    if (!ret && child_access_granted == OMM_CHILD_NB) {
>> +        ret = stm32_omm_configure(dev);
>> +        if (ret)
>> +            return ret;
>> +    } 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)
>> +            return ret;
>> +    }
>> +
>> +    ret = devm_of_platform_populate(dev);
>> +    if (ret)
>> +        dev_err(dev, "Failed to create Octo Memory Manager child\n");
> 
> Should something like the code in the remove function be called here?

Good catch, yes if MUXEN bit is set, that means child's clock are still enable,
in this case we must disable these child clocks as well.

> 
> Also, maybe return dev_err_probe() and return 0; below?

Yes , dev_err_probe() can be added.

Thanks
Patrice

> 
>> +
>> +    return ret;
>> +}
> 
> ...
> 
> CJ
> 

Powered by blists - more mailing lists