[U-Boot] Pull request: u-boot-riscv/master

[Sean Anderson]

On 5/26/20 3:41 AM, Rick Chen wrote:
> Hi Tom
> 
>> From: Tom Rini [mailto:trini at konsulko.com]
>> Sent: Monday, May 25, 2020 11:40 PM
>> To: Open Source Project uboot
>> Cc: u-boot at lists.denx.de; Rick Jian-Zhi Chen(陳建志)
>> Subject: Re: [U-Boot] Pull request: u-boot-riscv/master
>>
>> On Mon, May 25, 2020 at 04:01:08PM +0800, uboot at andestech.com wrote:
>>
>>> Hi Tom,
>>>
>>> Please pull some riscv updates:
>>>
>>> - Add Sipeed Maix support.
>>> - sifive: fix palmer's email address.
>>> - Move all SMP related SBI calls to SBI_v01.
>>>
>>> https://travis-ci.org/github/rickchen36/u-boot-riscv/builds/690778926
>>>
>>> Thanks
>>> Rick
>>>
>>>
>>> The following changes since commit 9c5fef577494769e3ff07952a85f9b7125ef765b:
>>>
>>>   Merge git://git.denx.de/u-boot-usb (2020-05-22 22:58:50 -0400)
>>>
>>> are available in the Git repository at:
>>>
>>>   git at gitlab.denx.de:u-boot/custodians/u-boot-riscv.git
>>>
>>> for you to fetch changes up to 421c4eb2dcf39f65c31c1804369267ed8a7b5607:
>>>
>>>   riscv: Add Sipeed Maix support (2020-05-25 10:01:21 +0800)
>>>
>>

Hi, I just saw this thread, and I would like to comment on a few things.

>> There's too many generic changes in this PR for this late in the cycle, for things I gather related to the Maix platform.  All of the clk and DM changes impact virtually all platforms.  Please re-do this PR to drop the Maix platform support for now, and resubmit that for -next, which I will be opening shortly.  Everything else however is good for master.
> 
> Thanks for your explanations.
> Actually I have the same concern with you before sending this PR.
> 
> I have tried to ask Sean don't mix all codes together in one patchset.
> He shall separate and send them individually, If they are not highly dependent.

I have been asked by Bin Meng and others to keep together related
patches so that the patch which uses changed/new behaviour is in the
same series as the patch which changes the behavior. I have tried to
split out unrelated patches and minimize changes. Would you rather all
patches which do not explicitly add maix support be split off from this
series?

> 
> Following are my suggestions from those patch-work:
> 
> Re: [PATCH v2 01/11] clk: Always use the supplied struct clk
> https://patchwork.ozlabs.org/project/uboot/patch/da401261-b73f-afae-0702-ada1e7dd836b@gmail.com/

The composite clock framework is only used by IMX in U-Boot as far as I
can tell. As such, it has a number of (mis)features which make it
unsuitable for generic code. These quirks are generally worked around in
a variety of ways by IMX code, often by using custom functions instead
of those within the CCF itself. The maix platform has around 40 clocks
in a complicated tree, and is not constrained by RAM or flash. Given
these circumstances, I think using the CCF is the correct choice.
However, in order to do so, I have had to address many of the quirks. I
could also add support for maix by effectively duplicating the CCF
within the maix driver. However, I think such a duplication of code is
likely to rot over time.

I have tried to describe why I think each of the clk patches should be
included in their commit messages. It may be unclear what the impact
will be on other users of the clock subsystem and the CCF.

[01/21] clk: Always use the supplied struct clk

This patch only affects users of the CCF. As discussed in the above
link, this specific behaviour is left over from when the CCF was ported
from Linux, due to the different semantics between struct clk and struct
clk_core. The problem here is that with the current system, either a
struct device must be associated with each CCF clock (a significant
overhead), or custom functions must be used for CCF clock operations.
The latter approach is used by the IMX code, but I would like to be able
to use the generic functions which are provided by CCF clocks.

[v13,02/21] clk: Check that ops of composite clock components exist before calling 

This only affects the CCF subsystem. This is a bugfix for drivers which
use composite clocks and do not always have the same sub-clocks. Without
this patch, if one registers a composite clock with a gate sub-clock,
and then registers another composite clock without a gate sub-clock,
calling a enable or disable on the latter clock will result in calling a
NULL pointer. No drivers currently use this functionality, but it is a
"gotcha" if one implements a driver which does.

[v13,03/21] clk: Unconditionally recursively en-/dis-able clocks

This affects all clock drivers, but primarily affects CCF clocks. This
patch is designed to handle a case with a configuration like


   A
  / \
 B   C
 |
 D

where B is a clock with an id of 0. If D is enabled, B and A will not be
enabled. If C is enabled, A will also be enabled. If C is then disabled,
A will also be disabled. This can cause strange behaviour. I have since
added ids to all clocks I use, so this patch can be split off.

[v13,04/21] clk: Fix clk_get_by_* handling of index 

This affects all clock drivers, but is a bugfix. The parameters passed
to clk_get_by_index_tail have the wrong semantics. This patch primarily
affects callers of clk_get_by_index_nodev (2 drivers), and improves
error messages.

So of these four patches, I think 3 are 100% necessary for new drivers
using the CCF. One is nice to have, but could be split off to reduce the
scope of this patch. I would have liked to get all of these patches
reviewed by Lukas, but he hasn't commented on this series for several
months. I would also love to test some of these patches on IMX, but I
don't own any boards with IMX on them.

> Re: [PATCH v4 07/17] spi: dw: Add mem_ops
> https://patchwork.ozlabs.org/project/uboot/patch/20200211060425.1619471-8-seanga2@gmail.com/

Here I took your advice and split off the pinctrl-, gpio-, and spi-related patches.

> 
> Re: [PATCH v5 33/33] riscv: Add Sipeed Maix support
> http://u-boot.10912.n7.nabble.com/PATCH-v5-00-33-riscv-Add-Sipeed-Maix-support-tt401784.html#a402055

ditto

> 
> Re: [PATCH v6 04/19] clk: Add functions to register CCF clock structs
> https://patchwork.ozlabs.org/project/uboot/patch/20200305181308.944595-5-seanga2@gmail.com/

I removed this patch in the next revision of the series.

On 6/23/20 9:31 PM, Rick Chen wrote:
> Hi Sean
> 
> Tom Rini <trini at konsulko.com> 於 2020年6月23日 週二 上午8:45寫道：
>> So I looked over the generic changes again.  There's no other way to
>> support the platform without those type of changes, yes?  If so, yes,
>> lets put it in to -next.

I saw this only after writing the above explanation, but hopefully it
will stioll be helpful.

> 
> Please rebase -next for upstream
> There are some conflicts needed to be fixed.
> 
> Applying: clk: Always use the supplied struct clk
> Applying: clk: Check that ops of composite clock components exist before calling
> Applying: clk: Unconditionally recursively en-/dis-able clocks
> Applying: clk: Fix clk_get_by_* handling of index
> Applying: clk: Add K210 pll support
> Applying: clk: Add a bypass clock for K210
> Applying: clk: Add K210 clock support
> Applying: dm: Add support for simple-pm-bus
> Applying: dm: Fix error handling for dev_read_addr_ptr
> Applying: reset: Add generic reset driver
> error: patch failed: configs/sandbox_defconfig:196
> error: configs/sandbox_defconfig: patch does not apply
> Patch failed at 0010 reset: Add generic reset driver

Ok, I can rebase again.

--Sean