Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Krzysztof Kozlowski]

On 28/04/2022 03:29, Jung Daehwan wrote:
> On Tue, Apr 26, 2022 at 02:59:57PM +0200, Krzysztof Kozlowski wrote:
>> On 26/04/2022 11:18, Daehwan Jung wrote:
>>> This driver is for Samsung Exynos xhci host conroller. It uses xhci-plat
>>> driver mainly and extends some functions by xhci hooks and overrides.
>>>
>>> It supports USB Audio offload with Co-processor. It only cares DCBAA,
>>> Device Context, Transfer Ring, Event Ring, and ERST. They are allocated
>>> on specific address with xhci hooks. Co-processor could use them directly
>>> without xhci driver after then.
>>
>> This does not look like developed in current Linux kernel, but something
>> out-of-tree, with some other unknown modifications. This is not how the
>> code should be developed. Please rebase on linux-next and drop any
>> unrelated modifications (these which are not sent with this patchset).
>>
> 
> I've been developing on linux-next and I rebase before submitting.
> Could you tell me one of dropped modifications or patches?
> 
>> (...)
>>
>>> +
>>> +static int xhci_exynos_suspend(struct device *dev)
>>> +{
>>> +	struct usb_hcd	*hcd = dev_get_drvdata(dev);
>>> +	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>>> +
>>> +	/* TODO: AP sleep scenario*/
>>
>> Shall the patchset be called RFC?
>>
> OK. I will add RFC for this patch on next submission.
> 
>>> +
>>> +	return xhci_suspend(xhci, device_may_wakeup(dev));
>>> +}
>>> +
>>> +static int xhci_exynos_resume(struct device *dev)
>>> +{
>>> +	struct usb_hcd	*hcd = dev_get_drvdata(dev);
>>> +	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>>> +	int ret;
>>> +
>>> +	/* TODO: AP resume scenario*/
>>> +
>>> +	ret = xhci_resume(xhci, 0);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	pm_runtime_disable(dev);
>>> +	pm_runtime_set_active(dev);
>>> +	pm_runtime_enable(dev);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct dev_pm_ops xhci_exynos_pm_ops = {
>>> +	SET_SYSTEM_SLEEP_PM_OPS(xhci_exynos_suspend, xhci_exynos_resume)
>>> +};
>>> +
>>> +MODULE_DESCRIPTION("xHCI Exynos Host Controller Driver");
>>> +MODULE_LICENSE("GPL");
>>
>> You don't have list of compatibles (and missing bindings), driver
>> definition, driver registration. Entire solution is not used - nothing
>> calls xhci_exynos_vendor_init(), because nothign uses "ops".
>>
> 
> xhci_exynos_vendor_init is called in xhci-plat.c (xhci_vendor_init)
> [v4,2/5] usb: host: add xhci hooks for xhci-exynos
> ops are used in some files(xhci-mem.c, xhci.c ..) and the body of ops is in
> all xhci-exynos.


Nothing uses the "ops" except xhci_exynos_register_vendor_ops() which is
not called anywhere, so the xhci_vendor_init() does not call
xhci_exynos_vendor_init().

> 
> xhci-exynos is not a standalone driver. It could be enabled when other module
> makes xhci platform driver probed as it uses xhci platform mainly.

It "could be" or "will be"? We do not talk here about theoretical usage
of the driver, but a real one.

> I thought I just used existing compltible not adding new one.
> I will add them if needed.

Since you called everything here as "exynos" it is specific to one
hardware and not-reusable on anything else. How can then you use some
other compatible? It would be a misuse of Devicetree bindings.

Unless this is not specific to "exynos", but then please remove any
exynos and vendor references and make it integrated in generic XHCI
working for all drivers.


>> This does not work and it makes it impossible to test it. Please provide
>> proper XHCI Exynos driver, assuming you need it and it is not part of
>> regular Exynos XHCI drivers (DWC3 and so on).
>>
> 
> What makes you think it doesn't work?

Not possible to probe. The code cannot be loaded.

> I think it's almost proper. I just removed
> other IPs code like Co-Processor(we call it abox) or Power Management because
> it would make build-error. I've added hooking points in some files(xhci-mem.c,
> xhci.c..) and ops are implemented in xhci-exynos. It's mainly operated with
> xhci platform driver.

Best regards,
Krzysztof