From: Dmitry Osipenko <digetx@gmail.com>
To: Peter Chen <peter.chen@nxp.com>
Cc: Rob Herring <robh+dt@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Thierry Reding <thierry.reding@gmail.com>,
Jonathan Hunter <jonathanh@nvidia.com>,
Felipe Balbi <balbi@kernel.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
"linux-tegra@vger.kernel.org" <linux-tegra@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Peter Geis <pgwipeout@gmail.com>
Subject: Re: [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies
Date: Tue, 24 Dec 2019 07:21:28 +0300 [thread overview]
Message-ID: <f549b1f8-bc36-1a79-2f8a-042c7ee5fb56@gmail.com> (raw)
In-Reply-To: <VI1PR04MB5327ADFDE053ACA5ABAA85708B290@VI1PR04MB5327.eurprd04.prod.outlook.com>
24.12.2019 05:54, Peter Chen пишет:
>
>>
>> 23.12.2019 09:40, Peter Chen пишет:
>>> On 19-12-20 07:31:08, Dmitry Osipenko wrote:
>>>> 20.12.2019 06:56, Peter Chen пишет:
>>>>> On 19-12-20 04:52:38, Dmitry Osipenko wrote:
>>>>>> Now, when ci_hdrc_tegra kernel module is loaded, the phy_tegra_usb
>>>>>> module is loaded too regardless of kernel's configuration.
>>>>>> Previously this problem was masked because Tegra's EHCI driver is
>>>>>> usually enabled in kernel's config and thus PHY driver was getting
>>>>>> loaded because of it, but now I was making some more thorough
>>>>>> testing and noticed that PHY's module isn't getting auto-loaded without the
>> host driver.
>>>>>>
>>>>>> Note that ChipIdea's driver doesn't use any of the exported
>>>>>> functions of phy_tegra_usb module and thus the module needs to be
>> requested explicitly.
>>>>>>
>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>> ---
>>>>>> drivers/usb/chipidea/Kconfig | 1 +
>>>>>> drivers/usb/chipidea/ci_hdrc_tegra.c | 6 ++++++
>>>>>> 2 files changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/usb/chipidea/Kconfig
>>>>>> b/drivers/usb/chipidea/Kconfig index ae850b3fddf2..d53db520e209
>>>>>> 100644
>>>>>> --- a/drivers/usb/chipidea/Kconfig
>>>>>> +++ b/drivers/usb/chipidea/Kconfig
>>>>>> @@ -7,6 +7,7 @@ config USB_CHIPIDEA
>>>>>> select RESET_CONTROLLER
>>>>>> select USB_ULPI_BUS
>>>>>> select USB_ROLE_SWITCH
>>>>>> + select USB_TEGRA_PHY if ARCH_TEGRA
>>>>>> help
>>>>>> Say Y here if your system has a dual role high speed USB
>>>>>> controller based on ChipIdea silicon IP. It supports:
>>>>>> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>>>> b/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>>>> index 7455df0ede49..8bc11100245d 100644
>>>>>> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>>>> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>>>> @@ -53,6 +53,12 @@ static int tegra_udc_probe(struct platform_device
>> *pdev)
>>>>>> struct tegra_udc *udc;
>>>>>> int err;
>>>>>>
>>>>>> + if (IS_MODULE(CONFIG_USB_TEGRA_PHY)) {
>>>>>> + err = request_module("phy_tegra_usb");
>>>>>> + if (err)
>>>>>> + return err;
>>>>>> + }
>>>>>> +
>>>>>
>>>>> Why you do this dependency, if this controller driver can't get USB
>>>>> PHY, it should return error. What's the return value after calling
>>>>> below:
>>>>>
>>>>> udc->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "nvidia,phy",
>>>>> 0);
>>>>
>>>> It returns -EPROBE_DEFER when phy_tegra_usb isn't loaded.
>>>>
>>>> So if you'll do:
>>>>
>>>> # rmmod ci_hdrc_tegra; rmmod ci_hdrc; rmmod phy_tegra_usb; # modprobe
>>>> ci_hdrc_tegra # lsmod
>>>> Module Size Used by
>>>> ci_hdrc_tegra 16384 0
>>>> ci_hdrc 45056 1 ci_hdrc_tegra
>>>>
>>>> After this patch:
>>>>
>>>> # rmmod ci_hdrc_tegra; rmmod ci_hdrc; rmmod phy_tegra_usb; # modprobe
>>>> ci_hdrc_tegra # lsmod
>>>> Module Size Used by
>>>> Module Size Used by
>>>> phy_tegra_usb 20480 1
>>>> ci_hdrc_tegra 16384 0
>>>> ci_hdrc 45056 1 ci_hdrc_tegra
>>>
>>> I wonder why the driver needs such dependency? If there are two phy
>>> drivers could work with this controller driver, you may request two
>>> modules.
>>
>> Well, if somebody wants to use some PHY driver other than the upstream's
>> standard one, then that person could simply load the custom driver module first,
>> such that it will bind to the PHY's device first.
>>
>> It is also possible to manually unbind the standard driver from PHY's device and
>> then bind whatever driver you want.
>>
>>> Doesn't such dependency should be done by the board level script?
>>
>> This patch only improves the default behaviour that is common for all NVIDIA Tegra
>> boards, it doesn't prevent from doing any special customizations.
>>
>> Perhaps the Kconfig change could be dropped from this patch in order to provide a
>> bit more flexibility in regards to kernel's configuration, but I'm very doubtful that
>> realistically anyone would want to replace the default driver with anything else on
>> Tegra. The Kconfig change also puts ChipIdea's UDC driver in line with the Tegra's
>> EHCI driver that selects USB_TEGRA_PHY, please see drivers/usb/host/Kconfig.
>>
>>> Do you know are there any other drivers do such things?
>>
>> I don't think that any of the USB host drivers are currently doing such things, but in
>> general there are quite a lot of drivers in kernel that are using request_module [1].
>>
>> [1]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.c
>> om%2Flinux%2Flatest%2Fident%2Frequest_module&data=02%7C01%7Cpete
>> r.chen%40nxp.com%7Ca153b9e4d81044cde3c108d787ccdcb9%7C686ea1d3bc2b
>> 4c6fa92cd99c5c301635%7C0%7C0%7C637127186257612269&sdata=xOVnn
>> bVGRVhypbiMWpt2MUfcYayAl4ywpCa7xOAQ1vk%3D&reserved=0
>>
>> Please note that drivers/usb/host/ehci-tegra.c uses exported symbols from
>> usb/phy/phy-tegra-usb.c and that is why the EHCI driver doesn't need to explicitly
>> load the phy_tegra_usb module, the load happens automatically because of the
>> missing symbols.
>>
>> Also, please note that it is possible to squash the Tegra EHCI driver into
>> ci_hdrc_tegra.c and then the explicit dependency on the phy_tegra_usb won't be
>> needed anymore since it will be replaced with an implicit dependency. We (me and
>> Peter Geis) already had some experimental patches that do the successful
>> squashing of the drivers, but looks like Peter got sidetracked for a more important
>> things for now, we'll probably return to that work later on.
>
> Hi Dmitry,
>
> Thanks for explaining it. In fact, your case is very common for USB since PHY driver
> and controller driver are two independent drivers. If you have no other ways
> to fix this dependency issue, it is ok to add it at driver.
Hello Peter,
For now I'm not aware of any alternatives to this patch, thanks.
BTW, I'll make a v3 of this series because found more things that could
be improved in the Tegra's PHY driver.
next prev parent reply other threads:[~2019-12-24 4:21 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-20 1:52 [PATCH v2 00/10] NVIDIA Tegra USB2 drivers clean up Dmitry Osipenko
2019-12-20 1:52 ` [PATCH v2 01/10] dt-binding: usb: ci-hdrc-usb2: Document NVIDIA Tegra support Dmitry Osipenko
2019-12-20 1:52 ` [PATCH v2 02/10] usb: phy: tegra: Hook up init/shutdown callbacks Dmitry Osipenko
2019-12-20 1:52 ` [PATCH v2 03/10] usb: phy: tegra: Perform general clean up of the code Dmitry Osipenko
2019-12-20 1:52 ` [PATCH v2 04/10] usb: phy: tegra: Use relaxed versions of readl/writel Dmitry Osipenko
2019-12-20 1:52 ` [PATCH v2 05/10] usb: phy: tegra: Use generic stub for a missing VBUS regulator Dmitry Osipenko
2019-12-20 1:52 ` [PATCH v2 06/10] usb: ulpi: Add resource-managed variant of otg_ulpi_create() Dmitry Osipenko
2019-12-20 1:52 ` [PATCH v2 07/10] usb: phy: tegra: Use devm_otg_ulpi_create() Dmitry Osipenko
2019-12-20 1:52 ` [PATCH v2 08/10] usb: phy: tegra: Use u32 for hardware register variables Dmitry Osipenko
2019-12-22 13:24 ` Dejin Zheng
2019-12-22 21:48 ` Dmitry Osipenko
2019-12-23 14:53 ` Dejin Zheng
2019-12-20 1:52 ` [PATCH v2 09/10] usb: chipidea: tegra: Stop managing PHY's power Dmitry Osipenko
2019-12-20 1:52 ` [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies Dmitry Osipenko
2019-12-20 3:56 ` Peter Chen
2019-12-20 4:31 ` Dmitry Osipenko
2019-12-23 6:40 ` Peter Chen
2019-12-23 17:23 ` Dmitry Osipenko
2019-12-24 2:54 ` Peter Chen
2019-12-24 4:21 ` Dmitry Osipenko [this message]
2019-12-23 21:32 ` Michał Mirosław
2019-12-24 4:21 ` Dmitry Osipenko
2019-12-30 21:02 ` Michał Mirosław
2020-01-02 15:17 ` Dmitry Osipenko
2020-01-03 7:25 ` Michał Mirosław
2020-01-03 23:19 ` Dmitry Osipenko
2020-01-04 11:01 ` Michał Mirosław
2020-01-05 0:42 ` Dmitry Osipenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f549b1f8-bc36-1a79-2f8a-042c7ee5fb56@gmail.com \
--to=digetx@gmail.com \
--cc=balbi@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jonathanh@nvidia.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=peter.chen@nxp.com \
--cc=pgwipeout@gmail.com \
--cc=robh+dt@kernel.org \
--cc=thierry.reding@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).