[v9,3/7] ata: libahci: allow to use multiple PHYs

Commit Message

Antoine Tenart July 9, 2014, 8:23 a.m. UTC

Hi Tejun,

On Tue, Jul 08, 2014 at 05:40:10PM -0400, Tejun Heo wrote:
> Hey,
> 
> On Tue, Jul 08, 2014 at 07:49:00PM +0200, Antoine Ténart wrote:
> > > So, yeah, it's being used both as input and output and we also have
> > > the arguments which affect port_map, right?  It does seem confusing.
> > 
> > I do see priv->port_map as being automatically set and then restricted
> > if needed by the port_map input here. I don't see how that's confusing.
> > The only modification is we restrict the port_map parameter to the set
> > of available ports. The port_map argument affected priv->port_map before
> > this patch.
> 
> It is confusing.  If you wanna pass around available ports in hpriv,
> please add a separate field and replace the arguments to
> save_initial_config().

I don't get it. Which argument should I replace in
save_initial_config()? The change is we compute hpriv->port_map.
I don't see which arguments we can add or replace.

If that's more clear, the modification here is equivalent to (when using
the new bindings):


It just use the argument port_map on the real set of ports. If that's
still not clear, I can add a separate field. But the field will store
the exact same information as hpriv->port_map. We will have something
like:

if (hpriv->available_port_map)
	hpriv->port_map &= hpriv->available_port_map;

> > > Well, so does clk.  Let's say clk is more restricted and phy can be
> > > one or more per port and thus needs to be dynamic.  If so, shouldn't
> > > we at least have some correlation between phys and ports?  It bothers
> > > me that now libahci is carrying random number of resources that it has
> > > no idea how to associate with the ports it manages.  What if later we
> > > want to involve phy driver in power managing unoccupied ports?
> > 
> > I see. This is a first (working) attempt to have a one node per port. I
> > agree that would be nice to have a correlation between ports and PHYs.
> > This can definitively be added when needed without changing the dt
> > bindings as only the internal representation changes. This would also
> > require to get all phys from the port nodes, which is again internal
> > stuff.
> > 
> > Don't you think we can go by steps, and have a following up series for
> > this when needed (like in a power managing series for unoccupied ports)?
> 
> I don't know.  It isn't exactly difficult to make it per-port, is it?
> We already have ahci_port_priv and wouldn't the code actually be
> simpler that way?

I had a quick look on this, and it does not seems to be that simple. The
ahci_port_priv is stored inside the ata_port struct and not accessible
(as of now) from the ahci_host_priv one. The ahci_port_priv is
initialized at the end of ahci_platform_init_host(), far after we need
it. This requires quite a lot of changes. Or is there another way?

To be honest, we are now at v9 and it's been quite a long time since v1.
I'd really like it to be merged in 3.17. As I see it, this patch keeps
the same logic as what was in place before, only with more PHYs.

Don't take me wrong, I really think this is a good idea to have a
per-port PHY information. But this is a refactoring not clearly related
to this series as the logic is not changed. This definitively can be the
subject of a dedicated series, especially if I got it right and the
required modifications are not that obvious.

What do you think?

Thanks,

Antoine

Comments

Tejun Heo July 9, 2014, 1:59 p.m. UTC | #1

Hello,

On Wed, Jul 09, 2014 at 10:23:31AM +0200, Antoine Ténart wrote:
> > It is confusing.  If you wanna pass around available ports in hpriv,
> > please add a separate field and replace the arguments to
> > save_initial_config().
> 
> I don't get it. Which argument should I replace in
> save_initial_config()? The change is we compute hpriv->port_map.
> I don't see which arguments we can add or replace.

The @force_port_map and @mask_port_map of ahci_save_initial_config().
We end up with three params from two places modifying port_map and one
of those is in/out parameter, so ummm, no.  If you wanna add port
masking to @hpriv, please do it by moving @force_port_map and
@mask_port_map into @hpriv instead.

Sure, the proposed change is small but the end result is messy.

> I had a quick look on this, and it does not seems to be that simple. The
> ahci_port_priv is stored inside the ata_port struct and not accessible
> (as of now) from the ahci_host_priv one. The ahci_port_priv is
> initialized at the end of ahci_platform_init_host(), far after we need
> it. This requires quite a lot of changes. Or is there another way?

Yeah, it'd probably need separating out port resource handling, so
that the order is get_resources, host_alloc, get_port_resources and
then init and activate.  Hans, what do you think?

> To be honest, we are now at v9 and it's been quite a long time since v1.
> I'd really like it to be merged in 3.17. As I see it, this patch keeps
> the same logic as what was in place before, only with more PHYs.
> 
> Don't take me wrong, I really think this is a good idea to have a
> per-port PHY information. But this is a refactoring not clearly related
> to this series as the logic is not changed. This definitively can be the
> subject of a dedicated series, especially if I got it right and the
> required modifications are not that obvious.

Heh, I get that but, at the same time, this is the point where you're
most motivated to actually work on it.  :)

Let's see how much work it's gonna be.

Thanks.

Hans de Goede July 9, 2014, 2:02 p.m. UTC | #2

Hi,

On 07/09/2014 03:59 PM, Tejun Heo wrote:
> Hello,
> 
> On Wed, Jul 09, 2014 at 10:23:31AM +0200, Antoine Ténart wrote:
>>> It is confusing.  If you wanna pass around available ports in hpriv,
>>> please add a separate field and replace the arguments to
>>> save_initial_config().
>>
>> I don't get it. Which argument should I replace in
>> save_initial_config()? The change is we compute hpriv->port_map.
>> I don't see which arguments we can add or replace.
> 
> The @force_port_map and @mask_port_map of ahci_save_initial_config().
> We end up with three params from two places modifying port_map and one
> of those is in/out parameter, so ummm, no.  If you wanna add port
> masking to @hpriv, please do it by moving @force_port_map and
> @mask_port_map into @hpriv instead.
> 
> Sure, the proposed change is small but the end result is messy.
> 
>> I had a quick look on this, and it does not seems to be that simple. The
>> ahci_port_priv is stored inside the ata_port struct and not accessible
>> (as of now) from the ahci_host_priv one. The ahci_port_priv is
>> initialized at the end of ahci_platform_init_host(), far after we need
>> it. This requires quite a lot of changes. Or is there another way?
> 
> Yeah, it'd probably need separating out port resource handling, so
> that the order is get_resources, host_alloc, get_port_resources and
> then init and activate.  Hans, what do you think?

The order (and function names) you're suggesting here sound good to me.

Regards,

Hans

Alexandre Belloni July 9, 2014, 3:24 p.m. UTC | #3

Hi,

On 09/07/2014 at 09:59:10 -0400, Tejun Heo wrote :
> Heh, I get that but, at the same time, this is the point where you're
> most motivated to actually work on it.  :)
> 

Or, starting that kind of review after 9 revisions, especially so close
to the end of the development cycle is discouraging enough and will
probably be used as an example of why we still have so many evil vendor
trees.

Tejun Heo July 9, 2014, 3:42 p.m. UTC | #4

On Wed, Jul 09, 2014 at 05:24:46PM +0200, Alexandre Belloni wrote:
> Hi,
> 
> On 09/07/2014 at 09:59:10 -0400, Tejun Heo wrote :
> > Heh, I get that but, at the same time, this is the point where you're
> > most motivated to actually work on it.  :)
> > 
> 
> Or, starting that kind of review after 9 revisions, especially so close
> to the end of the development cycle is discouraging enough and will
> probably be used as an example of why we still have so many evil vendor
> trees.

Yeap, that's my bad.  I should have paid more attention.  Sorry about
that.  We still have quite a few weeks to go tho, so I don't think
it's too unreasonable to at least look into it.  We're gonna have to
do it anyway.

Thanks.

Patch

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 40ea583d3610..f9d3cfd5d1bd 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -513,7 +513,7 @@  void ahci_save_initial_config(struct device *dev,
        /* record values to use during operation */
        hpriv->cap = cap;
        hpriv->cap2 = cap2;
-       hpriv->port_map = port_map;
+       hpriv->port_map &= port_map;
 
        if (!hpriv->start_engine)
                hpriv->start_engine = ahci_start_engine;