Add tested id switch and vbus connect detect support for Chipidea

If the probe fails, the ci13xxx_add_device will not return error, (bus_probe_device doesn't has return value)
therefore, the platform layer can't know whether core's probe is successful or not, if platform layer goes on using core's struct which is initialized at core's probe, the error will occur.

This error is showed when I only compile gadget, the host-only controller reports "no supported roles", and fails probe, but imx platform code doesn't know it, and goes on using core's private data.

Signed-off-by Peter Chen <[email protected]>

Quoted 10 lines · Peter Chen

If the probe fails, the ci13xxx_add_device will not return error, (bus_probe_device doesn't has return value)
therefore, the platform layer can't know whether core's probe is successful or not, if platform layer goes on using core's struct which is initialized at core's probe, the error will occur.

This error is showed when I only compile gadget, the host-only controller reports "no supported roles", and fails probe, but imx platform code doesn't know it, and goes on using core's private data.

Quoted 1 line

Signed-off-by Peter Chen <[email protected]>

this just tells you that platform code shouldn't be using the driver directly. passing probe_retval via platform_data is an abomination, fix the real problem instead, whatever it is.

Quoted 13 lines · Felipe Balbi

Quoted 11 lines · Peter Chen

If the probe fails, the ci13xxx_add_device will not return error, (bus_probe_device doesn't has return value)
therefore, the platform layer can't know whether core's probe is successful or not, if platform layer goes on using core's struct which is initialized at core's probe, the error will occur.

This error is showed when I only compile gadget, the host-only controller reports "no supported roles", and fails probe, but imx platform code doesn't know it, and goes on using core's private data.

Signed-off-by Peter Chen <[email protected]>

Quoted 3 lines

this just tells you that platform code shouldn't be using the driver directly. passing probe_retval via platform_data is an abomination, fix the real problem instead, whatever it is.

So you suggest the platform glue layer should not use core driver's data directly, eg, for your dwc3, the platform glue layer should not use struct dwc3 *dwc directly?

At dwc3-exynos.c, it has code "exynos->dwc3 = dwc3;", so the exynos
platform code may will use struct dwc3 in future. Besides, if the dwc3

core driver's probe fails, the exynos platform code will not know it, the usb clk will be on on the usb can't be used.

If you suggest like above, we may need to add lots of notify function at

core driver as there are many platform specific things, eg, special init/ shutdown, suspend/resume, board layer gpio setting for vbus control (used

Hi,

Quoted 14 lines · Peter Chen

Quoted 13 lines · Felipe Balbi

Quoted 11 lines · Peter Chen

If the probe fails, the ci13xxx_add_device will not return error, (bus_probe_device doesn't has return value)
therefore, the platform layer can't know whether core's probe is successful or not, if platform layer goes on using core's struct which is initialized at core's probe, the error will occur.

This error is showed when I only compile gadget, the host-only controller reports "no supported roles", and fails probe, but imx platform code doesn't know it, and goes on using core's private data.

Signed-off-by Peter Chen <[email protected]>

Quoted 7 lines

Quoted 3 lines

this just tells you that platform code shouldn't be using the driver directly. passing probe_retval via platform_data is an abomination, fix the real problem instead, whatever it is.

So you suggest the platform glue layer should not use core driver's data directly, eg, for your dwc3, the platform glue layer should not use struct dwc3 *dwc directly?

yes, and it doesn't. Ever.

Quoted 2 lines

At dwc3-exynos.c, it has code "exynos->dwc3 = dwc3;", so the exynos
platform code may will use struct dwc3 in future. Besides, if the dwc3

nonsense. That's a structure platform_device which was created by the glue, it has nothing to do with struct dwc3. struct platform_device belongs to the glue, but there's an easy way to prevent that by using device_for_each_child() on your ->remove() method.

wrong. There's no need for any notification at all. A driver failing to probe is just normal life. DWC3 is releasing all resources it allocated, but the glue still needs the clock, then that's just the way it is.

There are many error messages which will tell the user that e.g. dwc3 failed to probe and user will just try again. If clocks are left enabled, that's a bug in either core driver or glue layer which needs fixing.

Quoted 2 lines

core driver as there are many platform specific things, eg, special init/ shutdown, suspend/resume, board layer gpio setting for vbus control (used

gpio handling should be done at board-file, that's a bug. You need to add a fixed regulator which is toggled by a GPIO.

Signature · 2 lines

-- 
balbi

Quoted 30 lines · Felipe Balbi

Hi,

Quoted 14 lines · Peter Chen

Quoted 13 lines · Felipe Balbi

Quoted 11 lines · Peter Chen

If the probe fails, the ci13xxx_add_device will not return error, (bus_probe_device doesn't has return value)
therefore, the platform layer can't know whether core's probe is successful or not, if platform layer goes on using core's struct which is initialized at core's probe, the error will occur.

This error is showed when I only compile gadget, the host-only controller reports "no supported roles", and fails probe, but imx platform code doesn't know it, and goes on using core's private data.

Signed-off-by Peter Chen <[email protected]>

Quoted 7 lines

Quoted 3 lines

this just tells you that platform code shouldn't be using the driver directly. passing probe_retval via platform_data is an abomination, fix the real problem instead, whatever it is.

So you suggest the platform glue layer should not use core driver's data directly, eg, for your dwc3, the platform glue layer should not use struct dwc3 *dwc directly?

yes, and it doesn't. Ever.

Quoted 2 lines

At dwc3-exynos.c, it has code "exynos->dwc3 = dwc3;", so the exynos
platform code may will use struct dwc3 in future. Besides, if the dwc3

Quoted 4 lines

nonsense. That's a structure platform_device which was created by the glue, it has nothing to do with struct dwc3. struct platform_device belongs to the glue, but there's an easy way to prevent that by using device_for_each_child() on your ->remove() method.

Sorry, I just thought it may use platform_get_drvdata(exynos->dwc3) to get core data if exynos has special suspend/resume routine, and need to visit dwc3 register.

Quoted 5 lines

There are many error messages which will tell the user that e.g. dwc3 failed to probe and user will just try again. If clocks are left enabled, that's a bug in either core driver or glue layer which needs fixing.

If the dwc3 core fails to probe, but controller core clk is still on, is it a valid case?

Quoted 4 lines

Quoted 2 lines

core driver as there are many platform specific things, eg, special init/ shutdown, suspend/resume, board layer gpio setting for vbus control (used

gpio handling should be done at board-file, that's a bug. You need to

Quoted 1 line

add a fixed regulator which is toggled by a GPIO.

I think I need to move vbus regulator from platform code to core code as we have struct otg at core data, and vbus operation is common operation.

Hi,

Quoted 29 lines · Peter Chen

Quoted 27 lines · Felipe Balbi

Hi,

Quoted 14 lines · Peter Chen

Quoted 13 lines · Felipe Balbi

Quoted 11 lines · Peter Chen

If the probe fails, the ci13xxx_add_device will not return error, (bus_probe_device doesn't has return value)
therefore, the platform layer can't know whether core's probe is successful or not, if platform layer goes on using core's struct which is initialized at core's probe, the error will occur.

This error is showed when I only compile gadget, the host-only controller reports "no supported roles", and fails probe, but imx platform code doesn't know it, and goes on using core's private data.

Signed-off-by Peter Chen <[email protected]>

Quoted 7 lines

Quoted 3 lines

this just tells you that platform code shouldn't be using the driver directly. passing probe_retval via platform_data is an abomination, fix the real problem instead, whatever it is.

So you suggest the platform glue layer should not use core driver's data directly, eg, for your dwc3, the platform glue layer should not use struct dwc3 *dwc directly?

yes, and it doesn't. Ever.

Quoted 2 lines

If the dwc3 core fails to probe, but controller core clk is still on, is it a valid case?

of course not, but then again, core clk shouldn't be handled by glue layer. You need to figure out who owns the clock, if it feeds DWC3 why would you clk_get() and clk_prepare_enable() from glue ? Makes no sense.

Quoted 26 lines

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

Quoted 11 lines

If the probe fails, the ci13xxx_add_device will not return error, (bus_probe_device doesn't has return value)
therefore, the platform layer can't know whether core's probe is successful or not, if platform layer goes on using core's struct which is initialized at core's probe, the error will occur.

This error is showed when I only compile gadget, the host-only controller reports "no supported roles", and fails probe, but imx platform code doesn't know it, and goes on using core's private data.

Signed-off-by Peter Chen <[email protected]>

this just tells you that platform code shouldn't be using the driver directly. passing probe_retval via platform_data is an abomination, fix the real problem instead, whatever it is.

Quoted 6 lines

Quoted 3 lines

So you suggest the platform glue layer should not use core driver's data directly, eg, for your dwc3, the platform glue layer should not use struct dwc3 *dwc directly?

yes, and it doesn't. Ever.

If the dwc3 core fails to probe, but controller core clk is still on, is it a valid case?

Quoted 4 lines

of course not, but then again, core clk shouldn't be handled by glue layer. You need to figure out who owns the clock, if it feeds DWC3 why would you clk_get() and clk_prepare_enable() from glue ? Makes no sense.

Sorry? I can't find clk_prepare_enable at dwc3/core.c, but at dwc3 core, it try to access register at probe, unless platform layer open the clock, how can the core visit the core register.

Hi,

Quoted 26 lines · Peter Chen

Quoted 26 lines

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

Quoted 11 lines

If the probe fails, the ci13xxx_add_device will not return error, (bus_probe_device doesn't has return value)
therefore, the platform layer can't know whether core's probe is successful or not, if platform layer goes on using core's struct which is initialized at core's probe, the error will occur.

This error is showed when I only compile gadget, the host-only controller reports "no supported roles", and fails probe, but imx platform code doesn't know it, and goes on using core's private data.

Signed-off-by Peter Chen <[email protected]>

this just tells you that platform code shouldn't be using the driver directly. passing probe_retval via platform_data is an abomination, fix the real problem instead, whatever it is.

Quoted 6 lines

Quoted 3 lines

So you suggest the platform glue layer should not use core driver's data directly, eg, for your dwc3, the platform glue layer should not use struct dwc3 *dwc directly?

yes, and it doesn't. Ever.

If the dwc3 core fails to probe, but controller core clk is still on, is it a valid case?

Quoted 8 lines

Quoted 4 lines

of course not, but then again, core clk shouldn't be handled by glue layer. You need to figure out who owns the clock, if it feeds DWC3 why would you clk_get() and clk_prepare_enable() from glue ? Makes no sense.

Sorry? I can't find clk_prepare_enable at dwc3/core.c, but at dwc3 core, it try to access register at probe, unless platform layer open the clock, how can the core visit the core register.

Is it really this difficult to figure out ? Fair enough, below are all the details:

To understand the reason why dwc3/core.c doesn't know about struct clk, you need to consider where the driver was originally written; it was written on an OMAP platform (actually first on a virtual model OMAP - somewhat like QEMU -, than on a PCIe FPGA prototype of the IP, then ARM-based FPGA prototype, then OMAP5, none of which needed explicit clock control, see below).

OMAP's PM is written in such a way that a pm_runtime_get() will enable the device the all clocks necessary to be usable. Since OMAP would never need to use clocks directly and I would never be able to test that code, I decided not to add it.

Now, if dwc3-exynos needs it, the sane thing to do would be add struct clk knowledge to dwc3/core.c but make it optional. If there are no clocks available, don't bail out.

Just because dwc3/core.c doesn't know about clocks, it doesn't mean it's correct to hack it into the glue layer if that doesn't need the clock.

Now that we know that's a bug, who's going to send me tested patches to teach dwc3/core.c about struct clk in a way that doesn't break PCIe, nor OMAP5 ?

Quoted 48 lines · Felipe Balbi

Hi,

Quoted 26 lines · Peter Chen

Quoted 26 lines

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

Quoted 11 lines

If the probe fails, the ci13xxx_add_device will not return error, (bus_probe_device doesn't has return value)
therefore, the platform layer can't know whether core's probe is successful or not, if platform layer goes on using core's struct which is initialized at core's probe, the error will occur.

This error is showed when I only compile gadget, the host-only controller reports "no supported roles", and fails probe, but imx platform code doesn't know it, and goes on using core's private data.

Signed-off-by Peter Chen <[email protected]>

this just tells you that platform code shouldn't be using the driver directly. passing probe_retval via platform_data is an abomination, fix the real problem instead, whatever it is.

Quoted 6 lines

Quoted 3 lines

So you suggest the platform glue layer should not use core driver's data directly, eg, for your dwc3, the platform glue layer should not use struct dwc3 *dwc directly?

yes, and it doesn't. Ever.

If the dwc3 core fails to probe, but controller core clk is still on, is it a valid case?

Quoted 8 lines

Quoted 4 lines

of course not, but then again, core clk shouldn't be handled by glue layer. You need to figure out who owns the clock, if it feeds DWC3 why would you clk_get() and clk_prepare_enable() from glue ? Makes no sense.

Sorry? I can't find clk_prepare_enable at dwc3/core.c, but at dwc3 core, it try to access register at probe, unless platform layer open the clock, how can the core visit the core register.

Is it really this difficult to figure out ? Fair enough, below are all the details:

To understand the reason why dwc3/core.c doesn't know about struct clk, you need to consider where the driver was originally written; it was written on an OMAP platform (actually first on a virtual model OMAP - somewhat like QEMU -, than on a PCIe FPGA prototype of the IP, then ARM-based FPGA prototype, then OMAP5, none of which needed explicit clock control, see below).

Quoted 8 lines

OMAP's PM is written in such a way that a pm_runtime_get() will enable the device the all clocks necessary to be usable. Since OMAP would never need to use clocks directly and I would never be able to test that code, I decided not to add it.

Now, if dwc3-exynos needs it, the sane thing to do would be add struct clk knowledge to dwc3/core.c but make it optional. If there are no clocks available, don't bail out.

I'm not too familiar with the multitudes of platforms out there, but my simple question is: why can't we have pm runtime take care of enabling/disabling the clocks so that we don't have to do it in drivers? Seems obvious that a platform/SoC/board should know about it's clock tree structure, so why doesn't the platform code then take care of all the dirty details?

It seems totally unreasonable and messy to add notion of clocks to drivers just because some platforms can't get their PM right.

Quoted 6 lines

Just because dwc3/core.c doesn't know about clocks, it doesn't mean it's correct to hack it into the glue layer if that doesn't need the clock.

Now that we know that's a bug, who's going to send me tested patches to teach dwc3/core.c about struct clk in a way that doesn't break PCIe, nor OMAP5 ?

So are you sure that's what you want?

Regards,
--
Alex

Quoted 54 lines

Quoted 45 lines

Quoted 26 lines

Quoted 26 lines

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

Quoted 11 lines

If the probe fails, the ci13xxx_add_device will not return error, (bus_probe_device doesn't has return value)
therefore, the platform layer can't know whether core's probe is successful or not, if platform layer goes on using core's struct which is initialized at core's probe, the error will occur.

This error is showed when I only compile gadget, the host-only controller reports "no supported roles", and fails probe, but imx platform code doesn't know it, and goes on using core's private data.

Signed-off-by Peter Chen <[email protected]>

this just tells you that platform code shouldn't be using the driver directly. passing probe_retval via platform_data is an abomination, fix the real problem instead, whatever it is.

Quoted 6 lines

Quoted 3 lines

So you suggest the platform glue layer should not use core driver's data directly, eg, for your dwc3, the platform glue layer should not use struct dwc3 *dwc directly?

yes, and it doesn't. Ever.

If the dwc3 core fails to probe, but controller core clk is still on, is it a valid case?

Quoted 8 lines

Quoted 4 lines

of course not, but then again, core clk shouldn't be handled by glue layer. You need to figure out who owns the clock, if it feeds DWC3 why would you clk_get() and clk_prepare_enable() from glue ? Makes no sense.

Sorry? I can't find clk_prepare_enable at dwc3/core.c, but at dwc3 core, it try to access register at probe, unless platform layer open the clock, how can the core visit the core register.

Is it really this difficult to figure out ? Fair enough, below are all the details:

To understand the reason why dwc3/core.c doesn't know about struct clk, you need to consider where the driver was originally written; it was written on an OMAP platform (actually first on a virtual model OMAP - somewhat like QEMU -, than on a PCIe FPGA prototype of the IP, then ARM-based FPGA prototype, then OMAP5, none of which needed explicit clock control, see below).

Quoted 8 lines

OMAP's PM is written in such a way that a pm_runtime_get() will enable the device the all clocks necessary to be usable. Since OMAP would never need to use clocks directly and I would never be able to test that code, I decided not to add it.

Now, if dwc3-exynos needs it, the sane thing to do would be add struct clk knowledge to dwc3/core.c but make it optional. If there are no clocks available, don't bail out.

Quoted 6 lines

I'm not too familiar with the multitudes of platforms out there, but my simple question is: why can't we have pm runtime take care of enabling/disabling the clocks so that we don't have to do it in drivers? Seems obvious that a platform/SoC/board should know about it's clock tree structure, so why doesn't the platform code then take care of all the dirty details?

I agree, clock stuffs should be handled at platform layer. For this corner case (core probe fails), if all of us agree with clock needs to be closed, we may need add some special handling.
For runtime pm enabled, it is easy. we can set runtime pm active at fail path, as platform is parent of core, it will call platform's runtime suspend to do low power handling.

For runtime pm disabled, we may had to add some ugly things, like notify core probe fail, and platform layer needs to handle this failed notify.

Hi,

Quoted 3 lines · Peter Chen

For runtime pm disabled, we may had to add some ugly things, like notify core probe fail, and platform layer needs to handle this failed notify.

Please stop talking about about "notify core probe fail" that will never happen. Not today, not ever. Forget that idea.

Signature · 2 lines

-- 
balbi

Quoted 8 lines · Felipe Balbi

Hi,

Quoted 3 lines · Peter Chen

For runtime pm disabled, we may had to add some ugly things, like notify core probe fail, and platform layer needs to handle this failed notify.

Quoted 2 lines

Please stop talking about about "notify core probe fail" that will never happen. Not today, not ever. Forget that idea.

Of course, it is ugly way, I just thought if it will affect power, eg dvfs, bus freq if the usb clock is on. Maybe the user will find USB core init fails case causes runtime power rise, and fix the core init fail bug.

Quoted 50 lines

Quoted 26 lines

Quoted 26 lines

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

Quoted 11 lines

If the probe fails, the ci13xxx_add_device will not return error, (bus_probe_device doesn't has return value)
therefore, the platform layer can't know whether core's probe is successful or not, if platform layer goes on using core's struct which is initialized at core's probe, the error will occur.

This error is showed when I only compile gadget, the host-only controller reports "no supported roles", and fails probe, but imx platform code doesn't know it, and goes on using core's private data.

Signed-off-by Peter Chen <[email protected]>

this just tells you that platform code shouldn't be using the driver directly. passing probe_retval via platform_data is an abomination, fix the real problem instead, whatever it is.

Quoted 6 lines

Quoted 3 lines

So you suggest the platform glue layer should not use core driver's data directly, eg, for your dwc3, the platform glue layer should not use struct dwc3 *dwc directly?

yes, and it doesn't. Ever.

If the dwc3 core fails to probe, but controller core clk is still on, is it a valid case?

Quoted 8 lines

Quoted 4 lines

of course not, but then again, core clk shouldn't be handled by glue layer. You need to figure out who owns the clock, if it feeds DWC3 why would you clk_get() and clk_prepare_enable() from glue ? Makes no sense.

Sorry? I can't find clk_prepare_enable at dwc3/core.c, but at dwc3 core, it try to access register at probe, unless platform layer open the clock, how can the core visit the core register.

Is it really this difficult to figure out ? Fair enough, below are all the details:

To understand the reason why dwc3/core.c doesn't know about struct clk, you need to consider where the driver was originally written; it was written on an OMAP platform (actually first on a virtual model OMAP - somewhat like QEMU -, than on a PCIe FPGA prototype of the IP, then ARM-based FPGA prototype, then OMAP5, none of which needed explicit clock control, see below).

OMAP's PM is written in such a way that a pm_runtime_get() will enable the device the all clocks necessary to be usable. Since OMAP would never need to use clocks directly and I would never be able to test that code, I decided not to add it.

Quoted 3 lines

Now, if dwc3-exynos needs it, the sane thing to do would be add struct clk knowledge to dwc3/core.c but make it optional. If there are no clocks available, don't bail out.

I'm not too familiar with the multitudes of platforms out there, but my simple question is: why can't we have pm runtime take care of enabling/disabling the clocks so that we don't have to do it in drivers?

Seems obvious that a platform/SoC/board should know about it's clock tree structure, so why doesn't the platform code then take care of all the dirty details?

It seems totally unreasonable and messy to add notion of clocks to drivers just because some platforms can't get their PM right.

Quoted 6 lines

Just because dwc3/core.c doesn't know about clocks, it doesn't mean it's correct to hack it into the glue layer if that doesn't need the clock.

Now that we know that's a bug, who's going to send me tested patches to teach dwc3/core.c about struct clk in a way that doesn't break PCIe, nor OMAP5 ?

So are you sure that's what you want?

Hi,

Quoted 50 lines · Alexander Shishkin

Quoted 50 lines

Quoted 26 lines

Quoted 26 lines

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

Quoted 11 lines

If the probe fails, the ci13xxx_add_device will not return error, (bus_probe_device doesn't has return value)
therefore, the platform layer can't know whether core's probe is successful or not, if platform layer goes on using core's struct which is initialized at core's probe, the error will occur.

This error is showed when I only compile gadget, the host-only controller reports "no supported roles", and fails probe, but imx platform code doesn't know it, and goes on using core's private data.

Signed-off-by Peter Chen <[email protected]>

this just tells you that platform code shouldn't be using the driver directly. passing probe_retval via platform_data is an abomination, fix the real problem instead, whatever it is.

Quoted 6 lines

Quoted 3 lines

So you suggest the platform glue layer should not use core driver's data directly, eg, for your dwc3, the platform glue layer should not use struct dwc3 *dwc directly?

yes, and it doesn't. Ever.

If the dwc3 core fails to probe, but controller core clk is still on, is it a valid case?

Quoted 8 lines

Quoted 4 lines

of course not, but then again, core clk shouldn't be handled by glue layer. You need to figure out who owns the clock, if it feeds DWC3 why would you clk_get() and clk_prepare_enable() from glue ? Makes no sense.

Sorry? I can't find clk_prepare_enable at dwc3/core.c, but at dwc3 core, it try to access register at probe, unless platform layer open the clock, how can the core visit the core register.

Is it really this difficult to figure out ? Fair enough, below are all the details:

To understand the reason why dwc3/core.c doesn't know about struct clk, you need to consider where the driver was originally written; it was written on an OMAP platform (actually first on a virtual model OMAP - somewhat like QEMU -, than on a PCIe FPGA prototype of the IP, then ARM-based FPGA prototype, then OMAP5, none of which needed explicit clock control, see below).

OMAP's PM is written in such a way that a pm_runtime_get() will enable the device the all clocks necessary to be usable. Since OMAP would never need to use clocks directly and I would never be able to test that code, I decided not to add it.

Quoted 7 lines

Quoted 3 lines

Now, if dwc3-exynos needs it, the sane thing to do would be add struct clk knowledge to dwc3/core.c but make it optional. If there are no clocks available, don't bail out.

I'm not too familiar with the multitudes of platforms out there, but my simple question is: why can't we have pm runtime take care of enabling/disabling the clocks so that we don't have to do it in drivers?

that's what OMAP does.

Quoted 3 lines

Seems obvious that a platform/SoC/board should know about it's clock tree structure, so why doesn't the platform code then take care of all the dirty details?

it might seem that way, but it's not that obvious ;-) Some platforms have a single clock, some will split interface and functional clocks, in some cases, you have extra optional clocks which might be needed during certain use cases or to implement erratas at locations that only driver knows.

It's a tradeoff, of course.

Quoted 2 lines

It seems totally unreasonable and messy to add notion of clocks to drivers just because some platforms can't get their PM right.

it's not that simple ;-)

Quoted 3 lines

Quoted 3 lines

Just because dwc3/core.c doesn't know about clocks, it doesn't mean it's correct to hack it into the glue layer if that doesn't need the clock.

Quoted 5 lines

Quoted 3 lines

Now that we know that's a bug, who's going to send me tested patches to teach dwc3/core.c about struct clk in a way that doesn't break PCIe, nor OMAP5 ?

So are you sure that's what you want?

Well, how quickly can Exynos be changed to handle clocks without driver's knowledge ?

Also, I'm a lot more 'at ease' when I see the driver explicitly handling all of its resources. The whole "let's hide XYZ from driver because driver authors never get things right" always causes problems. Specially in the ARM land where there's no standardization at all.

Quoted 64 lines

Quoted 50 lines

Quoted 50 lines

Quoted 26 lines

Quoted 26 lines

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

Quoted 11 lines

If the probe fails, the ci13xxx_add_device will not return error, (bus_probe_device doesn't has return value)
therefore, the platform layer can't know whether core's probe is successful or not, if platform layer goes on using core's struct which is initialized at core's probe, the error will occur.

This error is showed when I only compile gadget, the host-only controller reports "no supported roles", and fails probe, but imx platform code doesn't know it, and goes on using core's private data.

Signed-off-by Peter Chen <[email protected]>

this just tells you that platform code shouldn't be using the driver directly. passing probe_retval via platform_data is an abomination, fix the real problem instead, whatever it is.

Quoted 6 lines

Quoted 3 lines

So you suggest the platform glue layer should not use core driver's data directly, eg, for your dwc3, the platform glue layer should not use struct dwc3 *dwc directly?

yes, and it doesn't. Ever.

If the dwc3 core fails to probe, but controller core clk is still on, is it a valid case?

Quoted 8 lines

Quoted 4 lines

of course not, but then again, core clk shouldn't be handled by glue layer. You need to figure out who owns the clock, if it feeds DWC3 why would you clk_get() and clk_prepare_enable() from glue ? Makes no sense.

Sorry? I can't find clk_prepare_enable at dwc3/core.c, but at dwc3 core, it try to access register at probe, unless platform layer open the clock, how can the core visit the core register.

Is it really this difficult to figure out ? Fair enough, below are all the details:

To understand the reason why dwc3/core.c doesn't know about struct clk, you need to consider where the driver was originally written; it was written on an OMAP platform (actually first on a virtual model OMAP - somewhat like QEMU -, than on a PCIe FPGA prototype of the IP, then ARM-based FPGA prototype, then OMAP5, none of which needed explicit clock control, see below).

OMAP's PM is written in such a way that a pm_runtime_get() will enable the device the all clocks necessary to be usable. Since OMAP would never need to use clocks directly and I would never be able to test that code, I decided not to add it.

Quoted 7 lines

Quoted 3 lines

Now, if dwc3-exynos needs it, the sane thing to do would be add struct clk knowledge to dwc3/core.c but make it optional. If there are no clocks available, don't bail out.

I'm not too familiar with the multitudes of platforms out there, but my simple question is: why can't we have pm runtime take care of enabling/disabling the clocks so that we don't have to do it in drivers?

that's what OMAP does.

Quoted 3 lines

Seems obvious that a platform/SoC/board should know about it's clock tree structure, so why doesn't the platform code then take care of all the dirty details?

Quoted 5 lines

it might seem that way, but it's not that obvious ;-) Some platforms have a single clock, some will split interface and functional clocks, in some cases, you have extra optional clocks which might be needed during certain use cases or to implement erratas at locations that only driver knows.

Then drivers could use platform fixup callbacks. Curious how many

drivers out there do proper handling of interface vs functional clocks. Something tells me that the common pattern will be "enable all

clocks with lots of line of boilerplate copied from that other driver" in probe() and "disable all" in remove().

Quoted 2 lines

Well, how quickly can Exynos be changed to handle clocks without driver's knowledge ?

Motivation for the platforms to change should come from the general direction of linux kernel maintainers, in order for the change to happen. :)

But yes, for the time being, you're right, we'll probably have to just cope with it.

Quoted 4 lines

Also, I'm a lot more 'at ease' when I see the driver explicitly handling all of its resources. The whole "let's hide XYZ from driver because driver authors never get things right" always causes problems. Specially in the ARM land where there's no standardization at all.

I think it's going to be like that as long as developers are working in "hack it and ship it and let somebody else figure out APIs" routine. I

understand that some drivers will always need to call into clock framework directly, but other should have an option not to if their operation doesn't depend on it.

Changes for v9:
- Some small comments from Alex like: variable comment for otg event additional newline. [3/9]
- Import function tell show if the controller has otg capable, if the controller supports both host and device, we think it is otg capable, and can read OTGSC. [3/9]
- Merge two otg patches [v8 3/8] and [v8 4/8] to one [v9 3/9]. [3/9]
- Add inline to ci_hdrc_gadget_destroy if CONFIG_USB_CHIPIDEA_UDC is not defined, it can fix one build warning "defined but not used" [3/9]
- One comment from Felipe about changing calling gadget disconnect API at chipidea's udc driver. I move calling ci->driver->disconnect from _gadget_stop_activity to which calls _gadget_stop_activity except ci13xxx_stop, as udc core will call disconnect when do rmmod gadget. [7/9]
- Add ci core probe's return value to ci's platform_data, we do this for getting core's probe's result at platform layer, and quit it if the core's probe fails. [8/9] [9/9]

Changes for v8:
- Add ci_supports_gadget helper to know if the controller supports gadget, if the controller supports gadget, it needs to read otgsc to know the vbus value, basically, if the controller supports gadget, it will support host as well [3/8]
- At ci_hdrc_probe, it needs to add free memory at error path [3/8]
- Cosolidate ci->driver = NULL at ci13xxx_stop [8/8]

Changes for v7:
For Patch 8/8, we only need to set ci->driver to NULL when usb cable is not connected, for other changes, it will case some runtime pm unmatch and un-align with udc-core & composite driver problems.

Changes for v6:
- Add Alex comments for init/destroy function [3/8] [4/8]
- Remove memset(&ci->gadget, 0, sizeof(ci->gadget)) at destory function [4/8]
- Add Kishon's comment: Change the format of struct usb_otg otg at drivers/usb/chipidea/ci.h [1/8]
- Add comments for CI_VBUS_STABLE_TIMEOUT [3/8]
- Change the otg_set_peripheral return value check as the fully
chipidea driver users don't need it. [4/8]
- Fix one bug that the oops when re-plug in usb cable after
rmmod gadget [8/8]

Anyone who has tested this patchset, please give a tested-by, thanks.

This patchset adds tested otg id switch function and vbus connect/disconnect detection for chipidea driver. The mainly design of id/vbus handling follows msm otg driver. I hope the msm usb maintainer can have a look of this patchset, and give some comments, and move the whole msm usb driver to chipidea framework if possible in the future.

This patch is fully tested at imx6 sabresd platform.
My chipidea repo: https://github.com/hzpeterchen/linux-usb.git
which is rebased of Sascha's git
(git://git.pengutronix.de/git/imx/linux-2.6.git tags/usb-chipidea-otg-for-next)

At sascha's tag, it finishes some dr_mode and phy_type patches, in that way, we can choose host-only and gadget-only function at dts file as well as we can choose different phy setting.

Besides sascha's work, there are some Michael Grzeschik/Marc Kleine-Budde usbmisc patch, imx chipidea suspend/resume patch, and one pin control patch for imx6

Peter Chen (9):
Revert "USB: chipidea: add vbus detect for udc"
usb: chipidea: add otg file
usb: chipidea: add otg id switch and vbus connect/disconnect detect
usb: chipidea: udc: add pullup/pulldown dp at hw_device_state
usb: chipidea: udc: retire the flag CI13_PULLUP_ON_VBUS
usb: chipidea: imx: add internal vbus regulator control
usb: chipidea: udc: fix the oops when plugs in usb cable after rmmod
gadget
usb: chipidea: tell platform layer the ci core probe's result
usb: chipidea: imx: fix the error that using uninitialized pointer

 drivers/usb/chipidea/Makefile      |    2 +-
 drivers/usb/chipidea/bits.h        |   10 ++
 drivers/usb/chipidea/ci.h          |   14 +++-
 drivers/usb/chipidea/ci13xxx_imx.c |   85 ++++++++++++----
 drivers/usb/chipidea/ci13xxx_msm.c |    1 -
 drivers/usb/chipidea/core.c        |  192 ++++++++++++++++++++++++++++++++----
 drivers/usb/chipidea/host.c        |    6 +
 drivers/usb/chipidea/host.h        |    6 +-
 drivers/usb/chipidea/otg.c         |   68 +++++++++++++
 drivers/usb/chipidea/otg.h         |    9 ++
 drivers/usb/chipidea/udc.c         |  118 ++++++++++------------
 drivers/usb/chipidea/udc.h         |    4 +
 include/linux/usb/chipidea.h       |    2 +-
 13 files changed, 405 insertions(+), 112 deletions(-)
 create mode 100644 drivers/usb/chipidea/otg.c
 create mode 100644 drivers/usb/chipidea/otg.h

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html