Am 17.01.2017 um 21:08 schrieb Heiner Kallweit:
> Am 17.01.2017 um 09:11 schrieb Felipe Balbi:
>>
>> Hi,
>>
>> Heiner Kallweit <hkallwe...@gmail.com> writes:
>>> Am 16.01.2017 um 15:05 schrieb Felipe Balbi:
>>>>
>>>> Hi,
>>>>
>>>> Heiner Kallweit <hkallwe...@gmail.com> 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 <hkallwe...@gmail.com>
>>>>> ---
>>>>> 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?
>
By the way: This would basically reverse what was done in
9024c495f35b "Add device mode to the dwc2-pci driver"
There the glue layer was introduced by John.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
