Re: [PATCH v1 5/8] usb: chipidea: tegra: Support host mode

[Dmitry Osipenko <digetx@gmail.com>]

16.12.2020 09:08, Peter Chen пишет:
> On 20-12-15 23:21:10, Dmitry Osipenko wrote:
>> From: Peter Geis <pgwipeout@gmail.com>
>>  
>>  struct tegra_usb_soc_info {
>>  	unsigned long flags;
>> +	unsigned int txfifothresh;
>> +	enum usb_dr_mode dr_mode;
>> +};
>> +
>> +static const struct tegra_usb_soc_info tegra20_ehci_soc_info = {
>> +	.flags = CI_HDRC_REQUIRES_ALIGNED_DMA |
>> +		 CI_HDRC_OVERRIDE_PHY_CONTROL,
>> +	.dr_mode = USB_DR_MODE_HOST,
>> +	.txfifothresh = 10,
>> +};
>> +
>> +static const struct tegra_usb_soc_info tegra30_ehci_soc_info = {
>> +	.flags = CI_HDRC_REQUIRES_ALIGNED_DMA |
>> +		 CI_HDRC_OVERRIDE_PHY_CONTROL
>> +	.dr_mode = USB_DR_MODE_HOST,
>> +	.txfifothresh = 16,
>>  };
>>  
>>  static const struct tegra_usb_soc_info tegra_udc_soc_info = {
>> -	.flags = CI_HDRC_REQUIRES_ALIGNED_DMA,
>> +	.flags = CI_HDRC_REQUIRES_ALIGNED_DMA |
>> +		 CI_HDRC_OVERRIDE_PHY_CONTROL,
>> +	.dr_mode = USB_DR_MODE_UNKNOWN,
>>  };
> 
> What's the usage for this dr_mode? Does these three SoCs only supports
> single mode, it usually sets dr_mode at board dts file to indicate
> USB role for this board.

All Tegra SoCs have three USB controllers.  Only one of these three
controllers supports peripheral / OTG modes, the other two controllers
are fixed to the host mode and device trees do not specify the dr_mode
for the host controllers.

Hence we need to enforce the dr_mode=host for the host-mode controllers.

For UDC the default mode is "unknown" because actual mode comes from a
board's device tree.

>>  static const struct of_device_id tegra_usb_of_match[] = {
>>  	{
>> +		.compatible = "nvidia,tegra20-ehci",
>> +		.data = &tegra20_ehci_soc_info,
>> +	}, {
>> +		.compatible = "nvidia,tegra30-ehci",
>> +		.data = &tegra30_ehci_soc_info,
>> +	}, {
>>  		.compatible = "nvidia,tegra20-udc",
>>  		.data = &tegra_udc_soc_info,
> 
> Your compatible indicates this controller is single USB role, is it
> on purpose?

Yes, the "tegra20/30-ehci" controllers are fixed to the host-mode in
hardware.

...
>> +static int tegra_usb_internal_port_reset(struct ehci_hcd *ehci,
>> +					 u32 __iomem *portsc_reg,
>> +					 unsigned long *flags)
>> +{
>> +	u32 saved_usbintr, temp;
>> +	unsigned int i, tries;
>> +	int retval = 0;
>> +
>> +	saved_usbintr = ehci_readl(ehci, &ehci->regs->intr_enable);
>> +	/* disable USB interrupt */
>> +	ehci_writel(ehci, 0, &ehci->regs->intr_enable);
>> +	spin_unlock_irqrestore(&ehci->lock, *flags);
>> +
>> +	/*
>> +	 * Here we have to do Port Reset at most twice for
>> +	 * Port Enable bit to be set.
>> +	 */
>> +	for (i = 0; i < 2; i++) {
>> +		temp = ehci_readl(ehci, portsc_reg);
>> +		temp |= PORT_RESET;
>> +		ehci_writel(ehci, temp, portsc_reg);
>> +		mdelay(10);
> 
> Needs accurate delay? How about usleep_range?

To be hones I don't know for sure whether hub_control() could be used
from interrupt context.  This mdelay is borrowed from the older
ehci-tegra driver.

Are you suggesting that it should be safe to sleep here?

...
>>  static int tegra_usb_probe(struct platform_device *pdev)
>>  {
>>  	const struct tegra_usb_soc_info *soc;
>> @@ -83,24 +292,49 @@ static int tegra_usb_probe(struct platform_device *pdev)
>>  		return err;
>>  	}
>>  
>> +	if (device_property_present(&pdev->dev, "nvidia,needs-double-reset"))
>> +		usb->needs_double_reset = true;
>> +
>> +	err = tegra_usb_reset_controller(&pdev->dev);
>> +	if (err) {
>> +		dev_err(&pdev->dev, "failed to reset controller: %d\n", err);
>> +		goto fail_power_off;
>> +	}
>> +
>> +	/*
>> +	 * USB controller registers shan't be touched before PHY is
> 
> %s/shan't/shouldn't

ok

...
>>  static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
>>  {
>>  	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
>> @@ -160,14 +166,14 @@ static int host_start(struct ci_hdrc *ci)
>>  		pinctrl_select_state(ci->platdata->pctl,
>>  				     ci->platdata->pins_host);
>>  
>> +	ci->hcd = hcd;
>> +
>>  	ret = usb_add_hcd(hcd, 0, 0);
>>  	if (ret) {
>>  		goto disable_reg;
>>  	} else {
>>  		struct usb_otg *otg = &ci->otg;
>>  
>> -		ci->hcd = hcd;
>> -
> 
> Why this changed?

The ci->hcd is used by tegra_usb_notify_event() to handle reset event
and the reset happens once usb_add_hcd() is invoked.  Hence we need to
pre-assign the hcd pointer, otherwise there will be a NULL dereference.