Hi Heikki,
Sorry I am starting a new branch on this thread.
I need to go back to another topic on this same patch.
On Fri, Jan 23, 2015 at 05:12:58PM +0200, Heikki Krogerus wrote:
> TUSB1210 ULPI PHY has vendor specific register for eye
> diagram tuning. On some platforms the system firmware has
> set optimized value to it. In order to not loose the
> optimized value, the driver stores it during probe and
> restores it every time the PHY is powered back on.
>
> Signed-off-by: Heikki Krogerus <[email protected]>
> Cc: Kishon Vijay Abraham I <[email protected]>
> ---
[snip]
> + /* Store initial eye diagram optimisation value */
> + ret = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
We can't rely on eye diagram optimization value here. It's possible the
phy went through reset (e.g. module unloading/loading).
We need a more consistent method. Could we use _DSD instead? That would
be more compatible with DT too.
Br, David
> + if (ret < 0)
> + return ret;
> +
> + tusb->eye_diagram_tuning = ret;
> +
> + tusb->phy = ulpi_phy_create(ulpi, &phy_ops);
> + if (IS_ERR(tusb->phy))
> + return PTR_ERR(tusb->phy);
> +
> + tusb->ulpi = ulpi;
> +
> + phy_set_drvdata(tusb->phy, tusb);
> + ulpi_set_drvdata(ulpi, tusb);
> + return 0;
> +}
> +
Hi,
On Thu, Feb 12, 2015 at 05:52:42PM -0800, David Cohen wrote:
> Hi Heikki,
>
> Sorry I am starting a new branch on this thread.
> I need to go back to another topic on this same patch.
>
> On Fri, Jan 23, 2015 at 05:12:58PM +0200, Heikki Krogerus wrote:
> > TUSB1210 ULPI PHY has vendor specific register for eye
> > diagram tuning. On some platforms the system firmware has
> > set optimized value to it. In order to not loose the
> > optimized value, the driver stores it during probe and
> > restores it every time the PHY is powered back on.
> >
> > Signed-off-by: Heikki Krogerus <[email protected]>
> > Cc: Kishon Vijay Abraham I <[email protected]>
> > ---
>
> [snip]
>
> > + /* Store initial eye diagram optimisation value */
> > + ret = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
>
> We can't rely on eye diagram optimization value here. It's possible the
> phy went through reset (e.g. module unloading/loading).
> We need a more consistent method. Could we use _DSD instead? That would
> be more compatible with DT too.
I'm don't have any problem with getting the value from a property.
Sounds like the correct thing to do. Are you thinking about putting
that _DSD method to the ACPI device object for the dwc3? The correct
place would probable be separate device object for the PHY, but if we
do that we need to consider how that object is bound to the ulpi
device. There are few ways we can do that, so it requires some
thinking.
In any case this driver we can already implement so that it expects to
get the value from property. We just need the name for it.
The register has actually separate fields for High speed output
impedance configuration and High speed output driver strength
configuration for eye diagram tuning, plus control bit for DP/DM swap.
Maybe we should actually get them from three separate properties:
device_property_read_u8(dev, "datapolarity", &val);
...
device_property_read_u8(dev, "zhsdrv", &val);
...
device_property_read_u8(dev, "ihstx", &val);
...
What do you think?
Cheers,
--
heikki
On Fri, Feb 13, 2015 at 02:35:16PM +0200, Heikki Krogerus wrote:
> Hi,
>
> On Thu, Feb 12, 2015 at 05:52:42PM -0800, David Cohen wrote:
> > Hi Heikki,
> >
> > Sorry I am starting a new branch on this thread.
> > I need to go back to another topic on this same patch.
> >
> > On Fri, Jan 23, 2015 at 05:12:58PM +0200, Heikki Krogerus wrote:
> > > TUSB1210 ULPI PHY has vendor specific register for eye
> > > diagram tuning. On some platforms the system firmware has
> > > set optimized value to it. In order to not loose the
> > > optimized value, the driver stores it during probe and
> > > restores it every time the PHY is powered back on.
> > >
> > > Signed-off-by: Heikki Krogerus <[email protected]>
> > > Cc: Kishon Vijay Abraham I <[email protected]>
> > > ---
> >
> > [snip]
> >
> > > + /* Store initial eye diagram optimisation value */
> > > + ret = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
> >
> > We can't rely on eye diagram optimization value here. It's possible the
> > phy went through reset (e.g. module unloading/loading).
> > We need a more consistent method. Could we use _DSD instead? That would
> > be more compatible with DT too.
>
> I'm don't have any problem with getting the value from a property.
> Sounds like the correct thing to do. Are you thinking about putting
> that _DSD method to the ACPI device object for the dwc3? The correct
> place would probable be separate device object for the PHY, but if we
> do that we need to consider how that object is bound to the ulpi
> device. There are few ways we can do that, so it requires some
> thinking.
>
> In any case this driver we can already implement so that it expects to
> get the value from property. We just need the name for it.
>
> The register has actually separate fields for High speed output
> impedance configuration and High speed output driver strength
> configuration for eye diagram tuning, plus control bit for DP/DM swap.
>
> Maybe we should actually get them from three separate properties:
>
> device_property_read_u8(dev, "datapolarity", &val);
> ...
> device_property_read_u8(dev, "zhsdrv", &val);
> ...
> device_property_read_u8(dev, "ihstx", &val);
> ...
>
> What do you think?
it's probably better to pass each value and let the driver calculate the
register contents like this.
--
balbi