I guess Alexandre will help you prepare this for upstream inclusion, but just want to make sure that my main point makes it across -- this is a 3K line patch. Please try to split it up into patches that add no more than 300 lines at a time, preferably fewer (but this isn't always possible). Ideally each patch should introduce one conceptual unit. Of course different people might draw the line of "conceptual unit" differently, but try to do it so that each one becomes 100-300 lines of code :) These aren't hard limits by the way, but good rules of thumb to go by when sending patches for upstream inclusion. Cheers, -ilia On Thu, Mar 12, 2015 at 1:20 AM, Deepak Goyal <dgoyal@xxxxxxxxxx> wrote: > Hi Mirkin, > > Your observations are quiet correct. > After the boot code is submitted successfully, I will submit the code to configure & enable features of PMU.(This will be done by sending cmds to PMU). > > Now talking about this patch: > Apart from just the boot code, I have also included some things in this patch that I can remove for now(I will include these things in later digestible chunks): > > - Debugfs support (can be removed for now) > - Debug support for dumping PMU falcon registers(can be removed for now) > - PMU interacts with Kernel via interrupt mechanism. > For interaction with PMU, we have > defined command structs, functions to prepare/validate and send commands to PMU. > This infrastructure is basically to send commands to PMU.(right now it can be removed though we still require to receive messages from PMU to know if it has booted successfully). > > But this will be all that I will be able to remove from this patch. > Can I go ahead with removing above suggestions? > > Regards, > Deepak G > > -----Original Message----- > From: ibmirkin@xxxxxxxxx [mailto:ibmirkin@xxxxxxxxx] On Behalf Of Ilia Mirkin > Sent: Wednesday, March 11, 2015 10:41 PM > To: Deepak Goyal > Cc: Ben Skeggs; Alexandre Courbot; nouveau@xxxxxxxxxxxxxxxxxxxxx; linux-tegra@xxxxxxxxxxxxxxx > Subject: Re: [Nouveau] [PATCH] pmu/gk20a: PMU boot support. > > Hi Deepak, > > There's... a lot of stuff going on here. Can you describe the goal of > this patch (which could then be used as the patch commit message)? The > current one basically boils down to "Add support for loading PMU", but > merely loading the fw into a fuc engine is just a handful lines of > code. Also, except in rare cases, it's customary to split up patches > of this size into smaller, more reviewable chunks, which add on bits > of functionality as they go. > > From what I can tell, you're adding the kernel-side interface for a > hypothetical (and presumably closed-source) PMU blob that NVIDIA will > supply. In essence, the blob is expected to implement a RTOS which > runs on the PMU's falcon CPU. There are a bunch of API's implemented > by this blob that the host can call, but it also does things on its > own. For the kernel side, each of these API calls should probably be a > separate patch (after an initial "just load it and do nothing" style > patch). Or perhaps have the infrastructure that you add first and then > something that implements the API calls. > > Cheers, > > -ilia > > > On Wed, Mar 11, 2015 at 2:33 AM, Deepak Goyal <dgoyal@xxxxxxxxxx> wrote: >> It adds PMU boot support.It loads PMU >> firmware into PMU falcon.RM/Kernel driver >> receives INIT ack (through interrupt mechanism) >> from PMU when PMU boots with success. >> >> Signed-off-by: Deepak Goyal <dgoyal@xxxxxxxxxx> >> --- >> drm/nouveau/include/nvkm/subdev/pmu.h | 26 +- >> drm/nouveau/nvkm/subdev/pmu/base.c | 108 ++ >> drm/nouveau/nvkm/subdev/pmu/gk20a.c | 2131 ++++++++++++++++++++++++++++++++- >> drm/nouveau/nvkm/subdev/pmu/gk20a.h | 369 ++++++ >> drm/nouveau/nvkm/subdev/pmu/priv.h | 264 ++++ >> 5 files changed, 2884 insertions(+), 14 deletions(-) >> create mode 100644 drm/nouveau/nvkm/subdev/pmu/gk20a.h >> -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
