On 1/25/2017 12:37 PM, Heiner Kallweit wrote:
> Am 24.01.2017 um 09:46 schrieb Felipe Balbi:
>>
>> Hi,
>>
>> John Youn <John.Youn@xxxxxxxxxxxx> writes:
>>>> John Youn <John.Youn@xxxxxxxxxxxx> writes:
>>>>>>>>> @@ -1229,7 +1229,8 @@ static inline void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) {}
>>>>>>>>> static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) {}
>>>>>>>>> static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
>>>>>>>>> static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
>>>>>>>>> -static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>>>>>>>>> +static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
>>>>>>>>> + struct resource *res)
>>>>>>>>> { return 0; }
>>>>>>>>> static inline int dwc2_backup_host_registers(struct dwc2_hsotg *hsotg)
>>>>>>>>> { return 0; }
>>>>>>>>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>>>>>>>>> index 911c3b36..2cfbd10e 100644
>>>>>>>>> --- a/drivers/usb/dwc2/hcd.c
>>>>>>>>> +++ b/drivers/usb/dwc2/hcd.c
>>>>>>>>> @@ -4964,7 +4964,7 @@ static void dwc2_hcd_release(struct dwc2_hsotg *hsotg)
>>>>>>>>> * USB bus with the core and calls the hc_driver->start() function. It returns
>>>>>>>>> * a negative error on failure.
>>>>>>>>> */
>>>>>>>>> -int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>>>>>>>>> +int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq, struct resource *res)
>>>>>>>>> {
>>>>>>>>> struct usb_hcd *hcd;
>>>>>>>>> struct dwc2_host_chan *channel;
>>>>>>>>> @@ -5021,6 +5021,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>>>>>>>>>
>>>>>>>>> hcd->has_tt = 1;
>>>>>>>>>
>>>>>>>>> + hcd->rsrc_start = res->start;
>>>>>>>>> + hcd->rsrc_len = resource_size(res);
>>>>>>>>> +
>>>>>>>>> ((struct wrapper_priv_data *) &hcd->hcd_priv)->hsotg = hsotg;
>>>>>>>>> hsotg->priv = hcd;
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
>>>>>>>>> index 1ed5fa2b..2305b5fb 100644
>>>>>>>>> --- a/drivers/usb/dwc2/hcd.h
>>>>>>>>> +++ b/drivers/usb/dwc2/hcd.h
>>>>>>>>> @@ -521,7 +521,8 @@ static inline u8 dwc2_hcd_is_pipe_out(struct dwc2_hcd_pipe_info *pipe)
>>>>>>>>> return !dwc2_hcd_is_pipe_in(pipe);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> -extern int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq);
>>>>>>>>> +extern int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
>>>>>>>>> + struct resource *res);
>>>>>>>>> extern void dwc2_hcd_remove(struct dwc2_hsotg *hsotg);
>>>>>>>>>
>>>>>>>>> /* Transaction Execution Functions */
>>>>>>>>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>>>>>>>>> index 4fc8c603..5ddc8860 100644
>>>>>>>>> --- a/drivers/usb/dwc2/platform.c
>>>>>>>>> +++ b/drivers/usb/dwc2/platform.c
>>>>>>>>> @@ -445,7 +445,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> if (hsotg->dr_mode != USB_DR_MODE_PERIPHERAL) {
>>>>>>>>> - retval = dwc2_hcd_init(hsotg, hsotg->irq);
>>>>>>>>> + retval = dwc2_hcd_init(hsotg, hsotg->irq, res);
>>>>>>>>
>>>>>>>> This is a good idea, but there's more work that can come out of this, if
>>>>>>>> you're interested:
>>>>>>>>
>>>>>>>> i) devm_ioremap_resource() could be called by the generic layer
>>>>>>>> ii) devm_request_irq() could be move to the generic layer
>>>>>>>> iii) dwc2_hcd_init() could also be called generically as long as dr_mode
>>>>>>>> is set properly.
>>>>>>>> iv) dwc2_debugfs_init() could be called generically as well
>>>>>>>>
>>>>>>>> IOW, dwc2_driver_probe() could be as minimal as:
>>>>>>>>
>>>>>>>> static int dwc2_driver_probe(struct platform_device *dev)
>>>>>>>> {
>>>>>>>> struct dwc2_hsotg *hsotg;
>>>>>>>> struct resource *res;
>>>>>>>> int retval;
>>>>>>>>
>>>>>>>> hsotg = devm_kzalloc(&dev->dev, sizeof(*hsotg), GFP_KERNEL);
>>>>>>>> if (!hsotg)
>>>>>>>> return -ENOMEM;
>>>>>>>>
>>>>>>>> hsotg->dev = &dev->dev;
>>>>>>>>
>>>>>>>> if (!dev->dev.dma_mask)
>>>>>>>> dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
>>>>>>>>
>>>>>>>> retval = dma_set_coherent_mask(&dev->dev, DMA_BIT_MASK(32));
>>>>>>>> if (retval)
>>>>>>>> return retval;
>>>>>>>>
>>>>>>>> hsotg->res = platform_get_resource(dev, IORESOURCE_MEM, 0);
>>>>>>>> hsotg->irq = platform_get_irq(dev, 0);
>>>>>>>>
>>>>>>>> retval = dwc2_get_dr_mode(hsotg);
>>>>>>>> if (retval)
>>>>>>>> return retval;
>>>>>>>>
>>>>>>>> retval = dwc2_get_hwparams(hsotg);
>>>>>>>> if (retval)
>>>>>>>> return retval;
>>>>>>>>
>>>>>>>> platform_set_drvdata(dev, hsotg);
>>>>>>>>
>>>>>>>> retval = dwc2_core_init(hsotg);
>>>>>>>> if (retval)
>>>>>>>> return retval;
>>>>>>>>
>>>>>>>> return 0;
>>>>>>>> }
>>>>>>>>
>>>>>>>> dwc2_core_init() needs to implemented, of course, but it could hide all
>>>>>>>> details about what to do with IRQs and resources and what not. Assuming
>>>>>>>> you can properly initialize clocks, it could even handle clocks
>>>>>>>> generically for you.
>>>>>>>>
>>>>>>> I see what you mean. I'm just a little confused about the term "generic" here:
>>>>>>> For me something generic has more than one user. Here we seem to speak just
>>>>>>> about factoring out some code from the probe function, or?
>>>>>>
>>>>>> :-) by generic, I mean moving some of that code to
>>>>>> drivers/usb/dwc2/core.c so it can be used by both PCI and non-PCI
>>>>>> systems.
>>>>>>
>>>>>>> Your proposal for reducing the probe function would mean to reorder some calls
>>>>>>> like the ones to dwc2_lowlevel_hw_init and dwc2_lowlevel_hw_enable.
>>>>>>> At a first glance this seems to be ok, but I'm not 100% sure.
>>>>>>
>>>>>> right, it needs to be carefully considered. But to me it looks totally
>>>>>> doable and it would remove code from both platform.c and pci.c
>>>>>>
>>>>>
>>>>> Hi Felipe,
>>>>>
>>>>> I believe what you are asking for here is already done.
>>>>>
>>>>> The platform driver does everything generically and works for both pci
>>>>> and non-pci. The PCI driver is just a glue layer which adds a platform
>>>>> device similar to dwc3-pci.
>>>>
>>>> not really. We still have code requesting the IRQ in both PCI and
>>>> platform.c, we also have code ioremapping the memory resource,
>>>> etc... All of those could be moved into the core driver.
>>>>
>>>
>>> Hi Felipe,
>>>
>>> No please check again. There are only two drivers dwc2 (core platform
>>> driver) and dwc2-pci (pci glue driver).
>>>
>>> The platform.c file is part of the dwc2 core driver. All the work is
>>> done *only* in this driver.
>>
>> oh, understood now. So it's pretty similar to dwc3 in that sense. I
>> thought platform.c was completely separate and PCI didn't make use of
>> that.
>>
>> thanks for clarifying
>>
> After this having been clarified: how about the patch which started
> the discussion?
Hah, forgot about that.
I'll comment on the original patch.
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
