| Message ID | 1351588973-20699-2-git-send-email-dev@lynxeye.de |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
Hi Lucas, On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote: > There is no need to pass around all those parameters. The init functions > are able to easily extract all the needed setup info on their own. > > Signed-off-by: Lucas Stach <dev@lynxeye.de> > --- > arch/arm/cpu/armv7/tegra20/usb.c | 24 ++++++++++++------------ > 1 Datei geändert, 12 Zeilen hinzugefügt(+), 12 Zeilen entfernt(-) I'm not sure I agree with the premise of this patch. At the top level it calls clock_get_osc_freq() to get the frequency. That is then passed to the two places that need it. It doesn't seem right to me to call clock_get_osc_freq() again in the lower level function just to avoid a parameter. On ARM at least a few parameters are a cheap way of passing data around. It also allows the lower-level functions to deal with what they need to, rather than all functions having to reference the global state independently, each one digging down to what it actually needs. Regards, Simon
Hello Simon, Am Dienstag, den 30.10.2012, 06:03 -0700 schrieb Simon Glass: > Hi Lucas, > > On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote: > > There is no need to pass around all those parameters. The init functions > > are able to easily extract all the needed setup info on their own. > > > > Signed-off-by: Lucas Stach <dev@lynxeye.de> > > --- > > arch/arm/cpu/armv7/tegra20/usb.c | 24 ++++++++++++------------ > > 1 Datei geändert, 12 Zeilen hinzugefügt(+), 12 Zeilen entfernt(-) > > I'm not sure I agree with the premise of this patch. > > At the top level it calls clock_get_osc_freq() to get the frequency. > That is then passed to the two places that need it. > > It doesn't seem right to me to call clock_get_osc_freq() again in the > lower level function just to avoid a parameter. On ARM at least a few > parameters are a cheap way of passing data around. > The intent of this patch is not really to save up on parameters passed, but to make it possible to later move out the controller initialization into the ehci_hcd_init function without having to save away this global state for later use. We have to init at most 2 controllers where timing matters, so I think it's the right thing to get the SoC global clock state at those two occasions to avoid inflating the file global state. > It also allows the lower-level functions to deal with what they need > to, rather than all functions having to reference the global state > independently, each one digging down to what it actually needs. > The controller init functions get passed the state only of the one port they have to initialize. There is no point in extracting things at an upper level and passing it into the functions, if it's exactly the same thing that is stored in the port state. Regards, Lucas
Hi Lucas, On Tue, Oct 30, 2012 at 6:16 AM, Lucas Stach <dev@lynxeye.de> wrote: > Hello Simon, > > Am Dienstag, den 30.10.2012, 06:03 -0700 schrieb Simon Glass: >> Hi Lucas, >> >> On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote: >> > There is no need to pass around all those parameters. The init functions >> > are able to easily extract all the needed setup info on their own. >> > >> > Signed-off-by: Lucas Stach <dev@lynxeye.de> >> > --- >> > arch/arm/cpu/armv7/tegra20/usb.c | 24 ++++++++++++------------ >> > 1 Datei geändert, 12 Zeilen hinzugefügt(+), 12 Zeilen entfernt(-) >> >> I'm not sure I agree with the premise of this patch. >> >> At the top level it calls clock_get_osc_freq() to get the frequency. >> That is then passed to the two places that need it. >> >> It doesn't seem right to me to call clock_get_osc_freq() again in the >> lower level function just to avoid a parameter. On ARM at least a few >> parameters are a cheap way of passing data around. >> > The intent of this patch is not really to save up on parameters passed, > but to make it possible to later move out the controller initialization > into the ehci_hcd_init function without having to save away this global > state for later use. > > We have to init at most 2 controllers where timing matters, so I think > it's the right thing to get the SoC global clock state at those two > occasions to avoid inflating the file global state. OK fair enough, I see that you want to do these two types of init at different times. > >> It also allows the lower-level functions to deal with what they need >> to, rather than all functions having to reference the global state >> independently, each one digging down to what it actually needs. >> > The controller init functions get passed the state only of the one port > they have to initialize. There is no point in extracting things at an > upper level and passing it into the functions, if it's exactly the same > thing that is stored in the port state. Well if the upper level is extracting it anyway, it often saves code space to pass the extracted value. I would like to avoid this sort of thing: void do_something(struct level1 *level1) { struct level2 *level2 = level1->level2; struct level3 *level3 = level2->level3; level3->... level3->... } void do_something_else(struct level1 *level1) { struct level2 *level2 = level1->level2; struct level3 *level3 = level2->level3; level3->... level3->... } main() { do_something(level1) do_something_else(level1) } I generally prefer: void do_something(struct level3 *level3) { level3->... level3->... } void do_something_else(struct level3 *level3) { level3->... level3->... } main() { struct level2 *level2 = level1->level2; struct level3 *level3 = level2->level3; do_something(level3) do_something_else(level3) } I hope that example makes sense. > > Regards, > Lucas > > Regards, Simon
diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c index 9fd1edc..1725cd1 100644 --- a/arch/arm/cpu/armv7/tegra20/usb.c +++ b/arch/arm/cpu/armv7/tegra20/usb.c @@ -196,11 +196,12 @@ void usbf_reset_controller(struct fdt_usb *config, struct usb_ctlr *usbctlr) } /* set up the UTMI USB controller with the parameters provided */ -static int init_utmi_usb_controller(struct fdt_usb *config, - struct usb_ctlr *usbctlr, const u32 timing[]) +static int init_utmi_usb_controller(struct fdt_usb *config) { u32 val; int loop_count; + u32 *timing; + struct usb_ctlr *usbctlr = config->reg; clock_enable(config->periph_id); @@ -227,6 +228,8 @@ static int init_utmi_usb_controller(struct fdt_usb *config, * PLL Delay CONFIGURATION settings. The following parameters control * the bring up of the plls. */ + timing = usb_pll[clock_get_osc_freq()]; + val = readl(&usbctlr->utmip_misc_cfg1); clrsetbits_le32(&val, UTMIP_PLLU_STABLE_COUNT_MASK, timing[PARAM_STABLE_COUNT] << UTMIP_PLLU_STABLE_COUNT_SHIFT); @@ -329,12 +332,12 @@ static int init_utmi_usb_controller(struct fdt_usb *config, #endif /* set up the ULPI USB controller with the parameters provided */ -static int init_ulpi_usb_controller(struct fdt_usb *config, - struct usb_ctlr *usbctlr) +static int init_ulpi_usb_controller(struct fdt_usb *config) { u32 val; int loop_count; struct ulpi_viewport ulpi_vp; + struct usb_ctlr *usbctlr = config->reg; /* set up ULPI reference clock on pllp_out4 */ clock_enable(PERIPH_ID_DEV2_OUT); @@ -406,8 +409,7 @@ static int init_ulpi_usb_controller(struct fdt_usb *config, return 0; } #else -static int init_ulpi_usb_controller(struct fdt_usb *config, - struct usb_ctlr *usbctlr) +static int init_ulpi_usb_controller(struct fdt_usb *config) { printf("No code to set up ULPI controller, please enable" "CONFIG_USB_ULPI and CONFIG_USB_ULPI_VIEWPORT"); @@ -428,22 +430,20 @@ static void config_clock(const u32 timing[]) * @param config USB port configuration * @return 0 if ok, -1 if error (too many ports) */ -static int add_port(struct fdt_usb *config, const u32 timing[]) +static int add_port(struct fdt_usb *config) { - struct usb_ctlr *usbctlr = config->reg; - if (port_count == USB_PORTS_MAX) { printf("tegrausb: Cannot register more than %d ports\n", USB_PORTS_MAX); return -1; } - if (config->utmi && init_utmi_usb_controller(config, usbctlr, timing)) { + if (config->utmi && init_utmi_usb_controller(config)) { printf("tegrausb: Cannot init port\n"); return -1; } - if (config->ulpi && init_ulpi_usb_controller(config, usbctlr)) { + if (config->ulpi && init_ulpi_usb_controller(config)) { printf("tegrausb: Cannot init port\n"); return -1; } @@ -556,7 +556,7 @@ int board_usb_init(const void *blob) return -1; } - if (add_port(&config, usb_pll[freq])) + if (add_port(&config)) return -1; set_host_mode(&config); }
There is no need to pass around all those parameters. The init functions are able to easily extract all the needed setup info on their own. Signed-off-by: Lucas Stach <dev@lynxeye.de> --- arch/arm/cpu/armv7/tegra20/usb.c | 24 ++++++++++++------------ 1 Datei geändert, 12 Zeilen hinzugefügt(+), 12 Zeilen entfernt(-)