[U-Boot] usb: ehci: Take advantage of the new multi-controller feature for MXC
Hi Marek,
Thanks to Lucas' series coming with commits c7e3b2b and 676ae06, I'd like to use the multi-controller feature on MXC since most of these SoCs come with a USB IP supporting an OTG controller and multiple host-only controllers.
Currently the MXC code in ehci-mx{c|5|6}.c just ignores the index passed to ehci_hcd_init() and the like, and there are 3 port-specific configs (CONFIG_MXC_USB_PORT, CONFIG_MXC_USB_FLAGS and CONFIG_MXC_USB_PORTSC).
Not all USB ports from the USB IP will be available on each board for a given SoC, so we need a logical to physical USB port mapping.
I would suggest something like the following.
board.h: #define CONFIG_MXC_USB { \ { \ 0, \ MXC_EHCI_INTERNAL_PHY, \ MXC_EHCI_UTMI_16BIT | MXC_EHCI_MODE_UTMI \ }, { \ 1, \ MXC_EHCI_POWER_PINS_ENABLED | MXC_EHCI_PWR_PIN_ACTIVE_HIGH | \ MXC_EHCI_OC_PIN_ACTIVE_LOW, \ MXC_EHCI_MODE_ULPI \ }, \ }
ehci-fsl.h: struct mxc_ehci_cfg { int port; u32 flags; u32 portsc; };
ehci-mx{c|5|6}.c: static const struct mxc_ehci_cfg cfg[CONFIG_USB_MAX_CONTROLLER_COUNT] = CONFIG_MXC_USB;
Then, in ehci_hcd_init(), there would be the following replacements: - CONFIG_MXC_USB_PORT -> cfg[index].port, - CONFIG_MXC_USB_FLAGS -> cfg[index].flags, - CONFIG_MXC_USB_PORTSC -> cfg[index].portsc.
What do you think?
Best regards, Benoît
Dear Benoît Thébaudeau,
Hi Marek,
Thanks to Lucas' series coming with commits c7e3b2b and 676ae06, I'd like to use the multi-controller feature on MXC since most of these SoCs come with a USB IP supporting an OTG controller and multiple host-only controllers.
Currently the MXC code in ehci-mx{c|5|6}.c just ignores the index passed to ehci_hcd_init() and the like, and there are 3 port-specific configs (CONFIG_MXC_USB_PORT, CONFIG_MXC_USB_FLAGS and CONFIG_MXC_USB_PORTSC).
Not all USB ports from the USB IP will be available on each board for a given SoC, so we need a logical to physical USB port mapping.
I would suggest something like the following.
board.h: #define CONFIG_MXC_USB { \ { \ 0, \ MXC_EHCI_INTERNAL_PHY, \ MXC_EHCI_UTMI_16BIT | MXC_EHCI_MODE_UTMI \ }, { \ 1, \ MXC_EHCI_POWER_PINS_ENABLED | MXC_EHCI_PWR_PIN_ACTIVE_HIGH | \ MXC_EHCI_OC_PIN_ACTIVE_LOW, \ MXC_EHCI_MODE_ULPI \ }, \ }
ehci-fsl.h: struct mxc_ehci_cfg { int port; u32 flags; u32 portsc; };
ehci-mx{c|5|6}.c: static const struct mxc_ehci_cfg cfg[CONFIG_USB_MAX_CONTROLLER_COUNT] = CONFIG_MXC_USB;
Then, in ehci_hcd_init(), there would be the following replacements:
- CONFIG_MXC_USB_PORT -> cfg[index].port,
- CONFIG_MXC_USB_FLAGS -> cfg[index].flags,
- CONFIG_MXC_USB_PORTSC -> cfg[index].portsc.
What do you think?
What about passing port private / platform data instead of ID ?
Best regards, Marek Vasut
Dear Marek Vasut,
On Monday, November 5, 2012 11:54:12 PM, Marek Vasut wrote:
Dear Benoît Thébaudeau,
Hi Marek,
Thanks to Lucas' series coming with commits c7e3b2b and 676ae06, I'd like to use the multi-controller feature on MXC since most of these SoCs come with a USB IP supporting an OTG controller and multiple host-only controllers.
Currently the MXC code in ehci-mx{c|5|6}.c just ignores the index passed to ehci_hcd_init() and the like, and there are 3 port-specific configs (CONFIG_MXC_USB_PORT, CONFIG_MXC_USB_FLAGS and CONFIG_MXC_USB_PORTSC).
Not all USB ports from the USB IP will be available on each board for a given SoC, so we need a logical to physical USB port mapping.
I would suggest something like the following.
board.h: #define CONFIG_MXC_USB { \ { \ 0, \ MXC_EHCI_INTERNAL_PHY, \ MXC_EHCI_UTMI_16BIT | MXC_EHCI_MODE_UTMI \ }, { \ 1, \ MXC_EHCI_POWER_PINS_ENABLED | MXC_EHCI_PWR_PIN_ACTIVE_HIGH | \ MXC_EHCI_OC_PIN_ACTIVE_LOW, \ MXC_EHCI_MODE_ULPI \ }, \ }
ehci-fsl.h: struct mxc_ehci_cfg { int port; u32 flags; u32 portsc; };
ehci-mx{c|5|6}.c: static const struct mxc_ehci_cfg cfg[CONFIG_USB_MAX_CONTROLLER_COUNT] = CONFIG_MXC_USB;
Then, in ehci_hcd_init(), there would be the following replacements:
- CONFIG_MXC_USB_PORT -> cfg[index].port,
- CONFIG_MXC_USB_FLAGS -> cfg[index].flags,
- CONFIG_MXC_USB_PORTSC -> cfg[index].portsc.
What do you think?
What about passing port private / platform data instead of ID ?
The ID is already passed to ehci_hcd_init(), so we have to live with it if we don't want to change the newly introduced multi-controller infrastructure.
Or, perhaps this is what you meant, we could have some: int ehci_mxc_register(int index, const struct mxc_ehci_cfg *cfg); This function would simply fill an entry in the cfg array in ehci-mx{c|5|6}.c, this array becoming an array of pointers to struct mxc_ehci_cfg. This looks nicer, but it needs more code to do just the same thing as the CONFIG_MXC_USB would do, without adding any feature. The only benefit would be if index were actually the same as port here, but ehci_hcd_init() would still be called for all indexes, so it would have to fail e.g. if port 0 is unused but port 1 is used, which would probably generate some error noise for the user.
Or did you mean something else?
Best regards, Benoît
Dear Benoît Thébaudeau,
Dear Marek Vasut,
On Monday, November 5, 2012 11:54:12 PM, Marek Vasut wrote:
Dear Benoît Thébaudeau,
Hi Marek,
Thanks to Lucas' series coming with commits c7e3b2b and 676ae06, I'd like to use the multi-controller feature on MXC since most of these SoCs come with a USB IP supporting an OTG controller and multiple host-only controllers.
Currently the MXC code in ehci-mx{c|5|6}.c just ignores the index passed to ehci_hcd_init() and the like, and there are 3 port-specific configs (CONFIG_MXC_USB_PORT, CONFIG_MXC_USB_FLAGS and CONFIG_MXC_USB_PORTSC).
Not all USB ports from the USB IP will be available on each board for a given SoC, so we need a logical to physical USB port mapping.
I would suggest something like the following.
board.h: #define CONFIG_MXC_USB { \
{ \
0, \ MXC_EHCI_INTERNAL_PHY, \ MXC_EHCI_UTMI_16BIT | MXC_EHCI_MODE_UTMI \}, { \
1, \ MXC_EHCI_POWER_PINS_ENABLED | MXC_EHCI_PWR_PIN_ACTIVE_HIGH | \ MXC_EHCI_OC_PIN_ACTIVE_LOW, \ MXC_EHCI_MODE_ULPI \}, \
}
ehci-fsl.h: struct mxc_ehci_cfg {
int port; u32 flags; u32 portsc;
};
ehci-mx{c|5|6}.c: static const struct mxc_ehci_cfg cfg[CONFIG_USB_MAX_CONTROLLER_COUNT] =
CONFIG_MXC_USB;Then, in ehci_hcd_init(), there would be the following
replacements:
- CONFIG_MXC_USB_PORT -> cfg[index].port,
- CONFIG_MXC_USB_FLAGS -> cfg[index].flags,
- CONFIG_MXC_USB_PORTSC -> cfg[index].portsc.
What do you think?
What about passing port private / platform data instead of ID ?
The ID is already passed to ehci_hcd_init(), so we have to live with it if we don't want to change the newly introduced multi-controller infrastructure.
Let's change it .... remove the ID and pass some generic pdata.
Or, perhaps this is what you meant, we could have some: int ehci_mxc_register(int index, const struct mxc_ehci_cfg *cfg); This function would simply fill an entry in the cfg array in ehci-mx{c|5|6}.c, this array becoming an array of pointers to struct mxc_ehci_cfg. This looks nicer, but it needs more code to do just the same thing as the CONFIG_MXC_USB would do, without adding any feature. The only benefit would be if index were actually the same as port here, but ehci_hcd_init() would still be called for all indexes, so it would have to fail e.g. if port 0 is unused but port 1 is used, which would probably generate some error noise for the user.
Or did you mean something else?
Best regards, Benoît
Am Dienstag, den 06.11.2012, 00:56 +0100 schrieb Marek Vasut:
Dear Benoît Thébaudeau,
Dear Marek Vasut,
On Monday, November 5, 2012 11:54:12 PM, Marek Vasut wrote:
Dear Benoît Thébaudeau,
Hi Marek,
Thanks to Lucas' series coming with commits c7e3b2b and 676ae06, I'd like to use the multi-controller feature on MXC since most of these SoCs come with a USB IP supporting an OTG controller and multiple host-only controllers.
Currently the MXC code in ehci-mx{c|5|6}.c just ignores the index passed to ehci_hcd_init() and the like, and there are 3 port-specific configs (CONFIG_MXC_USB_PORT, CONFIG_MXC_USB_FLAGS and CONFIG_MXC_USB_PORTSC).
Not all USB ports from the USB IP will be available on each board for a given SoC, so we need a logical to physical USB port mapping.
I would suggest something like the following.
board.h: #define CONFIG_MXC_USB { \
{ \
0, \ MXC_EHCI_INTERNAL_PHY, \ MXC_EHCI_UTMI_16BIT | MXC_EHCI_MODE_UTMI \}, { \
1, \ MXC_EHCI_POWER_PINS_ENABLED | MXC_EHCI_PWR_PIN_ACTIVE_HIGH | \ MXC_EHCI_OC_PIN_ACTIVE_LOW, \ MXC_EHCI_MODE_ULPI \}, \
}
ehci-fsl.h: struct mxc_ehci_cfg {
int port; u32 flags; u32 portsc;
};
ehci-mx{c|5|6}.c: static const struct mxc_ehci_cfg cfg[CONFIG_USB_MAX_CONTROLLER_COUNT] =
CONFIG_MXC_USB;Then, in ehci_hcd_init(), there would be the following
replacements:
- CONFIG_MXC_USB_PORT -> cfg[index].port,
- CONFIG_MXC_USB_FLAGS -> cfg[index].flags,
- CONFIG_MXC_USB_PORTSC -> cfg[index].portsc.
What do you think?
What about passing port private / platform data instead of ID ?
The ID is already passed to ehci_hcd_init(), so we have to live with it if we don't want to change the newly introduced multi-controller infrastructure.
Let's change it .... remove the ID and pass some generic pdata.
I don't like the idea of passing around data at this level. It's breaking the abstraction, as we have to pass low-level usb information around in the higher USB stack levels.
The USB driver code should be able to do the virt-to-phys controller mapping on it's own. In the Tegra world we use the information we get from device tree to do so, but I don't see a reason why your USB host driver code wouldn't be able to just require an array with configuration data from the board file.
There is really no need to pass this information through all the USB stack interfaces.
Regards, Lucas
Dear Lucas Stach,
On Tuesday, November 6, 2012 8:43:43 AM, Lucas Stach wrote:
Am Dienstag, den 06.11.2012, 00:56 +0100 schrieb Marek Vasut:
Dear Benoît Thébaudeau,
Dear Marek Vasut,
On Monday, November 5, 2012 11:54:12 PM, Marek Vasut wrote:
Dear Benoît Thébaudeau,
Hi Marek,
Thanks to Lucas' series coming with commits c7e3b2b and 676ae06, I'd like to use the multi-controller feature on MXC since most of these SoCs come with a USB IP supporting an OTG controller and multiple host-only controllers.
Currently the MXC code in ehci-mx{c|5|6}.c just ignores the index passed to ehci_hcd_init() and the like, and there are 3 port-specific configs (CONFIG_MXC_USB_PORT, CONFIG_MXC_USB_FLAGS and CONFIG_MXC_USB_PORTSC).
Not all USB ports from the USB IP will be available on each board for a given SoC, so we need a logical to physical USB port mapping.
I would suggest something like the following.
board.h: #define CONFIG_MXC_USB { \
{ \
0, \ MXC_EHCI_INTERNAL_PHY, \ MXC_EHCI_UTMI_16BIT | MXC_EHCI_MODE_UTMI \}, { \
1, \ MXC_EHCI_POWER_PINS_ENABLED | MXC_EHCI_PWR_PIN_ACTIVE_HIGH | \ MXC_EHCI_OC_PIN_ACTIVE_LOW, \ MXC_EHCI_MODE_ULPI \}, \
}
ehci-fsl.h: struct mxc_ehci_cfg {
int port; u32 flags; u32 portsc;
};
ehci-mx{c|5|6}.c: static const struct mxc_ehci_cfg cfg[CONFIG_USB_MAX_CONTROLLER_COUNT] =
CONFIG_MXC_USB;Then, in ehci_hcd_init(), there would be the following
replacements:
- CONFIG_MXC_USB_PORT -> cfg[index].port,
- CONFIG_MXC_USB_FLAGS -> cfg[index].flags,
- CONFIG_MXC_USB_PORTSC -> cfg[index].portsc.
What do you think?
What about passing port private / platform data instead of ID ?
The ID is already passed to ehci_hcd_init(), so we have to live with it if we don't want to change the newly introduced multi-controller infrastructure.
Let's change it .... remove the ID and pass some generic pdata.
I don't like the idea of passing around data at this level. It's breaking the abstraction, as we have to pass low-level usb information around in the higher USB stack levels.
The USB driver code should be able to do the virt-to-phys controller mapping on it's own. In the Tegra world we use the information we get from device tree to do so, but I don't see a reason why your USB host driver code wouldn't be able to just require an array with configuration data from the board file.
There is really no need to pass this information through all the USB stack interfaces.
I agree, all the more ehci_hcd_init() is called from cmd_usb.c, completely outside of any board init context, so collecting the platform data would be a real pain, without bringing much. And moving usb_init() calls to board init context would also not be good because of the added boot time.
IMHO, the best solutions here are either a CONFIG_MXC_USB as I suggested, or the same structure passed to some init function specific to these EHCI drivers (which would add more code for little benefit).
Marek? Stefano?
Best regards, Benoît
Dear Benoît Thébaudeau,
Dear Lucas Stach,
On Tuesday, November 6, 2012 8:43:43 AM, Lucas Stach wrote:
Am Dienstag, den 06.11.2012, 00:56 +0100 schrieb Marek Vasut:
Dear Benoît Thébaudeau,
Dear Marek Vasut,
On Monday, November 5, 2012 11:54:12 PM, Marek Vasut wrote:
Dear Benoît Thébaudeau,
Hi Marek,
Thanks to Lucas' series coming with commits c7e3b2b and 676ae06, I'd like to use the multi-controller feature on MXC since most of these SoCs come with a USB IP supporting an OTG controller and multiple host-only controllers.
Currently the MXC code in ehci-mx{c|5|6}.c just ignores the index passed to ehci_hcd_init() and the like, and there are 3 port-specific configs (CONFIG_MXC_USB_PORT, CONFIG_MXC_USB_FLAGS and CONFIG_MXC_USB_PORTSC).
Not all USB ports from the USB IP will be available on each board for a given SoC, so we need a logical to physical USB port mapping.
I would suggest something like the following.
board.h: #define CONFIG_MXC_USB { \
{ \
0, \ MXC_EHCI_INTERNAL_PHY, \ MXC_EHCI_UTMI_16BIT | MXC_EHCI_MODE_UTMI \}, { \
1, \ MXC_EHCI_POWER_PINS_ENABLED |
MXC_EHCI_PWR_PIN_ACTIVE_HIGH
| \ MXC_EHCI_OC_PIN_ACTIVE_LOW, \ MXC_EHCI_MODE_ULPI \
}, \
}
ehci-fsl.h: struct mxc_ehci_cfg {
int port; u32 flags; u32 portsc;
};
ehci-mx{c|5|6}.c: static const struct mxc_ehci_cfg cfg[CONFIG_USB_MAX_CONTROLLER_COUNT] =
CONFIG_MXC_USB;Then, in ehci_hcd_init(), there would be the following
replacements:
- CONFIG_MXC_USB_PORT -> cfg[index].port,
- CONFIG_MXC_USB_FLAGS -> cfg[index].flags,
- CONFIG_MXC_USB_PORTSC -> cfg[index].portsc.
What do you think?
What about passing port private / platform data instead of ID ?
The ID is already passed to ehci_hcd_init(), so we have to live with it if we don't want to change the newly introduced multi-controller infrastructure.
Let's change it .... remove the ID and pass some generic pdata.
I don't like the idea of passing around data at this level. It's breaking the abstraction, as we have to pass low-level usb information around in the higher USB stack levels.
The USB driver code should be able to do the virt-to-phys controller mapping on it's own. In the Tegra world we use the information we get from device tree to do so, but I don't see a reason why your USB host driver code wouldn't be able to just require an array with configuration data from the board file.
There is really no need to pass this information through all the USB stack interfaces.
I agree, all the more ehci_hcd_init() is called from cmd_usb.c, completely outside of any board init context, so collecting the platform data would be a real pain, without bringing much. And moving usb_init() calls to board init context would also not be good because of the added boot time.
IMHO, the best solutions here are either a CONFIG_MXC_USB as I suggested, or the same structure passed to some init function specific to these EHCI drivers (which would add more code for little benefit).
I disagree ... mapping function is fine, but I'd like to be able to pass around pointer to some platform data.
Dear Lucas Stach,
[...]
What do you think?
What about passing port private / platform data instead of ID ?
The ID is already passed to ehci_hcd_init(), so we have to live with it if we don't want to change the newly introduced multi-controller infrastructure.
Let's change it .... remove the ID and pass some generic pdata.
I don't like the idea of passing around data at this level. It's breaking the abstraction, as we have to pass low-level usb information around in the higher USB stack levels.
Good, what do you suggest we do when we apply driver model onto this stuff?
The USB driver code should be able to do the virt-to-phys controller mapping on it's own. In the Tegra world
Tegra is completely unimportant part of the usb ecosystem.
we use the information we get from device tree to do so, but I don't see a reason why your USB host driver code wouldn't be able to just require an array with configuration data from the board file.
I don't see how you transfer DT information into controller # ...
There is really no need to pass this information through all the USB stack interfaces.
Please explain.
Regards, Lucas
Dear Marek Vasut,
Am Dienstag, den 06.11.2012, 23:35 +0100 schrieb Marek Vasut:
Dear Lucas Stach,
[...]
What do you think?
What about passing port private / platform data instead of ID ?
The ID is already passed to ehci_hcd_init(), so we have to live with it if we don't want to change the newly introduced multi-controller infrastructure.
Let's change it .... remove the ID and pass some generic pdata.
I don't like the idea of passing around data at this level. It's breaking the abstraction, as we have to pass low-level usb information around in the higher USB stack levels.
Good, what do you suggest we do when we apply driver model onto this stuff?
Sadly I have not found the time to take a deeper look into the driver model. But see below.
The USB driver code should be able to do the virt-to-phys controller mapping on it's own. In the Tegra world
Tegra is completely unimportant part of the usb ecosystem.
I know that your views are centred around a different point, which is fine with me, but please don't make the mistake to downplay the importance of _any_ part of the ecosystem.
we use the information we get from device tree to do so, but I don't see a reason why your USB host driver code wouldn't be able to just require an array with configuration data from the board file.
I don't see how you transfer DT information into controller # ...
There is really no need to pass this information through all the USB stack interfaces.
Please explain.
Tegra has a two step initialisation:
1. Init the driver at board_init time This is the step where we parse all the DT information and fill in all needed driver internal structures. At this point we do the virt to phys controller ID mapping.
2. For every controller that U-Boot really uses we activate host mode and do the real hardware initialisation at ehci_hcd_init time.
If I'm not completely mistaken such a model should align nicely with the upcoming driver model. The driver gets instantiated with information it gathers from global platform data, may it be device tree or any other form of driver related information.
In this case the ehci_hcd_init|stop entry points are only used to init/stop one specific controller, which is completely different matter from the driver being instantiated and as such should not carry any platform data. IMHO all platform data should be contained in the boards global data.
Regards, Lucas
Dear Lucas Stach,
Dear Marek Vasut,
Am Dienstag, den 06.11.2012, 23:35 +0100 schrieb Marek Vasut:
Dear Lucas Stach,
[...]
> What do you think?
What about passing port private / platform data instead of ID ?
The ID is already passed to ehci_hcd_init(), so we have to live with it if we don't want to change the newly introduced multi-controller infrastructure.
Let's change it .... remove the ID and pass some generic pdata.
I don't like the idea of passing around data at this level. It's breaking the abstraction, as we have to pass low-level usb information around in the higher USB stack levels.
Good, what do you suggest we do when we apply driver model onto this stuff?
Sadly I have not found the time to take a deeper look into the driver model. But see below.
You might want to ... I suspect instead of passing ID, we should start passing some USB pdata. EHCI Pdata for ehci I guess ...
The USB driver code should be able to do the virt-to-phys controller mapping on it's own. In the Tegra world
Tegra is completely unimportant part of the usb ecosystem.
I know that your views are centred around a different point, which is fine with me, but please don't make the mistake to downplay the importance of _any_ part of the ecosystem.
On the contrary, I'm trying to avoid making the mistake of focusing on any SoC too much.
we use the information we get from device tree to do so, but I don't see a reason why your USB host driver code wouldn't be able to just require an array with configuration data from the board file.
I don't see how you transfer DT information into controller # ...
There is really no need to pass this information through all the USB stack interfaces.
Please explain.
Tegra has a two step initialisation:
- Init the driver at board_init time
This is the step where we parse all the DT information and fill in all needed driver internal structures. At this point we do the virt to phys controller ID mapping.
Hm ... thinking about it, maybe you can do generic USB Pdata which would contain the controller # and additional pdata (like mmap address etc).
- For every controller that U-Boot really uses we activate host mode
and do the real hardware initialisation at ehci_hcd_init time.
Good.
If I'm not completely mistaken such a model should align nicely with the upcoming driver model. The driver gets instantiated with information it gathers from global platform data, may it be device tree or any other form of driver related information.
Yes, but you don't pass such data through the driver (yet). You need to do that and that's what I asked you to do.
In this case the ehci_hcd_init|stop entry points are only used to init/stop one specific controller, which is completely different matter from the driver being instantiated and as such should not carry any platform data. IMHO all platform data should be contained in the boards global data.
I believe you should be passing pdata to the ehci_hcd_init ... they might contain some register frobbing etc., but this is probably the part where we missed each ones point.
Regards, Lucas
Best regards, Marek Vasut
Dear Marek Vasut,
Am Mittwoch, den 07.11.2012, 14:25 +0100 schrieb Marek Vasut:
Dear Lucas Stach,
Dear Marek Vasut,
Am Dienstag, den 06.11.2012, 23:35 +0100 schrieb Marek Vasut:
I don't see how you transfer DT information into controller # ...
There is really no need to pass this information through all the USB stack interfaces.
Please explain.
Tegra has a two step initialisation:
- Init the driver at board_init time
This is the step where we parse all the DT information and fill in all needed driver internal structures. At this point we do the virt to phys controller ID mapping.
Hm ... thinking about it, maybe you can do generic USB Pdata which would contain the controller # and additional pdata (like mmap address etc).
- For every controller that U-Boot really uses we activate host mode
and do the real hardware initialisation at ehci_hcd_init time.
Good.
If I'm not completely mistaken such a model should align nicely with the upcoming driver model. The driver gets instantiated with information it gathers from global platform data, may it be device tree or any other form of driver related information.
Yes, but you don't pass such data through the driver (yet). You need to do that and that's what I asked you to do.
We do pass this data to the driver in the form of gd->fdt_blob. This data is driver (not controller) specific, so why would you pass this in at ehci_hcd_init time?
But while writing this I think I now see why we miss each others point: the Tegra EHCI driver is only instantiated once and used for all controllers. This probably has to be reworked for the driver model.
Now I would still argue that we should keep the two step init model, first we instantiate the driver with some form of pdata (we can certainly come up with a one-struct-fits-all for this) and later when we are really going to use one specific controller we do the real hardware init.
Now we seem to differ about the meaning of the usb stack functions. From your mails I see that you want ehci_hcd_init as the first init step where we instantiate the driver (and therefore need the pdata), where I treated it as the second step, because currently it is the point where the upper USB stack levels indicate that they are going to use a specific controller.
We should probably come up with some consensus about this before going forward. Sadly my free time is really limited right now, so it's hard for me to keep up even with things I planned to do in the next few weeks, not to speak about playing around with the driver model.
Regards, Lucas
Dear Lucas Stach,
Dear Marek Vasut,
Am Mittwoch, den 07.11.2012, 14:25 +0100 schrieb Marek Vasut:
Dear Lucas Stach,
Dear Marek Vasut,
Am Dienstag, den 06.11.2012, 23:35 +0100 schrieb Marek Vasut:
I don't see how you transfer DT information into controller # ...
There is really no need to pass this information through all the USB stack interfaces.
Please explain.
Tegra has a two step initialisation:
- Init the driver at board_init time
This is the step where we parse all the DT information and fill in all needed driver internal structures. At this point we do the virt to phys controller ID mapping.
Hm ... thinking about it, maybe you can do generic USB Pdata which would contain the controller # and additional pdata (like mmap address etc).
- For every controller that U-Boot really uses we activate host mode
and do the real hardware initialisation at ehci_hcd_init time.
Good.
If I'm not completely mistaken such a model should align nicely with the upcoming driver model. The driver gets instantiated with information it gathers from global platform data, may it be device tree or any other form of driver related information.
Yes, but you don't pass such data through the driver (yet). You need to do that and that's what I asked you to do.
We do pass this data to the driver in the form of gd->fdt_blob. This data is driver (not controller) specific, so why would you pass this in at ehci_hcd_init time?
Sorry, I don't understand you.
But while writing this I think I now see why we miss each others point: the Tegra EHCI driver is only instantiated once and used for all controllers. This probably has to be reworked for the driver model.
What do you mean "instantiated once"? There ALWAYS has to be only a single instance per one controller.
Now I would still argue that we should keep the two step init model, first we instantiate the driver with some form of pdata (we can certainly come up with a one-struct-fits-all for this) and later when we are really going to use one specific controller we do the real hardware init.
Now we seem to differ about the meaning of the usb stack functions. From your mails I see that you want ehci_hcd_init as the first init step where we instantiate the driver (and therefore need the pdata)
No, I don't care what you do in your ehci_hcd_init. And I don't care if you instantiate it there. But I suspect I understand your problem. I suspect the driver shall be instantiated from elsewhere and ehci_hcd_init() call shall only be used to fine-tune (or work around) controller bugs.
where I treated it as the second step, because currently it is the point where the upper USB stack levels indicate that they are going to use a specific controller.
We should probably come up with some consensus about this before going forward. Sadly my free time is really limited right now, so it's hard for me to keep up even with things I planned to do in the next few weeks, not to speak about playing around with the driver model.
You're actually free to not work on that. Concensus is, I think the multi- controller thing is misdesigned and we rather fix it sooner than later.
See my comment above about how I'd like to see it.
Regards, Lucas
Best regards, Marek Vasut
Dear Marek Vasut,
On Wednesday, November 7, 2012 3:13:51 PM, Marek Vasut wrote:
Dear Lucas Stach,
Dear Marek Vasut,
Am Mittwoch, den 07.11.2012, 14:25 +0100 schrieb Marek Vasut: Now I would still argue that we should keep the two step init model, first we instantiate the driver with some form of pdata (we can certainly come up with a one-struct-fits-all for this) and later when we are really going to use one specific controller we do the real hardware init.
Now we seem to differ about the meaning of the usb stack functions. From your mails I see that you want ehci_hcd_init as the first init step where we instantiate the driver (and therefore need the pdata)
No, I don't care what you do in your ehci_hcd_init. And I don't care if you instantiate it there. But I suspect I understand your problem. I suspect the driver shall be instantiated from elsewhere and ehci_hcd_init() call shall only be used to fine-tune (or work around) controller bugs.
Not only for controller bugs, but also for related board operations through board_ehci_hcd_init(), or simply to perform a clean new init following a stop (e.g. in the case of the "usb reset" command).
where I treated it as the second step, because currently it is the point where the upper USB stack levels indicate that they are going to use a specific controller.
We should probably come up with some consensus about this before going forward. Sadly my free time is really limited right now, so it's hard for me to keep up even with things I planned to do in the next few weeks, not to speak about playing around with the driver model.
You're actually free to not work on that. Concensus is, I think the multi- controller thing is misdesigned and we rather fix it sooner than later.
See my comment above about how I'd like to see it.
If I understand correctly what you said, ehci_hcd_init() can be left unchanged because you don't care about what it does, so it will keep using the USB controller index from the command line. And we should add some "int ehci_hcd_bind(void *pdata)" that would be called by the board init files to perform the driver instantiation.
Until the driver model is applied, this instantiation would be an empty operation except for the drivers like ehci-mxc.c that need some platform data. Hence, for now, this ehci_hcd_bind() function would have to be implemented only for such drivers, which would be a small change that can be done step by step.
Correct me if I'm wrong above. My goal here is only to find a quick and simple solution to take advantage of the multi-controller feature on MXC. I don't have enough time to rework the whole infrastructure, so if this goal is incompatible with the current infrastructure and how you want to make it evolve, I'll come back when the infrastructure allows to truly use this feature.
Best regards, Benoît
Dear Benoît Thébaudeau,
Dear Marek Vasut,
On Wednesday, November 7, 2012 3:13:51 PM, Marek Vasut wrote:
Dear Lucas Stach,
Dear Marek Vasut,
Am Mittwoch, den 07.11.2012, 14:25 +0100 schrieb Marek Vasut: Now I would still argue that we should keep the two step init model, first we instantiate the driver with some form of pdata (we can certainly come up with a one-struct-fits-all for this) and later when we are really going to use one specific controller we do the real hardware init.
Now we seem to differ about the meaning of the usb stack functions. From your mails I see that you want ehci_hcd_init as the first init step where we instantiate the driver (and therefore need the pdata)
No, I don't care what you do in your ehci_hcd_init. And I don't care if you instantiate it there. But I suspect I understand your problem. I suspect the driver shall be instantiated from elsewhere and ehci_hcd_init() call shall only be used to fine-tune (or work around) controller bugs.
Not only for controller bugs, but also for related board operations through board_ehci_hcd_init(), or simply to perform a clean new init following a stop (e.g. in the case of the "usb reset" command).
where I treated it as the second step, because currently it is the point where the upper USB stack levels indicate that they are going to use a specific controller.
We should probably come up with some consensus about this before going forward. Sadly my free time is really limited right now, so it's hard for me to keep up even with things I planned to do in the next few weeks, not to speak about playing around with the driver model.
You're actually free to not work on that. Concensus is, I think the multi- controller thing is misdesigned and we rather fix it sooner than later.
See my comment above about how I'd like to see it.
If I understand correctly what you said, ehci_hcd_init() can be left unchanged because you don't care about what it does, so it will keep using the USB controller index from the command line. And we should add some "int ehci_hcd_bind(void *pdata)" that would be called by the board init files to perform the driver instantiation.
Until the driver model is applied, this instantiation would be an empty operation except for the drivers like ehci-mxc.c that need some platform data. Hence, for now, this ehci_hcd_bind() function would have to be implemented only for such drivers, which would be a small change that can be done step by step.
Correct me if I'm wrong above. My goal here is only to find a quick and simple solution to take advantage of the multi-controller feature on MXC. I don't have enough time to rework the whole infrastructure, so if this goal is incompatible with the current infrastructure and how you want to make it evolve, I'll come back when the infrastructure allows to truly use this feature.
Yes, let's try ehci_hcd_bind().
Best regards, Marek Vasut
participants (3)
-
Benoît Thébaudeau -
Lucas Stach -
Marek Vasut