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.
> > > +
> > > +static int cmd_db_get_header(u64 query, struct entry_header *eh,
> > > + struct rsc_hdr *rh, bool use_addr)
> >
> > This function is static and there's only one caller, which has use_addr
> > as false. So please omit this parameter for now.
> >
> Ok.
> > > +{
> > > + struct rsc_hdr *rsc_hdr;
> > > + int i, j;
> > > +
> > > + if (!cmd_db_header)
> > > + return -EPROBE_DEFER;
> > > +
> > > + if (!eh || !rh)
> > > + return -EINVAL;
> > > +
> > > + rsc_hdr = &cmd_db_header->header[0];
> >
> > Rather than bumping the pointer in the loop, just move this inside the
> > loop as:
> > rsc_hdr = &cmd_db_header->header[i];
> >
> Sure.
>
> > > +
> > > + for (i = 0; i < MAX_SLV_ID ; i++, rsc_hdr++) {
> > > + struct entry_header *ent;
> > > +
> > > + if (!rsc_hdr->slv_id)
> > > + break;
> > > +
> > > + ent = (struct entry_header *)(start_addr
> > > + + sizeof(*cmd_db_header)
> > > + + rsc_hdr->header_offset);
> >
> > If you have start_addr expressed as a struct cmd_db_header then this
> > would be:
> >
> > ent = (struct entry_header *)db->data + rsc_hdr->header_offset;
> >
> > > +
> > > + for (j = 0; j < rsc_hdr->cnt; j++, ent++) {
> > > + if (use_addr) {
> > > + if (ent->addr == (u32)(query))
> > > + break;
> > > + } else if (ent->res_id == query)
> > > + break;
> > > + }
> > > +
> > > + if (j < rsc_hdr->cnt) {
> > > + memcpy(eh, ent, sizeof(*ent));
> > > + memcpy(rh, &cmd_db_header->header[i], sizeof(*rh));
> > > + return 0;
> >
> > This function really returns reference to data, length of the data and
> > version. How about just making this explicit, rather than copying two
> > sets of headers to the caller and having them extract this information?
> >
> > I think you should turn this function into:
> >
> > static void *cmd_db_find(u64 id, size_t *len, u16 *version)
> >
> I think this is much cleaner, to return the entry that we searched for,
> rather than return a void *.
>
It might be that I misunderstand the in-memory data structures. But as
it seems you really need to return both structs and then have the client
do math it seemed easier to just pick the values we know the accessors
are interested in and return those...
[..]
> > > +
> > > +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.
[..]
> > > +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.
> > > +
> > > +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"?
[..]
> > > + 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.
> > > +
> > > + 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()
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
