Hi Sudeep, > -----Original Message----- > From: Sudeep Holla [mailto:sudeep.holla@xxxxxxx] > Sent: Tuesday, March 13, 2018 3:24 AM > To: Jolly Shah <JOLLYS@xxxxxxxxxx> > Cc: Sudeep Holla <sudeep.holla@xxxxxxx>; ard.biesheuvel@xxxxxxxxxx; > mingo@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; matt@xxxxxxxxxxxxxxxxxxx; > hkallweit1@xxxxxxxxx; keescook@xxxxxxxxxxxx; > dmitry.torokhov@xxxxxxxxx; robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; > Rajan Vaja <RAJANV@xxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; michal.simek@xxxxxxxxxx > Subject: Re: [PATCH v5 2/4] drivers: firmware: xilinx: Add ZynqMP firmware > driver > > > > On 12/03/18 23:05, Jolly Shah wrote: > > > > [...] > > >> > >> OK, what are the types you are referring here ? or why PSCI is not sufficient ? > >> How do you plan to use these APIs in Linux ? > > > > It supports system/subsystem restart as types. For example, only APU > > restart, system restart, PS restart for ZynqMP PSCI doesn’t support > > any argument to identify these types. Linux, one can set the reset > > scope through debug interface and execute "reboot" then. Inside ATF, > > PSCI_SYSTEM_RESET mapped function will call EEMI API with that scope. > > OK, I am not sure how you use them in Linux. Please add drivers using them or > just drop them for now and add when you add the users of these functions. > > >> > >>>> > >>>>> +static const struct zynqmp_eemi_ops eemi_ops = { > >>>>> + .get_api_version = zynqmp_pm_get_api_version, > >>>>> + .get_chipid = zynqmp_pm_get_chipid, > >>>>> + .reset_assert = zynqmp_pm_reset_assert, > >>>>> + .reset_get_status = zynqmp_pm_reset_get_status, > >>>>> + .fpga_load = zynqmp_pm_fpga_load, > >>>>> + .fpga_get_status = zynqmp_pm_fpga_get_status, > >>>>> + .sha_hash = zynqmp_pm_sha_hash, > >>>>> + .rsa = zynqmp_pm_rsa, > >>>>> + .request_suspend = zynqmp_pm_request_suspend, > >>>>> + .force_powerdown = zynqmp_pm_force_powerdown, > >>>>> + .request_wakeup = zynqmp_pm_request_wakeup, > >>>>> + .set_wakeup_source = zynqmp_pm_set_wakeup_source, > >>>>> + .system_shutdown = zynqmp_pm_system_shutdown, > >>>>> + .request_node = zynqmp_pm_request_node, > >>>>> + .release_node = zynqmp_pm_release_node, > >>>>> + .set_requirement = zynqmp_pm_set_requirement, > >>>>> + .set_max_latency = zynqmp_pm_set_max_latency, > >>>>> + .set_configuration = zynqmp_pm_set_configuration, > >>>>> + .get_node_status = zynqmp_pm_get_node_status, > >>>>> + .get_operating_characteristic = > >>>> zynqmp_pm_get_operating_characteristic, > >>>>> + .init_finalize = zynqmp_pm_init_finalize, > >>>>> + .set_suspend_mode = zynqmp_pm_set_suspend_mode, > >>>>> + .ioctl = zynqmp_pm_ioctl, > >>>>> + .query_data = zynqmp_pm_query_data, > >>>>> + .pinctrl_request = zynqmp_pm_pinctrl_request, > >>>>> + .pinctrl_release = zynqmp_pm_pinctrl_release, > >>>>> + .pinctrl_get_function = zynqmp_pm_pinctrl_get_function, > >>>>> + .pinctrl_set_function = zynqmp_pm_pinctrl_set_function, > >>>>> + .pinctrl_get_config = zynqmp_pm_pinctrl_get_config, > >>>>> + .pinctrl_set_config = zynqmp_pm_pinctrl_set_config, > >>>>> + .clock_enable = zynqmp_pm_clock_enable, > >>>>> + .clock_disable = zynqmp_pm_clock_disable, > >>>>> + .clock_getstate = zynqmp_pm_clock_getstate, > >>>>> + .clock_setdivider = zynqmp_pm_clock_setdivider, > >>>>> + .clock_getdivider = zynqmp_pm_clock_getdivider, > >>>>> + .clock_setrate = zynqmp_pm_clock_setrate, > >>>>> + .clock_getrate = zynqmp_pm_clock_getrate, > >>>>> + .clock_setparent = zynqmp_pm_clock_setparent, > >>>>> + .clock_getparent = zynqmp_pm_clock_getparent, }; > >>>>> + > >>>> Instead of introducing all these in oneshot, add them as you have users of > it. > >>>> IOW, show the users of these functions in the series. Also I asked > >>>> to split this into functional changes like clock, pinctrl, power, etc. > >>> > >>> It can be split into functional changes in same series but it will > >>> be difficult to split between users as there are more than 10 driver > >>> users for different EEMI APIs and also multiple driver users using > >>> specifc EEMI APIs. They all can't be submitted as single series. > >>> > >> > >> Why ? Start with basic EEMI and one functionality with it's > >> user/client driver in one series. Then you can top up with EEMI > >> changes for other functionality with it's user. If you introduce > >> API's without the users in a series it's hard to review and if there are more > such unused APIs I will object it in future versions. > >> > >> To start with add only clock or power APIs and functionality in this > >> series, add drivers using then. Drop other functionalities like > >> pinctrl, fpga control and other functionalities. IOW start something basic and > simple. > >> > > > > > I am ok to break it for clock/pinctrl with users but there are > > multiple users for some APIs. In that case, it will create dependency > > issues when different owners are involved. > > Also, it will hard to visualize a whole EEMI interface if its broken > > into such pieces. > > > > It's opposite, you are just adding whole lot of functions/APIs here with out the > users of those APIs. I am unable to visualise how they are getting used. So for > me even if you just add handful of above APIs with drivers making call to each > one of them along with it, I can better understand it. Without any usage of these > APIs in this series, I fail to understand the need of it. > So NACK to this patch without the users of the APIs introduced here. > Ok. I will break series with users in next version. > -- > Regards, > Sudeep ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
