[PATCH v1 1/2] clk: socfpga: Read the clock parent's register base in probe function

[Simon Glass]

Hi Marek,

On Thu, 2 Apr 2020 at 15:07, Marek Vasut <marex at denx.de> wrote:
>
> On 4/2/20 10:54 PM, Tom Rini wrote:
> [...]
>
> >>>>>> I'm not sure it definitely should be changed. But I'll do a patch and
> >>>>>> see how it looks.
> >>>>>
> >>>>> Do I understand it correctly that the patch
> >>>>> 82de42fa14682d408da935adfb0f935354c5008f actually completely breaks
> >>>>> SoCFPGA ? Then I would say this is a release blocker ?
> >>>> Yes. A10 SPL won't boot at all. It crashes during the clock manager setup.
> >>>
> >>> This came in right at the beginning of the cycle. I thought the
> >>> purpose of the 3-month cycle was to allow time to test?
> >>
> >> It was ... altera ?
> >
> > Sorry, I'm missing how that's an answer to the question.  This came in
> > basically right at the start of the merge window.
>
> I don't have an A10 available right now, so what can I do ?
>
> >>> I do plan to try out changing the behaviour to read a parent's ofdata
> >>> before the child, but I am not comfortable adding such a major change
> >>> just before a release. It could have any number of ill effects.
> >>>
> >>> Can you update the clock driver? E.g. you could move some of the code
> >>> from socfpga_a10_ofdata_to_platdata() to a probe() method?
> >>
> >> Can we revert the patch which broke arria10 instead ? It did work
> >> before, so who knows how many other ill side effects there are ...
> >
> > I'm not in favor of reverting 82de42fa1468 unless the answer is "that
> > commit was wrong" and not "that commit caused other problems to surface,
> > that we can fix".  I am quite willing to say "we need to delay the
> > release to test changes for problems that've surfaced and verify more
> > hardware is fine".
>
> If I understand this correctly, the clock code did scan the DT and bound
> all the clocks, so that drivers can use them. That's how bind function
> should work, it creates instances of devices, but without probing them.
> This doesn't work anymore if I understand it correctly, but that means
> the basic premise of DT is broken ? And the solution is to do more magic
> in probe function , which I don't think even works here ?

No...please see here from the cover letter:

    Cover-letter:
    dm: core: Fully separate ofdata_to_platdata() from probe()
    At present ofdata_to_platdata() is done as part of device_probe(). Drivers
    are supposed to read their platdata in the ofdata_to_platdata method() but
    this requirement is not always followed.

    Having these methods separate helps deal with of-platdata, where the
    probe() method is common to both DT/of-platdata operation, but the
    ofdata_to_platdata() method is implemented differently.

    Another case has come up where this separate is useful. Generation of ACPI
    tables uses the of-platdata but does not want to probe the device. Probing
    would cause U-Boot to violate one of its design principles, viz that it
    should only probe devices that are used. For ACPI we want to generate a
    table for each device, even if U-Boot does not use it. In fact it may not
    even be possible to probe the device - e.g. an SD card which is not
    present will cause an error on probe, yet we still must tell Linux about
    the SD card connector in case it is used while Linux is running.

    This series splits out the ofdata_to_platdata() part of device_probe() so
    that it can be used separately from device_probe(). Thus devices now go
    through two distinct states when probing: reading platform data and
    actually touching the hardware to bring the device up.

    This should not break existing boards since the operations still happen in
    mostly the same order. The main change is that parents and uclasses are
    probed after ofdata_to_platdata() is called.

    HOWEVER it is quite possible that some boards break the rules and due to
    a series of unfortunate events, something will break. Two boards were
    found in this category already. SO this series may require some tidying up
    from board maintainers, if problems arise.

    Note that there are cases where devices must be probed in the
    ofdata_to_platdata() method. An example is where a GPIO is selected - this
    obviously requires that the GPIO device is probed.

    One board was found to burst its size limit with this series, despite the
    very small size increase. The patches to remove use of BUG_ON() are to
    ensure that this series passes tests.
    END


Regards,
Simon