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

[Patrice CHOTARD <patrice.chotard@xxxxxxxxxxx>]

On 3/19/25 08:37, Krzysztof Kozlowski wrote:
> On 18/03/2025 14:40, Patrice CHOTARD wrote:
>>
>>
>> On 3/13/25 08:33, Krzysztof Kozlowski wrote:
>>> On 12/03/2025 15:23, Patrice CHOTARD wrote:
>>>>>> +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;
>>>>>> +
>>>>>> +	for (i = 0; i < omm->nb_child; i++) {
>>>>>> +		ret = clk_prepare_enable(omm->child[i].clk);
>>>>>> +		if (ret) {
>>>>>> +			dev_err(dev, "Can not enable clock\n");
>>>>>> +			return ret;
>>>>>> +		}
>>>>>> +
>>>>>> +		reset = of_reset_control_get_exclusive(omm->child[i].node, 0);
>>>>>> +		if (IS_ERR(reset)) {
>>>>>> +			dev_err(dev, "Can't get child reset\n");
>>>>>
>>>>> Why do you get reset of child? Parent is not suppposed to poke there.
>>>>> You might not have the reset there in the first place and it would not
>>>>> be an error.
>>>>
>>>> By ressetting child (OSPI), we ensure they are disabled and in a known state.
>>>> See the comment below.
>>>>
>>>>>
>>>>>
>>>>>> +			return PTR_ERR(reset);
>>>>>> +		};
>>>>>> +
>>>>>> +		/* reset OSPI to ensure CR_EN bit is set to 0 */
>>>>>> +		reset_control_assert(reset);
>>>>>> +		udelay(2);
>>>>>> +		reset_control_deassert(reset);
>>>>>
>>>>> No, the child should handle this, not parent.
>>>>
>>>> Octo Memory Manager can only be configured if both child are disabled.
>>>> That's why here, parent handles this.
>>>
>>> So if device by any chance started and is doing some useful work, then
>>> you cancel that work and reset it?
>>
>> stm32_omm_configure() is only called if we get access granted on both children.
>> That means we are authorized to use these devices, so we can reset them.
>>
>>>
>>> And what if child does not have reset line? Your binding allows that, so
>>> how is it supposed to work then?
>>
>> Ah yes, you are right, the OSPI bindings need to be updated
>> by requiring reset lines and the driver spi-stm32-ospi.c as well.
>> I will send a fix for that.
>>
>> Thanks for pointing this.
>>
>>>
>>> This also leads me to questions about bindings - if you need to assert
>>> some reset, doesn't it mean that these resets are also coming through
>>> this device so they are part of this device node?
>>
>> As we are able to retrieve children's reset from their respective node,
>> if you don't mind, OMM bindings can be kept as it's currently.
> 
> But that is what the entire discussion is about - I do mind. I said it
> already - you are not supposed to poke into child's node.
> 
> If you need to toggle child's resources, then I claim these are your
> resources as well.

Hi Krzysztof 

Ok i will update both OMM driver and bindings accordingly.

> 
>>
>> And another information, on some MP2 SoCs family, there is only one 
>> OSPI instance. So for these SoCs, there is no Octo Memory Manager.
>>
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> +		reset_control_put(reset);
>>>>>> +		clk_disable_unprepare(omm->child[i].clk);
>>>>>> +	}
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int stm32_omm_probe(struct platform_device *pdev)
>>>>>> +{
>>>>>> +	struct platform_device *vdev;
>>>>>> +	struct device *dev = &pdev->dev;
>>>>>> +	struct stm32_omm *omm;
>>>>>> +	struct clk *clk;
>>>>>> +	int ret;
>>>>>> +	u8 child_access_granted = 0;
>>>>>
>>>>> Keep inits/assignments together
>>>>
>>>> ok
>>>>
>>>>>
>>>>>> +	u8 i, j;
>>>>>> +	bool child_access[OMM_CHILD_NB];
>>>>>> +
>>>>>> +	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");
>>>>>> +			ret = -E2BIG;
>>>>>> +			goto err_clk_release;
>>>>>> +		}
>>>>>> +
>>>>>> +		if (!of_device_is_compatible(child, "st,stm32mp25-ospi")) {
>>>>>> +			ret = -EINVAL;
>>>>>> +			goto err_clk_release;
>>>>>> +		}
>>>>>> +
>>>>>> +		ret = stm32_omm_check_access(dev, child);
>>>>>> +		if (ret < 0 && ret != -EACCES)
>>>>>> +			goto err_clk_release;
>>>>>> +
>>>>>> +		child_access[omm->nb_child] = false;
>>>>>> +		if (!ret) {
>>>>>> +			child_access_granted++;
>>>>>> +			child_access[omm->nb_child] = true;
>>>>>> +		}
>>>>>> +
>>>>>> +		omm->child[omm->nb_child].node = child;
>>>>>> +
>>>>>> +		clk = of_clk_get(child, 0);
>>>>>
>>>>> Why are you taking children clock? And why with this API, not clk_get?
>>>>
>>>> I need children's clock to reset them.
>>>
>>>
>>> The device driver should reset its device. It is not a discoverable bus,
>>> that would explain power sequencing from the parent.
>>>
>>>> Why of_clk_get() usage is a problem here ? i can't get your point ?
>>>
>>> Because it is not the API which device drivers should use. You should
>>> use clk_get or devm_clk_get.
>>
>>
>> ok, i will update this part using clk_get().
>>
>>>
>>>
>>>>
>>>>> This looks like mixing clock provider in the clock consumer.
>>>>>
>>>>>> +		if (IS_ERR(clk)) {
>>>>>> +			dev_err(dev, "Can't get child clock\n");
>>>>>
>>>>> Syntax is always return dev_err_probe (or ret = dev_err_probe).
>>>>
>>>> ok
>>>>
>>>>>
>>>>>> +			ret = PTR_ERR(clk);
>>>>>> +			goto err_clk_release;
>>>>>> +		};
>>>>>> +
>>>>>> +		omm->child[omm->nb_child].clk = clk;
>>>>>> +		omm->nb_child++;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (omm->nb_child != OMM_CHILD_NB) {
>>>>>> +		ret = -EINVAL;
>>>>>> +		goto err_clk_release;
>>>>>> +	}
>>>>>> +
>>>>>> +	platform_set_drvdata(pdev, omm);
>>>>>> +
>>>>>> +	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;
>>>>>> +
>>>>>> +	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))
>>>>>
>>>>> If you have a device available, why do you create one more platform device?
>>>>>
>>>>>> +			continue;
>>>>>> +
>>>>>> +		vdev = of_platform_device_create(omm->child[i].node, NULL, NULL);
>>>>>
>>>>> Why you cannot just populate the children?
>>>>
>>>> I can't use of_platform_populate(), by default it will populate all OMM's child.
>>>> Whereas here, we want to probe only the OMM's child which match our criteria.  
>>>
>>>
>>> Why wouldn't you populate everyone? The task of bus driver is not to
>>> filter out DT. If you got such DT - with all device nodes - you are
>>> expected to populate all of them. Otherwise, if you do not want all of
>>> them, it is expected that firmware or bootloader will give you DT
>>> without these nodes.
>>
>> We don't want to populate every child by default because we can get 
>> cases where one child is shared between Cortex A and Cortex M.
> 
> But in such case DTB would not have that child enabled.
> 
>> That's why we must check if access is granted which ensure that 
>> firewall semaphore is available (RIFSC semaphore in our case).
> 
> If you do not have access, means child is assigned to other processor,
> right? In that case that child would not have been enabled in your DTB.
> 
> Fix your DTB instead of creating another layer of handling children
> inside drivers.

In fact, initially, we wanted to avoid to trigger Illegal Access in case user 
didn't use correct DT, that's why, here, double checks have been implemented.
But Ok, i will clean this part and simply populate children.

Thanks
Patrice.

> 
> 
> Best regards,
> Krzysztof