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