[PATCH 2/3] soc: qcom: rpmh and cmd-db drivers

[Sumit Garg]

Hi Caleb,

On Mon, 8 Jul 2024 at 16:13, Caleb Connolly <caleb.connolly at linaro.org> wrote:
>
> Hi Sumit,
>
> Sorry for the late response.
>
> In general, I'd rather keep this port closer to the Linux original, so
> that future cherry-picking of new features and fixes is easier. This is
> quite standard practice in U-Boot (and why the compatibility macro's are
> included in the first place).
>
> Otherwise, we end up with our own quite different implementation, with
> the higher maintenance burden that comes with it.
>
> This port doesn't do a perfect job at keeping close to upstream, but
> still I don't think we should stray further without a good justification.
>

As per your cover letter:

Quote:
   "These drivers were originally taken from Linux, however they have been
    heavily modified and simplified for U-Boot. Since the original Linux
    drivers contained heavy optimisations related to multithreading and
    asynchronous probing, as well as support for idle and suspend states
    which we don't need to deal with here."

This made me think the driver is nowhere close to Linux driver
functionality wise. Also, the commit message doesn't say which Linux
commit this driver is influenced from. I am not really sure if
cherry-picking will be straight forward. IMHO, if we are heavily
modifying a driver port for U-Boot then it should be done properly as
per U-Boot needs. A middle ground approach which you have taken is
only going to lead to confusion and rather negatively impact
maintenance.

-Sumit

> Kind regards,
>
> >> +EXPORT_SYMBOL(cmd_db_read_addr);
> >
> > Export symbols aren't required in U-Boot here and other instances in this patch.
>
> ...
>
> >> +       /*
> >> +        * Wait until we read back the same value.  Use a counter rather than
> >> +        * ktime for timeout since this may be called after timekeeping stops.
> >> +        */
> >> +       for (i = 0; i < USEC_PER_SEC; i++) {
> >> +               if (readl(addr) == data)
> >> +                       return;
> >> +               udelay(1);
> >> +       }
> >
> > Can we rather use readx_poll_sleep_timeout() here instead?
>
> ...
> >> +       for (i = 0; i < 5 * USEC_PER_SEC; i++) {
> >> +               if (readl(tcs_reg_addr(drv, reg, tcs_id)) & BIT(tcs_id))
> >> +                       break;
> >> +               udelay(1);
> >> +       }
> >
> > Can we rather use readx_poll_sleep_timeout() here instead?
>
> ...
>
> >> +
> >> +       spin_lock_irqsave(&drv->lock, flags);
> >
> > Locks aren't needed in U-Boot, can be dropped here and other places.
> >
>
> ...
>
> >> +
> >> +       spin_lock_init(&drv->lock);
> >> +       init_waitqueue_head(&drv->tcs_wait);
> >
> > Similarly waitqueue should be dropped too.
> >
> > -Sumit
>
> --
> // Caleb (they/them)