Re: [PATCH v2 1/8] dm: fpga: Introduce new uclass

[Simon Glass <sjg@chromium.org>]

Hi Alex,

On Mon, 26 Sept 2022 at 00:14, Alexander Dahl <post@lespocky.de> wrote:
>
> Hello Simon,
>
> Am Sun, Sep 25, 2022 at 08:15:38AM -0600 schrieb Simon Glass:
> > Hi Michal,
> >
> > On Thu, 22 Sept 2022 at 05:45, Michal Simek <michal.simek@amd.com> wrote:
> > >
> > >
> > >
> > > On 9/22/22 13:35, Simon Glass wrote:
> > > > Hi,
> > > >
> > > > On Thu, 22 Sept 2022 at 12:27, Michal Simek <michal.simek@amd.com> wrote:
> > > >>
> > > >>
> > > >>
> > > >> On 9/21/22 15:22, Alexander Dahl wrote:
> > > >>> For future DM based FPGA drivers and for now to have a meaningful
> > > >>> logging class for old FPGA drivers.
> > > >>>
> > > >>> Suggested-by: Michal Simek <michal.simek@amd.com>
> > > >>> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> > > >>> ---
> > > >>>    include/dm/uclass-id.h | 1 +
> > > >>>    1 file changed, 1 insertion(+)
> > > >>>
> > > >>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > > >>> index a432e43871..c2b15881ba 100644
> > > >>> --- a/include/dm/uclass-id.h
> > > >>> +++ b/include/dm/uclass-id.h
> > > >>> @@ -56,6 +56,7 @@ enum uclass_id {
> > > >>>        UCLASS_ETH,             /* Ethernet device */
> > > >>>        UCLASS_ETH_PHY,         /* Ethernet PHY device */
> > > >>>        UCLASS_FIRMWARE,        /* Firmware */
> > > >>> +     UCLASS_FPGA,            /* FPGA device */
> > > >>>        UCLASS_FUZZING_ENGINE,  /* Fuzzing engine */
> > > >>>        UCLASS_FS_FIRMWARE_LOADER,              /* Generic loader */
> > > >>>        UCLASS_GPIO,            /* Bank of general-purpose I/O pins */
> > > >>
> > > >> Simon: the whole series look good to me. I am happy to take it via my tree when
> > > >> you ACK it. Also no problem if you want to take it via your tree.
> > > >> Please let me know which way you want to go.
> > > >
> > > > This is a good step forward but needs a lot more work.
> > > >
> > > > Please add a uclass file for the FPGA - i.e.
> > > > drivers/fpga/fpga-uclass.c - see other such files for examples.
> > > >
> > > > The FPGA uclass should have methods that match the non-DM interface.
> > > > You will likely need a DM_FPGA config to allow enabling the uclass.
> > > >
> > > > Also this needs a simple sandbox driver/emulator pair, so that it can
> > > > be tested, with tests in test/dm/fpga.c that use the driver.
> > > >
> > > > Admittedly this should have been done ages ago. I vaguely remember
> > > > mentioning it at the time, but perhaps I missed it. In any case, all
> > > > uclasses must have an API, implementation and tests that run in CI
> > > > with sandbox. Testing is a vital part of U-Boot and lack of testing is
> > > > the main reason why we went back to the 3-month release cycle.
> > >
> > > It can be done in steps for sure. Issues which Alex is addressing are there for
> > > quite some time and I think we shouldn't gate them by adding requirement to
> > > create the whole fpga uclass. It can be done on the top of this series.
> > > We know that it has to happen but I wouldn't push Alex to do it as condition for
> > > applying this series.
> > >  From my perspective if he has time to do, let's start with it. If not it can be
> > > done later.
> >
> > Well if this is a start, then let's make it a real start. At minimum:
> >
> > - add a uclass file with the uclass driver
> > - we can skip having any methods for now
> > - add a sandbox driver which does nothing
> > - add a test which probes the sandbox device
> >
> > That is about 50 lines of code and people can then add to it over time.
>
> FWIW, I already did that on the weekend, I just have to look over
> it again and maybe give it some polishing before sending.  Draft ist
> here:
>
> https://github.com/LeSpocky/u-boot/commit/49efd2a2d0129b977d38340c836bbbb1f080043b
>
> > Without that, I'd rather not have the UCLASS_FPGA.
>
> That's okay I guess.  Will just take me some time, it's not that easy
> if you have to learn about DM and UT first, and try it in the end of
> the day. ;-)

Yes, check the docs on testing.

What you have looks OK, just needs a uclass_first_device(UCLASS_FPGA,
..) in the test to make sure it can probe the device.

Regards,
Simon