Am 17.01.2017 um 09:11 schrieb Felipe Balbi:
>
> Hi,
>
> Heiner Kallweit <hkallweit1@xxxxxxxxx> writes:
>> Am 16.01.2017 um 15:05 schrieb Felipe Balbi:
>>>
>>> Hi,
>>>
>>> Heiner Kallweit <hkallweit1@xxxxxxxxx> writes:
>>>> Set the iomem parameters in the usb_hcd to fix this misleading
>>>> message during driver load:
>>>> dwc2 c9100000.usb: irq 22, io mem 0x00000000
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>>>> ---
>>>> drivers/usb/dwc2/core.h | 3 ++-
>>>> drivers/usb/dwc2/hcd.c | 5 ++++-
>>>> drivers/usb/dwc2/hcd.h | 3 ++-
>>>> drivers/usb/dwc2/platform.c | 2 +-
>>>> 4 files changed, 9 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>>>> index 9548d3e0..b66eaeea 100644
>>>> --- a/drivers/usb/dwc2/core.h
>>>> +++ b/drivers/usb/dwc2/core.h
>>>> @@ -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
>
Good, after having a closer look at the dwc2 driver:
There's not much code in pci.c so far. Basically it's a glue layer between
PCI and the code in platform.c.
If I understand you correctly it's about factoring out code from platform.c
to core.c and use this common code also from pci.c thus getting rid of the
glue layer. Right?
--
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
