Hi,
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.
--
balbi
Attachment:
signature.asc
Description: PGP signature
