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