From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jagan Teki Date: Mon, 8 Apr 2019 19:47:13 +0530 Subject: [U-Boot] arm: sunxi: Bananapi_M2_Ultra not working with DM_MMC In-Reply-To: References: Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de On Mon, Apr 8, 2019 at 7:31 PM Paul Kocialkowski wrote: > > Hi, > > Le lundi 08 avril 2019 à 19:21 +0530, Jagan Teki a écrit : > > On Mon, Apr 8, 2019 at 7:06 PM Paul Kocialkowski > > wrote: > > > Hi, > > > > > > Le lundi 08 avril 2019 à 19:03 +0530, Jagan Teki a écrit : > > > > On Mon, Apr 8, 2019 at 6:40 PM Paul Kocialkowski > > > > wrote: > > > > > Hi, > > > > > > > > > > Le lundi 08 avril 2019 à 18:23 +0530, Jagan Teki a écrit : > > > > > > Hi Paul, > > > > > > > > > > > > On Mon, Apr 8, 2019 at 6:00 PM Paul Kocialkowski > > > > > > wrote: > > > > > > > Hi, > > > > > > > > > > > > > > On Thu, 2019-04-04 at 05:51 -0300, Pablo Sebastián Greco wrote: > > > > > > > > A few days ago I tried to boot my Bananapi_M2_Ultra with 2019.04rc, I > > > > > > > > found that it wasn't booting, 2019.01 was working ok. > > > > > > > > Bisecting indicated that the problem was after > > > > > > > > http://git.denx.de/?p=u-boot.git;a=commitdiff;h=a7cca5793774ee139b75a704d6efaa4d29f09f93 > > > > > > > > > > > > > > I think the patch should be reverted ASAP since it obviously breaks > > > > > > > some supported configs. Sadly, the offending commit doesn't say > > > > > > > anything about the test coverage for the change and what the status is > > > > > > > after it. There is probably a reason why it was enabled for sun4i only > > > > > > > before and there must have been a motivation for doing this on all > > > > > > > sunxi platforms, but then again, the commit message says nothing about > > > > > > > those underlying reasons. > > > > > > > > > > > > > > I believe we should be more strict on patch review and not let any > > > > > > > change bringing such a major change get applied with a commit message > > > > > > > that provides no context about why the change is okay and how it was > > > > > > > tested. > > > > > > > > > > > > Appropriate your concern. > > > > > > > > > > > > If you please list what all boards are not working with this effect, > > > > > > please write back. we will defiantly look into it. All these changes > > > > > > were merged in MW which is 2.5 months back, commenting in final stage > > > > > > like this is not the professional way. > > > > > > > > > > I really do not think this is a sane approach to follow. You can't make > > > > > a change like this, with no context whatsoever in the commit message, > > > > > which ends up breaking other people's setups and wait for others to > > > > > debug subsequent issues it introduces that you don't encounter. > > > > > > > > > > Sorry but your commit should never have been merged. Sure, I wasn't > > > > > there to review it either, but the code review process definitely did > > > > > not go as planned here. > > > > > > > > Which commit message your referring to? are you referring this > > > > patch[1] commit message. let me point what exactly is the issue? > > > > > > > > [1] http://git.denx.de/?p=u-boot.git;a=commit;h=a7cca5793774ee139b75a704d6efaa4d29f09f93 > > > > > > Yes, this is the patch I'm talking about. > > > > > > The issue with it is that the commit message is totally redundant: the > > > description does not say more than what the diff does. > > > > > > A git commit description should provide context about what the change > > > does above the modified lines: what problem it tries to resolve and how > > > , on what hardware, how it was tested, etc. Especially for a commit > > > making such a big change, the commit message must have all this > > > information. > > > > > > Do you see why I think it's a problem? > > > > I can't agree if we consider the series of changes in one set. As I > > mentioned in another mail, this patch is last from the DM_MMC > > migration series and as the last one it enable the DM_MMC global to > > Allwinner (not respective to board). The previous patches are trying > > to support and fix DM_MMC on respective SoC's and board. ie the reason > > I didn't mention any thing related to board or any other information > > since It is global SoC change. > > I don't think this is relevant. You can't expect people to go through > the list archive, find which series this commit was attached to, > etc. Especially when running a bisect, where you'll end up with a > single offending commit. > > You need to make sure that each commit message provides context about > what it's doing, and not just rephrase the diff. Do you still think this message shows the diff? The change is as simple as enable DM_MMC for Allwinner and doesn't make any direct changes to underlying boards on the same change. "Enable DM_MMC for all Allwinner SoCs, this will eventually enable BLK." > > In order words, it's *never* okay to just re-work the diff, you > *always* need to describe the context. That's not something optional to > only do on special occasions. Mentioning again, I don't see any point to mention board information for this commit since the diff or change clearly focusing on global Allwinner platform enablement for DM_MMC and the same mentioned in the body.