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.
[..]
> > +
> > +int cmd_db_get_aux_data(const char *resource_id, u8 *data, int len)
>
> "cmd_db_read_data()" would better represent that we're copying the data
> into the buffer.
>
>
> (And the "aux" part in these function names doesn't seem to add any
> value)
>
That is how it is represented in the architecture document. It is more
helpful for the driver author to maintain relation with the database
that is being read.
Okay so it's not "the data" that being read but "the auxiliary data"
according to the command db specification. I guess that's fine then, but
think it's a "read" and not a "get" in Linux.
I can change to read_ instead of get_.
[..]
> > +int cmd_db_ready(void)
> > +{
> > + return cmd_db_status;
> > +}
> > +EXPORT_SYMBOL(cmd_db_ready);
>
> Move this function one step down, to keep the getters together.
>
> Based on the function name this function should return a bool.
>
Moved it up as I plan to reuse this function in
cmd_db_get_header_by_rsc_id().
Sounds good, but I presume we still need this function exported in order
for client drivers to be able to probe defer.
Yes.
> > +
> > +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'.
[..]
> > + res.start = readl_relaxed(dict);
> > + res.end = res.start + readl_relaxed(dict + 0x4);
> > + res.flags = IORESOURCE_MEM;
> > + iounmap(dict);
>
> Why can't we just describe the memory directly?
>
We could. Just that it is how the memory is defined in the remote end.
I think it's fine that the apps system as a whole implements this
functionality, but I think Linux should just have the real memory region
described in the reg.
And as the reserved-memory region seems to be the only resource for this
thing we should be able to describe it similar to the qcom,rmtfs-mem -
where the reserved-memory node has a compatible and is implemented by
the qcom_rmtfs_mem driver, without additional nodes referencing it.
Saw your other email in this regard. Will use reserved memory node.
> > +
> > + start_addr = devm_ioremap_resource(&pdev->dev, &res);
>
> No error handling?
>
Will fix.
As I said in my other reply (sorry for spreading this out), as this is
system ram, this should be a memremap()
Sure.
Thanks,
Lina
--
To unsubscribe from this list: send the line "unsubscribe linux-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
