On 5/26/20 3:41 AM, Rick Chen wrote:
Hi Tom
From: Tom Rini [mailto:trini@konsulko.com] Sent: Monday, May 25, 2020 11:40 PM To: Open Source Project uboot Cc: u-boot@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@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@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-ada...
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-se...
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-suppo...
ditto
Re: [PATCH v6 04/19] clk: Add functions to register CCF clock structs https://patchwork.ozlabs.org/project/uboot/patch/20200305181308.944595-5-sea...
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@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