| Message ID | 1354174439-5589-6-git-send-email-panto@antoniou-consulting.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
Hi Pantelis, > USB initialization shouldn't happen for all the boards. > The board_usb_init() follows u-boot policy, that SoC IPs (USB) are enabled and configured just before their usage. > Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com> > --- > common/cmd_dfu.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c > index 01d6b3a..327c738 100644 > --- a/common/cmd_dfu.c > +++ b/common/cmd_dfu.c > @@ -55,7 +55,10 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int > argc, char * const argv[]) goto done; > } > > +#ifdef CONFIG_TRATS > board_usb_init(); > +#endif > + In mine opinion this #ifdef shall be removed and each target board using the DFU shall define board_usb_init() at board file. > g_dnl_register(s); > while (1) { > if (ctrlc())
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 11/28/12 09:47, Lukasz Majewski wrote: > Hi Pantelis, > >> USB initialization shouldn't happen for all the boards. >> > > The board_usb_init() follows u-boot policy, that SoC IPs (USB) are > enabled and configured just before their usage. > > >> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com> >> --- common/cmd_dfu.c | 3 +++ 1 file changed, 3 insertions(+) >> >> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c index >> 01d6b3a..327c738 100644 --- a/common/cmd_dfu.c +++ >> b/common/cmd_dfu.c @@ -55,7 +55,10 @@ static int do_dfu(cmd_tbl_t >> *cmdtp, int flag, int argc, char * const argv[]) goto done; } >> >> +#ifdef CONFIG_TRATS board_usb_init(); +#endif + > In mine opinion this #ifdef shall be removed and each target board > using the DFU shall define board_usb_init() at board file. > But this isn't a called-only-once place. What are you really doing here and are you sure it's needed every time DFU is called? - -- Tom -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ iQIcBAEBAgAGBQJQtjM2AAoJENk4IS6UOR1WOu8P/3253rY48k6+NgCefiNZf6GH Sw9ZEh7fNkC3QgSkKA8/Ifa52F455UFpslftjSVHIrGIwIVc+3TCM2lOdbaBZgMi bL0MsPKRRbujx6U69lY5A6eaFyrhPJJJcCryFoPkfzsYSuvazol/crKCs9BB24Mk j35nvd2juxmhh4paQ9+7UVkxI50haLPVBHU7A5v8yv3i9/Cig+qwqewt+GWvIhoE w6frRy4WyczTClWqF+KwlfT4bwJVtnHxzfl5d2qRn4C/McTzUpwVePT8xrGaS4zc 3p+VCQ269Po176sgwoud5EwJdvCNBVfFeHaTORW8UJ2zLzU4iDixx4VY9SQhTfHF MP7Ch5p2DIJRrlEUWuRAgQwO6pICBHD+f3jw5q06hg35JWmTnltyq53M5UILGyi/ Vz+SN0xF4YnMJRvKGT9lal0OiRxr/rO63k6fl2XybEfTED6AHhvUJKBV+yb1OxV0 CTCiBRqfKQwkProdFtAAJT6+CeexV4Im2WcHQwqcKxVNqgEQhM6MBsM3DkjfE+nj naz8ITF6Lal1C0K5dUbSPiY8KqgphXre11wJ28BFp5WSM/p/0wCrhImXxuOMeUd3 QtWT/BT2fJfKcr2bGVLTKdHVBHLWziJQK6IjO8rGnot2QbdSh+QS8n9ZrDf9rJYS +Ha0VT7jIVP/6uF1FXdj =puWb -----END PGP SIGNATURE-----
Hi Tom, > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 11/28/12 09:47, Lukasz Majewski wrote: > > Hi Pantelis, > > > >> USB initialization shouldn't happen for all the boards. > >> > > > > The board_usb_init() follows u-boot policy, that SoC IPs (USB) are > > enabled and configured just before their usage. > > > > > >> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com> > >> --- common/cmd_dfu.c | 3 +++ 1 file changed, 3 insertions(+) > >> > >> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c index > >> 01d6b3a..327c738 100644 --- a/common/cmd_dfu.c +++ > >> b/common/cmd_dfu.c @@ -55,7 +55,10 @@ static int do_dfu(cmd_tbl_t > >> *cmdtp, int flag, int argc, char * const argv[]) goto done; } > >> > >> +#ifdef CONFIG_TRATS board_usb_init(); +#endif + > > In mine opinion this #ifdef shall be removed and each target board > > using the DFU shall define board_usb_init() at board file. > > > > But this isn't a called-only-once place. What are you really doing > here and are you sure it's needed every time DFU is called? > Hmm, you are correct here. But I don't have a good alternative for this. One solution would be to define a static flag for it at do_dfu function to indicate if this was executed once (however I'm reluctant do this). Any ideas? > - -- > Tom > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.11 (GNU/Linux) > Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ > > iQIcBAEBAgAGBQJQtjM2AAoJENk4IS6UOR1WOu8P/3253rY48k6+NgCefiNZf6GH > Sw9ZEh7fNkC3QgSkKA8/Ifa52F455UFpslftjSVHIrGIwIVc+3TCM2lOdbaBZgMi > bL0MsPKRRbujx6U69lY5A6eaFyrhPJJJcCryFoPkfzsYSuvazol/crKCs9BB24Mk > j35nvd2juxmhh4paQ9+7UVkxI50haLPVBHU7A5v8yv3i9/Cig+qwqewt+GWvIhoE > w6frRy4WyczTClWqF+KwlfT4bwJVtnHxzfl5d2qRn4C/McTzUpwVePT8xrGaS4zc > 3p+VCQ269Po176sgwoud5EwJdvCNBVfFeHaTORW8UJ2zLzU4iDixx4VY9SQhTfHF > MP7Ch5p2DIJRrlEUWuRAgQwO6pICBHD+f3jw5q06hg35JWmTnltyq53M5UILGyi/ > Vz+SN0xF4YnMJRvKGT9lal0OiRxr/rO63k6fl2XybEfTED6AHhvUJKBV+yb1OxV0 > CTCiBRqfKQwkProdFtAAJT6+CeexV4Im2WcHQwqcKxVNqgEQhM6MBsM3DkjfE+nj > naz8ITF6Lal1C0K5dUbSPiY8KqgphXre11wJ28BFp5WSM/p/0wCrhImXxuOMeUd3 > QtWT/BT2fJfKcr2bGVLTKdHVBHLWziJQK6IjO8rGnot2QbdSh+QS8n9ZrDf9rJYS > +Ha0VT7jIVP/6uF1FXdj > =puWb > -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 11/28/12 11:08, Lukasz Majewski wrote: > Hi Tom, > > On 11/28/12 09:47, Lukasz Majewski wrote: >>>> Hi Pantelis, >>>> >>>>> USB initialization shouldn't happen for all the boards. >>>>> >>>> >>>> The board_usb_init() follows u-boot policy, that SoC IPs >>>> (USB) are enabled and configured just before their usage. >>>> >>>> >>>>> Signed-off-by: Pantelis Antoniou >>>>> <panto@antoniou-consulting.com> --- common/cmd_dfu.c | 3 >>>>> +++ 1 file changed, 3 insertions(+) >>>>> >>>>> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c index >>>>> 01d6b3a..327c738 100644 --- a/common/cmd_dfu.c +++ >>>>> b/common/cmd_dfu.c @@ -55,7 +55,10 @@ static int >>>>> do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const >>>>> argv[]) goto done; } >>>>> >>>>> +#ifdef CONFIG_TRATS board_usb_init(); +#endif + >>>> In mine opinion this #ifdef shall be removed and each target >>>> board using the DFU shall define board_usb_init() at board >>>> file. >>>> >> >> But this isn't a called-only-once place. What are you really >> doing here and are you sure it's needed every time DFU is >> called? >> > > Hmm, you are correct here. > > But I don't have a good alternative for this. > > One solution would be to define a static flag for it at do_dfu > function to indicate if this was executed once (however I'm > reluctant do this). > > > Any ideas? I think the answer, and it's what we do on am335x is that arch_misc_init() is what calls the equiv of s3c_udc_probe(...) under the logic of "if we are built with usb gadget support, we want to use it, so init it". - -- Tom -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ iQIcBAEBAgAGBQJQt56WAAoJENk4IS6UOR1WznYP/23g0QuMB2slIC41OLTeGKfh 11zybSEVZYZmSPfgjEsXqEWh1cYryQNyiNyKIzNfPPyH/ZAA2PuMH7mKMmdp5St6 p7IIhmFwO+phkLGgpLVSJ6PsCGfY68N1r1FU04JJhpteoNmSPtutBWrb2bJ8tib/ 5HHSjUEUSYIgE1OHHVouGUx4KzNwWgyr0nds9WyfJ/X9OnQ22WRuVlkOIpy74NCz r9QSIEOSbmqY6uU+YFFOorgp0Ox97okRJAH0KAsBNxq6PE2NmZard0Qg2m2Ism7L NFbBvlfeF+/m9cicnrnuygyVkkNRcsX5NjWzVilzXQCfYmwBSH2YKPZbpRb3XGmr wNSNqbfSEWG3Oxa+g0NnqI8SPqsTNVXR8X1QsF/f7zIOHlYZfXlbqsDEzITm1YoI S1OEmpYXQQI1kZEOaxfXyJYbMXnA1/y8uItX8Bl/JUMWDQqQMFeJMVS711khGYuR EUVL8YQam6N7Xgzk89sN8UPyOfAbxxOgB5fNyKeuSL+sz0vBaAkmv69gNsdPsfIr vFvfyUKwyMtqhWZO+cG0VU4jzI0S0SMHdh52GtrU6P/3r77MC6zrhVja2EylXqvD p8pSi7eEdeBUMbJ6uMgLd0kxYwh3NWy5NTTR10yKDyTXi8kh/grG89syI5Eiczwj /CW6UuwG8R7T2l2+d1X3 =mdL4 -----END PGP SIGNATURE-----
Hi Tom, > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 11/28/12 11:08, Lukasz Majewski wrote: > > Hi Tom, > > > > On 11/28/12 09:47, Lukasz Majewski wrote: > >>>> Hi Pantelis, > >>>> > >>>>> USB initialization shouldn't happen for all the boards. > >>>>> > >>>> > >>>> The board_usb_init() follows u-boot policy, that SoC IPs > >>>> (USB) are enabled and configured just before their usage. > >>>> > >>>> > >>>>> Signed-off-by: Pantelis Antoniou > >>>>> <panto@antoniou-consulting.com> --- common/cmd_dfu.c | 3 > >>>>> +++ 1 file changed, 3 insertions(+) > >>>>> > >>>>> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c index > >>>>> 01d6b3a..327c738 100644 --- a/common/cmd_dfu.c +++ > >>>>> b/common/cmd_dfu.c @@ -55,7 +55,10 @@ static int > >>>>> do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const > >>>>> argv[]) goto done; } > >>>>> > >>>>> +#ifdef CONFIG_TRATS board_usb_init(); +#endif + > >>>> In mine opinion this #ifdef shall be removed and each target > >>>> board using the DFU shall define board_usb_init() at board > >>>> file. > >>>> > >> > >> But this isn't a called-only-once place. What are you really > >> doing here and are you sure it's needed every time DFU is > >> called? > >> > > > > Hmm, you are correct here. > > > > But I don't have a good alternative for this. > > > > One solution would be to define a static flag for it at do_dfu > > function to indicate if this was executed once (however I'm > > reluctant do this). > > > > > > Any ideas? > > I think the answer, and it's what we do on am335x is that > arch_misc_init() is what calls the equiv of s3c_udc_probe(...) under > the logic of "if we are built with usb gadget support, we want to use > it, so init it". I've understood the policy differently: "We are build with gadget support and we _might_ use it, so enable low level code only when (or just before) we use it". What's about the power consumption? Why IP block which will be used from time to time shall be enabled and operational? > > - -- > Tom > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.11 (GNU/Linux) > Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ > > iQIcBAEBAgAGBQJQt56WAAoJENk4IS6UOR1WznYP/23g0QuMB2slIC41OLTeGKfh > 11zybSEVZYZmSPfgjEsXqEWh1cYryQNyiNyKIzNfPPyH/ZAA2PuMH7mKMmdp5St6 > p7IIhmFwO+phkLGgpLVSJ6PsCGfY68N1r1FU04JJhpteoNmSPtutBWrb2bJ8tib/ > 5HHSjUEUSYIgE1OHHVouGUx4KzNwWgyr0nds9WyfJ/X9OnQ22WRuVlkOIpy74NCz > r9QSIEOSbmqY6uU+YFFOorgp0Ox97okRJAH0KAsBNxq6PE2NmZard0Qg2m2Ism7L > NFbBvlfeF+/m9cicnrnuygyVkkNRcsX5NjWzVilzXQCfYmwBSH2YKPZbpRb3XGmr > wNSNqbfSEWG3Oxa+g0NnqI8SPqsTNVXR8X1QsF/f7zIOHlYZfXlbqsDEzITm1YoI > S1OEmpYXQQI1kZEOaxfXyJYbMXnA1/y8uItX8Bl/JUMWDQqQMFeJMVS711khGYuR > EUVL8YQam6N7Xgzk89sN8UPyOfAbxxOgB5fNyKeuSL+sz0vBaAkmv69gNsdPsfIr > vFvfyUKwyMtqhWZO+cG0VU4jzI0S0SMHdh52GtrU6P/3r77MC6zrhVja2EylXqvD > p8pSi7eEdeBUMbJ6uMgLd0kxYwh3NWy5NTTR10yKDyTXi8kh/grG89syI5Eiczwj > /CW6UuwG8R7T2l2+d1X3 > =mdL4 > -----END PGP SIGNATURE----- > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 11/29/12 18:14, Lukasz Majewski wrote: > Hi Tom, > > On 11/28/12 11:08, Lukasz Majewski wrote: >>>> Hi Tom, >>>> >>>> On 11/28/12 09:47, Lukasz Majewski wrote: >>>>>>> Hi Pantelis, >>>>>>> >>>>>>>> USB initialization shouldn't happen for all the >>>>>>>> boards. >>>>>>>> >>>>>>> >>>>>>> The board_usb_init() follows u-boot policy, that SoC >>>>>>> IPs (USB) are enabled and configured just before their >>>>>>> usage. >>>>>>> >>>>>>> >>>>>>>> Signed-off-by: Pantelis Antoniou >>>>>>>> <panto@antoniou-consulting.com> --- common/cmd_dfu.c >>>>>>>> | 3 +++ 1 file changed, 3 insertions(+) >>>>>>>> >>>>>>>> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c >>>>>>>> index 01d6b3a..327c738 100644 --- a/common/cmd_dfu.c >>>>>>>> +++ b/common/cmd_dfu.c @@ -55,7 +55,10 @@ static >>>>>>>> int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char >>>>>>>> * const argv[]) goto done; } >>>>>>>> >>>>>>>> +#ifdef CONFIG_TRATS board_usb_init(); +#endif + >>>>>>> In mine opinion this #ifdef shall be removed and each >>>>>>> target board using the DFU shall define >>>>>>> board_usb_init() at board file. >>>>>>> >>>>> >>>>> But this isn't a called-only-once place. What are you >>>>> really doing here and are you sure it's needed every time >>>>> DFU is called? >>>>> >>>> >>>> Hmm, you are correct here. >>>> >>>> But I don't have a good alternative for this. >>>> >>>> One solution would be to define a static flag for it at >>>> do_dfu function to indicate if this was executed once >>>> (however I'm reluctant do this). >>>> >>>> >>>> Any ideas? > > I think the answer, and it's what we do on am335x is that > arch_misc_init() is what calls the equiv of s3c_udc_probe(...) > under the logic of "if we are built with usb gadget support, we > want to use it, so init it". > >> I've understood the policy differently: > >> "We are build with gadget support and we _might_ use it, so >> enable low level code only when (or just before) we use it". > >> What's about the power consumption? Why IP block which will be >> used from time to time shall be enabled and operational? Frankly, I think this shows we don't have a good setup right now, in general. Saying the gadget needs to whack and re-whack the device into on state (but isn't really turning it off once done) isn't ideal. There are also indeed drawbacks to saying "gadget support enabled at boot-time, let me set it up". Right now, I don't really have a good answer. It also needs to cover things like gadget ethernet where we don't really want to disappear immediately after each command. - -- Tom -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ iQIcBAEBAgAGBQJQuBJzAAoJENk4IS6UOR1WWncP/29E2kP1ErctSmBPUkJZASEk qmW+rYCwxpz3aesC6Dcnc0Urad+MwO9SPbzUFfQcB4CMVXXAXMcahH+4FY8FzTnV p8QqQjZ0o54xYxEI/evkaEWsTyM3nBBXtMq9II2gwYPMxP88i2JwL1NMOF6o2z+Y VVNyGO1685JIGU+WMzpoXp4WYjfd/8qv8JHhQ+moGoFe0p7p5c5kokYsbXdcxvyk rkeJrog8SV+vJQLnu0nUh8Q/pBYftiMDD+U8upQOiYNF1CPgGugX33XJydtYMoA8 rv6an/xh4PXUSJkAJbwp8vrxQqrHD0MYWtZEPwTEToFmTxvvaS2zJ0uOnNLkjanv dp8RrD38HIczsgMtAVXR9nhFsW89MB2EiXc7N/EoO0BA4E0pac1/+OUfUMwELj16 IKJIHa4BXkHfg/ybQXCxr+GDZEc8YwLGXxzyIw8cS3PffNSkR1fkJcjZwFg190y0 1IjmlF0Dhg0CGweej0WTFuzi3OmHQOvMJcvT0doiI35iVbFvGwhjNUt8dJeDr0Yr xkhWPhR3+iMXmzle+me+ZUxUe5g2vGqPC6HeGUO/fo0ntqZfITjzznMHKpN8Uxwv D6Njg2e785ZIgTxcMXKn/Q2xtPBF78/stWeStyIwIAxYOEDw/C+NipWAHEpJdGTp k8mhP+GKWgatyZ5yqDBA =01FA -----END PGP SIGNATURE-----
On Wed, Nov 28, 2012 at 03:47:43PM +0100, Lukasz Majewski wrote: > Hi Pantelis, > > > USB initialization shouldn't happen for all the boards. > > > > The board_usb_init() follows u-boot policy, that SoC IPs (USB) are > enabled and configured just before their usage. Going back to this old thread as I dig at things again. How much of s3c_udc_probe is actually enabling hardware as opposed to registering that such hardware exists and can be enabled when we call usb_gadget_register_driver ? It looks like all of the clocking (and pin muxing?) is already being done.
Hi Tom, > On Wed, Nov 28, 2012 at 03:47:43PM +0100, Lukasz Majewski wrote: > > > Hi Pantelis, > > > > > USB initialization shouldn't happen for all the boards. > > > > > > > The board_usb_init() follows u-boot policy, that SoC IPs (USB) are > > enabled and configured just before their usage. > > Going back to this old thread as I dig at things again. How much of > s3c_udc_probe is actually enabling hardware as opposed to registering > that such hardware exists and can be enabled when we call > usb_gadget_register_driver ? It looks like all of the clocking (and > pin muxing?) is already being done. > I will check if s3c_udc_probe can be removed totally and replaced by usb_gadget_register_driver (since some time has passed from original UDC driver posting).
On Mon, Dec 17, 2012 at 12:37 PM, Lukasz Majewski <l.majewski@samsung.com> wrote: > Hi Tom, > >> On Wed, Nov 28, 2012 at 03:47:43PM +0100, Lukasz Majewski wrote: >> >> > Hi Pantelis, >> > >> > > USB initialization shouldn't happen for all the boards. >> > > >> > >> > The board_usb_init() follows u-boot policy, that SoC IPs (USB) are >> > enabled and configured just before their usage. >> >> Going back to this old thread as I dig at things again. How much of >> s3c_udc_probe is actually enabling hardware as opposed to registering >> that such hardware exists and can be enabled when we call >> usb_gadget_register_driver ? It looks like all of the clocking (and >> pin muxing?) is already being done. >> > > I will check if s3c_udc_probe can be removed totally and replaced by > usb_gadget_register_driver (since some time has passed from original > UDC driver posting). Well, not so much replace but just, if there's no actual HW init, move the "probe" (which isn't a probe, it's registering board infos) call into arch_misc_init or similar. -- Tom
diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c index 01d6b3a..327c738 100644 --- a/common/cmd_dfu.c +++ b/common/cmd_dfu.c @@ -55,7 +55,10 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) goto done; } +#ifdef CONFIG_TRATS board_usb_init(); +#endif + g_dnl_register(s); while (1) { if (ctrlc())
USB initialization shouldn't happen for all the boards. Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com> --- common/cmd_dfu.c | 3 +++ 1 file changed, 3 insertions(+)