Re: [PATCH] remoteproc: k3-r5: Decouple firmware booting from probe routine

[Beleswar Prasad Padhi <b-padhi@ti.com>]

On 07/09/24 15:41, Kumar, Udit wrote:
>
> On 9/6/2024 3:10 PM, Beleswar Padhi wrote:
>> The current implementation of the waiting mechanism in probe() waits for
>> the 'released_from_reset' flag to be set which is done in
>> k3_r5_rproc_prepare() as part of rproc_fw_boot(). This causes unexpected
>> failures in cases where the firmware is unavailable at boot time,
>> resulting in probe failure and removal of the remoteproc handles in the
>> sysfs paths.
>
> I won't say failure, I will say this is behavior of driver.
>
> Driver expect firmware to be available , but I agree driver should be 
> able to execute
>
> with/without firmware availability.
>
>
>> To address this, the waiting mechanism is refactored out of the probe
>> routine into the appropriate k3_r5_rproc_prepare/unprepare() and
>> k3_r5_rproc_start/stop() functions. This allows the probe routine to
>> complete without depending on firmware booting, while still maintaining
>> the required power-synchronization between cores.
>>
>> Fixes: 61f6f68447ab ("remoteproc: k3-r5: Wait for core0 power-up 
>> before powering up core1")
>> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
>> ---
>> Posted this as a Fix as this was breaking usecases where we wanted to 
>> load a
>> firmware by writing to sysfs handles in userspace.
>>
>>   drivers/remoteproc/ti_k3_r5_remoteproc.c | 170 ++++++++++++++++-------
>>   1 file changed, 118 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
>> b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> [..]
>> +    core0 = list_first_entry(&cluster->cores, struct k3_r5_core, elem);
>> +    core1 = list_last_entry(&cluster->cores, struct k3_r5_core, elem);
>> +    if (cluster->mode == CLUSTER_MODE_SPLIT && core == core1 &&
>> +        core0->released_from_reset == false) {
>> +        ret = 
>> wait_event_interruptible_timeout(cluster->core_transition,
>> +                               core0->released_from_reset,
>> +                               msecs_to_jiffies(2000));
> only one wait in start should be good enough,


Won't that cause race conditions, where prepare for core1 can be called 
before core0?

>> +        if (ret <= 0) {
>> +            dev_err(dev, "can not power up core1 before core0");
>> +            return -EPERM;
>> +        }
>> +    }
>> +
>>       ret = ti_sci_proc_get_status(core->tsp, &boot_vec, &cfg, &ctrl, 
>> &stat);
>>       if (ret < 0)
>>           return ret;
>> @@ -470,6 +492,12 @@ static int k3_r5_rproc_prepare(struct rproc *rproc)
>>           return ret;
>>       }
>>   +    /* Notify all threads in the wait queue when core state has 
>> changed so
>> +     * that threads waiting for this condition can be executed.
>> +     */
>> +    core->released_from_reset = true;
>> +    wake_up_interruptible(&cluster->core_transition);
>> +
>>       /*
>>        * Newer IP revisions like on J7200 SoCs support h/w 
>> auto-initialization
>>        * of TCMs, so there is no need to perform the s/w memzero. 
>> This bit is
>> @@ -515,14 +543,46 @@ static int k3_r5_rproc_unprepare(struct rproc 
>> *rproc)
>>   {
>>       struct k3_r5_rproc *kproc = rproc->priv;
>>       struct k3_r5_cluster *cluster = kproc->cluster;
>> -    struct k3_r5_core *core = kproc->core;
>> +    struct k3_r5_core *core0, *core1, *core = kproc->core;
>
>
> why you need wait in unprepare/stop,
>
> In case you are failing during firmware load or so then already we are 
> in bad state.
>
> if this is call from user land then, i don't except anyone will auto 
> trigger stopping of core-0
>
> IMO, in unprepare/stop, if action is attempted on core1 with core-0 
> ON, simply return error.


Yes, you are correct. Will include this change in revision. Thanks!

>
>
>> [..]
>> @@ -629,7 +702,7 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
>>       struct k3_r5_rproc *kproc = rproc->priv;
>>       struct k3_r5_cluster *cluster = kproc->cluster;
>>       struct device *dev = kproc->dev;
>>