RE: [PATCH v5 2/4] drivers: firmware: xilinx: Add ZynqMP firmware driver

[Jolly Shah]

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