Re: [PATCH RFC 08/10] net: stmmac:sti: Add STi SOC glue driver.

[Maxime Ripard <maxime.ripard@free-electrons.com>]

[-- Attachment #1: Type: text/plain, Size: 3218 bytes --]

Hi Srinivas,

On Mon, Dec 02, 2013 at 12:48:24PM +0000, srinivas kandagatla wrote:
> Hi Maxime,
> 
> Thankyou for the comments.
> 
> On 29/11/13 19:37, Maxime Ripard wrote:
> >> +
> >> > +ethernet0: ethernet0{
> >> > +	#address-cells = <1>;
> >> > +	#size-cells = <1>;
> >> > +	compatible		= "st,stih415-dwmac";
> >> > +	reg			= <0x148 0x4>;
> >> > +	resets			= <&softreset STIH415_ETH0_SOFTRESET>;
> >> > +	st,syscon		= <&syscfg_rear>;
> >> > +	st,tx-retime-src	= "clk_125";
> >> > +	ranges;
> >> > +
> >> > +	dwmac0:dwmac@fe810000 {
> >> > +		device_type 	= "network";
> >> > +		compatible	= "snps,dwmac", "snps,dwmac-3.610";
> >> > +		reg 		= <0xfe810000 0x8000>;
> >> > +		interrupts 	= <0 147 0>;
> >> > +		interrupt-names = "macirq";
> >> > +		...
> >> > +	};
> >> > +};
> > Sorry for stepping up so late, but I dont' think this is the right way
> > to do it.
> > 
> Not a issue.
> 
> > DT is to describe how the hardware is laid out in a system agnostic
> > way, hence, it should not be impacted by the implementation details.
> > 
> If "hardware is laid out" means at SoC level? Then I attempted to do
> describe it in system agnostic way.
> 
> On ST SoCs "snps,dwmac" IP always has a hw glue layer on top of it.
> 
> > The fact that you use a glue to the dwmac driver *is* an
> > implementation detail.
> 
> Glue layer described here is actually a hardware glue on ST SoCs.

Ho, ok, it makes sense then :)

Sorry for the noise.

> > I think you'd rather have something like:
> > 
> > dwmac0: ethernet@fe810000 {
> > 	compatible		= "st,stih415-dwmac";
> > 	reg 			= <0xfe810000 0x8000 0x148 0x4>;
> > 	resets			= <&softreset STIH415_ETH0_SOFTRESET>;
> > 	st,syscon		= <&syscfg_rear>;
> > 	st,tx-retime-src	= "clk_125";
> > 	interrupts 		= <0 147 0>;
> > 	interrupt-names 	= "macirq";Am happy to go with 
> > 	...
> > };
> > 
> > Then, the driver can have its init functions associated to the
> > compatible you're using, through the .data field of the of_device_id
> > structure, and you just call it in the generic driver at probe's time.
> 
> This is changing the device tree bindings for the generic driver.
> Is this something Acceptable?

It really depends. As long as these are based on new compatibles, and
you require these new properties only for this new compatible, it's
fine most of the time.

> Peppe, are you Ok with such intrusive changes to the driver?
> 
> I did try few things before I sent this patch,
> 
> 1> Doing it via AUXDATA which was discouraged by Arnd.
> 
> 2> Doing it the way you suggested which did not fit in very neatly,
> which looked like lot of SOC specific changes are added to generic driver.
> 
> 3> Doing it as this patch, influenced by dwc3 code drivers/usb/dwc3/,
> Which fitted in very neatly without touching the existing generic driver.
> 
> stmmac driver is a generic synopsis driver which ST has written the
> driver, so I did not want to pollute this driver with ST specific glue
> logic bindings.

Yeah, I didn't know that. My apologies.

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]