Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface

[Jolly Shah]

Hi Sudeep,

ïOn 1/24/20, 3:32 AM, "Sudeep Holla" <sudeep.holla@xxxxxxx> wrote:

    Hi Greg,
    
    Thanks for raising various issues that I have repeatedly asked and
    constantly ignored.

Sorry, never intended to ignore you. We agreed to your comments for reg access and that patch is already removed. GGS and PGGS registers are for general purpose registers(set of 4) for users. Since this registers belongs to PMU register space, interface is provided to read or write the way user wants. 
    
    On Fri, Jan 24, 2020 at 07:03:39AM +0100, Greg KH wrote:
    > On Thu, Jan 23, 2020 at 11:47:57PM +0000, Jolly Shah wrote:
    > > Hi Greg,
    > >
    
    [...]
    
    > > Firmware driver was changed later to be platform driver but these
    > > interfaces were defined earlier and are in use.
    >
    > Defined and in use where?  Not in the upstream kernel tree, right?  Or
    > am I missing them somewhere else?
    >
    
    Exactly and they keep saying there partners are using these for 3-4 years
    and they want to retain that. I have told them to change several times.

Sorry we might have missed your comments for this change. We agree to your and greg's comment and will update sysfs path.


    > > > > +	ret = kstrtol(tok, 16, &value);
    > > > > +	if (ret) {
    > > > > +		ret = -EFAULT;
    > > > > +		goto err;
    > > > > +	}
    > > > > +
    > > > > +	ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
    > > >
    > > > This feels "tricky", if you tie this to the device you have your driver
    > > > bound to, will this make it easier instead of having to go through the
    > > > ioctl callback?
    > > >
    > >
    > > GGS(general global storage) registers are in PMU space and linux doesn't have access to it
    > > Hence ioctl is used.
    >
    > Why not just a "real" call to the driver to make this type of reading?
    > You don't have ioctls within the kernel for other drivers to call,
    > that's not needed at all.
    >
    
    Oh yes, this is so broken in design. This firmware is designed to abstract
    the power and configuration management on their platform, but they decided
    to add direct register access to some registers anyway. Weird use case,
    don't even ask. But I strongly objected such interface in sysfs even if
    they moved under platform device. If they need this at any cost, I have
    suggested debugfs.
    
As mentioned, these registers are for users  and for special needs where users wants to retain values over resets. but as they belong to PMU address space,     these interface APIs are provided. They donât allow access to any other registers.


    [...]
    
    > >
    > > Agree it will be simpler but to as firmware driver was changed to be
    > > platform driver, to keep paths same, we used sysfs.
    >
    > Keep them same from what?  Use the platform device as that is what it is
    > there for, do not go create new apis when they are not needed at all.
    >
    
    +1, and please don't add any sysfs that can read/write those GGS registers
    directly from userspace. Move them to debugfs if you are desperate to have
    something.


Thanks,
Jolly Shah

    
    --
    Regards,
    Sudeep