| Message ID | 20170221215032.79282-2-cbostic@linux.vnet.ibm.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
Hi Chris, > @@ -252,7 +271,31 @@ static int fsi_slave_scan(struct fsi_slave *slave) > * Unused address areas are marked by a zero type value; this > * skips the defined address areas > */ > - if (type != 0 && slots != 0) { > + if (type != 0) { > + > + if (type == FSI_ENGID_HUB_MASTER) { I'd split this into a separate part of the above 'if' statement. Or, convert it to a switch. > + hub = kzalloc(sizeof(*hub), GFP_KERNEL); > + if (!hub) > + return -ENOMEM; > + > + hub->base = FSI_HUB_LINK_OFFSET; > + hub->control_regs = engine_addr; > + hub->slave = slave; > + rc = hub_master_init(hub); > + > + continue; > + } > + if (type == FSI_ENGID_HUB_LINK) { > + if (!hub) > + continue; > + > + hub->master.n_links++; > + if (hub->master.n_links == > + (FSI_HUB_MASTER_MAX_LINKS * 2)) > + rc = fsi_master_register(&hub->master); > + > + continue; > + } This logic doesn't make a lot of sense. You're counting the number of link slots, but then registering the master when that count hits a predefined number. I'd suggest either: - just setting n_links to the predefined number; or - keeping a separate variable for n_links, and then once we've finished scanning the configuration table (after the for loop) if (hub) { hub->n_links = conf_link_count / 2; fsi_master_register(&hub->master); } I'd much prefer the second option, as that actually uses the link count from the config table. > int fsi_master_register(struct fsi_master *master) > { > - if (!master || !master->dev) > + if (!master) > return -EINVAL; I'm worried if this is needed - we should be creating a device for all masters. Cheers, Jeremy
On 2/21/17 7:02 PM, Jeremy Kerr wrote: > Hi Chris, > >> @@ -252,7 +271,31 @@ static int fsi_slave_scan(struct fsi_slave *slave) >> * Unused address areas are marked by a zero type value; this >> * skips the defined address areas >> */ >> - if (type != 0 && slots != 0) { >> + if (type != 0) { >> + >> + if (type == FSI_ENGID_HUB_MASTER) { > I'd split this into a separate part of the above 'if' statement. Or, > convert it to a switch. Hi Jeremy, Will change. >> + hub = kzalloc(sizeof(*hub), GFP_KERNEL); >> + if (!hub) >> + return -ENOMEM; >> + >> + hub->base = FSI_HUB_LINK_OFFSET; >> + hub->control_regs = engine_addr; >> + hub->slave = slave; >> + rc = hub_master_init(hub); >> + >> + continue; >> + } >> + if (type == FSI_ENGID_HUB_LINK) { >> + if (!hub) >> + continue; >> + >> + hub->master.n_links++; >> + if (hub->master.n_links == >> + (FSI_HUB_MASTER_MAX_LINKS * 2)) >> + rc = fsi_master_register(&hub->master); >> + >> + continue; >> + } > This logic doesn't make a lot of sense. You're counting the number of > link slots, but then registering the master when that count hits a > predefined number. I'd suggest either: > > - just setting n_links to the predefined number; or > > - keeping a separate variable for n_links, and then once we've finished > scanning the configuration table (after the for loop) > > if (hub) { > hub->n_links = conf_link_count / 2; > fsi_master_register(&hub->master); > } > > I'd much prefer the second option, as that actually uses the link count > from the config table. Will implement the second option >> int fsi_master_register(struct fsi_master *master) >> { >> - if (!master || !master->dev) >> + if (!master) >> return -EINVAL; > I'm worried if this is needed - we should be creating a device for all > masters. Will add a dev for struct fsi_master. Thanks > Cheers, > > > Jeremy >
diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c index d63a892..646bb43 100644 --- a/drivers/fsi/fsi-core.c +++ b/drivers/fsi/fsi-core.c @@ -43,6 +43,11 @@ #define FSI_IPOLL_PERIOD msecs_to_jiffies(fsi_ipoll_period_ms) +#define FSI_ENGID_HUB_MASTER 0x1c +#define FSI_ENGID_HUB_LINK 0x1d +#define FSI_HUB_LINK_OFFSET 0x80000 +#define FSI_HUB_MASTER_MAX_LINKS 8 + static const int engine_page_size = 0x400; static struct task_struct *master_ipoll; static unsigned int fsi_ipoll_period_ms = 100; @@ -58,6 +63,14 @@ struct fsi_slave { uint8_t id; }; +struct fsi_master_hub { + struct fsi_master master; + struct fsi_slave *slave; + uint32_t control_regs; /* slave-relative addr regs */ + uint32_t base; /* slave-relative addr of */ + /* master address space */ +}; + #define to_fsi_slave(d) container_of(d, struct fsi_slave, dev) static int fsi_slave_read(struct fsi_slave *slave, uint32_t addr, @@ -203,12 +216,18 @@ static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr, slave->id, addr, val, size); } +static int hub_master_init(struct fsi_master_hub *hub) +{ + return 0; +} + static int fsi_slave_scan(struct fsi_slave *slave) { uint32_t engine_addr; uint32_t conf; int rc, i; uint8_t si1s_bit = 1; + struct fsi_master_hub *hub; INIT_LIST_HEAD(&slave->my_engines); @@ -252,7 +271,31 @@ static int fsi_slave_scan(struct fsi_slave *slave) * Unused address areas are marked by a zero type value; this * skips the defined address areas */ - if (type != 0 && slots != 0) { + if (type != 0) { + + if (type == FSI_ENGID_HUB_MASTER) { + hub = kzalloc(sizeof(*hub), GFP_KERNEL); + if (!hub) + return -ENOMEM; + + hub->base = FSI_HUB_LINK_OFFSET; + hub->control_regs = engine_addr; + hub->slave = slave; + rc = hub_master_init(hub); + + continue; + } + if (type == FSI_ENGID_HUB_LINK) { + if (!hub) + continue; + + hub->master.n_links++; + if (hub->master.n_links == + (FSI_HUB_MASTER_MAX_LINKS * 2)) + rc = fsi_master_register(&hub->master); + + continue; + } /* create device */ dev = fsi_create_device(slave); @@ -597,7 +640,7 @@ DEVICE_ATTR(fsi_ipoll_period, S_IRUGO | S_IWUSR, fsi_ipoll_period_show, int fsi_master_register(struct fsi_master *master) { - if (!master || !master->dev) + if (!master) return -EINVAL; master->idx = ida_simple_get(&master_ida, 0, INT_MAX, GFP_KERNEL);
On detect of hub master engine in config space set up a master device and register it with the core. Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com> --- Changes in V2: - Refactor structures for hub master. Previously hub master engine was considered a slave engine. --- drivers/fsi/fsi-core.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 45 insertions(+), 2 deletions(-)