Re: [PATCH 1/2] drivers: qcom: add command DB driver

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Wed 07 Feb 13:43 PST 2018, Lina Iyer wrote:

> On Wed, Feb 07 2018 at 21:06 +0000, Bjorn Andersson wrote:
> > On Wed 07 Feb 10:29 PST 2018, Lina Iyer wrote:
> > > On Mon, Feb 05 2018 at 23:18 +0000, Bjorn Andersson wrote:
> > > > On Thu 18 Jan 14:08 PST 2018, Lina Iyer wrote:
> > [..]
> > > > > +static u64 cmd_db_get_u64_id(const char *id)
> > > > > +{
> > > > > +	uint64_t rsc_id = 0;
> > > > > +	uint8_t *ch  = (uint8_t *)&rsc_id;
> > > > > +	int i;
> > > > > +
> > > > > +	for (i = 0; ((i < sizeof(rsc_id)) && id[i]); i++)
> > > > > +		ch[i] = id[i];
> > > > > +
> > > > > +	return rsc_id;
> > > > > +}
> > > >
> > > > So this "casts" a 8 byte string to a u64. Why not just use u64 constants
> > > > to represent the resources, as we've done in the past?
> > > >
> > > This matches the resource specification on the remote end for this new
> > > architecture.
> > > 
> > 
> > But are these resource constants going to be used in the Linux kernel?
> > 
> > In previous variants of the RPM we had these too and in some places they
> > where described in their string variant and some in integer constants.
> > Upstream they all became integer constants, typically with a comment.
> > 
> > Unless there's some code sharing here I think we should do the
> > translation offline and encode them as hexadecimal constants.
> > 
> While I agree with the suggestion. I doubt this is making it any easier.
> The general identifier of a resource is a human readadble stringified
> name. It makes understanding the information regarding the resource -
> like the rail name, clock etc much more user friendly. I would like to
> keep it that way. I am quite sure, the driver authors would prefer to,
> as well. This makes it easier when reading through DT.
> 
> Ex: gfx.lvl, ddr.lvl, ddr.tmr as opposed to -
> 0x6c766c2e786667, 0x6c766c2e726464, 0x726d742e726464  respectively.
> 

The way that upstream regulator, clock and the current direction
interconnect is taken these constants are not visible in DT.

The way we have dealt with these constants previously (it's pretty much
the same with previous RPM versions) is to used defines in those
drivers.  E.g. RPM_KEY_{SWEN,UV,MA} in qcom_smd-regulator.c


That said, I don't have a strong opinion about this so feel free to
retain this model if you think it helps those drivers.

[..]
> > > > > +int cmd_db_get_slave_id(const char *resource_id)
> > > > > +{
> > > > > +	int ret;
> > > > > +	struct entry_header ent;
> > > > > +	struct rsc_hdr rsc_hdr;
> > > > > +
> > > > > +	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
> > > > > +	return ret < 0 ? 0 : (ent.addr >> SLAVE_ID_SHIFT) & SLAVE_ID_MASK;
> > > >
> > > > Why is this bit 16-19 of the address? What is ent.addr?! Why isn't this
> > > > rsc_hdr.slv_id?
> > > >
> > > BITS 16:18 indicate what type of slave it is - ARC, BCM, VRM. This
> > > information is part of each entry.
> > > 
> > 
> > Okay, that sounds reasonable.
> > 
> > But what's the value in rsc_hdr.slv_id representing? The "id" of the
> > resource? Why is that a "slave id"?
> > 
> The master is the client that runs on Linux and other processors like
> GPU, Modem etc. The slave is the actual resource like the clock,
> regulator, etc. Hence the name 'slave id'.
> 

Okay, that does make sense.

But if a "slave" is a "resource" it would be convenient if the driver
picked one of the names and stuck with that.

And it sounds like this function should be named get_slave_type() (or
get_resource_type()).


Thanks,
Bjorn