Re: [PATCH 4/5] phy: add Broadcom SATA3 PHY driver for Broadcom STB SoCs

[Kishon Vijay Abraham I <kishon@ti.com>]

On Thursday 02 April 2015 07:58 AM, Brian Norris wrote:
> On Tue, Mar 31, 2015 at 11:31:40AM +0530, Kishon Vijay Abraham I wrote:
>> On Saturday 28 March 2015 05:58 AM, Brian Norris wrote:
>>> On Thu, Mar 26, 2015 at 03:29:44AM +0530, Kishon Vijay Abraham I wrote:
>>>> On Thursday 19 March 2015 06:53 AM, Brian Norris wrote:
>>>>> +static struct phy *brcm_sata_phy_xlate(struct device *dev,
>>>>> +				       struct of_phandle_args *args)
>>>>> +{
>>>>> +	struct brcm_sata_phy *priv = dev_get_drvdata(dev);
>>>>> +	int i = args->args[0];
>>>>> +
>>>>> +	if (i >= MAX_PORTS || !priv->phys[i].phy) {
>>>>> +		dev_err(dev, "invalid phy: %d\n", i);
>>>>> +		return ERR_PTR(-ENODEV);
>>>>> +	}
>>>>> +
>>>>> +	return priv->phys[i].phy;
>>>>> +}
>>>>
>>>> this xlate is not required at all if the controller device tree node has
>>>> phandle to the phy node (sub node) instead of the phy provider device tree
>>>> node.
>>>
>>> That doesn't match any convention I see in existing SATA phy bindings,
>>> nor do I see how the existing of_phy_simple_xlate() would support this,
>>> unless I instantiate a device for each port's PHY. If I adjust the
>>> device tree as you suggest, and use of_phy_simple_xlate() instead of
>>> this, of_phy_get() can't find the PHY provider, because the provider is
>>> registered to the parent, not the subnode.
>>
>> The phy core should still be able to get the PHY provider.
>> See this in of_phy_provider_lookup
>>                  for_each_child_of_node(phy_provider->dev->of_node, child)
>>                          if (child == node)
>>                                  return phy_provider;
>
> That just searches for children of the node. It doesn't walk parent
> nodes.

okay.. in your phy_create pass the np of the PHYs (sub-node pointer to phy 
provider).

>
>> Can you post your device tree node here?
>
> You mean patch 5?
>
> https://lkml.org/lkml/2015/3/18/937
>
>>>
>>> Can you elaborate on your suggestion?

Change the dt node to something like below..

+
+		sata@f045a000 {
+			<snip>
+
+			sata0: sata-port@0 {
+				reg = <0>;
+				phys = <&sata_phy0>;
+			};
+
+			sata1: sata-port@1 {
+				reg = <1>;
+				phys = <&sata_phy1>;
+			};
+		};
+
+		sata_phy: sata-phy@f0458100 {
+			compatible = "brcm,bcm7445-sata-phy", "brcm,phy-sata3";
+			reg = <0x458100 0x1e00>, <0x45804c 0x10>;
+			reg-names = "phy", "port-ctrl";
+			#address-cells = <0x1>;
+			#size-cells = <0x0>;
+
+			sata_phy0: sata-phy@0 {
+				reg = <0>;
+				#phy-cells = <0>;
+			};
+
+			sata_phy1: sata-phy@1 {
+				#phy-cells = <0>;
+			};
+		};
  	};
>>>
>>>>> +
>>>>> +static const struct of_device_id brcmstb_sata_phy_of_match[] = {
>>>>> +	{ .compatible	= "brcm,bcm7445-sata-phy" },
>>>>> +	{},
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(of, brcmstb_sata_phy_of_match);
>>>>> +
>>>>> +static int brcmstb_sata_phy_probe(struct platform_device *pdev)
>>>>> +{
>>>>> +	struct device *dev = &pdev->dev;
>>>>> +	struct device_node *dn = dev->of_node, *child;
>>>>> +	struct brcm_sata_phy *priv;
>>>>> +	struct resource *res;
>>>>> +	struct phy_provider *provider;
>>>>> +	int count = 0;
>>>>> +
>>>>> +	if (of_get_child_count(dn) == 0)
>>>>> +		return -ENODEV;
>>>>> +
>>>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>>> +	if (!priv)
>>>>> +		return -ENOMEM;
>>>>> +	dev_set_drvdata(dev, priv);
>>>>> +	priv->dev = dev;
>>>>> +
>>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "port-ctrl");
>>>>> +	if (!res) {
>>>>> +		dev_err(dev, "couldn't get port-ctrl resource\n");
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +	/*
>>>>> +	 * Don't request region, since it may be within a region owned by the
>>>>> +	 * SATA driver
>>>>
>>>> It should be in the SATA driver then. Why is it here?
>>>
>>> Did you read the discussion branching here?
>>>
>>> http://article.gmane.org/gmane.linux.drivers.devicetree/114637
>>>
>>> I've seen the exact opposite suggestion already (move it to the PHY
>>> driver), and I'm not sure either suggestion is correct. The same
>>> register block has registers for both the PHY and the SATA controller.
>>
>> IMHO the register space shouldn't be defined based on the programming sequence
>> but by where the register is actually present in the IP. From that thread it
>> doesn't look like it is present in the PHY IP.
>
> If you say so. But it has plenty of PHY controls packed into those
> registers, so are you just recommending handling those bits from the
> SATA driver?

Yes. Let's program only the PHY IP in this driver.
>
> ...
>
>>>> lets not make the boot noisy. Make it dev_vdbg if it is required.
>>>
>>> I think it's important to have at least some of the three informational
>>> prints that you're suggesting turn into dbg. It's pretty important to
>>> see that we're powering on one or more PHY ports, for both
>>> power/correctness concerns (trying to power on a port that is
>>> OTP-disabled, for instance) and debugging concerns (the suggestions you
>>> made about the device tree yielded a dead SATA, and it was obvious,
>>> because the "powering on" prints were missing).
>>
>> All these are debugging info. Hence it's better to keep in dev_vdbg or dev_dbg.
>>>
>>> I'd kinda like to see the previous power on/off prints above stay as
>>> dev_info(), though the "registered" print might be superfluous, as the
>>> registration info should show up in sysfs.
>>>
>>> Related: I don't see any API for monitoring the PHY status from user
>>> space. e.g., there's nothing useful in /sys/class/phy/*/.
>>
>> Do you really need to monitor the PHY status? We should be careful about
>> exposing anything to the user space since it will become an ABI and we can
>> never modify it.
>
> Not really, but the debugging info (which you want me to remove by
> default, and which is unretrievable after system boot) is the next

I didn't ask you to remove it. I just asked you to keep in dev_dbg. If you are 
interested in the debugging info, all you need to do is change the debug log level.

Cheers
Kishon