Re: [PATCH 1/8] mmc: sdhci-cadence: Add reset control support

[Tanmay Kathpalia <tanmay.kathpalia@altera.com>]

On 11/27/2025 3:18 PM, Peng Fan wrote:
>> Subject: Re: [PATCH 1/8] mmc: sdhci-cadence: Add reset control
>> support
>>
>> Thanks for the suggestion, Peng.
>>
>> On 11/26/2025 7:47 AM, Peng Fan wrote:
>>> On Thu, Nov 20, 2025 at 08:29:11PM +0530, Tanmay Kathpalia
>> wrote:
>>>> Thanks for your comment, Peng.
>>>>
>>>> On 11/18/2025 10:39 AM, Peng Fan wrote:
>>>>> On Mon, Nov 10, 2025 at 09:37:30AM -0800, Tanmay Kathpalia
>> wrote:
>>>>>> Add reset control functionality to the SDHCI Cadence driver to
>>>>>> properly handle hardware reset sequences during probe. This
>> ensures
>>>>>> the controller is in a known state before initialization.
>>>>>>
>>>>>> Signed-off-by: Tanmay Kathpalia <tanmay.kathpalia@altera.com>
>>>>>> Reviewed-by: Balsundar Ponnusamy
>> <balsundar.ponnusamy@altera.com>
>>>>>> ---
>>>>>> drivers/mmc/sdhci-cadence.c | 8 ++++++++
>>>>>> 1 file changed, 8 insertions(+)
>>>>>>
>>>>> ....
>>>>>>
>>>>>> @@ -225,6 +227,12 @@ static int sdhci_cdns_probe(struct
>> udevice *dev)
>>>>>> 	if (!plat->hrs_addr)
>>>>>> 		return -ENOMEM;
>>>>>>
>>>>>> +	ret = reset_get_bulk(dev, &reset_bulk);
>>>>>
>>>>> Should this be optional? Some in tree platforms may not have the
>>>>> reset supported.
>>>>>
>>>>
>>>> Yes, you're right-some in-tree platforms may not have reset support.
>>>> In those cases, the code will print a warning message ("Can't get
>>>> reset") and continue the probe process.
>>>> If you prefer, I can remove the warning and let the function fail
>>>> silently instead, or is there any other way you would suggest to
>> make this optional?
>>>
>>> devm_reset_bulk_get_optional() may help.
>>>
>>> Regards
>>> Peng
>>>
>>
>> I looked into devm_reset_bulk_get_optional(), and I see that it
>> dynamically allocates the struct reset_ctl_bulk and adds it to the
>> device resources list if CONFIG_DEVRES is enabled. However, if
>> CONFIG_DEVRES is not enabled, we need to manually free the memory
>> using
>> reset_release_bulk() in the driver's remove function. This means we
>> would need to store a pointer to struct reset_ctl_bulk in the driver's
>> private data, which would require additional changes to the sdhci-
>> cadence driver (since it currently uses the generic struct sdhci_host
>> with .priv_auto = sizeof(struct sdhci_host)).
>>
>> Let me know if you’re okay with this approach, as it would require
>> other changes in the driver, or if you have any further
>> recommendations.
>> Alternatively, I can simplify the implementation as shown below:
>>
>> ret = reset_get_bulk(dev, &reset_bulk);
>> if (!ret)
>> 	reset_deassert_bulk(&reset_bulk);
> 
> For better, an optional API is preferred, since non-devres API
> is not there, I am fine with your above changes.
> 
> Regards
> Peng.
> 

I will make the changes and push the V2 series.

Regards,
Tanmay

>>
>> Please let me know your preference.
>>
>> Regards,
>> Tanmay
>>
>>>>
>>>>> Regards
>>>>> Peng
>>>>
>>>>
>