From: Peter Chen <hidden> Date: 2013-02-17 09:24:34
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
From: Peter Chen <hidden> Date: 2013-02-17 09:24:35
vbus register is at otgsc, and vbus detect does not belong
to device function. Revert this patch, and will move
vbus detect function to drivers/usb/chipidea/udc.c
This reverts commit 8c4fc031954b4eb72daf13d3c907a985a3eee208.
Signed-off-by: Peter Chen <redacted>
---
drivers/usb/chipidea/ci.h | 1 -
drivers/usb/chipidea/udc.c | 39 +--------------------------------------
2 files changed, 1 insertions(+), 39 deletions(-)
@@ -0,0 +1,60 @@+/*+*otg.c-ChipIdeaUSBIPcoreOTGdriver+*+*Copyright(C)2012FreescaleSemiconductor,Inc.+*+*Author:PeterChen+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodify+*itunderthetermsoftheGNUGeneralPublicLicenseversion2as+*publishedbytheFreeSoftwareFoundation.+*/++#include<linux/platform_device.h>+#include<linux/module.h>+#include<linux/io.h>+#include<linux/irq.h>+#include<linux/kernel.h>+#include<linux/slab.h>+#include<linux/usb/gadget.h>+#include<linux/usb/otg.h>+#include<linux/usb/chipidea.h>++#include"ci.h"+#include"udc.h"+#include"bits.h"+#include"host.h"+#include"debug.h"++staticintci_otg_set_peripheral(structusb_otg*otg,+structusb_gadget*periph)+{+otg->gadget=periph;++return0;+}++staticintci_otg_set_host(structusb_otg*otg,structusb_bus*host)+{+otg->host=host;++return0;+}++/**+*ci_hdrc_otg_init-initializedevicerelatedbits+*ci:thecontroller+*+*Thisfunctioncreateotgstruct,ifthedevicecanswitchbetween+*deviceandhost.+*/+intci_hdrc_otg_init(structci13xxx*ci)+{+/* Useless at current */+ci->otg.set_peripheral=ci_otg_set_peripheral;+ci->otg.set_host=ci_otg_set_host;+if(!IS_ERR_OR_NULL(ci->transceiver))+ci->transceiver->otg=&ci->otg;++return0;+}
From: Peter Chen <hidden> Date: 2013-02-17 09:24:37
The main design flow is the same with msm otg driver, that is the id and
vbus interrupt are handled at core driver, others are handled by
individual drivers.
- At former design, when switch usb role from device->host, it will call
udc_stop, it will remove the gadget driver, so when switch role
from host->device, it can't add gadget driver any more.
At new design, when role switch occurs, the gadget just calls
usb_gadget_vbus_disconnect/usb_gadget_vbus_connect as well as
reset controller, it will not free any device/gadget structure
- Add vbus connect and disconnect to core interrupt handler, it can
notify udc driver by calling usb_gadget_vbus_disconnect
/usb_gadget_vbus_connect.
- vbus interrupt needs to be handled when gadget function is enabled
- Create/destroy the gadget at udc's init and destory function
- start/stop API are used at otg id switch and probe routine
- Defer some gadget operations at ci's delayed work queue
Signed-off-by: Peter Chen <redacted>
---
drivers/usb/chipidea/bits.h | 10 ++
drivers/usb/chipidea/ci.h | 9 ++-
drivers/usb/chipidea/ci13xxx_imx.c | 3 +-
drivers/usb/chipidea/core.c | 190 ++++++++++++++++++++++++++++++++----
drivers/usb/chipidea/host.c | 6 +
drivers/usb/chipidea/host.h | 6 +-
drivers/usb/chipidea/otg.c | 28 ++++--
drivers/usb/chipidea/otg.h | 3 +
drivers/usb/chipidea/udc.c | 39 ++++++--
drivers/usb/chipidea/udc.h | 4 +
10 files changed, 256 insertions(+), 42 deletions(-)
@@ -201,6 +202,14 @@ static int hw_device_init(struct ci13xxx *ci, void __iomem *base)if(ci->hw_ep_max>ENDPT_MAX)return-ENODEV;+/* Disable all interrupts bits */+hw_write(ci,OP_USBINTR,0xffffffff,0);+ci_disable_otg_interrupt(ci,OTGSC_INT_EN_BITS);++/* Clear all interrupts status bits*/+hw_write(ci,OP_USBSTS,0xffffffff,0xffffffff);+ci_clear_otg_interrupt(ci,OTGSC_INT_STATUS_BITS);+dev_dbg(ci->dev,"ChipIdea HDRC found, lpm: %d; cap: %p op: %p\n",ci->hw_bank.lpm,ci->hw_bank.cap,ci->hw_bank.op);
@@ -302,24 +311,131 @@ static enum ci_role ci_otg_role(struct ci13xxx *ci)}/**-*ci_role_work-performrolechangingbasedonIDpin-*@work:workstruct+*hw_wait_reg:waittheregistervalue+*+*Sometimes,itneedstowaitregistervaluebeforegoingon.+*Eg,whenswitchtodevicemode,thevbusvalueshouldbelower+*thanOTGSC_BSVbeforeconnectstohost.+*+*@ci:thecontroller+*@reg:registerindex+*@mask:mastbit+*@value:thebitvaluetowait+*@timeout:timeouttoindicateanerror+*+*Thisfunctionreturnsanerrorcodeiftimeout*/-staticvoidci_role_work(structwork_struct*work)+staticinthw_wait_reg(structci13xxx*ci,enumci13xxx_regsreg,u32mask,+u32value,unsignedlongtimeout)+{+unsignedlongelapse=jiffies+timeout;++while(hw_read(ci,reg,mask)!=value){+if(time_after(jiffies,elapse)){+dev_err(ci->dev,"timeout waiting for %08x in %d\n",+mask,reg);+return-ETIMEDOUT;+}+msleep(20);+}++return0;+}++/* +*Sincetherearesomecapacitancesatvbus,thevbusmaynot+*changeveryquicklywhenwestop/startinternal5v.+*Belowisavbusstabletimeoutvalue,theroutinewillquit+*ifthevbusgetstherequiredvalue,wecanthinktherearesome+*problemsforhardwareifthevbuscan'tgettherequiredvalue+*within5seconds.+*/+#define CI_VBUS_STABLE_TIMEOUT 500+staticvoidci_handle_id_switch(structci13xxx*ci){-structci13xxx*ci=container_of(work,structci13xxx,work);enumci_rolerole=ci_otg_role(ci);if(role!=ci->role){dev_dbg(ci->dev,"switching from %s to %s\n",ci_role(ci)->name,ci->roles[role]->name);+/* 1. Finish the current role */ci_role_stop(ci);+hw_device_reset(ci,USBMODE_CM_IDLE);++/* 2. Turn on/off vbus according to coming role */+if(role==CI_ROLE_GADGET){+otg_set_vbus(&ci->otg,false);+/* wait vbus lower than OTGSC_BSV */+hw_wait_reg(ci,OP_OTGSC,OTGSC_BSV,0,+CI_VBUS_STABLE_TIMEOUT);+}elseif(role==CI_ROLE_HOST){+otg_set_vbus(&ci->otg,true);+/* wait vbus higher than OTGSC_AVV */+hw_wait_reg(ci,OP_OTGSC,OTGSC_AVV,OTGSC_AVV,+CI_VBUS_STABLE_TIMEOUT);+}++/* 3. Begin the new role */ci_role_start(ci,role);-enable_irq(ci->irq);}}+voidci_handle_vbus_change(structci13xxx*ci)+{+u32otgsc=hw_read(ci,OP_OTGSC,~0);++if(otgsc&OTGSC_BSV)+usb_gadget_vbus_connect(&ci->gadget);+else+usb_gadget_vbus_disconnect(&ci->gadget);+}++/**+*ci_otg_work-performotg(vbus/id)eventhandle+*@work:workstruct+*/+staticvoidci_otg_work(structwork_struct*work)+{+structci13xxx*ci=container_of(work,structci13xxx,work);++if(ci->id_event){+ci->id_event=false;+ci_handle_id_switch(ci);+}elseif(ci->b_sess_valid_event){+ci->b_sess_valid_event=false;+ci_handle_vbus_change(ci);+}else+dev_err(ci->dev,"unexpected event occurs at %s\n",__func__);++enable_irq(ci->irq);+}++staticvoidci_delayed_work(structwork_struct*work)+{+structdelayed_work*dwork=to_delayed_work(work);+structci13xxx*ci=container_of(dwork,structci13xxx,dwork);++if(ci->role==CI_ROLE_GADGET){+/*+*ifitisdevicemode:+*-Enablevbusdetect+*-Ifithasalreadyconnectedtohost,notifyudc+*/+ci_enable_otg_interrupt(ci,OTGSC_BSVIE);+ci_handle_vbus_change(ci);+}elseif(ci->is_otg&&(ci->role==CI_ROLE_HOST)){+/* USB Device at the MicroB to A cable */+otg_set_vbus(&ci->otg,true);+}+}++staticinlinevoidci_role_destroy(structci13xxx*ci)+{+ci_hdrc_gadget_destroy(ci);+ci_hdrc_host_destroy(ci);+}+staticssize_tshow_role(structdevice*dev,structdevice_attribute*attr,char*buf){
@@ -512,7 +654,10 @@ static int ci_hdrc_probe(struct platform_device *pdev)}if(ci->roles[CI_ROLE_HOST]&&ci->roles[CI_ROLE_GADGET]){+dev_dbg(dev,"support otg\n");ci->is_otg=true;+/* if otg is supported, create struct usb_otg */+ci_hdrc_otg_init(ci);/* ID pin needs 1ms debouce time, we delay 2ms for safe */mdelay(2);ci->role=ci_otg_role(ci);
@@ -1371,6 +1372,7 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active)pm_runtime_get_sync(&_gadget->dev);hw_device_reset(ci,USBMODE_CM_DC);hw_device_state(ci,ci->ep0out->qh.dma);+dev_dbg(ci->dev,"Connected to host\n");}else{hw_device_state(ci,0);if(ci->platdata->notify_event)
@@ -1378,6 +1380,7 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active)CI13XXX_CONTROLLER_STOPPED_EVENT);_gadget_stop_activity(&ci->gadget);pm_runtime_put_sync(&_gadget->dev);+dev_dbg(ci->dev,"Disconnected from host\n");}}
@@ -1741,7 +1744,12 @@ static int udc_start(struct ci13xxx *ci)if(!IS_ERR_OR_NULL(ci->transceiver)){retval=otg_set_peripheral(ci->transceiver->otg,&ci->gadget);-if(retval)+/* +*IfweimplementallUSBfunctionsusingchipideadrivers,+*itdoesn'tneedtocallaboveAPI,meanwhile,ifweonly+*usegadgetfunction,callingaboveAPIisuseless.+*/+if(retval&&retval!=-ENOTSUPP)gotoremove_dbg;}
From: Peter Chen <hidden> Date: 2013-02-17 09:24:38
- During the connect/disconnect host, we need to pullup
and pulldown dp
- Make sure the dp is not pullup until the vbus is on when
flag CI13XXX_PULLUP_ON_VBUS is set
- Using hw_device_state when set run/stop bit
Signed-off-by: Peter Chen <redacted>
---
drivers/usb/chipidea/udc.c | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
From: Peter Chen <hidden> Date: 2013-02-17 09:24:39
(change CI13XXX to CI13 to avoid junk email check)
Now, we have handled vbus session in core driver when the
vbus interrupt occurs, so our pullup operations are all
according to vbus.
Of cource, the software can still call .pullup when device connects
to host if it wants to connect/disconnect with host.
Signed-off-by: Peter Chen <redacted>
---
drivers/usb/chipidea/ci13xxx_imx.c | 1 -
drivers/usb/chipidea/ci13xxx_msm.c | 1 -
drivers/usb/chipidea/udc.c | 23 ++++++++---------------
include/linux/usb/chipidea.h | 1 -
4 files changed, 8 insertions(+), 18 deletions(-)
From: Peter Chen <hidden> Date: 2013-02-17 09:24:40
- For host, the vbus should always be on.
- For otg, the vbus is off defaultly, the vbus needs to be
turned on/off when usb role switches.
Signed-off-by: Peter Chen <redacted>
---
drivers/usb/chipidea/ci.h | 2 +
drivers/usb/chipidea/ci13xxx_imx.c | 76 ++++++++++++++++++++++++++++--------
2 files changed, 61 insertions(+), 17 deletions(-)
@@ -85,11 +85,43 @@ EXPORT_SYMBOL_GPL(usbmisc_get_init_data);/* End of common functions shared by usbmisc drivers*/+staticintci13xxx_otg_set_vbus(structusb_otg*otg,boolenabled)+{+structci13xxx*ci=container_of(otg,structci13xxx,otg);+structregulator*reg_vbus=ci->reg_vbus;+intret;++WARN_ON(!reg_vbus);++if(reg_vbus){+if(enabled){+ret=regulator_enable(reg_vbus);+if(ret){+dev_err(ci->dev,+"Failed to enable vbus regulator, ret=%d\n",+ret);+returnret;+}+}else{+ret=regulator_disable(reg_vbus);+if(ret){+dev_err(ci->dev,+"Failed to disable vbus regulator, ret=%d\n",+ret);+returnret;+}+}+}++return0;+}+staticintci13xxx_imx_probe(structplatform_device*pdev){structci13xxx_imx_data*data;structci13xxx_platform_data*pdata;structplatform_device*plat_ci;+structci13xxx*ci;structresource*res;structregulator*reg_vbus;structpinctrl*pinctrl;
@@ -163,20 +195,11 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)data->phy=phy;}-/* we only support host now, so enable vbus here */reg_vbus=devm_regulator_get(&pdev->dev,"vbus");-if(!IS_ERR(reg_vbus)){-ret=regulator_enable(reg_vbus);-if(ret){-dev_err(&pdev->dev,-"Failed to enable vbus regulator, err=%d\n",-ret);-gotoerr_clk;-}+if(!IS_ERR(reg_vbus))data->reg_vbus=reg_vbus;-}else{+elsereg_vbus=NULL;-}pdata->phy=data->phy;
@@ -186,7 +209,7 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)if(!pdev->dev.dma_mask){ret=-ENOMEM;dev_err(&pdev->dev,"Failed to alloc dma_mask!\n");-gotoerr;+gotoerr_clk;}*pdev->dev.dma_mask=DMA_BIT_MASK(32);dma_set_coherent_mask(&pdev->dev,*pdev->dev.dma_mask);
@@ -579,9 +579,6 @@ static int _gadget_stop_activity(struct usb_gadget *gadget)usb_ep_fifo_flush(&ci->ep0out->ep);usb_ep_fifo_flush(&ci->ep0in->ep);-if(ci->driver)-ci->driver->disconnect(gadget);-/* make sure to disable all endpoints */gadget_for_each_ep(ep,gadget){usb_ep_disable(ep);
@@ -1373,6 +1375,8 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active)hw_device_state(ci,ci->ep0out->qh.dma);dev_dbg(ci->dev,"Connected to host\n");}else{+if(ci->driver)+ci->driver->disconnect(&ci->gadget);hw_device_state(ci,0);if(ci->platdata->notify_event)ci->platdata->notify_event(ci,
@@ -1580,13 +1584,14 @@ static int ci13xxx_stop(struct usb_gadget *gadget,if(ci->platdata->notify_event)ci->platdata->notify_event(ci,CI13XXX_CONTROLLER_STOPPED_EVENT);-ci->driver=NULL;spin_unlock_irqrestore(&ci->lock,flags);_gadget_stop_activity(&ci->gadget);spin_lock_irqsave(&ci->lock,flags);pm_runtime_put(&ci->gadget.dev);}+ci->driver=NULL;+spin_unlock_irqrestore(&ci->lock,flags);return0;
From: Peter Chen <hidden> Date: 2013-02-17 09:24:42
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 <redacted>
---
drivers/usb/chipidea/core.c | 2 ++
include/linux/usb/chipidea.h | 1 +
2 files changed, 3 insertions(+), 0 deletions(-)
From: Peter Chen <hidden> Date: 2013-02-17 09:24:43
If the core's probe fails, the platform layer may not get core's
private data, if the platform tries to use struct ci13xxx, it will
use uninitialized pointer. Besides, if the core's probe fails,
the platform layer should know it, and let its probe fail too.
Signed-off-by: Peter Chen <redacted>
---
drivers/usb/chipidea/ci13xxx_imx.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
From: Peter Chen <hidden> Date: 2013-02-26 09:36:00
On Sun, Feb 17, 2013 at 05:24:34PM +0800, Peter Chen wrote:
Hi Alex,
Ping............
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
From: Felipe Balbi <hidden> Date: 2013-02-26 09:42:34
On Sun, Feb 17, 2013 at 05:24:42PM +0800, Peter Chen wrote:
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 <redacted>
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.
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130226/b1d050bd/attachment.sig>
From: Peter Chen <hidden> Date: 2013-02-27 02:22:03
On Tue, Feb 26, 2013 at 11:42:34AM +0200, Felipe Balbi wrote:
On Sun, Feb 17, 2013 at 05:24:42PM +0800, Peter Chen wrote:
quoted
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 <redacted>
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
at id switch), probe fail, etc.
--
Best Regards,
Peter Chen
From: Felipe Balbi <hidden> Date: 2013-02-27 12:12:38
Hi,
On Wed, Feb 27, 2013 at 10:22:03AM +0800, Peter Chen wrote:
On Tue, Feb 26, 2013 at 11:42:34AM +0200, Felipe Balbi wrote:
quoted
On Sun, Feb 17, 2013 at 05:24:42PM +0800, Peter Chen wrote:
quoted
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 <redacted>
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.
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.
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.
so ? If the clock belongs to the glue, then the glue should enable it in
order to have access to its registers; if the clock belongs to the dwc3
core, then the glue is buggy; if the clock is shared between glue and
dwc3 core, then that's another bug, since nobody added clk handling code
to dwc3.
If you suggest like above, we may need to add lots of notify function at
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.
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.
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130227/a5bfa1e1/attachment.sig>
From: Peter Chen <hidden> Date: 2013-02-28 03:11:20
On Wed, Feb 27, 2013 at 02:12:38PM +0200, Felipe Balbi wrote:
Hi,
On Wed, Feb 27, 2013 at 10:22:03AM +0800, Peter Chen wrote:
quoted
On Tue, Feb 26, 2013 at 11:42:34AM +0200, Felipe Balbi wrote:
quoted
On Sun, Feb 17, 2013 at 05:24:42PM +0800, Peter Chen wrote:
quoted
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 <redacted>
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
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.
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.
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.
Here, we just talk the controller core clk, if the core fails to probe,
any meaningful for platform layer still alive?
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
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.
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.
From: Felipe Balbi <hidden> Date: 2013-02-28 07:26:17
Hi,
On Thu, Feb 28, 2013 at 11:11:20AM +0800, Peter Chen wrote:
On Wed, Feb 27, 2013 at 02:12:38PM +0200, Felipe Balbi wrote:
quoted
Hi,
On Wed, Feb 27, 2013 at 10:22:03AM +0800, Peter Chen wrote:
quoted
On Tue, Feb 26, 2013 at 11:42:34AM +0200, Felipe Balbi wrote:
quoted
On Sun, Feb 17, 2013 at 05:24:42PM +0800, Peter Chen wrote:
quoted
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 <redacted>
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
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.
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.
that would be utterly wrong and has caused many issues before (see
MUSB). Glue shouldn't know anything about the core IP.
quoted
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?
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
quoted
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
I mean "shouldn't"
quoted
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.
From: Peter Chen <hidden> Date: 2013-02-28 08:31:44
On Thu, Feb 28, 2013 at 09:26:17AM +0200, Felipe Balbi wrote:
Hi,
On Thu, Feb 28, 2013 at 11:11:20AM +0800, Peter Chen wrote:
quoted
On Wed, Feb 27, 2013 at 02:12:38PM +0200, Felipe Balbi wrote:
quoted
Hi,
On Wed, Feb 27, 2013 at 10:22:03AM +0800, Peter Chen wrote:
quoted
On Tue, Feb 26, 2013 at 11:42:34AM +0200, Felipe Balbi wrote:
quoted
On Sun, Feb 17, 2013 at 05:24:42PM +0800, Peter Chen wrote:
quoted
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 <redacted>
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.
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.
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.
--
Best Regards,
Peter Chen
From: Felipe Balbi <hidden> Date: 2013-02-28 08:49:33
Hi,
On Thu, Feb 28, 2013 at 04:31:44PM +0800, Peter Chen wrote:
quoted
quoted
quoted
quoted
quoted
quoted
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 <redacted>
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.
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.
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 ?
cheers
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130228/fd6a9b2f/attachment.sig>
Hi,
On Thu, Feb 28, 2013 at 04:31:44PM +0800, Peter Chen wrote:
quoted
quoted
quoted
quoted
quoted
quoted
quoted
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 <redacted>
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.
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.
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.
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.
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
From: Peter Chen <hidden> Date: 2013-02-28 10:06:55
On Thu, Feb 28, 2013 at 11:32:09AM +0200, Alexander Shishkin wrote:
Felipe Balbi [off-list ref] writes:
quoted
Hi,
On Thu, Feb 28, 2013 at 04:31:44PM +0800, Peter Chen wrote:
quoted
quoted
quoted
quoted
quoted
quoted
quoted
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 <redacted>
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.
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.
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.
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.
It seems totally unreasonable and messy to add notion of clocks to
drivers just because some platforms can't get their PM right.
quoted
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
From: Felipe Balbi <hidden> Date: 2013-02-28 10:42:40
Hi,
On Thu, Feb 28, 2013 at 11:32:09AM +0200, Alexander Shishkin wrote:
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
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 <redacted>
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.
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.
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.
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.
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.
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
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?
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.
When you think solely about x86 platforms, where you have ACPI properly
standardized and anyone wanting to build an x86 processor needs to
implement ACPI, it's a lot easier :-)
I'm sure the new "get default pinctrl state before driver probe()" will
cause issues down the road (think silicon erratas, shared balls on the
BGA packaging).
So, if they can hide clock handling from driver easily, good, let's do
just that. Otherwise, meanwhile we need to cope with the extra power
consumption that error condition would cause.
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130228/0b60a4ad/attachment.sig>
From: Felipe Balbi <hidden> Date: 2013-02-28 10:45:59
Hi,
On Thu, Feb 28, 2013 at 06:06:55PM +0800, Peter Chen wrote:
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
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 <redacted>
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.
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.
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.
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.
if core probe fails, we should still call pm_runtime_put_sync() and
pm_runtime_disable() and that should be enough to trigger your
->pm_domain->runtime_suspend() which can be used to turn off unnecessary
clocks.
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.
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130228/d2a08220/attachment.sig>
Hi,
On Thu, Feb 28, 2013 at 11:32:09AM +0200, Alexander Shishkin wrote:
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
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 <redacted>
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.
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.
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.
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
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.
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().
It's a tradeoff, of course.
quoted
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
quoted
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?
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.
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.
Regards,
--
Alex
From: Peter Chen <hidden> Date: 2013-03-01 01:45:51
On Thu, Feb 28, 2013 at 12:45:59PM +0200, Felipe Balbi wrote:
Hi,
On Thu, Feb 28, 2013 at 06:06:55PM +0800, Peter Chen wrote:
quoted
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.
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.
From: Felipe Balbi <hidden> Date: 2013-03-01 06:29:18
On Fri, Mar 01, 2013 at 09:45:51AM +0800, Peter Chen wrote:
On Thu, Feb 28, 2013 at 12:45:59PM +0200, Felipe Balbi wrote:
quoted
Hi,
On Thu, Feb 28, 2013 at 06:06:55PM +0800, Peter Chen wrote:
quoted
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.
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.
right, we need to fix the bug, but hiding it under some obscure way of
passing return values back to glue layer isn't correct. We should fix
the real issue, always, instead of hacking around buggy code.
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130301/5a467e76/attachment.sig>
From: Felipe Balbi <hidden> Date: 2013-03-01 06:36:43
Hi,
On Thu, Feb 28, 2013 at 01:55:30PM +0200, Alexander Shishkin wrote:
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
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 <redacted>
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.
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.
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.
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
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.
Then drivers could use platform fixup callbacks. Curious how many
ugh, I just puked in my mouth a little bit...
drivers out there do proper handling of interface vs functional
clocks. Something tells me that the common pattern will be "enable all
that, in itself, is no argument against explicitly handling clocks. Bugs
are bugs and will always exist, instead of hiding the problem, we should
fix them ;-)
clocks with lots of line of boilerplate copied from that other driver"
in probe() and "disable all" in remove().
for a first iteration, you're likely right. When we get to a point where
driver is really stable we can start trying to optimize runtime power
consumption by fiddling with clocks.
If we're not going to access a device's register, why would you keep
interface clock running ? That's where e.g. runtime_pm fails to give us
a good solution. There's no (easy) way to tell pm_domain code that it
can gate interface clock but not functional clock.
quoted
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.
exactly. Just because XYZ wants clocks to be handled all via pm_domain,
it doesn't mean we can switch to that overnight. On top of that there
are situations such as the one describe above.
quoted
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
that's why we need capable maintainers. Sometimes things will fall
through the cracks, sure, but in most cases we should be blocking
"hack and ship" during code review ;-)
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.
right, so that's why I asked to add struct clk knowledge to dwc3 but
make it optional: OMAP doesn't need it, PCIe doesn't need it; only
exynos (currently) needs it.
cheers
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130301/da110f05/attachment-0001.sig>