Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 22 Mar 2022 11:30:18 +0900
From:   Jung Daehwan <dh10.jung@samsung.com>
To:     =?iso-8859-1?Q?Bj=F8rn?= Mork <bjorn@mork.no>
Cc:     Mathias Nyman <mathias.nyman@intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        Howard Yen <howardyen@google.com>,
        Jack Pham <jackp@codeaurora.org>,
        Puma Hsu <pumahsu@google.com>,
        "J . Avila" <elavila@google.com>, sc.suh@samsung.com
Subject: Re: [PATCH v3 4/4] usb: host: add xhci-exynos driver
Message-ID: <20220322023018.GC67215@ubuntu>
MIME-Version: 1.0
In-Reply-To: <87r16v9uo2.fsf@miraculix.mork.no>
User-Agent: Mutt/1.5.24 (2015-08-30)
Content-Type: multipart/mixed;
        boundary="----.rW42XXn7_Ji6OxwuAF4RAMbP.qed.OeqKlvEhSLhB3noATS=_22cbc0_"
CMS-TYPE: 102P
DLP-Filter: Pass
References: <1647853194-62147-1-git-send-email-dh10.jung@samsung.com>
        <CGME20220321090205epcas2p15ac16f281554b663062e0e31666defab@epcas2p1.samsung.com>
        <1647853194-62147-5-git-send-email-dh10.jung@samsung.com>
        <87r16v9uo2.fsf@miraculix.mork.no>
Precedence: bulk

------.rW42XXn7_Ji6OxwuAF4RAMbP.qed.OeqKlvEhSLhB3noATS=_22cbc0_
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: 8bit
Content-Disposition: inline

On Mon, Mar 21, 2022 at 04:45:01PM +0100, Bj?rn Mork wrote:
> Daehwan Jung <dh10.jung@samsung.com> writes:
> 
> > +++ b/drivers/usb/host/xhci-exynos.c
> > @@ -0,0 +1,982 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * xhci-exynos.c - xHCI host controller driver platform Bus Glue for Exynos.
> > + *
> > + * Copyright (C) 2022 Samsung Electronics Incorporated - http://www.samsung.com
> > + * Author: Daehwan Jung <dh10.jung@samsung.com>
> > + *
> > + * A lot of code borrowed from the Linux xHCI driver.
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/usb/phy.h>
> > +#include <linux/slab.h>
> > +#include <linux/acpi.h>
> > +#include <linux/usb/of.h>
> > +
> > +#include "xhci.h"
> > +#include "xhci-plat.h"
> > +#include "xhci-mvebu.h"
> > +#include "xhci-rcar.h"

> 
> The xhci-plat.c file is Copyright (C) 2012 Texas Instruments Incorporated
> You can't just steal it.
> 
> Besides, even if you could, this isn't about copying as much code as
> posible from A to B.  The point is to add as *little* code as possible
> to support your hardware.
> 

Hi bjorn,

Thanks for your comment. I will write more detail about copyright on
next submission.

> > +static int xhci_exynos_vendor_init(struct xhci_hcd *xhci)
> > +{
> > +	/* TODO */
> > +	return 0;
> > +}
> 
> And you didn't even add that?
> 
> > +static int xhci_exynos_wake_lock(struct xhci_hcd_exynos *xhci_exynos,
> > +				   int is_main_hcd, int is_lock)
> > +{
> > +	struct usb_hcd	*hcd = xhci_exynos->hcd;
> > +	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> > +	struct wakeup_source *main_wakelock, *shared_wakelock;
> > +
> > +	main_wakelock = xhci_exynos->main_wakelock;
> > +	shared_wakelock = xhci_exynos->shared_wakelock;
> 
> Are these fields initialized anywhere?
> 
> 
> > +
> > +	if (xhci->xhc_state & XHCI_STATE_REMOVING)
> > +		return -ESHUTDOWN;
> > +
> > +	if (is_lock) {
> 
> bool?
> 

Yes, Currently I use it as if bool. 

> > +		if (is_main_hcd)
> 
> another bool?

Same

> 
> > +			__pm_stay_awake(main_wakelock);
> > +		else
> > +			__pm_stay_awake(shared_wakelock);
> > +	} else {
> > +		if (is_main_hcd)
> > +			__pm_relax(main_wakelock);
> > +		else
> > +			__pm_relax(shared_wakelock);
> > +	}
> 
> Looks interesting.   Are you signalling relax/wakeups events to the PM
> core on device suspend/resume?  Why?

I want to enter system sleep if possible at this point. It's related to
power scenario on my SOC.

> 
> > +static int xhci_exynos_address_device(struct usb_hcd *hcd, struct usb_device *udev)
> > +{
> > +	struct xhci_hcd *xhci;
> > +	int ret;
> > +
> > +	ret = xhci_address_device(hcd, udev);
> > +	xhci = hcd_to_xhci(hcd);
> > +
> > +	return ret;
> > +}
> 
> What's left here if we drop the unused parts?
> 

There's some missing code. Thanks..


> > +#ifdef CONFIG_OF
> > +static const struct xhci_plat_priv xhci_plat_marvell_armada = {
> > +	.init_quirk = xhci_mvebu_mbus_init_quirk,
> > +};
> > +
> > +static const struct xhci_plat_priv xhci_plat_marvell_armada3700 = {
> > +	.plat_setup = xhci_mvebu_a3700_plat_setup,
> > +	.init_quirk = xhci_mvebu_a3700_init_quirk,
> > +};
> 
> 
> Right...
> 
> > +#ifdef CONFIG_ACPI
> > +static const struct acpi_device_id usb_xhci_acpi_match[] = {
> > +	/* XHCI-compliant USB Controller */
> > +	{ "PNP0D10", },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(acpi, usb_xhci_acpi_match);
> > +#endif
> 
> Nice one
> 
> There's no need to copy me if you plan to resend any of this.  I'm just
> a drive-by reader here anyway, and I've seen enough.
> 
> Good luck!
> 
> 
> 
> 
> Bj?rn
> 

------.rW42XXn7_Ji6OxwuAF4RAMbP.qed.OeqKlvEhSLhB3noATS=_22cbc0_
Content-Type: text/plain; charset="utf-8"


------.rW42XXn7_Ji6OxwuAF4RAMbP.qed.OeqKlvEhSLhB3noATS=_22cbc0_--