[V8 PATCH 01/16] usb: phy: mv_usb2: add PHY driver for marvell usb2 controller

Chao Xie xiechao.mail at gmail.com
Tue Mar 5 21:11:41 EST 2013


On Tue, Mar 5, 2013 at 7:04 PM, Felipe Balbi <balbi at ti.com> wrote:
> Hi,
>
> On Tue, Mar 05, 2013 at 10:03:01AM +0800, Chao Xie wrote:
>> >> +enum mv_usb2_phy_type {
>> >> +     PXA168_USB,
>> >> +     PXA910_USB,
>> >> +     MMP2_USB,
>> >> +};
>> >
>> >
>> > ewww... you really don't need (and *shouldn't* use) u2o_set() or
>> > u2o_clear(). They clearly prevent compiler from optimizing variable
>> > usage and could cause pressure on your interconnect. By writing and
>> > reading multiple times for no reason.
>> >
>>
>> The APIs defined here is for device operation. The device register
>> read/write is not same as memory.
>> When a silicon comes, it may not be stable, and will have done some
>> workaround epecially for the device register read/write. to define the
>> APIs for the register read/write will help to implement the workaround
>> without changing phy init code every time.
>> Now the only constrain is should read back the registers if you have
>> writen to it.
>> It will low down the performance, but it does not matter. Because phy
>> init will only done once when you initialize it.
>> I will think about reove the u2o_xxx APIs.
>
> You didn't even understand what I meant. Seriously.
>
> Anyway, details are as follows:
>
> readl() and writel() always add a memory barrier around each operation.
>
> This is good because it makes sure memory mapped I/O region is always
> ordered, but your current usage will post reads and writes on the
> interconnect over and over again for no reason. What you should do, with
> any register access is:
>
> reg = readl();
>
> reg &= ~BITS_TO_DISABLE;
> reg |= BITS_TO_ENABLE;
>
> writel(reg);
>
> Whereas your current code does:
>
> reg = readl();
> reg &= ~BITS_TO_DISABLE;
> writel(reg);
>
> reg = readl();
> reg |= BITS_TO_ENABLE;
> writel(reg);
>
> You do that for each bit, in some cases.
>
>> >> +static int _mv_usb2_phy_init(struct mv_usb2_phy *mv_phy)
>> >> +{
>> >> +     struct platform_device *pdev = mv_phy->pdev;
>> >> +     unsigned int loops = 0;
>> >> +     void __iomem *base = mv_phy->base;
>> >> +
>> >> +     dev_dbg(&pdev->dev, "phy init\n");
>> >
>> > remove this debugging message.
>> >
>> >> +     /* Initialize the USB PHY power */
>> >> +     if (mv_phy->type == PXA910_USB) {
>> >
>> > you _do_ have a REVISION register. Are you 100% certain that revision is
>> > the same on all your devices ? It seems to me that you should be doing
>> > proper revision detection instead of adding the hacky enumeration of
>> > yours.
>>
>> We do not have revison registers and will do not want ot define
>> #ifdef CPU_PXA910 or CPU_PXA_XXX
>> or
>> use is_cpu_pxa910 or cpu_pxa_xxx
>> because it is not acceptable. for example, if we have new SOC and it
>> use the same PHY as pxa910
>> i have to change the USB driver code to support it. for example
>> #ifdef CPU_PXA910 || CPU_XXX
>
> what a load of crap.
>
> *read your code!!!*
>
> There is a UTMI_REVISION register defined at offset 0.
>
>> So i have to depends on the device_id to do the work.
>> >
>> >> +     /* UTMI_IVREF */
>> >> +     if (mv_phy->type == PXA168_USB)
>> >> +             /* fixing Microsoft Altair board interface with NEC hub issue -
>> >> +              * Set UTMI_IVREF from 0x4a3 to 0x4bf */
>> >
>> > wrong comment style. Run *ALL* your patches through checkpatch.pl
>> > --strict and sparse.
>> >
>>
>> It seems that checkpatch.pl can not detect everything. I really use
>> checpatch.pl for
>> every patch i sent to maillist.
>> sorry for that, i will fix it.
>
> did you use --strict ?
>
>> >> +static int _mv_usb2_phy_shutdown(struct mv_usb2_phy *mv_phy)
>> >> +{
>> >> +     void __iomem *base = mv_phy->base;
>> >> +
>> >> +     if (mv_phy->type == PXA168_USB)
>> >> +             u2o_clear(base, UTMI_OTG_ADDON, UTMI_OTG_ADDON_OTG_ON);
>> >> +
>> >> +     u2o_clear(base, UTMI_CTRL, UTMI_CTRL_RXBUF_PDWN);
>> >> +     u2o_clear(base, UTMI_CTRL, UTMI_CTRL_TXBUF_PDWN);
>> >> +     u2o_clear(base, UTMI_CTRL, UTMI_CTRL_USB_CLK_EN);
>> >> +     u2o_clear(base, UTMI_CTRL, 1<<UTMI_CTRL_PWR_UP_SHIFT);
>> >> +     u2o_clear(base, UTMI_CTRL, 1<<UTMI_CTRL_PLL_PWR_UP_SHIFT);
>> >
>> > NAK, this is stupid, read once, clear bits you want to clear and write
>> > only once.
>> >
>>
>> I will check with silicon design engineer. becuase all the phy init
>> code is delivered by them.
>
> right, but you need to be critical with any code you read. You can't
> simply blindly make it compile on linux and hope that it'll work
> efficiently.
>
> I doubt your silicon validation evironment has support for the memory
> barriers which are added on each readl()/writel() calls.
>
>> I can not tell that if there are any speciall reason they will do so
>> because the device register read/write is not same as normal memory.
>
> seriously ? Read your documentation dude, there's nothing that will
> require you to enable one bit in the register, then flush the write,
> then write another bit, flush the write, write another bit, flush the
> write and so on.
>
> Those cases are extremely rare (MUSB has only *ONE* such case with
> regards to DMA_MODE bit) and in 99% of the cases, you can write
> everything to a variable and post a single write to your interconnect.
>
> Stop trying to come up with nonsensical excuses that "register
> read/write is not same as normal memory", of course it's not same as
> normal memory, and that's why we have standardized I/O functions.
>
>> >> +     return 0;
>> >> +}
>> >> +
>> >> +static int mv_usb2_phy_init(struct usb_phy *phy)
>> >> +{
>> >> +     struct mv_usb2_phy *mv_phy = container_of(phy, struct mv_usb2_phy, phy);
>> >> +     int i = 0;
>> >> +
>> >> +     mutex_lock(&mv_phy->phy_lock);
>> >
>> > what's this mutex for ?
>> >
>> >> +     if (mv_phy->refcount++ == 0) {
>> >
>> > can this device really be used simultaneously by multiple devices ?
>> >
>>
>> Sure, we have another EHCI device, it will share the PHY with this USB
>> controller.
>
> what is "this USB controller", current driver is for the PHY only. Do
> you mean to say that PHY will be shared by EHCI and UDC ?
>
>> So we will use refcount and mutext to protect the phy init.
>
> in that case, it might be better to add a generic refcounting to the PHY
> layer as a whole, instead of cooking your own private solutions.
>
>> >> +     for (i = 0; i < mv_phy->clks_num; i++) {
>> >> +             mv_phy->clks[i] = devm_clk_get(&pdev->dev,
>> >> +                                             pdata->clkname[i]);
>> >
>> > *NEVER* pass clock names via platform_data, this is utterly wrong.
>> >
>> without device tree support, the only way we can get the clock is the pdata.
>> the use phy have mutiple clocks.
>> So what do you suggest to handle it?
>
> clkdev
>
>> >> +static struct platform_driver mv_usb2_phy_driver = {
>> >> +     .probe  = mv_usb2_phy_probe,
>> >> +     .remove = mv_usb2_phy_remove,
>> >> +     .driver = {
>> >> +             .name   = "pxa168-usb-phy",
>> >> +     },
>> >> +     .id_table = mv_usb2_phy_ids,
>> >> +};
>> >> +
>> >> +static int __init mv_usb2_phy_driver_init(void)
>> >> +{
>> >> +     return platform_driver_register(&mv_usb2_phy_driver);
>> >> +}
>> >> +arch_initcall(mv_usb2_phy_driver_init);
>> >
>> > NAK, use module_platform_driver() like everybody else and handle
>> > registration ordering with -EPROBE_DEFER.
>> >
>>
>> The reason i do not use module_platform_driver is the compiling
>> sequence of each directory of driver/usb/
>> the phy is compiled after otg/ehci. So it means that it can not find
>> the usb phy, and will register fail.
>
> it doesn't matter, you should teach EHCI about -EPROBE_DEFER. If it
> can't grab the PHY, return -EPROBE_DEFER.
>
> --
> balbi

Thanks for the comments, i summary the feedbacks and suggestions as below,
1. For the phy setting and u2o_xx APIs something,  i will optimize it.
If there are some special cases, i will add comments.
2. For the module_platform_driver, -EPROBE_DEFER is what i missed. I
will check it and thanks for suggestion.
3. For the revison register. It exists in some SOCes(pxa168), but for
some SOCes, the register dispears(pxa910, armada610). These SOCes are
developed by different desgin teams, and it need to be enhanced, but
for current products i have to use the device_id to detect the PHY
controller.
4. For the mutex and refcount. The "USB controller" includes two
blocks - the udc and ehci. In fact they will not be used at same time,
but for some SOCes it duplicates the ehci block. It means that the
SOCes have two or more ehci blocks. The added ehci blocks depend on
the PHY to be initialized, and they can be used at same time as the
"USB controller". That is the reason i add mutex and refcount for phy
driver.
5. For the clock name. Can you describe it in details? For clkdev, if
it want to find the clock, it still depends on the dev_name and
con_id.



More information about the linux-arm-kernel mailing list