Skip to contentSkip to conversation

usb: ehci: Take advantage of the new multi-controller feature for MXC

U-Boot Benoît Thébaudeau 3 participants 14 messages Nov 5, 2012 – Nov 18, 2012 1 message not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

#1 lore raw

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

#2 lore raw

Dear Beno?t Th?baudeau,

Quoted 39 lines

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] =

Quoted 8 lines
		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

#3 lore raw

Dear Marek Vasut,

Quoted 55 lines · Marek Vasut

Dear Beno?t Th?baudeau,

Quoted 53 lines

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.

Quoted 3 lines
Quoted 1 line

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

#4 lore raw

Dear Beno?t Th?baudeau,

Quoted 68 lines

Dear Marek Vasut,

Quoted 65 lines · Marek Vasut

Dear Beno?t Th?baudeau,

Quoted 63 lines

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.

Quoted 7 lines
Quoted 3 lines
Quoted 1 line

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.

Quoted 15 lines

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

#5 lore raw

Am Dienstag, den 06.11.2012, 00:56 +0100 schrieb Marek Vasut:

Quoted 72 lines

Dear Beno?t Th?baudeau,

Quoted 70 lines

Dear Marek Vasut,

Quoted 67 lines · Marek Vasut

Dear Beno?t Th?baudeau,

Quoted 64 lines

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?

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

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

#6 lore raw

Dear Lucas Stach,

Quoted 101 lines · Lucas Stach

Am Dienstag, den 06.11.2012, 00:56 +0100 schrieb Marek Vasut:

Quoted 79 lines

Dear Beno?t Th?baudeau,

Quoted 77 lines

Dear Marek Vasut,

Quoted 74 lines · Marek Vasut

Dear Beno?t Th?baudeau,

Quoted 71 lines

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?

Quoted 9 lines
Quoted 6 lines
Quoted 1 line

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.

Quoted 2 lines

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

#7 lore raw

Dear Beno?t Th?baudeau,

Quoted 51 lines

Dear Lucas Stach,

Quoted 48 lines · Lucas Stach

Am Dienstag, den 06.11.2012, 00:56 +0100 schrieb Marek Vasut:

Quoted 47 lines

Dear Beno?t Th?baudeau,

Quoted 45 lines

Dear Marek Vasut,

Quoted 42 lines · Marek Vasut

Dear Beno?t Th?baudeau,

Quoted 40 lines

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

Quoted 58 lines
Quoted 55 lines
Quoted 34 lines
Quoted 34 lines
Quoted 34 lines
Quoted 33 lines
		
		| \
		
		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?

Quoted 8 lines
Quoted 6 lines
Quoted 1 line

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.

Quoted 2 lines

There is really no need to pass this information through all the USB stack interfaces.

Quoted 8 lines

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.

#8 lore raw
In reply to Lucas Stach

Dear Lucas Stach,

[...]

Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
Quoted 1 line

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.

Quoted 5 lines
Quoted 1 line

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?

Quoted 2 lines

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.

Quoted 4 lines

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 # ...

Quoted 2 lines

There is really no need to pass this information through all the USB stack interfaces.

Please explain.

Quoted 2 lines

Regards,
Lucas

#9 lore raw

Dear Marek Vasut,

Am Dienstag, den 06.11.2012, 23:35 +0100 schrieb Marek Vasut:

Quoted 12 lines

Dear Lucas Stach,

[...]

Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
Quoted 1 line

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.

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

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.

Quoted 5 lines
Quoted 2 lines

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.

Quoted 11 lines
Quoted 4 lines

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 # ...

Quoted 2 lines

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

#10 lore raw

Dear Lucas Stach,

Quoted 21 lines

Dear Marek Vasut,

Am Dienstag, den 06.11.2012, 23:35 +0100 schrieb Marek Vasut:

Quoted 18 lines

Dear Lucas Stach,

[...]

Quoted 13 lines
Quoted 9 lines
Quoted 7 lines
Quoted 3 lines
Quoted 1 line

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.

Quoted 5 lines
Quoted 2 lines

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 ...

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

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.

Quoted 10 lines
Quoted 10 lines
Quoted 4 lines

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 # ...

Quoted 2 lines

There is really no need to pass this information through all the USB stack interfaces.

Quoted 8 lines
Quoted 1 line

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.

Hm ... thinking about it, maybe you can do generic USB Pdata which would contain the controller # and additional pdata (like mmap address etc).

Quoted 2 lines

2. For every controller that U-Boot really uses we activate host mode and do the real hardware initialisation at ehci_hcd_init time.

Good.

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 2 lines

Regards,
Lucas

Best regards,
Marek Vasut

#11 lore raw

Dear Marek Vasut,

Am Mittwoch, den 07.11.2012, 14:25 +0100 schrieb Marek Vasut:

Quoted 27 lines

Dear Lucas Stach,

Quoted 8 lines

Dear Marek Vasut,

Am Dienstag, den 06.11.2012, 23:35 +0100 schrieb Marek Vasut:

Quoted 5 lines

I don't see how you transfer DT information into controller # ...

Quoted 2 lines

There is really no need to pass this information through all the USB stack interfaces.

Quoted 8 lines
Quoted 1 line

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.

Hm ... thinking about it, maybe you can do generic USB Pdata which would contain the controller # and additional pdata (like mmap address etc).

Quoted 2 lines

2. For every controller that U-Boot really uses we activate host mode and do the real hardware initialisation at ehci_hcd_init time.

Good.

Quoted 8 lines
Quoted 4 lines

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

#12 lore raw

Dear Lucas Stach,

Quoted 35 lines

Dear Marek Vasut,

Am Mittwoch, den 07.11.2012, 14:25 +0100 schrieb Marek Vasut:

Quoted 32 lines

Dear Lucas Stach,

Quoted 8 lines

Dear Marek Vasut,

Am Dienstag, den 06.11.2012, 23:35 +0100 schrieb Marek Vasut:

Quoted 5 lines

I don't see how you transfer DT information into controller # ...

Quoted 2 lines

There is really no need to pass this information through all the USB stack interfaces.

Quoted 8 lines
Quoted 1 line

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.

Hm ... thinking about it, maybe you can do generic USB Pdata which would contain the controller # and additional pdata (like mmap address etc).

Quoted 2 lines

2. For every controller that U-Boot really uses we activate host mode and do the real hardware initialisation at ehci_hcd_init time.

Good.

Quoted 4 lines

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.

Quoted 6 lines
Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 9 lines

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.

Quoted 9 lines

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.

Quoted 2 lines

Regards,
Lucas

Best regards,
Marek Vasut

#13 lore raw

Dear Marek Vasut,

Quoted 19 lines · Marek Vasut

Dear Lucas Stach,

Quoted 12 lines

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.

Quoted 4 lines

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)

Quoted 7 lines

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).

Quoted 13 lines
Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 6 lines

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

#14 lore raw

Dear Beno?t Th?baudeau,

Quoted 66 lines

Dear Marek Vasut,

Quoted 19 lines · Marek Vasut

Dear Lucas Stach,

Quoted 12 lines

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.

Quoted 4 lines

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)

Quoted 7 lines

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).

Quoted 13 lines
Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 6 lines

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